Closed
Bug 1193414
Opened 9 years ago
Closed 9 years ago
[Meta-Bug] Intermittent failures in many sharedWorker tests
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(2 files, 3 obsolete files)
4.98 KB,
patch
|
Details | Diff | Splinter Review | |
12.54 KB,
patch
|
khuey
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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•9 years ago
|
||
Attachment #8646458 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → amarchesini
Keywords: checkin-needed,
leave-open
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9d0ca315d2a
Assignee | ||
Comment 7•9 years ago
|
||
Here a patch to fix the issue.
Attachment #8663295 -
Flags: review?(khuey)
Assignee | ||
Comment 8•9 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•9 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•9 years ago
|
||
Attachment #8663295 -
Attachment is obsolete: true
Attachment #8664093 -
Flags: review?(khuey)
Assignee | ||
Comment 13•9 years ago
|
||
With a mochitest.
Attachment #8664093 -
Attachment is obsolete: true
Attachment #8664093 -
Flags: review?(khuey)
Attachment #8664196 -
Flags: review?(khuey)
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+
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/30c848734bea
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/30c848734bea
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 35•9 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?
Comment 36•9 years ago
|
||
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•9 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.)
Comment 40•9 years ago
|
||
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 41•9 years ago
|
||
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.
Description
•