Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2526293002:
[wrapper-tracing] Untangle non-trivial mixin ctors (Closed)
|
Created:
4 years ago by Michael Lippautz Modified:
4 years ago Reviewers:
haraken CC:
chromium-reviews, blink-reviews, jochen (gone - plz use gerrit), hlopko Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wrapper-tracing] Untangle non-trivial mixin ctors
Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins
that could trigger V8 GCs.
BUG=chromium:668059, chromium:668164
Committed: https://crrev.com/03e5b7ab69b0d8f4dce70b930db57cc9bdddfb8d
Cr-Commit-Position: refs/heads/master@{#434466}
Patch Set 1 : Refactor Request #Patch Set 2 : Refactor BodyStreamBuffer #Patch Set 3 : s/new BodyStreamBuffer/BodyStreamBuffer::create/ #
Total comments: 2
Patch Set 4 : Revet BodyStreamBufffer changes #
Messages
Total messages: 24 (17 generated)
Description was changed from ========== [wrapper-tracing] Untangle non-trivial mixin ctors BUG= ========== to ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059 ==========
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...
Description was changed from ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059 ========== to ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059, chromium:668164 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mlippautz@chromium.org changed reviewers: + haraken@chromium.org
Please take a close look. This should fix the issue with TracePrologue we see on
Canary and some bots. It might be a good idea to add
CHECK(!ThreadState::current()->isGCForbidden());
to all API entry points, since any V8 allocation can also trigger an incremental
marking step.
Description was changed from ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059, chromium:668164 ========== to ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059,chromium:668164 ==========
On 2016/11/24 18:41:56, Michael Lippautz wrote: > Please take a close look. This should fix the issue with TracePrologue we see on > Canary and some bots. It might be a good idea to add > CHECK(!ThreadState::current()->isGCForbidden()); > to all API entry points, since any V8 allocation can also trigger an incremental > marking step. Yeah, agreed. Let's do that. https://codereview.chromium.org/2526293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (left): https://codereview.chromium.org/2526293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:84: v8::Local<v8::Value> bodyValue = toV8(this, scriptState); Why do we need to move the contents of the constructor to ::create? BodyStreamBuffer::BodyStreamBuffer() : ... { // <--- Object construction is complete at this point. ...; // <-- So it's fine to call V8 APIs here. }
Thanks! Reverted back to just handling FetchRequestData properly. In any case, this scenario is responsible for _all_ of the TracePrologue crashers I looked at. https://codereview.chromium.org/2526293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (left): https://codereview.chromium.org/2526293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:84: v8::Local<v8::Value> bodyValue = toV8(this, scriptState); On 2016/11/24 23:36:32, haraken wrote: > > Why do we need to move the contents of the constructor to ::create? > > BodyStreamBuffer::BodyStreamBuffer() > : ... { // <--- Object construction is complete at this point. > ...; // <-- So it's fine to call V8 APIs here. > } Thanks for the clarification, makes sense.
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...
LGTM
Description was changed from ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059,chromium:668164 ========== to ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059,chromium:668164 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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": 60001, "attempt_start_ts": 1480065885314880,
"parent_rev": "1640e408f6675d857ebaba84fd1aa4fd80b44f85", "commit_rev":
"2ce182d83b623279f01657b9de4f617603e228a8"}
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059,chromium:668164 ========== to ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059,chromium:668164 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059,chromium:668164 ========== to ========== [wrapper-tracing] Untangle non-trivial mixin ctors Refactor constructors to corresponding ::create methods of non-trivial Oilpan mixins that could trigger V8 GCs. BUG=chromium:668059,chromium:668164 Committed: https://crrev.com/03e5b7ab69b0d8f4dce70b930db57cc9bdddfb8d Cr-Commit-Position: refs/heads/master@{#434466} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/03e5b7ab69b0d8f4dce70b930db57cc9bdddfb8d Cr-Commit-Position: refs/heads/master@{#434466} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
