1526588 - Need to fix js::GetFirstGlobalInCompartment to handle the case when the first Realm has no global

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: log.txt.zip

I managed to get this on try with a stack. The log (containing the stack) is attached. Search for "Assertion failure: global, at /builds/worker/workspace/build/src/js/src/jsfriendapi.cpp:1153" and then scroll down a bit to see the stack.

The upshot is that we're doing XPCConvert::NativeInterface2JSObject, which calls XPCWrappedNativeScope::GetGlobalForWrappedNatives (elided from the stack, apparently) which calls js::GetFirstGlobalInCompartment. Amusingly enough, it doesn't even matter here, because our object is a Web IDL object and hence will ignore that global anyway...

In any case, we have a perfectly good global around (the one that's the current global of cx). We should be able to find a global to use for XPCWrappedNatives in this compartment instead of failing this assert.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that I tried https://hg.mozilla.org/try/rev/c5d603d42c57e2326d551d1eedbd9c11faf544b1 but that gives me a bunch of near-zero crashes in js::GetFirstGlobalInCompartment on try. But they are not the MOZ_CRASH, as far as I can tell, at least from the logs.

Comment 2

[Jan de Mooij [:jandem]]

Hm looking at these crashes I wonder if the compartment passed to GetFirstGlobalInCompartment is nullptr. Note the Vector.h for example.

The patch looks good but I think we should call realm->sweepGlobalObject() before using its global to handle incremental sweeping correctly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I wonder if the compartment passed to GetFirstGlobalInCompartment is nullptr

I had wondered that too, but I'm not sure how that would happen. For example, https://taskcluster-artifacts.net/TY439Rf7SYC9K02RDfMqFA/0/public/logs/live_backing.log is a log that shows the crash on 0x40 in Vector.h. It shows a stack like so:

js::GetFirstGlobalInCompartment(JS::Compartment*)
XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, xpcObjectHelper&, nsID const*, bool, nsresult*)
XPCWrappedNativeScope::GetComponentsJSObject(JS::MutableHandle<JSObject*>) XPCWrappedNativeScope::AttachComponentsObject(JSContext*)
xpc::InitGlobalObject(JSContext*, JS::Handle<JSObject*>, unsigned int)
nsGlobalWindowOuter::SetNewDocument(mozilla::dom::Document*, nsISupports*, bool)

InitGlobalObject takes a newly-created global as arg, and does a JSAutoRealm to enter its Realm. So when we land in XPCConvert::NativeInterface2JSObject we're in the Realm of that new global. Then we do:

XPCWrappedNativeScope* xpcscope = ObjectScope(JS::CurrentGlobalOrNull(cx));

(which goes via the CompartmentPrivate of the object) and

JSAutoRealm ar(cx, xpcscope->GetGlobalForWrappedNatives());

which is what does the GetFirstGlobalInCompartment() call. This last is getting the JS::Compartment from the Compartment() of the XPCWrappedNativeScope. This could be null if we swept it in XPCWrappedNativeScope::UpdateWeakPointersAfterGC but then how did we end up with the same XPCWrappedNativeScope for the new global? Was the JS::Compartment still hanging out in some way that caused us to reuse it, even though finalizing the last global in it puts it into an unusable state via PCWrappedNativeScope::UpdateWeakPointersAfterGC? I suppose that's possible....

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

but then how did we end up with the same XPCWrappedNativeScope for the new global? Was the JS::Compartment still hanging out in some way that caused us to reuse it, even though finalizing the last global in it puts it into an unusable state via PCWrappedNativeScope::UpdateWeakPointersAfterGC?

So IIUC maybe the code to pick a compartment to reuse needs to check for a live global/realm in it?

(In reply to Jan de Mooij [:jandem] from comment #2)

The patch looks good but I think we should call realm->sweepGlobalObject() before using its global to handle incremental sweeping correctly.

Thinking about it more, we want to do something like what realm->globalIsAboutToBeFinalized() does (beware the isGCSweeping assertion there).

Comment 5

[Boris Zbarsky [:bzbarsky]]

So IIUC maybe the code to pick a compartment to reuse needs to check for a live global/realm in it?

If we can have live compartments that don't have that, then yes. As you can see in https://hg.mozilla.org/try/rev/a553e62d36f1 I already had to check for nuked compartments, so we could expand that check to include the other bit. I'll give that a try run.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1526588 - Fix some issues with js::GetFirstGlobalInCompartment and XPCWrappedNativeScope::UpdateWeakPointersInAllScopesAfterGC. r?bzbarsky!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c306186cbd7
Fix some issues with js::GetFirstGlobalInCompartment and XPCWrappedNativeScope::UpdateWeakPointersInAllScopesAfterGC. r=bzbarsky

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8c306186cbd7