Ensure ServiceWorkerManager captures "atomically" properly.

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

33 Branch
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

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.
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.
Created attachment 8562862 [details] [diff] [review]
ServiceWorkerManager capture "atomically" properly

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: nobody → nsm.nikhil
Status: NEW → ASSIGNED
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?
Sorry about the debugs left in and the test code commented out. Both were mistakes.
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+
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.
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
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!
test_workerUpdate.html was failing nearly across the board, so I disabled it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/465e4d4986e1
Depends on: 1134067
https://hg.mozilla.org/mozilla-central/rev/f64c2ee33087
https://hg.mozilla.org/mozilla-central/rev/465e4d4986e1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.