Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(117)

Issue 2731973002: Assert when loading websites in windows build (debug mode) (Closed)

Created:
3 years, 9 months ago by josyula
Modified:
3 years, 9 months ago
CC:
aboxhall+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Assert when loading websites in windows build (debug mode) When page is loaded, ia_role is not set (value is 0) when the current UX role is AX_ROLE_HEADING. Similarly when htmltag is empty, we set the role_name as "div" but not ia_role. This causes an assert in Windows debug build in the following line DCHECK(!role_name.empty() || ia_role); BUG=698728 Review-Url: https://codereview.chromium.org/2731973002 Cr-Commit-Position: refs/heads/master@{#458014} Committed: https://chromium.googlesource.com/chromium/src/+/22a053d0048c877629a0ba807290e65f730f715b

Patch Set 1 #

Total comments: 4

Patch Set 2 : Assert when loading websites in windows build (debug mode) #

Patch Set 3 : Assert when loading websites in windows build (debug mode) #

Patch Set 4 : Assert when loading websites in windows build (debug mode) #

Patch Set 5 : Assert when loading websites in windows build (debug mode) #

Patch Set 6 : Assert when loading websites in windows build (debug mode) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -1 line) Patch
M AUTHORS View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_win.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M content/test/data/accessibility/aria/aria-heading.html View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M content/test/data/accessibility/aria/aria-heading-expected-android.txt View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M content/test/data/accessibility/aria/aria-heading-expected-mac.txt View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M content/test/data/accessibility/aria/aria-heading-expected-win.txt View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 57 (26 generated)
josyula
Could you please take a look!
3 years, 9 months ago (2017-03-07 06:17:37 UTC) #2
josyula
3 years, 9 months ago (2017-03-07 06:41:24 UTC) #4
Srirama
Peer review looks good to me.
3 years, 9 months ago (2017-03-07 06:47:01 UTC) #6
dmazzoni
not lgtm Maybe I'm misunderstanding, but this fix doesn't look right. The current behavior is ...
3 years, 9 months ago (2017-03-08 23:49:30 UTC) #7
chromium-reviews
I think I saw this bug before and I was puzzled as well before the ...
3 years, 9 months ago (2017-03-09 00:03:10 UTC) #8
josyula
@marconi, @Nektarios. Thanks for the review and comments. > The DCHECK says that either ia_role ...
3 years, 9 months ago (2017-03-09 13:46:32 UTC) #9
chromium-reviews
I was able to reproduce this issue with the following HTML snippet: <p><span role="heading" aria-level="1">heading</span></p> ...
3 years, 9 months ago (2017-03-10 21:12:54 UTC) #11
dmazzoni
ok, I see what's going on. html_tag shouldn't be empty, but it's possible that it ...
3 years, 9 months ago (2017-03-10 21:35:23 UTC) #12
josyula
Thanks for the review comments. Will upload a modified patch. https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibility/browser_accessibility_win.cc File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibility/browser_accessibility_win.cc#newcode5453 ...
3 years, 9 months ago (2017-03-11 05:38:17 UTC) #13
josyula
On 2017/03/11 05:38:17, josyula wrote: > Thanks for the review comments. > Will upload a ...
3 years, 9 months ago (2017-03-13 05:35:43 UTC) #14
dmazzoni
Thank you. The patch looks good now. Please update the change description. I have one ...
3 years, 9 months ago (2017-03-13 16:00:06 UTC) #15
chromium-reviews
If the test is not triggering the failure, you might try modifying it by adding: ...
3 years, 9 months ago (2017-03-13 17:07:59 UTC) #16
josyula
On 2017/03/13 16:00:06, dmazzoni wrote: > Thank you. The patch looks good now. Please update ...
3 years, 9 months ago (2017-03-14 14:54:43 UTC) #17
chromium-reviews
When I run a debug build of Chrome and in the address bar type data:text/html,<div><span ...
3 years, 9 months ago (2017-03-14 19:50:18 UTC) #18
josyula
On 2017/03/14 19:50:18, chromium-reviews wrote: > When I run a debug build of Chrome and ...
3 years, 9 months ago (2017-03-16 09:45:15 UTC) #19
chromium-reviews
Could you please let me know how to get the test expectations > updated for ...
3 years, 9 months ago (2017-03-16 16:00:13 UTC) #20
josyula
On 2017/03/16 16:00:13, chromium-reviews wrote: > Could you please let me know how to get ...
3 years, 9 months ago (2017-03-17 02:35:53 UTC) #25
chromium-reviews
Done. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" ...
3 years, 9 months ago (2017-03-17 03:14:17 UTC) #30
dmazzoni
lgtm Thanks! I'll run it through the commit queue now to see if any more ...
3 years, 9 months ago (2017-03-17 03:16:08 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731973002/60001
3 years, 9 months ago (2017-03-17 03:16:26 UTC) #33
commit-bot: I haz the power
The author venkat.nj@samsung.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign ...
3 years, 9 months ago (2017-03-17 03:16:30 UTC) #35
josyula
On 2017/03/17 03:16:30, commit-bot: I haz the power wrote: > The author mailto:venkat.nj@samsung.com has not ...
3 years, 9 months ago (2017-03-17 14:33:06 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731973002/60001
3 years, 9 months ago (2017-03-17 14:33:51 UTC) #40
nektarios
lgtm
3 years, 9 months ago (2017-03-17 14:33:53 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731973002/60001
3 years, 9 months ago (2017-03-17 14:34:13 UTC) #43
commit-bot: I haz the power
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_ng/builds/409227)
3 years, 9 months ago (2017-03-17 15:30:49 UTC) #45
dmazzoni
You may be able to run this script to update the test expectations: tools/accessibility/rebase_dump_accessibility_tree_test.py The ...
3 years, 9 months ago (2017-03-17 16:13:38 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731973002/80001
3 years, 9 months ago (2017-03-18 13:53:14 UTC) #49
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/331281)
3 years, 9 months ago (2017-03-18 15:22:22 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731973002/100001
3 years, 9 months ago (2017-03-20 02:30:20 UTC) #54
commit-bot: I haz the power
3 years, 9 months ago (2017-03-20 03:37:40 UTC) #57
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/22a053d0048c877629a0ba807290...

Powered by Google App Engine
This is Rietveld 408576698