Created attachment 214559 [details] [review]
libtracker-miner: Avoid double unref in TrackerFileSystem

tracker_file_system_delete_files() needs to explicitly remove the files
from the tree, rather than waiting for the last ref to disappear to do
so. There are multiple locations that call this function, so if the
objects aren't removed from the tree immediately (which they will not be
if there are refs on them held by item queues, etc.) we risk removing
extra references.

Fixes GB#676489.

Comment on attachment 214559 [details] [review]
libtracker-miner: Avoid double unref in TrackerFileSystem

>@@ -75,15 +72,7 @@ file_node_data_free (FileNodeData *data,
> {
> 	guint i;
> 
>-	if (data->file) {
>-		if (!data->shallow) {
>-			g_object_weak_unref (G_OBJECT (data->file),
>-			                     file_weak_ref_notify,
>-			                     node);
>-		}
>-
>-		g_object_unref (data->file);
>-	}
>+	g_object_unref (data->file);

Is there a chance data->file could already be NULL?
I am always nervous about g_object_unref() calls without checking the pointer first.

>@@ -122,9 +111,6 @@ file_node_data_new (TrackerFileSystem *file_system,
> 	data->file_type = file_type;
> 	data->properties = g_array_new (FALSE, TRUE, sizeof (FileNodeProperty));
> 
>-	/* We use weak refs to keep track of files */
>-	g_object_weak_ref (G_OBJECT (data->file), file_weak_ref_notify, node);
>-
> 	node_data = g_object_get_qdata (G_OBJECT (data->file),
> 	                                quark_file_node);
> 
>@@ -401,26 +387,6 @@ reparent_child_nodes_to_parent (GNode *node)
> 	}
> }
> 
>-static void
>-file_weak_ref_notify (gpointer  user_data,
>-                      GObject  *prev_location)
>-{
>-	FileNodeData *data;
>-	GNode *node;
>-
>-	node = user_data;
>-	data = node->data;
>-
>-	g_assert (data->file == (GFile *) prev_location);
>-
>-	data->file = NULL;
>-	reparent_child_nodes_to_parent (node);
>-
>-	/* Delete node tree here */
>-	file_node_data_free (data, NULL);
>-	g_node_destroy (node);
>-}
>-
> static GNode *
> file_system_get_node (TrackerFileSystem *file_system,
>                       GFile             *file)
>@@ -834,12 +800,28 @@ append_deleted_files (GNode    *node,
> 
> 	if (data->file_type == G_FILE_TYPE_UNKNOWN ||
> 	    node_data->file_type == data->file_type) {
>-		data->list = g_list_prepend (data->list, node_data->file);
>+		data->list = g_list_prepend (data->list, node);
> 	}
> 
> 	return FALSE;
> }
> 
>+static void remove_from_tree (gpointer data,
>+                              gpointer user_data)

Coding style ... function name on separate line.

The patch looks sane to me. Carlos, any comments (this is firmly in your realm) :)

Hmm, I think this patch breaks the assumption in file_notifier_traverse_tree(), where non-directory GFiles are disposed, in the expectation that those files are already queued and referenced, and the associated data in the TrackerFileSystem is kept alive with them. Actually... the underlying problem rather sounds to me like a reference counting error on the canonical GFile returned by the TrackerFileSystem, particularly calling delete_files() multiple times on the same tree sounds icky...

tracker_file_system_delete_files() is called in a few places:
  * in file_notifier_traverse_tree(), performing the optimisation you mention of leaving only directories in tree once crawling is completed
  * in monitor_item_deleted_cb(), ... which now I think about it ... why?
  * in monitor_item_moved_cb() ... again, why?

So it sounds like the real error is that we should only ever call delete_files() once, and on deletion we should remove things from the TrackerFileSystem using a different method that gets rid of the directories too.

Still, I'm not sure I like the idea of files being removed from the TrackerFileSystem tree when the last reference is dropped - since we ref GFiles in the extraction queue, if they are mount points, etc. surely this means they'll hang around in the tree longer than necessary, when we could just remove them explicitly? It seems kind of "magic" that g_object_unref() would have the side effect of removing a file from the tree. Or this that just an artifact of tracker-file-system-test and not how things actually work now?

Either way, I guess the correct fix is to remove deleted files and directories from the tree using whatever is the correct method, as tracker_file_system_delete_files() is definitely not what should be used (and it has kind of a confusing name :)

