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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Allow multiple windows to be paused for SharedWorkers. r=baku

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Testing if this fixes the intermittent my patches in bug 1303167 provoke:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97cccbc52159b839fc7625ecba2180169c57c4d4

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Andrea Marchesini [:baku]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5f21382c83
Allow multiple windows to be paused for SharedWorkers. r=baku

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9b5f21382c83