Brendan Dahl [:bdahl] Reporter
	Description • 9 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 1 • 9 years ago

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?

	Brendan Dahl [:bdahl] Reporter
	Comment 2 • 9 years ago

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

	Brendan Dahl [:bdahl] Reporter
	Comment 3 • 9 years ago

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

	Eddy Bruel [:ejpbruel]
	Comment 4 • 8 years ago

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.

	Eddy Bruel [:ejpbruel]
	Comment 6 • 8 years ago

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.

	Andrew Morris
	Comment 7 • 8 years ago

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.

	Jason Laster [:jlast]
	Comment 8 • 7 years ago

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

	Jim Blandy :jimb Assignee
	Comment 10 • 6 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 11 • 6 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 12 • 6 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 13 • 6 years ago

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!

	Jim Blandy :jimb Assignee
	Comment 15 • 6 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 16 • 6 years ago

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

	Logan Smyth [:loganfsmyth]
	Comment 17 • 6 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 20 • 6 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 21 • 6 years ago

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

	Jim Blandy :jimb Assignee
	Comment 22 • 6 years ago

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

	Jim Blandy :jimb Assignee
	Comment 23 • 6 years ago

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

	Jim Blandy :jimb Assignee
	Comment 24 • 6 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 26 • 5 years ago

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.

	Pulsebot
	Comment 28 • 5 years ago

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/800851f0399e
Document OffThreadPromiseTask. r=luke

	Jim Blandy :jimb Assignee
	Comment 29 • 5 years ago

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.

	Narcis Beleuzu [:NarcisB]
	Comment 30 • 5 years ago
bugherder

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

	Pulsebot
	Comment 31 • 5 years ago

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba5d3ec164f2
Remove unnecessary SpiderMonkey internal job queue reset. r=jorendorff

	Andrei Ciure[:aciure]
	Comment 32 • 5 years ago
bugherder

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

	Jim Blandy :jimb Assignee
	Comment 33 • 5 years ago

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

	Jim Blandy :jimb Assignee
	Comment 34 • 5 years ago

same

	Jim Blandy :jimb Assignee
	Comment 35 • 5 years ago

for comment

	Jim Blandy :jimb Assignee
	Comment 36 • 5 years ago

Left to do: onPop, onNewGlobalObject, handleUncaughtException.

	Pulsebot
	Comment 47 • 5 years ago

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8993106d37a6
Document OffThreadPromiseRuntimeState fields in more detail. r=luke

	Pulsebot
	Comment 52 • 5 years ago

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

	Pulsebot
	Comment 53 • 5 years ago

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

	Pulsebot
	Comment 54 • 5 years ago

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6216c391d41
Implement JS::AutoDebuggerJobQueueInterruption. r=arai,smaug

	Pulsebot
	Comment 55 • 5 years ago

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ae9f2b94f97
Use AutoDebuggerJobQueueInterruption in Debugger. r=jorendorff

	Pulsebot
	Comment 56 • 5 years ago

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f792a3cc1d0
xpcshell test: Debugger callbacks protect debuggee's microtask queue. r=jlast