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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P3)

Description

[blois]

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."

Comment 2

[Simon Bennetts [:psiinon]]

:past is this being looked at?

Comment 3

[Marion Daly [:mdaly]]

:asuth confirm the regression

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Ensure client id's are consistently {}-less UUIDs

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Wow, thanks for fixing this!

Comment 9

[davidscrobonia]

Ditto. Real quick turn around!

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04a90ec6ed0bcb7351a1bf49cf1a3e9e963a4df3
Bug 1446225 - Ensure client id's are consistently {}-less UUIDs. r=catalinb

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/04a90ec6ed0b

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Please request beta uplift for this.  Thanks again.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8961438 [details] [diff] [review]
Ensure client id's are consistently {}-less UUIDs

service workers regression fix for beta60

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/49448fdb2c96

Comment 16

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

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.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Yes, a ridealong could make sense.  The patch appears to graft cleanly to release and shouldn't have any weird interactions with other patches.