Closed
Bug 1146580
Opened 9 years ago
Closed 9 years ago
FinalizationWitnessService calls NS_DispatchToMainThread during xpcom shutdown
Categories
(Toolkit :: Async Tooling, defect)
Toolkit
Async Tooling
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
5.63 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
This shows up in xpcshell tests. In this case finalization [1] is happening on the main thread, so we could just side step the issue by calling the observer directly. [1] https://hg.mozilla.org/mozilla-central/annotate/e730012260a4/toolkit/components/finalizationwitness/FinalizationWitnessService.cpp#l109 Full stack: >Crash reason: SIGSEGV >Crash address: 0x0 >Thread 0 (crashed) > 0 libxul.so!NS_DispatchToMainThread(nsIRunnable*, unsigned int) [nsThreadUtils.cpp:c9453fa5fee2 : 174 + 0x6] > 1 libxul.so!mozilla::::Finalize [FinalizationWitnessService.cpp:c9453fa5fee2 : 109 + 0x6] > 2 libxul.so!FinalizeTypedArenas<JSObject> [jsobjinlines.h:c9453fa5fee2 : 61 + 0x12] > 3 libxul.so!js::gc::ArenaLists::forceFinalizeNow(js::FreeOp*, js::gc::AllocKind, js::gc::ArenaLists::KeepArenasEnum, js::gc::ArenaHeader**) [jsgc.cpp:c9453fa5fee2 : 2807 + 0x4] > 4 libxul.so!js::gc::ArenaLists::queueForegroundObjectsForSweep(js::FreeOp*) [jsgc.cpp:c9453fa5fee2 : 2790 + 0x22] > 5 libxul.so!js::gc::GCRuntime::beginSweepingZoneGroup() [jsgc.cpp:c9453fa5fee2 : 5096 + 0x4] > 6 libxul.so!js::gc::GCRuntime::beginSweepPhase(bool) [jsgc.cpp:c9453fa5fee2 : 5190 + 0x7] > 7 libxul.so!js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason) [jsgc.cpp:c9453fa5fee2 : 5930 + 0xb] > 8 libxul.so!js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) [jsgc.cpp:c9453fa5fee2 : 6119 + 0xd] > 9 libxul.so!js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) [jsgc.cpp:c9453fa5fee2 : 6231 + 0xa] >10 libxul.so!js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) [jsgc.cpp:c9453fa5fee2 : 6292 + 0x6] >11 libxul.so!nsXPConnect::~nsXPConnect() [nsXPConnect.cpp:c9453fa5fee2 : 88 + 0x8] >12 libxul.so!nsXPConnect::~nsXPConnect() [nsXPConnect.cpp:c9453fa5fee2 : 110 + 0x4] >13 libxul.so!nsXPConnect::Release() [nsXPConnect.cpp:c9453fa5fee2 : 40 + 0x8] >14 libxul.so!xpcModuleDtor() [XPCModule.cpp:c9453fa5fee2 : 22 + 0x4] >15 libxul.so!nsComponentManagerImpl::KnownModule::~KnownModule() [nsComponentManager.h:c9453fa5fee2 : 234 + 0x1] >16 libxul.so!nsAutoPtr<nsComponentManagerImpl::KnownModule>::~nsAutoPtr() [nsAutoPtr.h:c9453fa5fee2 : 74 + 0x7] >17 libxul.so!nsTArray_Impl<nsAutoPtr<nsComponentManagerImpl::KnownModule>, nsTArrayInfallibleAllocator>::Clear() [nsTArray.h:c9453fa5fee2 : 489 + 0x4] >18 libxul.so!nsComponentManagerImpl::Shutdown() [nsComponentManager.cpp:c9453fa5fee2 : 898 + 0xb] >19 libxul.so!mozilla::ShutdownXPCOM(nsIServiceManager*) [XPCOMInit.cpp:c9453fa5fee2 : 984 + 0x4]
Well, the idea of dispatching it is to make sure that we cannot do weird things (such as calling JS code) during garbage-collection. I'm pretty sure that calling the observer directly would be a Bad Idea (tm).
Comment 2•9 years ago
|
||
Hmm, yeah just running random code during the GC is not a good idea. CycleCollectedJSRuntime has some similar DeferredFinalizer stuff, which uses NS_DispatchToCurrentThread(), though maybe not during the GC itself. Probably a better idea is to figure out how to get this to be destroyed earlier. :) We should probably not be running the GC at the point where we can't dispatch a runnable any more.
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #2) > Hmm, yeah just running random code during the GC is not a good idea. > CycleCollectedJSRuntime has some similar DeferredFinalizer stuff, which uses > NS_DispatchToCurrentThread(), though maybe not during the GC itself. > Probably a better idea is to figure out how to get this to be destroyed > earlier. :) We should probably not be running the GC at the point where we > can't dispatch a runnable any more. I should note invoking the GC is intentional [1] and there's a comment in FinalizationWitnessService [2] indicating they know calling |NS_DispatchToMainThread| can fail. Bobby, do you have any idea if we can move this logic outside of nsXPConnect's dtor? [1] https://hg.mozilla.org/mozilla-central/annotate/bc85c479668a/js/xpconnect/src/nsXPConnect.cpp#l88 [2] https://hg.mozilla.org/mozilla-central/annotate/e730012260a4/toolkit/components/finalizationwitness/FinalizationWitnessService.cpp#l110
Flags: needinfo?(bobbyholley)
Comment 4•9 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #3) > (In reply to Andrew McCreight [:mccr8] from comment #2) > > Hmm, yeah just running random code during the GC is not a good idea. > > CycleCollectedJSRuntime has some similar DeferredFinalizer stuff, which uses > > NS_DispatchToCurrentThread(), though maybe not during the GC itself. > > Probably a better idea is to figure out how to get this to be destroyed > > earlier. :) We should probably not be running the GC at the point where we > > can't dispatch a runnable any more. > > I should note invoking the GC is intentional [1] and there's a comment in > FinalizationWitnessService [2] indicating they know calling > |NS_DispatchToMainThread| can fail. > > Bobby, do you have any idea if we can move this logic outside of > nsXPConnect's dtor? Which logic? Do you mean "avoid GCing twice during XPConnect shutdown"? I think we do want to GC _once_ during shutdown, at least in debug builds. Doing it twice is dumb, but that code is a pile of live wires. I wouldn't recommend digging into it unless you're willing to get sucked down a couple of rabbit holes. If you're referring specifically to "moving it out of the XPConnect destructor", we could do that, yes - but given that the XPConnect singleton is manually deleted, it's not clear to me how that would help.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #4) > > Which logic? Do you mean "avoid GCing twice during XPConnect shutdown"? I > think we do want to GC _once_ during shutdown, at least in debug builds. > Doing it twice is dumb, but that code is a pile of live wires. I wouldn't > recommend digging into it unless you're willing to get sucked down a couple > of rabbit holes. > > If you're referring specifically to "moving it out of the XPConnect > destructor", we could do that, yes - but given that the XPConnect singleton > is manually deleted, it's not clear to me how that would help. The latter. We're trying make |NS_DispatchToMainThread| infallible under the assumption that calling it after xpcom shutdown is incorrect. By the time |nsComponentManagerImpl::Shutdown()| is called [1] it is no longer permissible to post messages to the main thread. [1] http://hg.mozilla.org/mozilla-central/annotate/bc85c479668a/xpcom/build/XPCOMInit.cpp#l984
Comment 6•9 years ago
|
||
Oh actually, the XPConnect singleton _isn't_ manually deleted - ReleaseXPConnectSingleton operates via refcounting. Anyway, I don't have any deus ex machina ideas here - somebody just needs to read the code and come up with something.
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad9784a1a37
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f08edd66fd1e
Assignee | ||
Comment 9•9 years ago
|
||
Jeff, you seem to have been the original reviewer for FinalizationWitnessService. Do you mind doing a review or redirecting to a more appropriate reviewer?
Attachment #8616308 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8616308 [details] [diff] [review] Make FinalizationWitnessService listen for xpcom shutdown Review of attachment 8616308 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/finalizationwitness/FinalizationWitnessService.cpp @@ +105,1 @@ > // Forget() has been called That's not true anymore.
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb2ee0a98cf9
Assignee | ||
Comment 12•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4ba06b7a62
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8616308 [details] [diff] [review] Make FinalizationWitnessService listen for xpcom shutdown Bobby would you might reviewing this, Waldo doesn't feel well versed enough in xpcom/xpconnect shutdown to do the review.
Attachment #8616308 -
Flags: review?(jwalden+bmo) → review?(bobbyholley)
Updated•9 years ago
|
Attachment #8616308 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/179adcb0f862
https://hg.mozilla.org/mozilla-central/rev/179adcb0f862
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•