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

Source/core/html/HTMLMediaElement.cpp

Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp b/Source/core/html/HTMLMediaElement.cpp
index 5a413b76a92dd59e447c11ba062f168869783f8c..83450a0a093732dae49ec8c5fa5aaff4ab020733 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -282,9 +282,6 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_tracksAreReady(true)
     , m_haveVisibleTextTrack(false)
     , m_processingPreferenceChange(false)
-#if ENABLE(OILPAN)
-    , m_isFinalizing(false)
-#endif
     , m_lastTextTrackUpdateTime(-1)
     , m_textTracks(nullptr)
     , m_ignoreTrackDisplayUpdate(0)
@@ -341,7 +338,6 @@ HTMLMediaElement::~HTMLMediaElement()
 
 #if !ENABLE(OILPAN)
     removeElementFromDocumentMap(this, &document());
-#endif
 
     // Destroying the player may cause a resource load to be canceled,
     // which could result in userCancelledLoad() being called back.
@@ -353,7 +349,7 @@ HTMLMediaElement::~HTMLMediaElement()
     // With Oilpan load events on the Document are always delayed during
     // sweeping so we don't need to explicitly increment and decrement
     // load event delay counts.
-#if !ENABLE(OILPAN)
+
     // Destroying the player may cause a resource load to be canceled,
     // which could result in Document::dispatchWindowLoadEvent() being
     // called via ResourceFetch::didLoadResource() then
@@ -361,26 +357,17 @@ HTMLMediaElement::~HTMLMediaElement()
     // object destruction, we use Document::incrementLoadEventDelayCount().
     // See http://crbug.com/275223 for more details.
     document().incrementLoadEventDelayCount();
-#endif
 
-#if ENABLE(OILPAN)
-    // Oilpan: the player must be released, but the player object
-    // cannot safely access this player client any longer as parts of
-    // it may have been finalized already (like the media element's
-    // supplementable table.)  Handled for now by entering an
-    // is-finalizing state, which is explicitly checked for if the
-    // player tries to access the media element during shutdown.
-    //
-    // FIXME: Oilpan: move the media player to the heap instead and
-    // avoid having to finalize it from here; this whole #if block
-    // could then be removed (along with the state bit it depends on.)
-    // crbug.com/378229
-    m_isFinalizing = true;
-#endif
     clearMediaPlayerAndAudioSourceProviderClient();
 
-#if !ENABLE(OILPAN)
     document().decrementLoadEventDelayCount();
+#else
+    // If the media element is finalized at the same time as
+    // the media player, the latter cannot be accessed and
+    // detached from the destructor. The web layer can be
+    // cleared out safely, however.
+    if (m_player)
+        mediaPlayerSetWebLayer(0);

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

This looks OK in this CL, but I'm curious what happens if you move
WebMediaPlayer to the heap in a follow-up.

[haraken, 2014/06/03 06:00:14]

So the above comment doesn't make sense.

Revised comment: how about moving the mediaPlayerSetWebLayer(0) call from
~HTMLMediaElement to ~WebMediaPlayerClientImpl, because
~WebMediaPlayerClientImpl owns the m_webMediaPlayer ?

[sof, 2014/06/03 06:11:43]

The m_webMediaPlayer and WebMediaPlayerClientImpl's client (an HTMLMediaElement)
keeps track of the current WebLayer, not WebMediaPlayerClientImpl, so it
wouldn't know what object to unregister with GraphicsLayer in
~WebMediaPlayerClientImpl (or detach(), I suppose.)

It could intercept the setting of WebLayers via setWebLayer() and keep a
separate copy, clearly. The question then becomes: is that worth doing?

 #endif
 }
 
@@ -3138,6 +3125,21 @@ void HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClient()
         audioSourceProvider()->setClient(0);
 #endif
 
+    // With Oilpan, if the media player is explicitly cleared, the
+    // player is detach()ed first to notify dependent objects.
+    // If this media element (and the player's "client") is finalized
+    // at the same time as the media player, no need to explicitly
+    // detach and notify. All heap-residing objects will die at the
+    // same time, and the media player object will handle notification
+    // of the browser process WebMediaPlayer object when finalized.
+    //
+    // To keep code paths regular, we explicitly detach in a non-Oilpan
+    // setting also rather than do it as part of the player's destructor.
+    if (m_player) {
+        // Reset the web layer right away.
+        mediaPlayerSetWebLayer(0);
+        m_player->detach();
+    }
     m_player.clear();
 
 #if ENABLE(WEB_AUDIO)
@@ -3435,13 +3437,15 @@ void* HTMLMediaElement::preDispatchEventHandler(Event* event)
 
 void HTMLMediaElement::createMediaPlayer()
 {
+    closeMediaSource();
+
+    clearMediaPlayerAndAudioSourceProviderClient();

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

I wonder why you had to add this to oilpan builds.

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

We are possibly severing the connection to a pre-existing media player, so it
will have to be detached.

+
 #if ENABLE(WEB_AUDIO)
     if (m_audioSourceNode)
         m_audioSourceNode->lock();
 #endif
 
-    closeMediaSource();
-
     m_player = MediaPlayer::create(this);
 
 #if ENABLE(WEB_AUDIO)
@@ -3634,9 +3638,8 @@ void HTMLMediaElement::mediaPlayerSetWebLayer(blink::WebLayer* webLayer)
     if (m_webLayer)
         GraphicsLayer::unregisterContentsLayer(m_webLayer);
     m_webLayer = webLayer;
-    if (m_webLayer) {
+    if (m_webLayer)
         GraphicsLayer::registerContentsLayer(m_webLayer);
-    }
 }
 
 void HTMLMediaElement::mediaPlayerMediaSourceOpened(blink::WebMediaSource* webMediaSource)
@@ -3663,9 +3666,11 @@ void HTMLMediaElement::trace(Visitor* visitor)
     visitor->trace(m_error);
     visitor->trace(m_currentSourceNode);
     visitor->trace(m_nextChildNodeToConsider);
+    visitor->trace(m_player);
     visitor->trace(m_textTracks);
     visitor->trace(m_textTracksWhenResourceSelectionBegan);
     WillBeHeapSupplementable<HTMLMediaElement>::trace(visitor);
+    MediaPlayerClient::trace(visitor);
     HTMLElement::trace(visitor);
 }