1134329 - Force updating a service worker from devtools

Status: NEW

(Core :: DOM: Service Workers, defect, P5)

Description

[José Antonio Olivera Ortega [:jaoo]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150218030228

Steps to reproduce:

* Run https://jaoo.github.io/service-worker-testing/index.html on top of the latest build at https://blog.wanderview.com/sw-builds/ (Builds created on February 14, 2015 either Mac build or FxOS one)
* Once the service worker gets installed and activated update the service worker js file.


Actual results:

* The new service worker should be started and the install event should be fired.
* When the new service worker takes control, its activate event should be fired.


Expected results:

None of the above happens actually.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

Always update when pref 'dom.serviceWorkers.testing.enabled' true. Still need to update always if devtool are open.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

Devtools stuff added. Still need to let the service worker manager know that devtools are open and service worker testing is enabled through an attribute to nsPIDOMWindow.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

First WIP version in which we force Service Worker scripts to update either situation 'dom.serviceWorkers.testing.enabled' pref is enabled or devtools are open plus the service worker testing checkbox is enabled. Still consider this work as WIP as it deserves a few tests. Nikhil, would you mind to have a look and provide some feedback please?

Comment 4

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

Comment on attachment 8569331 [details] [diff] [review]
WIP

Review of attachment 8569331 [details] [diff] [review]:
-----------------------------------------------------------------

You are on the right track.

::: dom/base/nsPIDOMWindow.h
@@ +181,5 @@
> +  virtual void SetServiceWorkersTestingEnabled(bool aServiceWorkersTestingEnabled)
> +  {
> +    MOZ_ASSERT(IsOuterWindow());
> +    mServiceWorkersTestingEnabled = aServiceWorkersTestingEnabled;
> +  }

Newline here.

::: dom/workers/ServiceWorkerManager.cpp
@@ +451,5 @@
>    nsCString mScriptSpec;
>    nsRefPtr<ServiceWorkerRegistrationInfo> mRegistration;
>    nsRefPtr<ServiceWorkerUpdateFinishCallback> mCallback;
>    nsCOMPtr<nsIPrincipal> mPrincipal;
> +  nsCOMPtr<nsIDOMWindow> mWindow;

Please do the computation for whether update is required in SWM::Register() and just pass the bool to the job (or even better use the other ctor form that is used for Update(), if that works). I'd rather not hold on to a window from a job that can stay around for a while.

Comment 5

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

Comment on attachment 8569331 [details] [diff] [review]
WIP

Review of attachment 8569331 [details] [diff] [review]:
-----------------------------------------------------------------

You are on the right track.

::: dom/base/nsPIDOMWindow.h
@@ +181,5 @@
> +  virtual void SetServiceWorkersTestingEnabled(bool aServiceWorkersTestingEnabled)
> +  {
> +    MOZ_ASSERT(IsOuterWindow());
> +    mServiceWorkersTestingEnabled = aServiceWorkersTestingEnabled;
> +  }

Newline here.

::: dom/workers/ServiceWorkerManager.cpp
@@ +451,5 @@
>    nsCString mScriptSpec;
>    nsRefPtr<ServiceWorkerRegistrationInfo> mRegistration;
>    nsRefPtr<ServiceWorkerUpdateFinishCallback> mCallback;
>    nsCOMPtr<nsIPrincipal> mPrincipal;
> +  nsCOMPtr<nsIDOMWindow> mWindow;

Please do the computation for whether update is required in SWM::Register() and just pass the bool to the job (or even better use the other ctor form that is used for Update(), if that works). I'd rather not hold on to a window from a job that can stay around for a while.

@@ +516,5 @@
> +            outerWindow->ServiceWorkersTestingEnabled()) {
> +          alwaysUpdate = true;
> +        }
> +        if (alwaysUpdate) {
> +          Update();

Please integrate bug 1138563.

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> Comment on attachment 8569331 [details] [diff] [review]
> WIP
> 
> Review of attachment 8569331 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the feedback.

> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +451,5 @@
> >    nsCString mScriptSpec;
> >    nsRefPtr<ServiceWorkerRegistrationInfo> mRegistration;
> >    nsRefPtr<ServiceWorkerUpdateFinishCallback> mCallback;
> >    nsCOMPtr<nsIPrincipal> mPrincipal;
> > +  nsCOMPtr<nsIDOMWindow> mWindow;
> 
> Please do the computation for whether update is required in SWM::Register()
> and just pass the bool to the job

Done.

> @@ +516,5 @@
> > +            outerWindow->ServiceWorkersTestingEnabled()) {
> > +          alwaysUpdate = true;
> > +        }
> > +        if (alwaysUpdate) {
> > +          Update();
> 
> Please integrate bug 1138563.

Done.

This patch has a few tests for the devtools bits, one of them fails and I would need some help from the fx-team. There are no other test for the service worker logic because the platform does not support chrome ServiceWorkers yet.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

This is the v1 version of the patch. Tests pass locally (I fixed the issue I commented about at comment 6). Try is closed, I'll push to try once It gets open and request review.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Try push result at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c4a81a5f0f9

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> Try push result at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c4a81a5f0f9

Too much orange there. Fix added. Test are passing locally, let see what happens now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23baf01fa256

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

The last few patches had mochitest devtool tests that didn't pass when e10s mode is on (mostly because of bug 1030318). I dropped those tests and new one are going to be added. I decided to go for bug 1003991 first because it would make things easier here. This patch depends now on bug 1003991. Tests coming here.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v4

The test I mentioned at comment 10.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34084d4c6b2

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Bug 1099370 reproduces in tests added in the patch in this bug (some mochitest-devtools tests e10s mode are not passing). My bet is another test in browser/devtools/framework/test/ is not destroying the toolbox correctly. I am following the tips at bug 1099370 so I hope to find it.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v5

Everything should be green now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef8f92e2e962

Nikhil, should we request review at someone else? I mean someone from the devtool team. I leave that to you. Thanks!

Comment 14

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

Comment on attachment 8576536 [details] [diff] [review]
v5

Review of attachment 8576536 [details] [diff] [review]:
-----------------------------------------------------------------

the .js test will need devtools review.

::: dom/workers/ServiceWorkerManager.cpp
@@ +510,5 @@
>  
>        if (mRegistration) {
> +        bool alwaysUpdate = false;
> +        if (mAlwaysUpdate ||
> +	    Preferences::GetBool("dom.serviceWorkers.testing.enabled")) {

Why are you gating on the testing.enabled pref here? If you really need to check this preference, just pass "serviceWorkersTestingEnabled || Preferences::GetBool(...)" to the RegisterJob ctor.

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v6

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #14)
> Comment on attachment 8576536 [details] [diff] [review]
> v5

Thanks for the review!
 
> Review of attachment 8576536 [details] [diff] [review]:
> -----------------------------------------------------------------

Comments addressed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93959c8cdaa

> 
> the .js test will need devtools review.

Mike, would you mind to review the devtool test bits please? Thanks!

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8578035 [details] [diff] [review]
v6

Review of attachment 8578035 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me but you still have lots of oranges on try.

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js
@@ +117,5 @@
> +    let button = doc.getElementById("button");
> +
> +    function doTheCheck() {
> +      info("Testing it registers correctly and there are three worker: an " +
> +           " installing, a wating and active worker");

s/wating/waiting/

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v7

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #16)
> Comment on attachment 8578035 [details] [diff] [review]
> v6
> 
> Review of attachment 8578035 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me but you still have lots of oranges on try.

Existing oranges at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93959c8cdaa were intermittent, they were restarted and passed.

Try results for this patch at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ee7bea08fc2

Mike, would you mind to check whether everything is fine now please? Thanks!

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8579289 [details] [diff] [review]
v7

Review of attachment 8579289 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, try looks better now... we have had lots of issues with intermittent oranges so we were just playing it safe.

Awesome work by the way.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8579289 [details] [diff] [review]
v7

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> Comment on attachment 8579289 [details] [diff] [review]
> v7
> 
> Review of attachment 8579289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yup, try looks better now... we have had lots of issues with intermittent
> oranges so we were just playing it safe.

Thanks Mike!

Nikhil, would you mind to review this please? Thanks!

Comment 20

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

Comment on attachment 8579289 [details] [diff] [review]
v7

Review of attachment 8579289 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +509,5 @@
>      , mScope(aScope)
>      , mScriptSpec(aScriptSpec)
>      , mCallback(aCallback)
>      , mPrincipal(aPrincipal)
> +    , mAlwaysUpdate(aAlwaysUpdate)

Please rename the member and the argument to mForceUpdate/aForceUpdate.

@@ +543,5 @@
>        if (mRegistration) {
> +        bool alwaysUpdate = false;
> +        if (mAlwaysUpdate) {
> +          alwaysUpdate = true;
> +        }

Just use mForceUpdate in the check below. You can get rid of this entire block.

@@ +895,5 @@
>    }
>  
>    nsCOMPtr<nsIURI> documentURI = doc->GetBaseURI();
>  
> +  bool testingEnabled = false, authenticatedOrigin = false;

each variable declaration on separate line please:
bool testingEnabled = false;
bool authenticatedOrigin = false;

@@ +900,3 @@
>    if (Preferences::GetBool("dom.serviceWorkers.testing.enabled") ||
>        serviceWorkersTestingEnabled) {
> +    authenticatedOrigin = testingEnabled = true;

Actually, this is slightly incorrect. We don't always want to update when the testing pref is enabled, because then we can't test for the 'don't update' condition.

How hard would it be to do the following:
Add a separate checkbox 'Force Service Worker update on registration' next to the 'enable serviceworker testing' that is added in bug 1003991?
That checkbox is used to control a bool forceUpdate here in Register(), then use forceUpdate in the RegisterJob ctor.

Sorry about the last minute change, but it gives more flexibility.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #20)
> Comment on attachment 8579289 [details] [diff] [review]
> v7
> 
> Review of attachment 8579289 [details] [diff] [review]:
> -----------------------------------------------------------------

> Sorry about the last minute change, but it gives more flexibility.

No worries, your request makes sense. Working on it.

Comment 22

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v8

Nikhil, is that what you meant at comment 20? It's basically adding a new option as we did in bug 1003991.

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v9

v8 with tests. Nikhil, see comment 22 please. Thanks!

Comment 24

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

Comment on attachment 8580930 [details] [diff] [review]
v9

Review of attachment 8580930 [details] [diff] [review]:
-----------------------------------------------------------------

Needs devtools sign off again, and please clarify the tests actually run, since there is a typo in them.
I don't understand how textContent is populated with "installing worker" and other such text. Is that in some other bug?

::: browser/devtools/framework/test/browser_toolbox_options_serviceworkers.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +// It tests that enabling Service Workers testing option enables the

s/It t/T/

@@ +126,5 @@
> +           " installing, a waiting and active worker");
> +      is(output.textContent,
> +         "Installing worker/Waiting worker/Active worker/",
> +         "Installing, waiting and active worker expected");
> +      toggleCheckbox(FORCEUPDATE_ELEMENT_ID).them(() => {

s/them/then/

Are you sure the tests run ;)

::: dom/workers/ServiceWorkerManager.cpp
@@ +884,5 @@
>    bool serviceWorkersTestingEnabled =
>      outerWindow->GetServiceWorkersTestingEnabled();
> +  bool forceServiceWorkerUpdateOnRegistration =
> +    outerWindow->GetForceServiceWorkerUpdateOnRegistration();
> +  

Nit: whitespace

Comment 25

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

Comment on attachment 8580930 [details] [diff] [review]
v9

Review of attachment 8580930 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/test/browser_toolbox_options_serviceworkers.js
@@ +99,5 @@
> +    function doTheCheck() {
> +      info("Testing it registers correctly and there are both an installing " +
> +           "and an active worker");
> +      is(output.textContent,
> +         "Installing worker/Active worker/",

I am concerned about timing issues here.
If the test just calls register() and checks for active after that there is no guarantee it will be non-null.

Once a SW is installed, it goes to the waiting state after which it goes to active. At any point during this, if another worker is registered, that is installing. If that gets installed, but the old worker is still 'waiting', the old one will be replaced by the new one.

At a minimum this test should check that either of waiting or active are available and installing is also available. That said, I'm not sure where this test calls register() for the second time. In addition, with Bug 931249, even if the update is forced, if the contents of the new script match the old one, no install and activate events will be sent.

Please explain what the test is doing before we review this further. Thanks!

Comment 26

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #24)
> Comment on attachment 8580930 [details] [diff] [review]
> v9
> 
> Review of attachment 8580930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Needs devtools sign off again, and please clarify the tests actually run,
> since there is a typo in them.

The version reviewed was obsolete. I noticed the typo in the test too late. I didn't want to upload a new version until having some feedback.

I'll request review again at Mike once we discuss about clarifying what and how the test does.

> I don't understand how textContent is populated with "installing worker" and
> other such text. Is that in some other bug?

The tests being added here complement the ones from bug 1003991.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #25)
> Comment on attachment 8580930 [details] [diff] [review]
> v9
> 
> Review of attachment 8580930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please explain what the test is doing before we review this further. Thanks!

Sure, the tests are a bit tricky actually. So lets try to explain them.

The navigator.serviceWorker.register() function is called three times in a row. Once the first call is done the ServiceWorkerRegistration object has an installing service worker. We enable the 'force update' checkbox in devtools. After enabling the flag, the tab is reloaded and the navigator.serviceWorker.register() function is called the second time. At this point the ServiceWorkerRegistration object has both an installing service worker and an active one. The test reloads the tab again so the navigator.serviceWorker.register() function gets called for the third time thus the ServiceWorkerRegistration object has now an installing service worker, a waiting one and an active service worker.

> ::: browser/devtools/framework/test/browser_toolbox_options_serviceworkers.js
> @@ +99,5 @@
> > +    function doTheCheck() {
> > +      info("Testing it registers correctly and there are both an installing " +
> > +           "and an active worker");
> > +      is(output.textContent,
> > +         "Installing worker/Active worker/",
> 
> I am concerned about timing issues here.
> If the test just calls register() and checks for active after that there is
> no guarantee it will be non-null.

There is no such timing issues. Let me explain myself. The `output.textContent` property gets populated once the promise returned by the navigator.serviceWorker.register() function is resolved. Once it gets populated the test continues. The test waits until the `output.textContent` property gets populated by using a button and its `onclick` events. To sum up, once the promise resolves the `output.textContent` property is populated and the button is clicked. As the test is waiting for that click once the `onclick` event is fired the test continues and do the check.

Are we ok now? Please, let me know. Thanks!

Comment 27

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v10

Review comments (from comment 24) addressed.

Comment 28

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

After talking with :jaoo on IRC, we determined that the test procedure is acceptable, but it will fall prey to changes in Bug 931249. The current solution :jaoo will implement is to have register() call a .sjs URL which will return slightly different 'worker scripts' on each call to the same URL. What this bug tries to fix+test is Step 4.2.2.1 of the Register algorithm - http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#register-algorithm

So we have to have an active worker, then call register with the same URL and if the devtools pref is set, we should skip step 4.2.2.1 and proceed with Update. Bug 931249 deals with the later step in Update where byte-for-byte equality is checked. Byte-for-byte equal scripts will abort the update, so that no new installing worker is set, which will trip up this test, hence the fix suggested above.

Comment 29

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v11

This version implements the tests as Nikhil suggested at comment 28.

Comment 30

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

Comment on attachment 8582672 [details] [diff] [review]
v11

Review of attachment 8582672 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/test/serviceworker.sjs
@@ +1,4 @@
> +  /* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function handleRequest(request, response) {

Block comment explaining what this test does.

@@ +5,5 @@
> +  let serviceWorkers = [
> +    "// empty service worker for testing https only restriction",
> +    "// empty service worker for testing installing worker",
> +    "// empty service worker for testing active worker",
> +    "// empty service worker for testing waiting worker",

I'd like comments for why these tests are ordered in this way.

@@ +7,5 @@
> +    "// empty service worker for testing installing worker",
> +    "// empty service worker for testing active worker",
> +    "// empty service worker for testing waiting worker",
> +  ];
> +  let index = +getState("index");

If getState returns null or something the first time, i'd rather use |getState() || 0| than JS type coercion. Also, use parseInt.

@@ +9,5 @@
> +    "// empty service worker for testing waiting worker",
> +  ];
> +  let index = +getState("index");
> +
> +  response.write(serviceWorkers[index] || "// empty service worker");

Why do you need "// empty service worker" if serviceWorkers[0] will always match? If this is called too many times, then respond with a 500 error code rather than empty service worker.

Comment 31

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v12

Addressees review comments from comment 30. Thanks!

Comment 32

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

Comment on attachment 8582982 [details] [diff] [review]
v12

Review of attachment 8582982 [details] [diff] [review]:
-----------------------------------------------------------------

assuming you only changed serviceworker.sjs, update the comments and land it! Thanks.

::: browser/devtools/framework/test/serviceworker.sjs
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// For bug 1134329 we need to test that several calls to
> +// navigator.serviceWorker.register() with the same script URL and if the

s/and if/with

@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// For bug 1134329 we need to test that several calls to
> +// navigator.serviceWorker.register() with the same script URL and if the
> +// devtools 'force update' pref is set skip [[Register]] step 4.2.2.1 and the

s/is//
s/set/set,/

@@ +11,5 @@
> +function handleRequest(request, response) {
> +  // These are the different service worker scripts we will return. The order
> +  // does not really matter but the comment in them gives us an idea about what
> +  // script will be use for each test. The goal is the script we return differs
> +  // byte-for-byte from the prevoiusly script returned.

Nit: previously

@@ +22,5 @@
> +  let index = parseInt(getState("index")) || 0;
> +
> +  // This server-side JavaScript file will be requested to return more service
> +  // worker scritps than the ones we defined above. This is because while
> +  // clearing the pref in the devtool option panel the tab is reloaded so let's

Period after reloaded. Capitalize s in So.

Comment 33

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v12

Addressees review comments from comment 32. Thanks!

Carrying out r=nsm

Mike, would you mind you have a look before landing please? Thanks!

Comment 34

[José Antonio Olivera Ortega [:jaoo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0b0b0a145f5

Comment 35

[José Antonio Olivera Ortega [:jaoo]]

Lets wait until bug 931249 lands. Once it lands this one will be landed.

Comment 36

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

Problem landing this after 931249 landed is that the sjs is being queried twice per script:
My hypothetical scenario of why this happens:
1) start with no registration 2) first call to register() makes one network request, it gets cached. 3) before (2) can be activated, register() is called again (assuming your test runs quickly), so [[Update]] is called again, which now makes a request so that it can compare against the cache

to fix this while avoiding intermittents the test should either 1) slow down to wait until previous reg gets activated 2) have activation fail (by having the worker cause an error in onactivate) so that it never activates and always hits the network

:jaoo, from next time please put information relevant to the bug in bugzilla or a public channel so it is archived. You put some pastebin results that confirmed the sjs receiving requests twice, but I'd like to know if that is due to my hypothesis.

Comment 37

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Test the sjs is being queried twice per script

This simple test shows how the JS server-side file is queried twice per registration. There is only one call to register(). Just apply only this patch on top of latest m-c a give it a try please.

Comment 38

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #37)
> Created attachment 8589600 [details] [diff] [review]
> Test the sjs is being queried twice per script
> 
> This simple test shows how the JS server-side file is queried twice per
> registration. There is only one call to register(). Just apply only this
> patch on top of latest m-c a give it a try please.

I see there are a couple of queries before the registration gets activated, the first one is made when comparing at serviceWorkerScriptCache::Compare() (https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#767) and the second one is made after seeing the result of the comparison at ServiceWorkerRegisterJob::ComparisonResult() (https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#594).

Comment 39

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #38)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #37)
> > Created attachment 8589600 [details] [diff] [review]
> > Test the sjs is being queried twice per script
> > 
> > This simple test shows how the JS server-side file is queried twice per
> > registration. There is only one call to register(). Just apply only this
> > patch on top of latest m-c a give it a try please.
> 
> I see there are a couple of queries before the registration gets activated,
> the first one is made when comparing at serviceWorkerScriptCache::Compare()
> (https://mxr.mozilla.org/mozilla-central/source/dom/workers/
> ServiceWorkerManager.cpp#767) and the second one is made after seeing the
> result of the comparison at ServiceWorkerRegisterJob::ComparisonResult()
> (https://mxr.mozilla.org/mozilla-central/source/dom/workers/
> ServiceWorkerManager.cpp#594).

This is happening because the comparison results in a different script and then the loader loads the script again so it can cache it (because the current cached copy is old). It would be nice if we could integrate the two steps. Please file a follow up blocking ServiceWorkers-v2. Meanwhile let's land the test with the double entries.

Comment 40

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

Jose Antonio, the twice network hit has a patch up (bug 1154494), but I'm not sure if that will help in your case, because as i pointed out, your test may be calling register() too quickly. Could you try it out?

Comment 41

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #40)
> Jose Antonio, the twice network hit has a patch up (bug 1154494), but I'm
> not sure if that will help in your case, because as i pointed out, your test
> may be calling register() too quickly. Could you try it out?

Sure, I'll give a try and let you know. I was ready to land this as the latest try seems really good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e5d1720bc99

Comment 42

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #40)
> Jose Antonio, the twice network hit has a patch up (bug 1154494), but I'm
> not sure if that will help in your case, because as i pointed out, your test
> may be calling register() too quickly. Could you try it out?

It helped, thanks. Tests are passing locally with that new patch. Lets see the try results at:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93df9b9babb7

Comment 43

[José Antonio Olivera Ortega [:jaoo]]

Seeing the issue reported at bug 1151892 when running alone the tests being added here (I mean not running the whole devtool test suite.

Comment 44

[José Antonio Olivera Ortega [:jaoo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1886efc1d48

That push to try includes the patches from the bug that are blocking this one. It looks good so once they land we will land this one.

Comment 45

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8583311 [details] [diff] [review]
v12

I'll move this to mozreview.

Comment 46

[José Antonio Olivera Ortega [:jaoo]]

Attachment: MozReview Request: Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Comment 47

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8616017 [details]
MozReview Request: Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Comment 48

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8616017 [details]
MozReview Request: Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Comment 49

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8616017 [details]
MozReview Request: Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Comment 50

[José Antonio Olivera Ortega [:jaoo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c46bba26d693

Comment 51

[Alexandre Poirot [:ochameau]]

Do we really need another option dedicated to service worker?
Shouldn't we apply the pref when we enable the service workers, via the existing checkbox in devtools?
If that helps we can rename the existing checkbox label to better match its behavior!

Comment 52

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Alexandre Poirot [:ochameau] from comment #51)
> Do we really need another option dedicated to service worker?

Yes, we do.

> Shouldn't we apply the pref when we enable the service workers, via the
> existing checkbox in devtools?

No, we should not. It was that way at the beginningm, but that seemed wrong. See the reason bellow.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #20)
> Comment on attachment 8579289 [details] [diff] [review]
> v7
> 
> Review of attachment 8579289 [details] [diff] [review]:
> -----------------------------------------------------------------

> Actually, this is slightly incorrect. We don't always want to update when
> the testing pref is enabled, because then we can't test for the 'don't
> update' condition.
> 
> How hard would it be to do the following:
> Add a separate checkbox 'Force Service Worker update on registration' next
> to the 'enable serviceworker testing' that is added in bug 1003991?
> That checkbox is used to control a bool forceUpdate here in Register(), then
> use forceUpdate in the RegisterJob ctor.
> 
> Sorry about the last minute change, but it gives more flexibility.

Comment 53

[Alexandre Poirot [:ochameau]]

https://reviewboard.mozilla.org/r/10393/#review9285

Ok, ok, let's go with another option.
But if you add yet another one, it will be useful to introduce a "Service worker" paragraph in the options menu!

Looks good overall, but try is still unhappy, mochitest-devtools are failing on linux e10s.

AsyncShutdown timeout in ShutdownLeaks: Wait for cleanup to be finished before checking for leaks Conditions: [{"name":"DevTools: Wait until toolbox is destroyed","state":"(none)","filename":"resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js","lineNumber":1855,"stack":["resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:Toolbox.prototype.destroy/leakCheckObse

That error means that toolbox.destroy() failed to resolved.
Either it had an exception poorly handled, that doesn't resolve/reject the promise,
or, one of the underlying promise just stay stalled and never resolves.
The code you are modifying in toolbox.js is related to toolbox.destroy.

::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd:112
(Diff revision 3)
> +<!ENTITY options.serviceWorkersForceUpdateOnRegistration.label     "Force Service Workers to be update on registration (when toolbox is open)">

I think you should either say:
  Workers to be updated
or
  Workers to update

::: browser/devtools/framework/toolbox.js:1791
(Diff revision 3)
> +        "forceServiceWorkersUpdateOnRegistration": false

This is useless as this code is for remote runtime before Firefox 41, which doesn't implement forceServiceWorkersUpdateOnRegistration.

::: dom/workers/ServiceWorkerManager.cpp:732
(Diff revision 3)
> +    , mForceUpdate(aForceUpdate)    

Note the trailing space here.

Comment 54

[José Antonio Olivera Ortega [:jaoo]]

https://reviewboard.mozilla.org/r/10393/#review9381

> This is useless as this code is for remote runtime before Firefox 41, which doesn't implement forceServiceWorkersUpdateOnRegistration.

Oh, I wasn't aware of it. Thanks!

> I think you should either say:
>   Workers to be updated
> or
>   Workers to update

I prefer to keep as it is because we already have 'Service Worker' in the existing service worker checkbox option.

> Note the trailing space here.

Thanks!

Comment 55

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8616017 [details]
MozReview Request: Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Comment 56

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #54)
> https://reviewboard.mozilla.org/r/10393/#review9381

> > I think you should either say:
> >   Workers to be updated
> > or
> >   Workers to update
> 
> I prefer to keep as it is because we already have 'Service Worker' in the
> existing service worker checkbox option.

Oh, go it. I'll change it. Sorry.

Comment 57

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8616017 [details]
MozReview Request: Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Comment 58

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8616017 [details]
MozReview Request: Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau

Cancel the review request until I address the mochitest-devtools fail on linux e10s.

Comment 59

[José Antonio Olivera Ortega [:jaoo]]

Alexandre, I had the chance to work on this again so I did some changes and pushed to try again. What we needed to figure out was the mochitest-devtools tests still failing on linux e10s. Well, the last try I did (see it a [1] please) shows the issues are already happening (even filed) so there is not something we are adding here. Since I did every possible change, I'm out of ideas and the failing tests are already present I would need to know how to continue here, what should we do at this point? Thanks!

Comment 60

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #59)
> Alexandre, I had the chance to work on this again so I did some changes and
> pushed to try again. What we needed to figure out was the mochitest-devtools
> tests still failing on linux e10s. Well, the last try I did (see it a [1]
> please) shows the issues are already happening (even filed) so there is not
> something we are adding here. Since I did every possible change, I'm out of
> ideas and the failing tests are already present I would need to know how to
> continue here, what should we do at this point? Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=409fc6d6b484

Comment 61

[Alexandre Poirot [:ochameau]]

https://reviewboard.mozilla.org/r/10395/#review16681

::: browser/devtools/framework/toolbox.js:359
(Diff revision 4)
> +      this._applyServiceWorkersForceUpdateOnRegistrationFlag();

I don't know why, but if I comment this line, the tests pass...
I'm wondering if that's just a race?

I can easily reproduce the error by running just this test:
./mach mochitest  browser/devtools/netmonitor/test/browser_net_aaa_leaktest.js --e10s

Comment 62

[Alexandre Poirot [:ochameau]]

What I'm seeing is that adding yet another "reconfigure" request in toolbox creation process,
ends up racing some short living tests.
We pile up more and more reconfigure requests on toolbox creation,
these requests are being process while the test is running other requests.
And piling this new one ends up overlapping test end.
We are still processing the new service worker reconfigure request while the test is already over.
The tab is closed, its message manager is destroyed, we fake a tabDetached message from here:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#1034
but we don't close the connection.
The reconfigure request had time to reach the content process, but the response didn't. The message manager is closed before the response had time to reach the parent process.
Then, the toolbox destruction code call DebuggerClient.close() which call each "Client" instances "detach" methods and ends up stuck on the TabActor one, which is still waiting for the reconfigure response.

Comment 63

[Alexandre Poirot [:ochameau]]

Attachment: toolbox race patch v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70cbde55733f

Modifying this codepath is quite frightening, but let's see what try says.

Comment 64

[Alexandre Poirot [:ochameau]]

José, the test should pass with this additional patch.
Could you rebase your patch and see if that works with it on try?

Comment 65

[Alexandre Poirot [:ochameau]]

Hum. Actually my patch in attachment 8658634 [details] [diff] [review] doesn't make sense.
This is really complex. We have some requests (here, the reconfigure request done on toolbox creation for service worker), that stay stalled when the tab/child process closes.
There is no warning about this and it ends up preventing client.close from finishing as we are waiting for ever on the reconfigure request to end.
I imagine this can happen in various other cases. To fix that, we would need to reject all request done to a child that closes but that's all but simple.

Comment 66

[Alexandre Poirot [:ochameau]]

https://reviewboard.mozilla.org/r/10395/#review16803

::: browser/devtools/framework/toolbox.js:359
(Diff revision 4)
> +      this._applyServiceWorkersForceUpdateOnRegistrationFlag();

I think, the easiest here would be to yield on all these _apply* methods, so that the toolbox creation wait for all these request to be done before proceeding.

Comment 67

[(no longer active)]

Resummarizing to the best of my understanding of what this bug is about.

Comment 68

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 69

[Jens Stutte [:jstutte]]

Is this still a thing?