Force updating a service worker from devtools

ASSIGNED
Assigned to

Status

()

defect
P5
normal
ASSIGNED
4 years ago
2 months ago

People

(Reporter: jaoo, Assigned: jaoo)

Tracking

(Blocks 1 bug)

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 18 obsolete attachments)

40 bytes, text/x-review-board-request
Details
978 bytes, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → josea.olivera
Posted patch WIP (obsolete) — Splinter Review
Always update when pref 'dom.serviceWorkers.testing.enabled' true. Still need to update always if devtool are open.
Posted patch WIP (obsolete) — Splinter Review
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.
Attachment #8566629 - Attachment is obsolete: true
Posted patch WIP (obsolete) — Splinter Review
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?
Attachment #8568731 - Attachment is obsolete: true
Attachment #8569331 - Flags: feedback?(nsm.nikhil)
See Also: → 1003991
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.
Attachment #8569331 - Flags: feedback?(nsm.nikhil) → feedback+
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.
Posted patch WIP (obsolete) — Splinter Review
(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.
Attachment #8569331 - Attachment is obsolete: true
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #8572042 - Attachment is obsolete: true
Posted patch v2 (obsolete) — Splinter Review
(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
Attachment #8572249 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Posted patch v3 (obsolete) — Splinter Review
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.
Attachment #8573585 - Attachment is obsolete: true
Posted patch v4 (obsolete) — Splinter Review
The test I mentioned at comment 10.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34084d4c6b2
Attachment #8574578 - Attachment is obsolete: true
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.
See Also: → 1099370
Posted patch v5 (obsolete) — Splinter Review
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!
Attachment #8574928 - Attachment is obsolete: true
Attachment #8576536 - Flags: review?(nsm.nikhil)
Depends on: 1003991
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.
Attachment #8576536 - Flags: review?(nsm.nikhil)
Posted patch v6 (obsolete) — Splinter Review
(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!
Attachment #8576536 - Attachment is obsolete: true
Attachment #8578035 - Flags: review?(mratcliffe)
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/
Attachment #8578035 - Flags: review?(mratcliffe)
Posted patch v7 (obsolete) — Splinter Review
(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!
Attachment #8578035 - Attachment is obsolete: true
Attachment #8579289 - Flags: review?(mratcliffe)
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.
Attachment #8579289 - Flags: review?(mratcliffe) → review+
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!
Attachment #8579289 - Flags: review?(nsm.nikhil)
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.
Attachment #8579289 - Flags: review?(nsm.nikhil)
(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.
No longer depends on: 1003991
Posted patch v8 (obsolete) — Splinter Review
Nikhil, is that what you meant at comment 20? It's basically adding a new option as we did in bug 1003991.
Attachment #8579289 - Attachment is obsolete: true
Attachment #8580714 - Flags: feedback?(nsm.nikhil)
Posted patch v9 (obsolete) — Splinter Review
v8 with tests. Nikhil, see comment 22 please. Thanks!
Attachment #8580714 - Attachment is obsolete: true
Attachment #8580714 - Flags: feedback?(nsm.nikhil)
Attachment #8580930 - Flags: feedback?(nsm.nikhil)
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
Attachment #8580930 - Flags: feedback?(nsm.nikhil) → feedback+
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!
(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!
Flags: needinfo?(nsm.nikhil)
Posted patch v10 (obsolete) — Splinter Review
Review comments (from comment 24) addressed.
Attachment #8580930 - Attachment is obsolete: true
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.
Flags: needinfo?(nsm.nikhil)
Posted patch v11 (obsolete) — Splinter Review
This version implements the tests as Nikhil suggested at comment 28.
Attachment #8582463 - Attachment is obsolete: true
Attachment #8582672 - Flags: review?(nsm.nikhil)
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.
Attachment #8582672 - Flags: review?(nsm.nikhil)
Posted patch v12 (obsolete) — Splinter Review
Addressees review comments from comment 30. Thanks!
Attachment #8582672 - Attachment is obsolete: true
Attachment #8582982 - Flags: review?(nsm.nikhil)
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.
Attachment #8582982 - Flags: review?(nsm.nikhil) → review+
Posted patch v12 (obsolete) — Splinter Review
Addressees review comments from comment 32. Thanks!

Carrying out r=nsm

Mike, would you mind you have a look before landing please? Thanks!
Attachment #8582982 - Attachment is obsolete: true
Attachment #8583311 - Flags: review?(mratcliffe)
Attachment #8583311 - Flags: review?(mratcliffe) → review+
Lets wait until bug 931249 lands. Once it lands this one will be landed.
Keywords: checkin-needed
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.
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.
Flags: needinfo?(nsm.nikhil)
(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).
(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.
Flags: needinfo?(nsm.nikhil)
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?
(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
(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
Seeing the issue reported at bug 1151892 when running alone the tests being added here (I mean not running the whole devtool test suite.
Depends on: 1151892
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.
Depends on: 1153407
Comment on attachment 8583311 [details] [diff] [review]
v12

I'll move this to mozreview.
Attachment #8583311 - Attachment is obsolete: true
Bug 1134329 - Service worker install and activate events not fired when updating the service worker javascript file. r=nsm,ochameau
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
Attachment #8616017 - Flags: review?(poirot.alex)
Attachment #8616017 - Flags: review?(nsm.nikhil)
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
Attachment #8616017 - Flags: review?(nsm.nikhil)
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
Attachment #8589600 - Attachment is obsolete: true
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!
(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.
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.
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 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
Attachment #8616017 - Flags: review?(nsm.nikhil)
(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 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
Attachment #8616017 - Flags: review?(nsm.nikhil)

Updated

4 years ago
Blocks: ServiceWorkers-B2G
No longer blocks: ServiceWorkers-v1
Target Milestone: --- → NGA S3 (26Jun)
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.
Attachment #8616017 - Flags: review?(poirot.alex)

Updated

4 years ago
Target Milestone: NGA S3 (26Jun) → FxOS-S1 (26Jun)
Target Milestone: FxOS-S1 (26Jun) → FxOS-S2 (10Jul)
Target Milestone: FxOS-S2 (10Jul) → FxOS-S3 (24Jul)
Target Milestone: FxOS-S3 (24Jul) → FxOS-S4 (07Aug)
Target Milestone: FxOS-S4 (07Aug) → FxOS-S5 (21Aug)
Target Milestone: FxOS-S5 (21Aug) → ---
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!
Flags: needinfo?(poirot.alex)
(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
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
Flags: needinfo?(poirot.alex)
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70cbde55733f

Modifying this codepath is quite frightening, but let's see what try says.
José, the test should pass with this additional patch.
Could you rebase your patch and see if that works with it on try?
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.
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

4 years ago
Resummarizing to the best of my understanding of what this bug is about.
Blocks: ServiceWorkers-postv1
No longer blocks: ServiceWorkers-B2G
Summary: Service worker install and activate events not fired when updating the service worker javascript file → Force updating a service worker from devtools

Updated

11 months ago
Priority: -- → P5
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.