Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by chrishtr Modified:
4 years, 10 months ago CC:
blink-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, oilpan-reviews, philipj_slow Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionInvalidate video elements when starting up media players and changing DisplayMode.
BUG=497614
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197301
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Messages
Total messages: 13 (3 generated)
chrishtr@chromium.org changed reviewers: + wangxianzhu@chromium.org
I am going to look at a test, but given the crashes I am requesting to not have a test added once I have one. I verified that the link the bug does not crash.
The CQ bit was checked by wangxianzhu@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180073014/40001
In VideoPainter, we have early outs to avoid drawing when display mode or media player init are not set. Hence we need to invalidate when they appear.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197301
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:63: if (mediaElement().layoutObject()) 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.)
Message was sent while issue was closed.
https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:63: if (mediaElement().layoutObject()) On 2015/06/18 at 14:02:49, sof wrote: > 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.) 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?
Message was sent while issue was closed.
On 2015/06/18 14:37:09, chrishtr wrote: > https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... > File Source/web/WebMediaPlayerClientImpl.cpp (right): > > https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... > Source/web/WebMediaPlayerClientImpl.cpp:63: if (mediaElement().layoutObject()) > On 2015/06/18 at 14:02:49, sof wrote: > > 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.) > > 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? It is accessing an object hanging off the media element that's the problem -- that works out in the non-Oilpan case, but not so with. Can the invalidation flag be set in HTMLMediaElement::clearMediaPlayer() instead?
Message was sent while issue was closed.
On 2015/06/18 at 14:43:00, sigbjornf wrote: > On 2015/06/18 14:37:09, chrishtr wrote: > > https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... > > File Source/web/WebMediaPlayerClientImpl.cpp (right): > > > > https://codereview.chromium.org/1180073014/diff/40001/Source/web/WebMediaPlay... > > Source/web/WebMediaPlayerClientImpl.cpp:63: if (mediaElement().layoutObject()) > > On 2015/06/18 at 14:02:49, sof wrote: > > > 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.) > > > > 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? > > It is accessing an object hanging off the media element that's the problem -- that works out in the non-Oilpan case, but not so with. > > Can the invalidation flag be set in HTMLMediaElement::clearMediaPlayer() instead? Yes I think that would work. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
