1130065 - Ensure ServiceWorkerManager captures "atomically" properly.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: 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

Comment 3

[Andrea Marchesini [:baku]]

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?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Sorry about the debugs left in and the test code commented out. Both were mistakes.

Comment 5

[Andrea Marchesini [:baku]]

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?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8423d5a83b

Comment 9

[Carsten Book [:Tomcat]]

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!

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f64c2ee33087

Comment 11

[Ryan VanderMeulen [:RyanVM]]

test_workerUpdate.html was failing nearly across the board, so I disabled it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/465e4d4986e1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f64c2ee33087
https://hg.mozilla.org/mozilla-central/rev/465e4d4986e1