[Meta-Bug] Intermittent failures in many sharedWorker tests

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43+ fixed, firefox44 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

No description provided.
Posted patch shared.patch (obsolete) — Splinter Review
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.
Attachment #8646458 - Flags: review?(bkelly)
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.
Attachment #8646458 - Flags: review?(bkelly) → review+
Posted patch shared.patchSplinter Review
Attachment #8646458 - Attachment is obsolete: true
Assignee: nobody → amarchesini
Depends on: 1204775
Depends on: 1205516
Depends on: 1205506
Depends on: 1205250
Depends on: 1205208
Depends on: 1140775
Posted patch sharedWorker.patch (obsolete) — Splinter Review
Here a patch to fix the issue.
Attachment #8663295 - Flags: review?(khuey)
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 on attachment 8663295 [details] [diff] [review]
sharedWorker.patch

I tried to test this locally but everything leaks.
Attachment #8663295 - Flags: review?(khuey) → review-
https://treeherder.mozilla.org/#/jobs?repo=try&revision=266df641fc72

I think you are wrong. Have you applied my 2 changes from comment 8 ?
Flags: needinfo?(khuey)
Oh.  Just fold those into the patch ...
Flags: needinfo?(khuey)
Posted patch sharedWorker.patch (obsolete) — Splinter Review
Attachment #8663295 - Attachment is obsolete: true
Attachment #8664093 - Flags: review?(khuey)
With a mochitest.
Attachment #8664093 - Attachment is obsolete: true
Attachment #8664093 - Flags: review?(khuey)
Attachment #8664196 - Flags: review?(khuey)
Depends on: 1206344
Depends on: 1206328
Depends on: 1206327
Depends on: 1206326
Depends on: 1206261
Depends on: 1206260
Depends on: 1206451
Depends on: 1206360
Depends on: 1206256
Depends on: 1206255
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
Attachment #8664196 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/30c848734bea
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Duplicate of this bug: 1140775
Duplicate of this bug: 1144313
Duplicate of this bug: 1168411
Duplicate of this bug: 1179169
Duplicate of this bug: 1205208
Duplicate of this bug: 1205250
Duplicate of this bug: 1205506
Duplicate of this bug: 1205516
Duplicate of this bug: 1206328
Duplicate of this bug: 1206327
Duplicate of this bug: 1206326
Duplicate of this bug: 1206261
Duplicate of this bug: 1206260
Duplicate of this bug: 1206256
Duplicate of this bug: 1206255
Duplicate of this bug: 1206344
Duplicate of this bug: 1206360
Duplicate of this bug: 1206451
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
Attachment #8664196 - Flags: approval-mozilla-aurora?
Depends on: 1207603
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.
Duplicate of this bug: 1181369
Duplicate of this bug: 1189886
(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.)
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 on attachment 8664196 [details] [diff] [review]
sharedWorker.patch

Has tests, blocking bug is uplifted to aurora, so this is ready for uplift too now.
Attachment #8664196 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.