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

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Ginn Chen]

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

Comment 2

[Evan Yan]

Attachment: 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.

Comment 3

[Evan Yan]

Attachment: add a bool member to nsAccessible to prevent recurse into the same node.

Fix for bug 391132 is backed out. With this patch, we don't need that change any more.

Comment 4

[Aaron Leventhal]

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.

Comment 5

[Evan Yan]

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.

Comment 6

[Evan Yan]

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.

Comment 7

[Aaron Leventhal]

Attachment: Keeping it simple -- boolean isAlreadyHere for recursion protection

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.

Comment 8

[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.

Comment 9

[Ginn Chen]

+  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 10

[Aaron Leventhal]

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.

Comment 11

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

Comment on attachment 285920 [details] [diff] [review]
Keeping it simple -- boolean isAlreadyHere for recursion protection

a=endgame drivers for M9

Comment 12

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102605 Minefield/3.0a9pre