Closed Bug 721603 Opened 8 years ago Closed 8 years ago

We should spin the loop after calling asyncClose in Database.cpp

Categories

(Toolkit :: Places, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 3 obsolete files)

We close the database as part of the shutdown process, but currently nothing waits for the tasks to complete.
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
Attachment #594174 - Flags: review?(mak77)
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.
Attachment #597122 - Flags: review?(mak77) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/888416bbd8f3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 728653
This got reverted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #601935 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/aa8798ce6416
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.