Need to fix js::GetFirstGlobalInCompartment to handle the case when the first Realm has no global
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jandem)
References
Details
Attachments
(2 files)
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.
Reporter | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
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.
Reporter | ||
Comment 3•6 years ago
|
||
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....
Assignee | ||
Comment 4•6 years ago
|
||
(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).
Reporter | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
Comment 8•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Description
•