1252055 - Service Worker objects not identical

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Michael Stillwell]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

There are several ways of getting access to what should be the the same ServiceWorker: ServiceWorkerContainer.controller, ServiceWorkerRegistration.active, and via the ServiceWorkerContainer.oncontrollerchange event.

On Chrome, these objects are identical, but in FF, ServiceWorkerRegistration.active is different to the other two.

Reproduce code:

    navigator.serviceWorker.register("skipwaitingandclaim.js").then(function(r) {
      navigator.serviceWorker.addEventListener('controllerchange', function(e) {
        var c = navigator.serviceWorker.controller;
        var a = r.active;
        var t = e.target.controller;
        console.log("CONTROLLERCHANGE");
        console.log("c", c);
        console.log("a", a);
        console.log("t", t);
        console.log("c === a", c === a); // Chrome: true, FF: false
        console.log("c === t", c === t); // Chrome: true, FF: true
        console.log("a === t", a === t); // Chrome: true, FF: false
      });
    });

skipwaitingandclaim.js:

self.addEventListener('install', function (event) {
  event.waitUntil(self.skipWaiting());
});

self.addEventListener('activate', function (event) {
  event.waitUntil(self.clients.claim());
});

Working example:

https://rawgit.com/ithinkihaveacat/dadd4ccab60c74e6656f/raw/aa0d031144c35193e46c32c8a3b30670a953b432/controllerchange.html


Actual results:

c === a false
c === t true
a === t false


Expected results:

c === a true
c === t true
a === t true

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

NI myself to look.  I think this might be fixed on a later branch.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

So, bug 1201127 fixed this for registrations, but we still need to fix it for ServiceWorker objects themselves.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make equivalent ServiceWorker DOM objects strictly equal in js. r=ehsan

This fixes the comment 0 case for me.  It was relatively easy since we already track the ServiceWorker DOM object instances for each underlying ServiceWorkerInfo object.

I'll look at tests next.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update tests to verify ServiceWorker object equality. r=ehsan

This adds some tests for equality in the wpt tests.  It also fixes a problem in our mochitests that relied on ServiceWorker objects being different.

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Its too late for 45, but we should try to get this into 46 if we can.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4e37c4268e
https://hg.mozilla.org/integration/mozilla-inbound/rev/39482a4423f1

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8724908 [details] [diff] [review]
P1 Make equivalent ServiceWorker DOM objects strictly equal in js. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: This is a compat issue that might break sites that currently work in chrome.
[Describe test coverage new/current, TreeHerder]:  Tests included in P2 patch here.
[Risks and why]: Minimal.  Only affects service workers and simply exposes information we were already tracking.
[String/UUID change made/needed]: None.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f4e37c4268e
https://hg.mozilla.org/mozilla-central/rev/39482a4423f1

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724908 [details] [diff] [review]
P1 Make equivalent ServiceWorker DOM objects strictly equal in js. r=ehsan

While this hasn't had a lot of time or testing, this adds new tests and may help web compatibility. Please uplift to aurora

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm hitting merge conflicts on applying this to aurora.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8724908 [details] [diff] [review]
P1 Make equivalent ServiceWorker DOM objects strictly equal in js. r=ehsan

Re-request approval to uplift since I failed to rebase this before 46 merged to beta.  Sorry for my delay!

See comment 7 again for reasons for uplift.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724908 [details] [diff] [review]
P1 Make equivalent ServiceWorker DOM objects strictly equal in js. r=ehsan

Please uplift to beta, aiming to still get this in beta 1 and go to build later today

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I have rebased patches, but I'm getting mochitest intermittents.  I think we need to uplift bug 1237992 as well if we can.  Running some local tests with that now.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Beta 46 P1 rebased on top of bug 1237992

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Beta 46 P2 rebased on top of bug 1237992

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I verified service worker mochitests and web-platform-tests locally with bug 1237992 and these patches on beta 46.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, let's keep this and the patch in 1237992 back for beta 2. I don't think we need to hold beta 1 for it.

Comment 18

[Carsten Book [:Tomcat]]

has problems to apply to beta:

adding 1252055 to series file
renamed 1252055 -> bug1252055_p2_test.patch
applying bug1252055_p2_test.patch
patching file dom/workers/test/serviceworkers/test_claim_oninstall.html
Hunk #1 FAILED at 31
1 out of 1 hunks FAILED -- saving rejects to file dom/workers/test/serviceworkers/test_claim_oninstall.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1252055_p2_test.patch

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Carsten, did you apply bug 1237992 first?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I'm guessing not since bug 1237992 uplift flag is still waiting for approval.

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Ben Kelly [:bkelly] from comment #20)
> I'm guessing not since bug 1237992 uplift flag is still waiting for approval.

yeah but that landed now, still getting conflicts on beta like:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 7f4e37c4268e
grafting 332001:7f4e37c4268e "Bug 1252055 P1 Make equivalent ServiceWorker DOM objects strictly equal in js. r=ehsan"
merging dom/workers/ServiceWorker.h
merging dom/workers/ServiceWorkerManager.cpp
merging dom/workers/ServiceWorkerManager.h
warning: conflicts while merging dom/workers/ServiceWorker.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/workers/ServiceWorkerManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Carsten Book [:Tomcat] from comment #21)
> Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 7f4e37c4268e

Oh, you need to use the beta re-based patches I previously attached to this bug.  I'll land them.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/94f0993b4d00
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/38cd24c015e9