Closed Bug 1180189 Opened 5 years ago Closed 5 years ago

crash in mozilla::a11y::HTMLTableRowAccessible::GroupPosition()

Categories

(Core :: Disability Access APIs, defect, critical)

x86_64
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- fixed
firefox42 --- verified

People

(Reporter: MarcoZ, Assigned: fredw)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-21b5b0de-8a97-49e6-b1c4-62cd52150703.
=============================================================

I am consistently getting this crash while typing in the summary field of a new Bugzilla bug report. With NVDA on, create a new Bugzilla bug, focus the Summary field and start typing, After a few characters, a crash occurs like this. This is even with a build that contains the fix for bug 1178817.
HG Blame also implies bug 1177268, it's in the code that was last changed there, as can be seen in the first frame of the crash report.
Line 378 is nsCoreUtils::GetUIntAttr(mContent, ...) so I guess this can be avoided by checking HasOwnContent(). But as in bug 1179483, I don't know if this is the "right" way to fix this.
(note: the issue in bug 1178817 was just the line above: nsCoreUtils::GetUIntAttr(nsAccUtils::TableFor(this)->GetContent() ; because the <mtable> was not retrieved by nsAccUtils::TableFor)
Attached patch Patch (obsolete) — Splinter Review
Proposed fix (not tested).
Attachment #8629850 - Flags: review?(mzehe)
Comment on attachment 8629850 [details] [diff] [review]
Patch

Would it make sense to test for HasOwnContent first to avoid fetching the table alltogether if this check fails?
Yes, I think you are right.
Attachment #8629850 - Attachment is obsolete: true
Attachment #8629850 - Flags: review?(mzehe)
Attachment #8629866 - Flags: review?(mzehe)
Comment on attachment 8629866 [details] [diff] [review]
Add HasOwnContent on the line given by the crash report (does not fix the crash)

Unfortunately, this patch doesn't fix the crash. I still get a crash when i start typing in the Summary field of a new Bugzilla bug and NVDA is running.
Attachment #8629866 - Flags: review?(mzehe)
Attachment #8629956 - Flags: feedback?(mzehe)
Comment on attachment 8629956 [details] [diff] [review]
Patch (more NULL checks)

This patch applied to a local build no longer crashes for me. :)
Attachment #8629956 - Flags: feedback?(mzehe) → feedback+
it's quite strange to have HTMLTableRowAccessible with no content.
(In reply to alexander :surkov from comment #10)
> it's quite strange to have HTMLTableRowAccessible with no content.

Yes, I'm not sure the crash report gave the correct line given that the first attempt did not help.

MarcoZ: are you able to reduce a bit the patch to determine which checks are really needed (e.g. start removing the HasOwnContent check)?
Flags: needinfo?(mzehe)
Comment on attachment 8629956 [details] [diff] [review]
Patch (more NULL checks)

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

::: accessible/html/HTMLTableAccessible.cpp
@@ +374,5 @@
>  {
>    int32_t count = 0, index = 0;
> +  if (HasOwnContent()) {
> +    Accessible* table = nsAccUtils::TableFor(this);
> +    if (table && table->HasOwnContent() &&

If I remove the "table" null check from this line, I get the crash.
Flags: needinfo?(mzehe)
What if you remove all but this "table" null check?

It's possible that we actually forgot another case in nsAccUtils::TableFor (like math table in bug 1178817). Maybe Alex knows other possible cases for tables...
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(mzehe)
(In reply to Frédéric Wang (:fredw) from comment #13)
> What if you remove all but this "table" null check?

agree, it should be enough

> It's possible that we actually forgot another case in nsAccUtils::TableFor
> (like math table in bug 1178817). Maybe Alex knows other possible cases for
> tables...

not sure, maybe something related with overriden role on HTML table. It'd be good to check it with debugger, check mRoleMapEntry and parent chain of table row accessible.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #14)
> It'd be good to check it with debugger, check mRoleMapEntry and parent chain of table row accessible.

Agreed and a reduced test case would be nice as well.
(In reply to Frédéric Wang (:fredw) from comment #13)
> What if you remove all but this "table" null check?
 No crash. Sorry I thought my previous reply made that clear. This was the one check that, if removed, caused the crash to appear. Obviously, if that null check validates to false, the whole if block never gets executed, so all other null checks within it are pointless. And I already had ruled out the HasOwnContent() check for the actual TableRowAccessible beforehand (the outer if block).
Flags: needinfo?(mzehe)
Attachment #8629866 - Attachment description: Patch V2 → Add HasOwnContent on the line given by the crash report.
Attachment #8629866 - Attachment description: Add HasOwnContent on the line given by the crash report. → Add HasOwnContent on the line given by the crash report (does not fix the crash)
So here is the reduced patch. But it would be good to know why TableFor returns NULL.
Comment on attachment 8630906 [details] [diff] [review]
Verify that the result of TableFor is non-NULL

Yes, this looks correct from the null check standpoint. Do you want to investigate this further?
(In reply to Marco Zehe (:MarcoZ) from comment #18)
> Yes, this looks correct from the null check standpoint. Do you want to
> investigate this further?

Unfortunately, I don't know how to reproduce the problem. So unless we have a reliable reduced testcase I'm not sure I can help.
I've so far been unlucky getting a small test case. This looks like it is very specific to Bugzilla and has to do with the way Bugzilla starts to populate the possible duplicates table below the Summary field once you start typing. Without the full Bugzilla backend, there is really no exact way to reproduce this it seems.
Attachment #8630906 - Flags: review?(mzehe)
Attachment #8630906 - Flags: review?(mzehe) → review+
Took the liberty of landing this on inbound to get the bug out of the way so I can resume testing on Nightly: http://hg.mozilla.org/integration/mozilla-inbound/rev/c7866af65f2a
https://hg.mozilla.org/mozilla-central/rev/c7866af65f2a
Assignee: nobody → fred.wang
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Verified on 42 nightly, bug 41 is also affected by the original checkin.
Comment on attachment 8630906 [details] [diff] [review]
Verify that the result of TableFor is non-NULL

Approval Request Comment
[Feature/regressing bug #]: Bug 1177268
[User impact if declined]: Crash under certain circumstances when, for example, filing a new bug in Bugzilla and filling in the Summary field while a screen reader is also active, but mostly on Windows.
[Describe test coverage new/current, TreeHerder]: Local build, manual testing of nightly build.
[Risks and why]: No risk, null check. 
[String/UUID change made/needed]: None.
Attachment #8630906 - Flags: approval-mozilla-aurora?
Comment on attachment 8630906 [details] [diff] [review]
Verify that the result of TableFor is non-NULL

Not null checks are good. Let's uplift to Aurora.
Attachment #8630906 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.