HTMLMediaElement::clearMediaPlayer should acquire MediaElementAudioSourceNode::lock()

Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 3367 matching lines @@
 
     // Reset m_readyState since m_player is gone.
     m_readyState = HAVE_NOTHING;
     updateMediaController();
     if (RuntimeEnabledFeatures::videoTrackEnabled())
         updateActiveTextTrackCues(0);
 }
 
 void HTMLMediaElement::clearMediaPlayer(int flags)
 {
+#if ENABLE(WEB_AUDIO)
+    if (m_audioSourceNode)
+        m_audioSourceNode->lock();

[Ken Russell (switch to Gerrit), 2013/11/26 20:09:24]

The manual calls to lock() and unlock() are unfortunate. It would be much better
to use a Locker class like in wtf/Locker.h. Unfortunately, that class deals only
with references, not with pointers that might be NULL.

+#endif
+
     removeAllInbandTracks();
 
     closeMediaSource();
 
     m_player.clear();
 #if ENABLE(WEB_AUDIO)
     if (audioSourceProvider())
         audioSourceProvider()->setClient(0);

[acolwell GONE FROM CHROMIUM, 2013/11/26 16:08:58]

How does this ever get executed? Doesn't audioSourceProvider() return 0 since
m_player is null at this point.

[DaleCurtis, 2013/11/26 18:36:15]

Hah, wow, yeah that's true even in the original CL.  I missed that. The code
should be above the .clear() ? It must introduce enough "hand-waving" to prevent
the race the previous Cl was fixing.

[Ken Russell (switch to Gerrit), 2013/11/26 20:09:24]

This should be fixed, and further, HTMLMediaElement::~HTMLMediaElement has the
same problem. Please refactor this code to share the m_player.clear() and
audioSourceProvider()->setClient(0) calls between these two methods.

[haraken, 2013/11/27 01:10:29]

Done.

 #endif
+
+#if ENABLE(WEB_AUDIO)

[acolwell GONE FROM CHROMIUM, 2013/11/26 16:08:58]

nit: Remove this line and the #endif right above it.

[haraken, 2013/11/27 01:10:29]

Done.

+    if (m_audioSourceNode)
+        m_audioSourceNode->unlock();
+#endif
+
     stopPeriodicTimers();
     m_loadTimer.stop();
 
     m_pendingActionFlags &= ~flags;
     m_loadState = WaitingForSource;
 
     if (m_textTracks)
         configureTextTrackDisplay(AssumeNoVisibleChange);
 }
 
@@ skipping 293 matching lines @@
     }
 #endif
 }
 
 #if ENABLE(WEB_AUDIO)
 void HTMLMediaElement::setAudioSourceNode(MediaElementAudioSourceNode* sourceNode)
 {
     m_audioSourceNode = sourceNode;
 
     if (audioSourceProvider())
         audioSourceProvider()->setClient(m_audioSourceNode);

[acolwell GONE FROM CHROMIUM, 2013/11/26 16:08:58]

Why don't you need a lock around this call of setClient()?

[Ken Russell (switch to Gerrit), 2013/11/26 20:09:24]

If the lock above is necessary then it's necessary here too.

[haraken, 2013/11/27 01:10:29]

Done.

 }
 
 AudioSourceProvider* HTMLMediaElement::audioSourceProvider()
 {
     if (m_player)
         return m_player->audioSourceProvider();
 
     return 0;
 }
 #endif
@@ skipping 151 matching lines @@
 {
     scheduleLayerUpdate();
 }
 
 bool HTMLMediaElement::isInteractiveContent() const
 {
     return fastHasAttribute(controlsAttr);
 }
 
 }