Bug 1569930 - Handle races in CrossProcessPaint without crashing, and instead report it back to the caller. r=rhunt

Differential Revision: https://phabricator.services.mozilla.com/D41727

--HG--
extra : moz-landing-system : lando

gfx/ipc/CrossProcessPaint.cpp

@@ -206,6 +206,16 @@ CrossProcessPaint::CrossProcessPaint(dom::Promise* aPromise, float aScale,
 
 CrossProcessPaint::~CrossProcessPaint() {}
 
+static dom::TabId GetTabId(dom::WindowGlobalParent* aWGP) {
+  // There is no unique TabId for a given WindowGlobalParent, as multiple
+  // WindowGlobalParents share the same PBrowser actor. However, we only
+  // ever queue one paint per PBrowser by just using the current
+  // WindowGlobalParent for a PBrowser. So we can interchange TabId and
+  // WindowGlobalParent when dealing with resolving surfaces.
+  RefPtr<dom::BrowserParent> browserParent = aWGP->GetBrowserParent();
+  return browserParent ? browserParent->GetTabId() : dom::TabId(0);
+}
+
 void CrossProcessPaint::ReceiveFragment(dom::WindowGlobalParent* aWGP,
                                         PaintFragment&& aFragment) {
   if (IsCleared()) {
@@ -213,20 +223,22 @@ void CrossProcessPaint::ReceiveFragment(dom::WindowGlobalParent* aWGP,
     return;
   }
 
+  dom::TabId surfaceId = GetTabId(aWGP);
+
   MOZ_ASSERT(mPendingFragments > 0);
-  MOZ_ASSERT(!mReceivedFragments.GetValue(aWGP));
+  MOZ_ASSERT(!mReceivedFragments.GetValue(surfaceId));
   MOZ_ASSERT(!aFragment.IsEmpty());
 
   // Double check our invariants to protect against a compromised content
   // process
-  if (mPendingFragments == 0 || mReceivedFragments.GetValue(aWGP) ||
+  if (mPendingFragments == 0 || mReceivedFragments.GetValue(surfaceId) ||
       aFragment.IsEmpty()) {
     CPP_LOG("Dropping invalid fragment from %p.\n", aWGP);
     LostFragment(aWGP);
     return;
   }
 
-  CPP_LOG("Receiving fragment from %p.\n", aWGP);
+  CPP_LOG("Receiving fragment from %p(%llu).\n", aWGP, (uint64_t)surfaceId);
 
   // Queue paints for child tabs
   for (auto iter = aFragment.mDependencies.Iter(); !iter.Done(); iter.Next()) {
@@ -252,7 +264,7 @@ void CrossProcessPaint::ReceiveFragment(dom::WindowGlobalParent* aWGP,
     QueuePaint(wgp, Nothing());
   }
 
-  mReceivedFragments.Put(aWGP, std::move(aFragment));
+  mReceivedFragments.Put(surfaceId, std::move(aFragment));
   mPendingFragments -= 1;
 
   // Resolve this paint if we have received all pending fragments
@@ -265,14 +277,14 @@ void CrossProcessPaint::LostFragment(dom::WindowGlobalParent* aWGP) {
     return;
   }
 
-  mPromise->MaybeReject(NS_ERROR_FAILURE);
+  mPromise->MaybeReject(NS_ERROR_LOSS_OF_SIGNIFICANT_DATA);
   Clear();
 }
 
 void CrossProcessPaint::QueuePaint(dom::WindowGlobalParent* aWGP,
                                    const Maybe<IntRect>& aRect,
                                    nscolor aBackgroundColor) {
-  MOZ_ASSERT(!mReceivedFragments.GetValue(aWGP));
+  MOZ_ASSERT(!mReceivedFragments.GetValue(GetTabId(aWGP)));
 
   CPP_LOG("Queueing paint for %p.\n", aWGP);
 
@@ -289,16 +301,6 @@ void CrossProcessPaint::Clear() {
 
 bool CrossProcessPaint::IsCleared() const { return !mPromise; }
 
-static dom::TabId GetTabId(dom::WindowGlobalParent* aWGP) {
-  // There is no unique TabId for a given WindowGlobalParent, as multiple
-  // WindowGlobalParents share the same PBrowser actor. However, we only
-  // ever queue one paint per PBrowser by just using the current
-  // WindowGlobalParent for a PBrowser. So we can interchange TabId and
-  // WindowGlobalParent when dealing with resolving surfaces.
-  RefPtr<dom::BrowserParent> browserParent = aWGP->GetBrowserParent();
-  return browserParent ? browserParent->GetTabId() : dom::TabId(0);
-}
-
 void CrossProcessPaint::MaybeResolve() {
   // Don't do anything if we aren't ready, experienced an error, or already
   // resolved this paint
@@ -312,12 +314,15 @@ void CrossProcessPaint::MaybeResolve() {
 
   // Resolve the paint fragments from the bottom up
   ResolvedSurfaceMap resolved;
-  if (!ResolveInternal(mRoot, &resolved)) {
-    CPP_LOG("Couldn't resolve.\n");
+  {
+    nsresult rv = ResolveInternal(GetTabId(mRoot), &resolved);
+    if (NS_FAILED(rv)) {
+      CPP_LOG("Couldn't resolve.\n");
 
-    mPromise->MaybeReject(NS_ERROR_FAILURE);
-    Clear();
-    return;
+      mPromise->MaybeReject(rv);
+      Clear();
+      return;
+    }
   }
 
   // Grab the result from the resolved table.
@@ -338,32 +343,25 @@ void CrossProcessPaint::MaybeResolve() {
   Clear();
 }
 
-bool CrossProcessPaint::ResolveInternal(dom::WindowGlobalParent* aWGP,
-                                        ResolvedSurfaceMap* aResolved) {
-  // Convert aWGP to an ID we can use for surfaces
-  dom::TabId surfaceId = GetTabId(aWGP);
-
+nsresult CrossProcessPaint::ResolveInternal(dom::TabId aTabId,
+                                            ResolvedSurfaceMap* aResolved) {
   // We should not have resolved this paint already
-  MOZ_ASSERT(!aResolved->GetWeak(surfaceId));
+  MOZ_ASSERT(!aResolved->GetWeak(aTabId));
 
-  CPP_LOG("Resolving fragment %p.\n", aWGP);
+  CPP_LOG("Resolving fragment %llu.\n", (uint64_t)aTabId);
 
-  Maybe<PaintFragment> fragment = mReceivedFragments.GetAndRemove(aWGP);
+  Maybe<PaintFragment> fragment = mReceivedFragments.GetAndRemove(aTabId);
+  if (!fragment) {
+    return NS_ERROR_LOSS_OF_SIGNIFICANT_DATA;
+  }
 
   // Rasterize all the dependencies first so that we can resolve this fragment
   for (auto iter = fragment->mDependencies.Iter(); !iter.Done(); iter.Next()) {
     auto dependency = dom::TabId(iter.Get()->GetKey());
 
-    dom::ContentProcessManager* cpm =
-        dom::ContentProcessManager::GetSingleton();
-    dom::ContentParentId cpId = cpm->GetTabProcessId(dependency);
-    RefPtr<dom::BrowserParent> tab =
-        cpm->GetBrowserParentByProcessAndTabId(cpId, dependency);
-    RefPtr<dom::WindowGlobalParent> wgp =
-        tab->GetBrowsingContext()->GetCurrentWindowGlobal();
-
-    if (!ResolveInternal(wgp, aResolved)) {
-      return false;
+    nsresult rv = ResolveInternal(dependency, aResolved);
+    if (NS_FAILED(rv)) {
+      return rv;
     }
   }
 
@@ -372,9 +370,9 @@ bool CrossProcessPaint::ResolveInternal(dom::WindowGlobalParent* aWGP,
       gfxPlatform::GetPlatform()->CreateOffscreenContentDrawTarget(
           fragment->mSize, SurfaceFormat::B8G8R8A8);
   if (!drawTarget || !drawTarget->IsValid()) {
-    CPP_LOG("Couldn't create (%d x %d) surface for fragment %p.\n",
-            fragment->mSize.width, fragment->mSize.height, aWGP);
-    return false;
+    CPP_LOG("Couldn't create (%d x %d) surface for fragment %llu.\n",
+            fragment->mSize.width, fragment->mSize.height, (uint64_t)aTabId);
+    return NS_ERROR_FAILURE;
   }
 
   // Translate the recording using our child tabs
@@ -383,15 +381,16 @@ bool CrossProcessPaint::ResolveInternal(dom::WindowGlobalParent* aWGP,
     translator.SetExternalSurfaces(aResolved);
     if (!translator.TranslateRecording((char*)fragment->mRecording.mData,
                                        fragment->mRecording.mLen)) {
-      CPP_LOG("Couldn't translate recording for fragment %p.\n", aWGP);
-      return false;
+      CPP_LOG("Couldn't translate recording for fragment %llu.\n",
+              (uint64_t)aTabId);
+      return NS_ERROR_FAILURE;
     }
   }
 
   RefPtr<SourceSurface> snapshot = drawTarget->Snapshot();
   if (!snapshot) {
-    CPP_LOG("Couldn't get snapshot for fragment %p.\n", aWGP);
-    return false;
+    CPP_LOG("Couldn't get snapshot for fragment %llu.\n", (uint64_t)aTabId);
+    return NS_ERROR_FAILURE;
   }
 
   // We are done with the resolved images of our dependencies, let's remove
@@ -401,8 +400,8 @@ bool CrossProcessPaint::ResolveInternal(dom::WindowGlobalParent* aWGP,
     aResolved->Remove(dependency);
   }
 
-  aResolved->Put(surfaceId, snapshot);
-  return true;
+  aResolved->Put(aTabId, snapshot);
+  return NS_OK;
 }
 
 }  // namespace gfx

gfx/ipc/CrossProcessPaint.h

@@ -116,9 +116,7 @@ class CrossProcessPaint final {
 
  private:
   typedef nsRefPtrHashtable<nsUint64HashKey, SourceSurface> ResolvedSurfaceMap;
-  typedef nsDataHashtable<nsRefPtrHashKey<dom::WindowGlobalParent>,
-                          PaintFragment>
-      ReceivedFragmentMap;
+  typedef nsDataHashtable<nsUint64HashKey, PaintFragment> ReceivedFragmentMap;
 
   CrossProcessPaint(dom::Promise* aPromise, float aScale,
                     dom::WindowGlobalParent* aRoot);
@@ -137,8 +135,7 @@ class CrossProcessPaint final {
   /// Resolves the paint fragments if we have none pending and resolves the
   /// promise.
   void MaybeResolve();
-  bool ResolveInternal(dom::WindowGlobalParent* aWGP,
-                       ResolvedSurfaceMap* aResolved);
+  nsresult ResolveInternal(dom::TabId aTabId, ResolvedSurfaceMap* aResolved);
 
   RefPtr<dom::Promise> mPromise;
   RefPtr<dom::WindowGlobalParent> mRoot;