Closed Bug 458099 Opened 12 years ago Closed 11 years ago

DOMOperationCallback can get a JSContext with no global object

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: bent.mozilla, Assigned: igor)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 4 obsolete files)

crash-stats is showing 56 crashes in the last week in JS_IsSystemObject called from DOMOperationCallback. Apparently JS_GetGlobalObject is returning null. Looks like this was changed in bug 411267.
Flags: blocking1.9.1?
bp-a07a37ba-8fc5-11dd-a246-001cc4e2bf68 shows a good stack where we're in cycle collection, which nulls out all contexts global objects (?), and at that point we end up unlinking elements, which ends up destroying a docshell, which ends up firing pagehide notifications, i.e. running JS.

Clearly that's not cool, where'd be the best place to fix that?
We could delay frameloader finalization here, as we do elsewhere.  Would that help?
So finalization would happen after CC?
In this case, yes.

Alternately, we need to fix CC to deal with arbitrary script running during Unlink() (which we may need anyway).
I wonder if we could move RestoreContextGlobals to before the call to nsCycleCollector_finishCollection.
(In reply to comment #6)
> I wonder if we could move RestoreContextGlobals to before the call to
> nsCycleCollector_finishCollection.

This is very good observation. In fact, since RestoreContextGlobals iterates over JSContext instances, it should run in response not to JSGC_END, but rather to JSGC_FINALIZE_END, when access to all JSContext is still serialized under the JS GC lock.
Assignee: nobody → igor
Attached patch untested fix (obsolete) — Splinter Review
The patch introduces a new JS option, JSOPTION_UNROOTED_GLOBAL, that allows to keep globals in the context while implementing the desired cycle collection semantics.

I have not tested the patch, but who should review it when it will be ready?
Attached patch v1 (obsolete) — Splinter Review
Compared with the previous, the new patch fixes some comments and removes no longer used function prototypes. It passes mochi tests locally.
Attachment #342812 - Attachment is obsolete: true
Attachment #342857 - Flags: superreview?(jst)
Attachment #342857 - Flags: review?(jst)
Attached patch v2 (obsolete) — Splinter Review
The new assert fixes the assert in XPCJSRuntime::TraceXPConnectRoots. The previous version was too strict.
Attachment #342857 - Attachment is obsolete: true
Attachment #342859 - Flags: superreview?(jst)
Attachment #342859 - Flags: review?(jst)
Attachment #342857 - Flags: superreview?(jst)
Attachment #342857 - Flags: review?(jst)
Attachment #342859 - Flags: superreview?(peterv)
Attachment #342859 - Flags: superreview?(jst)
Attachment #342859 - Flags: review?(peterv)
Attachment #342859 - Flags: review?(jst)
Attached patch v2b (obsolete) — Splinter Review
The new version of the patch is a sync of v2 with the trunk.
Attachment #342859 - Attachment is obsolete: true
Attachment #343225 - Flags: review?(peterv)
Attachment #342859 - Flags: superreview?(peterv)
Attachment #342859 - Flags: review?(peterv)
Actually, I'm not sure I still understand how this works. The JSContexts are destroyed when their nsJSContext is unlinked, which happens in FinishCollection. Doesn't marking context globals before doing unlinking mean that we will always mark them all, even the ones we want to unlink?

Nit on the patch, use JSBool and JS_TRUE/JS_FALSE.
Maybe we could null out the global from JSContextParticipant's RootAndUnlinkJSObjects, that will be called at the end of nsCycleCollector_beginCollection for those contexts that are in a cycle that we want to unlink.
(In reply to comment #12)
> Doesn't marking context globals before doing unlinking mean
> that we will always mark them all, even the ones we want to unlink?

That just means that the globals will be collected during the next JS GC. I suspect that this was done to avoid premature finalization of JS stuff that may still can be accessed during the unlinking.
Attached patch v3Splinter Review
The new version uses the gold old JS bools and not that wonderful C++ stuff.
Attachment #343225 - Attachment is obsolete: true
Attachment #344962 - Flags: review?(peterv)
Attachment #343225 - Flags: review?(peterv)
I think we should either take Igor's patch here for 1.9.1, or we should bandaid the crash for now. Peter/Igor, what do you think? Plusing for now.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(In reply to comment #16)
> I think we should either take Igor's patch here for 1.9.1, or we should bandaid
> the crash for now.

My patch is pretty much the bandaid - it does not change the reliance in the current code on the extra JS GC cycle. Its potential drawback is exposure through a SpiderMonkey's public API of somewhat esoteric functionality, but I can move that out from the public headers.
Comment on attachment 344962 [details] [diff] [review]
v3

>Index: 31-ff/js/src/xpconnect/src/nsXPConnect.cpp
>===================================================================

>+    switch (status)

Nit: no space after switch

>Index: 31-ff/js/src/xpconnect/src/xpcjsruntime.cpp
>===================================================================

>+void XPCJSRuntime::TraceXPConnectRoots(JSTracer *trc, JSBool restoreGlobals)

restoreGlobals seems like the wrong name. rootGlobals?

>+        NS_ASSERTION(!restoreGlobals || mUnrootedGlobalCount == 0, "bad state");

Couldn't we destroy a context before calling TraceXPConnectRoots? mUnrootedGlobalCount would not be 0 in that case.

>Index: 31-ff/js/src/xpconnect/src/xpcprivate.h
>===================================================================

>+    uintN mUnrootedGlobalCount;

It looks like you only use mUnrootedGlobalCount in assertions, so I'd make it #ifdef DEBUG.

You might need a r= from a JS peer for adding JSOPTION_UNROOTED_GLOBAL.
Attachment #344962 - Flags: review?(peterv) → review+
Attachment #344962 - Flags: superreview?(mrbkap)
Comment on attachment 344962 [details] [diff] [review]
v3

Blake, this look good to you too?
Attachment #344962 - Flags: superreview?(mrbkap) → superreview+
This looks ready to go?
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1
(In reply to comment #18)
> (From update of attachment 344962 [details] [diff] [review])
> >Index: 31-ff/js/src/xpconnect/src/xpcjsruntime.cpp
> >+        NS_ASSERTION(!restoreGlobals || mUnrootedGlobalCount == 0, "bad state");
> 
> Couldn't we destroy a context before calling TraceXPConnectRoots?
> mUnrootedGlobalCount would not be 0 in that case.

TraceXPConnectRoots(trc, restoreGlobals == JS_TRUE) is called as a part of the JS GC in response to JSGC_MARK_END notification after a call to UnrootContextGlobals from JSGC_BEGIN. Since JS GC assumes that no JSContext instances could be destroyed between these two notificatiosn, the assertion must hold.  

> 
> >Index: 31-ff/js/src/xpconnect/src/xpcprivate.h
> >===================================================================
> 
> >+    uintN mUnrootedGlobalCount;
> 
> It looks like you only use mUnrootedGlobalCount in assertions, so I'd make it
> #ifdef DEBUG.
> 
> You might need a r= from a JS peer for adding JSOPTION_UNROOTED_GLOBAL.
Attachment #352515 - Flags: review+
I could not land the patch myself since the tree is orange when I have time to monitor it.
http://hg.mozilla.org/mozilla-central/rev/4d7eb87cc160
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #352515 - Flags: approval1.9.1+
This broke DEBUG_CC builds...

BeginCycleCollection:
nsXPConnect.cpp:556: error: ‘class XPCJSRuntime’ has no member named ‘UnsetContextGlobals’

FinishCycleCollection:
nsXPConnect.cpp:603: error: ‘class XPCJSRuntime’ has no member named ‘RestoreContextGlobals’
Hm, can we just remove those lines now?
Attached patch Fix DEBUG_CCSplinter Review
Ugh, forgot about this.
Attachment #355793 - Flags: review?(bent.mozilla)
(In reply to comment #28)
> Created an attachment (id=355793) [details]
> Fix DEBUG_CC

This should go to a separated bug for proper handling.
Comment on attachment 355793 [details] [diff] [review]
Fix DEBUG_CC

This works well, thanks!
Attachment #355793 - Flags: review?(bent.mozilla) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.