1970687 - When pausing the browser from the browser toolbox, many other tasks may keep running and cause run-to-completion issues.

Status: NEW

(DevTools :: General, defect, P3)

Description

[Alexandre Poirot [:ochameau]]

Bug 1961814 highlights pretty well a case where DevTools code being debugged by the browser toolbox opened from about:debugging doesn't pause all JS code.

An easier example is about running the following in the browser toolbox:
Services.tm.dispatchToMainThread(() => console.log("bar")); debugger;
The "bar" log will be displayed in the console while you are being paused on the debugger statement, whereas we would expect the dispatchToMainThread to be paused and not trigger its callback.

The lack of pause of these calls to dispatchToMainThread explains why in bug 1961814, the Promise returned by client.connect() never resolves and why the debugger looks stall when you step over this line. The debugger isn't broken, it is rather the Promise that never resolves because we miss the "root" event being fired unexpectedly sooner, while we are paused on that line.

It would be neat to have a generic handling of all the tasks to get rid of all run-to-completion issues.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - [devtools] Pause all tasks except the ones related to the browser toolbox when debugging the main thread.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - Expose an EventTarget dedicated to DevTools through nsIThread.devToolsTarget.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - Use the new EventTarget dedicated to DevTools

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - Expose an EventTarget dedicated to DevTools through nsIThread.devToolsTarget.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - Use the new EventTarget dedicated to DevTools

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - Suspend all but DevTools EventTarget/TaskManager when pausing from the browser toolbox.

Introduce a new EventQueue/nsIRunnable/Task priority for DevTools.

The DevToolsEventTarget, which is 99% copy of ThreadEventTarget.
The only change is the call to mSink->PutEvent which is called with EventQueuePriority::DevTools
So that this priority can be read from the TaskController class in order to
accept running DevTools related tasks.

suspend and resume methods are exposed on nsIThread in order to allow DevTools
to pause all non-DevTools related Tasks. This ultimately set a mDevToolsSuspended boolean
on TaskController.

TODO: I worked around by accepting all JSActor tasks.

But the main issue is about deadlocks. It looks like having two EventTargets
wired on TackController is going off and introduces cycles in the EventQueues.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - Introduce a DevToolsTaskManager

This doesn't look useful as we can work solely based on the Task's Priority.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: WIP: Bug 1970687 - Set a DefaultTaskManager to all Tasks.

This also doesn't look useful as we can distinguish tasks solely based on their priority.
And also, re-using IsSuspended() for even more managers seems to introduce even more dead locks.

Comment 9

[Alexandre Poirot [:ochameau]]

For the record, bug 1074448 comment 4 is 10 years old, but still a pretty good picture of the situation.

My first attempt was querying nsIXPConnectWrappedJS interface from RunnableTask's so that we could easily identify DevTools Browser Toolbox server related Tasks. The Browser Toolber server code is privileged and is running from a dedicated global created by mozJSModuleLoader with invisibleToDebugger compartment flag set to true. I'm using that compartment flag to allow devtools tasks to run and nothing else.

I found this approach quite appealing as it was only allowing the three types of tasks (or three types of JS callbacks) to run:

• nsPipe callback used to receive new RDP requests (DevTools TCP procotol): nsIAsyncInput/OutputStream.asyncWait usage and related Runnable instantiating in C++)
• Timer.sys.mjs / nsITimer: DevTools usage of Timer.sys.mjs and especially this one, creation of the JS callback from Timer.sys.mjs
• dispatchToMainThread usages, especially this one, used from there in the protocol layer (but there is many other important usages).

But this approach is quite implicit as this is based on the global of the js callback related to each task. And this trick can only be used for the browser toolbox. JS Callbacks registered by web pages won't be wrapped into nsIXPConnectWrappedJS, so that it won't be possible to differentiate the paused web page from the others running in the same process. Also it pauses lots of things, it notably prevents the inspector node picker from being functional.

Comment 10

[Alexandre Poirot [:ochameau]]

Based on offline discussion with Nika and Olli, I investigated another approach on this patch queue.

I started by introducing an EventTarget dedicated to the Browser Toolbox server exposed via nsIThread.devToolsEventTarget (first patch).
A second patch migrates DevTools code to use this event target when we are running the browser toolbox server.
The third patch make the DevToolsEventTarget dispatch its runnable via a brand new DevToolsTaskManager.
The last patch is more sizeable and do many things:

• similarly to the first approach introduce nsIThread.{suspend,resume}ByDevtools and make DevTools call them for the browser toolbox
• These methods toggles mDevToolsSuspended on TaskController which uses this boolean to filter Tasks from TaskController::DoExecuteNextTaskOnlyMainThreadInternal.
• When suspended, we only allow to run tasks:
  • scheduled through the DevToolsTaskManager,
  • with Vsync priority to avoid firefox from bieng considered and blocked and keep rendering it
  • all AsyncEventDispatcher tasks, in order to make the inspector node picker functional. It will receive events via Document.setSuppressedEventListener, but the events should still be supressed for other js listeners as DevTools calls nsIDOMWindowUtils.suppressEventHandling for all the windows.
• Add a special trick in JS Actor codebase to allow DevTools related code to run.
  JSProcessActor used by DevTools server, this uses the existing JSActorProtocol.mLoadInDevToolsLoader flag in order to dispatch JSProcessActor's tasks related to the browser toolbox server via the DevToolsTaskManager. Could we possibly pass a custom EventTarget via ProcessActorOptions?

I've already been warned by Nika that pausing only a subset of IPC messages, like I'm doing with the JS Actor trick may easily lead to crashes due to out of order messages.
Having said that, with this new patch queue, I have a demo of browser toolbox pausing almost everything, while still having a functional inspector, and getting rid of typical run-to-completion issues raised from bug 1961814.