Assert when loading websites in windows build (debug mode)

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

[josyula, 2017-03-07 06:17:37]

venkat.nj@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[josyula, 2017-03-07 06:17:37]

Could you please take a look!

[josyula, 2017-03-07 06:41:24]

venkat.nj@samsung.com changed reviewers:
+ srirama.m@chromium.org

[Srirama, 2017-03-07 06:47:00]

srirama.m@samsung.com changed reviewers:
+ srirama.m@samsung.com

[Srirama, 2017-03-07 06:47:01]

Peer review looks good to me.

[dmazzoni, 2017-03-08 23:49:30]

not lgtm

Maybe I'm misunderstanding, but this fix doesn't look right.

The current behavior is desired; it's unusual but it matches
what Firefox does.

The DCHECK says that either ia_role should be set, or
role_name should be set.

As far as I can tell that should always hold. It's not supposed
to be necessary to set ia_role if role_name is nonempty.

[chromium-reviews, 2017-03-09 00:03:10]

I think I saw this bug before and I was puzzled as well before the DCHECK should
indeed hold.
I don’t remember of what website though. Do you have an example? I remember for
some specific objects, the role was set to None and there was no name.
I suspect that for some roles such as block quote and heading, we set the
ia2_role but not the ia_role. We set the tag-name to the HTML tag but what if
the tag comes back empty either because screen accessibility flags are off for
HTML or because there is simply no tag somehow?

-- 
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.

[josyula, 2017-03-09 13:46:32]

@marconi, @Nektarios. Thanks for the review and comments.
> The DCHECK says that either ia_role should be set, or
> role_name should be set.

> As far as I can tell that should always hold. It's not supposed
> to be necessary to set ia_role if role_name is nonempty.

Yes I agree but sometimes I can see that the role_name is empty and the ia_role
is 0.
This is when the assert happens.
In case of ui::AX_ROLE_HEADING, role_name is set to htmltag (which is empty), 
and ia_role is not set (although ia2_role is set).

> I think I saw this bug before and I was puzzled as well before the DCHECK
should
> indeed hold.
> I don’t remember of what website though. Do you have an example? I remember
for
> some specific objects, the role was set to None and there was no name.
> I suspect that for some roles such as block quote and heading, we set the
> ia2_role but not the ia_role. We set the tag-name to the HTML tag but what if
> the tag comes back empty either because screen accessibility flags are off for
> HTML or because there is simply no tag somehow?

This issue can be seen in when I build chromium dev code (debug build) on
windows and access
sites like edition.cnn.com or bbc.co.uk.
However when I tried in canary or dev, there is no issue.
Release builds anyways work well.

Please share your thoughts :)

[josyula, 2017-03-09 13:47:16]

Description was changed from

==========
Assert when loading websites in windows build

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
==========

to

==========
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
==========

[chromium-reviews, 2017-03-10 21:12:54]

I was able to reproduce this issue with the following HTML snippet:
<p><span role="heading" aria-level="1">heading</span></p>
We don't preserve the tag names of spans.
@dmazzoni do you think that we should we have a default ia_role for 
these situations or should we wire through the span tag from Blink?

@others
FYI, if you ever want to do any future debugging by logging the 
accessibility subtree rooted at a node then use the following code:
     base::string16 contents;
     auto formatter = AccessibilityTreeFormatter::Create();
formatter->SetFilters(std::vector<AccessibilityTreeFormatter::Filter>{AccessibilityTreeFormatter::Filter(L"*",

AccessibilityTreeFormatter::Filter::ALLOW)});
     formatter->FormatAccessibilityTree(this, &contents);
     LOG(ERROR) << contents;

-- 
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.

[dmazzoni, 2017-03-10 21:35:23]

ok, I see what's going on. html_tag shouldn't be empty,
but it's possible that it is sometimes, and then the
assert can get hit.

Fix it so that you only set the ia_role when the
html_tag is empty, and then the fix will be fine.

https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:5453: ia_role =
ROLE_SYSTEM_GROUPING;
This one shouldn't be needed because the role_name
will always get something nonempty

