Service Worker objects not identical

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mjs, Assigned: bkelly)

Tracking

(Depends on 1 bug)

44 Branch
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45 wontfix, firefox46 fixed, firefox47 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(4 attachments)

Reporter

Description

3 years ago
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

Updated

3 years ago
Component: Untriaged → DOM: Service Workers
Product: Firefox → Core
NI myself to look.  I think this might be fixed on a later branch.
Flags: needinfo?(bkelly)
Whiteboard: btpp-active
So, bug 1201127 fixed this for registrations, but we still need to fix it for ServiceWorker objects themselves.
Flags: needinfo?(bkelly)
See Also: → 1201127
Assignee: nobody → bkelly
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8724908 - Flags: review?(ehsan)

Updated

3 years ago
Attachment #8724908 - Flags: review?(ehsan) → review+
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
Attachment #8726371 - Flags: review?(ehsan)
Its too late for 45, but we should try to get this into 46 if we can.

Updated

3 years ago
Attachment #8726371 - Flags: review?(ehsan) → review+
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.
Attachment #8724908 - Flags: approval-mozilla-aurora?

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7f4e37c4268e
https://hg.mozilla.org/mozilla-central/rev/39482a4423f1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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
Attachment #8724908 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm hitting merge conflicts on applying this to aurora.
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly) → needinfo?(lhenry)
Attachment #8724908 - Flags: approval-mozilla-beta?
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
Flags: needinfo?(lhenry)
Attachment #8724908 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Depends on: 1237992
I verified service worker mochitests and web-platform-tests locally with bug 1237992 and these patches on beta 46.
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.
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
Flags: needinfo?(bkelly)
Carsten, did you apply bug 1237992 first?
Flags: needinfo?(bkelly) → needinfo?(cbook)
I'm guessing not since bug 1237992 uplift flag is still waiting for approval.
(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)
Flags: needinfo?(cbook) → needinfo?(bkelly)
(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.
Flags: needinfo?(bkelly)
You need to log in before you can comment on or make changes to this bug.