1084065 - [jsdbg2] Add a Debugger.prototype.onPromiseSettled hook

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Where "settled" != "resolved" (aka fate decided) and instead wait until its state is either fulfilled or rejected.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: on-promise-settled-pt-1.patch

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: on-promise-settled-pt-2.patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8507152 [details] [diff] [review]
on-promise-settled-pt-2.patch

I don't think you need the cx-stack behavior ThreadsafeAutoJSContext has here.  Just use AutoJSAPI?

I think Settle() should passed both the state and the value and do he SetValue call itself.

r=me with that

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(Jim: review ping -- it's been 20 days)

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8507150 [details] [diff] [review]
on-promise-settled-pt-1.patch

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

::: js/src/jit-test/tests/debug/Debugger-onPromiseSettled-02.js
@@ +9,5 @@
> +assertEq(log, '');
> +
> +dbg.onPromiseSettled = function (promise) {
> +  log += 's';
> +  assertEq(promise.seen, undefined);

Is this assert checking that onPromiseSettled only get called once per promise?

::: js/src/vm/Debugger.cpp
@@ +1568,5 @@
>  }
>  
> +JSTrapStatus
> +Debugger::firePromiseHook(JSContext *cx, Hook hook, HandleObject promise, MutableHandleValue vp)
> +{

Could you add an assert here that |hook| is one of the promise hooks?

@@ +1601,2 @@
>  /* static */ void
> +Debugger::slowPathPromiseHook(JSContext *cx, GlobalObject::DebuggerVector &dbgs, Hook hook,

Why is it okay to take in DebuggerVector here, instead of going through Debugger::dispatchHook?

In dispatchHook, the idea is to make a copy of the list of receiver Debuggers, since the hook itself may mutate the global's list of Debuggers.

@@ +1607,5 @@
>  
>      JSTrapStatus status = JSTRAP_CONTINUE;
>      RootedValue value(cx);
>  
>      for (Debugger **dbgp = dbgs.begin(); dbgp < dbgs.end(); dbgp++) {

dbgp != dbgs.end()

@@ +6799,5 @@
> +    ASSERT_PROMISE(cx, promise);
> +    GlobalObject::DebuggerVector *dbgs = promise->global().getDebuggers();
> +    if (!dbgs || dbgs->length() == 0)
> +        return;
> +    Debugger::slowPathOnPromiseSettled(cx, *dbgs, promise);

See comment above about going through dispatchHook. It's weird to me this JS:: function gets out a DebuggerVector and passes it.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: on-promise-settled-pt-1.patch

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: on-promise-settled-pt-2.patch

bz, it seems like things have changed a little bit since I wrote this patch. Would you mind taking another quick look? Thanks!

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7f50b056c267

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8521054 [details] [diff] [review]
on-promise-settled-pt-2.patch

r=me

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8521053 [details] [diff] [review]
on-promise-settled-pt-1.patch

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

::: js/src/builtin/TestingFunctions.cpp
@@ +2287,5 @@
>  "  any of the other behavior associated with promises."),
>  
> +    JS_FN_HELP("settleFakePromise", SettleFakePromise, 1, 0,
> +"settleFakePromise(promise)",
> +"  'Settle' a 'promise' created by makeFakePromise(). This doesn't have any\n"

lol

::: js/src/vm/Debugger.cpp
@@ +6790,5 @@
>      return true;
>  }
>  
> +static inline void
> +assertPromise(JSContext *cx, HandleObject promise)

Nit: capitalize static functions. I'd probably rename this to AssertIsPromise.

@@ +6797,3 @@
>      assertSameCompartment(cx, promise);
>      MOZ_ASSERT(strcmp(promise->getClass()->name, "Promise") == 0 ||
>                 strcmp(promise->getClass()->name, "MozAbortablePromise") == 0);

Oh I just noticed: can this not do direct class pointer comparisons? Are there multiple fake/real promise classes all with "Promise" as the string?

@@ +6804,5 @@
> +{
> +    assertPromise(cx, promise);
> +    GlobalObject::DebuggerVector *dbgs = promise->global().getDebuggers();
> +    if (!dbgs || dbgs->length() == 0)
> +        return;

No need to double check for debuggers.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: on-promise-settled-pt-1.patch

Carrying over r=shu.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: on-promise-settled-pt-2.patch

Rebased. Carrying over r=bz.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=baa4fccca5bb

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0fd69c402d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff8b0877262

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d0fd69c402d
https://hg.mozilla.org/mozilla-central/rev/3ff8b0877262