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())
+        audioSourceProvider()->setClient(0);

[haraken, 2014/06/03 05:31:08]

I'm a bit confused: Why do we need to explicitly clear the client in oilpan
builds, whereas we are not doing the clear in non-oilpan builds?

[sof, 2014/06/03 08:04:11]

Because we will call clearMediaPlayerAndAudioSourceProviderClient() in the
non-Oilpan case, which calls setClient().

In the Oilpan case, should this object die at the same time as the media
element, we will have to arrange for setClient(0) to be called on the
AudioSourceProvider also. That is the purpose of this call.

To avoid performing a repeated setClient(0) call on said provider, we clear out
the m_webAudioSourceProvider when the client is cleared. That is the purpose of
the added code in setClient().

+#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 05:31:08]

The same question here. I'm curious why we need to introduce this clear in
oilpan builds.

[sof, 2014/06/03 08:04:11]

See answer to 
https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye...

+    }
 }
 
 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