Bug 1584007 - let ClientChannelHelperParent manage FutureClientSourceParent lifetime r=dom-workers-and-storage-reviewers,mattwoodrow,asuth
authorPerry Jiang <perry@mozilla.com>
Tue, 24 Mar 2020 15:18:31 +0000
changeset 520468 0d130a64b0f5c16db4995d3f27f2e6b21c656977
parent 520467 8ca43f80bdd229df6d76e9c6598ac2200567bb0a
child 520469 c626a363823edf7b8ba37e7915a2c339c6f9318d
push id37251
push usermalexandru@mozilla.com
push dateThu, 26 Mar 2020 09:33:08 +0000
treeherdermozilla-central@3e5a7430c8d7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdom-workers-and-storage-reviewers, mattwoodrow, asuth
bugs1584007
milestone76.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1584007 - let ClientChannelHelperParent manage FutureClientSourceParent lifetime r=dom-workers-and-storage-reviewers,mattwoodrow,asuth ClientChannelHelperParent is the thing creating the ClientInfos which aren't backed by existing ClientSources, so it may make sense for CCHP to tell the ClientManagerService (CMS) to "expect" or "forget" a "future" ClientSource(Parent). When such a ClientInfo is created, CCHP notifies the CMS that a future ClientSource may be created. This notification has to be observed before any ClientHandles try to query CMS to a ClientSourceParent, which is the case because the notification as well as ClientHandleParent constructors occur over PBackground, and the notification sending method is called first. CMS is told to forget the future ClientSource whenever a redirect occurs that would result in the creation of a new ClientSource (i.e. a new ClientInfo). It's also possible that the ClientInfo's LoadInfo's channel is cancelled. To account for this, CHCP stores the most recent ClientInfo it's created and tells CMS to _possibly_ forget the associated future ClientSource in its destructor. It's possible that the channel completed its load, in which case this notification is a no-op. This also relies on CHCP being destroyed after the reserved ClientSource has a chance to both be created and register its ClientSourceParent. Differential Revision: https://phabricator.services.mozilla.com/D66529
dom/clients/manager/ClientChannelHelper.cpp
--- a/dom/clients/manager/ClientChannelHelper.cpp
+++ b/dom/clients/manager/ClientChannelHelper.cpp
@@ -158,53 +158,79 @@ class ClientChannelHelper : public nsIIn
 
  public:
   ClientChannelHelper(nsIInterfaceRequestor* aOuter,
                       nsISerialEventTarget* aEventTarget)
       : mOuter(aOuter), mEventTarget(aEventTarget) {}
 
   NS_DECL_ISUPPORTS
 
-  static void CreateClientForPrincipal(nsILoadInfo* aLoadInfo,
-                                       nsIPrincipal* aPrincipal,
-                                       nsISerialEventTarget* aEventTarget) {
+  virtual void CreateClientForPrincipal(nsILoadInfo* aLoadInfo,
+                                        nsIPrincipal* aPrincipal,
+                                        nsISerialEventTarget* aEventTarget) {
     // Create the new ClientSource.  This should only happen for window
     // Clients since support cross-origin redirects are blocked by the
     // same-origin security policy.
     UniquePtr<ClientSource> reservedClient = ClientManager::CreateSource(
         ClientType::Window, aEventTarget, aPrincipal);
     MOZ_DIAGNOSTIC_ASSERT(reservedClient);
 
     aLoadInfo->GiveReservedClientSource(std::move(reservedClient));
   }
 };
 
 NS_IMPL_ISUPPORTS(ClientChannelHelper, nsIInterfaceRequestor,
                   nsIChannelEventSink);
 
 class ClientChannelHelperParent final : public ClientChannelHelper {
-  ~ClientChannelHelperParent() = default;
+  ~ClientChannelHelperParent() {
+    // This requires that if a load completes, the associated ClientSource is
+    // created and registers itself before this ClientChannelHelperParent is
+    // destroyed. Otherwise, we may incorrectly "forget" a future ClientSource
+    // which will actually be created.
+    SetFutureSourceInfo(Nothing());
+  }
 
   void CreateClient(nsILoadInfo* aLoadInfo, nsIPrincipal* aPrincipal) override {
     CreateClientForPrincipal(aLoadInfo, aPrincipal, mEventTarget);
   }
 
+  void SetFutureSourceInfo(Maybe<ClientInfo>&& aClientInfo) {
+    if (mRecentFutureSourceInfo) {
+      // No-op if the corresponding ClientSource has alrady been created, but
+      // it's not known if that's the case here.
+      ClientManager::ForgetFutureSource(*mRecentFutureSourceInfo);
+    }
+
+    if (aClientInfo) {
+      Unused << NS_WARN_IF(!ClientManager::ExpectFutureSource(*aClientInfo));
+    }
+
+    mRecentFutureSourceInfo = std::move(aClientInfo);
+  }
+
+  // Keep track of the most recent ClientInfo created which isn't backed by a
+  // ClientSource, which is used to notify ClientManagerService that the
+  // ClientSource won't ever actually be constructed.
+  Maybe<ClientInfo> mRecentFutureSourceInfo;
+
  public:
-  static void CreateClientForPrincipal(nsILoadInfo* aLoadInfo,
-                                       nsIPrincipal* aPrincipal,
-                                       nsISerialEventTarget* aEventTarget) {
+  void CreateClientForPrincipal(nsILoadInfo* aLoadInfo,
+                                nsIPrincipal* aPrincipal,
+                                nsISerialEventTarget* aEventTarget) override {
     // If we're managing redirects in the parent, then we don't want
     // to create a new ClientSource (since those need to live with
     // the global), so just allocate a new ClientInfo/id and we can
     // create a ClientSource when the final channel propagates back
     // to the child.
     Maybe<ClientInfo> reservedInfo =
         ClientManager::CreateInfo(ClientType::Window, aPrincipal);
     if (reservedInfo) {
       aLoadInfo->SetReservedClientInfo(*reservedInfo);
+      SetFutureSourceInfo(std::move(reservedInfo));
     }
   }
   ClientChannelHelperParent(nsIInterfaceRequestor* aOuter,
                             nsISerialEventTarget* aEventTarget)
       : ClientChannelHelper(aOuter, nullptr) {}
 };
 
 class ClientChannelHelperChild final : public ClientChannelHelper {
@@ -289,22 +315,22 @@ nsresult AddClientChannelHelperInternal(
       reservedClientInfo.reset();
     }
   }
 
   nsCOMPtr<nsIInterfaceRequestor> outerCallbacks;
   rv = aChannel->GetNotificationCallbacks(getter_AddRefs(outerCallbacks));
   NS_ENSURE_SUCCESS(rv, rv);
 
+  RefPtr<ClientChannelHelper> helper = new T(outerCallbacks, aEventTarget);
+
   if (initialClientInfo.isNothing() && reservedClientInfo.isNothing()) {
-    T::CreateClientForPrincipal(loadInfo, channelPrincipal, aEventTarget);
+    helper->CreateClientForPrincipal(loadInfo, channelPrincipal, aEventTarget);
   }
 
-  RefPtr<ClientChannelHelper> helper = new T(outerCallbacks, aEventTarget);
-
   // Only set the callbacks helper if we are able to reserve the client
   // successfully.
   rv = aChannel->SetNotificationCallbacks(helper);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (initialClientInfo.isSome()) {
     loadInfo->SetInitialClientInfo(initialClientInfo.ref());
   }