Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567463003:
[wrapper-tracing] Fix write barrier in copy ctor (Closed)
|
Created:
4 years ago by Michael Lippautz Modified:
4 years ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, jochen (gone - plz use gerrit), sigbjorn Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wrapper-tracing] Fix write barrier in copy ctor
When copying around TraceWrapperMember, e.g., when it is dded to a backing
store, we want the write barrier because the initial trigger could've observed a
different state.
(This is how it was supposed to be when initially designed.)
BUG=chromium:468240
Committed: https://crrev.com/a7e77f9e401be15b797aaa7a9b8e9e47a3add3f6
Cr-Commit-Position: refs/heads/master@{#437674}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix swap for TraceWrapperMember #
Dependent Patchsets: Messages
Total messages: 21 (10 generated)
mlippautz@chromium.org changed reviewers: + haraken@chromium.org, hlopko@chromium.org
Complicated repro and issue with a trivial fix. Thanks for thinking this through! https://codereview.chromium.org/2567463003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h (left): https://codereview.chromium.org/2567463003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:42: template <typename U> There's no sense in a different type U here. https://codereview.chromium.org/2567463003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h (right): https://codereview.chromium.org/2567463003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/TraceWrapperMember.h:40: TraceWrapperMember(const TraceWrapperMember& other) { *this = other; } The copy ctor did not forward to the explicitly defined operator=, but to some compiler-generated one . This is because the signatures didn't match.
Complicated repro and issue with a trivial fix. Thanks for thinking this through!
lgtm
LGTM Great detective work!
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_chromeos_ozone_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_asan_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
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, hlopko@chromium.org Link to the patchset: https://codereview.chromium.org/2567463003/#ps20001 (title: "Fix swap for TraceWrapperMember")
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": 20001, "attempt_start_ts": 1481313705446010,
"parent_rev": "d4fda578198d68d6de6baa66eaca22b26619aeb6", "commit_rev":
"9c065710fa820639395627eff9d4f3b1269730de"}
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Fix write barrier in copy ctor When copying around TraceWrapperMember, e.g., when it is dded to a backing store, we want the write barrier because the initial trigger could've observed a different state. (This is how it was supposed to be when initially designed.) BUG=chromium:468240 ========== to ========== [wrapper-tracing] Fix write barrier in copy ctor When copying around TraceWrapperMember, e.g., when it is dded to a backing store, we want the write barrier because the initial trigger could've observed a different state. (This is how it was supposed to be when initially designed.) BUG=chromium:468240 Review-Url: https://codereview.chromium.org/2567463003 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Fix write barrier in copy ctor When copying around TraceWrapperMember, e.g., when it is dded to a backing store, we want the write barrier because the initial trigger could've observed a different state. (This is how it was supposed to be when initially designed.) BUG=chromium:468240 Review-Url: https://codereview.chromium.org/2567463003 ========== to ========== [wrapper-tracing] Fix write barrier in copy ctor When copying around TraceWrapperMember, e.g., when it is dded to a backing store, we want the write barrier because the initial trigger could've observed a different state. (This is how it was supposed to be when initially designed.) BUG=chromium:468240 Committed: https://crrev.com/a7e77f9e401be15b797aaa7a9b8e9e47a3add3f6 Cr-Commit-Position: refs/heads/master@{#437674} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a7e77f9e401be15b797aaa7a9b8e9e47a3add3f6 Cr-Commit-Position: refs/heads/master@{#437674} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
