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

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Brendan Dahl [:bdahl]]

Attachment: promise.html

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.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

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?

Comment 2

[Brendan Dahl [: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?).

Comment 3

[Brendan Dahl [:bdahl]]

Attachment: promise-debugger.html

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

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: xpcshell test that triggers DebuggeeWouldRun

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.

Comment 6

[Eddy Bruel [:ejpbruel]]

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.

Comment 7

[Andrew Morris]

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.

Comment 8

[Jason Laster [:jlast]]

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

Comment 10

[Jim Blandy :jimb]

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.

Comment 11

[Jim Blandy :jimb]

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.

Comment 12

[Jim Blandy :jimb]

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.

Comment 13

[Jim Blandy :jimb]

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!

Comment 14

[Jim Blandy :jimb]

Attachment: Define chrome webidl PromiseDebugging.withFreshMicroTaskQueue function.

Comment 15

[Jim Blandy :jimb]

Attachment: demo fix for debugger statements only

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.

Comment 16

[Jim Blandy :jimb]

It would probably better if withFreshMicroTask queue didn't require a closure to be created for every call.

Comment 17

[Logan Smyth [:loganfsmyth]]

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.

Comment 18

[Jim Blandy :jimb]

Attachment: Define chrome webidl PromiseDebugging.withFreshMicroTaskQueue function.

Comment 19

[Jim Blandy :jimb]

Attachment: Make PromiseDebugging visible via the devtools loader.

Comment 20

[Jim Blandy :jimb]

Attachment: Override Debugger hook accessors to always protect microtask queue.

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.

Comment 21

[Jim Blandy :jimb]

(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);

Comment 22

[Jim Blandy :jimb]

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).

Comment 23

[Jim Blandy :jimb]

(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.

Comment 24

[Jim Blandy :jimb]

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.

Comment 25

[Jim Blandy :jimb]

Attachment: xpcshell test: Debugger callbacks protect debuggee's microtask queue.

Comment 26

[Jim Blandy :jimb]

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.

Comment 27

[Jim Blandy :jimb]

Attachment: Bug 1145201: Document OffThreadPromiseTask. r?luke

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/800851f0399e
Document OffThreadPromiseTask. r=luke

Comment 29

[Jim Blandy :jimb]

Attachment: Bug 1145201: Remove unnecessary SpiderMonkey internal job queue reset. r?jorendorff

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.

Comment 30

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/800851f0399e

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba5d3ec164f2
Remove unnecessary SpiderMonkey internal job queue reset. r=jorendorff

Comment 32

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ba5d3ec164f2

Comment 33

[Jim Blandy :jimb]

Attachment: Replace EnqueuePromiseJobCallback and GetIncumbentGlobalCallback with new JobQueue abstract base class.

Not ready for review yet, but comments welcome. Just posting to get it off my laptop.

Comment 34

[Jim Blandy :jimb]

Attachment: Implement JS::AutoSaveMicrotaskQueue.

same

Comment 35

[Jim Blandy :jimb]

Attachment: Use AutoSaveMicrotaskQueue in Debugger.

for comment

Comment 36

[Jim Blandy :jimb]

Left to do: onPop, onNewGlobalObject, handleUncaughtException.

Comment 37

[Jim Blandy :jimb]

Attachment: Replace EnqueuePromiseJobCallback and GetIncumbentGlobalCallback with new JobQueue abstract base class.

Revised.

Comment 38

[Jim Blandy :jimb]

Attachment: Implement JS::AutoSaveMicrotaskQueue.

Revised.

Comment 39

[Jim Blandy :jimb]

Attachment: Use AutoSaveMicrotaskQueue in Debugger.

Revised. This covers all Debugger hooks.

Comment 40

[Jim Blandy :jimb]

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

Comment 41

[Jim Blandy :jimb]

Attachment: Bug 1145201: Document OffThreadPromiseRuntimeState fields in more detail. r?luke

Comment 42

[Jim Blandy :jimb]

Attachment: Bug 1145201: Replace EnqueuePromiseJobCallback and GetIncumbentGlobalCallback with new JobQueue abstract base class. r?smaug,arai

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

Comment 43

[Jim Blandy :jimb]

Attachment: Bug 1145201: Implement JS::AutoDebuggerJobQueueInterruption. r?arai,smaug

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

Comment 44

[Jim Blandy :jimb]

Attachment: Bug 1145201: Use AutoDebuggerJobQueueInterruption in Debugger. r=jorendorff

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

Comment 45

[Jim Blandy :jimb]

Attachment: Bug 1145201: xpcshell test: Debugger callbacks protect debuggee's microtask queue.

Depends on D17547

Comment 46

[Jim Blandy :jimb]

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

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8993106d37a6
Document OffThreadPromiseRuntimeState fields in more detail. r=luke

Comment 48

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/8993106d37a6

Comment 49

[Jim Blandy :jimb]

Attachment: Bug 1145201: Fix xpcshell tests not to mix the test's own microtasks with the debuggee's. r=jlast

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.

Comment 50

[Jim Blandy :jimb]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc08c29e2aac145f6507f09685a160ab6220971

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

(Phabricator is hopeless for any discussions.)

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

Comment 52

[Pulsebot]

Pushed by [e-mail]:
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

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/414b2c238839
Replace EnqueuePromiseJobCallback and GetIncumbentGlobalCallback with new JobQueue abstract base class. r=arai,smaug

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6216c391d41
Implement JS::AutoDebuggerJobQueueInterruption. r=arai,smaug

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ae9f2b94f97
Use AutoDebuggerJobQueueInterruption in Debugger. r=jorendorff

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f792a3cc1d0
xpcshell test: Debugger callbacks protect debuggee's microtask queue. r=jlast

Comment 57

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9a2412c232a5
https://hg.mozilla.org/mozilla-central/rev/414b2c238839
https://hg.mozilla.org/mozilla-central/rev/b6216c391d41
https://hg.mozilla.org/mozilla-central/rev/3ae9f2b94f97
https://hg.mozilla.org/mozilla-central/rev/1f792a3cc1d0