Closed
Bug 1130065
Opened 9 years ago
Closed 9 years ago
Ensure ServiceWorkerManager captures "atomically" properly.
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(1 file)
30.98 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
Some of our algorithms do not use the job queue where they should and some don't split at the right boundaries. A fair number of intermittent failures are likely due to this. Fixes coming up.
Assignee | ||
Updated•9 years ago
|
Blocks: ServiceWorkers-v1
Assignee | ||
Comment 1•9 years ago
|
||
Try builds with patch and all our existing tests enabled https://treeherder.mozilla.org/#/jobs?repo=try&revision=083037922056 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee099f65016c (this one has 2 tests that i forgot to enable before - test_worker{Unregister,Update}.html) I'll put the patch up for review as soon as I've a couple of retriggers to spot intermittents.
Assignee | ||
Comment 2•9 years ago
|
||
Main changes are to not assert active worker in ::FinishActivate and to finish the RegistrationJob before attempting to activate. Tests are enabled on everything except b2g since they've passed successive retriggers https://treeherder.mozilla.org/#/jobs?repo=try&revision=083037922056 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee099f65016c2 - this one enables workerUnregister and workerUpdate. Folds: Enable most SW tests Cannot rely on controllerchange firing in an already controlled window. The AbortError case is no longer relevant due to FIFO ordering Too bad we have to use timeouts
Attachment #8562862 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment 3•9 years ago
|
||
Comment on attachment 8562862 [details] [diff] [review] ServiceWorkerManager capture "atomically" properly Review of attachment 8562862 [details] [diff] [review]: ----------------------------------------------------------------- can you tell me more about the tests commented out? ::: dom/workers/ServiceWorkerManager.cpp @@ -76,5 @@ > if (mWaitingWorker) { > mWaitingWorker->UpdateState(ServiceWorkerState::Redundant); > // Fire statechange. > mWaitingWorker = nullptr; > - mWaitingToActivate = false; You didn't remove this variable from Manager.h @@ +559,5 @@ > mRegistration->mScope, > getter_AddRefs(serviceWorker)); > > if (NS_WARN_IF(NS_FAILED(rv))) { > + return ContinueAfterInstallEvent(false /* aSuccess */, false /* aActivateImmediately */); why this change? @@ +1042,5 @@ > swm->CreateServiceWorker(mActiveWorker->ScriptSpec(), > mScope, > getter_AddRefs(serviceWorker)); > if (NS_WARN_IF(NS_FAILED(rv))) { > + nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethodWithArg<bool>(this, &ServiceWorkerRegistrationInfo::FinishActivate, false /* success */); indent this line. @@ +1043,5 @@ > mScope, > getter_AddRefs(serviceWorker)); > if (NS_WARN_IF(NS_FAILED(rv))) { > + nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethodWithArg<bool>(this, &ServiceWorkerRegistrationInfo::FinishActivate, false /* success */); > + NS_DispatchToMainThread(r); MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)); ::: dom/workers/ServiceWorkerManager.h @@ +48,5 @@ > NS_DECL_ISUPPORTS > > virtual void Start() = 0; > > + virtual void Dump() = 0; #ifdef DEBUG ... You don't use this method in prod builds. @@ +86,5 @@ > bool wasEmpty = mJobs.IsEmpty(); > mJobs.AppendElement(aJob); > +#ifdef DEBUG > + fprintf(stderr, "Appended job to queue "); > + aJob->Dump(); make Dump() to receive a format string and you can remove the fprintf here. Plus... this debug messages are not really part of this bug. I would like to remove it and add it in a separate bug/patch. @@ +92,4 @@ > if (wasEmpty) { > +#ifdef DEBUG > + fprintf(stderr, "Starting job immediately "); > + aJob->Dump(); same here. ::: dom/workers/test/serviceworkers/test_install_event.html @@ +45,3 @@ > ok(swr.installing.state == "installing", "Installing worker's state should be 'installing'"); > + //return new Promise(function(resolve, reject) { > + // swr.installing.onstatechange = function(e) { why this code is comment out? @@ +91,4 @@ > .then(nextRegister) > .then(installError) > + //.then(activateError) > + //.then(unregister) same here. ::: dom/workers/test/serviceworkers/test_unregister.html @@ +22,5 @@ > > function testControlled() { > var testPromise = new Promise(function(res, rej) { > window.onmessage = function(e) { > + dump("NSMR GOT MESSSAGE"); info? @@ +87,5 @@ > > function runTest() { > simpleRegister() > .then(testControlled) > + .then(function(v) { dump("\n\ntestControlled done\n\n"); }) I'm not really happy about this dump calls :) What about info?
Assignee | ||
Comment 4•9 years ago
|
||
Sorry about the debugs left in and the test code commented out. Both were mistakes.
Comment 5•9 years ago
|
||
Comment on attachment 8562862 [details] [diff] [review] ServiceWorkerManager capture "atomically" properly Review of attachment 8562862 [details] [diff] [review]: ----------------------------------------------------------------- Revert all the test changes. If you really need the Dump() method, can you write it in a separate patch?
Attachment #8562862 -
Flags: review?(amarchesini) → review+
Comment 6•9 years ago
|
||
I was going to try to include this in the next build, but it needs to be rebased now that the persistent registration patches have landed.
Assignee | ||
Comment 7•9 years ago
|
||
couldnt land due to tree being closed. i'm away till monday, but you can grab them from here https://treeherder.mozilla.org/#/jobs?repo=try&revision=98b5d0e4a7fb
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8423d5a83b
Comment 9•9 years ago
|
||
sorry i had to to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=491e81cd9cf8 - seems one of this changes was causing on multiple desktop platforms test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6670670&repo=mozilla-inbound that became frequent to perma failures starting with this csets. Could you take a look at this, thanks!
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f64c2ee33087
Comment 11•9 years ago
|
||
test_workerUpdate.html was failing nearly across the board, so I disabled it. https://hg.mozilla.org/integration/mozilla-inbound/rev/465e4d4986e1
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f64c2ee33087 https://hg.mozilla.org/mozilla-central/rev/465e4d4986e1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•