1480113 - Create BrowsingContextChrome, and move mProcessId to it

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1480113-Split-BrowsingContext-to-Content-and-Chr.patch

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8998887 [details] [diff] [review]
0001-Bug-1480113-Split-BrowsingContext-to-Content-and-Chr.patch

Review of attachment 8998887 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/BrowsingContext.cpp
@@ +18,5 @@
>  #include "nsContentUtils.h"
>  #include "nsThreadUtils.h"
>  
> +namespace {
> +class ChromeBrowsingContext final : public mozilla::dom::BrowsingContext

Why put this within the .cpp file? I'd think we would want to make the type distinction public (so that to get the mProcessId you'd have to call a method to switch to a ChromeBrowsingContext)

@@ +47,5 @@
> +  // Indicates which process owns the docshell.
> +  uint64_t mProcessId;
> +};
> +
> +class ContentBrowsingContext final : public mozilla::dom::BrowsingContext

I don't think there will be any state in content which we won't also want in chrome, so I think we can just use BrowsingContext in content.

Comment 3

[Andreas Farre [:farre]]

Attachment: Bug 1480113 - Split parent process specific parts out of BrowsingContext

Create ChromeBrowsingContext and move parent process specific parts
from BrowsingContext there. After that make sure that all
BrowsingContexts created in the parent process is actually
ChromeBrowsingContexts and all BrowsingContexts in the child processes
are BrowsingContexts.

Comment 4

[Andreas Farre [:farre]]

Comment on attachment 8998887 [details] [diff] [review]
0001-Bug-1480113-Split-BrowsingContext-to-Content-and-Chr.patch

>From 30f7c58cace0933656e1cb567c6460282ce74fc3 Mon Sep 17 00:00:00 2001
>From: Andreas Farre <[e-mail]>
>Date: Wed, 8 Aug 2018 09:56:28 +0200
>Subject: [PATCH] Bug 1480113 - Split BrowsingContext to Content and Chrome
> versions. r=nika
>
>Add two versions of BrowsingContext::Create for creating browsing
>contexts in parent and child process that both are exposed as
>BrowsingContext but that can contain different kinds of data.
>
>diff --git a/docshell/base/BrowsingContext.cpp b/docshell/base/BrowsingContext.cpp
>index e0b493c6e9c1..2fc145272978 100644
>--- a/docshell/base/BrowsingContext.cpp
>+++ b/docshell/base/BrowsingContext.cpp
>@@ -13,16 +13,61 @@
> #include "mozilla/StaticPtr.h"
> 
> #include "nsDataHashtable.h"
> #include "nsRefPtrHashtable.h"
> #include "nsIDocShell.h"
> #include "nsContentUtils.h"
> #include "nsThreadUtils.h"
> 
>+namespace {
>+class ChromeBrowsingContext final : public mozilla::dom::BrowsingContext
>+{
>+public:
>+  ChromeBrowsingContext(uint64_t aBrowsingContextId,
>+                        const nsAString& aName,
>+                        uint64_t aProcessId)
>+    : BrowsingContext(aBrowsingContextId, aName)
>+    , mProcessId(aProcessId)
>+  {
>+  }
>+
>+  explicit ChromeBrowsingContext(nsIDocShell* aDocShell)
>+    : BrowsingContext(aDocShell)
>+    , mProcessId(0) // Ehh, is this correct?
>+  {
>+  }
>+
>+  virtual mozilla::Maybe<uint64_t> OwnerProcessId() const final
>+  {
>+    return mozilla::Some(mProcessId);
>+  }
>+
>+private:
>+  virtual ~ChromeBrowsingContext() {}
>+
>+  // Indicates which process owns the docshell.
>+  uint64_t mProcessId;
>+};
>+
>+class ContentBrowsingContext final : public mozilla::dom::BrowsingContext
>+{
>+public:
>+  ContentBrowsingContext(nsIDocShell* aDocShell)
>+    : BrowsingContext(aDocShell)
>+  {
>+  }
>+
>+  virtual mozilla::Maybe<uint64_t> OwnerProcessId() const final
>+  {
>+    return mozilla::Nothing();
>+  }
>+};
>+}
>+
> namespace mozilla {
> namespace dom {
> 
> static LazyLogModule gBrowsingContextLog("BrowsingContext");
> 
> static StaticAutoPtr<BrowsingContext::Children> sRootBrowsingContexts;
> 
> static StaticAutoPtr<nsDataHashtable<nsUint64HashKey, BrowsingContext*>>
>@@ -49,16 +94,36 @@ BrowsingContext::Init()
> 
>   if (!sCachedBrowsingContexts) {
>     sCachedBrowsingContexts =
>       new nsRefPtrHashtable<nsUint64HashKey, BrowsingContext>();
>     ClearOnShutdown(&sCachedBrowsingContexts);
>   }
> }
> 
>+/* static */ already_AddRefed<BrowsingContext>
>+BrowsingContext::Create(nsIDocShell* aDocShell)
>+{
>+  if (XRE_IsParentProcess()) {
>+    return MakeAndAddRef<ChromeBrowsingContext>(aDocShell);
>+  } else {
>+    return MakeAndAddRef<ContentBrowsingContext>(aDocShell);
>+  }
>+}
>+
>+/* static */ already_AddRefed<BrowsingContext>
>+BrowsingContext::Create(uint64_t aBrowsingContextId,
>+                        const nsAString& aName,
>+                        uint64_t aProcessId)
>+{
>+  MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess());
>+  return MakeAndAddRef<ChromeBrowsingContext>(
>+    aBrowsingContextId, aName, aProcessId);
>+}
>+
> /* static */ LogModule*
> BrowsingContext::GetLog()
> {
>   return gBrowsingContextLog;
> }
> 
> // TODO(farre): BrowsingContext::CleanupContexts starts from the list
> // of root BrowsingContexts. This isn't enough when separate
>@@ -68,51 +133,43 @@ BrowsingContext::GetLog()
> /* static */ void
> BrowsingContext::CleanupContexts(uint64_t aProcessId)
> {
>   if (sRootBrowsingContexts) {
>     RefPtr<BrowsingContext> context = sRootBrowsingContexts->getFirst();
> 
>     while (context) {
>       RefPtr<BrowsingContext> next = context->getNext();
>-      if (context->IsOwnedByProcess() &&
>-          aProcessId == context->OwnerProcessId()) {
>+      if (context->IsOwnedByProcess(aProcessId)) {
>         context->Detach();
>       }
>       context = next;
>     }
>   }
> }
> 
> /* static */ already_AddRefed<BrowsingContext>
> BrowsingContext::Get(uint64_t aId)
> {
>   RefPtr<BrowsingContext> abc = sBrowsingContexts->Get(aId);
>   return abc.forget();
> }
> 
> BrowsingContext::BrowsingContext(nsIDocShell* aDocShell)
>   : mBrowsingContextId(nsContentUtils::GenerateBrowsingContextId())
>-  , mProcessId(Nothing())
>   , mDocShell(aDocShell)
>+
> {
>-  sBrowsingContexts->Put(mBrowsingContextId, this);
> }
> 
> BrowsingContext::BrowsingContext(uint64_t aBrowsingContextId,
>-                                 const nsAString& aName,
>-                                 const Maybe<uint64_t>& aProcessId)
>+                                 const nsAString& aName)
>   : mBrowsingContextId(aBrowsingContextId)
>-  , mProcessId(aProcessId)
>   , mName(aName)
> {
>-  // mProcessId only really has a meaning in the parent process, where
>-  // it keeps track of which BrowsingContext is actually holding the
>-  // nsDocShell.
>-  MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess() || aProcessId.isNothing());
>   sBrowsingContexts->Put(mBrowsingContextId, this);
> }
> 
> void
> BrowsingContext::Attach(BrowsingContext* aParent)
> {
>   if (isInList()) {
>     MOZ_LOG(GetLog(),
>@@ -218,23 +275,16 @@ BrowsingContext::CacheChildren()
> }
> 
> bool
> BrowsingContext::IsCached()
> {
>   return sCachedBrowsingContexts->Contains(Id());
> }
> 
>-uint64_t
>-BrowsingContext::OwnerProcessId() const
>-{
>-  MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess());
>-  return mProcessId.value();
>-}
>-
> BrowsingContext::~BrowsingContext()
> {
>   MOZ_DIAGNOSTIC_ASSERT(!isInList());
> 
>   if (sBrowsingContexts) {
>     sBrowsingContexts->Remove(mBrowsingContextId);
>   }
> }
>diff --git a/docshell/base/BrowsingContext.h b/docshell/base/BrowsingContext.h
>index caed385483f7..5f3ca2ddfcb3 100644
>--- a/docshell/base/BrowsingContext.h
>+++ b/docshell/base/BrowsingContext.h
>@@ -36,32 +36,34 @@ namespace dom {
> // BrowsingContext tree for a tab, in both the parent and the child
> // process.
> class BrowsingContext
>   : public SupportsWeakPtr<BrowsingContext>
>   , public LinkedListElement<RefPtr<BrowsingContext>>
> {
> public:
>   static void Init();
>-  static LogModule* GetLog();
>-  static void CleanupContexts(uint64_t aProcessId);
>-
>-  static already_AddRefed<BrowsingContext> Get(uint64_t aId);
> 
>   // Create a new BrowsingContext for 'aDocShell'. The id will be
>   // generated so that it is unique across all content child processes
>   // and the content parent process.
>-  explicit BrowsingContext(nsIDocShell* aDocShell);
>+  static already_AddRefed<BrowsingContext> Create(nsIDocShell* aDocShell);
>+
>   // Create a BrowsingContext for a particular BrowsingContext id, in
>   // the case where the id is known beforehand and a nsDocShell isn't
>   // needed (e.g. when creating BrowsingContexts in the parent
>-  // process).
>-  BrowsingContext(uint64_t aBrowsingContextId,
>-                  const nsAString& aName,
>-                  const Maybe<uint64_t>& aProcessId = Nothing());
>+  // process). Only valid in the parent process.
>+  static already_AddRefed<BrowsingContext> Create(uint64_t aBrowsingContextId,
>+                                                  const nsAString& aName,
>+                                                  uint64_t aProcessId);
>+
>+  static LogModule* GetLog();
>+  static void CleanupContexts(uint64_t aProcessId);
>+
>+  static already_AddRefed<BrowsingContext> Get(uint64_t aId);
> 
>   // Attach the current BrowsingContext to its parent, in both the
>   // child and the parent process. If 'aParent' is null, 'this' is
>   // taken to be a root BrowsingContext.
>   void Attach(BrowsingContext* aParent);
> 
>   // Detach the current BrowsingContext from its parent, in both the
>   // child and the parent process.
>@@ -73,35 +75,42 @@ public:
> 
>   bool IsCached();
> 
>   void SetName(const nsAString& aName) { mName = aName; }
>   void GetName(nsAString& aName) { aName = mName; }
>   bool NameEquals(const nsAString& aName) { return mName.Equals(aName); }
> 
>   uint64_t Id() const { return mBrowsingContextId; }
>-  uint64_t OwnerProcessId() const;
>-  bool IsOwnedByProcess() const { return mProcessId.isSome(); }
>+  bool IsOwnedByProcess(uint64_t aProcessId) const
>+  {
>+    Maybe<uint64_t> processId = OwnerProcessId();
>+    return processId.isSome() && processId.value() == aProcessId;
>+  }
> 
>   BrowsingContext* Parent() const { return mParent; }
> 
>   MOZ_DECLARE_WEAKREFERENCE_TYPENAME(BrowsingContext)
>   NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(BrowsingContext)
>   NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(BrowsingContext)
> 
>   using Children = AutoCleanLinkedList<RefPtr<BrowsingContext>>;
> 
>-private:
>-  ~BrowsingContext();
>+protected:
>+  virtual Maybe<uint64_t> OwnerProcessId() const = 0;
> 
>-  const uint64_t mBrowsingContextId;
>+  explicit BrowsingContext(nsIDocShell* aDocShell);
> 
>-  // Indicates which process owns the docshell. Only valid in the
>-  // parent process.
>-  Maybe<uint64_t> mProcessId;
>+  BrowsingContext(uint64_t aBrowsingContextId,
>+                  const nsAString& aName);
>+
>+  virtual ~BrowsingContext();
>+
>+private:
>+  const uint64_t mBrowsingContextId;
> 
>   WeakPtr<BrowsingContext> mParent;
>   Children mChildren;
>   nsCOMPtr<nsIDocShell> mDocShell;
>   nsString mName;
> };
> 
> } // namespace dom
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>index ea0c2d3e40d4..e1b741aa7184 100644
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -471,17 +471,17 @@ nsDocShell::Init()
>   // ref to us...  use an InterfaceRequestorProxy to do this.
>   nsCOMPtr<nsIInterfaceRequestor> proxy =
>     new InterfaceRequestorProxy(static_cast<nsIInterfaceRequestor*>(this));
>   mLoadGroup->SetNotificationCallbacks(proxy);
> 
>   rv = nsDocLoader::AddDocLoaderAsChildOfRoot(this);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  mBrowsingContext = new BrowsingContext(this);
>+  mBrowsingContext = BrowsingContext::Create(this);
> 
>   // Add as |this| a progress listener to itself.  A little weird, but
>   // simpler than reproducing all the listener-notification logic in
>   // overrides of the various methods via which nsDocLoader can be
>   // notified.   Note that this holds an nsWeakPtr to ourselves, so it's ok.
>   return AddProgressListener(this, nsIWebProgress::NOTIFY_STATE_DOCUMENT |
>                                      nsIWebProgress::NOTIFY_STATE_NETWORK);
> }
>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 356eca614184..c29a124200fb 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp
>@@ -6010,17 +6010,17 @@ ContentParent::RecvAttachBrowsingContext(
>     MOZ_LOG(
>       BrowsingContext::GetLog(),
>       LogLevel::Debug,
>       ("ParentIPC: Trying to attach to already detached parent 0x%08" PRIx64,
>        (uint64_t)aParentId));
>     return IPC_OK();
>   }
> 
>-  if (parent && parent->OwnerProcessId() != ChildID()) {
>+  if (parent && !parent->IsOwnedByProcess(ChildID())) {
>     // Where trying attach a child BrowsingContext to a parent
>     // BrowsingContext in another process. This is illegal since the
>     // only thing that could create that child BrowsingContext is a
>     // parent docshell in the same process as that BrowsingContext.
> 
>     // TODO(farre): We're doing nothing now, but is that exactly what
>     // we want? Maybe we want to crash the child currently calling
>     // SendAttachBrowsingContext and/or the child that originally
>@@ -6048,17 +6048,17 @@ ContentParent::RecvAttachBrowsingContext(
>             ("ParentIPC: Trying to attach already attached 0x%08" PRIx64
>              " to 0x%08" PRIx64,
>              child->Id(),
>              (uint64_t)aParentId));
>     return IPC_OK();
>   }
> 
>   if (!child) {
>-    child = new BrowsingContext(aChildId, aName, Some(ChildID()));
>+    child = BrowsingContext::Create(aChildId, aName, ChildID());
>   }
>   child->Attach(parent);
> 
>   return IPC_OK();
> }
> 
> mozilla::ipc::IPCResult
> ContentParent::RecvDetachBrowsingContext(const BrowsingContextId& aContextId,
>@@ -6069,17 +6069,17 @@ ContentParent::RecvDetachBrowsingContext(const BrowsingContextId& aContextId,
>   if (!context) {
>     MOZ_LOG(BrowsingContext::GetLog(),
>             LogLevel::Debug,
>             ("ParentIPC: Trying to detach already detached 0x%08" PRIx64,
>              (uint64_t)aContextId));
>     return IPC_OK();
>   }
> 
>-  if (context->OwnerProcessId() != ChildID()) {
>+  if (!context->IsOwnedByProcess(ChildID())) {
>     // Where trying to detach a child BrowsingContext in another child
>     // process. This is illegal since the owner of the BrowsingContext
>     // is the proccess with the in-process docshell, which is tracked
>     // by OwnerProcessId.
> 
>     // TODO(farre): To crash or not to crash. Same reasoning as in
>     // above TODO. [Bug 1471598]
>     MOZ_LOG(BrowsingContext::GetLog(),
>-- 
>2.17.1
>

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 9007755 [details]
Bug 1480113 - Split parent process specific parts out of BrowsingContext

Peter Van der Beken [:peterv] has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1de6dfd1e6ed
Split parent process specific parts out of BrowsingContext r=peterv

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1de6dfd1e6ed

Comment 8

[Chris Peterson [:cpeterson]]

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732