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..3700361c8a196907cb63c7f2f55968e5092d84ec 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)
@@ -330,18 +327,17 @@ HTMLMediaElement::~HTMLMediaElement()
 #if !ENABLE(OILPAN)
     if (m_textTracks)
         m_textTracks->clearOwner();
-#endif
 
     if (m_mediaController) {
         m_mediaController->removeMediaElement(this);
         m_mediaController = nullptr;
     }
+#endif
 
     closeMediaSource();
 
 #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);
 #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();

[acolwell GONE FROM CHROMIUM, 2014/06/03 19:23:08]

Why is this done here? I believe this needs to occur after the
m_audioSourceNode->lock() to avoid the audio thread from accessing the audio
source provider during destruction.

[sof, 2014/06/03 19:52:44]

"this" == closeMediaSource() ? clearMediaPlayerAndAudioSourceProviderClient()
has its own lock-unlock pairing.

It mirrors the ordering for clearMediaPlayer(), where closeMediaSource() is
called prior to clear...Client(); and there's no explicit lock there first.

IOW, I thought the lock call here was to control/protect the
MediaPlayer::create(this) call.

+
 #if ENABLE(WEB_AUDIO)
     if (m_audioSourceNode)
         m_audioSourceNode->lock();
 #endif
 
-    closeMediaSource();
-
     m_player = MediaPlayer::create(this);
 
 #if ENABLE(WEB_AUDIO)
@@ -3521,7 +3525,7 @@ MediaController* HTMLMediaElement::controller() const
     return m_mediaController.get();
 }
 
-void HTMLMediaElement::setController(PassRefPtr<MediaController> controller)
+void HTMLMediaElement::setController(PassRefPtrWillBeRawPtr<MediaController> controller)
 {
     // 4.8.10.11.2 Media controllers: controller attribute.
     // On setting, it must first remove the element's mediagroup attribute, if any,
@@ -3530,7 +3534,7 @@ void HTMLMediaElement::setController(PassRefPtr<MediaController> controller)
     setControllerInternal(controller);
 }
 
-void HTMLMediaElement::setControllerInternal(PassRefPtr<MediaController> controller)
+void HTMLMediaElement::setControllerInternal(PassRefPtrWillBeRawPtr<MediaController> controller)
 {
     if (m_mediaController)
         m_mediaController->removeMediaElement(this);
@@ -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,12 @@ 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);
+    visitor->trace(m_mediaController);
     WillBeHeapSupplementable<HTMLMediaElement>::trace(visitor);
+    MediaPlayerClient::trace(visitor);
     HTMLElement::trace(visitor);
 }