Closed Bug 1592584 Opened 5 years ago Closed 3 years ago

Directly connect the console to a DevToolsServer on the worker thread for worker targets

Categories

(DevTools :: Console, task, P2)

task

Tracking

(Fission Milestone:MVP, firefox84 fixed)

RESOLVED FIXED
84 Branch
Fission Milestone MVP
Tracking Status
firefox84 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 6 open bugs)

Details

(Whiteboard: dt-fission)

Attachments

(1 file, 1 obsolete file)

If I don't misunderstand this comment dom/console/Console.cpp#79-84, console messages emitted from a worker thread are cloned to the main thread.

We should check if that's still needed since we now have the ability to attach to multiple thread at the same time.

Andrea, are there any other reason we need to do this that I'm not thinking of?

Flags: needinfo?(amarchesini)

If we are able to retrieve console events from any thread, using actors, then we can remove that code.
Currently, console events are sent to the main-thread because this is the only way they can be shown in the devtools console.

Note that, Console API already exposes events on the worker thread, via WorkerDebuggerGlobalScope::retrieveGlobalEvents().
We can do something similar for any other global (worklet, window, etc).

Do you mind to organize a quick meeting to see how we want to proceed? I can take care of the console API part.

Flags: needinfo?(amarchesini) → needinfo?(nchevobbe)

We're not ready to make the switch yet, but I'll set up the meeting when we're in good position to do so.
Thanks baku!

Flags: needinfo?(nchevobbe)
Priority: -- → P3
Whiteboard: dt-fission-reserve
Blocks: 1215120
Priority: P3 → P2
Whiteboard: dt-fission-reserve → dt-fission-m2-mvp

Tracking DevTools bugs for Fission Nightly (M6) milestone

Fission Milestone: --- → M6
Whiteboard: dt-fission-m2-mvp

Baku, do you think we can put the worker message forwarding to the main thread behind a pref?
This way we'll be able to start working on connecting the console to workers directly, without having duplicated messages.

Flags: needinfo?(amarchesini)

Sure. I can do it. I'll file a separate bug and I'll CC you when I have a patch.

Flags: needinfo?(amarchesini)

(In reply to Andrea Marchesini [:baku] from comment #6)

Sure. I can do it. I'll file a separate bug and I'll CC you when I have a patch.

great, thanks a lot!

Bug 1613147 added a pref you can set to false in order to not clone objects from worker thread to main thread.

Depends on: 1613147
Attached file Bug 1592584 - demo (obsolete) —

Comment on attachment 9125576 [details]
Bug 1592584 - demo

STR to test this patch:

  • toggle dom.worker.console.dispatch_events_to_main_thread to false
  • open a web page
  • evaluate this in the console:
new Worker(URL.createObjectURL(new Blob([`
  console.log("inWorker");
  console.log(new SharedArrayBuffer())
  console.log("after sharedArrayBuffer");
`], { type: "text/javascript" })))

You should get the worker messages via the console actor instead of the platform clones.

Bug 1573779 may be a good thing to do first in order to mitigate this issue where consoleActor is undefined on the targetFront.targetForm when onTargetAvailable function of the WebConsole is called.

Otherwise, possible blockers are:

  • performance. We would need to create descriptors and targets for all workers. Note that the debugger will enable such work for the "windowless-service-workers" feature anyway.
  • early messages. We may miss messages that are sent very early when the worker just starts. It doesn't look like it is the case in this example, but I wouldn't be surprise if we miss things.
  • do we miss any feature against additional targets in the console? Whatever we miss in the Multiprocess Browser Toolbox, would be missing for workers. But I imagine that here, for workers, whatever was missing, wasn't working anyway.
Blocks: 1058130
See Also: → 1624098

Commenting since I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1641121. Not sure if this has been covered example of things this breaks, if you run the following snippet on a page:

w = new Worker(URL.createObjectURL(new Blob([`eval(\`console.log("omg");\`);`])));

clicking on the message's URL fails because there server does not send along an actor ID (because the actor exists in the worker's server, not the main-thread server) and the console.log happens inside an eval and thus has no useful URL of its own so the debugger has no way to know what file to open.

Adding dt-fission whiteboard tag to DevTools bugs that mention Fission or block Fission meta bugs but don't already have a dt-fission whiteboard tag.

Whiteboard: dt-fission

Moving these DevTools Fission bugs from Fission's old M6 Nightly milestone to M7 Beta. I am assuming these bugs would have the dt-fission-m2-mvp whiteboard tag if they were Fission Nightly blockers.

Fission Milestone: M6 → M7
Assignee: nobody → nchevobbe
Attachment #9167024 - Attachment description: Bug 1592584 - Connect to worker target directly. → Bug 1592584 - Accept worker targets in console. r=ochameau.
Status: NEW → ASSIGNED
Attachment #9167024 - Attachment description: Bug 1592584 - Accept worker targets in console. r=ochameau. → Bug 1592584 - [devtools] Accept worker targets in console. r=ochameau.

Bulk change of all bugs with whiteboard tag of dt-fission to Fission MVP milestone.

Fission Milestone: M7 → MVP
Blocks: 1674342
Summary: Investigate if our current Worker setup could be refactored → Directly connect the console to a DevToolsServer on the worker thread for worker targets
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8944f6b214a0
[devtools] Accept worker targets in console. r=ochameau.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Attachment #9125576 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: