Implement Debugger.DebuggeeWouldRun

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: mhordecki, Assigned: shu)

Tracking

(Blocks 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(7 attachments)

This covers both the error type itself and its use in the existing codebase.
I've attached the proposed implementation of the exception and the usage in two arbitrary cases, i.e. Debugger.Enviroment.{get/set}Variable and Debugger.Object.getOwnPropertyDescriptor. If you green-light the approach, I will go ahead and add proper guard statements for the remaining functions in vm/Debugger.cpp.

I'm not quite sure about the way I've implemented the exception itself - I had to go and modify a bunch of files that I'm unfamiliar with and it could certainly end in a total disaster (n.b. all tests pass, so there's that at least).
Attachment #799267 - Flags: review?(jimb)
Comment on attachment 799267 [details] [diff] [review]
0001-Bug-912337-Implement-Debugger.DebuggeeWouldRun.patch

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

This is a great first cut --- thanks very much for taking this on!

The stuff with the JSEXN_ values seems like a distraction, but it also seems hard to create normal error subclasses any other way. But that code insists on creating a global variable referring to the error constructor; doesn't this patch get you a new DebuggeeWouldRun global variable? It seems bizarre to expose that to web content that can't access Debugger. Perhaps we could teach that code how not to create global object properties for certain errors; I don't know.

So, why is it that we can construct the exception object in the debuggee's compartment (js::RunScript calls ReportDebuggeeWouldRunError directly), but then we can use instanceof passing the debugger's compartment's Debugger.DebuggeeWouldRun constructor? Is that what ErrorCopier takes care of?

::: js/src/jsapi.h
@@ +749,5 @@
>          JSEXN_SYNTAXERR,
>          JSEXN_TYPEERR,
>          JSEXN_URIERR,
> +        JSEXN_LIMIT,
> +        JSEXN_WOULDRUNERR = 42

I think this doesn't work. The JSEXN_ values are assumed to be dense, and get used as indices into arrays at various points.

For example, we define the members of JSAtomState using the JS_FOR_EACH_PROTOTYPE macro; then js::ClassName assumes that those members have been laid out in memory as an array indexed by the JSProto_ enum values; and then GetExceptionProtoKey assumes that JSEXN_ and JSProto_ values line up. If JSEXN_ isn't dense, then that all falls apart.

::: js/src/vm/Debugger.cpp
@@ +230,5 @@
>      }
>  };
>  
> +void
> +js::ReportDebuggeeWouldRunError(JSContext *cx, DebuggeeWouldRunCause cause) {

Won't this create the exception in the debuggee's compartment? Shouldn't we create it in the compartment of the debugger that caused the debuggee compartment to be frozen?

@@ +2311,5 @@
>          global->compartment()->removeDebuggee(fop, global, dmgc, compartmentEnum);
>  }
>  
> +bool
> +Debugger::getDebuggeeWouldRun(JSContext *cx, unsigned argc, Value *vp)

If we have multiple globals that each have their own Debugger constructor and are all active at the same time, how can we ensure that each debugger sees a DebuggeeWouldRun exception that is an instance of its Debugger.DebuggeeWouldRun constructor?

