Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2051053002:
Fix data race in blink::ThreadHeap::detach (Closed)
|
Created:
4 years, 6 months ago by keishi Modified:
4 years, 6 months ago CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), rouslan+autofill_chromium.org, jdonnelly+autofillwatch_chromium.org, blink-reviews, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, kouhei+heap_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix data race in blink::ThreadHeap::detach
BUG=618504
Committed: https://crrev.com/32ab2d4c4cdca33398c7af1e05c058d5f10102ab
Cr-Commit-Position: refs/heads/master@{#399126}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : fix #Messages
Total messages: 18 (4 generated)
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-review@chromium.org
PTAL
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.cpp:277: DCHECK(!thread->isMainThread() || m_threads.isEmpty()); This assert isn't accurate, DCHECK(thread->isMainThread() == m_threads.isEmpty());
https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.cpp:281: delete this; Won't this UAF when the mutex locker stack object is destructed?
https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.cpp:277: DCHECK(!thread->isMainThread() || m_threads.isEmpty()); On 2016/06/09 09:55:09, sof wrote: > This assert isn't accurate, > > DCHECK(thread->isMainThread() == m_threads.isEmpty()); The main thread must be the last thread that gets detached. If the ThreadHeap is for a per-thread-heap enabled thread, the last thread being detached is not the main thread. https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.cpp:281: delete this; On 2016/06/09 10:00:46, sof wrote: > Won't this UAF when the mutex locker stack object is destructed? I guess you're right. Changed to destruct out of the scope.
https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.cpp:277: DCHECK(!thread->isMainThread() || m_threads.isEmpty()); On 2016/06/09 10:37:25, keishi wrote: > On 2016/06/09 09:55:09, sof wrote: > > This assert isn't accurate, > > > > DCHECK(thread->isMainThread() == m_threads.isEmpty()); > > The main thread must be the last thread that gets detached. > If the ThreadHeap is for a per-thread-heap enabled thread, the last thread being > detached is not the main thread. Yes, the assert as-is doesn't verify that last condition.
https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.cpp:277: DCHECK(!thread->isMainThread() || m_threads.isEmpty()); For a per-thread-heap enabled thread: thread->isMainThread() will be false m_threads.isEmpty() will be true so thread->isMainThread() == m_threads.isEmpty() is not true. this DCHECK is just trying to do if (thread->isMainThread()) DCHECK(m_threads.isEmpty()); Maybe I should be doing this? if (thread->perThreadHeapEnabled() || thread->isMainThread()) DCHECK(m_threads.isEmpty());
https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.cpp:277: DCHECK(!thread->isMainThread() || m_threads.isEmpty()); On 2016/06/09 11:47:34, keishi wrote: > For a per-thread-heap enabled thread: > thread->isMainThread() will be false > m_threads.isEmpty() will be true > so thread->isMainThread() == m_threads.isEmpty() is not true. > > this DCHECK is just trying to do > if (thread->isMainThread()) > DCHECK(m_threads.isEmpty()); > > Maybe I should be doing this? > > if (thread->perThreadHeapEnabled() || thread->isMainThread()) > DCHECK(m_threads.isEmpty()); Yes, that condition captures the "owner" of the ThreadHeap best, i think.
On 2016/06/09 12:00:25, sof wrote: > https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/2051053002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/Heap.cpp:277: > DCHECK(!thread->isMainThread() || m_threads.isEmpty()); > On 2016/06/09 11:47:34, keishi wrote: > > For a per-thread-heap enabled thread: > > thread->isMainThread() will be false > > m_threads.isEmpty() will be true > > so thread->isMainThread() == m_threads.isEmpty() is not true. > > > > this DCHECK is just trying to do > > if (thread->isMainThread()) > > DCHECK(m_threads.isEmpty()); > > > > Maybe I should be doing this? > > > > if (thread->perThreadHeapEnabled() || thread->isMainThread()) > > DCHECK(m_threads.isEmpty()); > > Yes, that condition captures the "owner" of the ThreadHeap best, i think. OK. I've flipped the conditions around and made it // The last thread begin detached should be the owning thread, which would // be the main thread for the mainThreadHeap and a per thread heap enabled // thread otherwise. if (isLastThread) DCHECK(thread->perThreadHeapEnabled() || thread->isMainThread()); because I think its easier to understand.
lgtm
LGTM
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051053002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Fix data race in blink::ThreadHeap::detach BUG=618504 ========== to ========== Fix data race in blink::ThreadHeap::detach BUG=618504 Committed: https://crrev.com/32ab2d4c4cdca33398c7af1e05c058d5f10102ab Cr-Commit-Position: refs/heads/master@{#399126} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/32ab2d4c4cdca33398c7af1e05c058d5f10102ab Cr-Commit-Position: refs/heads/master@{#399126} |
