Closed
Bug 1200514
Opened 9 years ago
Closed 9 years ago
protect against leaks from CycleCollectedJSRuntime::RunInStableState() late in shutdown
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: karlt, Assigned: cpearce)
References
Details
(Keywords: regression, Whiteboard: [MemShrink:P2])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mccr8
:
review+
froydnj
:
review+
|
Details | Diff | Splinter Review |
CycleCollectedJSRuntime::RunInStableState() infallibly appends to mStableStateEvents a strong reference to the runnable. After the last AfterProcessTask() call, mStableStateEvents will not be run until the CycleCollectedJSRuntime is destroyed. CycleCollectedJSRuntime is owned by XPCJSRuntime by nsXPConnect released during nsComponentManagerImpl::Shutdown(), which is after the last cycle collection. In order to avoid leaks, RunInStableState() needs to fail if invoked after the point at which the runnable will not be run before the last cycle collection.
Reporter | ||
Updated•9 years ago
|
Keywords: regression
Do you think processing the queue immediately before the final cycle collection would be sufficient?
Flags: needinfo?(karlt)
Reporter | ||
Comment 2•9 years ago
|
||
Processing the queue immediately before the final cycle collection would not deal with the situation of a RunInStableState call during the final cycle collection. which was essentially the situation in https://bugzilla.mozilla.org/show_bug.cgi?id=1193922#c8 I don't see anything inherently "wrong" in trying RunInStableState() from a destructor provided failure is handled appropriately.
Flags: needinfo?(karlt)
I would prefer that we make keep it infallible and just run event processing whenever we need to during shutdown.
Reporter | ||
Comment 4•9 years ago
|
||
Running events synchronously might be an option. Perhaps that could be problematic - I'm not sure. Async "whenever we need to" would require another run of cycle collection after each event processing stage needed. I guess that's OK in debug builds.
Reporter | ||
Updated•9 years ago
|
Whiteboard: [MemShrink]
Updated•9 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 5•9 years ago
|
||
I am also hitting this with https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c6c5c4fba08&selectedJob=15431342 Running runnables passed to CycleCollectedJSRuntime::RunInStableState() immediately/synchronously if XPCOM shutdown has already started seems to work...
Assignee | ||
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a939bbcc4a2
Assignee | ||
Comment 7•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31491/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31491/
Attachment #8709611 -
Flags: review?(khuey)
Assignee | ||
Comment 8•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31493/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31493/
Attachment #8709612 -
Flags: review?(roc)
Comment on attachment 8709611 [details] MozReview Request: Bug 1200514 - Run stable state runnables immediately if XPCOM has shutdown. r?khuey I don't review patches in mozreview, sorry. Please attach it to the bug as a diff.
Attachment #8709611 -
Flags: review?(khuey) → review-
Assignee | ||
Updated•9 years ago
|
Attachment #8709611 -
Attachment is obsolete: true
Attachment #8709611 -
Flags: review-
Assignee | ||
Updated•9 years ago
|
Attachment #8709612 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8709612 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Attachment #8709666 -
Flags: review?(khuey)
Comment on attachment 8709666 [details] [diff] [review] Run stable state runnables immediately if XPCOM has shutdown Review of attachment 8709666 [details] [diff] [review]: ----------------------------------------------------------------- CycleCollectedJSRuntime::RunInStableState is used on DOM worker threads, and afaict IsXPCOMShuttingDown is only safe to use on the main thread. So that needs to be prefixed with some sort of NS_IsMainThread() check. But the more interesting question is this: at this point in shutdown (the final cycle collection) any normal events you dispatch never get run (afaict, anyways). Why should stable state be any different?
Attachment #8709666 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11) > But the more interesting question is this: at this point in shutdown (the > final cycle collection) any normal events you dispatch never get run > (afaict, anyways). Why should stable state be any different? Indeed, it seems unreasonable for RunInStableState to work during the final CC when normal event dispatch does not. So should we instead expect every caller to RunInStableState that's at risk of being run during shutdown to check whether we've begun shutdown, and do something sensible if so? Should we make RunInStableState either assert/warn that shutdown has not begun? Or fail if shutdown has begun (i.e. make RunInStableState fallible).
I don't know. Nathan? I'm inclined to take the patch (with the threadsafety fix) but that does still leave the fact that the regular event queue doesn't work at this point (again, afaict).
Flags: needinfo?(nfroyd)
Comment 14•9 years ago
|
||
That's a very colorful try run. Changing all RunInStableState callers to do error checking seems like a large task, although probably one we're going to need if we're ever going to make shutdown work reasonably. Can we just run any pending runnables during nsCycleCollector_shutdown(): https://dxr.mozilla.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#950 https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#4157 or do we keep dispatching events even after the cycle collector has "shutdown"? It looks like the JS runtime can outlive the cycle collector shutting down, which doesn't seem helpful here. I guess we can take the patch, but please don't add IsXPCOMShuttingDown in a public header like that; otherwise people will misuse that function, rather than adding observers or similar. Just use gXPCOMShuttingDown.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9329899aebae
Assignee | ||
Comment 16•9 years ago
|
||
Processing the state state queue after the final cycle collection as suggested above seems to work.
Attachment #8712863 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Attachment #8709666 -
Attachment is obsolete: true
Comment 17•9 years ago
|
||
Comment on attachment 8712863 [details] [diff] [review] Run remaining stable state runnables after final cycle collection Review of attachment 8712863 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable to me; mccr8 is the cycle collection expert.
Attachment #8712863 -
Flags: review?(nfroyd)
Attachment #8712863 -
Flags: review?(continuation)
Attachment #8712863 -
Flags: review+
Comment 18•9 years ago
|
||
Comment on attachment 8712863 [details] [diff] [review] Run remaining stable state runnables after final cycle collection Review of attachment 8712863 [details] [diff] [review]: ----------------------------------------------------------------- I don't really know anything about this particular runnable queue, but this seems like a reasonable place to stick this if you want it to run after CC shutdown.
Attachment #8712863 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 19•9 years ago
|
||
Ok, thanks all.
Comment 21•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/140bd18a7c32
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•