Bug 1266747 P3 Sort output of Clients.matchAll(). r=smaug
authorBen Kelly <ben@wanderview.com>
Tue, 28 Feb 2017 22:12:27 -0500
changeset 374292 eda3f225fb73815601b84da0adf8be23c9199ef0
parent 374291 f0ef771861c23b3e4219ec5cb38c8a8b630bd113
child 374293 69a692df37c9d5e30678c4673229018c9fda0180
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1266747
milestone54.0a1
Bug 1266747 P3 Sort output of Clients.matchAll(). r=smaug
dom/workers/ServiceWorkerClient.cpp
dom/workers/ServiceWorkerClient.h
dom/workers/ServiceWorkerManager.cpp
dom/workers/test/serviceworkers/claim_worker_2.js
--- a/dom/workers/ServiceWorkerClient.cpp
+++ b/dom/workers/ServiceWorkerClient.cpp
@@ -28,18 +28,19 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(Se
 NS_IMPL_CYCLE_COLLECTING_ADDREF(ServiceWorkerClient)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(ServiceWorkerClient)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ServiceWorkerClient)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
 
-ServiceWorkerClientInfo::ServiceWorkerClientInfo(nsIDocument* aDoc)
+ServiceWorkerClientInfo::ServiceWorkerClientInfo(nsIDocument* aDoc, uint32_t aOrdinal)
   : mType(ClientType::Window)
+  , mOrdinal(aOrdinal)
   , mWindowId(0)
   , mFrameType(FrameType::None)
 {
   MOZ_ASSERT(aDoc);
   nsresult rv = aDoc->GetOrCreateId(mClientId);
   if (NS_FAILED(rv)) {
     NS_WARNING("Failed to get the UUID of the document.");
   }
@@ -77,16 +78,46 @@ ServiceWorkerClientInfo::ServiceWorkerCl
     mFrameType = FrameType::Nested;
   } else if (outerWindow->HadOriginalOpener()) {
     mFrameType = FrameType::Auxiliary;
   } else {
     mFrameType = FrameType::Top_level;
   }
 }
 
+bool
+ServiceWorkerClientInfo::operator<(const ServiceWorkerClientInfo& aRight) const
+{
+  // Note: the mLastFocusTime comparisons are reversed because we need to
+  // put most recently focused values first.  The mOrdinal comparison is
+  // normal, though, because otherwise we want normal creation order.
+
+  if (mLastFocusTime == aRight.mLastFocusTime) {
+    return mOrdinal < aRight.mOrdinal;
+  }
+
+  if (mLastFocusTime.IsNull()) {
+    return false;
+  }
+
+  if (aRight.mLastFocusTime.IsNull()) {
+    return true;
+  }
+
+  return mLastFocusTime > aRight.mLastFocusTime;
+}
+
+bool
+ServiceWorkerClientInfo::operator==(const ServiceWorkerClientInfo& aRight) const
+{
+  return mLastFocusTime == aRight.mLastFocusTime &&
+         mOrdinal == aRight.mOrdinal &&
+         mClientId == aRight.mClientId;
+}
+
 JSObject*
 ServiceWorkerClient::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
 {
   return ClientBinding::Wrap(aCx, this, aGivenProto);
 }
 
 ClientType
 ServiceWorkerClient::Type() const
