Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592063002:
SameThreadCheckedMember<>: verify same-thread usage of heap references. (Closed)
DescriptionSameThreadCheckedMember<>: verify same-thread usage of heap references.
In debug builds, Member<> checks that the heap references stored resides
in a heap belonging to the same thread as the Member<> itself. The check
carries some overhead, hence it is not enabled outside of checked builds.
In order to be able to diagnose and catch code that incorrectly stores
heap pointers belonging to other threads in a Member<>, add the
SameThreadCheckedMember<> variant of Member<>. A drop-in replacement
for Member<> that can be used to selectively diagnose.
R=
BUG=
Committed: https://crrev.com/a9f54eaa205a364f6f33260d7d768f8237a0f439
Cr-Commit-Position: refs/heads/master@{#440117}
Patch Set 1 #Patch Set 2 : clang compile fix #Patch Set 3 : clang compile fix, pt2 #
Total comments: 2
Patch Set 4 : emphasize that the extended Member is for diagnosing/debugging #
Messages
Total messages: 29 (17 generated)
The CQ bit was checked by sigbjornf@opera.com 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...
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. (any use of this extra will be landed separately in followups.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sigbjornf@opera.com 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 checked by sigbjornf@opera.com 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...
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Member.h:261: class SameThreadCheckedMember : public Member<T> { Instead of introducing SafeThreadCheckedMember and duplicating some code, would it be an option to introduce T::EnableSameThreadCheck and make Member<T> use CHECK if T::EnableSameThreadCheck is true? In this case, we can add ENABLE_SAME_THREAD_CHECK() to the MessagePort class. (Sorry if it will lead to dropping this CL...)
https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Member.h:261: class SameThreadCheckedMember : public Member<T> { On 2016/12/21 12:52:16, haraken wrote: > > Instead of introducing SafeThreadCheckedMember and duplicating some code, would > it be an option to introduce T::EnableSameThreadCheck and make Member<T> use > CHECK if T::EnableSameThreadCheck is true? In this case, we can add > ENABLE_SAME_THREAD_CHECK() to the MessagePort class. > > (Sorry if it will lead to dropping this CL...) Where would you store the "created ThreadState" ?
On 2016/12/21 12:56:36, sof wrote: > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/Member.h (right): > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/Member.h:261: class > SameThreadCheckedMember : public Member<T> { > On 2016/12/21 12:52:16, haraken wrote: > > > > Instead of introducing SafeThreadCheckedMember and duplicating some code, > would > > it be an option to introduce T::EnableSameThreadCheck and make Member<T> use > > CHECK if T::EnableSameThreadCheck is true? In this case, we can add > > ENABLE_SAME_THREAD_CHECK() to the MessagePort class. > > > > (Sorry if it will lead to dropping this CL...) > > Where would you store the "created ThreadState" ? Ah, that's a good point. (Maybe we can just drop the created ThreadState because what we really want to check here is CHECK_EQ(&ThreadState::fromObject(this->m_raw)->heap(), ¤t->heap()). But yes, there would be no reason to restrict the verification.) Would it be possible to reduce code duplication by making SameThreadMember inherit from Member?
On 2016/12/21 13:18:35, haraken wrote: > On 2016/12/21 12:56:36, sof wrote: > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/Member.h (right): > > > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/Member.h:261: class > > SameThreadCheckedMember : public Member<T> { > > On 2016/12/21 12:52:16, haraken wrote: > > > > > > Instead of introducing SafeThreadCheckedMember and duplicating some code, > > would > > > it be an option to introduce T::EnableSameThreadCheck and make Member<T> use > > > CHECK if T::EnableSameThreadCheck is true? In this case, we can add > > > ENABLE_SAME_THREAD_CHECK() to the MessagePort class. > > > > > > (Sorry if it will lead to dropping this CL...) > > > > Where would you store the "created ThreadState" ? > > Ah, that's a good point. > > (Maybe we can just drop the created ThreadState because what we really want to > check here is CHECK_EQ(&ThreadState::fromObject(this->m_raw)->heap(), > ¤t->heap()). But yes, there would be no reason to restrict the > verification.) > > Would it be possible to reduce code duplication by making SameThreadMember > inherit from Member? That's what it is already doing? :) Inheritance doesn't encompass the assignment operator really, however.
On 2016/12/21 13:23:22, sof wrote: > On 2016/12/21 13:18:35, haraken wrote: > > On 2016/12/21 12:56:36, sof wrote: > > > > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/heap/Member.h (right): > > > > > > > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/heap/Member.h:261: class > > > SameThreadCheckedMember : public Member<T> { > > > On 2016/12/21 12:52:16, haraken wrote: > > > > > > > > Instead of introducing SafeThreadCheckedMember and duplicating some code, > > > would > > > > it be an option to introduce T::EnableSameThreadCheck and make Member<T> > use > > > > CHECK if T::EnableSameThreadCheck is true? In this case, we can add > > > > ENABLE_SAME_THREAD_CHECK() to the MessagePort class. > > > > > > > > (Sorry if it will lead to dropping this CL...) > > > > > > Where would you store the "created ThreadState" ? > > > > Ah, that's a good point. > > > > (Maybe we can just drop the created ThreadState because what we really want to > > check here is CHECK_EQ(&ThreadState::fromObject(this->m_raw)->heap(), > > ¤t->heap()). But yes, there would be no reason to restrict the > > verification.) > > > > Would it be possible to reduce code duplication by making SameThreadMember > > inherit from Member? > > That's what it is already doing? :) Inheritance doesn't encompass the assignment > operator really, however. Ah, got it. LGTM. (The other concern is about introducing a new smart pointer that is not going to be widely used (i.e., not tested), but I cannot come up with any alternative.)
On 2016/12/21 13:28:56, haraken wrote: > On 2016/12/21 13:23:22, sof wrote: > > On 2016/12/21 13:18:35, haraken wrote: > > > On 2016/12/21 12:56:36, sof wrote: > > > > > > > > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/platform/heap/Member.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/heap/Member.h:261: class > > > > SameThreadCheckedMember : public Member<T> { > > > > On 2016/12/21 12:52:16, haraken wrote: > > > > > > > > > > Instead of introducing SafeThreadCheckedMember and duplicating some > code, > > > > would > > > > > it be an option to introduce T::EnableSameThreadCheck and make Member<T> > > use > > > > > CHECK if T::EnableSameThreadCheck is true? In this case, we can add > > > > > ENABLE_SAME_THREAD_CHECK() to the MessagePort class. > > > > > > > > > > (Sorry if it will lead to dropping this CL...) > > > > > > > > Where would you store the "created ThreadState" ? > > > > > > Ah, that's a good point. > > > > > > (Maybe we can just drop the created ThreadState because what we really want > to > > > check here is CHECK_EQ(&ThreadState::fromObject(this->m_raw)->heap(), > > > ¤t->heap()). But yes, there would be no reason to restrict the > > > verification.) > > > > > > Would it be possible to reduce code duplication by making SameThreadMember > > > inherit from Member? > > > > That's what it is already doing? :) Inheritance doesn't encompass the > assignment > > operator really, however. > > Ah, got it. > > LGTM. > > (The other concern is about introducing a new smart pointer that is not going to > be widely used (i.e., not tested), but I cannot come up with any alternative.) At least I'd prefer naming it MemberForDebugging or something.
On 2016/12/21 13:29:47, haraken wrote: > On 2016/12/21 13:28:56, haraken wrote: > > On 2016/12/21 13:23:22, sof wrote: > > > On 2016/12/21 13:18:35, haraken wrote: > > > > On 2016/12/21 12:56:36, sof wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/platform/heap/Member.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2592063002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/heap/Member.h:261: class > > > > > SameThreadCheckedMember : public Member<T> { > > > > > On 2016/12/21 12:52:16, haraken wrote: > > > > > > > > > > > > Instead of introducing SafeThreadCheckedMember and duplicating some > > code, > > > > > would > > > > > > it be an option to introduce T::EnableSameThreadCheck and make > Member<T> > > > use > > > > > > CHECK if T::EnableSameThreadCheck is true? In this case, we can add > > > > > > ENABLE_SAME_THREAD_CHECK() to the MessagePort class. > > > > > > > > > > > > (Sorry if it will lead to dropping this CL...) > > > > > > > > > > Where would you store the "created ThreadState" ? > > > > > > > > Ah, that's a good point. > > > > > > > > (Maybe we can just drop the created ThreadState because what we really > want > > to > > > > check here is CHECK_EQ(&ThreadState::fromObject(this->m_raw)->heap(), > > > > ¤t->heap()). But yes, there would be no reason to restrict the > > > > verification.) > > > > > > > > Would it be possible to reduce code duplication by making SameThreadMember > > > > inherit from Member? > > > > > > That's what it is already doing? :) Inheritance doesn't encompass the > > assignment > > > operator really, however. > > > > Ah, got it. > > > > LGTM. > > > > (The other concern is about introducing a new smart pointer that is not going > to > > be widely used (i.e., not tested), but I cannot come up with any alternative.) > > At least I'd prefer naming it MemberForDebugging or something. It would have to be {Something}Member<T> to follow convention.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added a comment to emphasize that this intended for temporary diagnosing, but couldn't think of a better name, so going with (the bulky) SameThreadCheckedMember<T>.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2592063002/#ps60001 (title: "emphasize that the extended Member is for diagnosing/debugging")
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": 1482330583891900,
"parent_rev": "508a4f908f592de44f3cdf054aebbd51367eafb5", "commit_rev":
"e5baff695d7475b2cb9c57cbaad70c90242c67a8"}
Message was sent while issue was closed.
Description was changed from ========== SameThreadCheckedMember<>: verify same-thread usage of heap references. In debug builds, Member<> checks that the heap references stored resides in a heap belonging to the same thread as the Member<> itself. The check carries some overhead, hence it is not enabled outside of checked builds. In order to be able to diagnose and catch code that incorrectly stores heap pointers belonging to other threads in a Member<>, add the SameThreadCheckedMember<> variant of Member<>. A drop-in replacement for Member<> that can be used to selectively diagnose. R= BUG= ========== to ========== SameThreadCheckedMember<>: verify same-thread usage of heap references. In debug builds, Member<> checks that the heap references stored resides in a heap belonging to the same thread as the Member<> itself. The check carries some overhead, hence it is not enabled outside of checked builds. In order to be able to diagnose and catch code that incorrectly stores heap pointers belonging to other threads in a Member<>, add the SameThreadCheckedMember<> variant of Member<>. A drop-in replacement for Member<> that can be used to selectively diagnose. R= BUG= Review-Url: https://codereview.chromium.org/2592063002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== SameThreadCheckedMember<>: verify same-thread usage of heap references. In debug builds, Member<> checks that the heap references stored resides in a heap belonging to the same thread as the Member<> itself. The check carries some overhead, hence it is not enabled outside of checked builds. In order to be able to diagnose and catch code that incorrectly stores heap pointers belonging to other threads in a Member<>, add the SameThreadCheckedMember<> variant of Member<>. A drop-in replacement for Member<> that can be used to selectively diagnose. R= BUG= Review-Url: https://codereview.chromium.org/2592063002 ========== to ========== SameThreadCheckedMember<>: verify same-thread usage of heap references. In debug builds, Member<> checks that the heap references stored resides in a heap belonging to the same thread as the Member<> itself. The check carries some overhead, hence it is not enabled outside of checked builds. In order to be able to diagnose and catch code that incorrectly stores heap pointers belonging to other threads in a Member<>, add the SameThreadCheckedMember<> variant of Member<>. A drop-in replacement for Member<> that can be used to selectively diagnose. R= BUG= Committed: https://crrev.com/a9f54eaa205a364f6f33260d7d768f8237a0f439 Cr-Commit-Position: refs/heads/master@{#440117} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a9f54eaa205a364f6f33260d7d768f8237a0f439 Cr-Commit-Position: refs/heads/master@{#440117} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
