Closed Bug 1200514 Opened 9 years ago Closed 8 years ago

protect against leaks from CycleCollectedJSRuntime::RunInStableState() late in shutdown

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

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)

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.
Keywords: regression
Do you think processing the queue immediately before the final cycle collection would be sufficient?
Flags: needinfo?(karlt)
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.
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.
Blocks: 1223691
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
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...
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-
Attachment #8709611 - Attachment is obsolete: true
Attachment #8709611 - Flags: review-
Attachment #8709612 - Flags: review?(roc)
Attachment #8709612 - Attachment is obsolete: true
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
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-
(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)
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)
Processing the state state queue after the final cycle collection as suggested above seems to work.
Attachment #8712863 - Flags: review?(nfroyd)
Attachment #8709666 - Attachment is obsolete: true
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 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+
Ok, thanks all.
https://hg.mozilla.org/mozilla-central/rev/140bd18a7c32
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: