Bug 1529684 - Part 6: Store a mIsInProcess flag to preserve WindowProxy behaviour, r=farre
authorNika Layzell <nika@thelayzells.com>
Thu, 14 Mar 2019 18:50:54 +0000
changeset 464039 2e9f31ee6571578ab190de831353a3a870609f07
parent 464038 cb217921cf9a1aee8d4f3b43fe0209c0899ddcc4
child 464040 42b3b54ac6f7fbdee69434f33658e0353c2f03bb
push id35707
push userrmaries@mozilla.com
push dateFri, 15 Mar 2019 03:42:43 +0000
treeherdermozilla-central@5ce27c44f79e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
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 1529684 - Part 6: Store a mIsInProcess flag to preserve WindowProxy behaviour, r=farre Currently when we have an in-process WindowProxy object, we will attempt to either use the cached mWindowProxy value, or fetch the nsGlobalWindowOuter object from through the nsDocShell. Unfortunately, when the BrowsingContext is detached, we will fail to get the nsGlobalWindowOuter object. This happens to be OK for our test cases, as the cached mWindowProxy value doesn't have the chance to go away, but isn't acceptable long-term. These patches exascerbated that issue by causing the nsDocShell pointer itself to be cleared when it is destroyed, which caused the Remote WindowProxy logic to be triggered. To deal with that case, this patch adds a new mIsInProcess flag to continue to act like the old code-path. In the future, we will need to also handle ensuring that the nsGlobalWindowOuter lives for long enough, however that is not being done in this patch in order to land it sooner rather than later. Depends on D22763 Differential Revision: https://phabricator.services.mozilla.com/D22764
--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -156,16 +156,17 @@ BrowsingContext::BrowsingContext(Browsin
                                  BrowsingContext* aOpener,
                                  const nsAString& aName,
                                  uint64_t aBrowsingContextId, Type aType)
     : mName(aName),
+      mIsInProcess(false),
       mIsActivatedByUserGesture(false) {
   mCrossOriginPolicy = nsILoadInfo::CROSS_ORIGIN_POLICY_NULL;
   // Specify our group in our constructor. We will explicitly join the group
   // when we are registered, as doing so will take a reference.
   if (mParent) {
     mGroup = mParent->Group();
     mCrossOriginPolicy = mParent->CrossOriginPolicy();
@@ -179,16 +180,17 @@ BrowsingContext::BrowsingContext(Browsin
 void BrowsingContext::SetDocShell(nsIDocShell* aDocShell) {
   // XXX(nika): We should communicate that we are now an active BrowsingContext
   // process to the parent & do other validation here.
   MOZ_RELEASE_ASSERT(nsDocShell::Cast(aDocShell)->GetBrowsingContext() == this);
   mDocShell = aDocShell;
+  mIsInProcess = true;
 void BrowsingContext::Attach(bool aFromIPC) {
   MOZ_LOG(GetLog(), LogLevel::Debug,
           ("%s: %s 0x%08" PRIx64 " to 0x%08" PRIx64,
            XRE_IsParentProcess() ? "Parent" : "Child",
            sCachedBrowsingContexts->has(Id()) ? "Re-connecting" : "Connecting",
            Id(), mParent ? mParent->Id() : 0));
@@ -236,16 +238,21 @@ void BrowsingContext::Detach(bool aFromI
   // By definition, we no longer are the current process for this
   // BrowsingContext - clear our now-dead nsDocShell reference.
   mDocShell = nullptr;
+  // As our nsDocShell is going away, this should implicitly mark us as closed.
+  // We directly set our member, rather than using a transaction as we're going
+  // to send a `Detach` message to other processes either way.
+  mClosed = true;
   if (!aFromIPC && XRE_IsContentProcess()) {
     auto cc = ContentChild::GetSingleton();
     cc->SendDetachBrowsingContext(this, false /* aMoveToBFCache */);
 void BrowsingContext::CacheChildren(bool aFromIPC) {
--- a/docshell/base/BrowsingContext.h
+++ b/docshell/base/BrowsingContext.h
@@ -160,16 +160,21 @@ class BrowsingContext : public nsWrapper
   // Create a BrowsingContext object from over IPC.
   static already_AddRefed<BrowsingContext> CreateFromIPC(
       BrowsingContext* aParent, BrowsingContext* aOpener,
       const nsAString& aName, uint64_t aId, ContentParent* aOriginProcess);
   // Cast this object to a canonical browsing context, and return it.
   CanonicalBrowsingContext* Canonical();
+  // Is the most recent Document in this BrowsingContext loaded within this
+  // process? This may be true with a null mDocShell after the Window has been
+  // closed.
+  bool IsInProcess() const { return mIsInProcess; }
   // Get the DocShell for this BrowsingContext if it is in-process, or
   // null if it's not.
   nsIDocShell* GetDocShell() { return mDocShell; }
   void SetDocShell(nsIDocShell* aDocShell);
   // Get the outer window object for this BrowsingContext if it is in-process
   // and still has a docshell, or null otherwise.
   nsPIDOMWindowOuter* GetDOMWindow() const {
@@ -369,26 +374,32 @@ class BrowsingContext : public nsWrapper
   // Unique id identifying BrowsingContext
   const uint64_t mBrowsingContextId;
   RefPtr<BrowsingContextGroup> mGroup;
   RefPtr<BrowsingContext> mParent;
   Children mChildren;
   WeakPtr<BrowsingContext> mOpener;
   nsCOMPtr<nsIDocShell> mDocShell;
   // This is not a strong reference, but using a JS::Heap for that should be
   // fine. The JSObject stored in here should be a proxy with a
   // nsOuterWindowProxy handler, which will update the pointer from its
   // objectMoved hook and clear it from its finalize hook.
   JS::Heap<JSObject*> mWindowProxy;
   LocationProxy mLocation;
   // This flag is only valid in the top level browsing context, it indicates
   // whether the corresponding document has been activated by user gesture.
   bool mIsActivatedByUserGesture;
+  // Is the most recent Document in this BrowsingContext loaded within this
+  // process? This may be true with a null mDocShell after the Window has been
+  // closed.
+  bool mIsInProcess : 1;
  * Gets a WindowProxy object for a BrowsingContext that lives in a different
  * process (creating the object if it doesn't already exist). The WindowProxy
  * object will be in the compartment that aCx is currently in. This should only
  * be called if aContext doesn't hold a docshell, otherwise the BrowsingContext
  * lives in this process, and a same-process WindowProxy should be used (see
--- a/dom/bindings/ToJSValue.cpp
+++ b/dom/bindings/ToJSValue.cpp
@@ -64,17 +64,17 @@ bool ToJSValue(JSContext* aCx, Promise& 
 bool ToJSValue(JSContext* aCx, const WindowProxyHolder& aArgument,
                JS::MutableHandle<JS::Value> aValue) {
   BrowsingContext* bc = aArgument.get();
   if (!bc) {
     return true;
   JS::Rooted<JSObject*> windowProxy(aCx);
-  if (bc->GetDocShell()) {
+  if (bc->IsInProcess()) {
     windowProxy = bc->GetWindowProxy();
     if (!windowProxy) {
       nsPIDOMWindowOuter* window = bc->GetDOMWindow();
       if (!window) {
         // Torn down enough that we should just return null.
         return true;