1018598 - Shutdown jump list thread correctly to avoid debug assertions

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Brian R. Bondy [:bbondy]]

If the jump list enabled pref changes, and you're using a debug build, there will be a crash on shutdown.  This will also happen if you're generating a jump list and icons need to be generated.

Win debug talos tests never seen this before though because no thread was spawned. Bug 724423 made it so there is a thread spawned during some tests, causing this assertion.

I'm also going to add a couple bugs to cover testing the pref change code which deletes all favicons and to make sure we cover jump list icon generation in tests.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: bug1018598_shutdownjumplist.diff

The problem is that we handle the profile-before-change observed event inside WindowsJumpLists.jsm.  And then we remove our reference from the xpcom object.

But the xpcom object isn't freed right there.  It looks like it is, but it's not.
http://dxr.mozilla.org/mozilla-central/source/browser/modules/WindowsJumpLists.jsm?from=WindowsJumpLists.jsm#538

So then an assertion happens on thread teardown before we even get to the C++ Jump list builder destructor.

To fix this we should just tear down the thread via the c++ code by observing the event there.  

I verified and this gets rid of the assertion.


To reproduce the assertion you can just:
- Open your browser with a debug build.
- Toggle the taskbar list enabled pref
- Close the browser
- *Crash* (debug assertion)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8432105 [details] [diff] [review]
bug1018598_shutdownjumplist.diff

Review of attachment 8432105 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/JumpListBuilder.cpp
@@ +70,5 @@
>  {
> +  nsCOMPtr<nsIObserverService> observerService =
> +    do_GetService("@mozilla.org/observer-service;1");
> +  if (observerService) {
> +    observerService->RemoveObserver(this, TOPIC_PROFILE_BEFORE_CHANGE);

Sorry, just driving by, but this is impossible. As you're currently telling the observer service to hold a strong reference to this object it can never be destroyed while the object is still registered.

Comment 3

[Brian R. Bondy [:bbondy]]

Thanks Bent, I'll take a look tonight, I know the sutdown assertion isn't happening anymore and the thread shutdown is being called. But I'll put a breakpoint in the destructor and see if that's being hit.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, the rest of the patch (including the thread shutdown) looks fine, it's just that your destructor can't be called if the object is still registered with the observer service.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: bug1018598_shutdownjumplist.diff

So strangely enough the destructor is being called, but the observer service Get returns nullptr.  I guess the reference is released when the observer service is destroyed? 

I moved the call to remove the observer inside the observer notification.

Carrying forward r+ from jimm but I'll wait for feedback+ on the new patch.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8432849 [details] [diff] [review]
bug1018598_shutdownjumplist.diff

Yep, you're exactly right. Observer service releases everything at shutdown (which is nice but makes leak-until-the-app-shuts-down bugs hard to see).

Comment 7

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45a3e5d2b169

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45a3e5d2b169