765172 - Crash during a11y/accessible/focus/test_takeFocus.html | Exited with code -1073741819 during test run [@ Accessible::GetARIAName(nsAString_internal &)]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mike Hommey [:glandium]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=12689828&full=1&branch=mozilla-central

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/focus/test_takeFocus.html | Exited with code -1073741819 during test run
INFO | automation.py | Application ran for: 0:02:10.751000
INFO | automation.py | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpyadbsqpidlog
==> process 212 launched child process 3460
INFO | automation.py | Checking for orphan process with PID: 3460
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1339741465/firefox-16.0a1.en-US.win32.crashreporter-symbols.zip
PROCESS-CRASH | chrome://mochitests/content/a11y/accessible/focus/test_takeFocus.html | application crashed (minidump found)
Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpytn4i3\minidumps\f40df75d-48de-49fb-8fd6-8512b0628a7e.dmp
Operating system: Windows NT
                  6.1.7600 
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0x0

Thread 0 (crashed)
 0  xul.dll!Accessible::GetARIAName(nsAString_internal &) [Accessible.cpp:c1c12246f5ac : 2436 + 0x9]
    eip = 0x6c7dc232   esp = 0x002ec00c   ebp = 0x002ec038   ebx = 0x00000001
    esi = 0x11b82ba0   edi = 0x002ec0e8   eax = 0x00000001   ecx = 0x00000000
    edx = 0x00000000   efl = 0x00010246
    Found by: given as instruction pointer in context
 1  xul.dll!nsAString_internal::SetCapacity(unsigned int,mozilla::fallible_t const &) [nsTSubstring.cpp:c1c12246f5ac : 544 + 0x9]
    eip = 0x6c8da9d6   esp = 0x002ec040   ebp = 0x002ec038
    Found by: stack scanning

Comment 1

[Marco Zehe (:MarcoZ)]

Mark, care to take a look? This looks like it could be a crash that also bites users. The fact that it only now occurred makes me suspicious that it's some recent regression.

Comment 2

[Trevor Saunders (:tbsaunde)]

it's pretty hard to say anything when we only have the top 2 stack frames.

Comment 3

[Ed Morley [:emorley]]

Sadly this isn't intermittent, it has happened at least twice on the first push so far, and again on the next.

Comment 4

[Mike Hommey [:glandium]]

Since this didn't happen on try yesterday with bug 616262, I'm testing the theory that this could come from bad dependencies, and try again after a clobber.

Comment 5

[Ed Morley [:emorley]]

Was resolved by clobber :-)

Comment 6

[Mike Hommey [:glandium]]

So, there is a real bug here and here is why:

