Bug 1569930 - Handle races in CrossProcessPaint without crashing, and instead report it back to the caller. r=rhunt
authorMatt Woodrow <mwoodrow@mozilla.com>
Wed, 14 Aug 2019 10:23:34 +0000
changeset 487878 b8c5fc82d21ddb485d6565dd49f31f98e95a684f
parent 487877 8fc581cf9ff89d2ad6993d82ba165a2b76fbcd02
child 487879 5b8583d3aa3782ca1140b20e493d1e10e6390ffc
push id36433
push userbtara@mozilla.com
push dateWed, 14 Aug 2019 21:57:52 +0000
treeherdermozilla-central@7d9a2196d313 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrhunt
bugs1569930
milestone70.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 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
gfx/ipc/CrossProcessPaint.cpp
gfx/ipc/CrossProcessPaint.h
--- a/gfx/ipc/CrossProcessPaint.cpp
+++ b/gfx/ipc/CrossProcessPaint.cpp
@@ -201,37 +201,49 @@ bool CrossProcessPaint::Start(dom::Windo
 }
 
 CrossProcessPaint::CrossProcessPaint(dom::Promise* aPromise, float aScale,
                                      dom::WindowGlobalParent* aRoot)
     : mPromise{aPromise}, mRoot{aRoot}, mScale{aScale}, mPendingFragments{0} {}
 
 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()) {
     CPP_LOG("Ignoring fragment from %p.\n", 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()) {
     auto dependency = dom::TabId(iter.Get()->GetKey());
 
     // Get the current WindowGlobalParent of the remote browser that was marked
     // as a dependency
     dom::ContentProcessManager* cpm =
@@ -247,82 +259,75 @@ void CrossProcessPaint::ReceiveFragment(
               (uint64_t)dependency);
       continue;
     }
 
     // TODO: Apply some sort of clipping to visible bounds here (Bug 1562720)
     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
   MaybeResolve();
 }
 
 void CrossProcessPaint::LostFragment(dom::WindowGlobalParent* aWGP) {
   if (IsCleared()) {
     CPP_LOG("Ignoring lost fragment from %p.\n", 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);
 
   // TODO - Don't apply the background color to all paints (Bug 1562722)
   aWGP->DrawSnapshotInternal(this, aRect, mScale, aBackgroundColor);
   mPendingFragments += 1;
 }
 
 void CrossProcessPaint::Clear() {
   mPromise = nullptr;
   mPendingFragments = 0;
   mReceivedFragments.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
   if (IsCleared() || mPendingFragments > 0) {
     CPP_LOG("Not ready to resolve yet, have %u fragments left.\n",
             mPendingFragments);
     return;
   }
 
   CPP_LOG("Starting to resolve fragments.\n");
 
   // 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.
   RefPtr<SourceSurface> root = resolved.Get(GetTabId(mRoot));
   CPP_LOG("Resolved all fragments.\n");
 
   ErrorResult rv;
   RefPtr<dom::ImageBitmap> bitmap = dom::ImageBitmap::CreateFromSourceSurface(
@@ -333,77 +338,71 @@ void CrossProcessPaint::MaybeResolve() {
     mPromise->MaybeResolve(bitmap);
   } else {
     CPP_LOG("Couldn't create ImageBitmap for SourceSurface.\n");
     mPromise->MaybeReject(rv);
   }
   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 %llu.\n", (uint64_t)aTabId);
 
-  CPP_LOG("Resolving fragment %p.\n", aWGP);
-
-  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;
     }
   }
 
   // Create the destination draw target
   RefPtr<DrawTarget> drawTarget =
       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
   {
     InlineTranslator translator(drawTarget, nullptr);
     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
   // them
   for (auto iter = fragment->mDependencies.Iter(); !iter.Done(); iter.Next()) {
     auto dependency = iter.Get()->GetKey();
     aResolved->Remove(dependency);
   }
 
-  aResolved->Put(surfaceId, snapshot);
-  return true;
+  aResolved->Put(aTabId, snapshot);
+  return NS_OK;
 }
 
 }  // namespace gfx
 }  // namespace mozilla
--- a/gfx/ipc/CrossProcessPaint.h
+++ b/gfx/ipc/CrossProcessPaint.h
@@ -111,19 +111,17 @@ class CrossProcessPaint final {
                     dom::Promise* aPromise);
 
   void ReceiveFragment(dom::WindowGlobalParent* aWGP,
                        PaintFragment&& aFragment);
   void LostFragment(dom::WindowGlobalParent* aWGP);
 
  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);
   ~CrossProcessPaint();
 
   void QueuePaint(dom::WindowGlobalParent* aWGP, const Maybe<IntRect>& aRect,
                   nscolor aBackgroundColor = NS_RGBA(0, 0, 0, 0));
 
@@ -132,18 +130,17 @@ class CrossProcessPaint final {
   void Clear();
 
   /// Returns if this paint has been cleared.
   bool IsCleared() const;
 
   /// 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;
   float mScale;
   uint32_t mPendingFragments;
   ReceivedFragmentMap mReceivedFragments;
 };