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/02 15:24:48]

Instead of adding these lines, if possible, it would be better to move
AudioSourceProvider to the heap at the same time.

+#endif
+    // See detach() comment why we are clearing this reference.
+    m_client.clear();

[haraken, 2014/06/02 15:24:48]

hmm, I don't still understand why you need this clear. Here the
WebMediaPlayerClientImpl is about to die, so we no longer need to take care of
Members, don't we?

[sof, 2014/06/02 15:46:55]

It is needed for the exact same reason as it is needed in detach(). If the
client object dies without calling detach() (a possibility with Oilpan), then we
have to prevent the WebMediaPlayer from attempting to clear the WebLayer by
calling back into the client when destructed.

[haraken, 2014/06/02 16:01:42]

Makes sense.

Then who calls mediaPlayerSetWebLayer(0) when the WebMediaPlayerClient and the
HTMLMediaElement die at the same time? It seems we need to call
mediaPlayerSetWebLayer(0) even when they die togther, in order to call
GraphicsLayer::unregisterContentsLayer(m_webLayer).

[sof, 2014/06/02 16:25:08]

When doing the obvious finalization of that pre-existing layer, this lead to
stability problems in a number of cases. I will have to re-investigate.

[sof, 2014/06/02 22:00:46]

I cannot reproduce that instability now (but am running it past all bots atm to
verify) -- it could be that this case of WebLayer resetting got tangled up with
clearing the client reference correctly. Not sure.

Anyway, I've re-instated that call to mediaPlayerSetWebLayer(0) in
~HTMLMediaElement in the Oilpan case. Only called if the player hasn't been
detached, i.e., the media element and player die at the same time.

+
     // Explicitly destroy the WebMediaPlayer to allow verification of tear down.
     m_webMediaPlayer.clear();

[haraken, 2014/06/02 15:24:48]

You don't need this since the OwnPtr will be automatically destroyed when
WebMediaPlayerClientImpl is destructed.

[sof, 2014/06/02 16:25:08]

I'm just keeping what was here previously.

+#else
+    // Check that detach has been called. It should always have been done
+    // with Oilpan not enabled.
+    ASSERT(!m_webMediaPlayer);
+#endif
+}
 
+void WebMediaPlayerClientImpl::detach()
+{

[zerny-chromium, 2014/06/02 12:59:07]

The two issues below are not nice, but I think your proposals for follow-up
fixes seem reasonable.

+    // 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.

[haraken, 2014/06/02 15:24:48]

Understood. A follow-up is fine :)

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

[zerny-chromium, 2014/06/02 12:59:07]

Yeah, this is is really not nice, but fixing in a follow-up is ok with me.

+    }
     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/02 15:24:48]

This looks weird. Moving AudioSourceProvider to the heap will allow us to remove
these lines, but would it be hard?

Nit: You need #if ENABLE(WEB_AUDIO).

[sof, 2014/06/02 16:25:08]

Hmm, I think this CL is complex enough as it is.
Already within such a context.

[haraken, 2014/06/02 16:32:40]

I'm fine with addressing the issue in a follow-up. A FIXME is fine in this CL.

[sof, 2014/06/02 22:00:46]

Added a FIXME next to the AudoSourceProviderImpl declaration.

+    }
 }
 
 void WebMediaPlayerClientImpl::AudioSourceProviderImpl::provideInput(AudioBus* bus, size_t framesToProcess)