Bug 1885762 - Ensure DrawTargetWebgl shmem snapshots get copied before invalidation. r=aosmond

There is a corner case where if DrawTargetWebgl's shmem framebuffer is used as a snapshot that
happens to outlive RecordedTextureData::ReturnSnapshot, and then RecordedTextureData::BorrowDrawTarget
is used to overwrite the framebuffer, then the snapshot data may be invalid when it is read.

To work around this, we need to first track whether the wrapped snapshot is pointing to a
DTWebgl shmem. Then when BorrowDrawTarget attempts to begin modifying the framebuffer,
we copy the snapshot's internal data to avoid the data being overwritten. Subsequent attempts
to read the snapshot can then just read from the copied data safely.

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

gfx/layers/client/TextureRecorded.cpp

@@ -33,7 +33,7 @@ RecordedTextureData::~RecordedTextureData() {
   // We need the translator to drop its reference for the DrawTarget first,
   // because the TextureData might need to destroy its DrawTarget within a lock.
   mSnapshot = nullptr;
-  mSnapshotWrapper = nullptr;
+  DetachSnapshotWrapper();
   mDT = nullptr;
   mCanvasChild->CleanupTexture(mTextureId);
   mCanvasChild->RecordEvent(RecordedTextureDestruction(
@@ -92,10 +92,25 @@ bool RecordedTextureData::Lock(OpenMode aMode) {
   return true;
 }
 
+void RecordedTextureData::DetachSnapshotWrapper(bool aInvalidate,
+                                                bool aRelease) {
+  if (mSnapshotWrapper) {
+    // If the snapshot only has one ref, then we don't need to worry about
+    // copying before invalidation since it is about to be deleted. Otherwise,
+    // we need to ensure any internal data is appropriately copied before
+    // shmems are potentially overwritten if there are still existing users.
+    mCanvasChild->DetachSurface(mSnapshotWrapper,
+                                aInvalidate && !mSnapshotWrapper->hasOneRef());
+    if (aRelease) {
+      mSnapshotWrapper = nullptr;
+    }
+  }
+}
+
 void RecordedTextureData::Unlock() {
   if ((mLockedMode == OpenMode::OPEN_READ_WRITE) &&
       mCanvasChild->ShouldCacheDataSurface()) {
-    mSnapshotWrapper = nullptr;
+    DetachSnapshotWrapper();
     mSnapshot = mDT->Snapshot();
     mDT->DetachAllSnapshots();
     mCanvasChild->RecordEvent(RecordedCacheDataSurface(mSnapshot.get()));
@@ -108,11 +123,9 @@ void RecordedTextureData::Unlock() {
 
 already_AddRefed<gfx::DrawTarget> RecordedTextureData::BorrowDrawTarget() {
   if (mLockedMode & OpenMode::OPEN_WRITE) {
+    // The snapshot will be invalidated.
     mSnapshot = nullptr;
-    if (mSnapshotWrapper) {
-      mCanvasChild->DetachSurface(mSnapshotWrapper);
-      mSnapshotWrapper = nullptr;
-    }
+    DetachSnapshotWrapper(true);
   }
   return do_AddRef(mDT);
 }
@@ -122,18 +135,22 @@ void RecordedTextureData::EndDraw() {
   MOZ_ASSERT(mLockedMode == OpenMode::OPEN_READ_WRITE);
 
   if (mCanvasChild->ShouldCacheDataSurface()) {
-    mSnapshotWrapper = nullptr;
+    DetachSnapshotWrapper();
     mSnapshot = mDT->Snapshot();
     mCanvasChild->RecordEvent(RecordedCacheDataSurface(mSnapshot.get()));
   }
 }
 
 already_AddRefed<gfx::SourceSurface> RecordedTextureData::BorrowSnapshot() {
-  if (mSnapshotWrapper && (!mDT || !mDT->IsDirty())) {
-    // The DT is unmodified since the last time snapshot was borrowed, so it
-    // is safe to reattach the snapshot for shmem readbacks.
-    mCanvasChild->AttachSurface(mSnapshotWrapper);
-    return do_AddRef(mSnapshotWrapper);
+  if (mSnapshotWrapper) {
+    if (!mDT || !mDT->IsDirty()) {
+      // The DT is unmodified since the last time snapshot was borrowed, so it
+      // is safe to reattach the snapshot for shmem readbacks.
+      mCanvasChild->AttachSurface(mSnapshotWrapper);
+      return do_AddRef(mSnapshotWrapper);
+    }
+
+    DetachSnapshotWrapper();
   }
 
   // There are some failure scenarios where we have no DrawTarget and
@@ -153,9 +170,10 @@ already_AddRefed<gfx::SourceSurface> RecordedTextureData::BorrowSnapshot() {
 void RecordedTextureData::ReturnSnapshot(
     already_AddRefed<gfx::SourceSurface> aSnapshot) {
   RefPtr<gfx::SourceSurface> snapshot = aSnapshot;
-  if (mSnapshotWrapper) {
-    mCanvasChild->DetachSurface(mSnapshotWrapper);
-  }
+  // The snapshot needs to be marked detached but we keep the wrapper around
+  // so that it can be reused without repeatedly creating it and accidentally
+  // reading back data for each new instantiation.
+  DetachSnapshotWrapper(false, false);
 }
 
 void RecordedTextureData::Deallocate(LayersIPCChannel* aAllocator) {}

gfx/layers/client/TextureRecorded.h

@@ -58,6 +58,8 @@ class RecordedTextureData final : public TextureData {
 
   ~RecordedTextureData() override;
 
+  void DetachSnapshotWrapper(bool aInvalidate = false, bool aRelease = true);
+
   int64_t mTextureId;
   RefPtr<CanvasChild> mCanvasChild;
   gfx::IntSize mSize;

gfx/layers/ipc/CanvasChild.cpp

@@ -130,6 +130,16 @@ class SourceSurfaceCanvasRecording final : public gfx::SourceSurface {
   void AttachSurface() { mDetached = false; }
   void DetachSurface() { mDetached = true; }
 
+  void InvalidateDataSurface() {
+    if (mDataSourceSurface && mMayInvalidate) {
+      // This must be the only reference to the data left.
+      MOZ_ASSERT(mDataSourceSurface->hasOneRef());
+      mDataSourceSurface =
+          gfx::Factory::CopyDataSourceSurface(mDataSourceSurface);
+      mMayInvalidate = false;
+    }
+  }
+
   already_AddRefed<gfx::SourceSurface> ExtractSubrect(
       const gfx::IntRect& aRect) final {
     return mRecordedSurface->ExtractSubrect(aRect);
@@ -139,8 +149,8 @@ class SourceSurfaceCanvasRecording final : public gfx::SourceSurface {
   void EnsureDataSurfaceOnMainThread() {
     // The data can only be retrieved on the main thread.
     if (!mDataSourceSurface && NS_IsMainThread()) {
-      mDataSourceSurface =
-          mCanvasChild->GetDataSurface(mTextureId, mRecordedSurface, mDetached);
+      mDataSourceSurface = mCanvasChild->GetDataSurface(
+          mTextureId, mRecordedSurface, mDetached, mMayInvalidate);
     }
   }
 
@@ -164,6 +174,7 @@ class SourceSurfaceCanvasRecording final : public gfx::SourceSurface {
   RefPtr<CanvasDrawEventRecorder> mRecorder;
   RefPtr<gfx::DataSourceSurface> mDataSourceSurface;
   bool mDetached = false;
+  bool mMayInvalidate = false;
 };
 
 CanvasChild::CanvasChild(dom::ThreadSafeWorkerRef* aWorkerRef)
@@ -386,7 +397,8 @@ int64_t CanvasChild::CreateCheckpoint() {
 }
 
 already_AddRefed<gfx::DataSourceSurface> CanvasChild::GetDataSurface(
-    int64_t aTextureId, const gfx::SourceSurface* aSurface, bool aDetached) {
+    int64_t aTextureId, const gfx::SourceSurface* aSurface, bool aDetached,
+    bool& aMayInvalidate) {
   NS_ASSERT_OWNINGTHREAD(CanvasChild);
   MOZ_ASSERT(aSurface);
 
@@ -422,6 +434,7 @@ already_AddRefed<gfx::DataSourceSurface> CanvasChild::GetDataSurface(
       RefPtr<gfx::DataSourceSurface> dataSurface =
           gfx::Factory::CreateWrappingDataSourceSurface(shmemPtr, stride, size,
                                                         format);
+      aMayInvalidate = true;
       return dataSurface.forget();
     }
   }
@@ -453,6 +466,7 @@ already_AddRefed<gfx::DataSourceSurface> CanvasChild::GetDataSurface(
   RefPtr<gfx::DataSourceSurface> dataSurface =
       gfx::Factory::CreateWrappingDataSourceSurface(
           data, stride, ssSize, ssFormat, ReleaseDataShmemHolder, closure);
+  aMayInvalidate = false;
   return dataSurface.forget();
 }
 
@@ -498,10 +512,14 @@ void CanvasChild::AttachSurface(const RefPtr<gfx::SourceSurface>& aSurface) {
   }
 }
 
-void CanvasChild::DetachSurface(const RefPtr<gfx::SourceSurface>& aSurface) {
+void CanvasChild::DetachSurface(const RefPtr<gfx::SourceSurface>& aSurface,
+                                bool aInvalidate) {
   if (auto* surface =
           static_cast<SourceSurfaceCanvasRecording*>(aSurface.get())) {
     surface->DetachSurface();
+    if (aInvalidate) {
+      surface->InvalidateDataSurface();
+    }
   }
 }
 

gfx/layers/ipc/CanvasChild.h

@@ -22,7 +22,7 @@ class ThreadSafeWorkerRef;
 namespace gfx {
 class DrawTargetRecording;
 class SourceSurface;
-}
+}  // namespace gfx
 
 namespace layers {
 class CanvasDrawEventRecorder;
@@ -132,7 +132,8 @@ class CanvasChild final : public PCanvasChild, public SupportsWeakPtr {
   /**
    * The DrawTargetRecording is about to change, so detach the old snapshot.
    */
-  void DetachSurface(const RefPtr<gfx::SourceSurface>& aSurface);
+  void DetachSurface(const RefPtr<gfx::SourceSurface>& aSurface,
+                     bool aInvalidate = false);
 
   /**
    * Get DataSourceSurface from the translated equivalent version of aSurface in
@@ -141,11 +142,13 @@ class CanvasChild final : public PCanvasChild, public SupportsWeakPtr {
    * @param aSurface the SourceSurface in this process for which we need a
    *                 DataSourceSurface
    * @param aDetached whether the surface is old
+   * @param aMayInvalidate whether the data may be invalidated by future changes
    * @returns a DataSourceSurface created from data for aSurface retrieve from
    *          GPU process
    */
   already_AddRefed<gfx::DataSourceSurface> GetDataSurface(
-      int64_t aTextureId, const gfx::SourceSurface* aSurface, bool aDetached);
+      int64_t aTextureId, const gfx::SourceSurface* aSurface, bool aDetached,
+      bool& aMayInvalidate);
 
   bool RequiresRefresh(int64_t aTextureId) const;