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

Source/web/WebMediaPlayerClientImpl.cpp

 // Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "web/WebMediaPlayerClientImpl.h"
 
 #include "core/frame/LocalFrame.h"
 #include "core/html/HTMLMediaElement.h"
 #include "core/html/TimeRanges.h"
@@ skipping 41 matching lines @@
 {
     return m_webMediaPlayer.get();
 }
 
 // WebMediaPlayerClient --------------------------------------------------------
 
 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()
 {
     m_client->mediaPlayerNetworkStateChanged();
 }
 
 void WebMediaPlayerClientImpl::readyStateChanged()
 {
     m_client->mediaPlayerReadyStateChanged();
@@ skipping 123 matching lines @@
     // FIXME: Remove this cast
     LocalFrame* frame = mediaElement().document().frame();
 
     WebURL poster = m_client->mediaPlayerPosterURL();
 
     KURL kurl(ParsedURLString, url);
     m_webMediaPlayer = createWebMediaPlayer(this, kurl, frame, HTMLMediaElementEncryptedMedia::contentDecryptionModule(mediaElement()));
     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());
 #endif
 
     m_webMediaPlayer->setVolume(mediaElement().effectiveMediaVolume());
 
     m_webMediaPlayer->setPoster(poster);
 
     setPreload(mediaElement().effectivePreloadType());
@@ skipping 87 matching lines @@
 }
 
 DEFINE_TRACE(WebMediaPlayerClientImpl::AudioClientImpl)
 {
     visitor->trace(m_client);
 }
 
 #endif
 
 } // namespace blink