1827024 - Crash in [@ mozilla::layers::ImageBridgeChild::FlushAllImagesSync] on poison values

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/3a6d293c-2cf0-49fd-aff3-542da0230405

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libxul.so  mozilla::layers::ImageBridgeChild::FlushAllImagesSync  gfx/layers/ipc/ImageBridgeChild.cpp:383
1  libxul.so  mozilla::detail::runnable_args_base<  dom/media/webrtc/transport/runnable_utils.h:41
2  libxul.so  nsThread::ProcessNextEvent  xpcom/threads/nsThread.cpp:1219
2  libxul.so  NS_ProcessNextEvent  xpcom/threads/nsThreadUtils.cpp:477
3  libxul.so  mozilla::ipc::MessagePumpForNonMainThreads::Run  ipc/glue/MessagePump.cpp:300
4  libxul.so  MessageLoop::RunInternal  ipc/chromium/src/base/message_loop.cc:381
4  libxul.so  MessageLoop::RunHandler  ipc/chromium/src/base/message_loop.cc:374
4  libxul.so  MessageLoop::Run  ipc/chromium/src/base/message_loop.cc:356
5  libxul.so  nsThread::ThreadFunc  xpcom/threads/nsThread.cpp:384
6  libnss3.so  _pt_root  nsprpub/pr/src/pthreads/ptthread.c:201

17 crashes on this signature, on Android and Windows, in the last 6 months. None on 110, and those on 111 are all Android, for whatever that means. All of them are on poison values.

The crashes are on the line aClient->FlushAllImages();. aClient is passed in as a raw pointer. The caller, ImageBridgeChild::FlushAllImages, uses what I guess is a sync runnable and says "RefPtrs on arguments are not needed since this dispatches synchronously.", and looking at all threads you can see that ImageBridgeChild::FlushAllImages is still around on another thread in a few crashes I looked at. In fact, FlushAllImages seemed to be happening on a DOM worker thread, being called from OffscreenCanvasDisplayHelper::CommitFrameToCompositor in the crashes I looked at, so maybe this is offscreen canvas related?

I'm not sure how we get from one to another, but from SearchFox it looks like the caller of FlushAllImages is ImageContainer::ClearAllImages(), where the callsite looks like this:
imageBridge->FlushAllImages(mImageClient, this);

This means that aClient from above is mImageClient. I don't know if it is possible, but if anything happens to overwrite mImageClient between here and when we do all of our sync runnable shenanigans then I think the image client can get destroyed. Maybe it should get rooted here? It also looks like the two callers of ClearAllImages are on fields, so this in ClearAllImages() might have similar issues, so maybe those should get rooted on the stack instead of the heap.

Comment 1

[Andrew McCreight [:mccr8]]

FWIW, I looked at all 17 of the crashes from the last 6 months, and for all of them the caller of ImageContainer::ClearAllImages() on the other thread was OffscreenCanvasDisplayHelper::CommitFrameToCompositor.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

I don't fully understand how the lifetimes are failing here, but I'll take a deeper look since I broke it.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

mImageClient is not the safest pointer. We could example, replace it in ImageContainer::EnsureImageClient:
https://searchfox.org/mozilla-central/rev/23690c9281759b41eedf730d3dcb9ae04ccaddf8/gfx/layers/ImageContainer.cpp#166

which is called by ImageContainer::GetAsyncContainerHandle during display list building:
https://searchfox.org/mozilla-central/rev/23690c9281759b41eedf730d3dcb9ae04ccaddf8/gfx/layers/wr/WebRenderUserData.cpp#214

while we are accessing it on the DOM worker thread. In theory this could happen because the GPU process crashed?

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1827024.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9327681 [details]
Bug 1827024.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Most of the patch is adding thread annotations, and it will be obvious there are some gaps, but I believe it would require some study to see which ones have issues and how to trigger them.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Relatively easy, there are some things that got added since the ESR.
• How likely is this patch to cause regressions; how much testing does it need?: It is possible there may be some additional deadlocks, there are a lot of locks involved here, but the main lock is recursive at least.
• Is Android affected?: Yes

Comment 6

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9327681 [details]
Bug 1827024.

Approved to uplift and land

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Andrew, can you please land and request Beta approval on this patch? And it sounds like we'll need a different patch for ESR also?

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9327681 [details]
Bug 1827024.

Beta/Release Uplift Approval Request

