Oilpan: move the MediaPlayer and MediaPlayerClient objects to the heap.

Source/web/WebMediaPlayerClientImpl.cpp

 // Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "web/WebMediaPlayerClientImpl.h"
 
 #include "core/frame/LocalFrame.h"
 #include "core/html/HTMLMediaElement.h"
 #include "core/html/TimeRanges.h"
@@ skipping 52 matching lines @@
 
 WebMediaPlayer* WebMediaPlayerClientImpl::webMediaPlayer() const
 {
     return m_webMediaPlayer.get();
 }
 
 // WebMediaPlayerClient --------------------------------------------------------
 
 WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl()
 {
+#if ENABLE(OILPAN)
+#if ENABLE(WEB_AUDIO)
+    if (audioSourceProvider())

[haraken, 2014/06/03 09:19:47]

I think this cannot be 0.

+        audioSourceProvider()->setClient(0);
+#endif
+    // See detach() comment why we are clearing this reference.
+    m_client.clear();
+
     // Explicitly destroy the WebMediaPlayer to allow verification of tear down.
     m_webMediaPlayer.clear();
+#else
+    // Check that detach has been called. It should always have been done
+    // with Oilpan not enabled.
+    ASSERT(!m_webMediaPlayer);
+#endif
+}
 
+void WebMediaPlayerClientImpl::detach()
+{
+    // FIXME: This ad-hoc notification call to the supplement is out
+    // here to avoid a core/html/ -> modules/encryptedmedia/
+    // dependency. Consider adding a detach notification from the
+    // media element supplementable to its supplements instead to
+    // avoid involving this client object as a third party.
     HTMLMediaElementEncryptedMedia::playerDestroyed(mediaElement());
+
+    // For Oilpan, the client object is on the heap and cannot be safely
+    // accessed by WebMediaPlayerClientImpl when it later is GCed.
+    // Clear out the client reference early.
+    //
+    // FIXME: the browser-side WebMediaPlayer implementation performs a
+    // twisty callback to clear the WebLayer (i.e., calls setWebLayer(0))
+    // when being destructed. This resetting is now handled by the
+    // media element directly when detaching. Follow up and support
+    // clearing of the WebMediaPlayer's client reference, so as to
+    // avoid that callback.
+    m_client.clear();
+
+    // Explicitly destroy the WebMediaPlayer to allow verification of tear down.
+    m_webMediaPlayer.clear();
+}
+
+void WebMediaPlayerClientImpl::trace(Visitor* visitor)
+{
+    visitor->trace(m_client);
+    MediaPlayer::trace(visitor);
 }
 
 void WebMediaPlayerClientImpl::networkStateChanged()
 {
     m_client->mediaPlayerNetworkStateChanged();
 }
 
 void WebMediaPlayerClientImpl::readyStateChanged()
 {
     m_client->mediaPlayerReadyStateChanged();
@@ skipping 49 matching lines @@
     HTMLMediaElementEncryptedMedia::keyMessage(mediaElement(), keySystem, sessionId, message, messageLength, defaultURL);
 }
 
 void WebMediaPlayerClientImpl::keyNeeded(const WebString& contentType, const unsigned char* initData, unsigned initDataLength)
 {
     HTMLMediaElementEncryptedMedia::keyNeeded(mediaElement(), contentType, initData, initDataLength);
 }
 
 void WebMediaPlayerClientImpl::setWebLayer(blink::WebLayer* layer)
 {
+    if (!m_client) {
+        // Only expect the WebLayer to be cleared when finalizing.
+        // FIXME: switch to assume a non-zero layer argument; see
+        // detach() comment.
+        ASSERT(!layer);
+        return;
+    }
     m_client->mediaPlayerSetWebLayer(layer);
 }
 
 void WebMediaPlayerClientImpl::addTextTrack(WebInbandTextTrack* textTrack)
 {
     m_client->mediaPlayerDidAddTextTrack(textTrack);
 }
 
 void WebMediaPlayerClientImpl::removeTextTrack(WebInbandTextTrack* textTrack)
 {
@@ skipping 204 matching lines @@
     return timeValue;
 }
 
 #if ENABLE(WEB_AUDIO)
 AudioSourceProvider* WebMediaPlayerClientImpl::audioSourceProvider()
 {
     return &m_audioSourceProvider;
 }
 #endif
 
