Bug 1524280 - Part 1. Ensure we always post when freeing SharedUserData. r=jrmuizel a=jcristau
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 28 May 2019 13:24:49 -0400
changeset 536566 ead433b4f740d1de53ab55b9196900eba788b1f1
parent 536565 250fc068195338d7430f2c82483bbda726f44421
child 536567 dfe3f74cba935f205a6b160bd0084eaf584e2cc6
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel, jcristau
bugs1524280
milestone68.0
Bug 1524280 - Part 1. Ensure we always post when freeing SharedUserData. r=jrmuizel a=jcristau We now also post the releasing of the shared surface image keys and external image ID to the main thread. This allows the current transaction to complete before freeing the surface, which guards against cases where the surface is referenced and released somehow in the space of the same transaction. Differential Revision: https://phabricator.services.mozilla.com/D32861
gfx/layers/ipc/SharedSurfacesChild.cpp
gfx/layers/ipc/SharedSurfacesChild.h
--- a/gfx/layers/ipc/SharedSurfacesChild.cpp
+++ b/gfx/layers/ipc/SharedSurfacesChild.cpp
@@ -49,48 +49,51 @@ void SharedSurfacesChild::ImageKeyData::
     if (aDirtyRect) {
       mDirtyRect->UnionRect(mDirtyRect.ref(), aDirtyRect.ref());
     }
   } else {
     mDirtyRect = aDirtyRect;
   }
 }
 
+SharedSurfacesChild::SharedUserData::SharedUserData(
+    const wr::ExternalImageId& aId)
+    : Runnable("SharedSurfacesChild::SharedUserData"),
+      mId(aId),
+      mShared(false) {}
+
 SharedSurfacesChild::SharedUserData::~SharedUserData() {
+  // We may fail to dispatch during shutdown, and since it would be issued on
+  // the main thread, it releases the runnable instead of leaking it.
   if (mShared || !mKeys.IsEmpty()) {
     if (NS_IsMainThread()) {
       SharedSurfacesChild::Unshare(mId, mShared, mKeys);
     } else {
-      class DestroyRunnable final : public Runnable {
-       public:
-        DestroyRunnable(const wr::ExternalImageId& aId, bool aReleaseId,
-                        nsTArray<ImageKeyData>&& aKeys)
-            : Runnable("SharedSurfacesChild::SharedUserData::DestroyRunnable"),
-              mId(aId),
-              mReleaseId(aReleaseId),
-              mKeys(std::move(aKeys)) {}
-
-        NS_IMETHOD Run() override {
-          SharedSurfacesChild::Unshare(mId, mReleaseId, mKeys);
-          return NS_OK;
-        }
-
-       private:
-        wr::ExternalImageId mId;
-        bool mReleaseId;
-        AutoTArray<ImageKeyData, 1> mKeys;
-      };
-
-      nsCOMPtr<nsIRunnable> task =
-          new DestroyRunnable(mId, mShared, std::move(mKeys));
-      SystemGroup::Dispatch(TaskCategory::Other, task.forget());
+      MOZ_ASSERT_UNREACHABLE("Shared resources not released!");
     }
   }
 }
 
+/* static */
+void SharedSurfacesChild::SharedUserData::Destroy(void* aClosure) {
+  MOZ_ASSERT(aClosure);
+  RefPtr<SharedUserData> data =
+      dont_AddRef(static_cast<SharedUserData*>(aClosure));
+  if (data->mShared || !data->mKeys.IsEmpty()) {
+    SystemGroup::Dispatch(TaskCategory::Other, data.forget());
+  }
+}
+
+NS_IMETHODIMP SharedSurfacesChild::SharedUserData::Run() {
+  SharedSurfacesChild::Unshare(mId, mShared, mKeys);
+  mShared = false;
+  mKeys.Clear();
+  return NS_OK;
+}
+
 wr::ImageKey SharedSurfacesChild::SharedUserData::UpdateKey(
     RenderRootStateManager* aManager, wr::IpcResourceUpdateQueue& aResources,
     const Maybe<IntRect>& aDirtyRect) {
   MOZ_ASSERT(aManager);
   MOZ_ASSERT(!aManager->IsDestroyed());
 
   // We iterate through all of the items to ensure we clean up the old
   // RenderRootStateManager references. Most of the time there will be few
@@ -161,23 +164,16 @@ SourceSurfaceSharedData* SharedSurfacesC
       return static_cast<SourceSurfaceSharedData*>(childSurface);
     }
     default:
       return nullptr;
   }
 }
 
 /* static */
