Bug 1539959 - Ensure origin process is subscribed when attaching remote BC, r=farre a=pascalc
authorNika Layzell <nika@thelayzells.com>
Thu, 02 May 2019 08:50:12 +0000
changeset 526465 e9cc7e9d9c734497f3e9c226cbda678871ebbe06
parent 526464 89ab6b801db05cfcbdb94e9376d3db952ada4011
child 526466 cdf167f4186a12d4e5cf34655dfd201da159f0cd
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, pascalc
Bug 1539959 - Ensure origin process is subscribed when attaching remote BC, r=farre a=pascalc We were incorrectly not performing an EnsureSubscribed for the origin process which a BrowsingContext came from. Normally this would mean that the BrowsingContext could die in the parent process before the WindowGlobalParent listener arrived, but that didn't generally happen due to the low likelihood of a CC occuring between the two messages. It's likely that this crash was caused by people on lower-end hardware triggering a print. This would be a longer-running operation in the content process between the first and second message than usual. On systems with memory pressure, there would be a higher chance of a CC occuring between the messages, which would then cause this crash. This patch correctly connects the origin ContentParent to the BrowsingContextGroup, which will prevent the CC from destroying our objects. In the future, it may be desirable to ensure that this doesn't happen more reliably by using a ContentParent-local table for looking up BrowsingContext IDs sent over IPC. This would prevent our current dependency on the weak pointer behaviour of the current global ID cache. Unfortunately, this patch adds no tests, and I'm not aware of a good way to test this edge case to confirm it has been fixed. I believe that this patch should fix the issue I mention, however. Differential Revision: https://phabricator.services.mozilla.com/D29563
--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -142,17 +142,21 @@ already_AddRefed<BrowsingContext> Browsi
 /* static */
 already_AddRefed<BrowsingContext> BrowsingContext::CreateFromIPC(
     BrowsingContext::IPCInitializer&& aInit, BrowsingContextGroup* aGroup,
     ContentParent* aOriginProcess) {
   MOZ_DIAGNOSTIC_ASSERT(aOriginProcess || XRE_IsContentProcess());
-  uint64_t originId = aOriginProcess ? aOriginProcess->ChildID() : 0;
+  uint64_t originId = 0;
+  if (aOriginProcess) {
+    originId = aOriginProcess->ChildID();
+    aGroup->EnsureSubscribed(aOriginProcess);
+  }
   MOZ_LOG(GetLog(), LogLevel::Debug,
           ("Creating 0x%08" PRIx64 " from IPC (origin=0x%08" PRIx64 ")",
            aInit.mId, originId));
   RefPtr<BrowsingContext> parent = aInit.GetParent();
   RefPtr<BrowsingContext> context;