Closed
Bug 1083210
Opened 10 years ago
Closed 10 years ago
[jsdbg2] Add a Debugger.prototype.onNewPromise hook
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(2 files, 8 obsolete files)
26.57 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
3.62 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8506419 -
Flags: review?(jimb)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8506420 -
Flags: review?(bzbarsky)
Comment 3•10 years ago
|
||
Comment on attachment 8506420 [details] [diff] [review] on-new-promise-pt-2.patch r=me
Attachment #8506420 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Rebased on latest m-c.
Attachment #8506419 -
Attachment is obsolete: true
Attachment #8506419 -
Flags: review?(jimb)
Attachment #8506578 -
Flags: review?(jimb)
Assignee | ||
Comment 5•10 years ago
|
||
Rebased on latest m-c. Carrying over r+.
Attachment #8506420 -
Attachment is obsolete: true
Attachment #8506579 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8cf544026732
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
Gah, looks like windows can't find where I've actually implemented JS::dbg::onNewPromise. Any tips?
Comment hidden (typo) |
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
(Jim: review ping -- it's been 20 days)
Assignee | ||
Updated•10 years ago
|
Attachment #8506841 -
Flags: review?(jimb) → review?(shu)
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
Using dispatchHook now, but it isn't super pretty.
Attachment #8506841 -
Attachment is obsolete: true
Attachment #8520985 -
Flags: review?(shu)
Assignee | ||
Comment 14•10 years ago
|
||
Rebased + carrying over r+.
Attachment #8506579 -
Attachment is obsolete: true
Attachment #8520987 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=295c82ad421a
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
Carrying over r=shu.
Attachment #8521052 -
Attachment is obsolete: true
Attachment #8523994 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
Rebased. Carrying over r=bz.
Attachment #8520987 -
Attachment is obsolete: true
Attachment #8523997 -
Flags: review+
Assignee | ||
Comment 20•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 21•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fadf0d13882 https://hg.mozilla.org/integration/mozilla-inbound/rev/f79910e319fd
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9fadf0d13882 https://hg.mozilla.org/mozilla-central/rev/f79910e319fd
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
•