Closed
Bug 498913
Opened 15 years ago
Closed 15 years ago
Crash when visiting www.bicycling.com with JAWS 10 running
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
status1.9.1 | --- | .8-fixed |
People
(Reporter: MarcoZ, Assigned: MarcoZ)
References
()
Details
(Keywords: access, crash, verified1.9.1)
Attachments
(1 file, 1 obsolete file)
1.10 KB,
patch
|
davidb
:
review+
dveditz
:
approval1.9.1.8+
|
Details | Diff | Splinter Review |
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•15 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•15 years ago
|
||
null check. The other places in this stack all check for success of their calls, this one doesn't.
Comment 3•15 years ago
|
||
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•15 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•15 years ago
|
||
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 5•15 years ago
|
||
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•15 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/678c63dec8b0
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Attachment #383743 -
Flags: approval1.9.1?
Attachment #383743 -
Flags: approval1.9.0.12?
Assignee | ||
Comment 7•15 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.
Comment 8•15 years ago
|
||
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•15 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 | ||
Comment 10•15 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•15 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
Updated•15 years ago
|
Attachment #383743 -
Flags: approval1.9.0.12?
Comment 12•15 years ago
|
||
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•15 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 14•15 years ago
|
||
Comment on attachment 383743 [details] [diff] [review] Better patch bug 499093 was reopened.
Attachment #383743 -
Flags: approval1.9.0.12?
Assignee | ||
Updated•15 years ago
|
Attachment #383743 -
Flags: approval1.9.1? → approval1.9.1.2?
Comment 15•15 years ago
|
||
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 16•15 years ago
|
||
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 17•15 years ago
|
||
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?
Comment 18•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
landed on 1.9.1.8 - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b78bbc04ed99
Keywords: fixed1.9.1
Updated•15 years ago
|
status1.9.1:
--- → .8-fixed
Keywords: fixed1.9.1
Assignee | ||
Comment 21•15 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
Updated•9 years ago
|
Keywords: testcase-wanted
Comment 22•7 years ago
|
||
http://bikesrider.com/ Crash
You need to log in
before you can comment on or make changes to this bug.
Description
•