Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1184373006:
Oilpan: add prefinalizer for HTMLMediaElement. (Closed)
|
Created:
5 years, 6 months ago by sof Modified:
5 years, 6 months ago CC:
blink-reviews, nessy, mlamouri+watch-blink_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews-html_chromium.org, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: add prefinalizer for HTMLMediaElement.
A prefinalizer is needed for the media element for the case where it is
finalized in the same GC as its execution context (Document.) The
interaction between the media element and the Document to synchronize
load event handling can only safely be done prior to either is swept
out and finalized.
R=
BUG=500352
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197339
Patch Set 1 #Patch Set 2 : move out a potentially-allocating action from prefinalizer #
Total comments: 3
Messages
Total messages: 17 (5 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. Finalization of this element is not the most straightforward & regular. As prefinalizers cannot allocate, we can only delegate some of the finalization steps to dispose().
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:390: if (m_closeMediaSourceWhenFinalizing) I guess this hack could be moved to the pre-finalizer as well, but we can do this in a follow-up.
https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:390: if (m_closeMediaSourceWhenFinalizing) On 2015/06/17 20:29:02, haraken wrote: > > I guess this hack could be moved to the pre-finalizer as well, but we can do > this in a follow-up. I don't think it can be, as it allocates & fires an event.
https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:390: if (m_closeMediaSourceWhenFinalizing) On 2015/06/17 20:40:46, sof wrote: > On 2015/06/17 20:29:02, haraken wrote: > > > > I guess this hack could be moved to the pre-finalizer as well, but we can do > > this in a follow-up. > > I don't think it can be, as it allocates & fires an event. Allocation is allowed in pre-finalizers. closeMediaSource schedules an event (i.e., MediaSource::onReadyStateChange enqueues an event) but it doesn't dispatch the event in closeMediaSource. (If it does, it would be already problematic because it's dangerous to dispatch an event in a destructor.)
On 2015/06/17 20:47:15, haraken wrote: > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > Source/core/html/HTMLMediaElement.cpp:390: if (m_closeMediaSourceWhenFinalizing) > On 2015/06/17 20:40:46, sof wrote: > > On 2015/06/17 20:29:02, haraken wrote: > > > > > > I guess this hack could be moved to the pre-finalizer as well, but we can do > > > this in a follow-up. > > > > I don't think it can be, as it allocates & fires an event. > > Allocation is allowed in pre-finalizers. > Really? Doesn't invokePreFinalizers() run within a NoAllocationScope ?
On 2015/06/17 20:49:17, sof wrote: > On 2015/06/17 20:47:15, haraken wrote: > > > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > > File Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > > Source/core/html/HTMLMediaElement.cpp:390: if > (m_closeMediaSourceWhenFinalizing) > > On 2015/06/17 20:40:46, sof wrote: > > > On 2015/06/17 20:29:02, haraken wrote: > > > > > > > > I guess this hack could be moved to the pre-finalizer as well, but we can > do > > > > this in a follow-up. > > > > > > I don't think it can be, as it allocates & fires an event. > > > > Allocation is allowed in pre-finalizers. > > > > Really? Doesn't invokePreFinalizers() run within a NoAllocationScope ? Yeah. Hmm, I don't understand why we need to forbid allocation during pre-finalizers. Given that we already allow allocations during destructors, I guess there would be no issue in allowing allocations during pre-finalizers...
On 2015/06/17 20:53:07, haraken wrote: > On 2015/06/17 20:49:17, sof wrote: > > On 2015/06/17 20:47:15, haraken wrote: > > > > > > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > > > File Source/core/html/HTMLMediaElement.cpp (right): > > > > > > > > > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > > > Source/core/html/HTMLMediaElement.cpp:390: if > > (m_closeMediaSourceWhenFinalizing) > > > On 2015/06/17 20:40:46, sof wrote: > > > > On 2015/06/17 20:29:02, haraken wrote: > > > > > > > > > > I guess this hack could be moved to the pre-finalizer as well, but we > can > > do > > > > > this in a follow-up. > > > > > > > > I don't think it can be, as it allocates & fires an event. > > > > > > Allocation is allowed in pre-finalizers. > > > > > > > Really? Doesn't invokePreFinalizers() run within a NoAllocationScope ? > > Yeah. Hmm, I don't understand why we need to forbid allocation during > pre-finalizers. Given that we already allow allocations during destructors, I > guess there would be no issue in allowing allocations during pre-finalizers... I sure would appreciate it if you could check details before counter-claiming..
On 2015/06/17 20:58:49, sof wrote: > On 2015/06/17 20:53:07, haraken wrote: > > On 2015/06/17 20:49:17, sof wrote: > > > On 2015/06/17 20:47:15, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > > > > File Source/core/html/HTMLMediaElement.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1184373006/diff/20001/Source/core/html/HTMLMe... > > > > Source/core/html/HTMLMediaElement.cpp:390: if > > > (m_closeMediaSourceWhenFinalizing) > > > > On 2015/06/17 20:40:46, sof wrote: > > > > > On 2015/06/17 20:29:02, haraken wrote: > > > > > > > > > > > > I guess this hack could be moved to the pre-finalizer as well, but we > > can > > > do > > > > > > this in a follow-up. > > > > > > > > > > I don't think it can be, as it allocates & fires an event. > > > > > > > > Allocation is allowed in pre-finalizers. > > > > > > > > > > Really? Doesn't invokePreFinalizers() run within a NoAllocationScope ? > > > > Yeah. Hmm, I don't understand why we need to forbid allocation during > > pre-finalizers. Given that we already allow allocations during destructors, I > > guess there would be no issue in allowing allocations during pre-finalizers... > > I sure would appreciate it if you could check details before counter-claiming.. I think: - Allocation is not allowed in global weak processing, because pages to be swept are not yet moved to a list of m_firstUnsweptPage. - Allocation is allowed in thread-specific weak processing, because pages to be swept has already been moved to a list of m_firstUnsweptPage. However, it's not allowed to mutate the object graph in a way in which a dead object gets resurrected. Also it's not allowed to mutate a HashTable because HashTable's weak processing assumes that there has been no mutation since the marking phase. In short, it's complicated, so it would be safe to just disallow allocations. - Allocation is allowed in pre-finalizers, because pages to be swept has already been moved to a list of m_firstUnsweptPage. However, it's not allowed to mutate the object graph in a way in which a dead object gets resurrected. This is the same restriction as destructors. I'll try to remove NoAllocationScope from pre-finalizers. This CL looks good either way.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1184373006/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59426)
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1184373006/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197339 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
