We close the database as part of the shutdown process, but currently nothing waits for the tasks to complete.

An initial try push is at https://tbpl.mozilla.org/?tree=Try&rev=ff40f32fedfe

A new try push with a fix for 721812 included is in https://tbpl.mozilla.org/?tree=Try&rev=d24d584cfce4

I simplified the spinner code a bit, rebased and pushed to https://tbpl.mozilla.org/?tree=Try&rev=02692c9ed2b9

Comment on attachment 594174 [details] [diff] [review] We should spin the loop after calling asyncClose in Database.cpp Review of attachment 594174 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Database.cpp @@ +287,5 @@ > + nsCOMPtr<nsIThread> thread = do_GetCurrentThread(); > + while (!mDone) { > + NS_ProcessNextEvent(thread); > + } > +} So, initially PlacesEvent was just an nsIRunnable. I added NS_DECL_MOZISTORAGECOMPLETIONCALLBACK to it cause it was the smaller approach. Though actually the only use of it is for asyncClose, and here you are practically replacing its only use. Thus I'd prefer if you remove NS_DECL_MOZISTORAGECOMPLETIONCALLBACK and Complete from PlacesEvent and in Helpers.h/cpp add your own separate NS_DECL_MOZISTORAGECOMPLETIONCALLBACK class. In the end should be clearer. Let's call it BlockingConnectionCloseCallback or something like that

Comment on attachment 597122 [details] [diff] [review] now with a fixed assert Review of attachment 597122 [details] [diff] [review]: ----------------------------------------------------------------- just a couple things, but looks fine ::: toolkit/components/places/Database.cpp @@ +260,5 @@ > + BlockingConnectionCloseCallback(); > + void Spin(); > +}; > + > +NS_IMETHODIMP I think we use NS_IMETHOD for inline definitions @@ +268,5 @@ > + return NS_OK; > +} > + > +BlockingConnectionCloseCallback::BlockingConnectionCloseCallback() : > + mDone(false) per module code style, please put the colon here like Class:Class(a, b) : mProperty(a) , mPRoperty2(b) { ... @@ +270,5 @@ > + > +BlockingConnectionCloseCallback::BlockingConnectionCloseCallback() : > + mDone(false) > +{ > +} please MOZ_ASSERT(NS_IsMainThread()); in the constructor. @@ +1714,3 @@ > new PlacesEvent(TOPIC_PLACES_CONNECTION_CLOSED); > + DebugOnly<nsresult> rv = NS_DispatchToMainThread(closeEvent); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); we should just notify it synchronously with NotifyObservers() in ::Complete() at this point, otherwise we are just enqueueing it to fire after xpcom-shutdown, not much useful.

I did a try push with the requested changes to https://tbpl.mozilla.org/?tree=Try&rev=500317f901bb I will push to m-i if the try is green.

Could you please add an "if (os) {" condition after nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); just to be paranoid. should not change the result of your try testing fwiw.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=888416bbd8f3 As discussed on IRC I pushed with the compromise: 1.27 + MOZ_ASSERT(os); 1.28 + if (!os) 1.29 + return NS_OK; If find our uses of "if (os)" a bit disturbing. For them to be correct, the receiving side must be designed under the assumption that the messages are optional.

This got reverted.