Promise then-handlers can still be executed while the debugger is paused.

RESOLVED FIXED in Firefox 67

Status

defect
P1
normal
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: bdahl, Assigned: jimb)

Tracking

(Blocks 1 bug)

unspecified
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [devtools-platform])

Attachments

(10 attachments, 13 obsolete attachments)

436 bytes, text/html
Details
2.33 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
jimb
: checkin+
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Posted file promise.html (obsolete) —
STR:
1) Open debugger
2) Open attached promise.html
3) Observe the debugger stopping on the first 'debugger;' statement
4) Observe "This should NOT print." is printed to the console while the debugger is still on the first debugger statement.

This makes debugging js programs that use a resolved promise for the next tick extremely difficult to reason about.

I'm guessing this is similar to bug 1124122.
Component: Developer Tools → Developer Tools: Debugger
I just tried to reproduce and I couldn't. What I see is the debugger pausing on the second 'debugger;' statement, then I resume and console.log is executed and then it pauses on the second 'debugger;' statement. I'm using the latest Nightly on OS X. If you can still see this on the latest Nightly, can you tell me more about your system?
Flags: needinfo?(bdahl)
The behavior seems to have changed on nightly, though I still can reproduce in the program I'm working on. I'll leave the need info so when I get some more time I can try to tweak the example to reproduce again.

This may be a different bug, but shouldn't the debugger still be stopping on the first 'debugger;' statement(I thought runnables were guaranteed to run in order?).
Here's a better test. You can see that the values pushed into the window.out array become intermingled while the debugger is open. It should be solid [A,A,B,B].  Very similar to bug 1157861
Attachment #8580104 - Attachment is obsolete: true
Flags: needinfo?(bdahl)
Whiteboard: [devtools-platform]
Assignee: nobody → ejpbruel
Priority: -- → P1
Summary: All script execution does not stop on debugger statement with promises. → Promise then-handlers can still be executed while the debugger is paused.
Here is an xpcshell test that uses the DebuggeeWouldRun feature that Shu implemented recently: https://bugzilla.mozilla.org/show_bug.cgi?id=1074448#c7

It pauses the debugger, then evaluates, Promise.resolve().then(function () {}). Since the debugger is paused, the then-handler should not be executed until the debugger unpauses, but it seems like it does.
Attachment #8725635 - Attachment description: patch → xpcshell test that triggers DebuggeeWouldRun
Attachment #8725635 - Attachment is patch: true
Attachment #8725635 - Attachment mime type: text/x-patch → text/plain
Duplicate of this bug: 1157861
Note that since the debugger uses the same mechanism as synchronous XHR to 'pause' the debuggee, promise runnables can also be executed during synchronous XHR. Any fix we end up writing for this issue should also fix that issue, and we need to make sure to test for both.
I can reproduce this in nightly 48.0a1 (2016-03-07). I used this jsbin: http://output.jsbin.com/riwoce but also see the issue using :bdahl 's [A,A,B,B] vs [A,B,B,A] test.
Blocks: 1172876
Assignee: ejpbruel → nobody
I think i'm seeing a similar issue when this patch is applied in: client/inspector/fonts/fonts.js

The issue looks related to Task.async


> update: Task.async(function*() {
> 
>   // breakpoints work alright
> 
>   yield call()
> 
>   // breakpoints are less reliable
> 
> })

I'll try and make a small test case soon.

https://gist.github.com/jasonLaster/df1c280ba5b63bf4ddff0c6bf1680282
Duplicate of this bug: 1397933
Product: Firefox → DevTools
Yet another way to reproduce:

1) Visit about:blank.
2) Open the debugger, with the attached console.
3) In the console, enter:

function f() {
  Promise.resolve(42).then(v => { alert(`promise resolved to ${v}`); debugger; });
  debugger;
}

4) In the console, enter: f();
5) The alert shows.

It should not: the call to f() should complete before any promise callback is invoked.

6) Dismiss the alert.
7) The debugger pauses in the `debugger` statement in the `then` callback, with *two* stack frames for `f`.

It should not: the older of the two is paused at the second `debugger` statement, so it's clear that we initially paused at the second `debugger` statement, and then the promise callback ran during that pause, re-entering the debugger.
Some subtleties we need to take into account when fixing this:

ECMAScript specifies the timing and ordering of promise callbacks. In particular, when JavaScript is entered from the main event loop (for an event handler, or some other DOM callback), and that invocation resolves some promises, those callbacks are run as microtasks (ECMAScript "jobs") immediately after that invocation returns, and before any other main event loop work.

So for example, if an event handler / DOM callback resolves some promise P, it is an error for any other event handler or callback to run before P's callbacks run.
Assignee: nobody → canaltinova
Adding some more subtleties to comment 11:

When the developer stops at a breakpoint and then evaluates an expression in the console that resolves promises, it seems to me that those promise callbacks should run along with that console evaluation; they should not be delayed until the debugger continues execution. Sure, this means that the debugger can disturb the state of the debuggee; but that's what console evaluation is for.

One possible fix:

Introduce a new Chrome primitive, something like `Cu.withSuspendedMicrotasks(f)`, that calls `f` and passes along whatever return value or exception `f` produces. But before calling `f`, `withSuspendedMicrotasks` would save the current microtask queue (HTML calls them 'microtasks', ECMAScript calls them 'jobs'), and after `f` returns, it would drain the current microtask queue and then restore the old one. This restoration behavior would take place no matter whether `f` returns normally or throws an exception, sort of like a `try/finally` block.

In the case of nested evaluation like that described above, this would cause promises resolved by the JavaScript invocation that is being paused for a breakpoint or single step to have their callbacks execute when the debuggee is resumed; while promises resolved by console evaluation during that pause would have their callbacks execute within the pause. This seems like it should be the least surprising behavior.
Nazim, I've been assigned to work on this by my team, so I'm going to steal this bug. If that's going to cause you any difficulties, please let me know and we'll sort out how to proceed. Thanks!
Assignee: canaltinova → jimb
Here's a patch that, together with the previous patch, fixes the problem for debugger statements only, as a proof of concept. With these two applied, the steps to reproduce shown in comment 10 no longer exhibit the bug.

A similar fix just needs to be applied to all Debugger callbacks.
It would probably better if withFreshMicroTask queue didn't require a closure to be created for every call.
Perhaps we can leave it as-is and have a wrapper function? Like

> function dbgHandler(handler) {
>   return function() {
>     return PromiseDebugging.withFreshMicroTaskQueue(() => handler.apply(this, arguments));
>   };
> }

and

> onDebuggerStatement: dbgHandler(function(frame) {
>   // ...
> })

At least then the closure is relatively small and the main handler is still easy for SpiderMonkey to optimize.
This is a step towards a proper fix. There's a bit of ECMAScript object chicanery, but it seems better to just patch Debugger to behave the way it always must, than to try to remember to use a magic incantation every place we ever set (or get!) a hook, or on every hook we create.

