Closed Bug 1084065 Opened 10 years ago Closed 10 years ago

[jsdbg2] Add a Debugger.prototype.onPromiseSettled hook

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(2 files, 4 obsolete files)

Where "settled" != "resolved" (aka fate decided) and instead wait until its state is either fulfilled or rejected.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Attached patch on-promise-settled-pt-1.patch (obsolete) — Splinter Review
Attachment #8507150 - Flags: review?(jimb)
Attached patch on-promise-settled-pt-2.patch (obsolete) — Splinter Review
Attachment #8507152 - Flags: review?(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
Attachment #8507152 - Flags: review?(bzbarsky) → review+
No longer blocks: 1033153
(Jim: review ping -- it's been 20 days)
Attachment #8507150 - Flags: review?(jimb) → review?(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.
Attachment #8507150 - Flags: review?(shu)
Attached patch on-promise-settled-pt-1.patch (obsolete) — Splinter Review
Attachment #8507150 - Attachment is obsolete: true
Attachment #8521053 - Flags: review?(shu)
Attached patch on-promise-settled-pt-2.patch (obsolete) — Splinter Review
bz, it seems like things have changed a little bit since I wrote this patch. Would you mind taking another quick look? Thanks!
Attachment #8507152 - Attachment is obsolete: true
Attachment #8521054 - Flags: review?(bzbarsky)
Comment on attachment 8521054 [details] [diff] [review]
on-promise-settled-pt-2.patch

r=me
Attachment #8521054 - Flags: review?(bzbarsky) → review+
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.
Attachment #8521053 - Flags: review?(shu) → review+
Carrying over r=shu.
Attachment #8521053 - Attachment is obsolete: true
Attachment #8523999 - Flags: review+
Rebased. Carrying over r=bz.
Attachment #8521054 - Attachment is obsolete: true
Attachment #8524000 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1d0fd69c402d
https://hg.mozilla.org/mozilla-central/rev/3ff8b0877262
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1100923
You need to log in before you can comment on or make changes to this bug.