-void SharedSurfacesChild::DestroySharedUserData(void* aClosure) {
-  MOZ_ASSERT(aClosure);
-  auto data = static_cast<SharedUserData*>(aClosure);
-  delete data;
-}
-
-/* static */
 nsresult SharedSurfacesChild::ShareInternal(SourceSurfaceSharedData* aSurface,
                                             SharedUserData** aUserData) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aSurface);
   MOZ_ASSERT(aUserData);
 
   CompositorManagerChild* manager = CompositorManagerChild::GetInstance();
   if (NS_WARN_IF(!manager || !manager->CanSend() || !gfxVars::UseWebRender())) {
@@ -187,18 +183,19 @@ nsresult SharedSurfacesChild::ShareInter
     // out. Better to copy into a fresh buffer later.
     aSurface->FinishedSharing();
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   SharedUserData* data =
       static_cast<SharedUserData*>(aSurface->GetUserData(&sSharedKey));
   if (!data) {
-    data = new SharedUserData(manager->GetNextExternalImageId());
-    aSurface->AddUserData(&sSharedKey, data, DestroySharedUserData);
+    data =
+        MakeAndAddRef<SharedUserData>(manager->GetNextExternalImageId()).take();
+    aSurface->AddUserData(&sSharedKey, data, SharedUserData::Destroy);
   } else if (!manager->OwnsExternalImageId(data->Id())) {
     // If the id isn't owned by us, that means the bridge was reinitialized, due
     // to the GPU process crashing. All previous mappings have been released.
     data->SetId(manager->GetNextExternalImageId());
   } else if (data->IsShared()) {
     // It has already been shared with the GPU process.
     *aUserData = data;
     return NS_OK;
--- a/gfx/layers/ipc/SharedSurfacesChild.h
+++ b/gfx/layers/ipc/SharedSurfacesChild.h
@@ -10,16 +10,17 @@
 #include <stdint.h>                            // for uint32_t, uint64_t
 #include "mozilla/Attributes.h"                // for override
 #include "mozilla/Maybe.h"                     // for Maybe
 #include "mozilla/RefPtr.h"                    // for already_AddRefed
 #include "mozilla/StaticPtr.h"                 // for StaticRefPtr
 #include "mozilla/gfx/UserData.h"              // for UserDataKey
 #include "mozilla/webrender/WebRenderTypes.h"  // for wr::ImageKey
 #include "nsTArray.h"                          // for AutoTArray
+#include "nsThreadUtils.h"                     // for Runnable
 #include "ImageTypes.h"                        // for ContainerProducerID
 
 namespace mozilla {
 namespace layers {
 class AnimationImageKeyData;
 }  // namespace layers
 }  // namespace mozilla
 
@@ -133,31 +134,31 @@ class SharedSurfacesChild {
   };
 
  private:
   SharedSurfacesChild() = delete;
   ~SharedSurfacesChild() = delete;
 
   friend class SharedSurfacesAnimation;
 
-  class SharedUserData final {
+  class SharedUserData final : public Runnable {
    public:
-    SharedUserData() : mShared(false) {}
-
-    explicit SharedUserData(const wr::ExternalImageId& aId)
-        : mId(aId), mShared(false) {}
-
-    ~SharedUserData();
+    explicit SharedUserData(const wr::ExternalImageId& aId);
+    virtual ~SharedUserData();
 
     SharedUserData(const SharedUserData& aOther) = delete;
     SharedUserData& operator=(const SharedUserData& aOther) = delete;
 
     SharedUserData(SharedUserData&& aOther) = delete;
     SharedUserData& operator=(SharedUserData&& aOther) = delete;
 
+    static void Destroy(void* aClosure);
+
+    NS_IMETHOD Run() override;
+
     const wr::ExternalImageId& Id() const { return mId; }
 
     void SetId(const wr::ExternalImageId& aId) {
       mId = aId;
       mKeys.Clear();
       mShared = false;
     }