Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2884923002:
Make DCHECK of range shape result more verbose (Closed)
|
Created:
3 years, 7 months ago by kojii Modified:
3 years, 6 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, fmalita+watch_chromium.org, glebl+reviews_chromium.org, jchaffraix+rendering, Justin Novosad, kinuko+watch, leviw+renderwatch, ojan+watch_chromium.org, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake DCHECK of range shape result more verbose
The DCHECK failure in TwoClientAppsSyncTest.UninstallThenInstall_E2ETest
is not reproducible locally.
This patch does not fix, but will let us know whether the length is
wrong or the index is wrong. We can add more debug logs by figuring this
out.
BUG=722462
Review-Url: https://codereview.chromium.org/2884923002
Cr-Commit-Position: refs/heads/master@{#476594}
Committed: https://chromium.googlesource.com/chromium/src/+/b84dddac87497b87173c8860647477337d611031
Patch Set 1 #Patch Set 2 : Make DCHECK more verbose #Messages
Total messages: 36 (25 generated)
Description was changed from ========== hberr BUG= ========== to ========== hberr BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was checked by kojii@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 ========== hberr BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== Move DCHECK of range shape result from HarfBuzzShaper::Shape to NGInlineNode This patch moves the DCHECK from HarfBuzzShaper::Shape to NGInlineNode because TwoClientAppsSyncTest.UninstallThenInstall_E2ETest failed the DCHECK. While the exact reason is not determined, we expect to learn better as LayoutNG test coverage increases. BUG=722462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
kojii@chromium.org changed reviewers: + eae@chromium.org
PTAL. WDYT? I'm interested in understanding in what condition it can fail, but don't know how to setup sync server etc. If we can't reproduce in any other ways until we ship NG, hmm...consider it's rare enough, or try to reproduce using sync server?
On 2017/05/16 15:50:49, kojii wrote: > PTAL. WDYT? I'm interested in understanding in what condition it can fail, but > don't know how to setup sync server etc. If we can't reproduce in any other ways > until we ship NG, hmm...consider it's rare enough, or try to reproduce using > sync server? It would be good to understand why it is failing, perhaps the sync team can help?
The CQ bit was checked by kojii@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 checked by kojii@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
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 ========== Move DCHECK of range shape result from HarfBuzzShaper::Shape to NGInlineNode This patch moves the DCHECK from HarfBuzzShaper::Shape to NGInlineNode because TwoClientAppsSyncTest.UninstallThenInstall_E2ETest failed the DCHECK. While the exact reason is not determined, we expect to learn better as LayoutNG test coverage increases. BUG=722462 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== Make DCHECK of range shape result more verbose The DCHECK failure in TwoClientAppsSyncTest.UninstallThenInstall_E2ETest is not reproducible locally. This patch does not fix, but will let us know whether the length is wrong or the index is wrong. We can add more debug logs by figuring this out. BUG=722462 ==========
kojii@chromium.org changed reviewers: + wylieb@chromium.org
Commented in crbug.com/722462 but I'm not able to reproduce locally. Since a week has past, I'm leaning to add more logs to get info from the test bot. Is this ok?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
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.
On 2017/05/29 07:27:27, kojii wrote: > Commented in crbug.com/722462 but I'm not able to reproduce locally. > > Since a week has past, I'm leaning to add more logs to get info from the test > bot. Is this ok? Sounds good to me. Thank you for staying on top of this.
mind to sign off with the keyword? ;)
Fyi I don't think this issue is gone, just masked by chrome://chrome-signin being broken on ToT+Mac. On Tue, May 30, 2017 at 11:28 AM, <kojii@chromium.org> wrote: > mind to sign off with the keyword? ;) > > https://codereview.chromium.org/2884923002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Fyi I don't think this issue is gone, just masked by chrome://chrome-signin being broken on ToT+Mac. On Tue, May 30, 2017 at 11:28 AM, <kojii@chromium.org> wrote: > mind to sign off with the keyword? ;) > > https://codereview.chromium.org/2884923002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM
wylieb@google.com changed reviewers: + wylieb@google.com
lgtm
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496380432620690,
"parent_rev": "05381409884cc4b9b1e15eeeff4dc624088b9ab3", "commit_rev":
"b84dddac87497b87173c8860647477337d611031"}
Message was sent while issue was closed.
Description was changed from ========== Make DCHECK of range shape result more verbose The DCHECK failure in TwoClientAppsSyncTest.UninstallThenInstall_E2ETest is not reproducible locally. This patch does not fix, but will let us know whether the length is wrong or the index is wrong. We can add more debug logs by figuring this out. BUG=722462 ========== to ========== Make DCHECK of range shape result more verbose The DCHECK failure in TwoClientAppsSyncTest.UninstallThenInstall_E2ETest is not reproducible locally. This patch does not fix, but will let us know whether the length is wrong or the index is wrong. We can add more debug logs by figuring this out. BUG=722462 Review-Url: https://codereview.chromium.org/2884923002 Cr-Commit-Position: refs/heads/master@{#476594} Committed: https://chromium.googlesource.com/chromium/src/+/b84dddac87497b87173c88606474... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b84dddac87497b87173c88606474... |
