ServiceWorkerManager stores a single RegistrationDataPerPrincipal for all principals with same origin attributes

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
While investigating another bug I found that we are not actually storing separate RegistrationDataPerPrincipal structures for each principal.  Instead we are storing a single struct for all principals with the same extended origin attributes.  In desktop this mean nearly all principals end up with a single struct.

This will mostly work, but it means all domains are sharing job queues, etc.  We also have much slower lookup of registrations by scope since its O(n) across all registrations in the system.
(Assignee)

Comment 1

2 years ago
Created attachment 8726949 [details] [diff] [review]
Store separate service worker registration per principal correctly. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=234a9f4c3d03
(Assignee)

Comment 2

2 years ago
Created attachment 8727547 [details] [diff] [review]
Store separate service worker registration per principal correctly. r=baku
Attachment #8726949 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
Try is looking pretty green:

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

I'm going to clean up the patch for review.
(Assignee)

Comment 4

2 years ago
Created attachment 8727607 [details] [diff] [review]
P1 Require an explicit principal when looking up a service worker registration. r=baku

In preparation for using something other than origin attributes for the scope key, require all clients to pass a full nsIPrincipal when looking up a registration.
Attachment #8727547 - Attachment is obsolete: true
Attachment #8727607 - Flags: review?(amarchesini)
(Assignee)

Comment 5

2 years ago
Created attachment 8727608 [details] [diff] [review]
P2 Consistently use "scope key" terminology in ServiceWorkerManager. r=baku

Also rename variable to use the "scope key" terminology instead of referencing the origin suffix.

I stopped short of creating an opaque ScopeKey type because its nice to be able to just pass strings around.
Attachment #8727608 - Flags: review?(amarchesini)
(Assignee)

Comment 6

2 years ago
Created attachment 8727609 [details] [diff] [review]
P3 Use origin the ServiceWorkerManager scope key. r=baku

Change PrincipalToScopeKey() to use nsIPrincipal::GetOrigin().  This ensures that the hash table keys for different principals do not collide.  The origin string produced by nsIPrincipal is guaranteed to contain the suffix if present.

Also assert that we are not setting empty scope keys in the future.
Attachment #8727609 - Flags: review?(amarchesini)
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9de29d0937
Attachment #8727607 - Flags: review?(amarchesini) → review+
Attachment #8727608 - Flags: review?(amarchesini) → review+
Attachment #8727609 - Flags: review?(amarchesini) → review+

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e35f587fa328
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb0e778ab75
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ccab7d6447f

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e35f587fa328
https://hg.mozilla.org/mozilla-central/rev/ffb0e778ab75
https://hg.mozilla.org/mozilla-central/rev/8ccab7d6447f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.