Closed Bug 819303 Opened 9 years ago Closed 9 years ago

crash in nsTextEquivUtils::AppendTextEquivFromTextContent

Categories

(Core :: Disability Access APIs, defect)

17 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: MarcoZ, Assigned: davidb)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files, 2 obsolete files)

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.
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).
Attached patch null check. (obsolete) — Splinter Review
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.
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.
Attached file Crashing testcase
Just lauch this file in Nightly with NVDA running. Will crash immediately.
Keywords: testcase
(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
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.