• User impact if declined: Rare intermittent crash, sec lifetime issue
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Most of the changes are just annotations. The functional change is holding a lock a little longer to guarantee a lifetime in a multithreaded context.
• String changes made/needed:
• Is Android affected?: Yes

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

It is in the autoland queue right now.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/9a8764df77e0d2824daa87ce4147eb7e6671fd84
https://hg.mozilla.org/mozilla-central/rev/9a8764df77e0

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9327681 [details]
Bug 1827024.

Approved for 113.0b9.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e036a4dadc2

This will need a rebased patch for ESR uplift.

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Rebased for mozilla-esr, v1

ESR Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Low volume crash / sec issue
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: Most of the change is adding annotations to ensure we hold onto a lock, which is just a compile time change. The only user facing change is to extend the hold time on a lock to ensure an object doesn't get freed before we return a reference to it.
[String changes made/needed]: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9330970 [details]
Rebased for mozilla-esr, v1

commit cc1a8ceae4dc2517fe90f6f404bfd17d3fe9b6e9
Author: Andrew Osmond <[e-mail]>
Date:   Wed Apr 26 01:49:09 2023 +0000

    Bug 1827024. r=gfx-reviewers,lsalzman
    
    Differential Revision: https://phabricator.services.mozilla.com/D175005

diff --git a/gfx/layers/D3D11YCbCrImage.cpp b/gfx/layers/D3D11YCbCrImage.cpp
index b50c1f56ffb6d..c7f6eabd864a3 100644
--- a/gfx/layers/D3D11YCbCrImage.cpp
+++ b/gfx/layers/D3D11YCbCrImage.cpp
@@ -28,17 +28,17 @@ bool D3D11YCbCrImage::SetData(KnowsCompositor* aAllocator,
                               ImageContainer* aContainer,
                               const PlanarYCbCrData& aData) {
   mPictureRect = aData.mPictureRect;
   mColorDepth = aData.mColorDepth;
   mColorSpace = aData.mYUVColorSpace;
   mColorRange = aData.mColorRange;
   mChromaSubsampling = aData.mChromaSubsampling;
 
-  D3D11YCbCrRecycleAllocator* allocator =
+  RefPtr<D3D11YCbCrRecycleAllocator> allocator =
       aContainer->GetD3D11YCbCrRecycleAllocator(aAllocator);
   if (!allocator) {
     return false;
   }
 
   RefPtr<ID3D11Device> device = gfx::DeviceManagerDx::Get()->GetImageDevice();
   if (!device) {
     return false;
diff --git a/gfx/layers/ImageContainer.cpp b/gfx/layers/ImageContainer.cpp
index fae3e0940cd52..c6769a539b0a5 100644
--- a/gfx/layers/ImageContainer.cpp
+++ b/gfx/layers/ImageContainer.cpp
@@ -320,28 +320,32 @@ void ImageContainer::SetCurrentImages(const nsTArray<NonOwningImage>& aImages) {
             ImageBridgeChild::GetSingleton()) {
       imageBridge->UpdateImageClient(this);
     }
   }
   SetCurrentImageInternal(aImages);
 }
 
 void ImageContainer::ClearAllImages() {
+  mRecursiveMutex.Lock();
   if (mImageClient) {
+    RefPtr<ImageClient> imageClient = mImageClient;
+    mRecursiveMutex.Unlock();
+
     // Let ImageClient release all TextureClients. This doesn't return
     // until ImageBridge has called ClearCurrentImageFromImageBridge.
     if (RefPtr<ImageBridgeChild> imageBridge =
             ImageBridgeChild::GetSingleton()) {
-      imageBridge->FlushAllImages(mImageClient, this);
+      imageBridge->FlushAllImages(imageClient, this);
     }
     return;
   }
 
-  RecursiveMutexAutoLock lock(mRecursiveMutex);
   SetCurrentImageInternal(nsTArray<NonOwningImage>());
+  mRecursiveMutex.Unlock();
 }
 
 void ImageContainer::ClearCachedResources() {
   RecursiveMutexAutoLock lock(mRecursiveMutex);
   if (mImageClient && mImageClient->AsImageClientSingle()) {
     if (!mImageClient->HasTextureClientRecycler()) {
       return;
     }
@@ -355,29 +359,29 @@ void ImageContainer::SetCurrentImageInTransaction(Image* aImage) {
   AutoTArray<NonOwningImage, 1> images;
   images.AppendElement(NonOwningImage(aImage));
   SetCurrentImagesInTransaction(images);
 }
 
 void ImageContainer::SetCurrentImagesInTransaction(
     const nsTArray<NonOwningImage>& aImages) {
   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
-  NS_ASSERTION(!mImageClient,
+  NS_ASSERTION(!HasImageClient(),
                "Should use async image transfer with ImageBridge.");
 
   SetCurrentImageInternal(aImages);
 }
 
 bool ImageContainer::IsAsync() const { return mIsAsync; }
 
 CompositableHandle ImageContainer::GetAsyncContainerHandle() {
   NS_ASSERTION(IsAsync(),
                "Shared image ID is only relevant to async ImageContainers");
-  NS_ASSERTION(mAsyncContainerHandle, "Should have a shared image ID");
   RecursiveMutexAutoLock mon(mRecursiveMutex);
+  NS_ASSERTION(mAsyncContainerHandle, "Should have a shared image ID");
   EnsureImageClient();
   return mAsyncContainerHandle;
 }
 
 bool ImageContainer::HasCurrentImage() {
   RecursiveMutexAutoLock lock(mRecursiveMutex);
 
   return !mCurrentImages.IsEmpty();
@@ -428,19 +432,21 @@ void ImageContainer::NotifyComposite(
 
 void ImageContainer::NotifyDropped(uint32_t aDropped) {
   mDroppedImageCount += aDropped;
 }
 
 void ImageContainer::EnsureRecycleAllocatorForRDD(
     KnowsCompositor* aKnowsCompositor) {
   MOZ_ASSERT(!mIsAsync);
-  MOZ_ASSERT(!mImageClient);
   MOZ_ASSERT(XRE_IsRDDProcess());
 
+  RecursiveMutexAutoLock lock(mRecursiveMutex);
+  MOZ_ASSERT(!mImageClient);
+
   if (mRecycleAllocator &&
       aKnowsCompositor == mRecycleAllocator->GetKnowsCompositor()) {
     return;
   }
 
   if (!StaticPrefs::layers_recycle_allocator_rdd_AtStartup()) {
     return;
   }
@@ -448,42 +454,45 @@ void ImageContainer::EnsureRecycleAllocatorForRDD(
   static const uint32_t MAX_POOLED_VIDEO_COUNT = 5;
 
   mRecycleAllocator =
       new layers::TextureClientRecycleAllocator(aKnowsCompositor);
   mRecycleAllocator->SetMaxPoolSize(MAX_POOLED_VIDEO_COUNT);
 }
 
 #ifdef XP_WIN
-D3D11YCbCrRecycleAllocator* ImageContainer::GetD3D11YCbCrRecycleAllocator(
+already_AddRefed<D3D11YCbCrRecycleAllocator>
+ImageContainer::GetD3D11YCbCrRecycleAllocator(
     KnowsCompositor* aKnowsCompositor) {
-  if (mD3D11YCbCrRecycleAllocator &&
-      aKnowsCompositor == mD3D11YCbCrRecycleAllocator->GetKnowsCompositor()) {
-    return mD3D11YCbCrRecycleAllocator;
-  }
-
   if (!aKnowsCompositor->SupportsD3D11() ||
       !gfx::DeviceManagerDx::Get()->GetImageDevice()) {
     return nullptr;
   }
 
+  RecursiveMutexAutoLock lock(mRecursiveMutex);
+  if (mD3D11YCbCrRecycleAllocator &&
+      aKnowsCompositor == mD3D11YCbCrRecycleAllocator->GetKnowsCompositor()) {
+    return do_AddRef(mD3D11YCbCrRecycleAllocator);
+  }
+
   mD3D11YCbCrRecycleAllocator =
       new D3D11YCbCrRecycleAllocator(aKnowsCompositor);
-  return mD3D11YCbCrRecycleAllocator;
+  return do_AddRef(mD3D11YCbCrRecycleAllocator);
 }
 #endif
 
 #ifdef XP_MACOSX
-MacIOSurfaceRecycleAllocator*
+already_AddRefed<MacIOSurfaceRecycleAllocator>
 ImageContainer::GetMacIOSurfaceRecycleAllocator() {
+  RecursiveMutexAutoLock lock(mRecursiveMutex);
   if (!mMacIOSurfaceRecycleAllocator) {
     mMacIOSurfaceRecycleAllocator = new MacIOSurfaceRecycleAllocator();
   }
 
-  return mMacIOSurfaceRecycleAllocator;
+  return do_AddRef(mMacIOSurfaceRecycleAllocator);
 }
 #endif
 
 // -
 // https://searchfox.org/mozilla-central/source/dom/media/ipc/RemoteImageHolder.cpp#46
 
 Maybe<PlanarYCbCrData> PlanarYCbCrData::From(
     const SurfaceDescriptorBuffer& sdb) {
diff --git a/gfx/layers/ImageContainer.h b/gfx/layers/ImageContainer.h
index 298ffea1b4ade..22f8b95bc4b6d 100644
--- a/gfx/layers/ImageContainer.h
+++ b/gfx/layers/ImageContainer.h
@@ -19,16 +19,17 @@
 #include "mozilla/gfx/Rect.h"
 #include "mozilla/gfx/Types.h"           // For ColorDepth
 #include "mozilla/layers/LayersTypes.h"  // for LayersBackend, etc
 #include "mozilla/layers/CompositorTypes.h"
 #include "mozilla/mozalloc.h"  // for operator delete, etc
 #include "nsDebug.h"           // for NS_ASSERTION
 #include "nsISupportsImpl.h"   // for Image::Release, etc
 #include "nsTArray.h"          // for nsTArray
+#include "nsThreadUtils.h"     // for NS_IsMainThread
 #include "mozilla/Atomics.h"
 #include "mozilla/gfx/2D.h"
 #include "mozilla/EnumeratedArray.h"
 #include "mozilla/UniquePtr.h"
 #include "MediaInfo.h"
 #include "nsTHashMap.h"
 
 #ifdef XP_WIN
@@ -192,23 +193,23 @@ class BufferRecycleBin final {
  private:
   typedef mozilla::Mutex Mutex;
 
   // Private destructor, to discourage deletion outside of Release():
   ~BufferRecycleBin() = default;
 
   // This protects mRecycledBuffers, mRecycledBufferSize, mRecycledTextures
   // and mRecycledTextureSizes
-  Mutex mLock MOZ_UNANNOTATED;
+  Mutex mLock;
 
   // We should probably do something to prune this list on a timer so we don't
   // eat excess memory while video is paused...
-  nsTArray<mozilla::UniquePtr<uint8_t[]>> mRecycledBuffers;
+  nsTArray<mozilla::UniquePtr<uint8_t[]>> mRecycledBuffers GUARDED_BY(mLock);
   // This is only valid if mRecycledBuffers is non-empty
-  uint32_t mRecycledBufferSize;
+  uint32_t mRecycledBufferSize GUARDED_BY(mLock);
 };
 
 /**
  * A class that manages Image creation for a LayerManager. The only reason
  * we need a separate class here is that LayerManagers aren't threadsafe
  * (because layers can only be used on the main thread) and we want to
  * be able to create images from any thread, to facilitate video playback
  * without involving the main thread, for example.
@@ -246,18 +247,18 @@ class ImageContainerListener final {
   void ClearImageContainer();
   void DropImageClient();
 
  private:
   typedef mozilla::Mutex Mutex;
 
   ~ImageContainerListener();
 
-  Mutex mLock MOZ_UNANNOTATED;
-  ImageContainer* mImageContainer;
+  Mutex mLock;
+  ImageContainer* mImageContainer GUARDED_BY(mLock);
 };
 
 /**
  * A class that manages Images for an ImageLayer. The only reason
  * we need a separate class here is that ImageLayers aren't threadsafe
  * (because layers can only be used on the main thread) and we want to
  * be able to set the current Image from any thread, to facilitate
  * video playback without involving the main thread, for example.
@@ -437,46 +438,66 @@ class ImageContainer final : public SupportsThreadSafeWeakPtr<ImageContainer> {
 
   /**
    * Sets a size that the image is expected to be rendered at.
    * This is a hint for image backends to optimize scaling.
    * Default implementation in this class is to ignore the hint.
    * Can be called on any thread. This method takes mRecursiveMutex
    * when accessing thread-shared state.
    */
-  void SetScaleHint(const gfx::IntSize& aScaleHint) { mScaleHint = aScaleHint; }
+  void SetScaleHint(const gfx::IntSize& aScaleHint) {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
+    mScaleHint = aScaleHint;
+  }
 
-  const gfx::IntSize& GetScaleHint() const { return mScaleHint; }
+  const gfx::IntSize GetScaleHint() const {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
+    return mScaleHint;
+  }
 
   void SetTransformHint(const gfx::Matrix& aTransformHint) {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
     mTransformHint = aTransformHint;
   }
 
-  const gfx::Matrix& GetTransformHint() const { return mTransformHint; }
+  const gfx::Matrix GetTransformHint() const {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
+    return mTransformHint;
+  }
 
-  void SetRotation(VideoInfo::Rotation aRotation) { mRotation = aRotation; }
+  void SetRotation(VideoInfo::Rotation aRotation) {
+    MOZ_ASSERT(NS_IsMainThread());
+    mRotation = aRotation;
+  }
 
-  VideoInfo::Rotation GetRotation() const { return mRotation; }
+  VideoInfo::Rotation GetRotation() const {
+    MOZ_ASSERT(NS_IsMainThread());
+    return mRotation;
+  }
 
   void SetImageFactory(ImageFactory* aFactory) {
     RecursiveMutexAutoLock lock(mRecursiveMutex);
     mImageFactory = aFactory ? aFactory : new ImageFactory();
   }
 
-  ImageFactory* GetImageFactory() const { return mImageFactory; }
+  already_AddRefed<ImageFactory> GetImageFactory() const {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
+    return do_AddRef(mImageFactory);
+  }
 
   void EnsureRecycleAllocatorForRDD(KnowsCompositor* aKnowsCompositor);
 
 #ifdef XP_WIN
-  D3D11YCbCrRecycleAllocator* GetD3D11YCbCrRecycleAllocator(
+  already_AddRefed<D3D11YCbCrRecycleAllocator> GetD3D11YCbCrRecycleAllocator(
       KnowsCompositor* aKnowsCompositor);
 #endif
 
 #ifdef XP_MACOSX
-  MacIOSurfaceRecycleAllocator* GetMacIOSurfaceRecycleAllocator();
+  already_AddRefed<MacIOSurfaceRecycleAllocator>
+  GetMacIOSurfaceRecycleAllocator();
 #endif
 
   /**
    * Returns the delay between the last composited image's presentation
    * timestamp and when it was first composited. It's possible for the delay
    * to be negative if the first image in the list passed to SetCurrentImages
    * has a presentation timestamp greater than "now".
    * Returns 0 if the composited image had a null timestamp, or if no
@@ -504,18 +525,19 @@ class ImageContainer final : public SupportsThreadSafeWeakPtr<ImageContainer> {
    * frameID is not the same as the entry's.
    * Every expired image that is never composited is counted as dropped.
    */
   uint32_t GetDroppedImageCount() { return mDroppedImageCount; }
 
   void NotifyComposite(const ImageCompositeNotification& aNotification);
   void NotifyDropped(uint32_t aDropped);
 
-  ImageContainerListener* GetImageContainerListener() {
-    return mNotifyCompositeListener;
+  already_AddRefed<ImageContainerListener> GetImageContainerListener() {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
+    return do_AddRef(mNotifyCompositeListener);
   }
 
   /**
    * Get the ImageClient associated with this container. Returns only after
    * validating, and it will recreate the image client if that fails.
    * Returns nullptr if not applicable.
    */
   already_AddRefed<ImageClient> GetImageClient();
@@ -533,76 +555,86 @@ class ImageContainer final : public SupportsThreadSafeWeakPtr<ImageContainer> {
   void SetCurrentImageInternal(const nsTArray<NonOwningImage>& aImages);
 
   // This is called to ensure we have an active image, this may not be true
   // when we're storing image information in a RemoteImageData structure.
   // NOTE: If we have remote data mRemoteDataMutex should be locked when
   // calling this function!
   void EnsureActiveImage();
 
-  void EnsureImageClient();
+  void EnsureImageClient() REQUIRES(mRecursiveMutex);
+
+  bool HasImageClient() const {
+    RecursiveMutexAutoLock lock(mRecursiveMutex);
+    return !!mImageClient;
+  }
 
   // RecursiveMutex to protect thread safe access to the "current
   // image", and any other state which is shared between threads.
-  RecursiveMutex mRecursiveMutex MOZ_UNANNOTATED;
+  mutable RecursiveMutex mRecursiveMutex;
 
-  RefPtr<TextureClientRecycleAllocator> mRecycleAllocator;
+  RefPtr<TextureClientRecycleAllocator> mRecycleAllocator
+      GUARDED_BY(mRecursiveMutex);
 
 #ifdef XP_WIN
-  RefPtr<D3D11YCbCrRecycleAllocator> mD3D11YCbCrRecycleAllocator;
+  RefPtr<D3D11YCbCrRecycleAllocator> mD3D11YCbCrRecycleAllocator
+      GUARDED_BY(mRecursiveMutex);
 #endif
 #ifdef XP_MACOSX
-  RefPtr<MacIOSurfaceRecycleAllocator> mMacIOSurfaceRecycleAllocator;
+  RefPtr<MacIOSurfaceRecycleAllocator> mMacIOSurfaceRecycleAllocator
+      GUARDED_BY(mRecursiveMutex);
 #endif
 
-  nsTArray<OwningImage> mCurrentImages;
+  nsTArray<OwningImage> mCurrentImages GUARDED_BY(mRecursiveMutex);
 
   // Updates every time mActiveImage changes
-  uint32_t mGenerationCounter;
+  uint32_t mGenerationCounter GUARDED_BY(mRecursiveMutex);
 
   // Number of contained images that have been painted at least once.  It's up
   // to the ImageContainer implementation to ensure accesses to this are
   // threadsafe.
-  uint32_t mPaintCount;
+  uint32_t mPaintCount GUARDED_BY(mRecursiveMutex);
 
   // See GetPaintDelay. Accessed only with mRecursiveMutex held.
-  TimeDuration mPaintDelay;
+  TimeDuration mPaintDelay GUARDED_BY(mRecursiveMutex);
 
   // See GetDroppedImageCount.
   mozilla::Atomic<uint32_t> mDroppedImageCount;
 
   // This is the image factory used by this container, layer managers using
   // this container can set an alternative image factory that will be used to
   // create images for this container.
-  RefPtr<ImageFactory> mImageFactory;
+  RefPtr<ImageFactory> mImageFactory GUARDED_BY(mRecursiveMutex);
 
-  gfx::IntSize mScaleHint;
+  gfx::IntSize mScaleHint GUARDED_BY(mRecursiveMutex);
 
-  gfx::Matrix mTransformHint;
+  gfx::Matrix mTransformHint GUARDED_BY(mRecursiveMutex);
 
+  // Main thread only.
   VideoInfo::Rotation mRotation = VideoInfo::Rotation::kDegree_0;
 
-  RefPtr<BufferRecycleBin> mRecycleBin;
+  RefPtr<BufferRecycleBin> mRecycleBin GUARDED_BY(mRecursiveMutex);
 
   // This member points to an ImageClient if this ImageContainer was
   // sucessfully created with ENABLE_ASYNC, or points to null otherwise.
   // 'unsuccessful' in this case only means that the ImageClient could not
   // be created, most likely because off-main-thread compositing is not enabled.
   // In this case the ImageContainer is perfectly usable, but it will forward
   // frames to the compositor through transactions in the main thread rather
   // than asynchronusly using the ImageBridge IPDL protocol.
-  RefPtr<ImageClient> mImageClient;
+  RefPtr<ImageClient> mImageClient GUARDED_BY(mRecursiveMutex);
 
-  bool mIsAsync;
-  CompositableHandle mAsyncContainerHandle;
+  const bool mIsAsync;
+  CompositableHandle mAsyncContainerHandle GUARDED_BY(mRecursiveMutex);
 
   // ProducerID for last current image(s)
-  ProducerID mCurrentProducerID;
+  ProducerID mCurrentProducerID GUARDED_BY(mRecursiveMutex);
 
-  RefPtr<ImageContainerListener> mNotifyCompositeListener;
+  RefPtr<ImageContainerListener> mNotifyCompositeListener
+      GUARDED_BY(mRecursiveMutex);
 
   static mozilla::Atomic<uint32_t> sGenerationCounter;
 };
 
 class AutoLockImage {
  public:
   explicit AutoLockImage(ImageContainer* aContainer) {
     aContainer->GetCurrentImages(&mImages);

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Rebased for mozilla-esr, v2

Wrong file.

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9330971 [details] [diff] [review]
Rebased for mozilla-esr, v2

ESR Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Low volume crash / sec issue
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: Most of the change is adding annotations to ensure we hold onto a lock, which is just a compile time change. The only user facing change is to extend the hold time on a lock to ensure an object doesn't get freed before we return a reference to it.
[String changes made/needed]: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9330971 [details] [diff] [review]
Rebased for mozilla-esr, v2

Approved for 102.11esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/350c74fdc5e2