Closed Bug 1083210 Opened 5 years ago Closed 5 years ago

[jsdbg2] Add a Debugger.prototype.onNewPromise hook

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(2 files, 8 obsolete files)

No description provided.
Attached patch on-new-promise-pt-1.patch (obsolete) — Splinter Review
Attachment #8506419 - Flags: review?(jimb)
Attached patch on-new-promise-pt-2.patch (obsolete) — Splinter Review
Attachment #8506420 - Flags: review?(bzbarsky)
Comment on attachment 8506420 [details] [diff] [review]
on-new-promise-pt-2.patch

r=me
Attachment #8506420 - Flags: review?(bzbarsky) → review+
Attached patch on-new-promise-pt-1.patch (obsolete) — Splinter Review
Rebased on latest m-c.
Attachment #8506419 - Attachment is obsolete: true
Attachment #8506419 - Flags: review?(jimb)
Attachment #8506578 - Flags: review?(jimb)
Attached patch on-new-promise-pt-2.patch (obsolete) — Splinter Review
Rebased on latest m-c. Carrying over r+.
Attachment #8506420 - Attachment is obsolete: true
Attachment #8506579 - Flags: review+
Attached patch on-new-promise-pt-1.patch (obsolete) — Splinter Review
Woops, bad rebase.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=304ad19cd9db
Attachment #8506578 - Attachment is obsolete: true
Attachment #8506578 - Flags: review?(jimb)
Attachment #8506841 - Flags: review?(jimb)
Gah, looks like windows can't find where I've actually implemented JS::dbg::onNewPromise. Any tips?
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.
No longer blocks: 1033153
(Jim: review ping -- it's been 20 days)
Attachment #8506841 - Flags: review?(jimb) → review?(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)?
Attachment #8506841 - Flags: review?(shu)
Attached patch on-new-promise-pt-1.patch (obsolete) — Splinter Review
Using dispatchHook now, but it isn't super pretty.
Attachment #8506841 - Attachment is obsolete: true
Attachment #8520985 - Flags: review?(shu)
Attached patch on-new-promise-pt-2.patch (obsolete) — Splinter Review
Rebased + carrying over r+.
Attachment #8506579 - Attachment is obsolete: true
Attachment #8520987 - Flags: review+
Attached patch on-new-promise-pt-1.patch (obsolete) — Splinter Review
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
Attachment #8520985 - Attachment is obsolete: true
Attachment #8520985 - Flags: review?(shu)
Attachment #8521052 - Flags: review?(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().
Attachment #8521052 - Flags: review?(shu) → review+
Carrying over r=shu.
Attachment #8521052 - Attachment is obsolete: true
Attachment #8523994 - Flags: review+
Rebased. Carrying over r=bz.
Attachment #8520987 - Attachment is obsolete: true
Attachment #8523997 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9fadf0d13882
https://hg.mozilla.org/mozilla-central/rev/f79910e319fd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.