Closed Bug 1294013 Opened 6 years ago Closed 6 years ago

[jsdbg2] Disentangle parsing and checking of resumption values.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(9 files, 2 obsolete files)

2.02 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.74 KB, patch
jimb
: review+
Details | Diff | Splinter Review
1.32 KB, patch
jimb
: review+
Details | Diff | Splinter Review
6.54 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.35 KB, patch
jimb
: review+
Details | Diff | Splinter Review
5.55 KB, patch
jimb
: review+
Details | Diff | Splinter Review
12.00 KB, patch
jimb
: review+
Details | Diff | Splinter Review
8.84 KB, patch
jimb
: review+
Details | Diff | Splinter Review
3.00 KB, patch
Details | Diff | Splinter Review
In the debugger API, parseResumptionValue currently does *two* things:
1. It parses the resumption value.
2. If the current frame is for a derived constructor call, ES6 does not allow us
   to return from that call while the value of this is still uninitialised (it
   is initialised by the call to super()). The only exception to this is if the
   constructor returns something other than a primitive value. In that case, the
   returned value becomes the new this.

   parseResumptionValue checks the resumption value as follows: if the current
   frame is a derived constructor frame, AND the this value is optimized out,
   AND the resumption value is a 'return', AND the value to be returned is a
   primitive, throw an uninitialized this exception.

For the C++ interface for Debugger.Frame, we need to disentangle parsing the resumption value from checking the resumption value. See bug 1271650, comment 92, for an explanation why.
Disentangling this is not as simple as it seems. parseResumptionValue may cause an uncaught exception to be thrown. This uncaught exception is handled with a call to handleUncaughtException. handleUncaughtException may cause the exception handler to be called (if one is installed). The exception handler *also* returns a resumption value, which must then be parsed, which may caused further exceptions to be thrown, etc.

The control flow seems to be roughly this:
1. Try to parse the resumption value.
2. If parsing the resumption value causes an uncaught exception to be thrown:
   a. Handle the uncaught exception.
   b. If handling the uncaught exception causes a new resumption value to be created:
      I. Go back to 1.
3. Check the resumption value.
4. If checking the resumption value causes an exception to be thrown:
   a. Handle the uncaught exception.
   b. If handling the uncaught exception causes a new resumption value to be created:
      I. Go back to 1.

Right now, this control flow is implemented via recursion, which is why parseResumptionValue needs to carry around an AbstractFramePtr (since its required in step 3). We should be able to implement this logic as a loop instead, with steps 1-2 and steps 3-4 as separate functions.

