Bug 1446225 - Ensure client id's are consistently {}-less UUIDs. r=catalinb, a=jcristau
authorAndrew Sutherland <asutherland@asutherland.org>
Thu, 22 Mar 2018 11:33:13 -0400
changeset 462850 49448fdb2c96b17a290395110f3492870ccfebd5
parent 462849 79c7a5ef721f97f3bd08eca89d26ed81562e51fd
child 462851 1d6d0bc1cb27eb3e90fa33367869297b451ddb9c
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscatalinb, jcristau
bugs1446225
milestone60.0
Bug 1446225 - Ensure client id's are consistently {}-less UUIDs. r=catalinb, a=jcristau 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.
dom/clients/api/Clients.cpp
dom/serviceworkers/ServiceWorkerManager.cpp
dom/serviceworkers/test/match_all_clients/match_all_controlled.html
dom/serviceworkers/test/match_all_properties_worker.js
--- a/dom/clients/api/Clients.cpp
+++ b/dom/clients/api/Clients.cpp
@@ -60,17 +60,21 @@ Clients::Get(const nsAString& aClientID,
   workerPrivate->AssertIsOnWorkerThread();
 
   RefPtr<Promise> outerPromise = Promise::Create(mGlobal, aRv);
   if (aRv.Failed()) {
     return outerPromise.forget();
   }
 
   nsID id;
-  if (!id.Parse(NS_ConvertUTF16toUTF8(aClientID).get())) {
+  // nsID::Parse accepts both "{...}" and "...", but we only emit the latter, so
+  // forbid strings that start with "{" to avoid inconsistency and bugs like
+  // bug 1446225.
+  if (aClientID.IsEmpty() || aClientID.CharAt(0) == '{' ||
+      !id.Parse(NS_ConvertUTF16toUTF8(aClientID).get())) {
     // Invalid ID means we will definitely not find a match, so just
     // resolve with undefined indicating "not found".
     outerPromise->MaybeResolveWithUndefined();
     return outerPromise.forget();
   }
 
   const PrincipalInfo& principalInfo = workerPrivate->GetPrincipalInfo();
   nsCOMPtr<nsISerialEventTarget> target =
--- a/dom/serviceworkers/ServiceWorkerManager.cpp
+++ b/dom/serviceworkers/ServiceWorkerManager.cpp
@@ -2371,17 +2371,20 @@ public:
 
     nsString clientId;
     nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();
     if (loadInfo) {
       Maybe<ClientInfo> clientInfo = loadInfo->GetClientInfo();
       if (clientInfo.isSome()) {
         char buf[NSID_LENGTH];
         clientInfo.ref().Id().ToProvidedString(buf);
-        CopyUTF8toUTF16(nsDependentCString(buf), clientId);
+        NS_ConvertASCIItoUTF16 uuid(buf);
+
+        // Remove {} and the null terminator
+        clientId.Assign(Substring(uuid, 1, NSID_LENGTH - 3));
       }
     }
 
     rv = mServiceWorkerPrivate->SendFetchEvent(mChannel, mLoadGroup, clientId,
                                                mIsReload);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       HandleError();
     }
--- a/dom/serviceworkers/test/match_all_clients/match_all_controlled.html
+++ b/dom/serviceworkers/test/match_all_clients/match_all_controlled.html
@@ -19,31 +19,36 @@
   } else if (parent != window) {
     frameType = "top-level";
   } else {
     postResult(false, "Unexpected frameType");
   }
 
   window.onload = function() {
     navigator.serviceWorker.ready.then(function(swr) {
+        // Send a message to our SW that will cause it to do clients.matchAll()
+        // and send a message *to each client about themselves* (rather than
+        // replying directly to us with all the clients it finds).
         swr.active.postMessage("Start");
     });
   }
 
   function postResult(result, msg) {
     response = {
       result: result,
       message: msg
     };
 
     testWindow.postMessage(response, "*");
   }
 
-  navigator.serviceWorker.onmessage = function(msg) {
-    // worker message;
+  navigator.serviceWorker.onmessage = async function(msg) {
+    // ## Verify the contents of the SW's serialized rep of our client info.
+    // Clients are opaque identifiers at a spec level, but we want to verify
+    // that they are UUID's *without wrapping "{}" characters*.
     result = re.test(msg.data.id);
     postResult(result, "Client id test");
 
     result = msg.data.url == window.location;
     postResult(result, "Client url test");
 
     result = msg.data.visibilityState === document.visibilityState;
     postResult(result, "Client visibility test. expected=" +document.visibilityState);
@@ -52,16 +57,26 @@
     postResult(result, "Client focus test. expected=" + document.hasFocus());
 
     result = msg.data.frameType === frameType;
     postResult(result, "Client frameType test. expected=" + frameType);
 
     result = msg.data.type === "window";
     postResult(result, "Client type test. expected=window");
 
+    // ## Verify the FetchEvent.clientId
+    // In bug 1446225 it turned out we provided UUID's wrapped with {}'s.  We
+    // now also get coverage by forcing our clients.get() to forbid UUIDs
+    // with that form.
+
+    const clientIdResp = await fetch('clientId');
+    const fetchClientId = await clientIdResp.text();
+    result = re.test(fetchClientId);
+    postResult(result, "Fetch clientId test");
+
     postResult(true, "DONE");
     window.close();
   };
 </script>
 
 </head>
 <body>
 </body>
--- a/dom/serviceworkers/test/match_all_properties_worker.js
+++ b/dom/serviceworkers/test/match_all_properties_worker.js
@@ -1,8 +1,15 @@
+onfetch = function(e) {
+  if (/\/clientId$/.test(e.request.url)) {
+    e.respondWith(new Response(e.clientId));
+    return;
+  }
+}
+
 onmessage = function(e) {
   dump("MatchAllPropertiesWorker:" + e.data + "\n");
   self.clients.matchAll().then(function(res) {
     if (!res.length) {
       dump("ERROR: no clients are currently controlled.\n");
     }
 
     for (i = 0; i < res.length; i++) {