@@ +5479,4 @@
>  {
>      REQUIRE_ARGC("Debugger.Environment.getVariable", 1);
>      THIS_DEBUGENV_OWNER(cx, argc, vp, "getVariable", args, envobj, env, dbg);
> +    AutoDebuggeeWouldRunGuard ad(cx);

You should probably add a new 'object environment' cause for these.
Attachment #799267 - Flags: review?(jimb)
Thanks for the review!


> The stuff with the JSEXN_ values seems like a distraction, but it also seems
> hard to create normal error subclasses any other way. But that code insists
> on creating a global variable referring to the error constructor; doesn't
> this patch get you a new DebuggeeWouldRun global variable? It seems bizarre
> to expose that to web content that can't access Debugger. Perhaps we could
> teach that code how not to create global object properties for certain
> errors; I don't know.

Curious thing, I was almost certain that for some reason DebuggeeWouldRun does not get exposed. In fact, in my build of jsshell, when you reference DebuggeeWouldRun for the first time ReferenceError will be thrown, but then the constructor is lazily loaded and subsequent references return the constructor. I only checked for it once, that's why I was somehow convinced that for some uncanny reason this constructor in particular was hidden.

The problem is indeed nontrivial (in my perspective at least). Technically all I want to do is to be able to extract the class using GetExceptionProtoKey, which in turn uses js_GetClassObject, which I think only cares about slot indices in the global object itself. It may therefore be possible to set the slot, but not add any properties to the global object. Let me hack on the code for a few days, though.


> So, why is it that we can construct the exception object in the debuggee's
> compartment (js::RunScript calls ReportDebuggeeWouldRunError directly), but
> then we can use instanceof passing the debugger's compartment's
> Debugger.DebuggeeWouldRun constructor? Is that what ErrorCopier takes care
> of?

Yup, ErrorCopier does the copying using js_CopyErrorObject from jsexn.h. This copying also involves using the constructor from the new compartment.

This Just In: the way js_CopyErrorObject works, it discards any custom properties set on the error object. In this case, it's the "cause" property, which is discarded and not present in the copy. I will need to find a workaround.


> > +        JSEXN_LIMIT,
> > +        JSEXN_WOULDRUNERR = 42
> 
> I think this doesn't work. The JSEXN_ values are assumed to be dense, and
> get used as indices into arrays at various points.

(disclaimer: I might be very wrong with the following paragraph. The code works for me, though :) )

This is a slight hack which certainly warrants a proper comment in the code. The "= 42" part is in fact a difference
between indices of Error and DebuggeeWouldRun in jsprototypes.h (these indices are 11 and 53, respectively). All functions that use JSEXN_* work by incrementing the value of the enum by the absolute index of Error in the prototypes table (see assertions in jsexn.cpp at line 771 onwards).


> > +js::ReportDebuggeeWouldRunError(JSContext *cx, DebuggeeWouldRunCause cause) {
> 
> Won't this create the exception in the debuggee's compartment? Shouldn't we
> create it in the compartment of the debugger that caused the debuggee
> compartment to be frozen?

Exception is indeed created in the debuggee's compartment, but at some point ErrorCopier kicks in and copies the exception to the proper compartment.


> > +Debugger::getDebuggeeWouldRun(JSContext *cx, unsigned argc, Value *vp)
> 
> If we have multiple globals that each have their own Debugger constructor
> and are all active at the same time, how can we ensure that each debugger
> sees a DebuggeeWouldRun exception that is an instance of its
> Debugger.DebuggeeWouldRun constructor?

ErrorCopier? :)
(In reply to Mike Hordecki [:mhordecki] (UTC-7) from comment #3)
> > > +        JSEXN_LIMIT,
> > > +        JSEXN_WOULDRUNERR = 42
> > 
> > I think this doesn't work. The JSEXN_ values are assumed to be dense, and
> > get used as indices into arrays at various points.
> 
> (disclaimer: I might be very wrong with the following paragraph. The code
> works for me, though :) )
> 
> This is a slight hack which certainly warrants a proper comment in the code.
> The "= 42" part is in fact a difference
> between indices of Error and DebuggeeWouldRun in jsprototypes.h (these
> indices are 11 and 53, respectively). All functions that use JSEXN_* work by
> incrementing the value of the enum by the absolute index of Error in the
> prototypes table (see assertions in jsexn.cpp at line 771 onwards).

This is insanely delicate; we have to find a different way to deal with this.

What's the reason for not including it before JSEXN_LIMIT?
The ordering of JSEXN_* constants is based on relative positions of entries in jsprototypes.h. The ordering of prototypes in that file is fixed and new things can be added only at the end - at least according to the leading comment.

As far as internal consistency goes, quick grep tells that JSEXN_LIMIT (and, by extension, the act of iteration over all possible error types) happens only in jsexn.cpp, and these cases are already covered in my patch. This is of course a public API - this patch would kind of make this new exception type invisible to the existing users of the API.
I'll get to this in 2016.
Assignee: mike → shu
I'm getting to this now, and I'm not convinced that it is useful to throw a non-Error object (Debugger.DebuggeeWouldRun).

