1252945 - Extend Debugger.Object with a function to obtain a string representation of its referent.

Status: RESOLVED INCOMPLETE

(DevTools :: Debugger, defect, P5)

Description

[Eddy Bruel [:ejpbruel]]

As per our discussion on the mailing list, we're going to extend the interface of Debugger.Object so that the webconsole no longer needs to rely on unsafeDereference to implement the functionality it needs.

Comment 1

[Eddy Bruel [:ejpbruel]]

Marking this bug as P2 because it is a dependency for making the console work inside workers. This is not our topmost priority *right now*, but it will likely be in Q2.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed solution (WIP)

After thinking about it for some time, I think this is the way to go (the attached patch is still a WIP, and needs tests. I just want to know if you agree with the approach).

Remember that our goal here is to obtain a string representation of an error if evaluating a string throws. Since JavaScript can throw arbitrary values, the error can be anything, not just an Error object.

The error is represented as a wrapped debuggee value. If this value has a primitive type, we can simply call toString on the value to obtain a string representation. However, if the value is a Debugger.Object instance, toString will not work. What we need is for Debugger.Object to implement toString, and forward its implementation to its referent.

Of course, if the debuggee has overridden toString, this approach could cause debuggee code to run. The idea is to use the DebuggerNoExecute mechanism to make sure the debugger throws in that case.

Question for Shu: from looking at the code, it looks like we already use DebuggerNoExecute whenever a debugger trap is invoked, so I shouldn't have to do anything special in DebuggerObject_toString to make sure that running code in the debuggee throws. Is that correct?

We talked about how debugger API methods should normally not throw, unless API users can detect up front that this will happen. I opted not to do this for toString, partially because I want consumers to be able to distinguish between DebuggeeWouldRun and toString returning an empty string, partially because returning anything other than a string would break SpiderMonkey's contract for the toString method.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8732128 [details] [diff] [review]
Proposed solution (WIP)

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

This looks good, but right now the DebuggeeWouldRun checks are set up to only produce a warning, by default. Throwing errors would cause too much breakage at this point, since we haven't tracked down all the "escaping debuggee" bugs it revealed yet.

So what the patch needs is a way to temporarily turn on DebuggeeWouldRun errors unconditionally while calling Invoke, and then restore it to whatever its prior state was. I think we'll need this in other places too, so perhaps an RAII thing would be good.

The option you want to affect is cx->runtime()->options().throwOnDebuggeeWouldRun().

::: js/src/vm/Debugger.cpp
@@ +8530,5 @@
> +DebuggerObject_toString(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "toString", args, referent);
> +    AutoCompartment ac(cx, referent);
> +    Rooted<jsid> id(cx, NameToId(cx->names().toString));

There's a RootedId typedef.

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #3)
> Comment on attachment 8732128 [details] [diff] [review]
> Proposed solution (WIP)
> 
> Review of attachment 8732128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but right now the DebuggeeWouldRun checks are set up to
> only produce a warning, by default. Throwing errors would cause too much
> breakage at this point, since we haven't tracked down all the "escaping
> debuggee" bugs it revealed yet.
> 
> So what the patch needs is a way to temporarily turn on DebuggeeWouldRun
> errors unconditionally while calling Invoke, and then restore it to whatever
> its prior state was. I think we'll need this in other places too, so perhaps
> an RAII thing would be good.
> 
> The option you want to affect is
> cx->runtime()->options().throwOnDebuggeeWouldRun().
> 

That could be independently useful. I agree with Jim that it would be a good policy to have new APIs such as this to obey DWR strictly instead of only warning.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Improved solution (WIP)

This patch improves on the previous one by adding the RAII class AutoThrowOnDebuggeeWouldRun, which temporarily changes the behavior of DebuggeeNoExecute from a warning in to an error.

