From 3e3456e73eea8a6cdc2bb3b37d7cadf988a16244 Mon Sep 17 00:00:00 2001 From: Eden Chuang Date: Mon, 24 May 2021 13:55:07 +0000 Subject: [PATCH] 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 | 48 ++++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/dom/clients/manager/ClientChannelHelper.cpp b/dom/clients/manager/ClientChannelHelper.cpp index ec23981380bb..12a067306566 100644 --- a/dom/clients/manager/ClientChannelHelper.cpp +++ b/dom/clients/manager/ClientChannelHelper.cpp @@ -163,9 +163,9 @@ class ClientChannelHelper : public nsIInterfaceRequestor, 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. @@ -181,16 +181,41 @@ 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&& 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 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 @@ -200,6 +225,7 @@ class ClientChannelHelperParent final : public ClientChannelHelper { ClientManager::CreateInfo(ClientType::Window, aPrincipal); if (reservedInfo) { aLoadInfo->SetReservedClientInfo(*reservedInfo); + SetFutureSourceInfo(std::move(reservedInfo)); } } ClientChannelHelperParent(nsIInterfaceRequestor* aOuter, @@ -296,12 +322,12 @@ nsresult AddClientChannelHelperInternal(nsIChannel* aChannel, rv = aChannel->GetNotificationCallbacks(getter_AddRefs(outerCallbacks)); NS_ENSURE_SUCCESS(rv, rv); - if (initialClientInfo.isNothing() && reservedClientInfo.isNothing()) { - T::CreateClientForPrincipal(loadInfo, channelPrincipal, aEventTarget); - } - RefPtr helper = new T(outerCallbacks, aEventTarget); + if (initialClientInfo.isNothing() && reservedClientInfo.isNothing()) { + helper->CreateClientForPrincipal(loadInfo, channelPrincipal, aEventTarget); + } + // Only set the callbacks helper if we are able to reserve the client // successfully. rv = aChannel->SetNotificationCallbacks(helper);