(In reply to comment #4)
> tracker_file_system_delete_files() is called in a few places:
>   * in file_notifier_traverse_tree(), performing the optimisation you mention
> of leaving only directories in tree once crawling is completed
>   * in monitor_item_deleted_cb(), ... which now I think about it ... why?
>   * in monitor_item_moved_cb() ... again, why?
> 
> So it sounds like the real error is that we should only ever call
> delete_files() once, and on deletion we should remove things from the
> TrackerFileSystem using a different method that gets rid of the directories
> too.
> 
> Still, I'm not sure I like the idea of files being removed from the
> TrackerFileSystem tree when the last reference is dropped - since we ref GFiles
> in the extraction queue, if they are mount points, etc. surely this means
> they'll hang around in the tree longer than necessary, when we could just
> remove them explicitly? It seems kind of "magic" that g_object_unref() would
> have the side effect of removing a file from the tree. Or this that just an
> artifact of tracker-file-system-test and not how things actually work now?

That's how things work now, think of TrackerFileSystem as a GFile cache, so multiple requests pointing to a single file would always return the same single GFile pointer, and that long lived object would also hold the extra data attached to it, so the usual way to operate with that would be:

GFile *file, *cached;

file = g_file_new_for_uri (...)
cached = tracker_file_system_get_file (file_system, file, ...);
g_object_unref (file);

/* operate on cached file */
g_object_ref (cached);
...

/* unref file to express disinterest to it, if it the last/only
 * reference, it's removed from the TrackerFileSystem too 
 */
g_object_unref (cached);

The ref/unref pair on the cached file would be optional, but if some operation is to delete the file, it needs to do one (extra) unref so ref/unref pairs in other parts of the code may keep the file alive, and it's still eventually destroyed when those are finished.

> 
> Either way, I guess the correct fix is to remove deleted files and directories
> from the tree using whatever is the correct method, as
> tracker_file_system_delete_files() is definitely not what should be used (and
> it has kind of a confusing name :)

Yeah, agreed, that call was added in a later stage, maybe _forget_files() would have been a bit more descriptive

Having thought about this a bit more, I think I was correct originally that the files should be explicitly removed from the cache tree when tracker_file_system_delete_files() is called. The whole point of calling that function is that we no longer need the non-directory GFile objects, after all - surely we can assume anyone who wants them already has a pointer and a reference at this point?

It seems to me that with the current method, we'll always be at risk of unreffing a GFile more than once, because tracker_file_system_delete_files() can be called each time we complete crawling a directory and it is possible (however unlikely) that beneath that directory in the cache will be a file that was unreffed during a previous call to tracker_file_system_delete_files() but kept alive and in the tree by other references.

(In reply to comment #6)
> Having thought about this a bit more, I think I was correct originally that the
> files should be explicitly removed from the cache tree when
> tracker_file_system_delete_files() is called. The whole point of calling that
> function is that we no longer need the non-directory GFile objects, after all -
> surely we can assume anyone who wants them already has a pointer and a
> reference at this point?

That makes sense to me.
 
> It seems to me that with the current method, we'll always be at risk of
> unreffing a GFile more than once, because tracker_file_system_delete_files()
> can be called each time we complete crawling a directory and it is possible
> (however unlikely) that beneath that directory in the cache will be a file that
> was unreffed during a previous call to tracker_file_system_delete_files() but
> kept alive and in the tree by other references.

Hmm, it looks like we should do 2 things here ...

1. Rename the function (as Carlos says), that way the purpose is clearer.
2. Decide if _delete_files() should remove the data in ALL cases regardless of the weak reference count. It seems to me the weak reference here can cause us problems. If others ref the GFile, we don't get the notification to remove the file from the GNode tree. So we could end up with a situation where we have an old GFile pointer and the GNode exists in tact AFAICS. Carlos am I wrong?

(In reply to comment #6)
> > Having thought about this a bit more, I think I was correct originally that the
> > files should be explicitly removed from the cache tree when
> > tracker_file_system_delete_files() is called. The whole point of calling that
> > function is that we no longer need the non-directory GFile objects, after all -
> > surely we can assume anyone who wants them already has a pointer and a
> > reference at this point?

I'm still concerned about the tracker_file_system_delete_files() call in tracker_file_notifier_traverse_tree(), that call is merely an early disposal of non-directory GFiles that are already in the processing queues, but at the time those are processed, the TrackerFileSystem will still be asked for their IRI and other data stored there. If tracker_file_system_delete_files() effectively removes them from the tree, that data will be refetched file by file, affecting performance.

(In reply to comment #7)
> Hmm, it looks like we should do 2 things here ...
> 
> 1. Rename the function (as Carlos says), that way the purpose is clearer.
> 2. Decide if _delete_files() should remove the data in ALL cases regardless of
> the weak reference count. It seems to me the weak reference here can cause us
> problems. If others ref the GFile, we don't get the notification to remove the
> file from the GNode tree. So we could end up with a situation where we have an
> old GFile pointer and the GNode exists in tact AFAICS. Carlos am I wrong?

Well... the file would still have a GNode as long as the GFile object is valid, that may of course be leaked :). If the only problem here is reentrancy of tracker_file_system_delete_files(), It'd perhaps be better to have a

guint already_deleted : 1

flag in FileNodeData, and check/set that before unref'ing files there.

I like the idea of a flag. We already have some so it's no extra memory.

Although a file's IRI is queried after the files have been removed from the cache, it's only done by the item processing queue so there are no calls to tracker_file_system_get_file(), just to tracker_file_system_get_property(). So the other option would be to remove the files from the tree on forget_files(), but keep their properties in the hash table until the reference count on the GFile drops to zero.

(Out of interest, what's the reason for using a separate hash table for properties instead of qdata on the object? Would be worth putting this in a comment I think)

anyway the flag seems simpler so I'll do it that way.

Created attachment 216313 [details] [review]
libtracker-miner: Prevent double unref of cached GFile objects

The reference owned by the TrackerFileSystem cache may be removed
more than once. Add a flag to ensure that the cache will only ever
unref it once, even if it is being kept alive in the cache by
references held elsewhere in the code.

tracker_file_system_delete_files() is renamed to
tracker_file_system_forget_files() to avoid confusion.

Fixes GB#676849

Comment on attachment 216313 [details] [review]
libtracker-miner: Prevent double unref of cached GFile objects

I think the patch looks good :), as this api is private it can go to tracker-0.14 as well

Done, thanks!

*** Bug 675786 has been marked as a duplicate of this bug. ***