From a88d74d517e1155b56e2b99a5a05e74b214472bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 24 Sep 2020 16:01:50 +0000
Subject: [PATCH] Bug 1658571 - Don't create a reference cycle when catching
 redirects of a preload. r=smaug

PreloaderBase -> RedirectSink -> PreloaderBase is a strong,
non-cycle-collected reference cycle, which in cases where we don't drop
the channel because we never get an NotifyStop notification, it can
cause leaks.

I'm investigating the root cause of the lack of NotifyStop, but this
should fix the leak and is correct anyhow.

Move the class to the cpp file to ease debugging and changes.

Differential Revision: https://phabricator.services.mozilla.com/D91259
---
 uriloader/preload/PreloaderBase.cpp | 38 +++++++++++++++++++++++++----
 uriloader/preload/PreloaderBase.h   | 20 +--------------
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/uriloader/preload/PreloaderBase.cpp b/uriloader/preload/PreloaderBase.cpp
index 0e15d9c4ab71..961f9cd60761 100644
--- a/uriloader/preload/PreloaderBase.cpp
+++ b/uriloader/preload/PreloaderBase.cpp
@@ -22,11 +22,35 @@ PreloaderBase::UsageTimer::UsageTimer(PreloaderBase* aPreload,
                                       dom::Document* aDocument)
     : mDocument(aDocument), mPreload(aPreload) {}
 
+class PreloaderBase::RedirectSink final : public nsIInterfaceRequestor,
+                                          public nsIChannelEventSink,
+                                          public nsIRedirectResultListener {
+  RedirectSink() = delete;
+  virtual ~RedirectSink();
+
+ public:
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSIINTERFACEREQUESTOR
+  NS_DECL_NSICHANNELEVENTSINK
+  NS_DECL_NSIREDIRECTRESULTLISTENER
+
+  RedirectSink(PreloaderBase* aPreloader, nsIInterfaceRequestor* aCallbacks);
+
+ private:
+  WeakPtr<PreloaderBase> mPreloader;
+  nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
+  nsCOMPtr<nsIChannel> mRedirectChannel;
+};
+
 PreloaderBase::RedirectSink::RedirectSink(PreloaderBase* aPreloader,
                                           nsIInterfaceRequestor* aCallbacks)
-    : mPreloader(new nsMainThreadPtrHolder<PreloaderBase>(
-          "RedirectSink.mPreloader", aPreloader)),
-      mCallbacks(aCallbacks) {}
+    : mPreloader(aPreloader), mCallbacks(aCallbacks) {}
+
+PreloaderBase::RedirectSink::~RedirectSink() {
+  MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread(),
+                        "Should figure out how to safely drop mPreloader "
+                        "otherwise");
+}
 
 NS_IMPL_ISUPPORTS(PreloaderBase::RedirectSink, nsIInterfaceRequestor,
                   nsIChannelEventSink, nsIRedirectResultListener)
@@ -34,13 +58,17 @@ NS_IMPL_ISUPPORTS(PreloaderBase::RedirectSink, nsIInterfaceRequestor,
 NS_IMETHODIMP PreloaderBase::RedirectSink::AsyncOnChannelRedirect(
     nsIChannel* aOldChannel, nsIChannel* aNewChannel, uint32_t aFlags,
     nsIAsyncVerifyRedirectCallback* aCallback) {
+  MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread());
+
   mRedirectChannel = aNewChannel;
 
   // Deliberately adding this before confirmation.
   nsCOMPtr<nsIURI> uri;
   aNewChannel->GetOriginalURI(getter_AddRefs(uri));
-  mPreloader->mRedirectRecords.AppendElement(
-      RedirectRecord(aFlags, uri.forget()));
+  if (mPreloader) {
+    mPreloader->mRedirectRecords.AppendElement(
+        RedirectRecord(aFlags, uri.forget()));
+  }
 
   if (mCallbacks) {
     nsCOMPtr<nsIChannelEventSink> sink(do_GetInterface(mCallbacks));
diff --git a/uriloader/preload/PreloaderBase.h b/uriloader/preload/PreloaderBase.h
index 7c520eb49292..acbc41cb97ba 100644
--- a/uriloader/preload/PreloaderBase.h
+++ b/uriloader/preload/PreloaderBase.h
@@ -150,25 +150,7 @@ class PreloaderBase : public SupportsWeakPtr, public nsISupports {
   // directly is to keep PreloaderBase as simple as possible so that derived
   // classes don't have to deal with calling super when implementing these
   // interfaces from some reason as well.
-  class RedirectSink final : public nsIInterfaceRequestor,
-                             public nsIChannelEventSink,
-                             public nsIRedirectResultListener {
-    RedirectSink() = delete;
-    virtual ~RedirectSink() = default;
-
-   public:
-    NS_DECL_THREADSAFE_ISUPPORTS
-    NS_DECL_NSIINTERFACEREQUESTOR
-    NS_DECL_NSICHANNELEVENTSINK
-    NS_DECL_NSIREDIRECTRESULTLISTENER
-
-    RedirectSink(PreloaderBase* aPreloader, nsIInterfaceRequestor* aCallbacks);
-
-   private:
-    nsMainThreadPtrHandle<PreloaderBase> mPreloader;
-    nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
-    nsCOMPtr<nsIChannel> mRedirectChannel;
-  };
+  class RedirectSink;
 
   // A timer callback to trigger the unuse warning for this preload
   class UsageTimer final : public nsITimerCallback {