458099 - DOMOperationCallback can get a JSContext with no global object

Status: RESOLVED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sample crash: bp-243f96f0-8fe2-11dd-95cb-001a4bd43ed6

Comment 2

[Johnny Stenback (:jst)]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

We could delay frameloader finalization here, as we do elsewhere.  Would that help?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So finalization would happen after CC?

Comment 5

[Boris Zbarsky [:bzbarsky]]

In this case, yes.

Alternately, we need to fix CC to deal with arbitrary script running during Unlink() (which we may need anyway).

Comment 6

[Peter Van der Beken [:peterv]]

I wonder if we could move RestoreContextGlobals to before the call to nsCycleCollector_finishCollection.

Comment 7

[Igor Bukanov]

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

Comment 8

[Igor Bukanov]

Attachment: 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?

Comment 9

[Igor Bukanov]

Attachment: v1

Compared with the previous, the new patch fixes some comments and removes no longer used function prototypes. It passes mochi tests locally.

Comment 10

[Igor Bukanov]

Attachment: v2

The new assert fixes the assert in XPCJSRuntime::TraceXPConnectRoots. The previous version was too strict.

Comment 11

[Igor Bukanov]

Attachment: v2b

The new version of the patch is a sync of v2 with the trunk.

Comment 12

[Peter Van der Beken [: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.

Comment 13

[Peter Van der Beken [:peterv]]

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.

Comment 14

[Igor Bukanov]

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

Comment 15

[Igor Bukanov]

Attachment: v3

The new version uses the gold old JS bools and not that wonderful C++ stuff.

Comment 16

[Johnny Stenback (:jst)]

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.

Comment 17

[Igor Bukanov]

(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 18

[Peter Van der Beken [:peterv]]

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.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 344962 [details] [diff] [review]
v3

Blake, this look good to you too?

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This looks ready to go?

Comment 21

[Igor Bukanov]

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

Comment 22

[Igor Bukanov]

Attachment: v3 with nits addresses

Comment 23

[Igor Bukanov]

I could not land the patch myself since the tree is orange when I have time to monitor it.

Comment 24

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/4d7eb87cc160

Comment 25

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d22505fc2d2d

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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’

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, can we just remove those lines now?

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: Fix DEBUG_CC

Ugh, forgot about this.

Comment 29

[Igor Bukanov]

(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 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 355793 [details] [diff] [review]
Fix DEBUG_CC

This works well, thanks!