Everything seems to work, but there is one problem. Certain objects, such as arrays and Error objects, are implemented using self-hosted code. Calling toString on a D.O. wrapper for an Error should work (as long as the debuggee hasn't overridden Error.prototype.toString), but right now, the self-hosted code triggers DebuggeeWouldRun anyway.

Question for Shu: How can I work around this?

Comment 6

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #4)
> That could be independently useful. I agree with Jim that it would be a good
> policy to have new APIs such as this to obey DWR strictly instead of only
> warning.

YES

Comment 7

[Jim Blandy :jimb]

This question of whether self-hosted code counts as debuggee code came up when Shu was first implementing the DebuggeeWouldRun checks. Ideally, we would treat self-hosted code as non-debuggee code, just like the rest of the SpiderMonkey implementation, but still throw DebuggeeWouldRun if self-hosted code tried to call genuine debuggee code.

This turns out to be difficult, because self-hosted code lives in the debuggee compartment, and may call debuggee functions directly (and even have them inlined into it!), so there's no natural boundary at which to insert a DebuggeeWouldRun check. Shu looked into ways to hack in a DebuggeeWouldRun check when the self-hosted code called debuggee code, but it ended up involving new bytecodes and fiddling with inline caches and a bunch of other stuff that I really just don't want to get into.

In some cases, like lazy class initialization for Intl, we decided it was okay to special-case that invocation, because we are confident that a lazy initialization should never be able to reach debuggee code. That would be a bug in itself. But the same assumption isn't appropriate at all for the self-hosted toString methods. They could hit getters or proxies and anything could happen.

I think the most pragmatic way forward might be to have the devtools server *try* calling the new Debugger.Object method, and if it throws DebuggeeWouldRun (either because we unfortunately treat self-hosted code as debuggee code, or because content replaced the toString method with genuine debuggee code), fall back to some hand-written code based on D.O.p.class.

At that point, I don't know if there are enough objects whose toString methods are *not* self-hosted to make the new D.O.p method worthwhile anymore.

Comment 8

[Jim Blandy :jimb]

I'm not clearing Shu's needinfo request, just in case he's had some new ideas for how we could treat self-hosted code as non-debuggee code since we last worried about it.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 8732813 [details] [diff] [review]
Improved solution (WIP)

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

Needs docs.

::: js/src/jit-test/tests/debug/Object-toString-01.js
@@ +3,5 @@
> +load(libdir + "asserts.js");
> +
> +var g = newGlobal();
> +var dbg = Debugger(g);
> +dbg.onDebuggerStatement = function (frame) {

This test is way too indirect. There's no need for the onDebuggerStatement or frame.arguments or ...

Just say:

var gDO = dbg.addDebugee(g);
g.test = function (value, expected) {
    var valueDO = gDO.makeDebuggeeValue(value);
    assertEq(valueDO.toString(), expected);
}

@@ +24,5 @@
> +  // XXXshu: these tests should work, but trigger DebuggeeWouldRun because they
> +  // are implemented with self-hosted code. How can I work around this?
> +  // test([1, 2, 3], "1,2,3");
> +  // test(new Error("foo"), "");
> +}.toSource() + "()");

Rather than all this, just use a template string:

g.eval(`
    ... code ...
`);

