Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1714613003:
BodyStreamBuffer should release WebDataConsumerHandle::Reader eagerly (Closed)
DescriptionBodyStreamBuffer should release WebDataConsumerHandle::reader eagerly
WebDataConsumerHandle requires its registered client to be alive. Relying the
cleanup in the destructor is generally not safe because unreachable on-heap
objects are invalid even when they are not yet finalized.
BUG=587987, 576587
Committed: https://crrev.com/5d36d48c374ebb1ea64f8a5ab073885377db6c9b
Cr-Commit-Position: refs/heads/master@{#376524}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Messages
Total messages: 25 (11 generated)
Description was changed from ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fix the issue by making the cleanup function be called in the pre-finalizer step. BUG=587987 ========== to ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fix the issue by making the cleanup function be called in the pre-finalizer step. BUG=587987 ==========
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org, oilpan-reviews@chromium.org
Description was changed from ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fix the issue by making the cleanup function be called in the pre-finalizer step. BUG=587987 ========== to ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fix the issue by calling the cleanup function in the pre-finalizer step. BUG=587987 ==========
Description was changed from ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fix the issue by calling the cleanup function in the pre-finalizer step. BUG=587987 ========== to ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fix the issue by calling the cleanup function in the pre-finalizer step. BUG=587987, 576587 ==========
This is a speculative crash-fix. oilpan-reviews: can any of you take a look if I'm registering the pre-finalizer correctly?
Description was changed from ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fix the issue by calling the cleanup function in the pre-finalizer step. BUG=587987, 576587 ========== to ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fixes the issue by calling the cleanup function in the pre-finalizer step. BUG=587987, 576587 ==========
lgtm
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h (right): https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:28: USING_PRE_FINALIZER(BodyStreamBuffer, stop); This prefinalizer (and its registration) is correct, but the action itself isn't very interesting (wrt access of other Oilpan objects when finalizing), hence adding a smaller, but avoidable, overhead during GC. I'd argue that EAGERLY_FINALIZE() is what you want here instead. https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:74: const Member<ReadableByteStream> m_stream; ( what's the concern? )
https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h (right): https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:28: USING_PRE_FINALIZER(BodyStreamBuffer, stop); On 2016/02/19 06:17:11, sof wrote: > This prefinalizer (and its registration) is correct, but the action itself isn't > very interesting (wrt access of other Oilpan objects when finalizing), hence > adding a smaller, but avoidable, overhead during GC. > > I'd argue that EAGERLY_FINALIZE() is what you want here instead. Sorry I don't understand EAGERLY_FINALIZE: Can you tell me what it is? What is the difference between them?
On 2016/02/19 06:36:23, yhirano wrote: > https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h (right): > > https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:28: > USING_PRE_FINALIZER(BodyStreamBuffer, stop); > On 2016/02/19 06:17:11, sof wrote: > > This prefinalizer (and its registration) is correct, but the action itself > isn't > > very interesting (wrt access of other Oilpan objects when finalizing), hence > > adding a smaller, but avoidable, overhead during GC. > > > > I'd argue that EAGERLY_FINALIZE() is what you want here instead. > > Sorry I don't understand EAGERLY_FINALIZE: Can you tell me what it is? What is > the difference between them? Finalize the object upon finishing the GC, rather than wait until lazy sweeping kicks in. See MIDIAccess for an example use. Prefinalizers are chained and have to be iterated through by the GC to achieve same, but each prefinalizer has to answer yes/no if the object is still alive or not.
Description was changed from ========== BodyStreamBuffer should release WebDataConsumerHandle::reader explicitly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. This change fixes the issue by calling the cleanup function in the pre-finalizer step. BUG=587987, 576587 ========== to ========== BodyStreamBuffer should release WebDataConsumerHandle::reader eagerly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. BUG=587987, 576587 ==========
Thank you, fixed. https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h (right): https://codereview.chromium.org/1714613003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:74: const Member<ReadableByteStream> m_stream; On 2016/02/19 06:17:11, sof wrote: > ( what's the concern? ) Done.
LGTM
lgtm https://codereview.chromium.org/1714613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h (right): https://codereview.chromium.org/1714613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:28: EAGERLY_FINALIZE(); Add an explanatory comment so as to make it more understandable to the next one that comes along, // Eager finalization needed to promptly release owned FetchDataConsumerHandle objects.
https://codereview.chromium.org/1714613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h (right): https://codereview.chromium.org/1714613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:28: EAGERLY_FINALIZE(); On 2016/02/19 18:34:26, sof wrote: > Add an explanatory comment so as to make it more understandable to the next one > that comes along, > > // Eager finalization needed to promptly release owned > FetchDataConsumerHandle objects. Done.
https://codereview.chromium.org/1714613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h (right): https://codereview.chromium.org/1714613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.h:28: EAGERLY_FINALIZE(); On 2016/02/19 18:34:26, sof wrote: > Add an explanatory comment so as to make it more understandable to the next one > that comes along, > > // Eager finalization needed to promptly release owned > FetchDataConsumerHandle objects. Done.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, haraken@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1714613003/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714613003/40001
I can't view the associated bugs, but I reckon the change here will do some good. Whenever you have a "client" Blink GCed object that owns an embedder object that calls back like here, the Blink object needs to be promptly finalized unless the object has some explicit stop/teardown step before it is finalized. It's something to look out for when reviewing.
Message was sent while issue was closed.
Description was changed from ========== BodyStreamBuffer should release WebDataConsumerHandle::reader eagerly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. BUG=587987, 576587 ========== to ========== BodyStreamBuffer should release WebDataConsumerHandle::reader eagerly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. BUG=587987, 576587 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== BodyStreamBuffer should release WebDataConsumerHandle::reader eagerly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. BUG=587987, 576587 ========== to ========== BodyStreamBuffer should release WebDataConsumerHandle::reader eagerly WebDataConsumerHandle requires its registered client to be alive. Relying the cleanup in the destructor is generally not safe because unreachable on-heap objects are invalid even when they are not yet finalized. BUG=587987, 576587 Committed: https://crrev.com/5d36d48c374ebb1ea64f8a5ab073885377db6c9b Cr-Commit-Position: refs/heads/master@{#376524} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5d36d48c374ebb1ea64f8a5ab073885377db6c9b Cr-Commit-Position: refs/heads/master@{#376524} |
