SuspendWorkersForWindow() doesn't seem to take into account SharedWorkers

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

SuspendWorkersForWindow() gets all the Workers associated with the window and then calls ParentWindowPaused().  This method includes this assertion:

MOZ_ASSERT(!mParentWindowPaused, "Suspended more than once!");

This doesn't seem to properly expect what could happen with a SharedWorker.  In that case you could have multiple windows suspended and each could result in a ParentWindowPaused().  It seems we should have some kind of pause depth or perhaps similar logic as to what Freeze does.

Andrea, what do you think?
Flags: needinfo?(amarchesini)
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(amarchesini)
Comment on attachment 8804031 [details] [diff] [review]
Allow multiple windows to be paused for SharedWorkers. r=baku

This fixes the assertion failures in the try build.  Basically it just replaces the boolean with a count.  We assert we only have a count greater than one when its not a dedicated worker.  Only un-pause when the count reaches zero.

We might want to consider having separate sets of queued events for the different windows in the future, but this is no worse than the current condition.
Attachment #8804031 - Flags: review?(amarchesini)
Blocks: 1303167
Comment on attachment 8804031 [details] [diff] [review]
Allow multiple windows to be paused for SharedWorkers. r=baku

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

::: dom/workers/WorkerPrivate.h
@@ +197,5 @@
>    // thread as SharedWorkers are always top-level).
>    nsTArray<RefPtr<SharedWorker>> mSharedWorkers;
>  
>    uint64_t mBusyCount;
> +  uint32_t mParentWindowPausedDepth;

Write a comment about why this is uint32_t.
Attachment #8804031 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5f21382c83
Allow multiple windows to be paused for SharedWorkers. r=baku
https://hg.mozilla.org/mozilla-central/rev/9b5f21382c83
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.