Closed Bug 1178726 Opened 7 years ago Closed 7 years ago

WorkerDebugger should be unregistered when worker is frozen.


(DevTools :: Debugger, defect)

Not set


(firefox44 fixed)

Firefox 44
Tracking Status
firefox44 --- fixed


(Reporter: ejpbruel, Assigned: ejpbruel)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

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.
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)
And here are the necessary frontend changes.
Attachment #8627638 - Flags: review?(jlong)
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)
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)
Attachment #8627636 - Attachment is obsolete: true
Attachment #8664224 - Flags: review?(mrbkap) → review?(khuey)
Blocks: 1175550
Blocks: 1211903
Try failures due to a missing closing brace. New try push with that problem resolved:
Blocks: 1214248
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1216217
Summary: Simplify how we deal with freezing/thawing workers → WorkerDebugger should be unregistered when worker is frozen.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.