Bug 1644208: Change RecordSourceSurfaceDestruction to take a void* not SourceSurface* to avoid AddRef during destructor. r=jrmuizel
authorBob Owen <bobowencode@gmail.com>
Tue, 09 Jun 2020 14:21:31 +0000
changeset 534696 d2081cd20b58ac7ae9a6239bee334fba4c911c3f
parent 534695 891bf9d395fd25e5dee20f67278a591d7dd47e84
child 534697 84668b82ebee017f19fee4ef7a367a054ef8951f
push id37493
push usernerli@mozilla.com
push dateWed, 10 Jun 2020 04:36:07 +0000
treeherdermozilla-central@b2df79a80c03 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1644208
milestone79.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 1644208: Change RecordSourceSurfaceDestruction to take a void* not SourceSurface* to avoid AddRef during destructor. r=jrmuizel Differential Revision: https://phabricator.services.mozilla.com/D78770
gfx/2d/DrawEventRecorder.cpp
gfx/2d/DrawEventRecorder.h
gfx/layers/CanvasDrawEventRecorder.cpp
gfx/layers/CanvasDrawEventRecorder.h
--- a/gfx/2d/DrawEventRecorder.cpp
+++ b/gfx/2d/DrawEventRecorder.cpp
@@ -43,19 +43,18 @@ void DrawEventRecorderPrivate::StoreSour
   }
 
   DataSourceSurface::ScopedMap map(dataSurf, DataSourceSurface::READ);
   RecordEvent(RecordedSourceSurfaceCreation(
       aSurface, map.GetData(), map.GetStride(), dataSurf->GetSize(),
       dataSurf->GetFormat()));
 }
 
-void DrawEventRecorderPrivate::RecordSourceSurfaceDestruction(
-    SourceSurface* aSurface) {
-  RemoveSourceSurface(aSurface);
+void DrawEventRecorderPrivate::RecordSourceSurfaceDestruction(void* aSurface) {
+  RemoveSourceSurface(static_cast<SourceSurface*>(aSurface));
   RemoveStoredObject(aSurface);
   RecordEvent(RecordedSourceSurfaceDestruction(ReferencePtr(aSurface)));
 }
 
 void DrawEventRecorderPrivate::DecrementUnscaledFontRefCount(
     const ReferencePtr aUnscaledFont) {
   auto element = mUnscaledFontRefs.find(aUnscaledFont);
   MOZ_DIAGNOSTIC_ASSERT(element != mUnscaledFontRefs.end(),
--- a/gfx/2d/DrawEventRecorder.h
+++ b/gfx/2d/DrawEventRecorder.h
@@ -123,17 +123,28 @@ class DrawEventRecorderPrivate : public 
 
   void TakeExternalSurfaces(std::vector<RefPtr<SourceSurface>>& aSurfaces) {
     aSurfaces = std::move(mExternalSurfaces);
   }
 
   virtual void StoreSourceSurfaceRecording(SourceSurface* aSurface,
                                            const char* aReason);
 
-  virtual void RecordSourceSurfaceDestruction(SourceSurface* aSurface);
+  /**
+   * This is virtual to allow subclasses to control the recording, if for
+   * example it needs to happen on a specific thread. aSurface is a void*
+   * instead of a SourceSurface* because this is called during the SourceSurface
+   * destructor, so it is partially destructed and should not be accessed. If we
+   * use a SourceSurface* we might, for example, accidentally AddRef/Release the
+   * object by passing it to NewRunnableMethod to submit to a different thread.
+   * We are only using the pointer as a lookup ID to our internal maps and
+   * ReferencePtr to be used on the translation side.
+   * @param aSurface the surface whose destruction is being recorded
+   */
+  virtual void RecordSourceSurfaceDestruction(void* aSurface);
 
   virtual void AddDependentSurface(uint64_t aDependencyId) {
     MOZ_CRASH("GFX: AddDependentSurface");
   }
 
  protected:
   void StoreExternalSurfaceRecording(SourceSurface* aSurface, uint64_t aKey);
 
--- a/gfx/layers/CanvasDrawEventRecorder.cpp
+++ b/gfx/layers/CanvasDrawEventRecorder.cpp
@@ -486,24 +486,23 @@ void CanvasEventRingBuffer::ReturnRead(c
     availableToRead = std::min(bufRemaining, (mRead->returnCount - readCount));
   }
 
   memcpy(aOut, mBuf + bufPos, aSize);
   readCount += aSize;
   mWrite->returnCount = readCount;
 }
 
-void CanvasDrawEventRecorder::RecordSourceSurfaceDestruction(
-    gfx::SourceSurface* aSurface) {
+void CanvasDrawEventRecorder::RecordSourceSurfaceDestruction(void* aSurface) {
   // We must only record things on the main thread and surfaces that have been
   // recorded can sometimes be destroyed off the main thread.
   if (NS_IsMainThread()) {
     DrawEventRecorderPrivate::RecordSourceSurfaceDestruction(aSurface);
     return;
   }
 
-  NS_DispatchToMainThread(NewRunnableMethod<gfx::SourceSurface*>(
+  NS_DispatchToMainThread(NewRunnableMethod<void*>(
       "DrawEventRecorderPrivate::RecordSourceSurfaceDestruction", this,
       &DrawEventRecorderPrivate::RecordSourceSurfaceDestruction, aSurface));
 }
 
 }  // namespace layers
 }  // namespace mozilla
--- a/gfx/layers/CanvasDrawEventRecorder.h
+++ b/gfx/layers/CanvasDrawEventRecorder.h
@@ -242,17 +242,17 @@ class CanvasDrawEventRecorder final : pu
   void RecordEvent(const gfx::RecordedEvent& aEvent) final {
     if (!mOutputStream.good()) {
       return;
     }
 
     aEvent.RecordToStream(mOutputStream);
   }
 
-  void RecordSourceSurfaceDestruction(gfx::SourceSurface* aSurface) final;
+  void RecordSourceSurfaceDestruction(void* aSurface) final;
 
   void Flush() final {}
 
   void ReturnRead(char* aOut, size_t aSize) {
     mOutputStream.ReturnRead(aOut, aSize);
   }
 
   uint32_t CreateCheckpoint() { return mOutputStream.CreateCheckpoint(); }