Invalidate video elements when starting up media players and changing DisplayMode.

Source/web/WebMediaPlayerClientImpl.cpp

Index: Source/web/WebMediaPlayerClientImpl.cpp
diff --git a/Source/web/WebMediaPlayerClientImpl.cpp b/Source/web/WebMediaPlayerClientImpl.cpp
index d57802add60b16b2ddb4770a1ff086f4acc4eb47..28840ec7ddf0984aeec7a962841e1946e611df50 100644
--- a/Source/web/WebMediaPlayerClientImpl.cpp
+++ b/Source/web/WebMediaPlayerClientImpl.cpp
@@ -59,6 +59,9 @@ WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl()
 {
     // Explicitly destroy the WebMediaPlayer to allow verification of tear down.
     m_webMediaPlayer.clear();
+
+    if (mediaElement().layoutObject())

[sof, 2015/06/18 14:02:49]

Is this a safe thing to do? And is it needed?

WebMediaPlayerClientImpl will be destructed by the media element (via
clearMediaPlayerAndAudioSourceProvidedClientsWithoutLocking()), which is
accessed here in the dtor. In the Oilpan case, this will result in the access of
other heap objects (e.g., Node's rare data). Which may or may not have been
finalized. ASan picks up on this


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...

(the stacks reported from __asan_report_error() are broken atm for reasons
unknown.)

We may be able to accommodate this kind of access on the Oilpan side, but this
change is a bit tight (accessing a destructing object up the stack), so I'm
wondering if it is needed? (I haven't investigated your TCs, as I cannot access
the bug.)

 

[chrishtr, 2015/06/18 14:37:09]

 clearMediaPlayerAndAudioSourceProviderClientWithoutLocking is called in the
destructor of HTMLMediaElement, but also in cases when it wants to clear the
media element without immediate destruction, e.g.
HTMLMediaElement::clearMediaPlayer.

The reason I think I need code like this is that VideoPainter changes its
painting behavior in response to whether there is a media player connected to
the element or not, hence we need to issue paint invalidations when the media
player object is created or destroyed.

I don't quite get why this would be unsafe though. Is calling mediaElement() at
all unsafe because we're in the destructor of that element in some cases?
 

+        mediaElement().layoutObject()->setShouldDoFullPaintInvalidation();
 }
 
 void WebMediaPlayerClientImpl::networkStateChanged()
@@ -202,6 +205,9 @@ void WebMediaPlayerClientImpl::load(WebMediaPlayer::LoadType loadType, const WTF
     if (!m_webMediaPlayer)
         return;
 
+    if (mediaElement().layoutObject())
+        mediaElement().layoutObject()->setShouldDoFullPaintInvalidation();
+
 #if ENABLE(WEB_AUDIO)
     // Make sure if we create/re-create the WebMediaPlayer that we update our wrapper.
     m_audioSourceProvider.wrap(m_webMediaPlayer->audioSourceProvider());