-PassOwnPtr<MediaPlayer> WebMediaPlayerClientImpl::create(MediaPlayerClient* client)
+PassOwnPtrWillBeRawPtr<MediaPlayer> WebMediaPlayerClientImpl::create(MediaPlayerClient* client)
 {
-    return adoptPtr(new WebMediaPlayerClientImpl(client));
+    return adoptPtrWillBeNoop(new WebMediaPlayerClientImpl(client));
 }
 
 #if OS(ANDROID)
 void WebMediaPlayerClientImpl::paintOnAndroid(WebCore::GraphicsContext* context, const IntRect& rect, uint8_t alpha)
 {
     OwnPtr<blink::WebGraphicsContext3DProvider> provider = adoptPtr(blink::Platform::current()->createSharedOffscreenGraphicsContext3DProvider());
     if (!provider)
         return;
     WebGraphicsContext3D* context3D = provider->context3d();
     if (!context || !context3D || !m_webMediaPlayer || context->paintingDisabled())
@@ skipping 32 matching lines @@
     , m_rate(1.0)
 #if OS(ANDROID)
     , m_usePaintOnAndroid(false)
 #endif
 {
     ASSERT(m_client);
 }
 
 WebCore::HTMLMediaElement& WebMediaPlayerClientImpl::mediaElement() const
 {
-    return *static_cast<HTMLMediaElement*>(m_client);
+    return *static_cast<HTMLMediaElement*>(m_client.get());
 }
 
 #if ENABLE(WEB_AUDIO)
 void WebMediaPlayerClientImpl::AudioSourceProviderImpl::wrap(WebAudioSourceProvider* provider)
 {
     MutexLocker locker(provideInputLock);
 
     if (m_webAudioSourceProvider && provider != m_webAudioSourceProvider)
         m_webAudioSourceProvider->setClient(0);
 
     m_webAudioSourceProvider = provider;
     if (m_webAudioSourceProvider)
         m_webAudioSourceProvider->setClient(m_client.get());
 }
 
 void WebMediaPlayerClientImpl::AudioSourceProviderImpl::setClient(AudioSourceProviderClient* client)
 {
     MutexLocker locker(provideInputLock);
 
     if (client)
         m_client = adoptPtr(new WebMediaPlayerClientImpl::AudioClientImpl(client));
     else
         m_client.clear();
 
-    if (m_webAudioSourceProvider)
+    if (m_webAudioSourceProvider) {
         m_webAudioSourceProvider->setClient(m_client.get());
+        if (!m_client)
+            m_webAudioSourceProvider = 0;

[haraken, 2014/06/03 09:19:47]

Having this code looks harmless, but I still don't understand why you need this.

In non-oilpan builds, m_webAudioSourceProvider is not cleared when a client is
unregistered from the AudioSourceProviderImpl, is it?

[sof, 2014/06/03 16:35:31]

m_webAudioSourceProvider is something m_webMediaPlayer owns. We cannot keep that
reference once we delete m_webMediaPlayer, it's dead. Hence clearing it for both
Oilpan and not.

It is cleared in non-Oilpan builds also; see what happens when the media element
detaches the player.

+    }
 }
 
 void WebMediaPlayerClientImpl::AudioSourceProviderImpl::provideInput(AudioBus* bus, size_t framesToProcess)
 {
     ASSERT(bus);
     if (!bus)
         return;
 
     MutexTryLocker tryLocker(provideInputLock);
     if (!tryLocker.locked() || !m_webAudioSourceProvider || !m_client.get()) {
@@ skipping 12 matching lines @@
 
 void WebMediaPlayerClientImpl::AudioClientImpl::setFormat(size_t numberOfChannels, float sampleRate)
 {
     if (m_client)
         m_client->setFormat(numberOfChannels, sampleRate);
 }
 
 #endif
 
 } // namespace blink