crash in nsTextEquivUtils::AppendTextEquivFromTextContent

RESOLVED FIXED in mozilla20

Status

()

--
critical
RESOLVED FIXED
6 years ago
6 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)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

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

Updated

6 years ago
OS: Windows NT → Windows 7
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).
Created attachment 689707 [details] [diff] [review]
null check.
Attachment #689707 - Flags: review?(trev.saunders)
(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 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 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+
(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...
(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?
(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()
(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.
(Reporter)

Comment 19

6 years ago
Created attachment 689747 [details]
Crashing testcase

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

Updated

6 years ago
Keywords: testcase
Created attachment 689754 [details] [diff] [review]
patch 3 (carrying r+ from surkov)
Attachment #689735 - Attachment is obsolete: true
(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
(Reporter)

Comment 23

6 years ago
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
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/3968fd8edddb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.