498913 - Crash when visiting www.bicycling.com with JAWS 10 running

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Launch Firefox with JAWS 10 (latest at this time is 10.0.1154).
2. Go to http://www.bicycling.com.
3. Crash.

Reproducible: Always

Reported by Rob from Freedom Scientific. Excerpt from their analysis:
-------

The root cause of this bug is a failure to check for a null pointer prior to
accessing a pointer in the Mozilla Firefox source code.  The function in
question is nsAccUtils::GetDOMElementFor found in nsAccessibilityUtils.cpp
<http://mxr.mozilla.org/firefox/source/accessible/src/base/nsAccessibilityUtils.cpp>.
 This function calls node->IsNodeOfType without first verifying that node is
not NULL.

nsAccUtils::GetDOMElementFor is getting called indirectly from IsDataTable
(IAccessible *) in {FSSDK}/fsDomSrv/FSDomNodeFirefox/FSDomNodeFirefoxImpl.cpp. 
IsDataTable calls IAccessible2::get_attributes and this call to
IAccessible2::get_attributes is what causes nsAccUtils::GetDOMElementFor to
eventually be called.  The call stack is as follows:

xul.dll!nsAccUtils::GetDOMElementFor(nsIDOMNode * aNode=0x00000000)  Line 1115
+ 0xa bytes    C++
xul.dll!nsAccessNode::GetComputedStyleDeclaration(const nsAString_internal &
aPseudoElt={...}, nsIDOMNode * aNode=0x00000000, nsIDOMCSSStyleDeclaration * *
aCssDecl=0x0026ddfc)  Line 645 + 0x12 bytes    C++
xul.dll!nsHTMLTableAccessible::IsProbablyForLayout(int *
aIsProbablyForLayout=0x0026e0ac)  Line 1144 + 0x24 bytes    C++
xul.dll!nsHTMLTableAccessible::GetAttributesInternal(nsIPersistentProperties *
aAttributes=0x0026e0ac)  Line 234    C++
xul.dll!nsCOMPtr<nsIPersistentProperties>::operator=(const nsCOMPtr_helper &
rhs={...})  Line 782    C++
xul.dll!nsAccessible::GetAttributes(nsIPersistentProperties * *
aAttributes=0x0026e0ac)  Line 2064 + 0x11 bytes    C++
xul.dll!nsAccessibleWrap::get_attributes(wchar_t * * aAttributes=0x6a1bf62e) 
Line 1581    C++
FsDomNodeFirefox.dll!IsDataTable(IAccessible * acc=0x078d38c3)  Line 539 + 0x25
bytes    C++

-------
 
The crash report I just submitted is bp-541dcf75-b6bc-49e1-a4db-0c5302090617.

Comment 1

[Marco Zehe (:MarcoZ)]

From this stack trace, it looks like it's iterating through the rows from 0 to NumberOfRows -1 and looking at the style declaration for each. And during that loop a row it obtains is no longer valid. In other words, from the time it gets the html:tr elements a few lines above, to the loop, something gets pulled out from underneath it.

Comment 2

[Marco Zehe (:MarcoZ)]

Attachment: Patch

null check. The other places in this stack all check for success of their calls, this one doesn't.

Comment 3

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

I'm okay with the null check but this still seems very strange. 

Marco, can you try something else:

In nsHTMLTableAccessible::IsProbablyForLayout, where we loop, can you change rows to length?

So:
for (PRInt32 rowCount = 0; rowCount < rows; rowCount ++)

Becomes:
for (PRInt32 rowCount = 0; rowCount < length; rowCount ++)

We arrive at rows in a different way than length. If this works, it is worth investigating why we get different numbers.

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: Better patch

Thanks for the catch, David! This also fixes the crash.

Comment 5

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

Comment on attachment 383743 [details] [diff] [review]
Better patch

r=me, but we should get Alex's attention here as well. It is troubling that rows != length.

Comment 6

[Marco Zehe (:MarcoZ)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/678c63dec8b0

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 383743 [details] [diff] [review]
Better patch

Plain obvious fix: The loop was using the wrong end point variable to count up to and ran out of bounds.

Comment 8

[Daniel Veditz [:dveditz]]

Are we sure this is correct, that there's no additional problem lurking here? If length != rows how do we know length isn't changing further while you're looping?

Also please capture a minimal testcase in the bug so we can verify this if www.bicycling.com changes their page.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Are we sure this is correct, that there's no additional problem lurking here?
> If length != rows how do we know length isn't changing further while you're
> looping?
> 
> Also please capture a minimal testcase in the bug so we can verify this if
> www.bicycling.com changes their page.

Right. We need to extract mochitest from this site and understand why length != rows. It's pretty bad they are different.

Marco, since you landed the fix already then could you file new bug to find right approach and try to create mochitest?

Comment 10

[Marco Zehe (:MarcoZ)]

Filed bug 499093 with a testcase that reproduces the bug in Minefield builds up to Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090617 Minefield/3.6a1pre (.NET CLR 3.5.30729). The cause is an html:a element that gets added after the last closing TR but before the closing TABLE tag of the last table on www.bicycling.com.

Comment 11

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre (.NET CLR 3.5.30729).

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 383743 [details] [diff] [review]
Better patch

We're not going to take this on branch until we understand the issues in bug 499093 -- we don't want to just move this problem around.

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 383743 [details] [diff] [review]
Better patch

Rerequesting approval given Jonas' explanation in bug 499093, which has since been closed as invalid.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 383743 [details] [diff] [review]
Better patch

bug 499093 was reopened.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 383743 [details] [diff] [review]
Better patch

Still gonna wait until bug 499093 is resolved.

Comment 16

[Samuel Sidler (old account; do not CC)]

Comment on attachment 383743 [details] [diff] [review]
Better patch

We won't take this on the 1.9.1 branch until bug 499093 is resolved.

Comment 17

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

Comment on attachment 383743 [details] [diff] [review]
Better patch

1.9.1 drivers, this simple patch fixes the crash (see also bug 499093).

Comment 18

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

(In reply to comment #16)
> (From update of attachment 383743 [details] [diff] [review])
> We won't take this on the 1.9.1 branch until bug 499093 is resolved.

In a sense the resolution is the same regarding the crash fix. Longer term related work regarding how we walk table frames/nodes will happen elsewhere and will likely be a much larger delta than this crash fix.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 383743 [details] [diff] [review]
Better patch

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 20

[alexander :surkov (:asurkov)]

landed on 1.9.1.8 - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b78bbc04ed99

Comment 21

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.8pre) Gecko/20100104 Shiretoko/3.5.8pre (.NET CLR 3.5.30729)

Comment 22

[David]

http://bikesrider.com/
Crash