https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:5460: ia_role =
ROLE_SYSTEM_GROUPING;
How about we do this only if html_tag is empty.

Normally it won't be empty.

[josyula, 2017-03-11 05:38:17]

Thanks for the review comments.
Will upload a modified patch.

https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:5453: ia_role =
ROLE_SYSTEM_GROUPING;
On 2017/03/10 21:35:23, dmazzoni wrote:
> This one shouldn't be needed because the role_name
> will always get something nonempty

Ok.

https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:5460: ia_role =
ROLE_SYSTEM_GROUPING;
On 2017/03/10 21:35:23, dmazzoni wrote:
> How about we do this only if html_tag is empty.
> 
> Normally it won't be empty.

Ok sure. I will incorporate it.

[josyula, 2017-03-13 05:35:43]

On 2017/03/11 05:38:17, josyula wrote:
> Thanks for the review comments.
> Will upload a modified patch.
> 
>
https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
> 
>
https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.cc:5453: ia_role =
> ROLE_SYSTEM_GROUPING;
> On 2017/03/10 21:35:23, dmazzoni wrote:
> > This one shouldn't be needed because the role_name
> > will always get something nonempty
> 
> Ok.
> 
>
https://codereview.chromium.org/2731973002/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.cc:5460: ia_role =
> ROLE_SYSTEM_GROUPING;
> On 2017/03/10 21:35:23, dmazzoni wrote:
> > How about we do this only if html_tag is empty.
> > 
> > Normally it won't be empty.
> 
> Ok sure. I will incorporate it.

Incorporated the suggested changes.
Also modified the AUTHORS file since this is my first patch.
Request to please take a look.
Thanks.

[dmazzoni, 2017-03-13 16:00:06]

Thank you. The patch looks good now. Please update
the change description.

I have one more question. Please build
content_browsertests.exe and then run it with
the flag

--gtest_filter="DumpAccessibilityTreeTest.AccessibilityAriaHeading"

Does that test trigger the DCHECK failure?
If so, then note in the change description if it
fixes that test.

If not, why is that test not triggering that failure?
Can you modify the test so that it causes the failure
without this patch, and thus fixes it?

[chromium-reviews, 2017-03-13 17:07:59]

If the test is not triggering the failure, you might try modifying it by 
adding:
<div><span role="heading">Heading in span.</span></div>
to content/test/data/accessibility/aria/aria-heading.html
To ensure that test passes on all platforms I would also run "git cl 
try", wait for the bots to finish testing on all platforms and then 
update the test expectation files under the same folder.

-- 
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.

[josyula, 2017-03-14 14:54:43]

On 2017/03/13 16:00:06, dmazzoni wrote:
> Thank you. The patch looks good now. Please update
> the change description.
> 
> I have one more question. Please build
> content_browsertests.exe and then run it with
> the flag
> 
> --gtest_filter="DumpAccessibilityTreeTest.AccessibilityAriaHeading"
> 
> Does that test trigger the DCHECK failure?
> If so, then note in the change description if it
> fixes that test.
> 
> If not, why is that test not triggering that failure?
> Can you modify the test so that it causes the failure
> without this patch, and thus fixes it?

Thanks for the comments.
I have updated the change description as per suggestion.
Further, I built and ran content_browsertests.exe with the above 
suggested flag.
None of the tests do trigger the DCHECK failure.
Hence I checked for the scenario / content that can trigger the DCHECK
through content_browsertests.exe.
Observation is that as per our expectation, I can see that the 
rolename or ia_role is set always for different cases, including case
<div><span role="heading">Heading in span.</span></div>
Hence there is no  DCHECK failure in above case

