1482082 - Remove JSObject::deprecatedRealm/deprecatedGlobal calls in Debugger code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

Just a few callers left, two of them in Debugger code.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Remove remaining JSObject::deprecated{Realm,Global} calls in Debugger code

Two remaining callers in Debugger.cpp:

1) The deprecatedGlobal call in Debugger::construct can just be nonCCWGlobal. When I did the initial audit I didn't realize a CCW never wraps another CCW. Bug 1481793 part 3 is adding an assertion for this to Wrapper::wrappedObject.

2) In DebuggerObject::unwrap we use UnwrapOneChecked but this can return a CCW when a *same-compartment* Wrapper wraps a CCW. I think this is pretty uncommon but it does happen in the browser. It feels to me like the code probably intended this to be a CheckedUnwrap, this will unwrap the CCW too in that case.

This is green on Try so we don't have any tests relying on the current behavior of 2).

Comment 2

[Jim Blandy :jimb]

Comment on attachment 8998803 [details] [diff] [review]
Remove remaining JSObject::deprecated{Realm,Global} calls in Debugger code

Review of attachment 8998803 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +10828,5 @@
>  {
>      RootedObject referent(cx, object->referent());
>      Debugger* dbg = object->owner();
>  
> +    RootedObject unwrapped(cx, CheckedUnwrap(referent));

I don't think this is right. Debugger is actually supposed to expose the presence of wrappers (you can get two distinct Debugger.Objects, referring to a CCW and its referent), although we don't have a  lot of detailed support for it implemented. And hence, D.O.p.unwrap is supposed to remove only one layer of wrappers.

But I think it ought to be okay for this to return a CCW; see next comment.

@@ +10837,5 @@
>  
>      // Don't allow unwrapping to create a D.O whose referent is in an
>      // invisible-to-Debugger global. (If our referent is a *wrapper* to such,
>      // and the wrapper is in a visible realm, that's fine.)
> +    if (unwrapped->nonCCWRealm()->creationOptions().invisibleToDebugger()) {

I don't think it's meaningful for individual realms to be invisible to the debugger. When invisibility boundaries coincide with compartment boundaries, then only D.O.p.unwrap needs to be fallible; but if they need only coincide with realm boundaries, then pretty much any retrieval of a debuggee value might fail, and that seems intractable for users. So invisibility should have remained a property of compartments, and this check would not need to ask about unwrapped's realm.

As it stands, however, every use of JS::RealmCreationOptions::setInvisibleToDebugger also requests a fresh compartment for the new realm. So the same trick we used to enter a good-enough realm to operate on a Debugger.Object's referent should select an adequate realm here.

Comment 3

[Jan de Mooij [:jandem]]

newGlobal() for instance can create a new same-compartment realm with different invisibleToDebugger setting. So we will need to change that and also have to add assertions to the "new realm in existing compartment" code to verify this.

Also, DOM bindings have this SimpleGlobalObject global that's invisible to the debugger... Maybe it's okay for that to create a new compartment but it's a bit unfortunate to add a constraint to which globals can be put in the same compartment :/

Comment 4

[Jim Blandy :jimb]

Yes, newGlobal should be changed. I have a patch for that in bug 1482215.

SimpleGlobalObject already requests a separate compartment, in the system zone. If that's a problem, we could put all the SimpleGlobalObjects in a single invisible-to-debugger compartment.

It doesn't seem that unfortunate to me. We can only enforce security rules at compartment boundaries, so that already restricts which globals can go where. The 'invisible to debugger' boundary doesn't seem any harder to accommodate.

Comment 5

[Jim Blandy :jimb]

With bug 1482215 landed, I'm expecting that JSCompartment can now have an 'invisibleToDebugger' accessor, that consults the flag on the first realm in JSCompartment::realms_. Then, DebuggerObject::unwrap can check unwrapped->compartment()->invisibleToDebugger. So the check will still be there, but it won't need to pretend that the object belongs to a specific realm, only to a specific compartment.

Did I get that right?

Comment 6

[Jim Blandy :jimb]

Attachment: Bug 1482082: Delete JSObject::deprecatedGlobal, since it is unused. r?jandem

Depends on D14625

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a3522cd4bb7
Delete JSObject::deprecatedGlobal, since it is unused. r=jandem

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3a3522cd4bb7