--- a/dom/workers/ServiceWorkerClient.h
+++ b/dom/workers/ServiceWorkerClient.h
@@ -26,25 +26,29 @@ class ServiceWorkerWindowClient;
 // Used as a container object for information needed to create
 // client objects.
 class ServiceWorkerClientInfo final
 {
   friend class ServiceWorkerClient;
   friend class ServiceWorkerWindowClient;
 
 public:
-  explicit ServiceWorkerClientInfo(nsIDocument* aDoc);
+  explicit ServiceWorkerClientInfo(nsIDocument* aDoc, uint32_t aOrdinal = 0);
 
   const nsString& ClientId() const
   {
     return mClientId;
   }
 
+  bool operator<(const ServiceWorkerClientInfo& aRight) const;
+  bool operator==(const ServiceWorkerClientInfo& aRight) const;
+
 private:
   const mozilla::dom::ClientType mType;
+  const uint32_t mOrdinal;
   nsString mClientId;
   uint64_t mWindowId;
   nsString mUrl;
 
   // Window Clients
   VisibilityState mVisibilityState;
   FrameType mFrameType;
   TimeStamp mLastFocusTime;
--- a/dom/workers/ServiceWorkerManager.cpp
+++ b/dom/workers/ServiceWorkerManager.cpp
@@ -2969,71 +2969,74 @@ ServiceWorkerManager::GetAllClients(nsIP
 
   nsCOMPtr<nsISimpleEnumerator> enumerator;
   nsresult rv = obs->EnumerateObservers("service-worker-get-client",
                                         getter_AddRefs(enumerator));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return;
   }
 
-  auto ProcessDocument = [&aDocuments](nsIPrincipal* aPrincipal, nsIDocument* aDoc) {
-    if (!aDoc || !aDoc->GetWindow()) {
-      return;
+  // Get a list of Client documents out of the observer service
+  AutoTArray<nsCOMPtr<nsIDocument>, 32> docList;
+  bool loop = true;
+  while (NS_SUCCEEDED(enumerator->HasMoreElements(&loop)) && loop) {
+    nsCOMPtr<nsISupports> ptr;
+    rv = enumerator->GetNext(getter_AddRefs(ptr));
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      continue;
+    }
+
+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(ptr);
+    if (!doc || !doc->GetWindow()) {
+      continue;
     }
 
     bool equals = false;
-    aPrincipal->Equals(aDoc->NodePrincipal(), &equals);
+    Unused << aPrincipal->Equals(doc->NodePrincipal(), &equals);
     if (!equals) {
-      return;
+      continue;
     }
 
     // Treat http windows with devtools opened as secure if the correct devtools
     // setting is enabled.
-    if (!aDoc->GetWindow()->GetServiceWorkersTestingEnabled() &&
+    if (!doc->GetWindow()->GetServiceWorkersTestingEnabled() &&
         !Preferences::GetBool("dom.serviceWorkers.testing.enabled") &&
-        !IsFromAuthenticatedOrigin(aDoc)) {
-      return;
+        !IsFromAuthenticatedOrigin(doc)) {
+      continue;
     }
 
-    ServiceWorkerClientInfo clientInfo(aDoc);
-    aDocuments.AppendElement(aDoc);
-  };
-
-  // Since it's not simple to check whether a document is in
-  // mControlledDocuments, we take different code paths depending on whether we
-  // need to look at all documents.  The common parts of the two loops are
-  // factored out into the ProcessDocument lambda.
-  if (aIncludeUncontrolled) {
-    bool loop = true;
-    while (NS_SUCCEEDED(enumerator->HasMoreElements(&loop)) && loop) {
-      nsCOMPtr<nsISupports> ptr;
-      rv = enumerator->GetNext(getter_AddRefs(ptr));
-      if (NS_WARN_IF(NS_FAILED(rv))) {
+    // If we are only returning controlled Clients then skip any documents
+    // that are for different registrations.
+    if (!aIncludeUncontrolled) {
+      ServiceWorkerRegistrationInfo* reg = mControlledDocuments.GetWeak(doc);
+      if (!reg || reg->mScope != registration->mScope) {
         continue;
       }
-
-      nsCOMPtr<nsIDocument> doc = do_QueryInterface(ptr);
-      ProcessDocument(aPrincipal, doc);
+    }
+
+    if (!aIncludeUncontrolled && !mControlledDocuments.Contains(doc)) {
+      continue;
     }
-  } else {
-    for (auto iter = mControlledDocuments.Iter(); !iter.Done(); iter.Next()) {
-      ServiceWorkerRegistrationInfo* thisRegistration = iter.UserData();
-      MOZ_ASSERT(thisRegistration);
-      if (!registration->mScope.Equals(thisRegistration->mScope)) {
-        continue;
-      }
-
-      nsCOMPtr<nsIDocument> doc = do_QueryInterface(iter.Key());
-
-      // All controlled documents must have an outer window.
-      MOZ_ASSERT(doc->GetWindow());
-
-      ProcessDocument(aPrincipal, doc);
-    }
-  }
+
+    docList.AppendElement(doc.forget());
+  }
+
+  // The observer service gives us the list in reverse creation order.
+  // We need to maintain creation order, so reverse the list before
+  // processing.
+  docList.Reverse();
+
+  // Finally convert to the list of ServiceWorkerClientInfo objects.
+  uint32_t ordinal = 0;
+  for (uint32_t i = 0; i < docList.Length(); ++i) {
+    aDocuments.AppendElement(ServiceWorkerClientInfo(docList[i], ordinal));
+    ordinal += 1;
+  }
+
+  aDocuments.Sort();
 }
 
 void
 ServiceWorkerManager::MaybeClaimClient(nsIDocument* aDocument,
                                        ServiceWorkerRegistrationInfo* aWorkerRegistration)
 {
   MOZ_ASSERT(aWorkerRegistration);
   MOZ_ASSERT(aWorkerRegistration->GetActive());
--- a/dom/workers/test/serviceworkers/claim_worker_2.js
+++ b/dom/workers/test/serviceworkers/claim_worker_2.js
@@ -16,12 +16,15 @@ onactivate = function(e) {
 
     return claimPromise.then(self.clients.matchAll().then(function(matched) {
       // should be 1
       result.match_count_after = matched.length;
       if (result.match_count_after === 1) {
         matched[0].postMessage(result);
       } else {
         dump("ERROR: claim_worker_2 failed to capture clients.\n");
+        for (let i = 0; i < matched.length; ++i) {
+          dump('### ### matched[' + i + ']: ' + matched[i].url + '\n');
+        }
       }
     }));
   });
 }