1193414 - [Meta-Bug] Intermittent failures in many sharedWorker tests

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: shared.patch

This patch doesn't fix anything but it help us to see if the problem is because we reach the limit of SharedWorkers per domain.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8646458 [details] [diff] [review]
shared.patch

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

Seems its mainly adding telemetry?  Seems reasonable although I'm not that familiar with telemetry stuff.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: shared.patch

Comment 4

[Andrea Marchesini [:baku]]

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

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/984024ad7c0c

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/984024ad7c0c

Comment 7

[Andrea Marchesini [:baku]]

Attachment: sharedWorker.patch

Here a patch to fix the issue.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8663295 [details] [diff] [review]
sharedWorker.patch

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

With these 2 changes, it's green on try.

::: dom/workers/RuntimeService.cpp
@@ +1692,5 @@
>      Telemetry::AccumulateTimeDelta(Telemetry::SERVICE_WORKER_LIFE_TIME,
>                                     aWorkerPrivate->CreationTimeStamp());
>    }
>  
>    if (aWorkerPrivate->IsSharedWorker()) {

|| aWorkerPrivate->IsServiceWorker()) {

::: dom/workers/WorkerPrivate.cpp
@@ +3135,5 @@
> +
> +  for (uint32_t i = 0; i < mSharedWorkers.Length(); ++i) {
> +    mSharedWorkers[i]->Close();
> +  }
> +

mSharedWorkers.Clear();

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8663295 [details] [diff] [review]
sharedWorker.patch

I tried to test this locally but everything leaks.

Comment 10

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=266df641fc72

I think you are wrong. Have you applied my 2 changes from comment 8 ?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Oh.  Just fold those into the patch ...

Comment 12

[Andrea Marchesini [:baku]]

Attachment: sharedWorker.patch

Comment 13

[Andrea Marchesini [:baku]]

Attachment: sharedWorker.patch

With a mochitest.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8664196 [details] [diff] [review]
sharedWorker.patch

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

::: dom/workers/WorkerPrivate.cpp
@@ -2964,5 @@
> -  mSharedWorkers.RemoveElement(aSharedWorker);
> -
> -  // If there are still SharedWorker objects attached to this worker then they
> -  // may all be frozen and this worker would need to be frozen. Otherwise,
> -  // if that was the last SharedWorker then it's time to cancel this worker.

Please preserve this comment when you move the code.

@@ +3117,5 @@
> +
> +  AutoSafeJSContext cx;
> +
> +  if (!mSharedWorkers.IsEmpty()) {
> +    if (!Freeze(cx, nullptr)) {

The way Freeze with a null nsPIDOMWindow works is an ugly little backdoor.  We should clean that up at some point.

::: dom/workers/test/test_sharedWorker_lifetime.html
@@ +19,5 @@
> +  sw.port.onmessage = function(event) {
> +    if (gced) {
> +      ok(true, "The SW is still alive also after GC");
> +      SimpleTest.finish();
> +    }

Just ok(gced) and remove the conditional

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30c848734bea

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30c848734bea

Comment 35

[Andrea Marchesini [:baku]]

Comment on attachment 8664196 [details] [diff] [review]
sharedWorker.patch

Approval Request Comment
[Feature/regressing bug #]: SharedWorker
[User impact if declined]: possibly sharedWorker/ServiceWorker objects are CCed too early
[Describe test coverage new/current, TreeHerder]: yes, there is a mochitest.
[Risks and why]: the patch is quite simple.
[String/UUID change made/needed]: none

Comment 36

[Phil Ringnalda (:philor)]

Risk: Because fixing this introduced the permaorange bug 1207603, landing it on aurora means we will no longer run parts of the worker tests, or the subsequent dom/xbl, dom/xul and dom/xslt tests on debug Android.

Comment 39

[(no longer active)]

(In reply to Phil Ringnalda (:philor) from comment #36)
> Risk: Because fixing this introduced the permaorange bug 1207603, landing it
> on aurora means we will no longer run parts of the worker tests, or the
> subsequent dom/xbl, dom/xul and dom/xslt tests on debug Android.

I fixed that bug and asked for the fix to be backported to Aurora, but this test suite is still orange (see bug 1208920 for that.)

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

It looks like this should wait for uplift until the fixes for bug 1207603 get checked in for aurora. I'll come back to it tomorrow when the sheriffs are around.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8664196 [details] [diff] [review]
sharedWorker.patch

Has tests, blocking bug is uplifted to aurora, so this is ready for uplift too now.

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/36b3f1e21a29