Further, I tried dumping the rooted node when both rolename and 
ia_role is empty at same time (ie DCHECK is about to fail). 
Following are logs-
IA2_ROLE_UNKNOWN offset:0 auto-generated:false background-color:transparent
color:rgb(0\,0\,0) font-weight:normal font-style:normal invalid:false
language:en-US text-line-through-mode:continuous text-line-through-style:none
text-line-through-text: text-line-through-type:none text-line-through-width:auto
text-outline:false text-position:baseline text-shadow:none
text-underline-mode:continuous text-underline-style:none
text-underline-type:none text-underline-width:auto writing-mode:lr location=(56,
18) size=(121, 20) index_in_parent=1 n_relations=0 table_rows=856650440
table_columns=0 row_index=856707908 column_index=0 n_characters=0 n_selections=0
++IA2_ROLE_UNKNOWN offset:0 auto-generated:false background-color:transparent
color:rgb(0\,0\,0) font-weight:normal font-style:normal invalid:false
language:en-US text-line-through-mode:continuous text-line-through-style:none
text-line-through-text: text-line-through-type:none text-line-through-width:auto
text-outline:false text-position:baseline text-shadow:none
text-underline-mode:continuous text-underline-style:none
text-underline-type:none text-underline-width:auto writing-mode:lr location=(56,
18) size=(59, 19) index_in_parent=0 n_relations=0 table_rows=0 table_columns=0
row_index=1218616 column_index=0 n_characters=0 n_selections=0

[chromium-reviews, 2017-03-14 19:50:18]

When I run a debug build of Chrome and in the address bar type
data:text/html,<div><span role="heading">heading</span></div>
I do get the DCHECK to trigger.
I don't know why it wouldn't trigger when running content_browsertests.
I know that some bots have DCHECKs turned off but I think you are 
compiling content_browsertests locally on a Windows machine, isn't it?
Otherwise, if you are using the bots, perhaps you could try temporarily 
changing the DCHECK into a CHECK which always gets triggered regardless 
of the build type.
Personally, I am fine simply including the above test case in our 
aria-heading test stored under 
content/test/data/accessibility/aria/aria-heading.html and not worrying 
about triggering the DCHECK. We should just change the test expectations 
stored under 
content/test/data/accessibility/aria/aria-heading-expected-win.txt, to 
show that the role is no longer set to unknown.

-- 
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.

[josyula, 2017-03-16 09:45:15]

On 2017/03/14 19:50:18, chromium-reviews wrote:
> When I run a debug build of Chrome and in the address bar type
> data:text/html,<div><span role="heading">heading</span></div>
> I do get the DCHECK to trigger.
> I don't know why it wouldn't trigger when running content_browsertests.
I was unavailable yesterday. Hence the delay.
@Nekterios, Thanks for your comments and suggestions.
I have incorporated the suggestions as below.
On 2017/03/14 19:50:18, chromium-reviews wrote:
> When I run a debug build of Chrome and in the address bar type
> data:text/html,<div><span role="heading">heading</span></div>
> I do get the DCHECK to trigger.
> I don't know why it wouldn't trigger when running content_browsertests.
> I know that some bots have DCHECKs turned off but I think you are 
> compiling content_browsertests locally on a Windows machine, isn't it?

Yes I am compiling locally and running content_browsertests.
I am able to reproduce the crash when I use data scheme.
The patch further fixes this crash.

> Otherwise, if you are using the bots, perhaps you could try temporarily 
> changing the DCHECK into a CHECK which always gets triggered regardless 
> of the build type.
> Personally, I am fine simply including the above test case in our 
> aria-heading test stored under 
> content/test/data/accessibility/aria/aria-heading.html and not worrying 
> about triggering the DCHECK. We should just change the test expectations 
> stored under 
> content/test/data/accessibility/aria/aria-heading-expected-win.txt, to 
> show that the role is no longer set to unknown.
I have updated the test case and the expectations under
aria-heading-expected-win.txt
Please take a look.
Could you please let me know how to get the test expectations 
updated for mac and android.

[chromium-reviews, 2017-03-16 16:00:13]

Could you please let me know how to get the test expectations
> updated for mac and android.

I usually run
"git cl try"
wait for the results of the try bots to appear on the codereview page 
and then copy paste the expectations from the bot result pages.
Each failed test should have a separate log on the result page.

