Closed Bug 1146580 Opened 9 years ago Closed 9 years ago

FinalizationWitnessService calls NS_DispatchToMainThread during xpcom shutdown

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).
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.
(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)
(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)
(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
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.
Blocks: 1171716
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: 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.
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)
Attachment #8616308 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/179adcb0f862
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: