Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
796863002:
Oilpan: fix build after r186944. (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: fix build after r186944.
TBR=oilpan-reviews,haraken
BUG=439376
NOTRY=true
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186955
Patch Set 1 #
Total comments: 3
Messages
Total messages: 15 (2 generated)
Please take a look.
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/796863002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=186955
Message was sent while issue was closed.
LGTM with a comment. https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : m_asyncCallChains) { Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor?
Message was sent while issue was closed.
https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : m_asyncCallChains) { On 2014/12/11 16:22:14, haraken wrote: > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor? Certainly isn't, thanks for catching. What a mess, will fix it when I get a chance.
Message was sent while issue was closed.
aandrey@chromium.org changed reviewers: + aandrey@chromium.org, yurys@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : m_asyncCallChains) { On 2014/12/11 16:27:38, sof wrote: > On 2014/12/11 16:22:14, haraken wrote: > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor? > > Certainly isn't, thanks for catching. What a mess, will fix it when I get a > chance. Could you please elaborate why it is not OK? Is it Oilpan specific?
Message was sent while issue was closed.
On 2014/12/11 20:43:30, aandrey wrote: > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > File Source/core/inspector/AsyncCallStackTracker.cpp (right): > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : > m_asyncCallChains) { > On 2014/12/11 16:27:38, sof wrote: > > On 2014/12/11 16:22:14, haraken wrote: > > > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor? > > > > Certainly isn't, thanks for catching. What a mess, will fix it when I get a > > chance. > > Could you please elaborate why it is not OK? Is it Oilpan specific? In Oilpan, destructors are not allowed to touch other on-heap objects, because a destruction order is not guaranteed in a GCed system. The other on-heap objects you're touching can be swept earlier than the destructor you're calling.
Message was sent while issue was closed.
On 2014/12/11 23:33:55, haraken wrote: > On 2014/12/11 20:43:30, aandrey wrote: > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > File Source/core/inspector/AsyncCallStackTracker.cpp (right): > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : > > m_asyncCallChains) { > > On 2014/12/11 16:27:38, sof wrote: > > > On 2014/12/11 16:22:14, haraken wrote: > > > > > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor? > > > > > > Certainly isn't, thanks for catching. What a mess, will fix it when I get a > > > chance. > > > > Could you please elaborate why it is not OK? Is it Oilpan specific? > > In Oilpan, destructors are not allowed to touch other on-heap objects, because a > destruction order is not guaranteed in a GCed system. The other on-heap objects > you're touching can be swept earlier than the destructor you're calling. Yes, I think the notification that happens during clearing&finalization can be shifted around/up a bit to avoid the situation ( https://codereview.chromium.org/799693002/ ) I should have caught this originally by inspecting the dtor, but didn't.
Message was sent while issue was closed.
On 2014/12/11 23:33:55, haraken wrote: > On 2014/12/11 20:43:30, aandrey wrote: > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > File Source/core/inspector/AsyncCallStackTracker.cpp (right): > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : > > m_asyncCallChains) { > > On 2014/12/11 16:27:38, sof wrote: > > > On 2014/12/11 16:22:14, haraken wrote: > > > > > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor? > > > > > > Certainly isn't, thanks for catching. What a mess, will fix it when I get a > > > chance. > > > > Could you please elaborate why it is not OK? Is it Oilpan specific? > > In Oilpan, destructors are not allowed to touch other on-heap objects, because a > destruction order is not guaranteed in a GCed system. The other on-heap objects > you're touching can be swept earlier than the destructor you're calling. Note that the final goal of this changes is to move AsyncCallStacks machinery along with debugger support out of Blink so that it can be reused by other projects. So this code will eventually have to be deoilpanized.
Message was sent while issue was closed.
On 2014/12/12 09:39:26, yurys wrote: > On 2014/12/11 23:33:55, haraken wrote: > > On 2014/12/11 20:43:30, aandrey wrote: > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > File Source/core/inspector/AsyncCallStackTracker.cpp (right): > > > > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : > > > m_asyncCallChains) { > > > On 2014/12/11 16:27:38, sof wrote: > > > > On 2014/12/11 16:22:14, haraken wrote: > > > > > > > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor? > > > > > > > > Certainly isn't, thanks for catching. What a mess, will fix it when I get > a > > > > chance. > > > > > > Could you please elaborate why it is not OK? Is it Oilpan specific? > > > > In Oilpan, destructors are not allowed to touch other on-heap objects, because > a > > destruction order is not guaranteed in a GCed system. The other on-heap > objects > > you're touching can be swept earlier than the destructor you're calling. > > Note that the final goal of this changes is to move AsyncCallStacks machinery > along with debugger support out of Blink so that it can be reused by other > projects. So this code will eventually have to be deoilpanized. ok, thanks for letting us know about plans - we can help out with aspects of that, if needed. Being ping'ed early about CLs that involve Oilpan in inspector/, would be very helpful to us (==oilpan-reviews). Tree breakages are nonmaskable interrupts for Oilpan'ers, so any early warnings we can get.. :)
Message was sent while issue was closed.
On 2014/12/12 09:46:10, sof wrote: > On 2014/12/12 09:39:26, yurys wrote: > > On 2014/12/11 23:33:55, haraken wrote: > > > On 2014/12/11 20:43:30, aandrey wrote: > > > > > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > > File Source/core/inspector/AsyncCallStackTracker.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > > Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : > > > > m_asyncCallChains) { > > > > On 2014/12/11 16:27:38, sof wrote: > > > > > On 2014/12/11 16:22:14, haraken wrote: > > > > > > > > > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's destructor? > > > > > > > > > > Certainly isn't, thanks for catching. What a mess, will fix it when I > get > > a > > > > > chance. > > > > > > > > Could you please elaborate why it is not OK? Is it Oilpan specific? > > > > > > In Oilpan, destructors are not allowed to touch other on-heap objects, > because > > a > > > destruction order is not guaranteed in a GCed system. The other on-heap > > objects > > > you're touching can be swept earlier than the destructor you're calling. > > > > Note that the final goal of this changes is to move AsyncCallStacks machinery > > along with debugger support out of Blink so that it can be reused by other > > projects. So this code will eventually have to be deoilpanized. > > ok, thanks for letting us know about plans - we can help out with aspects of > that, if needed. > > Being ping'ed early about CLs that involve Oilpan in inspector/, would be very > helpful to us (==oilpan-reviews). Tree breakages are nonmaskable interrupts for > Oilpan'ers, so any early warnings we can get.. :) Sorry about that, will try to be more careful about Oilpan code. Would it make sense to cc oilpan-reviews on CLs like this or what's the best way to notify you about that?
Message was sent while issue was closed.
On 2014/12/12 09:52:26, yurys wrote: > On 2014/12/12 09:46:10, sof wrote: > > On 2014/12/12 09:39:26, yurys wrote: > > > On 2014/12/11 23:33:55, haraken wrote: > > > > On 2014/12/11 20:43:30, aandrey wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > > > File Source/core/inspector/AsyncCallStackTracker.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > > > Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : > > > > > m_asyncCallChains) { > > > > > On 2014/12/11 16:27:38, sof wrote: > > > > > > On 2014/12/11 16:22:14, haraken wrote: > > > > > > > > > > > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's > destructor? > > > > > > > > > > > > Certainly isn't, thanks for catching. What a mess, will fix it when I > > get > > > a > > > > > > chance. > > > > > > > > > > Could you please elaborate why it is not OK? Is it Oilpan specific? > > > > > > > > In Oilpan, destructors are not allowed to touch other on-heap objects, > > because > > > a > > > > destruction order is not guaranteed in a GCed system. The other on-heap > > > objects > > > > you're touching can be swept earlier than the destructor you're calling. > > > > > > Note that the final goal of this changes is to move AsyncCallStacks > machinery > > > along with debugger support out of Blink so that it can be reused by other > > > projects. So this code will eventually have to be deoilpanized. > > > > ok, thanks for letting us know about plans - we can help out with aspects of > > that, if needed. > > > > Being ping'ed early about CLs that involve Oilpan in inspector/, would be very > > helpful to us (==oilpan-reviews). Tree breakages are nonmaskable interrupts > for > > Oilpan'ers, so any early warnings we can get.. :) > > Sorry about that, will try to be more careful about Oilpan code. Would it make > sense to cc oilpan-reviews on CLs like this or what's the best way to notify you > about that? Just cc oilpan-reviews :)
Message was sent while issue was closed.
On 2014/12/12 09:52:26, yurys wrote: > On 2014/12/12 09:46:10, sof wrote: > > On 2014/12/12 09:39:26, yurys wrote: > > > On 2014/12/11 23:33:55, haraken wrote: > > > > On 2014/12/11 20:43:30, aandrey wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > > > File Source/core/inspector/AsyncCallStackTracker.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/796863002/diff/1/Source/core/inspector/AsyncC... > > > > > Source/core/inspector/AsyncCallStackTracker.cpp:75: for (auto it : > > > > > m_asyncCallChains) { > > > > > On 2014/12/11 16:27:38, sof wrote: > > > > > > On 2014/12/11 16:22:14, haraken wrote: > > > > > > > > > > > > > > Is it OK to touch m_asyncCallChains in AsyncCallChainMap's > destructor? > > > > > > > > > > > > Certainly isn't, thanks for catching. What a mess, will fix it when I > > get > > > a > > > > > > chance. > > > > > > > > > > Could you please elaborate why it is not OK? Is it Oilpan specific? > > > > > > > > In Oilpan, destructors are not allowed to touch other on-heap objects, > > because > > > a > > > > destruction order is not guaranteed in a GCed system. The other on-heap > > > objects > > > > you're touching can be swept earlier than the destructor you're calling. > > > > > > Note that the final goal of this changes is to move AsyncCallStacks > machinery > > > along with debugger support out of Blink so that it can be reused by other > > > projects. So this code will eventually have to be deoilpanized. > > > > ok, thanks for letting us know about plans - we can help out with aspects of > > that, if needed. > > > > Being ping'ed early about CLs that involve Oilpan in inspector/, would be very > > helpful to us (==oilpan-reviews). Tree breakages are nonmaskable interrupts > for > > Oilpan'ers, so any early warnings we can get.. :) > > Sorry about that, will try to be more careful about Oilpan code. Would it make > sense to cc oilpan-reviews on CLs like this or what's the best way to notify you > about that? We don't want Oilpan details to block your progress in any way; Cc: oilpan-reviews on the mere suspicion that Oilpan is involved, would work for us. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
