Bug 1507479: Make WorkerPrivate::mIsSecureContext const r=perry,asuth
☠☠ backed out by 3269a8661fdb ☠ ☠
authorYaron Tausky <ytausky@mozilla.com>
Mon, 19 Nov 2018 15:52:40 +0000
changeset 503484 7efd8b5c5b934ba3897fb33ac19720c07f4d4057
parent 503483 d834c0fe16e536cf3cae25946056f3372285f127
child 503485 6a9f15fb809da3bed50ede05e663537c44075778
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersperry, asuth
bugs1507479
milestone65.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 1507479: Make WorkerPrivate::mIsSecureContext const r=perry,asuth This member was never changed after it was set, but it couldn't be const because WorkerLoadInfo used a custom StealFrom() function instead of an idiomatic move constructor and move assignment operator. This commit replaces StealFrom with its idiomatic counterparts, adds a function that determines a new worker's secure context status, and uses that function to initialize the now const mIsSecureContext. Making constant members const is part of a greater effort to ensure that data is either modified from a single thread only, or access to it is synchronized properly. Differential Revision: https://phabricator.services.mozilla.com/D12030
dom/workers/WorkerLoadInfo.cpp
dom/workers/WorkerLoadInfo.h
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
--- a/dom/workers/WorkerLoadInfo.cpp
+++ b/dom/workers/WorkerLoadInfo.cpp
@@ -79,92 +79,29 @@ SwapToISupportsArray(SmartPtr<T>& aSrc,
 
   nsISupports* rawSupports =
     static_cast<typename ISupportsBaseInfo<T>::ISupportsBase*>(raw);
   dest->swap(rawSupports);
 }
 
 } // anonymous
 
-WorkerLoadInfo::WorkerLoadInfo()
+WorkerLoadInfoData::WorkerLoadInfoData()
   : mLoadFlags(nsIRequest::LOAD_NORMAL)
   , mWindowID(UINT64_MAX)
   , mReferrerPolicy(net::RP_Unset)
   , mFromWindow(false)
   , mEvalAllowed(false)
   , mReportCSPViolations(false)
   , mXHRParamsAllowed(false)
   , mPrincipalIsSystem(false)
   , mStorageAllowed(false)
   , mFirstPartyStorageAccessGranted(false)
   , mServiceWorkersTestingInWindow(false)
-{
-  MOZ_COUNT_CTOR(WorkerLoadInfo);
-}
-
-WorkerLoadInfo::~WorkerLoadInfo()
-{
-  MOZ_COUNT_DTOR(WorkerLoadInfo);
-}
-
-void
-WorkerLoadInfo::StealFrom(WorkerLoadInfo& aOther)
-{
-  MOZ_ASSERT(!mBaseURI);
-  aOther.mBaseURI.swap(mBaseURI);
-
-  MOZ_ASSERT(!mResolvedScriptURI);
-  aOther.mResolvedScriptURI.swap(mResolvedScriptURI);
-
-  MOZ_ASSERT(!mPrincipal);
-  aOther.mPrincipal.swap(mPrincipal);
-
-  // mLoadingPrincipal can be null if this is a ServiceWorker.
-  aOther.mLoadingPrincipal.swap(mLoadingPrincipal);
-
-  MOZ_ASSERT(!mScriptContext);
-  aOther.mScriptContext.swap(mScriptContext);
-
-  MOZ_ASSERT(!mWindow);
-  aOther.mWindow.swap(mWindow);
-
-  MOZ_ASSERT(!mCSP);
-  aOther.mCSP.swap(mCSP);
-
-  MOZ_ASSERT(!mChannel);
-  aOther.mChannel.swap(mChannel);
-
-  MOZ_ASSERT(!mLoadGroup);
-  aOther.mLoadGroup.swap(mLoadGroup);
-
-  MOZ_ASSERT(!mInterfaceRequestor);
-  aOther.mInterfaceRequestor.swap(mInterfaceRequestor);
-
-  MOZ_ASSERT(!mPrincipalInfo);
-  mPrincipalInfo = aOther.mPrincipalInfo.forget();
-
-  mDomain = aOther.mDomain;
-  mOrigin = aOther.mOrigin;
-  mServiceWorkerCacheName = aOther.mServiceWorkerCacheName;
-  mServiceWorkerDescriptor = aOther.mServiceWorkerDescriptor;
-  mServiceWorkerRegistrationDescriptor = aOther.mServiceWorkerRegistrationDescriptor;
-  mLoadFlags = aOther.mLoadFlags;
-  mWindowID = aOther.mWindowID;
-  mReferrerPolicy = aOther.mReferrerPolicy;
-  mFromWindow = aOther.mFromWindow;
-  mEvalAllowed = aOther.mEvalAllowed;
-  mReportCSPViolations = aOther.mReportCSPViolations;
-  mXHRParamsAllowed = aOther.mXHRParamsAllowed;
-  mPrincipalIsSystem = aOther.mPrincipalIsSystem;
-  mStorageAllowed = aOther.mStorageAllowed;
-  mFirstPartyStorageAccessGranted = aOther.mFirstPartyStorageAccessGranted;
-  mServiceWorkersTestingInWindow = aOther.mServiceWorkersTestingInWindow;
-  mOriginAttributes = aOther.mOriginAttributes;
-  mParentController = aOther.mParentController;
-}
+{}
 
 nsresult
 WorkerLoadInfo::SetPrincipalOnMainThread(nsIPrincipal* aPrincipal,
                                          nsILoadGroup* aLoadGroup)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(NS_LoadGroupMatchesPrincipal(aLoadGroup, aPrincipal));
 
@@ -533,10 +470,26 @@ InterfaceRequestor::GetAnyLiveTabChild()
   return nullptr;
 }
 
 NS_IMPL_ADDREF(WorkerLoadInfo::InterfaceRequestor)
 NS_IMPL_RELEASE(WorkerLoadInfo::InterfaceRequestor)
 NS_IMPL_QUERY_INTERFACE(WorkerLoadInfo::InterfaceRequestor,
                         nsIInterfaceRequestor)
 
+WorkerLoadInfo::WorkerLoadInfo()
+{
+  MOZ_COUNT_CTOR(WorkerLoadInfo);
+}
+
+WorkerLoadInfo::WorkerLoadInfo(WorkerLoadInfo&& aOther)
+  : WorkerLoadInfoData(std::move(aOther))
+{
+  MOZ_COUNT_CTOR(WorkerLoadInfo);
+}
+
+WorkerLoadInfo::~WorkerLoadInfo()
+{
+  MOZ_COUNT_DTOR(WorkerLoadInfo);
+}
+
 } // dom namespace
 } // mozilla namespace
--- a/dom/workers/WorkerLoadInfo.h
+++ b/dom/workers/WorkerLoadInfo.h
@@ -32,17 +32,17 @@ namespace mozilla {
 namespace ipc {
 class PrincipalInfo;
 } // namespace ipc
 
 namespace dom {
 
 class WorkerPrivate;
 
-struct WorkerLoadInfo
+struct WorkerLoadInfoData
 {
   // All of these should be released in WorkerPrivateParent::ForgetMainThreadObjects.
   nsCOMPtr<nsIURI> mBaseURI;
   nsCOMPtr<nsIURI> mResolvedScriptURI;
 
   // This is the principal of the global (parent worker or a window) loading
   // the worker. It can be null if we are executing a ServiceWorker, otherwise,
   // except for data: URL, it must subsumes the worker principal.
@@ -102,20 +102,31 @@ struct WorkerLoadInfo
   bool mReportCSPViolations;
   bool mXHRParamsAllowed;
   bool mPrincipalIsSystem;
   bool mStorageAllowed;
   bool mFirstPartyStorageAccessGranted;
   bool mServiceWorkersTestingInWindow;
   OriginAttributes mOriginAttributes;
 
+  WorkerLoadInfoData();
+  WorkerLoadInfoData(WorkerLoadInfoData&& aOther) = default;
+
+  WorkerLoadInfoData&
+  operator=(WorkerLoadInfoData&& aOther) = default;
+};
+
+struct WorkerLoadInfo : WorkerLoadInfoData
+{
   WorkerLoadInfo();
+  WorkerLoadInfo(WorkerLoadInfo&& aOther);
   ~WorkerLoadInfo();
 
-  void StealFrom(WorkerLoadInfo& aOther);
+  WorkerLoadInfo&
+  operator=(WorkerLoadInfo&& aOther) = default;
 
   nsresult
   SetPrincipalOnMainThread(nsIPrincipal* aPrincipal, nsILoadGroup* aLoadGroup);
 
   nsresult
   GetPrincipalAndLoadGroupFromChannel(nsIChannel* aChannel,
                                       nsIPrincipal** aPrincipalOut,
                                       nsILoadGroup** aLoadGroupOut);
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -2615,28 +2615,65 @@ WorkerPrivate::WorkerThreadAccessible::W
   , mFrozen(false)
   , mTimerRunning(false)
   , mRunningExpiredTimeouts(false)
   , mPeriodicGCTimerRunning(false)
   , mIdleGCTimerRunning(false)
   , mOnLine(aParent ? aParent->OnLine() : !NS_IsOffline())
 {}
 
+namespace {
+
+bool
+IsNewWorkerSecureContext(const WorkerPrivate* const aParent,
+                         const WorkerType aWorkerType,
+                         const WorkerLoadInfo& aLoadInfo)
+{
+  if (aParent) {
+    return aParent->IsSecureContext();
+  }
+
+  // Our secure context state depends on the kind of worker we have.
+
+  if (aLoadInfo.mPrincipalIsSystem) {
+    return true;
+  }
+
+  if (aWorkerType == WorkerTypeService) {
+    return true;
+  }
+
+  if (aLoadInfo.mWindow) {
+    // Shared and dedicated workers both inherit the loading window's secure
+    // context state.  Shared workers then prevent windows with a different
+    // secure context state from attaching to them.
+    return aLoadInfo.mWindow->IsSecureContext();
+  }
+
+  MOZ_ASSERT_UNREACHABLE("non-chrome worker that is not a service worker "
+                         "that has no parent and no associated window");
+
+  return false;
+}
+
+}
+
 WorkerPrivate::WorkerPrivate(WorkerPrivate* aParent,
                              const nsAString& aScriptURL,
                              bool aIsChromeWorker, WorkerType aWorkerType,
                              const nsAString& aWorkerName,
                              const nsACString& aServiceWorkerScope,
                              WorkerLoadInfo& aLoadInfo)
   : mMutex("WorkerPrivate Mutex")
   , mCondVar(mMutex, "WorkerPrivate CondVar")
   , mParent(aParent)
   , mScriptURL(aScriptURL)
   , mWorkerName(aWorkerName)
   , mWorkerType(aWorkerType)
+  , mLoadInfo(std::move(aLoadInfo))
   , mDebugger(nullptr)
   , mJSContext(nullptr)
   , mPRThread(nullptr)
   , mWorkerControlEventTarget(new WorkerEventTarget(this,
                                                     WorkerEventTarget::Behavior::ControlOnly))
   , mWorkerHybridEventTarget(new WorkerEventTarget(this,
                                                    WorkerEventTarget::Behavior::Hybrid))
   , mParentStatus(Pending)
@@ -2649,61 +2686,44 @@ WorkerPrivate::WorkerPrivate(WorkerPriva
   , mParentWindowPausedDepth(0)
   , mPendingEventQueueClearing(false)
   , mCancelAllPendingRunnables(false)
   , mWorkerScriptExecutedSuccessfully(false)
   , mFetchHandlerWasAdded(false)
   , mMainThreadObjectsForgotten(false)
   , mIsChromeWorker(aIsChromeWorker)
   , mParentFrozen(false)
-  , mIsSecureContext(false)
+  , mIsSecureContext(IsNewWorkerSecureContext(mParent, mWorkerType, mLoadInfo))
   , mDebuggerRegistered(false)
   , mIsInAutomation(false)
   , mPerformanceCounter(nullptr)
 {
   MOZ_ASSERT_IF(!IsDedicatedWorker(), NS_IsMainThread());
-  mLoadInfo.StealFrom(aLoadInfo);
 
   if (aParent) {
     aParent->AssertIsOnWorkerThread();
 
     // Note that this copies our parent's secure context state into mJSSettings.
     aParent->CopyJSSettings(mJSSettings);
 
-    // And manually set our mIsSecureContext, though it's not really relevant to
-    // dedicated workers...
-    mIsSecureContext = aParent->IsSecureContext();
     MOZ_ASSERT_IF(mIsChromeWorker, mIsSecureContext);
 
     mIsInAutomation = aParent->IsInAutomation();
 
     MOZ_ASSERT(IsDedicatedWorker());
 
     if (aParent->mParentFrozen) {
       Freeze(nullptr);
     }
   }
   else {
     AssertIsOnMainThread();
 
     RuntimeService::GetDefaultJSSettings(mJSSettings);
 
-    // Our secure context state depends on the kind of worker we have.
-    if (UsesSystemPrincipal() || IsServiceWorker()) {
-      mIsSecureContext = true;
-    } else if (mLoadInfo.mWindow) {
-      // Shared and dedicated workers both inherit the loading window's secure
-      // context state.  Shared workers then prevent windows with a different
-      // secure context state from attaching to them.
-      mIsSecureContext = mLoadInfo.mWindow->IsSecureContext();
-    } else {
-      MOZ_ASSERT_UNREACHABLE("non-chrome worker that is not a service worker "
-                             "that has no parent and no associated window");
-    }
-
     if (mIsSecureContext) {
       mJSSettings.chrome.realmOptions
                  .creationOptions().setSecureContext(true);
       mJSSettings.chrome.realmOptions
                  .creationOptions().setClampAndJitterTime(false);
       mJSSettings.content.realmOptions
                  .creationOptions().setSecureContext(true);
       mJSSettings.content.realmOptions
@@ -3128,17 +3148,17 @@ WorkerPrivate::GetLoadInfo(JSContext* aC
 
     rv = loadInfo.SetPrincipalFromChannel(loadInfo.mChannel);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   MOZ_DIAGNOSTIC_ASSERT(loadInfo.mLoadingPrincipal);
   MOZ_DIAGNOSTIC_ASSERT(loadInfo.PrincipalIsValid());
 
-  aLoadInfo->StealFrom(loadInfo);
+  *aLoadInfo = std::move(loadInfo);
   return NS_OK;
 }
 
 // static
 void
 WorkerPrivate::OverrideLoadInfoLoadGroup(WorkerLoadInfo& aLoadInfo,
                                          nsIPrincipal* aPrincipal)
 {
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -1472,24 +1472,22 @@ private:
   bool mCancelAllPendingRunnables;
   bool mWorkerScriptExecutedSuccessfully;
   bool mFetchHandlerWasAdded;
   bool mMainThreadObjectsForgotten;
   bool mIsChromeWorker;
   bool mParentFrozen;
 
   // mIsSecureContext is set once in our constructor; after that it can be read
-  // from various threads.  We could make this const if we were OK with setting
-  // it in the initializer list via calling some function that takes all sorts
-  // of state (loadinfo, worker type, parent).
+  // from various threads.
   //
   // It's a bit unfortunate that we have to have an out-of-band boolean for
   // this, but we need access to this state from the parent thread, and we can't
   // use our global object's secure state there.
-  bool mIsSecureContext;
+  const bool mIsSecureContext;
 
   bool mDebuggerRegistered;
 
   // mIsInAutomation is true when we're running in test automation.
   // We expose some extra testing functions in that case.
   bool mIsInAutomation;
 
   // This pointer will be null if dom.performance.enable_scheduler_timing is