Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(199)

Issue 303593002: Oilpan: prevent player from accessing its media element during finalization (Closed)

Created:
6 years, 7 months ago by sof
Modified:
6 years, 6 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Oilpan: prevent player from accessing its media element during finalization When finalizing the media element, its media player is also cleared out. With Oilpan enabled, that media player object must not touch the media element while destructing its "client", as it is not in a valid state (its heap object member may have been finalized already.) Arrange for that to not happen by having the media element enter a 'finalizing' state, which is explicitly checked for when the player attempts to access the media element during destruction. This is a shorter-term solution until the media player object itself is moved to the Oilpan heap; http://crbug.com/378229 for handling that. R=haraken@chromium.org,ager@chromium.org BUG=377567 TEST=media/track/track-removal-crash.html TEST=media/audio-delete-while-slider-thumb-clicked.html TEST=http/tests/media/media-source/mediasource-closed-on-htmlmediaelement-destruction.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174964

Patch Set 1 #

Patch Set 2 : Rephrase player finalization as detaching #

Total comments: 3

Patch Set 3 : Switch back to initial approach + FIXMEs for follow-up work #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -4 lines) Patch
M LayoutTests/OilpanExpectations View 1 2 2 chunks +1 line, -4 lines 0 comments Download
M Source/core/html/HTMLMediaElement.h View 2 2 chunks +7 lines, -0 lines 0 comments Download
M Source/core/html/HTMLMediaElement.cpp View 1 2 2 chunks +17 lines, -0 lines 0 comments Download
M Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp View 1 2 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
sof
Please take a look. I don't see a way around not introducing this internal state ...
6 years, 7 months ago (2014-05-27 10:49:46 UTC) #1
haraken
I wonder why WebMediaPlayerClientImpl doesn't keep a strong member to the HTMLMediaElement (i.e., WebMediaPlayerClientImpl::m_client). If ...
6 years, 7 months ago (2014-05-27 11:06:37 UTC) #2
Mads Ager (chromium)
We have: WebCore::MediaPlayerClient* m_client; in WebMediaPlayerClientImpl.h. This can only point to an HTMLMediaElement and MediaPlayerClient ...
6 years, 7 months ago (2014-05-27 11:08:56 UTC) #3
Mads Ager (chromium)
On 2014/05/27 11:08:56, Mads Ager (chromium) wrote: > We have: > > WebCore::MediaPlayerClient* m_client; > ...
6 years, 7 months ago (2014-05-27 11:09:29 UTC) #4
sof
On 2014/05/27 11:06:37, haraken wrote: > I wonder why WebMediaPlayerClientImpl doesn't keep a strong member ...
6 years, 7 months ago (2014-05-27 11:20:29 UTC) #5
sof
On 2014/05/27 11:20:29, sof wrote: > On 2014/05/27 11:06:37, haraken wrote: > > I wonder ...
6 years, 7 months ago (2014-05-27 12:35:35 UTC) #6
sof
On 2014/05/27 12:35:35, sof wrote: > On 2014/05/27 11:20:29, sof wrote: > > On 2014/05/27 ...
6 years, 7 months ago (2014-05-27 14:43:11 UTC) #7
sof
On 2014/05/27 14:43:11, sof wrote: .. > > Will put up a patch based on ...
6 years, 7 months ago (2014-05-27 15:15:23 UTC) #8
haraken
The approach looks OK, but calling detach() in a destructor is a bit nasty. It ...
6 years, 7 months ago (2014-05-27 23:09:09 UTC) #9
sof
On 2014/05/27 23:09:09, haraken wrote: > The approach looks OK, but calling detach() in a ...
6 years, 6 months ago (2014-05-28 05:28:05 UTC) #10
sof
https://codereview.chromium.org/303593002/diff/20001/Source/web/WebMediaPlayerClientImpl.cpp File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/303593002/diff/20001/Source/web/WebMediaPlayerClientImpl.cpp#newcode152 Source/web/WebMediaPlayerClientImpl.cpp:152: ASSERT(m_client || !layer); On 2014/05/27 23:09:09, haraken wrote: > ...
6 years, 6 months ago (2014-05-28 05:28:17 UTC) #11
haraken
6 years, 6 months ago (2014-05-28 05:36:51 UTC) #12
sof
Removing any exported API requires careful consideration. I think we need a solution sooner here ...
6 years, 6 months ago (2014-05-28 06:08:34 UTC) #13
haraken
On 2014/05/28 06:08:34, sof wrote: > Removing any exported API requires careful consideration. I think ...
6 years, 6 months ago (2014-05-28 06:16:16 UTC) #14
Mads Ager (chromium)
I'm fine with that as well LGTM. Having slept on this I also think that ...
6 years, 6 months ago (2014-05-28 09:09:44 UTC) #15
sof
On 2014/05/28 09:09:44, Mads Ager (chromium) wrote: > I'm fine with that as well LGTM. ...
6 years, 6 months ago (2014-05-28 11:11:13 UTC) #16
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 6 months ago (2014-05-28 11:28:56 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/303593002/40001
6 years, 6 months ago (2014-05-28 11:29:27 UTC) #18
commit-bot: I haz the power
6 years, 6 months ago (2014-05-28 12:22:14 UTC) #19
Message was sent while issue was closed.
Change committed as 174964

Powered by Google App Engine
This is Rietveld 408576698