Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
799693002:
Oilpan: clear out AsyncCallChainMaps in an orderly manner. (Closed)
|
Created:
6 years ago by sof Modified:
6 years ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: clear out AsyncCallChainMaps in an orderly manner.
Follow up r186955 and avoid delaying clearing an ExecutionContextData's
AsyncCallChainMaps until the next GC, as that's not safe due to it
touching other possibly-finalized heap objects at that time. Handle
it instead when the ExecutionContextData is notified of impending
destruction instead.
R=haraken,aandrey
BUG=439376
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187025
Patch Set 1 #
Total comments: 8
Patch Set 2 : tidying #Patch Set 3 : clear->dispose #
Total comments: 5
Patch Set 4 : review update #
Total comments: 4
Patch Set 5 : tweaks #
Messages
Total messages: 22 (7 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look.
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look (if rietveld is willing..)
LGTM
aandrey@chromium.org changed reviewers: + aandrey@chromium.org
https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:77: void clear() nit: clear -> dispose https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:79: if (!m_tracker) once a clear() is called, the object is unusable. so clear() should not be called twice. https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:84: listener->didRemoveAsyncCallChain(it.value.get()); please revert, as a didRemoveAsyncCallChain() may destruct a m_tracker->m_listener object https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:623: for (auto& it : m_executionContextDataMap) { nit: no need for { }
aandrey@chromium.org changed reviewers: + yurys@chromium.org
https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:77: void clear() On 2014/12/12 08:40:52, aandrey wrote: > nit: clear -> dispose done. https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:79: if (!m_tracker) On 2014/12/12 08:40:52, aandrey wrote: > once a clear() is called, the object is unusable. so clear() should not be > called twice. Done. https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:84: listener->didRemoveAsyncCallChain(it.value.get()); On 2014/12/12 08:40:52, aandrey wrote: > please revert, as a didRemoveAsyncCallChain() may destruct a > m_tracker->m_listener object Done. https://codereview.chromium.org/799693002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:623: for (auto& it : m_executionContextDataMap) { On 2014/12/12 08:40:52, aandrey wrote: > nit: no need for { } done.
https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:77: void dispose() turns out, we also need clear(), see comment below. https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:458: data->m_executionContextTaskCallChains.dispose(); this should be clear() and leave m_executionContextTaskCallChains reusable (do not clear m_tracker).
https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:458: data->m_executionContextTaskCallChains.dispose(); On 2014/12/12 10:18:53, aandrey wrote: > this should be clear() and leave m_executionContextTaskCallChains reusable (do > not clear m_tracker). Seems like the null check in clear() wasn't such a bad idea?
https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:77: void dispose() On 2014/12/12 10:18:53, aandrey wrote: > turns out, we also need clear(), see comment below. Done. https://codereview.chromium.org/799693002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:458: data->m_executionContextTaskCallChains.dispose(); On 2014/12/12 10:18:53, aandrey wrote: > this should be clear() and leave m_executionContextTaskCallChains reusable (do > not clear m_tracker). Done.
https://codereview.chromium.org/799693002/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/799693002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:79: clear(); nit: ASSERT(m_tracker); https://codereview.chromium.org/799693002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:93: m_tracker = nullptr; remove
https://codereview.chromium.org/799693002/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/799693002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:79: clear(); On 2014/12/12 10:57:47, aandrey wrote: > nit: ASSERT(m_tracker); Seems excessive. https://codereview.chromium.org/799693002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:93: m_tracker = nullptr; On 2014/12/12 10:57:47, aandrey wrote: > remove Done.
LGTM. Thanks for fixing this!
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/799693002/80001
The CQ bit was unchecked by sigbjornf@opera.com
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/799693002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187025 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
