1833637 - Pausing on breakpoint permits tasks from other frames to run

Status: NEW

(DevTools :: Debugger, defect, P2)

Description

[Colin Alworth]

Attachment: bug.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/112.0

Steps to reproduce:

This bug shows a difference between window.setTimeout and window.top.setTimeout - in the code where this was found, the iframe has lots of references to the outer window and document, and avoids referencing its own, to prevent cases where an object fails instanceof checks elsewhere.

1. Open the given HTML file (can be loaded from a server or from a file:/// URL). This will start an iframe which will set a timer and log some output periodically to the browser console.
2. Open browser tools. The next time the timer runs, it will pause at a debugger statement.
3. Click "Resume" in the debugger.

Actual results:

Console output and execution continued while the debugger was paused.

Also notice that the counter that increments when each timer goes off and decrements when the timer returns has managed to be incremented twice somehow, indicating that two timers appear to have run concurrently.

Note that if the JS run in the iframe uses self.setTimeout instead of top.setTimeout, the bug does not occur.

Note also that setting a breakpoint at if (depth !== 0) { will show a related failure - the console keeps being logged to, despite the debugger being paused (since the depth counter hasn't yet been incremented).

Expected results:

All JavaScript execution should be paused for this window, iframes notwithstanding, while paused in the breakpoint. Output should have resumed as before once the breakpoint was completed.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'DevTools::Debugger' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Julian Descottes [:jdescottes]]

Thanks for filing, we could have an option to pause all frames / threads on a page (but that's not necessarily something you want to have all the time).

This might be hard to achieve consistently for cross process iframes etc... And might require unpausing individual frames etc...

Comment 3

[Colin Alworth]

This shouldn't be a question of threads - an iframe's JS and its host page (at least when they are same-origin) still run on the same thread, correct? They would have to, since they can read/write each other's data freely (unlike workers, etc). And the bug doesn't seem to exist, with an apparent race between implementations of setTimeout, except when a breakpoint is hit.

Regardless, the setTimeout method is always called from within the iframe's context, and is always passed a callback from the iframe's context, so should run in the "iframe's thread", even though it is a different implementation of setTimeout?

This is a regression, though I don't have the exact version it started to fail under (and am not sure how to download old versions to check any more).

Comment 4

[Julian Descottes [:jdescottes]]

Yes you're correct, sorry about this.

So specifically about same origin iframes, I think this regressed around the time we started treating any frame as an individual "target" which was a fallout from our work to support cross process iframes. Basically we are using the same architecture to debug same origin iframes as the one to debug cross origin iframes, which had this unexpected side effect.

We have one preference which is supposed to drive this: devtools.every-frame-target.enabled. It was enabled for the release channel in 97 for the actual preference flip on the release channel: https://bugzilla.mozilla.org/show_bug.cgi?id=1731740

If you change this preference to false in about:config and try again your test case, it should now pass. Might need to restart Firefox, I'm not sure if this pref gets cached or not.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1731740

:nchevobbe, since you are the author of the regressor, bug 1731740, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 6

[Julian Descottes [:jdescottes]]

Will go back to triage on thursday, regressor is just a pref flip

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1731740

Comment 8

[Alexandre Poirot [:ochameau]]

This relates to this code, which controls the frozen windows:
https://searchfox.org/mozilla-central/rev/f030995a79461379153293c0e07f4982afe9ac28/devtools/server/actors/utils/event-loop.js#194-197

    for (const window of this.getAllWindowDebuggees()) {
      const { windowUtils } = window;
      windowUtils.suppressEventHandling(true);
      windowUtils.suspendTimeouts();

And to this code, which controls which windows are being frozen:
https://searchfox.org/mozilla-central/rev/f030995a79461379153293c0e07f4982afe9ac28/devtools/server/actors/utils/event-loop.js#135-137

  getAllWindowDebuggees() {
    return this._thread.dbg
      .getDebuggees()

Because of EFT work, we only freeze the paused window and none of the others.
While it makes sense to have one target and thread actor per WindowGlobal so that sources are per WindowGlobal and their lifecycle is easier to maintain...
...may be it is still relevant to have a shared EventLoop? Or have each Thread actor's Event loop to freeze all the same-process WindowGlobals?

It should be doable to revise getAllWindowDebuggees to go over all same-process, same browserId's WindowGlobals,
it is just unclear what could be the possible side effect of such change.
If one document pauses and another one manage to pause, but it should be hard given that timeouts and events are off... then we could start having trouble.