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

Source/web/WebMediaPlayerClientImpl.cpp

Index: Source/web/WebMediaPlayerClientImpl.cpp
diff --git a/Source/web/WebMediaPlayerClientImpl.cpp b/Source/web/WebMediaPlayerClientImpl.cpp
index ce4a9341a33b358954fe4897dfa37803f1a51bbc..9363fcc4701022cb3d69969702f1271c60a822c2 100644
--- a/Source/web/WebMediaPlayerClientImpl.cpp
+++ b/Source/web/WebMediaPlayerClientImpl.cpp
@@ -70,10 +70,52 @@ WebMediaPlayer* WebMediaPlayerClientImpl::webMediaPlayer() const
 
 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()
@@ -143,6 +185,13 @@ void WebMediaPlayerClientImpl::keyNeeded(const WebString& contentType, const uns
 
 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);
 }
 
@@ -367,9 +416,9 @@ AudioSourceProvider* WebMediaPlayerClientImpl::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)
@@ -422,7 +471,7 @@ WebMediaPlayerClientImpl::WebMediaPlayerClientImpl(MediaPlayerClient* client)
 
 WebCore::HTMLMediaElement& WebMediaPlayerClientImpl::mediaElement() const
 {
-    return *static_cast<HTMLMediaElement*>(m_client);
+    return *static_cast<HTMLMediaElement*>(m_client.get());
 }
 
 #if ENABLE(WEB_AUDIO)
@@ -447,8 +496,11 @@ void WebMediaPlayerClientImpl::AudioSourceProviderImpl::setClient(AudioSourcePro
     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)