Marco Zehe (:MarcoZ) Reporter
	Description • 9 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 1 • 9 years ago

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.

	Frédéric Wang (:fredw) Assignee
	Comment 2 • 9 years ago

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.

	Frédéric Wang (:fredw) Assignee
	Comment 3 • 9 years ago

(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)

	Frédéric Wang (:fredw) Assignee
	Comment 4 • 9 years ago

Proposed fix (not tested).

	Marco Zehe (:MarcoZ) Reporter
	Comment 5 • 9 years ago

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?

	Frédéric Wang (:fredw) Assignee
	Comment 6 • 9 years ago

Yes, I think you are right.

	Marco Zehe (:MarcoZ) Reporter
	Comment 7 • 9 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 9 • 9 years ago

Comment on attachment 8629956 [details] [diff] [review]
Patch (more NULL checks)

This patch applied to a local build no longer crashes for me. :)

	alexander :surkov (:asurkov)
	Comment 10 • 9 years ago

it's quite strange to have HTMLTableRowAccessible with no content.

	Frédéric Wang (:fredw) Assignee
	Comment 11 • 9 years ago

(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)?

	Marco Zehe (:MarcoZ) Reporter
	Comment 12 • 9 years ago

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.

	Frédéric Wang (:fredw) Assignee
	Comment 13 • 9 years ago

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...

	alexander :surkov (:asurkov)
	Comment 14 • 9 years ago

(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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 15 • 9 years ago

(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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 16 • 9 years ago

(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).

	Frédéric Wang (:fredw) Assignee
	Comment 17 • 9 years ago

So here is the reduced patch. But it would be good to know why TableFor returns NULL.

	Marco Zehe (:MarcoZ) Reporter
	Comment 18 • 9 years ago

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?

	Frédéric Wang (:fredw) Assignee
	Comment 19 • 9 years ago

(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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 20 • 9 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 21 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c7866af65f2a

	Marco Zehe (:MarcoZ) Reporter
	Comment 23 • 9 years ago

Verified on 42 nightly, bug 41 is also affected by the original checkin.

	Marco Zehe (:MarcoZ) Reporter
	Comment 24 • 9 years ago

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.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 25 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fcca0b037e1