744115 - Events dispatched to a thread too close to shutdown can get leaked

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ali Juma [:ajuma]]

Dispatching events to the main thread using nsThread::Dispatch can cause these events to be leaked if the dispatch happens close to shutdown, triggering the assertion:

###!!! ASSERTION: Non-empty event queue being destroyed; events being leaked.: 'IsEmpty()', file /firefox/mozilla-mac/xpcom/threads/nsEventQueue.cpp, line 64

See Attachment 613341 [details] [diff], Bug 741319, where we work around this problem by making a call to NS_GetMainThread, and only dispatch if this call succeeds. This workaround relies on the observation that dispatching events using NS_DispatchToMainThread (whose implementation aborts the dispatch when NS_GetMainThread fails) doesn't trigger the leak.

(Note that in Bug 741319, we couldn't simply use NS_DispatchToMainThread since we're not always dispatching to the main thread.)

Comment 1

[Benjamin Smedberg]

It is illegal to try and dispatch events after the xpcom-shutdown-threads notification, so you are doing something *way* late when you should be cleaning up earlier. See https://wiki.mozilla.org/XPCOM_Shutdown for the shutdown sequence. There really isn't a core bug here, but I'd happily accept an assertion which tries to detect this errant case.

Comment 2

[Josh Matthews [:jdm]]

Comment 1 sounds like a nice small task for someone with C++ experience. Similar to bug 683517, we'll want to create a class that implements nsIObserver and watches for the xpcom-shutdown-threads notification. The observer can store a flag that indicates whether it's still valid to dispatch events, and nsEventQueue::PutEvent (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsEventQueue.cpp#104) can assert (using MOZ_ASSERT) that this flag is true.

Comment 3

[Benjamin Smedberg]

I don't think it needs to be complicated: the check can be accomplished in nsThread::Dispatch and it can check a global state flag from here: http://hg.mozilla.org/mozilla-central/annotate/e636439e342f/xpcom/build/nsXPComInit.cpp#l619

Comment 4

[Josh Matthews [:jdm]]

True. Comment 2 can be ignored; comment 3's proposed solution is much easier.

Comment 5

[Marc Jessome[:mjessome]]

Attachment: Disallow dispatching events after threads shut down - v1

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 619720 [details] [diff] [review]
Disallow dispatching events after threads shut down - v1

This fell through the cracks.

Comment 7

[Marc Jessome[:mjessome]]

Comment on attachment 619720 [details] [diff] [review]
Disallow dispatching events after threads shut down - v1

Cancelling review, this was never run on Try until now: https://tbpl.mozilla.org/?tree=Try&rev=745f42d31a87

Comment 8

[Josh Matthews [:jdm]]

Resetting the owner of this task that hasn't seen any activity in a while.

Comment 9

[Arnaud Sourioux [:Six]]

Attachment: Disallow dispatching events after threads shut down

This is the same patch as proposed by mjessome but with last m-c version
build correctly on linux64
i will push it on tpbl if review is ok.

Comment 10

[Benjamin Smedberg]

Comment on attachment 764690 [details] [diff] [review]
Disallow dispatching events after threads shut down

By itself I don't think this will work: it will disallow events being dispatched to the main thread, which is something that can still happen and may be necessary for clean shutdown. This flag should really only affect events being dispatched to other threads.

Comment 11

[Arnaud Sourioux [:Six]]

Ok thanks for the heads up
i was just aplying the old patch on the latest m-c revision
if you can give me hints on which classes should be modified i will try to do it correctly ;)

Comment 12

[Josh Matthews [:jdm]]

You'll need to check NS_IsMainThread() to address Benjamin's comments.

Comment 13

[Arnaud Sourioux [:Six]]

So here:
https://bugzilla.mozilla.org/attachment.cgi?id=764690&action=diff#a/xpcom/threads/nsThread.cpp_sec3

i should have:
> if (gXPCOMThreadsShutDown && !NS_IsMainThread()) {
>    return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
> }

is this correct or i'm missing something?

Comment 14

[Benjamin Smedberg]

well... that will check whether the *current* thread is the main thread, but I think you probably want to check whether the *target* thread is the main thread, which I think you could just check with mIsMainThread.

Comment 15

[Arnaud Sourioux [:Six]]

Attachment: Disallow dispatching events after threads shut down

new version with your comments

Comment 16

[Arnaud Sourioux [:Six]]

I remove checkin-needed
i'm gonna give a push to try with tests tomorrow

Comment 17

[Arnaud Sourioux [:Six]]

Tbpl is here: https://tbpl.mozilla.org/?tree=Try&rev=16d7f28ae034
looks ok
so checkin should be good

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c69ab8cdad98

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c69ab8cdad98

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: backout on aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): The change in this bug caused bug 938107; shutdown hangs when exiting when playing audio and video.
User impact if declined: shutdown hangs when exiting Firefox while playing audio and video.
Testing completed (on m-c, etc.): We ran without this change for years, and the media code didn't hang on shutdown.
Risk to taking this patch (and alternatives if risky): lowish. In bug 938107 comment 23, bsmedberg says this patch isn't essential.
String or IDL/UUID changes made by this patch: None.

Comment 21

[bhavana bajaj [:bajaj]]

Comment on attachment 8343256 [details] [diff] [review]
Patch: backout on aurora

Approving to help with https://bugzilla.mozilla.org/show_bug.cgi?id=938107

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

This patch is approved for aurora by bbajaj, and needs landing.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/59b8da3b00a8