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

VERIFIED FIXED

Status

()

Core
Disability Access APIs
--
critical
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: MarcoZ, Assigned: MarcoZ)

Tracking

({access, crash, verified1.9.1})

Trunk
x86
Windows XP
access, crash, verified1.9.1
Points:
---

Firefox Tracking Flags

(status1.9.1 .8-fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
(Assignee)

Comment 2

8 years ago
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.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
Attachment #383734 - Flags: review?(bolterbugz)
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.
Status: ASSIGNED → NEW

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

8 years ago
Created attachment 383743 [details] [diff] [review]
Better patch

Thanks for the catch, David! This also fixes the crash.
Attachment #383734 - Attachment is obsolete: true
Attachment #383743 - Flags: review?(bolterbugz)
Attachment #383734 - Flags: review?(bolterbugz)
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.
Attachment #383743 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 6

8 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/678c63dec8b0
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #383743 - Flags: approval1.9.1?
Attachment #383743 - Flags: approval1.9.0.12?
(Assignee)

Comment 7

8 years ago
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.
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.
Keywords: testcase-wanted

Comment 9

8 years ago
(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?
(Assignee)

Updated

8 years ago
Depends on: 499093
(Assignee)

Comment 10

8 years ago
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.
(Assignee)

Comment 11

8 years ago
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).
Status: RESOLVED → VERIFIED
Attachment #383743 - Flags: approval1.9.0.12?
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.
(Assignee)

Comment 13

8 years ago
Comment on attachment 383743 [details] [diff] [review]
Better patch

Rerequesting approval given Jonas' explanation in bug 499093, which has since been closed as invalid.
Attachment #383743 - Flags: approval1.9.0.12?
Comment on attachment 383743 [details] [diff] [review]
Better patch

bug 499093 was reopened.
Attachment #383743 - Flags: approval1.9.0.12?
(Assignee)

Updated

8 years ago
Attachment #383743 - Flags: approval1.9.1? → approval1.9.1.2?
Comment on attachment 383743 [details] [diff] [review]
Better patch

Still gonna wait until bug 499093 is resolved.
Attachment #383743 - Flags: approval1.9.1.2? → approval1.9.1.3?
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.
Attachment #383743 - Flags: approval1.9.1.3?
Comment on attachment 383743 [details] [diff] [review]
Better patch

1.9.1 drivers, this simple patch fixes the crash (see also bug 499093).
Attachment #383743 - Flags: approval1.9.1.7?
(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 on attachment 383743 [details] [diff] [review]
Better patch

Approved for 1.9.1.8, a=dveditz for release-drivers
Attachment #383743 - Flags: approval1.9.1.8? → approval1.9.1.8+

Comment 20

7 years ago
landed on 1.9.1.8 - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b78bbc04ed99
Keywords: fixed1.9.1

Updated

7 years ago
status1.9.1: --- → .8-fixed
Keywords: fixed1.9.1
(Assignee)

Comment 21

7 years ago
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)
Keywords: verified1.9.1
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.