The crash happens on this line in Accessible::GetARIAName:
https://hg.mozilla.org/mozilla-central/file/a81526647059/accessible/src/generic/Accessible.cpp#l2435
  2434   if (label.IsEmpty() &&
  2435       mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::aria_label,
  2436                         label)) {

because mContent is NULL.

mContent is NULL because the Accessible on which this happens is a nsHTMLWin32ObjectAccessible, and these objects have a NULL mContent. Execution gets there from nsTextEquivUtils::AppendFromAccessible, which, in turn, comes from nsTextEquivUtils::AppendFromAccessibleChildren, which, itself, comes from nsTextEquivUtils::AppendFromAccessible.

With crazy patches like the following, the crash *doesn't* happen:
diff --git a/accessible/src/generic/Accessible.cpp b/accessible/src/generic/Accessible.cpp
--- a/accessible/src/generic/Accessible.cpp
+++ b/accessible/src/generic/Accessible.cpp
@@ -2429,6 +2429,8 @@ Accessible::GetARIAName(nsAString& aName
   if (NS_SUCCEEDED(rv)) {
     label.CompressWhitespace();
     aName = label;
+  } else {
+    MOZ_CRASH();
   }
 
   if (label.IsEmpty() &&

That else branch is effectively never taken during the test (I haven't run all tests, so maybe this crashes in other cases, but not in test_takeFocus.html).

Anyways, the interesting difference between a build that works and one that doesn't is that nsTextEquivUtils::AppendFromAccessible was never called for an nsHTMLWin32ObjectAccessible instance with a build that works. It was, however, in both cases, called for the parent nsHTMLWin32ObjectOwnerAccessible instance.

The difference is that in the build that works, this branch was never taken:
https://hg.mozilla.org/mozilla-central/file/a81526647059/accessible/src/base/nsTextEquivUtils.cpp#l220

And it was never taken because... aAccessible->Role() overflows the size of gRoleToNameRulesMap in https://hg.mozilla.org/mozilla-central/file/a81526647059/accessible/src/base/nsTextEquivUtils.cpp#l219, and the branch thus depends on whatever data the linker placed after gRoleToNameRulesMap, which explains the erratic behaviour.

Comment 7

[Mike Hommey [:glandium]]

This all means there is a lack of value in gRoleToNameRulesMap for:
ROLE_EMBEDDED_OBJECT
ROLE_NOTE
ROLE_FIGURE
ROLE_CHECK_RICH_OPTION
ROLE_DEFINITION_LIST
ROLE_TERM
and
ROLE_DEFINITION

Comment 8

[Mike Hommey [:glandium]]

Bug 485270, bug 610362, bug 658272, bug 726071 and bug 746358 all added roles without updating gRoleToNameRulesMap.
Bug 716644 removed the ROLE_LAST_ENTRY role, which could be used as a safeguard.

Comment 9

[Mike Hommey [:glandium]]

Actually, expanding the expando from bug 716644 to gRoleToNameRulesMap would solve the problem.
However, that wouldn't help fix it on beta, release and esr.

Comment 10

[Trevor Saunders (:tbsaunde)]

so, here's my guess as to what these should be, it'd be nice if someone can confirm this seems sane (david, Alex if your around)

(In reply to Mike Hommey [:glandium] from comment #7)
> This all means there is a lack of value in gRoleToNameRulesMap for:
> ROLE_EMBEDDED_OBJECT

eNoRule

> ROLE_NOTE

eFromSubtree seems reasonable although eNoRule might be safer, and someone needs to check the html spec here I think.

> ROLE_FIGURE

probably eNoRule or maybe eFromSubtree david thoughts?

> ROLE_CHECK_RICH_OPTION

eFromSubtree

> ROLE_DEFINITION_LIST

eFromSubtreeIfRec // same as ROLE_LIST

> ROLE_TERM

eFromSubtree // same as list item

> and
> ROLE_DEFINITION

eFromSubtree // same as list item

sorry about the trouble, I wouldn't mind puting that list in a patch if its easier.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap

This applies to beta, aurora and m-c.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Created attachment 634523 [details] [diff] [review]
> Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap
> 
> This applies to beta, aurora and m-c.

Err, I wasn't looking at the right thing, this applies on aurora and m-c.

Comment 13

[Mike Hommey [:glandium]]

Attachment: Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap (for beta)

Comment 14

[Mike Hommey [:glandium]]

Attachment: Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap (for release)

Comment 15

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #14)
> Created attachment 634525 [details] [diff] [review]
> Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap (for release)

(If we ever release a 13.0.2)

Comment 16

[Mike Hommey [:glandium]]

Attachment: Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap (for esr)

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #12)
> (In reply to Mike Hommey [:glandium] from comment #11)
> > Created attachment 634523 [details] [diff] [review]
> > Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap
> > 
> > This applies to beta, aurora and m-c.
> 
> Err, I wasn't looking at the right thing, this applies on aurora and m-c.

well, technically I bet it would apply just make our binaries a couple words bigger for no real purpose.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> well, technically I bet it would apply just make our binaries a couple words
> bigger for no real purpose.

Indeed.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Well wow. Thanks for tracking this down Mike!

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > ROLE_FIGURE
> 
> probably eNoRule or maybe eFromSubtree david thoughts?

eNoRule is right here.

Great to have the crash fixed (yikes). Generally we'll want QA on the specifics so if we can wait until Hamburg's morning I'd like Marco to give these a whirl.

Comment 20

[Marco Zehe (:MarcoZ)]

Wow, thanks, Mike! These look correct to me, so go ahead with the process of getting those in. If you need any help with any of the branches, let me know.

Comment 21

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/04c7d09b664f

Comment 22

[Marco Zehe (:MarcoZ)]

Comment on attachment 634523 [details] [diff] [review]
Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Several, see dependencies.
User impact if declined: If web authors will use WAI-ARIA properties on elements such as dl, dt, dd, and users view such pages with assistive technologies, they'll experience crashes. Other irratic behavior through running out of the array's bounds may also occur.
Testing completed (on m-c, etc.): Yes, on try-server build linked to from the bug.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Comment 23

[Marco Zehe (:MarcoZ)]

Comment on attachment 634524 [details] [diff] [review]
Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap (for beta)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Several, see dependencies.
User impact if declined: If the web author uses certain WAI-ARIA attributes on elements like dl, dt, etc., and users view those pages with accessibility enabled, they'll experience crashes. Other irratic behavior as a result of running out of the array's bounds may also occur.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 634525 [details] [diff] [review]
Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap (for release)

[Approval Request Comment]
Regression caused by (bug #): Several, see dependencies.
User impact if declined: Users will experience crashes if they view web pages where authors put WAI-ARIA attributes on dl, dt etc. elements.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None. Requesting to take this as a ride-along if we build an 13.0.2, but not chemspill for it.

Comment 25

[Marco Zehe (:MarcoZ)]

Comment on attachment 634526 [details] [diff] [review]
Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap (for esr)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Unpredictable memory access caused by certain accessibility methods potentially running out of the bounds of an array.
User impact if declined: Users using assistive technologies will experience crashes if viewing pages where web devs use WAI-ARIA attributes on certain elements like figure or figcaption etc. 
Fix Landed on Version: Central (16), approval requests pending for all other branches up to release.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 634523 [details] [diff] [review]
Add missing entries in nsTextEquivUtils::gRoleToNameRulesMap

[Triage Comment]
In support of testing accessibility, and given the fact that there's no risk, let's land on all branches.

Comment 27

[Mike Hommey [:glandium]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/d557426b0c8d
http://hg.mozilla.org/releases/mozilla-beta/rev/7169b4d7c0a8
http://hg.mozilla.org/releases/mozilla-release/rev/358350fdd135
http://hg.mozilla.org/releases/mozilla-esr10/rev/44a4f66b4406

Comment 28

[u279076]

I'm not familiar with tracking down results on tbpl to verify if this is fixed or not. Can someone please guide me how I do this?

Comment 29

[Ioana (away)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #28)
> I'm not familiar with tracking down results on tbpl to verify if this is
> fixed or not. Can someone please guide me how I do this?

Anthony, feel free to ping me on IRC or Skype when you have a couple of minutes and I can give you the guidelines you need.


This test is now passing on Firefox 15.0 beta 4:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14282008&full=1&branch=mozilla-beta

Also verified the crash stats. The crash has obviously not happened anymore:
https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&date=08%2F10%2F2012+15%3A08%3A50&query_search=signature&query_type=contains&query=Accessible%3A%3AGetARIAName%28nsAString_internal+%26%29&reason=&build_id=&process_type=any&hang_type=any&do_query=1

Comment 30

[Ioana (away)]

The test is also passing on Firefox 16 beta: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=15577034&full=1&branch=mozilla-beta

The crash didn't reappear in Socorro either: 
https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&date=08%2F10%2F2012+15%3A08%3A50&query_search=signature&query_type=contains&query=Accessible%3A%3AGetARIAName%28nsAString_internal+%26%29&reason=&build_id=&process_type=any&hang_type=any&do_query=1