Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2304143002:
Add speculative CHECKs to track down a failing ASSERT (Closed)
|
Created:
4 years, 3 months ago by foolip Modified:
4 years, 3 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd speculative CHECKs to track down a failing ASSERT
BUG=632848
R=eae@chromium.org
Committed: https://crrev.com/bed5e3cfe0bff0458604c83665da1d88bc38c8f4
Cr-Commit-Position: refs/heads/master@{#416620}
Patch Set 1 #
Total comments: 2
Patch Set 2 : CHECK all children #Messages
Total messages: 22 (11 generated)
The CQ bit was checked by foolip@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1166: LayoutBox* next = firstChildBox(); I don't have permission to see the bug report, but shouldn't you also check if firstChild() isBox()?
https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1166: LayoutBox* next = firstChildBox(); On 2016/09/02 18:10:35, mstensho wrote: > I don't have permission to see the bug report, but shouldn't you also check if > firstChild() isBox()? Yeah, I had CHECK(slowFirstChild->isBox()) here, but then backed it out because the failure was in fact on the nextSiblingBox call. I backed it mostly because of the risk of perf regressions from two slowFirstChild() calls, but I suppose I could rewrite it a bit to avoid that. WDYT? I'll add you to the issue too.
On 2016/09/02 18:59:48, foolip wrote: > https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): > > https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1166: LayoutBox* next > = firstChildBox(); > On 2016/09/02 18:10:35, mstensho wrote: > > I don't have permission to see the bug report, but shouldn't you also check if > > firstChild() isBox()? > > Yeah, I had CHECK(slowFirstChild->isBox()) here, but then backed it out because > the failure was in fact on the nextSiblingBox call. I backed it mostly because > of the risk of perf regressions from two slowFirstChild() calls, but I suppose I > could rewrite it a bit to avoid that. WDYT? I'll add you to the issue too. Yeah, avoiding two calls to slowFirstChild() should be easy enough. I think you should add it (kind of weird to skip checking the first child, even if no failure has been reported there), but I'm fine either way.
OK, LGTM
Description was changed from ========== Add a speculative CHECK to track down a failing ASSERT BUG=632848 R=eae@chromium.org ========== to ========== Add speculative CHECKs to track down a failing ASSERT BUG=632848 R=eae@chromium.org ==========
On 2016/09/02 19:13:36, mstensho wrote: > On 2016/09/02 18:59:48, foolip wrote: > > > https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): > > > > > https://codereview.chromium.org/2304143002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1166: LayoutBox* > next > > = firstChildBox(); > > On 2016/09/02 18:10:35, mstensho wrote: > > > I don't have permission to see the bug report, but shouldn't you also check > if > > > firstChild() isBox()? > > > > Yeah, I had CHECK(slowFirstChild->isBox()) here, but then backed it out > because > > the failure was in fact on the nextSiblingBox call. I backed it mostly because > > of the risk of perf regressions from two slowFirstChild() calls, but I suppose > I > > could rewrite it a bit to avoid that. WDYT? I'll add you to the issue too. > > Yeah, avoiding two calls to slowFirstChild() should be easy enough. I think you > should add it (kind of weird to skip checking the first child, even if no > failure has been reported there), but I'm fine either way. Can you take another look? I'm now checking all children with as little repeat work as I could manage, but I think it made things less readable. Seems OK?
lgtm, readable enough for me. :) Especially since we're not keeping it forever, right?
On 2016/09/06 08:52:17, mstensho wrote: > lgtm, readable enough for me. :) Especially since we're not keeping it forever, > right? Right, my guess is that this will go away with top layer.
The CQ bit was checked by foolip@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2304143002/#ps20001 (title: "CHECK all children")
Description was changed from ========== Add speculative CHECKs to track down a failing ASSERT BUG=632848 R=eae@chromium.org ========== to ========== Add speculative CHECKs to track down a failing ASSERT BUG=632848 R=eae@chromium.org ==========
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 ========== Add speculative CHECKs to track down a failing ASSERT BUG=632848 R=eae@chromium.org ========== to ========== Add speculative CHECKs to track down a failing ASSERT BUG=632848 R=eae@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add speculative CHECKs to track down a failing ASSERT BUG=632848 R=eae@chromium.org ========== to ========== Add speculative CHECKs to track down a failing ASSERT BUG=632848 R=eae@chromium.org Committed: https://crrev.com/bed5e3cfe0bff0458604c83665da1d88bc38c8f4 Cr-Commit-Position: refs/heads/master@{#416620} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bed5e3cfe0bff0458604c83665da1d88bc38c8f4 Cr-Commit-Position: refs/heads/master@{#416620} |