::: js/src/jit-test/tests/debug/Object-toString-02.js
@@ +3,5 @@
> +load(libdir + "asserts.js");
> +
> +var g = newGlobal();
> +var dbg = Debugger(g);
> +dbg.onDebuggerStatement = function (frame) {

Both comments for -01 apply to this test as well.

::: js/src/vm/Debugger.cpp
@@ +8606,5 @@
>      JS_FN("executeInGlobalWithBindings", DebuggerObject_executeInGlobalWithBindings, 2, 0),
>      JS_FN("asEnvironment", DebuggerObject_asEnvironment, 0, 0),
>      JS_FN("unwrap", DebuggerObject_unwrap, 0, 0),
>      JS_FN("unsafeDereference", DebuggerObject_unsafeDereference, 0, 0),
> +    JS_FN("toString", DebuggerObject_toString, 0, 0),

I don't think this should be called 'toString'; that means it'll be used to convert every Debugger.Object instance to a string. I think it's important for developers to be cognizant of the difference between an ordinary CCW for an object, and a D.O referring to the object, since they have completely different interfaces. So having a D.O print the way its reference would doesn't seem helpful.

Perhaps this method could be named "callToString"?

Comment 10

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #7)
> This question of whether self-hosted code counts as debuggee code came up
> when Shu was first implementing the DebuggeeWouldRun checks. Ideally, we
> would treat self-hosted code as non-debuggee code, just like the rest of the
> SpiderMonkey implementation, but still throw DebuggeeWouldRun if self-hosted
> code tried to call genuine debuggee code.
> 
> This turns out to be difficult, because self-hosted code lives in the
> debuggee compartment, and may call debuggee functions directly (and even
> have them inlined into it!), so there's no natural boundary at which to
> insert a DebuggeeWouldRun check. Shu looked into ways to hack in a
> DebuggeeWouldRun check when the self-hosted code called debuggee code, but
> it ended up involving new bytecodes and fiddling with inline caches and a
> bunch of other stuff that I really just don't want to get into.
> 
> In some cases, like lazy class initialization for Intl, we decided it was
> okay to special-case that invocation, because we are confident that a lazy
> initialization should never be able to reach debuggee code. That would be a
> bug in itself. But the same assumption isn't appropriate at all for the
> self-hosted toString methods. They could hit getters or proxies and anything
> could happen.
> 
> I think the most pragmatic way forward might be to have the devtools server
> *try* calling the new Debugger.Object method, and if it throws
> DebuggeeWouldRun (either because we unfortunately treat self-hosted code as
> debuggee code, or because content replaced the toString method with genuine
> debuggee code), fall back to some hand-written code based on D.O.p.class.
> 
> At that point, I don't know if there are enough objects whose toString
> methods are *not* self-hosted to make the new D.O.p method worthwhile
> anymore.

No new ideas. :(

We can't even really go with whitelisting for the Error case. ErrorToString tries to get the properties obj.name and obj.message on the error object. We can't whitelist this function without explicitly spelling out somehow that these property gets cannot invoke code. E.g., come up with new self-hosted instrinsics like |GetValueProperty| or something that DEBUG builds know to insert checks for. That seems onerous to me and maybe a nonstarter.

This is really quite crappy, because printing debuggee errors has got to be the top use case for toString.

Comment 11

[Eddy Bruel [:ejpbruel]]

(In reply to Shu-yu Guo [:shu] from comment #10)
> (In reply to Jim Blandy :jimb from comment #7)
> > This question of whether self-hosted code counts as debuggee code came up
> > when Shu was first implementing the DebuggeeWouldRun checks. Ideally, we
> > would treat self-hosted code as non-debuggee code, just like the rest of the
> > SpiderMonkey implementation, but still throw DebuggeeWouldRun if self-hosted
> > code tried to call genuine debuggee code.
> > 
> > This turns out to be difficult, because self-hosted code lives in the
> > debuggee compartment, and may call debuggee functions directly (and even
> > have them inlined into it!), so there's no natural boundary at which to
> > insert a DebuggeeWouldRun check. Shu looked into ways to hack in a
> > DebuggeeWouldRun check when the self-hosted code called debuggee code, but
> > it ended up involving new bytecodes and fiddling with inline caches and a
> > bunch of other stuff that I really just don't want to get into.
> > 
> > In some cases, like lazy class initialization for Intl, we decided it was
> > okay to special-case that invocation, because we are confident that a lazy
> > initialization should never be able to reach debuggee code. That would be a
> > bug in itself. But the same assumption isn't appropriate at all for the
> > self-hosted toString methods. They could hit getters or proxies and anything
> > could happen.
> > 
> > I think the most pragmatic way forward might be to have the devtools server
> > *try* calling the new Debugger.Object method, and if it throws
> > DebuggeeWouldRun (either because we unfortunately treat self-hosted code as
> > debuggee code, or because content replaced the toString method with genuine
> > debuggee code), fall back to some hand-written code based on D.O.p.class.
> > 
> > At that point, I don't know if there are enough objects whose toString
> > methods are *not* self-hosted to make the new D.O.p method worthwhile
> > anymore.
> 
> No new ideas. :(
> 
> We can't even really go with whitelisting for the Error case. ErrorToString
> tries to get the properties obj.name and obj.message on the error object. We
> can't whitelist this function without explicitly spelling out somehow that
> these property gets cannot invoke code. E.g., come up with new self-hosted
> instrinsics like |GetValueProperty| or something that DEBUG builds know to
> insert checks for. That seems onerous to me and maybe a nonstarter.
> 
> This is really quite crappy, because printing debuggee errors has got to be
> the top use case for toString.

Is there a reason we couldn't simply whitelist all self-hosted code? Self-hosted code is part of the JS implementation, so it's not unreasonable to claim that it's not part of the debuggee. Unless self-hosted code can cause observable side-effects in debuggee code, of course...

If we cannot simply whitelist all self-hosted code, could we do something clever where we reorganise the self-hosted code in such a way that we can only whitelist compartment(s) with code that we know doesn't contain any side effects?

In addition, could we allow property access under the following conditions?

1. The property is a data property
2. The property is an accessor property, but the getter/setter function live in a (whitelisted) compartment for self-hosted code.

If we had those two things, could we then whitelist the Error object and the implementation of its getters/setters?

Comment 12

[Jim Blandy :jimb]

(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> Is there a reason we couldn't simply whitelist all self-hosted code?
> Self-hosted code is part of the JS implementation, so it's not unreasonable
> to claim that it's not part of the debuggee. Unless self-hosted code can
> cause observable side-effects in debuggee code, of course...

We all agree on this. It's technically very difficult to implement, for reasons I gave in comment 7.

Comment 13

[Jim Blandy :jimb]

Another angle:

How important is it to self-host Error.prototype.toString? Could we move it back to C++ for this very common use case?

Comment 14

[Jim Blandy :jimb]

Till says moving Error.prototype.toString to C++ would be sad, but not otherwise harmful. I agree.

Comment 15

[Shu-yu Guo [:shu]]

To recap conversations between Jim, Eddy, and me:

The current idea is to have a new D.O API that attempts to call a builtin function/get a builtin property. This would not be an invocation function, and the call would happen without unlocking the debuggee compartments.

We would implement this by altering the self-hosted code cloning behavior. When invoked from this function, self-hosted code will be cloned into a separate, internal compartment than the debuggee compartment. This internal compartment will be unprivileged, and CCWs between it and the debuggee compartment will be transparent. This ensures that all outgoing calls to JS that originate from the user will have NX checks because they are now cross-compartment.

Comment 16

[Eddy Bruel [:ejpbruel]]

Jim, given that we need this feature in order to remove unsafeDereference from the webconsole, which is blocking us from making the webconsole work inside workers, and given that DebuggeeNoExecute currently cannot distinguish reliably between client code and self-hosted code, would you be ok with landing this patch without the DebuggeeNoExecute check for now, and filing a followup bug for that?

Comment 17

[Jim Blandy :jimb]

(In reply to Eddy Bruel [:ejpbruel] from comment #16)
> Jim, given that we need this feature in order to remove unsafeDereference
> from the webconsole, which is blocking us from making the webconsole work
> inside workers, and given that DebuggeeNoExecute currently cannot
> distinguish reliably between client code and self-hosted code, would you be
> ok with landing this patch without the DebuggeeNoExecute check for now, and
> filing a followup bug for that?

I think I missed a step. Are you suggesting simply disabling the DebuggeeNoExecute check and then calling the toString method?

Isn't that the same as this?

var protoDO = errorDO.proto;
if (!protoDO) throw ...;
var toStringDO = proto.getPropertyDescriptor('toString').value;
if (!toStringDO) throw ...;
var message = toStringDO.call(errorDO).value;
if (!message) throw ...;
return message;

Comment 18

[Jason Laster [:jlast]]

Jim, Logan, what do you think about the priority/relevance of this work?

Comment 19

[Jim Blandy :jimb]

The motivation for this bug was to help the console print errors thrown by worker content. Is that still a problem, or was there some other workaround?

Comment 20

[:Harald Kirschner :digitarald]

Nicolas, is this still relevant with Fission refactoring?

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

I don't think so as we'll directly connect to the worker target