Bug 857653 - Fix leak in AudioChannelAgent by making it hold a weak ref to nsHTMLMediaElement. r=bz

content/html/content/src/HTMLMediaElement.cpp

@@ -3687,7 +3687,8 @@ void HTMLMediaElement::UpdateAudioChannelPlayingState()
       if (!mAudioChannelAgent) {
         return;
       }
-      mAudioChannelAgent->Init(mAudioChannelType, this);
+      // Use a weak ref so the audio channel agent can't leak |this|.
+      mAudioChannelAgent->InitWithWeakCallback(mAudioChannelType, this);
 
       nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(OwnerDoc());
       if (domDoc) {

dom/audiochannel/AudioChannelAgent.cpp

@@ -11,8 +11,7 @@ using namespace mozilla::dom;
 NS_IMPL_ISUPPORTS1(AudioChannelAgent, nsIAudioChannelAgent)
 
 AudioChannelAgent::AudioChannelAgent()
-  : mCallback(nullptr)
-  , mAudioChannelType(AUDIO_AGENT_CHANNEL_ERROR)
+  : mAudioChannelType(AUDIO_AGENT_CHANNEL_ERROR)
   , mIsRegToService(false)
   , mVisible(true)
 {
@@ -32,8 +31,25 @@ NS_IMETHODIMP AudioChannelAgent::GetAudioChannelType(int32_t *aAudioChannelType)
   return NS_OK;
 }
 
-/* boolean init (in long channelType); */
+/* boolean init (in long channelType, in nsIAudioChannelAgentCallback callback); */
 NS_IMETHODIMP AudioChannelAgent::Init(int32_t channelType, nsIAudioChannelAgentCallback *callback)
+{
+  return InitInternal(channelType, callback, /* useWeakRef = */ false);
+}
+
+/* boolean initWithWeakCallback (in long channelType,
+ *                               in nsIAudioChannelAgentCallback callback); */
+NS_IMETHODIMP
+AudioChannelAgent::InitWithWeakCallback(int32_t channelType,
+                                        nsIAudioChannelAgentCallback *callback)
+{
+  return InitInternal(channelType, callback, /* useWeakRef = */ true);
+}
+
+nsresult
+AudioChannelAgent::InitInternal(int32_t aChannelType,
+                                nsIAudioChannelAgentCallback *aCallback,
+                                bool aUseWeakRef)
 {
   // We syncd the enum of channel type between nsIAudioChannelAgent.idl and
   // AudioChannelCommon.h the same.
@@ -54,13 +70,19 @@ NS_IMETHODIMP AudioChannelAgent::Init(int32_t channelType, nsIAudioChannelAgentC
                     "Enum of channel on nsIAudioChannelAgent.idl should be the same with AudioChannelCommon.h");
 
   if (mAudioChannelType != AUDIO_AGENT_CHANNEL_ERROR ||
-      channelType > AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION ||
-      channelType < AUDIO_AGENT_CHANNEL_NORMAL) {
+      aChannelType > AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION ||
+      aChannelType < AUDIO_AGENT_CHANNEL_NORMAL) {
     return NS_ERROR_FAILURE;
   }
 
-  mAudioChannelType = channelType;
-  mCallback = callback;
+  mAudioChannelType = aChannelType;
+
+  if (aUseWeakRef) {
+    mWeakCallback = do_GetWeakReference(aCallback);
+  } else {
+    mCallback = aCallback;
+  }
+
   return NS_OK;
 }
 
@@ -99,19 +121,31 @@ NS_IMETHODIMP AudioChannelAgent::SetVisibilityState(bool visible)
 {
   bool oldVisibility = mVisible;
 
+  nsCOMPtr<nsIAudioChannelAgentCallback> callback = GetCallback();
+
   mVisible = visible;
-  if (mIsRegToService && oldVisibility != mVisible && mCallback != nullptr) {
+  if (mIsRegToService && oldVisibility != mVisible && callback) {
     AudioChannelService *service = AudioChannelService::GetAudioChannelService();
-    mCallback->CanPlayChanged(!service->GetMuted(this, !mVisible));
+    callback->CanPlayChanged(!service->GetMuted(this, !mVisible));
   }
   return NS_OK;
 }
 
 void AudioChannelAgent::NotifyAudioChannelStateChanged()
 {
-  if (mCallback != nullptr) {
+  nsCOMPtr<nsIAudioChannelAgentCallback> callback = GetCallback();
+  if (callback) {
     AudioChannelService *service = AudioChannelService::GetAudioChannelService();
-    mCallback->CanPlayChanged(!service->GetMuted(this, !mVisible));
+    callback->CanPlayChanged(!service->GetMuted(this, !mVisible));
   }
 }
 
+already_AddRefed<nsIAudioChannelAgentCallback>
+AudioChannelAgent::GetCallback()
+{
+  nsCOMPtr<nsIAudioChannelAgentCallback> callback = mCallback;
+  if (!callback) {
+    callback = do_QueryReferent(mWeakCallback);
+  }
+  return callback.forget();
+}

dom/audiochannel/AudioChannelAgent.h

@@ -1,5 +1,5 @@
 /* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
-/* vim: set ts=2 et sw=2 tw=40: */
+/* vim: set ts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
@@ -9,6 +9,7 @@
 
 #include "nsIAudioChannelAgent.h"
 #include "nsCOMPtr.h"
+#include "nsWeakPtr.h"
 
 #define NS_AUDIOCHANNELAGENT_CONTRACTID "@mozilla.org/audiochannelagent;1"
 // f27688e2-3dd7-11e2-904e-10bf48d64bd4
@@ -30,7 +31,17 @@ public:
 
 private:
   virtual ~AudioChannelAgent();
+
+  // Returns mCallback if that's non-null, or otherwise tries to get an
+  // nsIAudioChannelAgentCallback out of mWeakCallback.
+  already_AddRefed<nsIAudioChannelAgentCallback> GetCallback();
+
+  nsresult InitInternal(int32_t aAudioAgentType,
+                        nsIAudioChannelAgentCallback* aCallback,
+                        bool aUseWeakRef);
+
   nsCOMPtr<nsIAudioChannelAgentCallback> mCallback;
+  nsWeakPtr mWeakCallback;
   int32_t mAudioChannelType;
   bool mIsRegToService;
   bool mVisible;

dom/audiochannel/nsIAudioChannelAgent.idl

@@ -31,7 +31,7 @@ interface nsIAudioChannelAgentCallback : nsISupports
  *   1. Changes to the playable status of this channel.
  */
 
-[scriptable, uuid(4d01d4f0-3d16-11e2-a0db-10bf48d64bd4)]
+[scriptable, uuid(f012a9b7-6431-4915-a4ac-4ba7d833e28e)]
 interface nsIAudioChannelAgent : nsISupports
 {
   const long AUDIO_AGENT_CHANNEL_NORMAL             = 0;
@@ -60,9 +60,19 @@ interface nsIAudioChannelAgent : nsISupports
    *       Gecko component.
    *    2. The callback is allowed to be null. Ex: telephony doesn't need to listen change
    *       of the playable status.
+   *    3. The AudioChannelAgent keeps a strong reference to the callback object.
    */
   void init(in long channelType, in nsIAudioChannelAgentCallback callback);
 
+  /**
+   * This method is just like init(), except the audio channel agent keeps a
+   * weak reference to the callback object.
+   *
+   * In order for this to work, |callback| must implement
+   * nsISupportsWeakReference.
+   */
+  void initWithWeakCallback(in long channelType, in nsIAudioChannelAgentCallback callback);
+
   /**
    * Notify the agent that we want to start playing.
    * Note: Gecko component SHOULD call this function first then start to

dom/audiochannel/tests/TestAudioChannelService.cpp

@@ -9,6 +9,11 @@
 
 #include "TestHarness.h"
 
+// Work around the fact that the nsWeakPtr, used by AudioChannelService.h, is
+// not exposed to consumers outside the internal API.
+#include "nsIWeakReference.h"
+typedef nsCOMPtr<nsIWeakReference> nsWeakPtr;
+
 #include "AudioChannelService.h"
 #include "AudioChannelAgent.h"