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())
+        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()
@@ -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 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)