819303 - crash in nsTextEquivUtils::AppendTextEquivFromTextContent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This bug was filed from the Socorro interface and is 
report bp-6a598f1c-068a-497d-98e8-e52e22121207 .
============================================================= 

This was reported to me by Léonie Watson, who reports this consistently happening on her banking site when she's logged in. This will make it hard to reproduce on our end, but perhaps the stack is enough to provide pointers. She's using JAWS.

Comment 1

[Marco Zehe (:MarcoZ)]

Another report:
https://crash-stats.mozilla.com/report/index/6a750472-90c3-4e1e-a5de-90a3f2121207

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

GetFirstChild must be failing here?
http://hg.mozilla.org/releases/mozilla-release/annotate/c23c45132139/accessible/src/html/HTMLTableAccessible.cpp#l131

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

This may have regressed from bug 455886 where we used to try to get a shell from the content passed in and null check the shell (in AppendTextEquivFromTextContent).

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

from/since

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: null check.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #2)
> GetFirstChild must be failing here?
> http://hg.mozilla.org/releases/mozilla-release/annotate/c23c45132139/
> accessible/src/html/HTMLTableAccessible.cpp#l131

right

(In reply to David Bolter [:davidb] from comment #3)
> This may have regressed from bug 455886 where we used to try to get a shell
> from the content passed in and null check the shell (in
> AppendTextEquivFromTextContent).

эка хватил (google failed to translate, sorry for that), bug 704754 is a cause

It seems we should crash for any

<table>
<tr>
<th><abbr></abbr></th>
</tr>
</table>

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 689707 [details] [diff] [review]
null check.

please add a null check where appropriate and it'd be nice to have a test for it (similar to http://hg.mozilla.org/releases/mozilla-release/rev/e1b71596938e)

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

OK I'll move it to the caller.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: null check in caller. test included.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 689735 [details] [diff] [review]
null check in caller. test included.

Review of attachment 689735 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/HTMLTableAccessible.cpp
@@ +120,5 @@
>    nsAutoString abbrText;
>    if (ChildCount() == 1) {
>      Accessible* abbr = FirstChild();
>      if (abbr->IsAbbreviation()) {
> +      nsIContent* firstChild = abbr->GetContent()->GetFirstChild();

nit: firstChildNode, otherwise it's referred to accessible object

::: accessible/tests/mochitest/attributes/test_obj.html
@@ +98,5 @@
>        testAttrs("draggable", {"draggable" : "true"}, true);
>        testAttrs("th1", { "abbr": "SS#" }, true);
>        testAttrs("th2", { "abbr": "SS#" }, true);
>        testAttrs("th2", { "axis": "social" }, true);
> +      testAttrs("th3", { "abbr": "empty" }, true);

interesting, I would miss @abbr attribute to not have a "clean case". btw, did you try to run the test without a fix, does it crash?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #10)

> interesting, I would miss @abbr attribute to not have a "clean case". btw,
> did you try to run the test without a fix, does it crash?

to not have -> to have

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks. I'll do some more investigation before landing.

(In reply to alexander :surkov from comment #10)
> Comment on attachment 689735 [details] [diff] [review]
> null check in caller. test included.
> 
> Review of attachment 689735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/html/HTMLTableAccessible.cpp
> @@ +120,5 @@
> >    nsAutoString abbrText;
> >    if (ChildCount() == 1) {
> >      Accessible* abbr = FirstChild();
> >      if (abbr->IsAbbreviation()) {
> > +      nsIContent* firstChild = abbr->GetContent()->GetFirstChild();
> 
> nit: firstChildNode, otherwise it's referred to accessible object

Will fix.

> ::: accessible/tests/mochitest/attributes/test_obj.html
> @@ +98,5 @@
> >        testAttrs("draggable", {"draggable" : "true"}, true);
> >        testAttrs("th1", { "abbr": "SS#" }, true);
> >        testAttrs("th2", { "abbr": "SS#" }, true);
> >        testAttrs("th2", { "axis": "social" }, true);
> > +      testAttrs("th3", { "abbr": "empty" }, true);
> 
> interesting, I would miss @abbr attribute to [sic] have a "clean case".

I thought of that but wondered what the test should be... perhaps just use axis instead?

> btw,
> did you try to run the test without a fix, does it crash?

Not yet but I will. Still doing a full build here...

Comment 13

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #12)

> > interesting, I would miss @abbr attribute to [sic] have a "clean case".
> 
> I thought of that but wondered what the test should be... perhaps just use
> axis instead?

just plain <th><abbr></abbr></th> should give a crash and that's what we testing. I don't like <th abbr="empty"><abbr></abbr></th> because if one day we will change the order and will check @abbr attribute before <abbr> element then we won't be testing crash anymore

> > btw,
> > did you try to run the test without a fix, does it crash?
> 
> Not yet but I will. Still doing a full build here...

cool

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #13)
> (In reply to David Bolter [:davidb] from comment #12)
> 
> > > interesting, I would miss @abbr attribute to [sic] have a "clean case".
> > 
> > I thought of that but wondered what the test should be... perhaps just use
> > axis instead?
> 
> just plain <th><abbr></abbr></th> should give a crash and that's what we
> testing. I don't like <th abbr="empty"><abbr></abbr></th> because if one day
> we will change the order and will check @abbr attribute before <abbr>
> element then we won't be testing crash anymore

I had exactly the same thought. I'm not sure what to test though in this case though? Maybe have no check at all?

Comment 15

[Marco Zehe (:MarcoZ)]

I can reproduce this crash. https://crash-stats.mozilla.com/report/index/bp-ca563d53-9ff1-4459-af58-2d80e2121207

Comment 16

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #14)

> I had exactly the same thought. I'm not sure what to test though in this
> case though? Maybe have no check at all?

testAbsentAttrs()

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #15)
> I can reproduce this crash.
> https://crash-stats.mozilla.com/report/index/bp-ca563d53-9ff1-4459-af58-
> 2d80e2121207

known testcase, str?

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #16)
> (In reply to David Bolter [:davidb] from comment #14)
> 
> > I had exactly the same thought. I'm not sure what to test though in this
> > case though? Maybe have no check at all?
> 
> testAbsentAttrs()

That's the ticket. Thanks.

Comment 19

[Marco Zehe (:MarcoZ)]

Attachment: Crashing testcase

Just lauch this file in Nightly with NVDA running. Will crash immediately.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 3 (carrying r+ from surkov)

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #19)
> Created attachment 689747 [details]
> Crashing testcase
> 
> Just lauch this file in Nightly with NVDA running. Will crash immediately.

cool

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

https://tbpl.mozilla.org/?tree=Try&rev=b4bcaca5ef79

Comment 23

[Marco Zehe (:MarcoZ)]

Landed on Davidb's behalf on inbound after I confirmed that the try build doesn't crash:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3968fd8edddb

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3968fd8edddb