Closed Bug 399858 Opened 13 years ago Closed 13 years ago

Crash [@ no stack] with label, input type=image

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: evan.yan)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files)

Attached file testcase
See testcase, you need to download the testcase to your computer to see the crash, because of the use of enhanced privileges.

It doesn't seem to crash on branch.

I don't get any useful stack trace with breakpad.
http://crash-stats.mozilla.com/report/index/34f93c47-7b21-11dc-9de7-001a4bd46e84
Assignee: aaronleventhal → Evan.Yan
Blocks: fox3access
stack overflow

#15 0xb6751fc4 in nsAccessible::AppendNameFromAccessibleFor (this=0x87905d8, 
    aContent=0xb3ddfbe0, aFlatString=0xbf02793c, aFromValue=0)
    at nsAccessible.cpp:1511
#16 0xb67538f1 in nsAccessible::AppendFlatStringFromContentNode (
    this=0x87905d8, aContent=0xb3ddfbe0, aFlatString=0xbf02793c)
    at nsAccessible.cpp:1618
#17 0xb6753b24 in nsAccessible::AppendFlatStringFromSubtreeRecurse (
    this=0x87905d8, aContent=0xb3ddfbe0, aFlatString=0xbf02793c)
    at nsAccessible.cpp:1671
#18 0xb6753b67 in nsAccessible::AppendFlatStringFromSubtreeRecurse (
    this=0x87905d8, aContent=0x8795cf8, aFlatString=0xbf02793c)
    at nsAccessible.cpp:1678
#19 0xb6753bb9 in nsAccessible::AppendFlatStringFromSubtree (this=0x87905d8, 
    aContent=0x8795cf8, aFlatString=0xbf02793c) at nsAccessible.cpp:1642
#20 0xb675490a in nsAccessible::GetHTMLName (this=0x87905d8, 
    aLabel=@0xbf027b38, aCanAggregateSubtree=0) at nsAccessible.cpp:1796
#21 0xb6754b31 in nsAccessible::GetName (this=0x87905d8, aName=@0xbf027b38)
    at nsAccessible.cpp:287
#22 0xb677d4bc in nsHTMLTableAccessible::GetName (this=0x87905d8, 
    aName=@0xbf027b38) at nsHTMLTableAccessible.cpp:160
#23 0xb6752054 in nsAccessible::AppendNameFromAccessibleFor (this=0x8790790, 
    aContent=0x8795d90, aFlatString=0xbf027ddc, aFromValue=0)
    at nsAccessible.cpp:1518
#24 0xb67538f1 in nsAccessible::AppendFlatStringFromContentNode (
    this=0x8790790, aContent=0x8795d90, aFlatString=0xbf027ddc)
    at nsAccessible.cpp:1618
#25 0xb6753b24 in nsAccessible::AppendFlatStringFromSubtreeRecurse (
    this=0x8790790, aContent=0x8795d90, aFlatString=0xbf027ddc)
    at nsAccessible.cpp:1671
#26 0xb6753b67 in nsAccessible::AppendFlatStringFromSubtreeRecurse (
    this=0x8790790, aContent=0x8795cf8, aFlatString=0xbf027ddc)
    at nsAccessible.cpp:1678
#27 0xb6753bb9 in nsAccessible::AppendFlatStringFromSubtree (this=0x8790790, 
    aContent=0x8795cf8, aFlatString=0xbf027ddc) at nsAccessible.cpp:1642
#28 0xb675490a in nsAccessible::GetHTMLName (this=0x8790790, 
    aLabel=@0xbf027fd8, aCanAggregateSubtree=0) at nsAccessible.cpp:1796
#29 0xb6754b31 in nsAccessible::GetName (this=0x8790790, aName=@0xbf027fd8)
    at nsAccessible.cpp:287
#30 0xb677d4bc in nsHTMLTableAccessible::GetName (this=0x8790790, 
    aName=@0xbf027fd8) at nsHTMLTableAccessible.cpp:160
#31 0xb6752054 in nsAccessible::AppendNameFromAccessibleFor (this=0x87905d8, 
    aContent=0x8795fb8, aFlatString=0xbf02827c, aFromValue=0)
    at nsAccessible.cpp:1518
OS: Windows XP → All
Attached file minimal testcase
It seems my fix for bug 391132 is not sophisticated (embarrassed..). Thanks for catch this, Martijn!

After discuss with Ginn, I think we'll have to add a bool member into nsAccessible to prevent the recursion.
Fix for bug 391132 is backed out. With this patch, we don't need that change any more.
Attachment #285376 - Flags: review?(aaronleventhal)
Comment on attachment 285376 [details] [diff] [review]
add a bool member to nsAccessible to prevent recurse into the same node.

Evan, I don't want to bloat our objects  by 4 bytes. Please try to do this by passing parameters as Bz originally suggested in the original bug.
Attachment #285376 - Flags: review?(aaronleventhal) → review-
Aaron, I also wish we could solve this by passing parameters.

The problem is, the function call is nested in many layers:

GetName()->GetHTMLName()->AppendFlatStringFromSubtree()->
  AppendFlatStringFromSubtreeRecurse()->AppendFlatStringFromCongtentNode()->
  AppendNameFromAccessibleFor()->GetName()

I'm afraid we'll have to add an additional parameter to all those functions.

And what's more, it may also cause other regressions that simply not recurse up
again when called on an ancestor, like bz originally suggested. Because the recursion circle could be various, like several nodes connected into a circle by aaa:labelledby.
Or we can use a stack to record all the nodes walked through, to prevent recursing into the same node. Still not a elegant way.
Evan, I cannot think of a real world case where I want AppendFlatStringFromSubtree() to end up happening twice in the same stack.

If you're collecting the text from a subtree, you want exactly that text. You don't want a relation which points outside of itself during that.
Attachment #285920 - Flags: review?(Evan.Yan)
Comment on attachment 285920 [details] [diff] [review]
Keeping it simple -- boolean isAlreadyHere for recursion protection

I thought this solution may cause regressions that some GetName() could failed, but let's get it in to see whether any real world case get broken.
Attachment #285920 - Flags: review?(Evan.Yan) → review+
+  static PRBool isAlreadyHere; // Prevent recursion which can cause infinite loops
+  if (isAlreadyHere) {

I think
static PRBool isAlreadyHere = PR_FALSE;
is better.

Although static variables are automatically initialized by 0.
Comment on attachment 285920 [details] [diff] [review]
Keeping it simple -- boolean isAlreadyHere for recursion protection

I can't think of any real cases where we want to include pointed to text when collating the text of a subtree.
Attachment #285920 - Flags: approval1.9?
Attachment #285920 - Flags: approvalM9?
Comment on attachment 285920 [details] [diff] [review]
Keeping it simple -- boolean isAlreadyHere for recursion protection

a=endgame drivers for M9
Attachment #285920 - Flags: approvalM9?
Attachment #285920 - Flags: approvalM9+
Attachment #285920 - Flags: approval1.9?
Attachment #285920 - Flags: approval1.9+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102605 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
Crash Signature: [@ no stack]
You need to log in before you can comment on or make changes to this bug.