Closed Bug 744115 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox27 --- disabled

People

(Reporter: ajuma, Assigned: Six)

References

Details

(Whiteboard: [mentor=jdm][lang=c++])

Attachments

(2 files, 2 obsolete files)

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.)
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 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.
Whiteboard: [mentor=jdm][lang=c++]
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
True. Comment 2 can be ignored; comment 3's proposed solution is much easier.
Assignee: nobody → marc.jessome
Comment on attachment 619720 [details] [diff] [review]
Disallow dispatching events after threads shut down - v1

This fell through the cracks.
Attachment #619720 - Flags: review?(benjamin)
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
Attachment #619720 - Flags: review?(benjamin)
Resetting the owner of this task that hasn't seen any activity in a while.
Assignee: marc.jessome → nobody
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.
Assignee: nobody → six.dsn
Attachment #619720 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #764690 - Flags: review?(benjamin)
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.
Attachment #764690 - Flags: review?(benjamin) → review-
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 ;)
You'll need to check NS_IsMainThread() to address Benjamin's comments.
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?
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.
new version with your comments
Attachment #764690 - Attachment is obsolete: true
Attachment #765479 - Flags: review?(benjamin)
Attachment #765479 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
I remove checkin-needed
i'm gonna give a push to try with tests tomorrow
Keywords: checkin-needed
Tbpl is here: https://tbpl.mozilla.org/?tree=Try&rev=16d7f28ae034
looks ok
so checkin should be good
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c69ab8cdad98
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 938107
[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.
Attachment #8343256 - Flags: approval-mozilla-aurora?
Comment on attachment 8343256 [details] [diff] [review]
Patch: backout on aurora

Approving to help with https://bugzilla.mozilla.org/show_bug.cgi?id=938107
Attachment #8343256 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This patch is approved for aurora by bbajaj, and needs landing.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.