WorkerDebugger should be unregistered when worker is frozen.

RESOLVED FIXED in Firefox 44

Status

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 44
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Currently, the worker debugger API provides a way to check whether a worker is frozen or not, and to detect when it freezes/thaws.

The reason we have this API is that we use the window that owns the worker to determine what page it belongs to. When a page that owns a worker is moved into the bfcache, the worker is frozen, but its window is still the same as the new page that is loaded in the tab. Consequently, we need to remove workers from the list of workers for a given page when they are frozen, and re-add them when they are thawed.

This solution was slightly buggy to begin with, as workers are also frozen when the main thread suspends worker events to create the illusion that it is blocked. Bug 1178721 will fix that problem by only queueing the events generated by workers on the main thread, without actually freezing them.

Once bug 1178721 lands, the only time a worker will be frozen is when the page that owns them is moved to the bfcache. Whenever that happens, the worker needs to be removed from the list of debuggable workers, and re-added when it is thawed.

This made me realise that we don't actually need a separate API for this: we can simply unregister and close the worker debugger for a given worker when it is frozen, and register a new worker debugger when it is thawed. The result is a significant simplification of the worker debugger API.
(Assignee)

Comment 1

3 years ago
Created attachment 8627636 [details] [diff] [review]
Simplify how we deal with freezing/thawing workers (backend).

This patch contains the changes required to the backend to implement the idea sketched in comment #1. The changes required to the frontend will be in the next patch, so they can be reviewed separately.

Note that these patches need to land together to prevent tests from breaking.
Attachment #8627636 - Flags: review?(khuey)
(Assignee)

Comment 2

3 years ago
Created attachment 8627638 [details] [diff] [review]
Simplify how we deal with freezing/thawing workers (frontend).

And here are the necessary frontend changes.
Attachment #8627638 - Flags: review?(jlong)
(Assignee)

Updated

3 years ago
Attachment #8627638 - Attachment description: simplify-frontend → Simplify how we deal with freezing/thawing workers (frontend).
Comment on attachment 8627636 [details] [diff] [review]
Simplify how we deal with freezing/thawing workers (backend).

Review of attachment 8627636 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/dom_worker_helper.js
@@ +61,5 @@
>  function waitForRegister(url, dbgUrl) {
>    return new Promise(function (resolve) {
>      wdm.addListener({
>        onRegister: function (dbg) {
> +        dump("FAK " + dbg.url + "\n");

Was perusing this patch and happened to see this, should remove this.
Comment on attachment 8627638 [details] [diff] [review]
Simplify how we deal with freezing/thawing workers (frontend).

Review of attachment 8627638 [details] [diff] [review]:
-----------------------------------------------------------------

I like it, nice simplification

::: browser/devtools/debugger/debugger-controller.js
@@ +479,5 @@
>        return;
>      }
>  
>      this._tabClient.listWorkers((response) => {
> +      let workerForms = Object.create(null);

Why use this pattern, instead of the literal empty object {} ?
Attachment #8627638 - Flags: review?(jlong) → review+
Comment on attachment 8627636 [details] [diff] [review]
Simplify how we deal with freezing/thawing workers (backend).

I'm assuming this patch depends on bug 1178721, so canceling review.
Attachment #8627636 - Flags: review?(khuey)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

3 years ago
Created attachment 8664224 [details] [diff] [review]
Simplify how we deal with freezing/thawing workers (backend).

We've decided to move forward with bug 1178721, so this bug can move forward as well. Blake agreed to review.

Here is a rebased version of the patch. Please see comment 1 for an explanation of what it does and why.
Attachment #8664224 - Flags: review?(mrbkap)
(Assignee)

Updated

3 years ago
Attachment #8627636 - Attachment is obsolete: true
Attachment #8664224 - Flags: review?(mrbkap) → review?(khuey)

Updated

3 years ago
Blocks: 1175550
(Assignee)

Updated

3 years ago
Blocks: 1211903
(Assignee)

Comment 8

3 years ago
Try failures due to a missing closing brace. New try push with that problem resolved:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c6d96a94f2

Updated

3 years ago
Blocks: 1214248
https://hg.mozilla.org/mozilla-central/rev/c07248ffb454
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1216217
(Assignee)

Updated

3 years ago
Summary: Simplify how we deal with freezing/thawing workers → WorkerDebugger should be unregistered when worker is frozen.

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.