[Meta-Bug] Intermittent failures in many sharedWorker tests

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
2 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)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8646458 [details] [diff] [review]
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.
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+
(Assignee)

Comment 3

3 years ago
Created attachment 8646906 [details] [diff] [review]
shared.patch
Attachment #8646458 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
Keywords: checkin-needed, leave-open
(Assignee)

Updated

3 years ago
Depends on: 1204775
(Assignee)

Updated

3 years ago
Depends on: 1205516
(Assignee)

Updated

3 years ago
Depends on: 1205506
(Assignee)

Updated

3 years ago
Depends on: 1205250
(Assignee)

Updated

3 years ago
Depends on: 1205208
(Assignee)

Updated

3 years ago
Depends on: 1140775
(Assignee)

Comment 7

3 years ago
Created attachment 8663295 [details] [diff] [review]
sharedWorker.patch

Here a patch to fix the issue.
Attachment #8663295 - Flags: review?(khuey)
(Assignee)

Comment 8

3 years ago
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-
(Assignee)

Comment 10

3 years ago
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)
(Assignee)

Comment 12

3 years ago
Created attachment 8664093 [details] [diff] [review]
sharedWorker.patch
Attachment #8663295 - Attachment is obsolete: true
Attachment #8664093 - Flags: review?(khuey)
(Assignee)

Comment 13

3 years ago
Created attachment 8664196 [details] [diff] [review]
sharedWorker.patch

With a mochitest.
Attachment #8664093 - Attachment is obsolete: true
Attachment #8664093 - Flags: review?(khuey)
Attachment #8664196 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Depends on: 1206344
(Assignee)

Updated

3 years ago
Depends on: 1206328
(Assignee)

Updated

3 years ago
Depends on: 1206327
(Assignee)

Updated

3 years ago
Depends on: 1206326
(Assignee)

Updated

3 years ago
Depends on: 1206261
(Assignee)

Updated

3 years ago
Depends on: 1206260
(Assignee)

Updated

3 years ago
Depends on: 1206451
(Assignee)

Updated

3 years ago
Depends on: 1206360
(Assignee)

Updated

3 years ago
Depends on: 1206256
(Assignee)

Updated

3 years ago
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
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1140775
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1144313
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1168411
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1179169
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1205208
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1205250
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1205506
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1205516
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206328
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206327
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206326
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206261
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206260
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206256
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206255
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206344
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206360
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1206451
(Assignee)

Comment 35

3 years ago
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.

Updated

3 years ago
Duplicate of this bug: 1181369

Updated

3 years ago
Duplicate of this bug: 1189886

Comment 39

3 years ago
(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.
tracking-firefox43: --- → +
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.