Bug 1691861 - Make the mHelper field of IconLoader a non-owning reference, and make sure it's always cleared with a call to Destroy before the helper goes away. r=mconley
authorMarkus Stange <mstange.moz@gmail.com>
Fri, 12 Feb 2021 00:32:01 +0000
changeset 3537632 ae78f34ef0c393503a0af0eb82e4b4312bc50d42
parent 3537631 b26ff99675eb6c025dd40ff0cc17781fb6c0563d
child 3537633 7446edbd382c732f8f88289446b604a6cad624d2
push id654313
push userwptsync@mozilla.com
push dateFri, 12 Feb 2021 09:09:29 +0000
treeherdertry@8a032d4accce [default view] [failures only]
reviewersmconley
bugs1691861
milestone87.0a1
Bug 1691861 - Make the mHelper field of IconLoader a non-owning reference, and make sure it's always cleared with a call to Destroy before the helper goes away. r=mconley This breaks another reference cycle. Differential Revision: https://phabricator.services.mozilla.com/D104621
widget/IconLoader.cpp
widget/IconLoader.h
widget/cocoa/nsMenuItemIconX.mm
widget/cocoa/nsTouchBarInputIcon.mm
--- a/widget/IconLoader.cpp
+++ b/widget/IconLoader.cpp
@@ -11,17 +11,17 @@
 #include "mozilla/dom/Document.h"
 #include "nsContentUtils.h"
 #include "nsIContent.h"
 
 using namespace mozilla;
 
 namespace mozilla::widget {
 
-NS_IMPL_CYCLE_COLLECTION(mozilla::widget::IconLoader, mHelper)
+NS_IMPL_CYCLE_COLLECTION(mozilla::widget::IconLoader)
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(mozilla::widget::IconLoader)
   NS_INTERFACE_MAP_ENTRY(imgINotificationObserver)
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
 NS_IMPL_CYCLE_COLLECTING_ADDREF(mozilla::widget::IconLoader)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(mozilla::widget::IconLoader)
 
 IconLoader::IconLoader(Helper* aHelper, const nsIntRect& aImageRegionRect)
@@ -32,19 +32,17 @@ IconLoader::IconLoader(Helper* aHelper, 
 
 IconLoader::~IconLoader() { Destroy(); }
 
 void IconLoader::Destroy() {
   if (mIconRequest) {
     mIconRequest->CancelAndForgetObserver(NS_BINDING_ABORTED);
     mIconRequest = nullptr;
   }
-  if (mHelper) {
-    mHelper = nullptr;
-  }
+  mHelper = nullptr;
 }
 
 nsresult IconLoader::LoadIcon(nsIURI* aIconURI, nsINode* aNode,
                               bool aIsInternalIcon) {
   if (mIconRequest) {
     // Another icon request is already in flight.  Kill it.
     mIconRequest->Cancel(NS_BINDING_ABORTED);
     mIconRequest = nullptr;
@@ -123,17 +121,19 @@ void IconLoader::Notify(imgIRequest* aRe
     if (NS_FAILED(rv)) {
       return;
     }
 
     nsCOMPtr<imgIContainer> image;
     aRequest->GetImage(getter_AddRefs(image));
     MOZ_ASSERT(image);
 
-    mHelper->OnComplete(image, mImageRegionRect);
+    if (mHelper) {
+      mHelper->OnComplete(image, mImageRegionRect);
+    }
     return;
   }
 
   if (aType == imgINotificationObserver::DECODE_COMPLETE) {
     if (mIconRequest && mIconRequest == aRequest) {
       mIconRequest->Cancel(NS_BINDING_ABORTED);
       mIconRequest = nullptr;
     }
--- a/widget/IconLoader.h
+++ b/widget/IconLoader.h
@@ -40,16 +40,20 @@ class IconLoader : public imgINotificati
     // participate in cycle collection
     virtual nsresult OnComplete(imgIContainer* aContainer,
                                 const nsIntRect& aRect) = 0;
 
    protected:
     virtual ~Helper() = default;
   };
 
+  // Create the loader.
+  // aHelper will be notified when the load is complete.
+  // The loader does not keep an owning reference to the helper. Call Destroy
+  // before the helper goes away.
   IconLoader(Helper* aHelper, const nsIntRect& aImageRegionRect);
 
  public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_IMGINOTIFICATIONOBSERVER
   NS_DECL_CYCLE_COLLECTION_CLASS(IconLoader)
 
   // LoadIcon will start a load request for the icon.
@@ -66,13 +70,18 @@ class IconLoader : public imgINotificati
 
  private:
   nsresult OnFrameComplete(imgIRequest* aRequest);
 
   nsContentPolicyType mContentType;
   RefPtr<imgRequestProxy> mIconRequest;
   nsIntRect mImageRegionRect;
   bool mLoadedIcon;
-  RefPtr<Helper> mHelper;
+
+  // The helper, which is notified when loading completes.
+  // Can be null, after a call to Destroy.
+  // This is a non-owning reference and needs to be cleared with a call to
+  // Destroy before the helper goes away.
+  Helper* mHelper;
 };
 
 }  // namespace mozilla::widget
 #endif  // mozilla_widget_IconLoader_h_
--- a/widget/cocoa/nsMenuItemIconX.mm
+++ b/widget/cocoa/nsMenuItemIconX.mm
@@ -56,19 +56,21 @@ nsMenuItemIconX::~nsMenuItemIconX() {
   MOZ_COUNT_DTOR(nsMenuItemIconX);
 }
 
 // Called from mMenuObjectX's destructor, to prevent us from outliving it
 // (as might otherwise happen if calls to our imgINotificationObserver methods
 // are still outstanding).  mMenuObjectX owns our mNativeMenuItem.
 void nsMenuItemIconX::Destroy() {
   if (mIconLoader) {
+    mIconLoader->Destroy();
     mIconLoader = nullptr;
   }
   if (mIconLoaderHelper) {
+    mIconLoaderHelper->Destroy();
     mIconLoaderHelper = nullptr;
   }
   mMenuObject = nullptr;
   mNativeMenuItem = nil;
 }
 
 nsresult nsMenuItemIconX::SetupIcon() {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
@@ -213,24 +215,26 @@ nsresult nsMenuItemIconX::GetIconURI(nsI
 nsresult nsMenuItemIconX::OnComplete() {
   if (!mIconLoaderHelper) {
     return NS_ERROR_FAILURE;
   }
 
   NSImage* image = mIconLoaderHelper->GetNativeIconImage();
   if (!mNativeMenuItem) {
     mIconLoaderHelper->Destroy();
+    mIconLoader->Destroy();
     return NS_ERROR_FAILURE;
   }
 
   if (!image) {
     [mNativeMenuItem setImage:nil];
     return NS_OK;
   }
 
   [mNativeMenuItem setImage:image];
   if (mMenuObject) {
     mMenuObject->IconUpdated();
   }
 
   mIconLoaderHelper->Destroy();
+  mIconLoader->Destroy();
   return NS_OK;
 }
--- a/widget/cocoa/nsTouchBarInputIcon.mm
+++ b/widget/cocoa/nsTouchBarInputIcon.mm
@@ -49,19 +49,21 @@ nsTouchBarInputIcon::~nsTouchBarInputIco
 }
 
 // Called from nsTouchBar's destructor, to prevent us from outliving it
 // (as might otherwise happen if calls to our imgINotificationObserver methods
 // are still outstanding).  nsTouchBar owns our mTouchBarInput.
 void nsTouchBarInputIcon::Destroy() {
   ReleaseJSObjects();
   if (mIconLoader) {
+    mIconLoader->Destroy();
     mIconLoader = nullptr;
   }
   if (mIconLoaderHelper) {
+    mIconLoaderHelper->Destroy();
     mIconLoaderHelper = nullptr;
   }
 
   mButton = nil;
   mShareScrubber = nil;
   mPopoverItem = nil;
 }
 
@@ -125,10 +127,11 @@ nsresult nsTouchBarInputIcon::OnComplete
   }
 
   NSImage* image = mIconLoaderHelper->GetNativeIconImage();
   [mButton setImage:image];
   [mShareScrubber setButtonImage:image];
   [mPopoverItem setCollapsedRepresentationImage:image];
 
   mIconLoaderHelper->Destroy();
+  mIconLoader->Destroy();
   return NS_OK;
 }