ServiceWorker's Client.id and FetchEvent.clientId do not match

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: blois, Assigned: asuth)

Tracking

(Blocks 1 bug)

59 Branch
mozilla61
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.146 Safari/537.36

Steps to reproduce:

Send a message to a service worker, see MessageEvent.source.id as being:
"XXX-XXX-XXX"

From the same client, make a fetch request that gets intercepted by the service worker, see FetchEvent.clientId as being:
"{XXX-XXX-XXX}"

From the service worker, the matching Client object does not have the surrounding brackets.


Actual results:

The two client ID strings differ in that one is surrounded with brackets.


Expected results:

The two should have been identical.

This worked correctly in FF 58 and changed in FF 59.

The docs for FetchEvent.clientId imply that it should be the same string:
https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/clientId

"The clientId read-only property of the FetchEvent returns the id of the Client that the current service worker is controlling."
Duplicate of this bug: 1447229
:past is this being looked at?
Flags: needinfo?(past)
Component: Untriaged → DOM: Service Workers
Flags: needinfo?(past)
Product: Firefox → Core
Assignee: nobody → bugmail
Priority: -- → P3
:asuth confirm the regression
Flags: needinfo?(bugmail)
Client::GetId() removes the {}'s when stringifying the nsID UUID:
https://searchfox.org/mozilla-central/source/dom/clients/api/Client.cpp#100

ContinueDispatchFetchEventRunnable::Run does not:
https://searchfox.org/mozilla-central/source/dom/serviceworkers/ServiceWorkerManager.cpp#2378
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
And the reason this doesn't pose a problem in existing tests is that Clients::Get()[1] uses nsID::Parse()[2] which parses UUIDs with or without the {}'s.

1: https://searchfox.org/mozilla-central/source/dom/clients/api/Clients.cpp#68
2: https://searchfox.org/mozilla-central/source/xpcom/base/nsID.cpp#64
Flags: needinfo?(bugmail)
Our Client.id values were being normalized from "{uuid}" to "uuid", but not
our FetchEvent.clientId values.  Because nsID::Parse accepts both forms,
this was not being caught by any tests.

Although there are ServiceWorker WPT tests that verify consistency of
returned Client.id values across multiple matchAll invocations (ex:
client-id.https.html), there are no tests that compare Client.id with
FetchEvent.clientId.  All the tests largely use Clients.get() to verify
correctness/round-tripping.  I looked into adding WPT tests, but we
quickly run into the test logistics problem where it's preferable to
avoid adding tests that involve effectively global state.

So, this patch:
- Changes Clients::Get() to explicitly treat client id's that start with a
  "{" as invalid.  This causes existing FetchEvent.clientId-related WPT
  tests to fail, as we would hope.
- Duplicates the client id normalization logic that strips {} for the
  FetchEvent.clientId to its point of origin in
  ContinueDispatchFetchEventRunnable::Run.
- Augments our dom/serviceworkers/test/test_match_all_client_properties.html
  test, which has been enforcing {}-less UUIDs for a while, to also test
  FetchEvent.clientId to verify it conforms.  I added some comments to the
  test files too.
Attachment #8961438 - Flags: review?(catalin.badea392)
Wow, thanks for fixing this!

Comment 9

a year ago
Ditto. Real quick turn around!
Attachment #8961438 - Flags: review?(catalin.badea392) → review+

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04a90ec6ed0b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please request beta uplift for this.  Thanks again.
Comment on attachment 8961438 [details] [diff] [review]
Ensure client id's are consistently {}-less UUIDs

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1293277 which landed in 59.

[User impact if declined]:
This is an edge-case related to ServiceWorkers.  Sites using ServiceWorkers to do complicated things may experience very hard-to-debug problems if they depend on Client.id equivalence with FetchEvent.clientId.

[Is this code covered by automated tests?]:
Yes. This patch includes test fixes that provide coverage.

[Has the fix been verified in Nightly?]:
Yes.  My repro steps are performed just now with today's nightly are:
- Browse to :bkelly's https://fetch-event-echo.glitch.me/ helper tool.
- Open the devtools console, ctrl-shift-k on linux.
- Type `fetch("foo")` and hit return.
- The second line after the returned Promise will look like `clientId: "989a9edc-f1ae-4065-b5f1-32af271b5837"` which is the correct format.
- (Comparing with a current beta build without this fix, we see the line looks like `clientId: "{46905635-54ce-4da8-bd69-3ddc451685e0}"`, which is bad because of the wrapping "{" and "}".)

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
None.  Patch grafts cleanly.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It's a minimal change which reuses existing, straightforward string processing code, so it's not even new code.

[String changes made/needed]:
None.
Attachment #8961438 - Flags: approval-mozilla-beta?
Comment on attachment 8961438 [details] [diff] [review]
Ensure client id's are consistently {}-less UUIDs

service workers regression fix for beta60
Attachment #8961438 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is this something that might make it into 59 as a ridealong? We don't have another dot release planned but if we do one, maybe this is upliftable.
Flags: needinfo?(bugmail)
Yes, a ridealong could make sense.  The patch appears to graft cleanly to release and shouldn't have any weird interactions with other patches.
Flags: needinfo?(bugmail)
You need to log in before you can comment on or make changes to this bug.