1074448 - (dbg-r2c) [meta] Run to completion bugs

Status: NEW

(DevTools :: Debugger, task, P3)

Description

[Eddy Bruel [:ejpbruel]]

This bug is an attempt to group together all bugs related to, or suspected to be related to, nested event loops.

Comment 1

[Eddy Bruel [:ejpbruel]]

Actually, this bug encompasses all bugs related to not allowing debuggee code to run while it is supposed to be paused. The best description I can come up with for this is 'run to completion semantics'.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The Real Fix(tm) would be https://bugzilla.mozilla.org/show_bug.cgi?id=715376, but there are workarounds we can do in the meantime.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I believe we also have these issues with

* Promise
* postMessage

but I don't have testcases/bugs on hand, but they probably exist somewhere in bugzilla.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Here are my notes from an in-person meeting with myself, bz, jimb, and jlongster:

We have 3 possible approaches:

1. Boil the ocean, multiple event queues of nsIRunnables (aka https://bugzilla.mozilla.org/show_bug.cgi?id=715376)

2. Have the code called by the runnable know how to postpone and delay itself
   for later. This is the existing bandaid that we have now (suppressEventsAndTimers or whatever it is called, same thing used by sync xhr), where we specialize
   on a case by case basis.

3. Create a side table for each runnable associating it with a global /
   compartment / document / something. In between 1 and 2, but just as much
   wack-a-mole as 2.

(1) isn't good because resizing windows and the resulting reflow and repaint is
an implementation detail that is also exposed to content. We want reflow/repaint when resizing windows when paused. The refresh driver
would need to be rewritten as well as it runs requestAnimationFrame directly
without entering the event queue. The nsIRunnable really doesn't have the info we need. So (3) isn't a great option either.

Seems like (2) is best (only) bet, when combined with...

Complementary idea for *identifying* all the existing places JS is run that
shouldn't be: set a flag on the JSCompartment or maybe in gecko (somewhere? didn't
catch where?) while we are paused and then
warn/log/throw-exception-instead-of-running-script if we try to run JS on a
compartment/whatever when the flag is set. After we have wacked all moles, we can consider making this a fatal assertion.

Comment 6

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #4)
> Complementary idea for *identifying* all the existing places JS is run that
> shouldn't be: set a flag on the JSCompartment or maybe in gecko (somewhere?
> didn't catch where?) while we are paused and then
> warn/log/throw-exception-instead-of-running-script if we try to run JS on a
> compartment/whatever when the flag is set.

With bug 912337, we now have this mechanism. We just need a way to get backtraces out of it even when it's a warning.

Comment 7

[Jim Blandy :jimb]

Here's an email Shu sent today. I think pretty much all of it belongs in the bug, unless we can find an even better spot for these docs:

----

Hello,

I recently landed an implementation of Debugger.DebuggeeWouldRun, which is an error that's thrown when debugger code attempts to re-enter debuggee code without going through a blessed "invocation function" (currently D.F.p.eval and D.O.p.executeInGlobal) [1]. These re-entries are the cause of the debugger not truly able to pause the debuggee.

Currently it is not an error but a warning instead, and is to help pinpoint sites in debugger code that attempts to re-enter debuggee code. The intention is that once all sites are fixed, it will be an error.

There are 2 prefs that should be toggled when using this to find bugs:

- javascript.options.dump_stack_on_debuggee_would_run
- javascript.options.throw_on_debuggee_would_run

(The reason, in case you were wondering, why the 2nd pref alone doesn't suffice is because there are indiscriminate catch-all blocks inside devtools code, so making it throw isn't enough to diagnose.)

I recommend folks who are interested in fixing these bugs to play around with those prefs.

One current shortcoming is that all self-hosted code in the debuggee compartment is banned. This means some operations that should in fact be allowed are warned against/disallowed. This problem is tricky implementation wise, and if you have any ideas please comment in bug 1249469 [2].

[1] https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Conventions#The_Debugger.DebuggeeWouldRun_Exception
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1249469

Comment 8

[Eddy Bruel [:ejpbruel]]

I'm going to mark this bug as P1. After (service) worker debugging, making the debugger behave correctly in all cases should be our number one priority, and making sure the debuggee actually pauses when it is supposed to be paused is an important part of that.

Comment 9

[Jason Laster [:jlast]]

I saw these two exceptions while reloading aggressively

STR:

1. go to dbg-html.glitch.me
2. add a breakpoint in index.html#30
3. reload several times

JavaScript warning: resource://devtools/server/actors/utils/event-loop.js, line 118: DebuggeeWouldRun: debuggee `resource://devtools/shared/transport/child-transport.js:65' would run
JavaScript warning: resource://devtools/server/actors/utils/event-loop.js, line 118: DebuggeeWouldRun: debuggee `resource://gre/modules/Timer.jsm:39' would run