Last Comment Bug 498913 - Crash when visiting www.bicycling.com with JAWS 10 running
: Crash when visiting www.bicycling.com with JAWS 10 running
Status: VERIFIED FIXED
: access, crash, verified1.9.1
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Marco Zehe (:MarcoZ) on PTO until August 15
:
Mentors:
http://www.bicycling.com
Depends on: 499093
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-17 08:52 PDT by Marco Zehe (:MarcoZ) on PTO until August 15
Modified: 2015-10-16 11:50 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.8-fixed


Attachments
Patch (772 bytes, patch)
2009-06-17 09:21 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
Better patch (1.10 KB, patch)
2009-06-17 09:57 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
dbolter: review+
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) on PTO until August 15 2009-06-17 08:52:35 PDT
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) on PTO until August 15 2009-06-17 09:07:55 PDT
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) on PTO until August 15 2009-06-17 09:21:51 PDT
Created attachment 383734 [details] [diff] [review]
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] 2009-06-17 09:33:10 PDT
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) on PTO until August 15 2009-06-17 09:57:49 PDT
Created attachment 383743 [details] [diff] [review]
Better patch

Thanks for the catch, David! This also fixes the crash.
Comment 5 David Bolter [:davidb] 2009-06-17 10:03:37 PDT
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) on PTO until August 15 2009-06-17 10:07:26 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/678c63dec8b0
Comment 7 Marco Zehe (:MarcoZ) on PTO until August 15 2009-06-17 10:08:54 PDT
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] 2009-06-17 15:45:24 PDT
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 2009-06-17 18:57:36 PDT
(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) on PTO until August 15 2009-06-18 01:36:24 PDT
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) on PTO until August 15 2009-06-18 07:38:51 PDT
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] 2009-06-19 11:20:20 PDT
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) on PTO until August 15 2009-06-19 15:27:24 PDT
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] 2009-06-24 15:44:58 PDT
Comment on attachment 383743 [details] [diff] [review]
Better patch

bug 499093 was reopened.
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2009-07-23 09:21:34 PDT
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) 2009-08-25 16:36:57 PDT
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] 2009-12-03 08:21:02 PST
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] 2009-12-03 08:22:55 PST
(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] 2009-12-21 15:23:03 PST
Comment on attachment 383743 [details] [diff] [review]
Better patch

Approved for 1.9.1.8, a=dveditz for release-drivers
Comment 20 alexander :surkov 2009-12-23 18:42:29 PST
landed on 1.9.1.8 - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b78bbc04ed99
Comment 21 Marco Zehe (:MarcoZ) on PTO until August 15 2010-01-04 07:36:38 PST
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)

Note You need to log in before you can comment on or make changes to this bug.