Bug 1691475 - Remove shared surfaces on the compositor thread. r=jrmuizel, a=pascalc
authorAndrew Osmond <aosmond@mozilla.com>
Mon, 08 Feb 2021 18:52:15 +0000
changeset 631507 2a5a30f4e77fa6fe2860d49c835ce34c49f37c56
parent 631506 eace8bb4ecd31d6e421f8265244e078e112bdd40
child 631508 588cc1ab1172683e065e1817c4db5cf34981441b
push id15099
push userjcristau@mozilla.com
push dateTue, 09 Feb 2021 17:09:59 +0000
treeherdermozilla-beta@588cc1ab1172 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel, pascalc
bugs1691475
milestone86.0
Bug 1691475 - Remove shared surfaces on the compositor thread. r=jrmuizel, a=pascalc We add a shared surface on the main thread to the shared surfaces table when in the compositor process because it uses a different wrapper object if the shared surface is in a different process. This structure was mirrored for the removal of a shared surface created in the compositor process, however that created a race between the main thread freeing the surface before the display list creation bound an image key to the shared surface's external image ID. As such, this patch makes removal always post to the compositor thread, whether in the compositor process or not. Differential Revision: https://phabricator.services.mozilla.com/D104426
gfx/layers/ipc/SharedSurfacesChild.cpp
gfx/layers/ipc/SharedSurfacesParent.cpp
gfx/layers/ipc/SharedSurfacesParent.h
--- a/gfx/layers/ipc/SharedSurfacesChild.cpp
+++ b/gfx/layers/ipc/SharedSurfacesChild.cpp
@@ -411,30 +411,21 @@ void SharedSurfacesChild::Unshare(const 
     return;
   }
 
   CompositorManagerChild* manager = CompositorManagerChild::GetInstance();
   if (MOZ_UNLIKELY(!manager || !manager->CanSend())) {
     return;
   }
 
-  if (manager->OtherPid() == base::GetCurrentProcId()) {
-    // We are in the combined UI/GPU process. Call directly to it to remove its
-    // wrapper surface to free the underlying buffer, but only if the external
-    // image ID is owned by the manager. It can be different if the surface was
-    // last shared with the GPU process, which crashed several times, and its
-    // job was moved into the parent process.
-    if (manager->OwnsExternalImageId(aId)) {
-      SharedSurfacesParent::RemoveSameProcess(aId);
-    }
-  } else if (manager->OwnsExternalImageId(aId)) {
-    // Only attempt to release current mappings in the GPU process. It is
-    // possible we had a surface that was previously shared, the GPU process
-    // crashed / was restarted, and then we freed the surface. In that case
-    // we know the mapping has already been freed.
+  if (manager->OwnsExternalImageId(aId)) {
+    // Only attempt to release current mappings in the compositor process. It is
+    // possible we had a surface that was previously shared, the compositor
+    // process crashed / was restarted, and then we freed the surface. In that
+    // case we know the mapping has already been freed.
     manager->SendRemoveSharedSurface(aId);
   }
 }
 
 /* static */ Maybe<wr::ExternalImageId> SharedSurfacesChild::GetExternalId(
     const SourceSurfaceSharedData* aSurface) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aSurface);
--- a/gfx/layers/ipc/SharedSurfacesParent.cpp
+++ b/gfx/layers/ipc/SharedSurfacesParent.cpp
@@ -140,23 +140,16 @@ void SharedSurfacesParent::AddSameProces
   }
   wr::RenderThread::Get()->RegisterExternalImage(id, texture.forget());
 
   surface->AddConsumer();
   sInstance->mSurfaces.Put(id, std::move(surface));
 }
 
 /* static */
-void SharedSurfacesParent::RemoveSameProcess(const wr::ExternalImageId& aId) {
-  MOZ_ASSERT(XRE_IsParentProcess());
-  MOZ_ASSERT(NS_IsMainThread());
-  Release(aId, /* aForCreator */ true);
-}
-
-/* static */
 void SharedSurfacesParent::DestroyProcess(base::ProcessId aPid) {
   StaticMutexAutoLock lock(sMutex);
   if (!sInstance) {
     return;
   }
 
   // Note that the destruction of a parent may not be cheap if it still has a
   // lot of surfaces still bound that require unmapping.
--- a/gfx/layers/ipc/SharedSurfacesParent.h
+++ b/gfx/layers/ipc/SharedSurfacesParent.h
@@ -62,17 +62,16 @@ class SharedSurfacesParent final {
 
  private:
   friend class SharedSurfacesChild;
 
   SharedSurfacesParent();
 
   static void AddSameProcess(const wr::ExternalImageId& aId,
                              gfx::SourceSurfaceSharedData* aSurface);
-  static void RemoveSameProcess(const wr::ExternalImageId& aId);
 
   static StaticMutex sMutex;
 
   static StaticAutoPtr<SharedSurfacesParent> sInstance;
 
   nsRefPtrHashtable<nsUint64HashKey, gfx::SourceSurfaceSharedDataWrapper>
       mSurfaces;
 };