Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2050463003:
Check if Member owner thread matches pointer thread and current thread (Closed)
|
Created:
4 years, 6 months ago by keishi Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, kouhei+heap_chromium.org, oilpan-reviews, Mads Ager (chromium) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck if Member owner thread matches pointer thread and current thread
BUG=591606
Committed: https://crrev.com/1ea4168e82bbba436733a26a157e3cb7e8736f19
Cr-Commit-Position: refs/heads/master@{#412765}
Patch Set 1 #Patch Set 2 : ` #Patch Set 3 : fix #Patch Set 4 : update #Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : fix #
Total comments: 20
Patch Set 8 : fix #
Total comments: 6
Patch Set 9 : fix #Patch Set 10 : fix #Patch Set 11 : fix #
Messages
Total messages: 44 (26 generated)
keishi@chromium.org changed reviewers: + haraken@chromium.org
The problems with this is that its heavy and can't lookup the thread of a memset Member.
Another verification idea would be: - When a Member/Persistent is created, remember the thread in Member/Persistent::m_creationThread. - When a Member/Persistent is accessed, check if the accessing thread is equal to Member/Persistent::m_creationThread. Will it work?
At the moment, we can ignore memset Members.
On 2016/06/08 07:43:56, haraken wrote: > Another verification idea would be: > > - When a Member/Persistent is created, remember the thread in > Member/Persistent::m_creationThread. I remember going with the hashmap workaround because I had trouble with includes and because I hit the SameSizeAs assert and I thought I need to disable all of it. But I tried it again now and it worked so I've uploaded it as PS2 Persistent has m_threadState assert so it already has this check.
LGTM On 2016/06/08 12:06:30, keishi wrote: > On 2016/06/08 07:43:56, haraken wrote: > > Another verification idea would be: > > > > - When a Member/Persistent is created, remember the thread in > > Member/Persistent::m_creationThread. > > I remember going with the hashmap workaround because I had trouble with includes > and because I hit the SameSizeAs assert and I thought I need to disable all of > it. > But I tried it again now and it worked so I've uploaded it as PS2 > > Persistent has m_threadState assert so it already has this check. The check is just checking if uninitialize is called on the same thread that called initialize. You still need to add a check to checkPointer. You can do that in a follow-up CL.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Why can't m_creationThreadHeap be derived from the page that |this| is on?
The CQ bit was checked by keishi@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 ========== [Not ready] Check if Member owner thread matches pointer thread and current thread BUG= ========== to ========== Check if Member owner thread matches pointer thread and current thread BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:149: if (tracenessConfiguration != UntracedMemberConfiguration && current) { Ignoring UntracedMember because in LayoutTests, GraphicsLayer::TakeDebugInfo gets called on the compositor thread, which calls LayoutObject::debugNode which creates a UntracedMember<Node> https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:175: void saveCreationThreadHeap() |this| may be on the stack, and if it is we can't tell it apart so I record m_creationThreadHeap.
keishi@chromium.org changed reviewers: + oilpan-reviews@chromium.org
Mostly looks good. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementRareData.cpp (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.cpp:111: #if !DCHECK_IS_ON() Can we update SameSizeAsElementRareData so that we don't need to disable the static assert in DCHECK_IS_ON? https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:133: #if !DCHECK_IS_ON() Ditto. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:16: enum TracenessMemberConfiguration { Shall we use an enum class? enum class TracedMemberConfiguration { Traced, Untraced, }; https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:25: // all Member fields of a live object will be traced marked as live as well. Remove this comment. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:149: if (tracenessConfiguration != UntracedMemberConfiguration && current) { On 2016/08/16 13:05:49, keishi wrote: > Ignoring UntracedMember because in LayoutTests, GraphicsLayer::TakeDebugInfo > gets called on the compositor thread, which calls LayoutObject::debugNode which > creates a UntracedMember<Node> 'tracenessConfiguration != UntracedMemberConfiguration' makes sense, but do we need to check 'current'? If ThreadState::current() is nullptr, Member/WeakMember should not be used whereas UntracedMember may be allowed. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:150: if (m_creationThreadHeap) { How is it possible that m_creationThreadHeap is nullptr when we reach here? https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:154: DCHECK(&ThreadState::fromObject(m_raw)->heap() == ¤t->heap()); Use DCHECK_EQ. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:178: if (ThreadState::current() && tracenessConfiguration != UntracedMemberConfiguration) Do we really need to check ThreadState::current()? As far as I understand, if ThreadState::current() is nullptr, Member/WeakMember should not be created. UntracedMember may be allowed though.
Also it would be more generic to store m_creationThreadState instead of m_creationThreadHeap.
The CQ bit was checked by keishi@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 ========== Check if Member owner thread matches pointer thread and current thread BUG= ========== to ========== Check if Member owner thread matches pointer thread and current thread BUG=591606 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementRareData.cpp (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.cpp:111: #if !DCHECK_IS_ON() On 2016/08/16 13:27:13, haraken wrote: > > Can we update SameSizeAsElementRareData so that we don't need to disable the > static assert in DCHECK_IS_ON? Done. Also renamed SameSizeAsElementRareData::persistentMember to SameSizeAsElementRareData::members because we no longer use PersistentWillBeMember. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:133: #if !DCHECK_IS_ON() On 2016/08/16 13:27:13, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:16: enum TracenessMemberConfiguration { On 2016/08/16 13:27:13, haraken wrote: > > Shall we use an enum class? > > enum class TracedMemberConfiguration { > Traced, > Untraced, > }; Done. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:25: // all Member fields of a live object will be traced marked as live as well. On 2016/08/16 13:27:13, haraken wrote: > > Remove this comment. Done. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:149: if (tracenessConfiguration != UntracedMemberConfiguration && current) { On 2016/08/16 13:27:13, haraken wrote: > On 2016/08/16 13:05:49, keishi wrote: > > Ignoring UntracedMember because in LayoutTests, GraphicsLayer::TakeDebugInfo > > gets called on the compositor thread, which calls LayoutObject::debugNode > which > > creates a UntracedMember<Node> > > 'tracenessConfiguration != UntracedMemberConfiguration' makes sense, but do we > need to check 'current'? > > If ThreadState::current() is nullptr, Member/WeakMember should not be used > whereas UntracedMember may be allowed. > Done. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:150: if (m_creationThreadHeap) { On 2016/08/16 13:27:13, haraken wrote: > > How is it possible that m_creationThreadHeap is nullptr when we reach here? I think heap collections use memset to initialize the Member and don't call the constructor. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:154: DCHECK(&ThreadState::fromObject(m_raw)->heap() == ¤t->heap()); On 2016/08/16 13:27:13, haraken wrote: > > Use DCHECK_EQ. Done. https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:178: if (ThreadState::current() && tracenessConfiguration != UntracedMemberConfiguration) On 2016/08/16 13:27:13, haraken wrote: > > Do we really need to check ThreadState::current()? > > As far as I understand, if ThreadState::current() is nullptr, Member/WeakMember > should not be created. UntracedMember may be allowed though. Done.
LGTM https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:150: if (m_creationThreadHeap) { On 2016/08/17 11:18:28, keishi wrote: > On 2016/08/16 13:27:13, haraken wrote: > > > > How is it possible that m_creationThreadHeap is nullptr when we reach here? > > I think heap collections use memset to initialize the Member and don't call the > constructor. OK, let's add a comment about it. https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:144: ThreadState* current = ThreadState::current(); Add DCHECK(current). https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:146: DCHECK_EQ(&ThreadState::fromObject(m_raw)->heap(), &m_creationThreadState->heap()); Add a developer-friendly comment to help them understand what's going on when they hit the DCHECK. https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:176: m_creationThreadState = ThreadState::current(); Add DCHECK(ThreadState::current()).
The CQ bit was checked by keishi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2050463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:150: if (m_creationThreadHeap) { On 2016/08/17 11:40:41, haraken wrote: > On 2016/08/17 11:18:28, keishi wrote: > > On 2016/08/16 13:27:13, haraken wrote: > > > > > > How is it possible that m_creationThreadHeap is nullptr when we reach here? > > > > I think heap collections use memset to initialize the Member and don't call > the > > constructor. > > OK, let's add a comment about it. Done. https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/Member.h (right): https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:144: ThreadState* current = ThreadState::current(); On 2016/08/17 11:40:41, haraken wrote: > > Add DCHECK(current). Done. https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:146: DCHECK_EQ(&ThreadState::fromObject(m_raw)->heap(), &m_creationThreadState->heap()); On 2016/08/17 11:40:41, haraken wrote: > > Add a developer-friendly comment to help them understand what's going on when > they hit the DCHECK. Done. https://codereview.chromium.org/2050463003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/Member.h:176: m_creationThreadState = ThreadState::current(); On 2016/08/17 11:40:41, haraken wrote: > > Add DCHECK(ThreadState::current()). Done.
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 keishi@chromium.org
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/2050463003/#ps180001 (title: "fix")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by keishi@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: This issue passed the CQ dry run.
The CQ bit was checked by keishi@chromium.org
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/2050463003/#ps200001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Check if Member owner thread matches pointer thread and current thread BUG=591606 ========== to ========== Check if Member owner thread matches pointer thread and current thread BUG=591606 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Check if Member owner thread matches pointer thread and current thread BUG=591606 ========== to ========== Check if Member owner thread matches pointer thread and current thread BUG=591606 Committed: https://crrev.com/1ea4168e82bbba436733a26a157e3cb7e8736f19 Cr-Commit-Position: refs/heads/master@{#412765} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/1ea4168e82bbba436733a26a157e3cb7e8736f19 Cr-Commit-Position: refs/heads/master@{#412765} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
