crash in nsTextEquivUtils::AppendTextEquivFromTextContent

RESOLVED FIXED in mozilla20

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: davidb)

Tracking

({crash, testcase})

17 Branch
mozilla20
x86
Windows 7
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

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.
Another report:
https://crash-stats.mozilla.com/report/index/6a750472-90c3-4e1e-a5de-90a3f2121207

Updated

5 years ago
OS: Windows NT → Windows 7
GetFirstChild must be failing here?
http://hg.mozilla.org/releases/mozilla-release/annotate/c23c45132139/accessible/src/html/HTMLTableAccessible.cpp#l131
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).
from/since
Created attachment 689707 [details] [diff] [review]
null check.
Attachment #689707 - Flags: review?(trev.saunders)

Comment 6

5 years ago
(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

5 years ago
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)
Attachment #689707 - Flags: review?(trev.saunders)
OK I'll move it to the caller.
Created attachment 689735 [details] [diff] [review]
null check in caller. test included.
Assignee: nobody → dbolter
Attachment #689707 - Attachment is obsolete: true
Attachment #689735 - Flags: review?(surkov.alexander)

Comment 10

5 years ago
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?
Attachment #689735 - Flags: review?(surkov.alexander) → review+

Comment 11

5 years ago
(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
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

5 years ago
(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
(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?
I can reproduce this crash. https://crash-stats.mozilla.com/report/index/bp-ca563d53-9ff1-4459-af58-2d80e2121207

Comment 16

5 years ago
(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

5 years ago
(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?
(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.
Created attachment 689747 [details]
Crashing testcase

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

Updated

5 years ago
Keywords: testcase
Created attachment 689754 [details] [diff] [review]
patch 3 (carrying r+ from surkov)
Attachment #689735 - Attachment is obsolete: true

Comment 21

5 years ago
(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
https://tbpl.mozilla.org/?tree=Try&rev=b4bcaca5ef79
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
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/3968fd8edddb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.