1083210 - [jsdbg2] Add a Debugger.prototype.onNewPromise hook

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

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

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

Comment 2

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

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8506420 [details] [diff] [review]
on-new-promise-pt-2.patch

r=me

Comment 4

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

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

Rebased on latest m-c.

Comment 5

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

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

Rebased on latest m-c. Carrying over r+.

Comment 6

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

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8cf544026732

Comment 7

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

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

Woops, bad rebase.

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

Comment 8

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

Gah, looks like windows can't find where I've actually implemented JS::dbg::onNewPromise. Any tips?

Comment 9

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

Gah, looks like the windows builds can't find where I've actually implemented JS::dbg::onNewPromise.

Any tips?

Comment 10

[Boris Zbarsky [:bzbarsky]]

On Windows, js and libxul are separate libraries, no?

Which means that JS apis meant for calling from outside JS need to either be inline or exported from the module via JS_FRIEND_API or JS_PUBLIC_API.

Comment 11

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

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

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8506841 [details] [diff] [review]
on-new-promise-pt-1.patch

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

::: js/src/builtin/TestingFunctions.cpp
@@ +973,5 @@
> +static bool
> +MakeFakePromise(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    RootedObject scope(cx, JS::CurrentGlobalOrNull(cx));

When would this be called with a compartmentless cx? Can this simply be cx->global()?

::: js/src/doc/Debugger/Debugger.md
@@ +87,5 @@
>  
>      This method's return value is ignored.
>  
> +<code>onNewPromise(<i>promise</i>, <i>global</i>)</code>
> +:   A new Promise object, referrenced by the [`Debugger.Object`][object]

typo: referenced

@@ +89,5 @@
>  
> +<code>onNewPromise(<i>promise</i>, <i>global</i>)</code>
> +:   A new Promise object, referrenced by the [`Debugger.Object`][object]
> +    instance *promise*, has been allocated in the scope of the debuggee global
> +    object referrenced by the *global* [`Debugger.Object`][object] instance.

typo: referenced

@@ +92,5 @@
> +    instance *promise*, has been allocated in the scope of the debuggee global
> +    object referrenced by the *global* [`Debugger.Object`][object] instance.
> +
> +    This handler method should return a [resumption value][rv] specifying how
> +    the debuggee's execution should proceed. However, note that a <code>{ return:

Design question: why should onNewPromise return resumption values at all? It's not really a control-flow related trap.

::: js/src/vm/Debugger.cpp
@@ +1567,5 @@
>      allocationsLogLength = 0;
>  }
>  
> +/* static */ void
> +Debugger::slowPathOnNewPromise(JSContext *cx, GlobalObject::DebuggerVector &dbgs,

Same question as the one on bug 1084065: why is it okay for this to take a DebuggerVector and not do the copy that dispatchHook does? Or rather, why isn't this just using dispatchHook (after patching it to support the promise hooks)?

Comment 13

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

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

Using dispatchHook now, but it isn't super pretty.

Comment 14

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

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

Rebased + carrying over r+.

Comment 15

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

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

Comment 16

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

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

Fixing r=shu in commit message and also bug where we have MozAbortablePromise that was asserting when passed to JS::dbg::onNewPromise.

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

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8521052 [details] [diff] [review]
on-new-promise-pt-1.patch

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

::: js/src/doc/Debugger/Debugger.md
@@ +95,5 @@
> +    This handler method should return a [resumption value][rv] specifying how
> +    the debuggee's execution should proceed. However, note that a <code>{ return:
> +    <i>value</i> }</code> resumption value is treated like `undefined` ("continue
> +    normally"); <i>value</i> is ignored. (Allowing the handler to substitute
> +    its own value for the new global object doesn't seem useful.)

The implementation below of the onNewPromise hook is 1-ary, but here it says it's 2-ary. Is the doc wrong?

Seems to be similarly wrong for onNewScript. I don't see any global passed to the handler in the implementation.

::: js/src/vm/Debugger.cpp
@@ +671,5 @@
>  Debugger::slowPathOnExceptionUnwind(JSContext *cx, AbstractFramePtr frame)
>  {
>      RootedValue rval(cx);
> +    RootedObject payload(cx);
> +    JSTrapStatus status = dispatchHook(cx, &rval, OnExceptionUnwind, payload);

You can pass in NullPtr() here for payload.

@@ +6782,5 @@
> +               strcmp(promise->getClass()->name, "MozAbortablePromise") == 0);
> +
> +    GlobalObject::DebuggerVector *dbgs = promise->global().getDebuggers();
> +    if (!dbgs || dbgs->length() == 0)
> +        return;

No need to double check here, dispatchHook checks for presence of debuggers on the global.

::: js/src/vm/Debugger.h
@@ +812,5 @@
>  Debugger::onDebuggerStatement(JSContext *cx, MutableHandleValue vp)
>  {
> +    if (cx->compartment()->debugMode()) {
> +        RootedObject payload(cx);
> +        return dispatchHook(cx, vp, OnDebuggerStatement, payload);

Ditto on NullPtr().

Comment 18

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

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

Carrying over r=shu.

Comment 19

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

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

Rebased. Carrying over r=bz.

Comment 20

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

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fadf0d13882
https://hg.mozilla.org/integration/mozilla-inbound/rev/f79910e319fd

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fadf0d13882
https://hg.mozilla.org/mozilla-central/rev/f79910e319fd