1) There's no precedent to throw non-Error objects in the engine, really.
2) I don't think giving a reason makes the error any more actionable.

I recommend just throwing an Error.
The intention was that Debugger.DebuggeeWouldRun would inherit from Error, the way RangeError and TypeError do. The intention was to make the errors easier to catch. But if that's hard to do, then I don't think it's critical to the design.
(In reply to Jim Blandy :jimb from comment #8)
> The intention was that Debugger.DebuggeeWouldRun would inherit from Error,
> the way RangeError and TypeError do. The intention was to make the errors
> easier to catch. But if that's hard to do, then I don't think it's critical
> to the design.

Oh, that's fine -- a new kind of Error. I am dubious of { reason: foo } though. Thoughts?
(In reply to Shu-yu Guo [:shu] from comment #9)
> Oh, that's fine -- a new kind of Error. I am dubious of { reason: foo }
> though. Thoughts?

You mean the 'cause' property? Is it hard for error subclasses to have extra properties?

I thought that would help the devtools produce better error messages. But I guess we're just using this to debug the devtools themselves.
Posting a brain dump for posterity.

The implementation of this is quite tricky. Namely, we need to track ownership of the no-execute locks to allocate the DebuggeeWouldRun exception in the right compartment.

For example, suppose we have 2 globals g1 and g2, and 2 debuggers dbg1 and dbg2; dbg1's debuggee set is {g1}, dbg2's debuggee set is {g1,g2}.

Given the following stack,

g1 -> dbg1 handler -> g2 -> dbg2 handler -> dbg2 invocation function -> g2

dbg1 handler locks g1. dbg2 handler locks g1 and g2. dbg2 invocation function unlocks g1 and g2. That is,

Lock l1(g1)
Lock l2(g1, g2)
Unlock u2(g1, g2)

So g1 is still locked by dbg1, g2 is unlocked. If g2 calls g1 code, then we should throw since dbg1 still has a lock on it. This should throw a Debugger.DebuggeeWouldRun error for the Debugger constructor inside dbg1. So, we need to track ownership of the lock stack.
Why isn't a simple count on each compartment adequate? That is, we do need to know that g1 is locked; but I'm not clear on why the error thrown needs to be somehow specific to dbg1. It should be enough to for g1 to know that one Debugger still has it locked.

And even if g1 does need to somehow specialize the error for dbg1, it can find out who has it locked by looking through its attached debuggers, at the time it needs to throw the error.
(In reply to Jim Blandy :jimb from comment #12)
> Why isn't a simple count on each compartment adequate? That is, we do need
> to know that g1 is locked; but I'm not clear on why the error thrown needs
> to be somehow specific to dbg1. It should be enough to for g1 to know that
> one Debugger still has it locked.
> 
> And even if g1 does need to somehow specialize the error for dbg1, it can
> find out who has it locked by looking through its attached debuggers, at the
> time it needs to throw the error.

For two reasons.

1) The error thrown should be an instanceof dbg1.Debugger.DebuggeeWouldRun. That is, it's an error in the debugger code and so should get an error allocated in the debugger's compartment.

2) At the point where we detect that an error should be thrown, we have already entered the debuggee compartment. Scanning through the debuggee compartment's debuggers to find out who has a lock on is pretty much tracking ownership anyways.
It's your call. But it seems easier to compute information from authoritative sources when it's needed than to maintain invariants while the program runs --- especially for information needed rarely.
Posted patch Tests.Splinter Review
Attachment #8720095 - Flags: review?(jimb)
Attachment #8720103 - Flags: review?(jimb)
There's one remaining failing test, Object-deleteProperty-error-02.js.

It's explicitly testing that D.O.p.deleteProperty can end up calling deleteProperty Proxy traps in the debuggee compartment. I think like D.O.p.deleteProperty shouldn't count as an invocation function. Not sure how to meaningfully change the test or if it should just be removed.
Comment on attachment 8720092 [details] [diff] [review]
Make a new Error subclass: Debugger.DebuggeeWouldRun.

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

::: js/src/vm/Debugger.cpp
@@ +8465,5 @@
>          objectProto(cx),
>          envProto(cx),
>          memoryProto(cx);
> +    RootedObject debuggeeWouldRunProto(cx);
> +    RootedValue debuggeeWouldRunCtor(cx);

Is there a reason not to include these in the list above?
Attachment #8720092 - Flags: review?(jimb) → review+
Comment on attachment 8720093 [details] [diff] [review]
Update docs on Debugger.DebuggeeWouldRun.

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

Looks good to me, with the requested fixes.

::: js/src/doc/Debugger/Conventions.md
@@ +155,5 @@
>  common conventions to report the debuggee's behavior safely. For other
>  methods, if their normal operation would cause debuggee code to run, they
>  throw an instance of the `Debugger.DebuggeeWouldRun` exception.
>  
> +If multiple there frames on the stack of debugger code from multiple Debugger

"If multiple there"

@@ +157,5 @@
>  throw an instance of the `Debugger.DebuggeeWouldRun` exception.
>  
> +If multiple there frames on the stack of debugger code from multiple Debugger
> +instances, the thrown exception is an instance of the topmost debugger frame's
> +global's `Debugger.DebuggeeWouldRun`.

I think this is missing the word "locking".
Attachment #8720093 - Flags: review?(jimb) → review+
Comment on attachment 8720094 [details] [diff] [review]
Prohibit debugger code from re-entering debuggee code.

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

This looks really great. I have some requests, though.

::: js/src/vm/Debugger.cpp
@@ +304,5 @@
> +        return dbg_;
> +    }
> +
> +#ifdef DEBUG
> +    bool isUniqueLockedInStack(Debugger& dbg) const {

It doesn't seem like this checks whether anything is locked or unique. Could we either make it pickier, or change the name?

@@ +318,5 @@
> +#endif
> +
> +    // Given a debuggee compartment, find the lock that locks it. Returns
> +    // nullptr if not found.
> +    EnterDebuggeeNoExecute* findInStack(JSCompartment* debuggee) const {

It looks to me like this should be a static method that takes a JSRuntime * and searches it. Then we could skip all the "if (cx->runtime()->noExecuteDebuggerTop)" tests at the call sites.

@@ +328,5 @@
> +        return nullptr;
> +    }
> +
> +    // As above, except for returning the Debugger instance.
> +    Debugger* findDebuggerInStack(JSCompartment* debuggee) const {

Similarly.

::: js/src/vm/Debugger.h
@@ +1101,5 @@
>  }
>  
>  bool ReportObjectRequired(JSContext* cx);
>  
> +bool CheckDebuggeeNoExecute(JSContext* cx);

Could we make this an inline function, with the definition in Debugger.cpp as a "slowPath..." and fold in the isDebuggee check that all the call sites are performing?
Attachment #8720094 - Flags: review?(jimb) → review+
Comment on attachment 8720095 [details] [diff] [review]
Tests.

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

So wonderful.
Attachment #8720095 - Flags: review?(jimb) → review+
Comment on attachment 8720103 [details] [diff] [review]
Update existing tests.

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

r=me with more BE BOLD

::: js/src/jit-test/tests/debug/Debugger-onIonCompilation.js
@@ +1,3 @@
> +// Disabled since onIonCompilation is slated for removal and the test throws
> +// DebuggeeWouldRun.
> +quit(0);

Just go ahead and delete the file.
Attachment #8720103 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #21)
> Comment on attachment 8720092 [details] [diff] [review]
> Make a new Error subclass: Debugger.DebuggeeWouldRun.
> 
> Review of attachment 8720092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/Debugger.cpp
> @@ +8465,5 @@
> >          objectProto(cx),
> >          envProto(cx),
> >          memoryProto(cx);
> > +    RootedObject debuggeeWouldRunProto(cx);
> > +    RootedValue debuggeeWouldRunCtor(cx);
> 
> Is there a reason not to include these in the list above?

Because debuggeeWouldRunProto is a RootedObject and not a RootedNativeObject, and the ctor is a RootedValue.
Attachment #8720620 - Flags: review?(jimb) → review+
Blocks: 1249469
Depends on: 1250190
Depends on: 1250520
You need to log in before you can comment on or make changes to this bug.