Closed
Bug 1084065
Opened 10 years ago
Closed 10 years ago
[jsdbg2] Add a Debugger.prototype.onPromiseSettled hook
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(2 files, 4 obsolete files)
21.88 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
7.27 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
Where "settled" != "resolved" (aka fate decided) and instead wait until its state is either fulfilled or rejected.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8507150 -
Flags: review?(jimb)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8507152 -
Flags: review?(bzbarsky)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
(Jim: review ping -- it's been 20 days)
Assignee | ||
Updated•10 years ago
|
Attachment #8507150 -
Flags: review?(jimb) → review?(shu)
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8507150 -
Attachment is obsolete: true
Attachment #8521053 -
Flags: review?(shu)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7f50b056c267
Comment 9•10 years ago
|
||
Comment on attachment 8521054 [details] [diff] [review] on-promise-settled-pt-2.patch r=me
Attachment #8521054 -
Flags: review?(bzbarsky) → review+
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Carrying over r=shu.
Attachment #8521053 -
Attachment is obsolete: true
Attachment #8523999 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
Rebased. Carrying over r=bz.
Attachment #8521054 -
Attachment is obsolete: true
Attachment #8524000 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=baa4fccca5bb
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0fd69c402d https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff8b0877262
Keywords: checkin-needed
Comment 15•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•