Question for jimb: can this algorithm ever enter an infinite loop? What happens if we check the resumption value, it fails the condition in comment 1, we throw an exception, the exception handler is caught, and it returns a resumption value that also fails this condition?
Flags: needinfo?(jimb)
(In reply to Eddy Bruel [:ejpbruel] from comment #1)
> Disentangling this is not as simple as it seems. parseResumptionValue may
> cause an uncaught exception to be thrown. This uncaught exception is handled
> with a call to handleUncaughtException. handleUncaughtException may cause
> the exception handler to be called (if one is installed). The exception
> handler *also* returns a resumption value, which must then be parsed, which
> may caused further exceptions to be thrown, etc.
> 
> The control flow seems to be roughly this:
> 1. Try to parse the resumption value.
> 2. If parsing the resumption value causes an uncaught exception to be thrown:
>    a. Handle the uncaught exception.
>    b. If handling the uncaught exception causes a new resumption value to be
> created:
>       I. Go back to 1.
> 3. Check the resumption value.
> 4. If checking the resumption value causes an exception to be thrown:
>    a. Handle the uncaught exception.
>    b. If handling the uncaught exception causes a new resumption value to be
> created:
>       I. Go back to 1.
> 
> Right now, this control flow is implemented via recursion, which is why
> parseResumptionValue needs to carry around an AbstractFramePtr (since its
> required in step 3). We should be able to implement this logic as a loop
> instead, with steps 1-2 and steps 3-4 as separate functions.
> 
> Question for jimb: can this algorithm ever enter an infinite loop? What
> happens if we check the resumption value, it fails the condition in comment
> 1, we throw an exception, the exception handler is caught, and it returns a
> resumption value that also fails this condition?

Nevermind. I just realised that if step 2b causes a new resumption value to be created, we go back to step 1, but with callHook set to false. That way, if parsing the new resumption value also causes an exception
to be thrown, we don't try to call the exception handler hook again.
Flags: needinfo?(jimb)
Ah, now I see why this is set up recursively:

We need to keep track of whether or not we already called the uncaught exception hook, which should only be called once. The `callHook` argument is used to determine whether the uncaught exception hook should be called or not. It is initially set to true, but after the first call to the uncaught exception hook, we parse the resulting new resumption value by recursively calling `parseResumptionValue` with the `callHook` argument set to false. This requires the `callHook` argument to be propagated to each subsequent step. It is for this reason that each subsequent step is set up as a recursive call.

What we want is to break up `parseResumptionValue` into two parts:
1. Parse the resumption value (steps 1-2 of the algorithm in comment 1)
2. Check the resumption value (steps 3-4 of the algorithm in comment 1)

By breaking up `parseResumptionValue` like this, only step 2 requires an AbstractFramePtr. However, this means that step 1 cannot recursively call step 2 (since it doesn't have an AbstractFramePtr) and propagate the value of `callHook`. Instead, we need to call these two steps in sequence. This poses a problem, because step 2 now doesn't know whether the uncaught exception hook was already called in step 2.

I can think of two ways to solve this. We can either:
1. Make `callHook` an input/output parameter to these functions, OR
2. Make `callHook` a member of Debugger, make sure it is set to true before we start, and make sure it is
   set back to true after we finish (possibly using some RAII construct).

My preference goes out to the latter. Jim, what is your opinion? If you disagree, we can discuss during the meeting today.
Flags: needinfo?(jimb)
I think it isn't strictly necessary that a resumption value parsing function must pass the exceptions it raises to handleUncaughtException itself. It happens to do this because it's often called as the last step before returning to the debuggee after calling a Debugger handler function.

But a resumption value parsing function could simply be fallible in the usual way: set an exception on the context, and return false. If a C++ onStep handler called such a fallible parsing function, and returned false, then that failure ought to be swept up in the handleUncaughtException machinery just like anything else must be.

I guess that counts as "disagree"? Yes, let's discuss today after other shorter business.
Flags: needinfo?(jimb)
This patch factors out those parts of parseResumptionValueHelper that actually deal with parsing the resumption value into a separate function called ParseResumptionValue.
Attachment #8780199 - Flags: review?(jimb)
Attached patch Factor out CheckResumptionValue. (obsolete) — Splinter Review
This patch factors out those parts of parseResumptionValueHelper that actually deal with checking the resumption value into a separate function called CheckResumptionValue.
Attachment #8780202 - Flags: review?(jimb)
This patch factors out those parts of parseResumptionValueHelper that are used by both parseResumptionValue and handleUncaughtException into a separate function called processResumptionValue.
Attachment #8780204 - Flags: review?(jimb)
This patch replaces the call to parseResumptionValueHelper with a call to processResumptionValueHelper in handleUncaughtException.

Unlike parseResumptionValueHelper, which calls handleUncaughtException recursively if an error occurs, processResumptionValue returns false if an error occurs.

If processResumptionValue fails, we should not try to call the uncaught exception hook again, so we can simply fall through to the rest of the function.
Attachment #8780208 - Flags: review?(jimb)
The only reason we needed the callHook argument in parseResumptionValueHelper was that we called it from handleUncaughtException. If this call to parseResumptionValueHelper ended up calling handleUncaughtException again, we had to make sure that we wouldn't try to call the exception handler hook again.

Now that we no longer call parseResumptionValueHelper from uncaughtExceptionHook, the callHook argument in the former is no longer necessary.
Attachment #8780210 - Flags: review?(jimb)
Jim, how do you feel about replacing handleUncaughtException with two functions:
1. handleUncaughtExceptionWithHook
2. handleUncaughtExceptionWithoutHook

I find this much clearer than something like handleUncaughtException(ac, false), which is what we have now.

I'd also like to propose renaming parseResumptionValue to processSuccessAndResumptionValue. In my opinion that is a much better description of what the function does (although perhaps a bit too verbose. Let me know if you can come up with something better!).
Flags: needinfo?(jimb)
(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> Jim, how do you feel about replacing handleUncaughtException with two
> functions:
> 1. handleUncaughtExceptionWithHook
> 2. handleUncaughtExceptionWithoutHook
> 
> I find this much clearer than something like handleUncaughtException(ac,
> false), which is what we have now.

This split is okay; I'd rather have them called handleUncaughtException and reportUncaughtException: "handle" should be enough to imply "... with the handler you set via Debugger.prototype.uncaughtExceptionHook". (It'd be better if we could rename that "uncaughtExceptionHandler", but that can be another bug.)

It sure would be great if there were some nice decomposition such that handleUncaughtException didn't have to deal with reporting via ReportExceptionClosure, and reportUncaughtException was the only thing that did that, and the two composed in some clean and obvious way:

    return handleUncaughtException(...) ||
           reportUncaughtException(...);

but I have no idea whether the details permit that.

> I'd also like to propose renaming parseResumptionValue to
> processSuccessAndResumptionValue. In my opinion that is a much better
> description of what the function does (although perhaps a bit too verbose.
> Let me know if you can come up with something better!).

I don't like this change: only successful handlers can return a resumption value anyway, so if we've got a resumption value, it obviously succeeded.
Flags: needinfo?(jimb)
(In reply to Jim Blandy :jimb from comment #11)
> I don't like this change: only successful handlers can return a resumption
> value anyway, so if we've got a resumption value, it obviously succeeded.

Except that is not how things are implemented: parseResumptionValue is called even if the handler failed. It takes the success code of the handler as a parameter, so it can call handleUncaughtException on its behalf.

One could argue that you shouldn't do that; an alternative would be to replace all code where we call parseResumptionValue with something like:

RootedValue rval(cx);
JSTrapStatus status;
RootedValue value(cx);
if (!js::Call(cx, fval, ... &rval) ||
    !processResumptionValue(cx, iter.abstractFramePtr(), iter.pc(), rval, &status, &value))
{
   status = handleUncaughtException(ac, &value, true, maybeThisv, frame);
}

This would allow us to completely replace parseResumptionValue with processResumptionValue.
Comment on attachment 8780199 [details] [diff] [review]
Factor out ParseResumptionValue.

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

There doesn't seem to be much point in keeping ParseResumptionValueAsObject separate from ParseResumptionValue any more. It makes plenty of sense to just have all the cases for a resumption value in one function.

::: js/src/vm/Debugger.cpp
@@ +1504,5 @@
>      return true;
>  }
>  
> +static bool
> +ParseResumptionValue(JSContext* cx, HandleValue rval, JSTrapStatus* statusp, MutableHandleValue vp)

Let's follow SpiderMonkey practice of using `&` for pointers that can't be null, so: `JSTrapStatus&`.

@@ -1508,5 @@
>  Debugger::parseResumptionValueHelper(Maybe<AutoCompartment>& ac, bool ok, const Value& rv,
>                                       const Maybe<HandleValue>& thisVForCheck, AbstractFramePtr frame,
>                                       MutableHandleValue vp, bool callHook)
>  {
> -    vp.setUndefined();

It makes me a bit nervous to take out this initialization. If I'm reading right, handleUncaughtException only writes to vp if this function does. Granted, callers shouldn't be looking at vp unless the JSTrapStatus says they should, but who knows.
Attachment #8780199 - Flags: review?(jimb) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #12)
> (In reply to Jim Blandy :jimb from comment #11)
> > I don't like this change: only successful handlers can return a resumption
> > value anyway, so if we've got a resumption value, it obviously succeeded.
> 
> Except that is not how things are implemented: parseResumptionValue is
> called even if the handler failed. It takes the success code of the handler
> as a parameter, so it can call handleUncaughtException on its behalf.

Okay, I see what you mean now.

I do think processSuccessAndResumptionValue is too long, though. How about processHandlerResult?  "Result" is meant to cover both success and failure, so "handler result" to refer to the idiom where we've got a boolean success indicator, a return value that is a resumption value, and a JSContext possibly holding an exception.
Comment on attachment 8780202 [details] [diff] [review]
Factor out CheckResumptionValue.

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

::: js/src/vm/Debugger.cpp
@@ +1524,5 @@
> +CheckResumptionValue(JSContext* cx, AbstractFramePtr frame, const Maybe<HandleValue>& maybeThisv,
> +                     JSTrapStatus* statusp, MutableHandleValue vp)
> +{
> +    if (maybeThisv.isSome()) {
> +        const HandleValue& thisv = maybeThisv.ref();

Doesn't this line belong inside the `vp.isUndefined` "then" block? It's only used there. If you put it there, then the outer condition (checking status, thisVForCheck, and v.isPrimitive()) can stay the same as in the original.
Attachment #8780202 - Flags: review?(jimb) → review+
Comment on attachment 8780202 [details] [diff] [review]
Factor out CheckResumptionValue.

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

Actually, I'm sorry --- Now I'm pretty sure this patch isn't correct.

When status is JSTRAP_RETURN, thisVForCheck is Some and not JS_UNITIALIZED_LOCAL, and v is undefined [deep breath] then we store thisVForCheck in v.

In the existing code, we do this after we call unwrapDebuggeeValue on v. This is fine, because thisVForCheck is also an unwrapped debuggee value.

With your patch applied, we store thisVForCheck.ref() in v *before* we call unwrapDebuggeeValue on v, and end up applying unwrapDebuggeeValue on an unwrapped debuggee value. That should elicit an error from Debugger::unwrapDebuggeeObject.

Or so it seems to me. If that's so, please include a test that fails when run against the incorrect patch.
Attachment #8780202 - Flags: review+ → review-
Comment on attachment 8780204 [details] [diff] [review]
Factor our processResumptionValue.

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

Looks good.
Attachment #8780204 - Flags: review?(jimb) → review+
Attachment #8780208 - Flags: review?(jimb) → review+
Attachment #8780210 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #13)
> Comment on attachment 8780199 [details] [diff] [review]
> Factor out ParseResumptionValue.
> 
> Review of attachment 8780199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There doesn't seem to be much point in keeping ParseResumptionValueAsObject
> separate from ParseResumptionValue any more. It makes plenty of sense to
> just have all the cases for a resumption value in one function.
> 

Ok, I will merge ParseResumptionValueAsObject into ParseResumptionValue.

> ::: js/src/vm/Debugger.cpp
> @@ +1504,5 @@
> >      return true;
> >  }
> >  
> > +static bool
> > +ParseResumptionValue(JSContext* cx, HandleValue rval, JSTrapStatus* statusp, MutableHandleValue vp)
> 
> Let's follow SpiderMonkey practice of using `&` for pointers that can't be
> null, so: `JSTrapStatus&`.
> 

That would be inconsistent with existing uses of JSTrapStatus* statusp in Debugger.cpp. I agree with the change, but let's do it in a separate patch.

> @@ -1508,5 @@
> >  Debugger::parseResumptionValueHelper(Maybe<AutoCompartment>& ac, bool ok, const Value& rv,
> >                                       const Maybe<HandleValue>& thisVForCheck, AbstractFramePtr frame,
> >                                       MutableHandleValue vp, bool callHook)
> >  {
> > -    vp.setUndefined();
> 
> It makes me a bit nervous to take out this initialization. If I'm reading
> right, handleUncaughtException only writes to vp if this function does.
> Granted, callers shouldn't be looking at vp unless the JSTrapStatus says
> they should, but who knows.

Some recent fuzz bugs have taught me to appreciate your paranoia ;-) Very well, I will put back the initialization.
> Comment on attachment 8780202 [details] [diff] [review]
> Factor out CheckResumptionValue.
> 
> Review of attachment 8780202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, I'm sorry --- Now I'm pretty sure this patch isn't correct.
> 
> When status is JSTRAP_RETURN, thisVForCheck is Some and not
> JS_UNITIALIZED_LOCAL, and v is undefined [deep breath] then we store
> thisVForCheck in v.
> 
> In the existing code, we do this after we call unwrapDebuggeeValue on v.
> This is fine, because thisVForCheck is also an unwrapped debuggee value.
> 
> With your patch applied, we store thisVForCheck.ref() in v *before* we call
> unwrapDebuggeeValue on v, and end up applying unwrapDebuggeeValue on an
> unwrapped debuggee value. That should elicit an error from
> Debugger::unwrapDebuggeeObject.
> 
> Or so it seems to me. If that's so, please include a test that fails when
> run against the incorrect patch.

You are absolutely right. Good catch!

I've added the test you requested.
Attachment #8780210 - Attachment description: Bug 1294013 - Remove callHook from parseResumptionValue(Helper). → Remove callHook from parseResumptionValue(Helper).
Attached patch Factor our CheckResumptionValue. (obsolete) — Splinter Review
Attachment #8780202 - Attachment is obsolete: true
Attachment #8780905 - Flags: review?(jimb)
Minor patch update: CheckResumptionValue never assigns to statusp, so we can change this from JSTrapStatus* statusp into JSTrapStatus status.
Attachment #8780905 - Attachment is obsolete: true
Attachment #8780905 - Flags: review?(jimb)
Attachment #8780906 - Flags: review?(jimb)
Ok, so it turns out that the only place where we already used JSTrapStatus* was in GetStatusProperty (in my defense, that means there was *some* precedent for doing things the way I did! :-P)

Since there is at least one existing function that already uses JSTrapStatus*, it's probably still best to replace all instances of JSTrapStatus* with JSTrapStatus& in a single patch.
Attachment #8780907 - Flags: review?(jimb)
Attachment #8780911 - Flags: review?(jimb)
Now that we call reportUncaughtException in those places where we don't want to call the exception handler hook, we no longer need the callHook argument in handleUncaughtException (since it is always true).
Attachment #8780912 - Flags: review?(jimb)
I started working on the patch to implement a C++ interface for onStep again (bug 1271650).

While working on that patch, I realised that, like Debugger::processHandlerValue, Debugger::onSingleStep will also need to obtain the this value for the current frame (iff that frame is for a call to a derived constructor), so it can check it against the resumption value if necessary.

Since the code to get the this value will be shared between those two functions, it seems like a good idea to factor that out into a its own function. And since we're already refactoring the logic for parsing resumption values in this bug, this seems like the appropriate place to do it.
Attachment #8780931 - Flags: review?(jimb)
Sorry for the large number of r?'s Jim! I have more patches ready, but I'll give you a chance to catch up before posting additional patches. In the meantime, I'm going to start looking into bug 1288084.
Comment on attachment 8780907 [details] [diff] [review]
Replace JSTrapStatus* with JSTrapStatus&.

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

Looks great. Thanks.
Attachment #8780907 - Flags: review?(jimb) → review+
Attachment #8780906 - Flags: review?(jimb) → review+
Comment on attachment 8780911 [details] [diff] [review]
Factor our reportUncaughtException.

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

Looks great.

::: js/src/vm/Debugger.cpp
@@ +1324,5 @@
> +
> +    // Uncaught exceptions arise from Debugger code, and so we must already be
> +    // in an NX section.
> +
> +    MOZ_ASSERT(EnterDebuggeeNoExecute::isLockedInStack(cx, *this));

The blank line belongs after the MOZ_ASSERT, not between it and its comment.
Attachment #8780911 - Flags: review?(jimb) → review+
Attachment #8780912 - Flags: review?(jimb) → review+
Comment on attachment 8780931 [details] [diff] [review]
Factor out GetThisValueForCheck.

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

I think I want to hold off on this. I am expecting Debugger::onSingleStep to use the same functions for checking and applying resumption values that other hook invocation spots do; it's not clear to me why it would need to start managing 'this' values itself.
Attachment #8780931 - Flags: review?(jimb)
(In reply to Jim Blandy :jimb from comment #29)
> Comment on attachment 8780931 [details] [diff] [review]
> Factor out GetThisValueForCheck.
> 
> Review of attachment 8780931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I want to hold off on this. I am expecting Debugger::onSingleStep to
> use the same functions for checking and applying resumption values that
> other hook invocation spots do; it's not clear to me why it would need to
> start managing 'this' values itself.

I've attached a WIP patch to implement a C++ interface for DebuggerFrame.onStep in bug 1271650. Hopefully, that will clarify why we need this patch.
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33283d086876
Factor out processResumptionValue. r=jimb
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0289f779f30b
Replace parseResumptionValueHelper with processResumptionValue in handleUncaughtException. r=jimb
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d14511be496e
Remove callHook from parseResumptionValue(Helper). r=jimb
Sorry had to backout your push for Window build bustage, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=34015895&repo=mozilla-inbound#L24584
Flags: needinfo?(ejpbruel)
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0e2ac69f72
[jsdbg2] Disentangle parsing and checking of resumption values. r=jimb
I'm sorry, it seems that’s an issue with an aws data center, I’ve re-pushed your patches. Please let me know if any further questions.
Flags: needinfo?(ejpbruel)
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d618b94e6612
Factor out CheckResumptionValue. r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed10cf2e100
Factor out processResumptionValue. r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23f82bf0443
Replace parseResumptionValueHelper with processResumptionValue in handleUncaughtException. r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/111cf77cead5
[jsdbg2] Disentangle parsing and checking of resumption values Part 2. r=jimb
New try push for the last 4 patches, with issues addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57a12bc16174
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d8be8150aa
Replace JSTrapStatus* with JSTrapStatus&. r=jimb
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7437baa4d459
Rename parseResumptionValue to processHandlerResult. r=jimb
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00b014a3016
Factor out reportUncaughtException. r=jimb
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7bd9803e10
Remove callHook from handleUncaughtException(Helper). r=jimb
You need to log in before you can comment on or make changes to this bug.