[jsdbg2] Disentangle parsing and checking of resumption values.

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(9 attachments, 2 obsolete attachments)

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
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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)
(Assignee)

Comment 2

2 years ago
(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)
(Assignee)

Comment 3

2 years ago
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)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
Created attachment 8780199 [details] [diff] [review]
Factor out ParseResumptionValue.

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)
(Assignee)

Comment 6

2 years ago
Created attachment 8780202 [details] [diff] [review]
Factor out CheckResumptionValue.

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)
(Assignee)

Comment 7

2 years ago
Created attachment 8780204 [details] [diff] [review]
Factor our processResumptionValue.

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)
(Assignee)

Comment 8

2 years ago
Created attachment 8780208 [details] [diff] [review]
Replace parseResumptionValueHelper with processResumptionValue in handleUncaughtException.

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)
(Assignee)

Comment 9

2 years ago
Created attachment 8780210 [details] [diff] [review]
Remove callHook from parseResumptionValue(Helper).

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)
(Assignee)

Comment 10

2 years ago
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)

Comment 11

2 years ago
(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)
(Assignee)

Comment 12

2 years ago
(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 13

2 years ago
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+

Comment 14

2 years ago
(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 15

2 years ago
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 16

2 years ago
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 17

2 years ago
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+

Updated

2 years ago
Attachment #8780208 - Flags: review?(jimb) → review+

Updated

2 years ago
Attachment #8780210 - Flags: review?(jimb) → review+
(Assignee)

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

2 years ago
> 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.
(Assignee)

Updated

2 years ago
Attachment #8780210 - Attachment description: Bug 1294013 - Remove callHook from parseResumptionValue(Helper). → Remove callHook from parseResumptionValue(Helper).
(Assignee)

Comment 20

2 years ago
Created attachment 8780905 [details] [diff] [review]
Factor our CheckResumptionValue.
Attachment #8780202 - Attachment is obsolete: true
Attachment #8780905 - Flags: review?(jimb)
(Assignee)

Comment 21

2 years ago
Created attachment 8780906 [details] [diff] [review]
Factor our CheckResumptionValue.

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)
(Assignee)

Comment 22

2 years ago
Created attachment 8780907 [details] [diff] [review]
Replace JSTrapStatus* with JSTrapStatus&.

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)
(Assignee)

Comment 23

2 years ago
Created attachment 8780911 [details] [diff] [review]
Factor our reportUncaughtException.
Attachment #8780911 - Flags: review?(jimb)
(Assignee)

Comment 24

2 years ago
Created attachment 8780912 [details] [diff] [review]
Remove callHook from handleUncaughtException(Helper).

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)
(Assignee)

Comment 25

2 years ago
Created attachment 8780931 [details] [diff] [review]
Factor out GetThisValueForCheck.

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)
(Assignee)

Comment 26

2 years ago
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 27

2 years ago
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+

Updated

2 years ago
Attachment #8780906 - Flags: review?(jimb) → review+

Comment 28

2 years ago
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+

Updated

2 years ago
Attachment #8780912 - Flags: review?(jimb) → review+

Comment 29

2 years ago
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)
(Assignee)

Comment 30

2 years ago
(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.

Comment 32

2 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ead64ff09e8
Factor out ParseResumptionValue. r=jimb

Comment 33

2 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eec5a814230
Factor out CheckResumptionValue. r=jimb

Comment 34

2 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33283d086876
Factor out processResumptionValue. r=jimb

Comment 35

2 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0289f779f30b
Replace parseResumptionValueHelper with processResumptionValue in handleUncaughtException. r=jimb

Comment 36

2 years ago
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)

Comment 40

2 years ago
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)

Comment 42

2 years ago
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
(Assignee)

Comment 44

2 years ago
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 → ---

Comment 45

2 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d8be8150aa
Replace JSTrapStatus* with JSTrapStatus&. r=jimb

Comment 46

2 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7437baa4d459
Rename parseResumptionValue to processHandlerResult. r=jimb

Comment 47

2 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00b014a3016
Factor out reportUncaughtException. r=jimb

Comment 48

2 years ago
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.