-- 
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.

[josyula, 2017-03-17 02:26:57]

The CQ bit was checked by venkat.nj@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 02:27:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 02:27:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 02:27:28]

Dry run: A disapproval has been posted by following reviewers:
dmazzoni@chromium.org.
Please make sure to get positive review before another CQ attempt.

[josyula, 2017-03-17 02:35:53]

On 2017/03/16 16:00:13, chromium-reviews wrote:
> Could you please let me know how to get the test expectations
> > updated for mac and android.
> 
> I usually run
> "git cl try"
> wait for the results of the try bots to appear on the codereview page 
> and then copy paste the expectations from the bot result pages.
> Each failed test should have a separate log on the result page.
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks for the suggestion.
I am not able to trigger "git cl try", may be due to permission ?
@dmazzoni, @@Nekterios
Could you please help me for running the 'git cl try' this time?
I shall update the test expectations for mac and android accordingly.

[nektarios, 2017-03-17 03:13:36]

The CQ bit was checked by nektar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 03:14:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 03:14:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 03:14:04]

Dry run: The author venkat.nj@samsung.com has not signed Google Contributor
License Agreement. Please visit https://cla.developers.google.com to sign and
manage CLA.

[chromium-reviews, 2017-03-17 03:14:17]

Done.

-- 
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.

[dmazzoni, 2017-03-17 03:16:07]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-03-17 03:16:08]

lgtm

Thanks!

I'll run it through the commit queue now to see if any more
test expectations need updating.

[commit-bot: I haz the power, 2017-03-17 03:16:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 03:16:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 03:16:30]

The author venkat.nj@samsung.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[josyula, 2017-03-17 14:26:19]

The CQ bit was checked by venkat.nj@samsung.com

[josyula, 2017-03-17 14:26:22]

The CQ bit was unchecked by venkat.nj@samsung.com

[josyula, 2017-03-17 14:33:06]

On 2017/03/17 03:16:30, commit-bot: I haz the power wrote:
> The author mailto:venkat.nj@samsung.com has not signed Google Contributor
License
> Agreement. Please visit https://cla.developers.google.com to sign and manage
> CLA.

CLA is done.

[josyula, 2017-03-17 14:33:37]

The CQ bit was checked by venkat.nj@samsung.com

[commit-bot: I haz the power, 2017-03-17 14:33:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nektarios, 2017-03-17 14:33:52]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-03-17 14:33:53]

lgtm

[commit-bot: I haz the power, 2017-03-17 14:34:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 15:30:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 15:30:49]

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_...)

[dmazzoni, 2017-03-17 16:13:38]

You may be able to run this script to update the test
expectations:

tools/accessibility/rebase_dump_accessibility_tree_test.py

The script runs fine from Mac & Linux. I haven't tested the
script from Windows so my apologies if it doesn't work there.

[josyula, 2017-03-18 13:53:05]

The CQ bit was checked by venkat.nj@samsung.com

[josyula, 2017-03-18 13:53:05]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2731973002/#ps80001
(title: "Assert when loading websites in windows build (debug mode)")

[commit-bot: I haz the power, 2017-03-18 13:53:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 15:22:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 15:22:22]

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_...)

[josyula, 2017-03-20 02:30:05]

The CQ bit was checked by venkat.nj@samsung.com

[josyula, 2017-03-20 02:30:06]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2731973002/#ps100001
(title: "Assert when loading websites in windows build (debug mode)")

[commit-bot: I haz the power, 2017-03-20 02:30:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 03:36:37]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489977005619510,
"parent_rev": "b75a3ecd08a0f4831ed95f1e36272676ef22e6fd", "commit_rev":
"22a053d0048c877629a0ba807290e65f730f715b"}

[commit-bot: I haz the power, 2017-03-20 03:37:39]

Description was changed from

==========
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
==========

to

==========
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/+/22a053d0048c877629a0ba807290...
==========

[commit-bot: I haz the power, 2017-03-20 03:37:40]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/22a053d0048c877629a0ba807290...