Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871143002:
BodyStreamBuffer: Avoid calling into V8 during construction (Closed)
|
Created:
3 years, 7 months ago by Michael Lippautz Modified:
3 years, 7 months ago Reviewers:
haraken CC:
chromium-reviews, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBodyStreamBuffer: Avoid calling into V8 during construction
::toImpl ca call AdjustAmountOfExternalMemory for ArrayBuffers which can
trigger a V8 GC. Avoid being in the constructor when creating a
BodyStreamBuffer in Response creation.
BUG=chromium:716364
Review-Url: https://codereview.chromium.org/2871143002
Cr-Commit-Position: refs/heads/master@{#470888}
Committed: https://chromium.googlesource.com/chromium/src/+/e563fd2fa55fd2d327c0f44bcfe6c4c98805bb55
Patch Set 1 #
Total comments: 2
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Response: Avoid calling into V8 during construction ::toImpl ca call AdjustAmountOfExternalMemory for ArrayBuffers which can trigger a V8 GC. Avoid being in the constructor when creating a BodyStreamBuffer in Response creation. BUG=chromium:716364 ========== to ========== BodyStreamBuffer: Avoid calling into V8 during construction ::toImpl ca call AdjustAmountOfExternalMemory for ArrayBuffers which can trigger a V8 GC. Avoid being in the constructor when creating a BodyStreamBuffer in Response creation. BUG=chromium:716364 ==========
mlippautz@chromium.org changed reviewers: + haraken@chromium.org
ptal. one of the defensive CHECKs fired in the attached bug.
https://codereview.chromium.org/2871143002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/Response.cpp (left): https://codereview.chromium.org/2871143002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Response.cpp:153: V8ArrayBuffer::toImpl(body.As<v8::Object>()))); Isn't it guaranteed that V8ArrayBuffer::toImpl(body.As<v8::Object>()) gets called before the construction of FormDataBytesConsumer starts?
I could not reproduce with this fix. Medium-term, I would like to get to the point where we can start wrapper tracing from any point but only make progress in actual tracing from points where it is safe. We would not crash anymore but just trace trace slightly more in the follow up steps or the final pause of the V8 GC, which always happens to be at a safe point. I don't want to backmerge such a CL though :) https://codereview.chromium.org/2871143002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/Response.cpp (left): https://codereview.chromium.org/2871143002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/Response.cpp:153: V8ArrayBuffer::toImpl(body.As<v8::Object>()))); On 2017/05/10 09:05:16, haraken wrote: > > Isn't it guaranteed that V8ArrayBuffer::toImpl(body.As<v8::Object>()) gets > called before the construction of FormDataBytesConsumer starts? My C++ foo is not strong enough to answer this by pointing at the spec. What I see from debugging though is that the MixinScope gets opened right before evaluating the parameter. So I assume that an uninitialized object is created before evaluating the parameter.
On 2017/05/10 10:51:25, Michael Lippautz wrote: > I could not reproduce with this fix. > > Medium-term, I would like to get to the point where we can start wrapper tracing > from any point but only make progress in actual tracing from points where it is > safe. We would not crash anymore but just trace trace slightly more in the > follow up steps or the final pause of the V8 GC, which always happens to be at a > safe point. I don't want to backmerge such a CL though :) > > https://codereview.chromium.org/2871143002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/fetch/Response.cpp (left): > > https://codereview.chromium.org/2871143002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/Response.cpp:153: > V8ArrayBuffer::toImpl(body.As<v8::Object>()))); > On 2017/05/10 09:05:16, haraken wrote: > > > > Isn't it guaranteed that V8ArrayBuffer::toImpl(body.As<v8::Object>()) gets > > called before the construction of FormDataBytesConsumer starts? > > My C++ foo is not strong enough to answer this by pointing at the spec. What I > see from debugging though is that the MixinScope gets opened right before > evaluating the parameter. > > So I assume that an uninitialized object is created before evaluating the > parameter. LGTM
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1494490758564060, "parent_rev":
"b75611794c4ee0538c0f62aaf75abaa409d8963d", "commit_rev":
"e563fd2fa55fd2d327c0f44bcfe6c4c98805bb55"}
Message was sent while issue was closed.
Description was changed from ========== BodyStreamBuffer: Avoid calling into V8 during construction ::toImpl ca call AdjustAmountOfExternalMemory for ArrayBuffers which can trigger a V8 GC. Avoid being in the constructor when creating a BodyStreamBuffer in Response creation. BUG=chromium:716364 ========== to ========== BodyStreamBuffer: Avoid calling into V8 during construction ::toImpl ca call AdjustAmountOfExternalMemory for ArrayBuffers which can trigger a V8 GC. Avoid being in the constructor when creating a BodyStreamBuffer in Response creation. BUG=chromium:716364 Review-Url: https://codereview.chromium.org/2871143002 Cr-Commit-Position: refs/heads/master@{#470888} Committed: https://chromium.googlesource.com/chromium/src/+/e563fd2fa55fd2d327c0f44bcfe6... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/e563fd2fa55fd2d327c0f44bcfe6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
