612455 - History.cpp should finalize statementCache on the background thread

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Title says it all, we could use the proxy runnable I added to Helpers.h in bug 599973

Comment 1

[Shawn Wilsher :sdwilsh]

Correctness fix that is important, but not too important.

Comment 2

[Shawn Wilsher :sdwilsh]

Need this before we merge.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 4

[Marco Bonardo [:mak]]

2 things we should figure out:

FinalizeStatementCacheProxy uses references, if we call it in destructor it's possible it will end up working on null and iirc History could do that. FaviconService shutdown is called by history thus there it's not a problem.
We probably need to build a new cache and migrate statements to it (a special copy contructor?) so we destroy an empty entity and we can finalize with all quiet.

StatementCache seem to allow asking for more statements after it has been finalized... well, is it possible we finalize the cache at places-shutdown when there is still some runnable queued up to be executed? then it would create never finalized statements. Maybe the cache should return null after it has been finalized?

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> FinalizeStatementCacheProxy uses references, if we call it in destructor it's
> possible it will end up working on null and iirc History could do that.
> FaviconService shutdown is called by history thus there it's not a problem.
> We probably need to build a new cache and migrate statements to it (a special
> copy contructor?) so we destroy an empty entity and we can finalize with all
> quiet.
Naw, we can just tack on strong reference to the object that owns it and we don't have to worry about it.

> StatementCache seem to allow asking for more statements after it has been
> finalized... well, is it possible we finalize the cache at places-shutdown when
> there is still some runnable queued up to be executed? then it would create
> never finalized statements. Maybe the cache should return null after it has
> been finalized?
It's not an issue if there are still runnables that are queued.  This is added at the end of the queue, and then we set the shutting down flag which means we won't add anything else to the queue.

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> It's not an issue if there are still runnables that are queued.  This is added
> at the end of the queue, and then we set the shutting down flag which means we
> won't add anything else to the queue.

to sum up irc discussion, favicons are more complicated and involve more runnables that are queued up later. thus this won't work for them.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

per comments, + more correct

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 491385 [details] [diff] [review]
v1.1

omponents/places/src/History.cpp
>+++ b/toolkit/components/places/src/History.cpp
>@@ -863,10 +863,6 @@ History::~History()
>                  "Not all Links were removed before we disappear!");
>   }
> #endif
>-
>-  // Places shutdown event may not occur, but we *must* clean up before History
>-  // goes away.
>-  Shutdown();
This is safe because we used to have to tear down a hashtable.  We don't do that anymore, so we don't need to call shutdown in our destructor (which we don't want to do anyway).

>+++ b/toolkit/components/places/src/nsFaviconService.cpp
>@@ -942,7 +942,7 @@ nsFaviconService::FinalizeStatements() {
> 
>   // Finalize the statementCache on the correct thread.
>   nsRefPtr<FinalizeStatementCacheProxy<mozIStorageStatement> > event =
>-    new FinalizeStatementCacheProxy<mozIStorageStatement>(mSyncStatements);
>+    new FinalizeStatementCacheProxy<mozIStorageStatement>(mSyncStatements, this);
This is safe because we'll never call this method in our destructor.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.1 with commit message

With commit message.  Can land!  Yay!

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/af7e6f7cbf16

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/af7e6f7cbf16