Oilpan: Use transition types for HTMLMediaElement and make AudioSourceProviderClient a GC mixin.

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 3b781bf062425814208b18e67bcd5c3da9712c68..b58d7e71d0d53929f002425986b430c05b97720d 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -284,7 +284,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_textTracks(nullptr)
     , m_ignoreTrackDisplayUpdate(0)
 #if ENABLE(WEB_AUDIO)
-    , m_audioSourceNode(0)
+    , m_audioSourceNode(nullptr)
 #endif
 {
     ASSERT(RuntimeEnabledFeatures::mediaEnabled());
@@ -372,7 +372,11 @@ HTMLMediaElement::~HTMLMediaElement()
     // crbug.com/378229
     m_isFinalizing = true;
 #endif
-    clearMediaPlayerAndAudioSourceProviderClient();
+
+#if !ENABLE(OILPAN)
+    ASSERT(!m_audioSourceNode);
+#endif
+    clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();

[acolwell GONE FROM CHROMIUM, 2014/06/10 17:07:17]

This makes me nervous. This is safe because we can't get here if MEASN is still
holding a reference to this object and MEASN won't get destructed unless it is
detached from the WebAudio node graph an therefore it isn't possible for a call
to come in from the audio processing thread? 

Assuming what I've said here is correct, it would be nice to have a comment here
outlining why it is ok to not lock.

[zerny-chromium, 2014/06/10 19:42:29]

Yes. The HTMLMediaElement can only die after the MEASN is gone, so we can assert
that in the non-oilpan build. With oilpan, it can be the case that both objects
are being finalized in the same GC round. Therefor, the destructor can not
access m_audioSourceNode which is potentially already finalized or will be soon.

I've added a comment to that effect.

 
 #if !ENABLE(OILPAN)
     document().decrementLoadEventDelayCount();
@@ -3119,17 +3123,23 @@ void HTMLMediaElement::userCancelledLoad()
     updateActiveTextTrackCues(0);
 }
 
+void HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking()
+{
+#if ENABLE(WEB_AUDIO)
+    if (audioSourceProvider())
+        audioSourceProvider()->setClient(0);
+#endif
+    m_player.clear();
+}
+
 void HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClient()

[acolwell GONE FROM CHROMIUM, 2014/06/10 17:07:17]

nit: How about just moving the body of this function into clearMediaPlayer()
since that appears to be the only call site?

[zerny-chromium, 2014/06/10 19:42:29]

Done.

 {
 #if ENABLE(WEB_AUDIO)
     if (m_audioSourceNode)
         m_audioSourceNode->lock();
-
-    if (audioSourceProvider())
-        audioSourceProvider()->setClient(0);
 #endif
 
-    m_player.clear();
+    clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();
 
 #if ENABLE(WEB_AUDIO)
     if (m_audioSourceNode)
@@ -3656,8 +3666,15 @@ void HTMLMediaElement::trace(Visitor* visitor)
     visitor->trace(m_nextChildNodeToConsider);
     visitor->trace(m_textTracks);
     visitor->trace(m_textTracksWhenResourceSelectionBegan);
+    visitor->registerWeakMembers<HTMLMediaElement, &HTMLMediaElement::clearWeakMembers>(this);
     WillBeHeapSupplementable<HTMLMediaElement>::trace(visitor);
     HTMLElement::trace(visitor);
 }
 
+void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
+{
+    if (!visitor->isAlive(m_audioSourceNode) && audioSourceProvider())
+        audioSourceProvider()->setClient(0);
+}
+
 }