Still doesn't work for breakpoints; those are not a hook, per se.
Attachment #9026620 - Attachment is obsolete: true
(In reply to Logan Smyth [:loganfsmyth] from comment #17)
> Perhaps we can leave it as-is and have a wrapper function?

Yes, I did something very much like this in protect-microtask.jsm. But there's really no need to do any allocation at all on every callback invocation. I think the WebIDL might make it easy and efficient to support:

    return PromiseDebugging.callWithFreshMicroTaskQueue(handler, this, ...args);
It seems like the `withFreshMicroTaskQueue` primitive will need to perform a microtask checkpoint - the debugger does indeed queue microtasks just before leaving the handler. (I could probably even get the promises to tell me where...) Those microtasks were almost certainly running interleaved with content microtasks (god save us all).
(In reply to Jim Blandy :jimb from comment #22)
> It seems like the `withFreshMicroTaskQueue` primitive will need to perform a
> microtask checkpoint

Since I only ran into this when I expanded the coverage to other hooks, and it seemed like the only hook being called other than onDebuggerStatement was onNewScript, it seems likely that onNewScript resolves promises that are presently (sans patch) running interleaved with debuggee microtasks. Since onNewScript usually isn't going to spin an event loop that would handle its microtasks (unless it has to fetch source maps), this would make sense.

Interleaving onNewScript's microtasks with the debuggee's could be hugely confusing, especially if we hit a breakpoint in those debuggee microtasks. We'll end up re-entering the debugger before the promise callbacks of the prior onNewScript invocation have even run.
Had a good conversation with bholley about this. It seems to me it's never correct to use Debugger hooks without protecting the content's microtask queue. The fact that we're monkey-patching Debugger's hooks suggests that the Debugger implementation itself should be responsible for stashing and restoring the microtask queue.

I don't think it should be much more work, in the end.
Just to get this into the bug:

It is not sufficient to save and restore the microtask queue as part of the devtools server's preNest/postNest hooks, as we do for all other sorts of events. The debugger server itself uses promises, and if a promise is resolved between the time a Debugger hook fires and the time we enter the nested event loop, then that microtask ought to run as part of the debugger's event loop, not the debuggees. It is at the point of *interruption* - the breakpoint, debugger statement, step, etc. - that we must separate the debuggee's microtasks from the server's.

This is why the fix for this bug needs to change the behavior of Debugger itself, not just the server.
Attachment #9026871 - Attachment is obsolete: true
Attachment #9026872 - Attachment is obsolete: true
Attachment #9026873 - Attachment is obsolete: true
Keywords: leave-open
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/800851f0399e
Document OffThreadPromiseTask. r=luke
The call to `cx->jobQueue->reset()` doesn't do anything that isn't also
accomplished by the call to `fop->delete_(cx->jobQueue.ref())` two lines later.
Since `cx->jobQueue` is a `ThreadData<PersistentRooted<JobQueue>*>`, the
`PersistentRooted` actually owns the `JobQueue` (despite the field's name
`ptr`), rather than holding a pointer to it, so deleting the `PersistentRooted`
invokes the `JobQueue` destructor.

In more detail:

`JSContext::jobQueue` is a `ThreadData<PersistentRooted<JobQueue>*>`, so the
call `cx->jobQueue->reset()` performs the following steps:

- Call `ProtectedData::operator->`, obtaining a (const reference to a non-const)
  pointer `PersistentRooted<JobQueue>*`.

- Call `PersistentRooted::reset`, which move-assigns a fresh `JobQueue` to the
  `ptr` member. Note that `PersistentRooted<JobQueue>::ptr` is a `JobQueue`,
  *not* a pointer.

But the subsequent call to `fop->delete_(cx->jobQueue.ref())` will perform the
following steps:

- Call `ProtectedData::ref`, obtaining a (reference to a)
  `PersistentRooted<JobQueue>*` pointer.

- Call `PersistentRooted<JobQueue>`'s destructor, which destructs `ptr`. Since
  `ptr` is a `JobQueue`, this calls the `JobQueue`'s destructor, safely freeing
  its resources.
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba5d3ec164f2
Remove unnecessary SpiderMonkey internal job queue reset. r=jorendorff
Not ready for review yet, but comments welcome. Just posting to get it off my laptop.
same
for comment
Left to do: onPop, onNewGlobalObject, handleUncaughtException.

Revised.

Attachment #9034353 - Attachment is obsolete: true

Revised. This covers all Debugger hooks.

Attachment #9034532 - Attachment is obsolete: true
Attachment #9034927 - Attachment description: Use AutoSaveMicrotaskQueue in Debugger. r=jorendorff → Use AutoSaveMicrotaskQueue in Debugger.

This includes an implementation of JS::JobQueue for the browser as well as the shell. Not really tested.

While the behavior of ECMAScript Promises and their associated job queue is
covered by the ECMAScript standard, the HTML specification amends that with
additional behavior the web platform requires. To support this, SpiderMonkey
provides hooks the embedding can set to replace SpiderMonkey's queue with its
own implementation.

At present, these hooks are C-style function-pointer-and-void-pointer pairs,
which are awkward to handle and mistake-prone, as passing a function the wrong
void* is not a type error. Later patches in this series must add new hooks,
making a bad situation worse.

A C++ abstract base class is a well-typed alternative. This introduces a new
JS::JobQueue abstract class, and adapts SpiderMonkey's internal job queue and
Gecko's customization to use it. GetIncumbentGlobalCallback and
EnqueuePromiseJobCallback become virtual methods.

Within SpiderMonkey, the patch gathers the various fields of JSContext that
implement the internal queue into their own type, js::InternalJobQueue. Various
jsfriendapi functions become veneers for calls to methods specific to the
derived class. The InternalJobQueue type itself remains private to SpiderMonkey,
as it uses types like TraceableFifo, derived from Fifo, that are not part of
SpiderMonkey's public API.

Within Gecko, CycleCollectedJSContext acquires JS::JobQueue as a private base
class, and a few static methods are cleaned up nicely.

There are a few other hooks defined in js/public/Promise.h that might make sense
to turn into virtual methods on JobQueue. For example,
DispatchToEventLoopCallback, used for resolving promises of results from
off-main-thread tasks, is probably necessarily connected to the JobQueue
implementation in use, so it might not be sensible to set one without the other.
But it was left unchanged to reduce this patch's size.

Depends on D17543

Define a new RAII class, AutoSaveJobQueue, to save and restore the current
ECMAScript job queue, to protect the debuggee's job queue from activity that
occurs in debugger callbacks. Add a new method to the JS::JobQueue abstract base
class, saveJobQueue, to support AutoSaveJobQueue. Comments on AutoSaveJobQueue
provide details.

Implement saveJobQueue for SpiderMonkey's internal job queue and for Gecko's job
queue in CycleCollectedJSContext.

Depends on D17544

Modify the Debugger API implementation to ensure that debugger code's promise
activity (resolving promises; running reaction jobs) cannot interfere with the
debuggee's.

Specifically, ensure that there is an AutoSaveJobQueue in scope at every site in
the Debugger implementation that might invoke a debugger hook function. This
saves the debuggee's job queue, emplaces a fresh empty queue, lets the hooks
run, drains the queue, and then restores the debuggee's original queue.

Since we must already mark sites that could invoke hooks with
EnterDebuggeeNoExecute, we ensure our job queue protection coverage is
complete statically by having EnterDebuggeeNoExecute's constructor require a
reference to an AutoSaveJobQueue.

Depends on D17546

Attachment #9028144 - Attachment is obsolete: true
Attachment #9032309 - Flags: checkin+
Attachment #9034925 - Attachment is obsolete: true
Attachment #9034926 - Attachment is obsolete: true
Attachment #9034927 - Attachment is obsolete: true
Depends on: 1522945

Adding job queue drains to Debugger hooks runs afoul of some OffThreadPromiseTask assumptions. Filed bug 1522945 for that, with patches already attached.

Attachment #9038919 - Attachment description: Bug 1145201: Document OffThreadPromiseRuntimeState fields in more detail. DONTBUILD r?luke → Bug 1145201: Document OffThreadPromiseRuntimeState fields in more detail. r?luke
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8993106d37a6
Document OffThreadPromiseRuntimeState fields in more detail. r=luke

Evaluation of debuggee code should always begin with an empty microtask queue.
In xpcshell tests, this is not guaranteed, as it is in the web platform. This
patch changes those devtools server xpcshell tests that break this rule in a
detectable way to run the debuggee code as a separate HTML task.

In an actual browser environment, debuggee JavaScript runs as an HTML task.
Since HTML requires a microtask checkpoint at the end of each task, this means
that a debuggee task begins execution with an empty microtask queue, free of
microtasks from other tabs or the browser machinery itself. Hence, while the
debugger is pausing debuggee code, it is safe for it to save the debuggee's
microtask queue, so that those jobs do not make progress. (Which is fortunate,
because it must do so, lest the debuggee's microtasks run during the pause!)

In an xpcshell test, however, there is no guarantee that debuggee code begins
execution with a fresh microtask queue: the test may call eval or
evalInSandbox at any time. If such an evaluation hits a breakpoint, debugger
statement, etc. that invokes a Debugger hook, supervisory microtasks from the
test harness code may be set aside along with the debuggee's microtasks. If the
hook code then blocks waiting for those microtasks to run, the test will hang.

Attachment #9038920 - Attachment description: Bug 1145201: Replace EnqueuePromiseJobCallback and GetIncumbentGlobalCallback with new JobQueue abstract base class. r?mccr8,arai → Bug 1145201: Replace EnqueuePromiseJobCallback and GetIncumbentGlobalCallback with new JobQueue abstract base class. r?smaug,arai
Attachment #9038922 - Attachment description: Bug 1145201: Implement JS::AutoSaveJobQueue. r?arai,mccr8 → Bug 1145201: Implement JS::AutoSaveJobQueue. r?arai,smaug
Attachment #9038923 - Attachment description: Bug 1145201: Use AutoSaveMicrotaskQueue in Debugger. r=jorendorff → Bug 1145201: Use AutoSaveJobQueue in Debugger. r=jorendorff

(Phabricator is hopeless for any discussions.)

What do devtools in other browsers do regarding microtasks when debugger is used?

Flags: needinfo?(jimb)
Attachment #9038922 - Attachment description: Bug 1145201: Implement JS::AutoSaveJobQueue. r?arai,smaug → Bug 1145201: Implement JS::AutoDebuggerJobQueueInterruption. r?arai,smaug
Attachment #9038923 - Attachment description: Bug 1145201: Use AutoSaveJobQueue in Debugger. r=jorendorff → Bug 1145201: Use AutoDebuggerJobQueueInterruption in Debugger. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a2412c232a5
Fix xpcshell tests not to mix the test's own microtasks with the debuggee's. r=jlast
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/414b2c238839
Replace EnqueuePromiseJobCallback and GetIncumbentGlobalCallback with new JobQueue abstract base class. r=arai,smaug
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6216c391d41
Implement JS::AutoDebuggerJobQueueInterruption. r=arai,smaug
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ae9f2b94f97
Use AutoDebuggerJobQueueInterruption in Debugger. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f792a3cc1d0
xpcshell test: Debugger callbacks protect debuggee's microtask queue. r=jlast
Status: NEW → RESOLVED
Closed: 5 months ago
Flags: needinfo?(jimb)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Depends on: 1527862
Depends on: 1530345
Regressions: 1545400
You need to log in before you can comment on or make changes to this bug.