Bug 1423412 P2 Copy the service worker controller across redirects by default and clear it explicitly for non-subresource redirects. r=baku
authorBen Kelly <ben@wanderview.com>
Tue, 05 Dec 2017 20:45:23 -0500
changeset 395175 09942818c67041082051e9bd25aee3f7899546ba
parent 395174 699fd0f2b04666c7b043f0350cf56f0bc07189ac
child 395176 9a5aac891c5f144811b17168353e55641a31e10c
push id98024
push userbkelly@mozilla.com
push dateWed, 06 Dec 2017 01:45:29 +0000
treeherdermozilla-inbound@7750a46652fa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1423412
milestone59.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 1423412 P2 Copy the service worker controller across redirects by default and clear it explicitly for non-subresource redirects. r=baku
dom/clients/manager/ClientChannelHelper.cpp
netwerk/base/LoadInfo.cpp
netwerk/base/nsILoadInfo.idl
--- a/dom/clients/manager/ClientChannelHelper.cpp
+++ b/dom/clients/manager/ClientChannelHelper.cpp
@@ -96,26 +96,16 @@ class ClientChannelHelper final : public
         if (reservedClientInfo.isSome()) {
           newLoadInfo->SetReservedClientInfo(reservedClientInfo.ref());
         }
 
         if (initialClientInfo.isSome()) {
           newLoadInfo->SetInitialClientInfo(initialClientInfo.ref());
         }
       }
-
-      // Make sure we keep the service worker controller on same-origin
-      // internal redirects.
-      if (oldLoadInfo != newLoadInfo &&
-          aFlags & nsIChannelEventSink::REDIRECT_INTERNAL) {
-        const Maybe<ServiceWorkerDescriptor>& controller = oldLoadInfo->GetController();
-        if (controller.isSome()) {
-          newLoadInfo->SetController(controller.ref());
-        }
-      }
     }
 
     // If it's a cross-origin redirect then we discard the old reserved client
     // and create a new one.
     else {
       // If CheckSameOrigin() worked, then the security manager must exist.
       nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
       MOZ_DIAGNOSTIC_ASSERT(ssm);
@@ -129,16 +119,30 @@ class ClientChannelHelper final : public
       // same-origin security policy.
       reservedClient.reset();
       reservedClient = ClientManager::CreateSource(ClientType::Window,
                                                    mEventTarget, principal);
 
       newLoadInfo->GiveReservedClientSource(Move(reservedClient));
     }
 
+    // Normally we keep the controller across channel redirects, but we must
+    // clear it when a non-subresource load redirects.  Only do this for real
+    // redirects, however.
+    //
+    // There is an open spec question about what to do in this case for
+    // worker script redirects.  For now we clear the controller as that
+    // seems most sane. See:
+    //
+    //  https://github.com/w3c/ServiceWorker/issues/1239
+    //
+    if (!(aFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) {
+      newLoadInfo->ClearController();
+    }
+
     nsCOMPtr<nsIChannelEventSink> outerSink = do_GetInterface(mOuter);
     if (outerSink) {
       return outerSink->AsyncOnChannelRedirect(aOldChannel, aNewChannel, aFlags,
                                                aCallback);
     }
 
     aCallback->OnRedirectVerifyCallback(NS_OK);
     return NS_OK;
--- a/netwerk/base/LoadInfo.cpp
+++ b/netwerk/base/LoadInfo.cpp
@@ -315,17 +315,17 @@ LoadInfo::LoadInfo(const LoadInfo& rhs)
   , mTriggeringPrincipal(rhs.mTriggeringPrincipal)
   , mPrincipalToInherit(rhs.mPrincipalToInherit)
   , mSandboxedLoadingPrincipal(rhs.mSandboxedLoadingPrincipal)
   , mResultPrincipalURI(rhs.mResultPrincipalURI)
   , mClientInfo(rhs.mClientInfo)
   // mReservedClientSource must be handled specially during redirect
   // mReservedClientInfo must be handled specially during redirect
   // mInitialClientInfo must be handled specially during redirect
-  // mController must be handled specially during redirect
+  , mController(rhs.mController)
   , mLoadingContext(rhs.mLoadingContext)
   , mContextForTopLevelLoad(rhs.mContextForTopLevelLoad)
   , mSecurityFlags(rhs.mSecurityFlags)
   , mInternalContentPolicyType(rhs.mInternalContentPolicyType)
   , mTainting(rhs.mTainting)
   , mUpgradeInsecureRequests(rhs.mUpgradeInsecureRequests)
   , mVerifySignedContent(rhs.mVerifySignedContent)
   , mEnforceSRI(rhs.mEnforceSRI)
@@ -1248,16 +1248,22 @@ LoadInfo::GetInitialClientInfo()
 }
 
 void
 LoadInfo::SetController(const ServiceWorkerDescriptor& aServiceWorker)
 {
   mController.emplace(aServiceWorker);
 }
 
+void
+LoadInfo::ClearController()
+{
+  mController.reset();
+}
+
 const Maybe<ServiceWorkerDescriptor>&
 LoadInfo::GetController()
 {
   return mController;
 }
 
 } // namespace net
 } // namespace mozilla
--- a/netwerk/base/nsILoadInfo.idl
+++ b/netwerk/base/nsILoadInfo.idl
@@ -936,14 +936,22 @@ interface nsILoadInfo : nsISupports
    * the first service worker interception occurs.  For subresource requests
    * it may be set by the source client if its already controlled by a
    * service worker.
    */
   [noscript, nostdcall, notxpcom]
   void SetController(in const_ServiceWorkerDescriptorRef aServiceWorker);
 
   /**
+   * Clear the service worker controller for this channel.  This should only
+   * be used for window navigation redirects.  By default we want to keep
+   * the controller in all other cases.
+   */
+  [noscript, nostdcall, notxpcom]
+  void ClearController();
+
+  /**
    * Get the service worker controlling this network request, if one has
    * been set.
    */
   [noscript, nostdcall, notxpcom]
   const_MaybeServiceWorkerDescriptorRef GetController();
 };