Bug 857653 - Fix leak in AudioChannelAgent by making it hold a weak ref to nsHTMLMediaElement. r=bz
authorJustin Lebar <justin.lebar@gmail.com>
Wed, 03 Apr 2013 16:35:05 -0400
changeset 138494 0a3ce751dd8ff06b8e4cbe12cf4a2d1b7a31b4a1
parent 138493 07d7c9d93b2a786b7ab824b1f1af8ab064001dea
child 138495 616f9e03bfb2a368e2513273ca26d173bdb61b1d
push id2579
push userakeybl@mozilla.com
push dateMon, 24 Jun 2013 18:52:47 +0000
treeherdermozilla-beta@b69b7de8a05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs857653
milestone23.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 857653 - Fix leak in AudioChannelAgent by making it hold a weak ref to nsHTMLMediaElement. r=bz
content/html/content/src/HTMLMediaElement.cpp
dom/audiochannel/AudioChannelAgent.cpp
dom/audiochannel/AudioChannelAgent.h
dom/audiochannel/nsIAudioChannelAgent.idl
dom/audiochannel/tests/TestAudioChannelService.cpp
--- a/content/html/content/src/HTMLMediaElement.cpp
+++ b/content/html/content/src/HTMLMediaElement.cpp
@@ -3682,17 +3682,18 @@ void HTMLMediaElement::UpdateAudioChanne
     mPlayingThroughTheAudioChannel = playingThroughTheAudioChannel;
 
     if (!mAudioChannelAgent) {
       nsresult rv;
       mAudioChannelAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1", &rv);
       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) {
         bool hidden = false;
         domDoc->GetHidden(&hidden);
         mAudioChannelAgent->SetVisibilityState(!hidden);
       }
     }
--- a/dom/audiochannel/AudioChannelAgent.cpp
+++ b/dom/audiochannel/AudioChannelAgent.cpp
@@ -6,18 +6,17 @@
 #include "AudioChannelCommon.h"
 #include "AudioChannelService.h"
 
 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)
 {
 }
 
 AudioChannelAgent::~AudioChannelAgent()
 {
   if (mIsRegToService) {
@@ -27,19 +26,36 @@ AudioChannelAgent::~AudioChannelAgent()
 
 /* readonly attribute long audioChannelType; */
 NS_IMETHODIMP AudioChannelAgent::GetAudioChannelType(int32_t *aAudioChannelType)
 {
   *aAudioChannelType = mAudioChannelType;
   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.
   MOZ_STATIC_ASSERT(static_cast<AudioChannelType>(AUDIO_AGENT_CHANNEL_NORMAL) ==
                     AUDIO_CHANNEL_NORMAL &&
                     static_cast<AudioChannelType>(AUDIO_AGENT_CHANNEL_CONTENT) ==
                     AUDIO_CHANNEL_CONTENT &&
                     static_cast<AudioChannelType>(AUDIO_AGENT_CHANNEL_NOTIFICATION) ==
                     AUDIO_CHANNEL_NOTIFICATION &&
@@ -49,23 +65,29 @@ NS_IMETHODIMP AudioChannelAgent::Init(in
                     AUDIO_CHANNEL_TELEPHONY &&
                     static_cast<AudioChannelType>(AUDIO_AGENT_CHANNEL_RINGER) ==
                     AUDIO_CHANNEL_RINGER &&
                     static_cast<AudioChannelType>(AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION) ==
                     AUDIO_CHANNEL_PUBLICNOTIFICATION,
                     "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;
 }
 
 /* boolean startPlaying (); */
 NS_IMETHODIMP AudioChannelAgent::StartPlaying(bool *_retval)
 {
   AudioChannelService *service = AudioChannelService::GetAudioChannelService();
   if (mAudioChannelType == AUDIO_AGENT_CHANNEL_ERROR ||
@@ -94,24 +116,36 @@ NS_IMETHODIMP AudioChannelAgent::StopPla
   return NS_OK;
 }
 
 /* void setVisibilityState (in boolean visible); */
 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();
+}
--- a/dom/audiochannel/AudioChannelAgent.h
+++ b/dom/audiochannel/AudioChannelAgent.h
@@ -1,19 +1,20 @@
 /* -*- 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/. */
 
 #ifndef mozilla_dom_audio_channel_agent_h__
 #define mozilla_dom_audio_channel_agent_h__
 
 #include "nsIAudioChannelAgent.h"
 #include "nsCOMPtr.h"
+#include "nsWeakPtr.h"
 
 #define NS_AUDIOCHANNELAGENT_CONTRACTID "@mozilla.org/audiochannelagent;1"
 // f27688e2-3dd7-11e2-904e-10bf48d64bd4
 #define NS_AUDIOCHANNELAGENT_CID {0xf27688e2, 0x3dd7, 0x11e2, \
       {0x90, 0x4e, 0x10, 0xbf, 0x48, 0xd6, 0x4b, 0xd4}}
 
 namespace mozilla {
 namespace dom {
@@ -25,17 +26,27 @@ public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIAUDIOCHANNELAGENT
 
   AudioChannelAgent();
   virtual void NotifyAudioChannelStateChanged();
 
 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;
 };
 
 } // namespace dom
 } // namespace mozilla
 #endif
--- a/dom/audiochannel/nsIAudioChannelAgent.idl
+++ b/dom/audiochannel/nsIAudioChannelAgent.idl
@@ -26,17 +26,17 @@ interface nsIAudioChannelAgentCallback :
  *   3. Notifying the agent when they start/stop using this channel.
  *   4. Notifying the agent of changes to the visibility of the component using
  *       this channel.
  *
  * The agent will invoke a callback to notify Gecko components of
  *   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;
   const long AUDIO_AGENT_CHANNEL_CONTENT            = 1;
   const long AUDIO_AGENT_CHANNEL_NOTIFICATION       = 2;
   const long AUDIO_AGENT_CHANNEL_ALARM              = 3;
   const long AUDIO_AGENT_CHANNEL_TELEPHONY          = 4;
   const long AUDIO_AGENT_CHANNEL_RINGER             = 5;
@@ -55,20 +55,30 @@ interface nsIAudioChannelAgent : nsISupp
    *
    * @param channelType
    *    Audio Channel Type listed as above
    * @param callback
    *    1. Once the playable status changes, agent uses this callback function to notify
    *       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
    *          play audio stream only when return value is true.
    *
    *
    * @return
    *    true: the agent has registered with audio channel service and the
    *          component should start playback.
--- a/dom/audiochannel/tests/TestAudioChannelService.cpp
+++ b/dom/audiochannel/tests/TestAudioChannelService.cpp
@@ -4,16 +4,21 @@
 #ifdef XP_WIN
 #include <windows.h>
 #else
 #include <unistd.h>
 #endif
 
 #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"
 
 #define TEST_ENSURE_BASE(_test, _msg)       \
   PR_BEGIN_MACRO                            \
     if (!_test) {                           \
       fail(_msg);                           \
       return NS_ERROR_FAILURE;              \