bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Shutdown jump list thread correctly to avoid debug assertions

RESOLVED FIXED in mozilla32



Widget: Win32
4 years ago
4 years ago


(Reporter: bbondy, Assigned: bbondy)


Windows 8.1

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

3.48 KB, patch
: review+
Ben Turner (not reading bugmail, use the needinfo flag!)
: feedback+
Details | Diff | Splinter Review


4 years ago
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

4 years ago
Created attachment 8432105 [details] [diff] [review]

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.

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)
Attachment #8432105 - Flags: review?(jmathies)
Comment on attachment 8432105 [details] [diff] [review]

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

4 years ago
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.


4 years ago
Attachment #8432105 - Flags: review?(jmathies) → review+
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

4 years ago
Created attachment 8432849 [details] [diff] [review]

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.
Attachment #8432105 - Attachment is obsolete: true
Attachment #8432849 - Flags: review+
Attachment #8432849 - Flags: feedback?(bent.mozilla)
Comment on attachment 8432849 [details] [diff] [review]

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).
Attachment #8432849 - Flags: feedback?(bent.mozilla) → feedback+
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.