Bug 1532661 - Part 5: Make the BrowsingContext opener edge a weak reference, r=farre
authorNika Layzell <nika@thelayzells.com>
Thu, 14 Mar 2019 18:51:11 +0000
changeset 524934 856a3d16a4ca856711ad9e6368cccd5aa1e22118
parent 524933 9ce1a2365afff092f9107a5f1ae1eca6a34dfce3
child 524935 97c2ee22169c641e88278fd0801abfc18db6c43f
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfarre
bugs1532661
milestone67.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 1532661 - Part 5: Make the BrowsingContext opener edge a weak reference, r=farre Depends on D22193 Differential Revision: https://phabricator.services.mozilla.com/D23047
docshell/base/BrowsingContext.cpp
docshell/base/BrowsingContext.h
docshell/base/BrowsingContextFieldList.h
dom/base/nsGlobalWindowOuter.cpp
dom/ipc/ContentChild.cpp
dom/ipc/ContentParent.cpp
--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -117,17 +117,17 @@ already_AddRefed<BrowsingContext> Browsi
                                            /* aProcessId */ 0, aType);
   } else {
     context = new BrowsingContext(aParent, group, id, aType);
   }
 
   // The name and opener fields need to be explicitly initialized. Don't bother
   // using transactions to set them, as we haven't been attached yet.
   context->mName = aName;
-  context->mOpener = aOpener;
+  context->mOpenerId = aOpener ? aOpener->Id() : 0;
 
   if (aParent) {
     context->mCrossOriginPolicy = aParent->mCrossOriginPolicy;
   } else if (aOpener) {
     context->mCrossOriginPolicy = aOpener->mCrossOriginPolicy;
   } else {
     context->mCrossOriginPolicy = nsILoadInfo::CROSS_ORIGIN_POLICY_NULL;
   }
@@ -160,20 +160,18 @@ already_AddRefed<BrowsingContext> Browsi
     context = new CanonicalBrowsingContext(parent, aGroup, aInit.mId, originId,
                                            Type::Content);
   } else {
     context = new BrowsingContext(parent, aGroup, aInit.mId, Type::Content);
   }
 
   Register(context);
 
-  // Initialize all non-opener fields. We skip the opener here, as when doing
-  // the initial sync of the fields we may not have our opener in this process
-  // yet at this point.
-#define MOZ_BC_FIELD_SKIP_OPENER
+  // Initialize all of our fields from IPC. We don't have to worry about
+  // mOpenerId, as we won't try to dereference it immediately.
 #define MOZ_BC_FIELD(name, ...) context->m##name = aInit.m##name;
 #include "mozilla/dom/BrowsingContextFieldList.h"
 
   // Caller handles attaching us to the tree.
 
   return context.forget();
 }
 
@@ -490,25 +488,25 @@ void BrowsingContext::NotifyResetUserGes
 bool BrowsingContext::GetUserGestureActivation() {
   RefPtr<BrowsingContext> topLevelBC = TopLevelBrowsingContext();
   return topLevelBC->GetIsActivatedByUserGesture();
 }
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(BrowsingContext)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(BrowsingContext)
-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocShell, mChildren, mParent, mOpener, mGroup)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocShell, mChildren, mParent, mGroup)
   if (XRE_IsParentProcess()) {
     CanonicalBrowsingContext::Cast(tmp)->Unlink();
   }
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(BrowsingContext)
-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocShell, mChildren, mParent, mOpener, mGroup)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocShell, mChildren, mParent, mGroup)
   if (XRE_IsParentProcess()) {
     CanonicalBrowsingContext::Cast(tmp)->Traverse(cb);
   }
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(BrowsingContext)
 
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(BrowsingContext, AddRef)
@@ -584,17 +582,17 @@ Nullable<WindowProxyHolder> BrowsingCont
   // We never return null or throw an error, but the implementation in
   // nsGlobalWindow does and we need to use the same signature.
   return WindowProxyHolder(TopLevelBrowsingContext());
 }
 
 void BrowsingContext::GetOpener(JSContext* aCx,
                                 JS::MutableHandle<JS::Value> aOpener,
                                 ErrorResult& aError) const {
-  BrowsingContext* opener = GetOpener();
+  RefPtr<BrowsingContext> opener = GetOpener();
   if (!opener) {
     aOpener.setNull();
     return;
   }
 
   if (!ToJSValue(aCx, WindowProxyHolder(opener), aOpener)) {
     aError.NoteJSContextException(aCx);
   }
@@ -804,40 +802,34 @@ bool IPDLParamTraits<dom::BrowsingContex
 
   return true;
 }
 
 void IPDLParamTraits<dom::BrowsingContext::IPCInitializer>::Write(
     IPC::Message* aMessage, IProtocol* aActor,
     const dom::BrowsingContext::IPCInitializer& aInit) {
   // Write actor ID parameters.
-  // NOTE: mOpenerId is synced separately due to ordering issues.
   WriteIPDLParam(aMessage, aActor, aInit.mId);
   WriteIPDLParam(aMessage, aActor, aInit.mParentId);
-  WriteIPDLParam(aMessage, aActor, aInit.mOpenerId);
 
   // Write other synchronized fields.
-#define MOZ_BC_FIELD_SKIP_OPENER
 #define MOZ_BC_FIELD(name, ...) WriteIPDLParam(aMessage, aActor, aInit.m##name);
 #include "mozilla/dom/BrowsingContextFieldList.h"
 }
 
 bool IPDLParamTraits<dom::BrowsingContext::IPCInitializer>::Read(
     const IPC::Message* aMessage, PickleIterator* aIterator, IProtocol* aActor,
     dom::BrowsingContext::IPCInitializer* aInit) {
   // Read actor ID parameters.
-  // NOTE: mOpenerId is synced separately due to ordering issues.
   if (!ReadIPDLParam(aMessage, aIterator, aActor, &aInit->mId) ||
-      !ReadIPDLParam(aMessage, aIterator, aActor, &aInit->mParentId) ||
-      !ReadIPDLParam(aMessage, aIterator, aActor, &aInit->mOpenerId)) {
+      !ReadIPDLParam(aMessage, aIterator, aActor, &aInit->mParentId)) {
     return false;
   }
 
   // Read other synchronized fields.
-#define MOZ_BC_FIELD_SKIP_OPENER
 #define MOZ_BC_FIELD(name, ...)                                       \
   if (!ReadIPDLParam(aMessage, aIterator, aActor, &aInit->m##name)) { \
     return false;                                                     \
   }
 #include "mozilla/dom/BrowsingContextFieldList.h"
 
   return true;
 }
--- a/docshell/base/BrowsingContext.h
+++ b/docshell/base/BrowsingContext.h
@@ -150,17 +150,22 @@ class BrowsingContext : public nsWrapper
   const nsString& Name() const { return mName; }
   void GetName(nsAString& aName) { aName = mName; }
   bool NameEquals(const nsAString& aName) { return mName.Equals(aName); }
 
   bool IsContent() const { return mType == Type::Content; }
 
   uint64_t Id() const { return mBrowsingContextId; }
 
-  BrowsingContext* GetParent() { return mParent; }
+  BrowsingContext* GetParent() const { return mParent; }
+
+  already_AddRefed<BrowsingContext> GetOpener() const { return Get(mOpenerId); }
+  void SetOpener(BrowsingContext* aOpener) {
+    SetOpenerId(aOpener ? aOpener->Id() : 0);
+  }
 
   void GetChildren(nsTArray<RefPtr<BrowsingContext>>& aChildren);
 
   BrowsingContextGroup* Group() { return mGroup; }
 
   // Using the rules for choosing a browsing context we try to find
   // the browsing context with the given name in the set of
   // transitively reachable browsing contexts. Performs access control
@@ -287,53 +292,41 @@ class BrowsingContext : public nsWrapper
    */
   struct IPCInitializer {
     uint64_t mId;
 
     // IDs are used for Parent and Opener to allow for this object to be
     // deserialized before other BrowsingContext in the BrowsingContextGroup
     // have been initialized.
     uint64_t mParentId;
-    uint64_t mOpenerId;
     already_AddRefed<BrowsingContext> GetParent();
     already_AddRefed<BrowsingContext> GetOpener();
 
     // Include each field, skipping mOpener, as we want to handle it
     // separately.
-#define MOZ_BC_FIELD_SKIP_OPENER
 #define MOZ_BC_FIELD(name, type) type m##name;
 #include "mozilla/dom/BrowsingContextFieldList.h"
   };
 
   // Create an IPCInitializer object for this BrowsingContext.
   IPCInitializer GetIPCInitializer() {
     IPCInitializer init;
     init.mId = Id();
     init.mParentId = mParent ? mParent->Id() : 0;
-    init.mOpenerId = mOpener ? mOpener->Id() : 0;
 
-#define MOZ_BC_FIELD_SKIP_OPENER
 #define MOZ_BC_FIELD(name, type) init.m##name = m##name;
 #include "mozilla/dom/BrowsingContextFieldList.h"
     return init;
   }
 
-  // Create a BrowsingContext object from over IPC. This method does not
-  // initialize the Opener field, which will need to be set after using the
-  // InitFromIPC method.
+  // Create a BrowsingContext object from over IPC.
   static already_AddRefed<BrowsingContext> CreateFromIPC(
       IPCInitializer&& aInitializer, BrowsingContextGroup* aGroup,
       ContentParent* aOriginProcess);
 
-  // Initialize the Opener property which was not set during CreateFromIPC.
-  void InitFromIPC(uint64_t aOpenerId) {
-    mOpener = BrowsingContext::Get(aOpenerId);
-    MOZ_RELEASE_ASSERT(mOpener || aOpenerId == 0);
-  }
-
  protected:
   virtual ~BrowsingContext();
   BrowsingContext(BrowsingContext* aParent, BrowsingContextGroup* aGroup,
                   uint64_t aBrowsingContextId, Type aType);
 
  private:
   // Find the special browsing context if aName is '_self', '_parent',
   // '_top', but not '_blank'. The latter is handled in FindWithName
@@ -393,20 +386,20 @@ class BrowsingContext : public nsWrapper
     friend class RemoteLocationProxy;
     BrowsingContext* GetBrowsingContext() {
       return reinterpret_cast<BrowsingContext*>(
           uintptr_t(this) - offsetof(BrowsingContext, mLocation));
     }
   };
 
   // Ensure that opener is in the same BrowsingContextGroup.
-  void WillSetOpener(const RefPtr<BrowsingContext>& aValue,
-                     ContentParent* aSource) {
-    if (aValue) {
-      MOZ_RELEASE_ASSERT(aValue->Group() == Group());
+  void WillSetOpener(const uint64_t& aValue, ContentParent* aSource) {
+    if (aValue != 0) {
+      RefPtr<BrowsingContext> opener = Get(aValue);
+      MOZ_RELEASE_ASSERT(opener && opener->Group() == Group());
     }
   }
 
   // Type of BrowsingContent
   const Type mType;
 
   // Unique id identifying BrowsingContext
   const uint64_t mBrowsingContextId;
--- a/docshell/base/BrowsingContextFieldList.h
+++ b/docshell/base/BrowsingContextFieldList.h
@@ -3,21 +3,18 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 MOZ_BC_FIELD(Name, nsString)
 MOZ_BC_FIELD(Closed, bool)
 MOZ_BC_FIELD(CrossOriginPolicy, nsILoadInfo::CrossOriginPolicy)
 
-// The current opener for this BrowsingContext. This field can be skipped by
-// defining `MOZ_BC_FIELD_SKIP_OPENER`, which is used when serializing
-// synchronized field state when initializing browsing contexts.
-#ifndef MOZ_BC_FIELD_SKIP_OPENER
-MOZ_BC_FIELD(Opener, RefPtr<BrowsingContext>)
-#endif
+// The current opener for this BrowsingContext. This is a weak reference, and
+// stored as the opener ID.
+MOZ_BC_FIELD(OpenerId, uint64_t)
 
 // Toplevel browsing contexts only. This field controls whether the browsing
 // context is currently considered to be activated by a gesture.
 MOZ_BC_FIELD(IsActivatedByUserGesture, bool)
 
 #undef MOZ_BC_FIELD
 #undef MOZ_BC_FIELD_SKIP_OPENER
--- a/dom/base/nsGlobalWindowOuter.cpp
+++ b/dom/base/nsGlobalWindowOuter.cpp
@@ -2477,20 +2477,21 @@ void nsGlobalWindowOuter::DetachFromDocS
   MaybeForgiveSpamCount();
   CleanUp();
 }
 
 void nsGlobalWindowOuter::SetOpenerWindow(nsPIDOMWindowOuter* aOpener,
                                           bool aOriginalOpener) {
   nsWeakPtr opener = do_GetWeakReference(aOpener);
   if (opener == mOpener) {
-    MOZ_DIAGNOSTIC_ASSERT(
-        !aOpener || !aOpener->GetDocShell() ||
-        (GetBrowsingContext() &&
-         GetBrowsingContext()->GetOpener() == aOpener->GetBrowsingContext()));
+    MOZ_DIAGNOSTIC_ASSERT(!aOpener || !aOpener->GetDocShell() ||
+                          (GetBrowsingContext() &&
+                           aOpener->GetBrowsingContext() &&
+                           aOpener->GetBrowsingContext()->Id() ==
+                               GetBrowsingContext()->GetOpenerId()));
     return;
   }
 
   NS_ASSERTION(!aOriginalOpener || !mSetOpenerWindowCalled,
                "aOriginalOpener is true, but not first call to "
                "SetOpenerWindow!");
   NS_ASSERTION(aOpener || !aOriginalOpener,
                "Shouldn't set mHadOriginalOpener if aOpener is null");
@@ -2503,17 +2504,18 @@ void nsGlobalWindowOuter::SetOpenerWindo
         !aOriginalOpener || !aOpener ||
         // TODO(farre): Allowing to set a closed or closing window as
         // opener is not ideal, since it won't have a docshell and
         // therefore no browsing context. This means that we're
         // effectively setting the browsing context opener to null and
         // the window opener to a closed window. This needs to be
         // cleaned up, see Bug 1511353.
         nsGlobalWindowOuter::Cast(aOpener)->IsClosedOrClosing() ||
-        aOpener->GetBrowsingContext() == GetBrowsingContext()->GetOpener());
+        aOpener->GetBrowsingContext()->Id() ==
+            GetBrowsingContext()->GetOpenerId());
     // TODO(farre): Here we really wish to only consider the case
     // where 'aOriginalOpener'. See bug 1509016.
     GetBrowsingContext()->SetOpener(aOpener ? aOpener->GetBrowsingContext()
                                             : nullptr);
   }
 
   // Check that the js visible opener matches! We currently don't depend on this
   // being true outside of nightly, so we disable the assertion in optimized
--- a/dom/ipc/ContentChild.cpp
+++ b/dom/ipc/ContentChild.cpp
@@ -3693,17 +3693,16 @@ mozilla::ipc::IPCResult ContentChild::Re
   RefPtr<BrowsingContext> child = BrowsingContext::Get(aInit.mId);
   MOZ_RELEASE_ASSERT(!child || child->IsCached());
 
   if (!child) {
     // Determine the BrowsingContextGroup from our parent or opener fields.
     RefPtr<BrowsingContextGroup> group =
         BrowsingContextGroup::Select(aInit.mParentId, aInit.mOpenerId);
     child = BrowsingContext::CreateFromIPC(std::move(aInit), group, nullptr);
-    child->InitFromIPC(aInit.mOpenerId);
   }
 
   child->Attach(/* aFromIPC */ true);
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult ContentChild::RecvDetachBrowsingContext(
@@ -3718,48 +3717,31 @@ mozilla::ipc::IPCResult ContentChild::Re
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult ContentChild::RecvRegisterBrowsingContextGroup(
     nsTArray<BrowsingContext::IPCInitializer>&& aInits) {
   RefPtr<BrowsingContextGroup> group = new BrowsingContextGroup();
 
-  // Hold a grip on each created BrowsingContext to ensure they don't die too
-  // early.
-  AutoTArray<RefPtr<BrowsingContext>, 8> grip;
-  grip.SetCapacity(aInits.Length());
-
   // Each of the initializers in aInits is sorted in pre-order, so our parent
   // should always be available before the element itself.
   for (auto& init : aInits) {
 #ifdef DEBUG
     RefPtr<BrowsingContext> existing = BrowsingContext::Get(init.mId);
     MOZ_ASSERT(!existing, "BrowsingContext must not exist yet!");
 
     RefPtr<BrowsingContext> parent = init.GetParent();
     MOZ_ASSERT_IF(parent, parent->Group() == group);
 #endif
 
-    // Create a BrowsingContext and add it to our grip list.
-    auto* ctxt = grip.AppendElement();
-    *ctxt = BrowsingContext::CreateFromIPC(std::move(init), group, nullptr);
+    RefPtr<BrowsingContext> ctxt = BrowsingContext::CreateFromIPC(std::move(init), group, nullptr);
 
     // FIXME: We should deal with cached & detached contexts as well.
-    (*ctxt)->Attach(/* aFromIPC */ true);
-  }
-
-  // Perform a second pass to initialize the `opener` fields of each
-  // BrowsingContext.
-  // Our `mId` and `mOpenerId` fields won't have been moved by the above loop,
-  // so they're OK to access here.
-  MOZ_ASSERT(grip.Length() == aInits.Length());
-  for (uint32_t i = 0; i < grip.Length(); ++i) {
-    MOZ_ASSERT(grip[i]->Id() == aInits[i].mId);
-    grip[i]->InitFromIPC(aInits[i].mOpenerId);
+    ctxt->Attach(/* aFromIPC */ true);
   }
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult ContentChild::RecvWindowClose(BrowsingContext* aContext,
                                                       bool aTrustedCaller) {
   if (!aContext) {
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -5735,17 +5735,16 @@ mozilla::ipc::IPCResult ContentParent::R
              aInit.mId, aInit.mParentId));
     return IPC_OK();
   }
 
   if (!child) {
     RefPtr<BrowsingContextGroup> group =
       BrowsingContextGroup::Select(aInit.mParentId, aInit.mOpenerId);
     child = BrowsingContext::CreateFromIPC(std::move(aInit), group, this);
-    child->InitFromIPC(aInit.mOpenerId);
   }
 
   child->Attach(/* aFromIPC */ true);
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult ContentParent::RecvDetachBrowsingContext(