DOMOperationCallback can get a JSContext with no global object

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Igor Bukanov)

Tracking

({fixed1.9.1})

unspecified
mozilla1.9.1
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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?
Sample crash: bp-243f96f0-8fe2-11dd-95cb-001a4bd43ed6
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.
(Assignee)

Comment 7

9 years ago
(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
(Assignee)

Comment 8

9 years ago
Created attachment 342812 [details] [diff] [review]
untested fix

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?
(Assignee)

Comment 9

9 years ago
Created attachment 342857 [details] [diff] [review]
v1

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)
(Assignee)

Comment 10

9 years ago
Created attachment 342859 [details] [diff] [review]
v2

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)
(Assignee)

Updated

9 years ago
Attachment #342859 - Flags: superreview?(peterv)
Attachment #342859 - Flags: superreview?(jst)
Attachment #342859 - Flags: review?(peterv)
Attachment #342859 - Flags: review?(jst)
(Assignee)

Comment 11

9 years ago
Created attachment 343225 [details] [diff] [review]
v2b

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.
(Assignee)

Comment 14

9 years ago
(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.
(Assignee)

Comment 15

9 years ago
Created attachment 344962 [details] [diff] [review]
v3

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
(Assignee)

Comment 17

9 years ago
(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+

Updated

9 years ago
Attachment #344962 - Flags: superreview?(mrbkap)
Comment on attachment 344962 [details] [diff] [review]
v3

Blake, this look good to you too?

Updated

9 years ago
Attachment #344962 - Flags: superreview?(mrbkap) → superreview+
This looks ready to go?
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1
(Assignee)

Comment 21

9 years ago
(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.
(Assignee)

Comment 22

9 years ago
Created attachment 352515 [details] [diff] [review]
v3 with nits addresses
Attachment #352515 - Flags: review+
(Assignee)

Comment 23

9 years ago
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
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

9 years ago
Attachment #352515 - Flags: approval1.9.1+

Comment 25

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d22505fc2d2d
Keywords: fixed1.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?
Created attachment 355793 [details] [diff] [review]
Fix DEBUG_CC

Ugh, forgot about this.
Attachment #355793 - Flags: review?(bent.mozilla)
(Assignee)

Comment 29

9 years ago
(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+
You need to log in before you can comment on or make changes to this bug.