391132 - Crash [@ nsAttrValue::ToString] with input type=image inside label and with display: table-cell

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes Mozilla on load.
The testcase uses enhanced priviliges, so you need to download it to your computer to see the crash.
It doesn't seem to crash on branch.

http://crash-stats.mozilla.com/report/index/f337246b-445e-11dc-8f8d-001a4bd46e84
0  	nsAttrValue::ToString(nsAString_internal&)  	 e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\base\src\nsattrvalue.cpp:307
1 	nsAttrValue::Equals(nsAString_internal const&, nsCaseTreatment) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\base\src\nsattrvalue.cpp:644
2 	nsAttrValue::Equals(nsIAtom*, nsCaseTreatment) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\base\src\nsattrvalue.cpp:656
3 	nsGenericElement::FindAttrValueIn(int, nsIAtom*, nsIAtom* const* const*, nsCaseTreatment) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\base\src\nsgenericelement.cpp:3784
4 	nsAccessible::AppendFlatStringFromContentNode(nsIContent*, nsAString_internal*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\accessible\src\base\nsaccessible.cpp:1579
5 	nsAccessible::AppendFlatStringFromSubtreeRecurse(nsIContent*, nsAString_internal*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\accessible\src\base\nsaccessible.cpp:1633
6 	nsAccessible::AppendFlatStringFromSubtreeRecurse(nsIContent*, nsAString_internal*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\accessible\src\base\nsaccessible.cpp:1640
7 	nsAccessible::AppendFlatStringFromSubtreeRecurse(nsIContent*, nsAString_internal*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\accessible\src\base\nsaccessible.cpp:1640
8 	nsAccessible::AppendFlatStringFromSubtree(nsIContent*, nsAString_internal*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\accessible\src\base\nsaccessible.cpp:1604
9 	nsAccessible::GetHTMLName(nsAString_internal&, int) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\accessible\src\base\nsaccessible.cpp:1884
10 	nsAccessible::GetName(nsAString_internal&) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\accessible\src\base\nsaccessible.cpp:294
etc.

Comment 1

[Evan Yan]

It's infinite recursion of calling nsAccessible::GetName().

We fixed the situation of A.GetName() calls itself in bug 389203. However, the situation in this bug is as below

  A.GetName() --> B.GetName() --> A.GetName()

Comment 2

[Aaron Leventhal]

Evan, can you fix it?

Comment 3

[Evan Yan]

I need some help on this.

We can use a stack to record all the accessibles. At the beginning of GetName(), push |this| into the stack, and pop at the end of GetName(). Check the stack to prevent recursive calling into GetName().

This solution is kinda ugly and tricky. Or we may need change how we implement GetName() function serial.

Comment 4

[Aaron Leventhal]

> A.GetName() --> B.GetName() --> A.GetName()
Where does B call back to A?

Comment 5

[Evan Yan]

Attachment: minimal testcase

the minimal testcase is like below

<label>
<input style="display: table-cell;" type="image">
<input style="display: table-cell;" type="image">
</label>

First, in the first |input|'s GetName(), it tries to get name from the |label|:

  1873   nsIContent *labelContent = GetHTMLLabelContent(content);

Then, |label| will AppendFlatStringFromSubtree(), which will calls its inside |input|s' GetName() indirectly. That makes the second |input|'s GetName() get called. 

That how B (the second |input|) call back to A (the first |input|) again.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sounds like GetName() , when called on an ancestor should simply not recurse up again.  That is, GetName() on the label as called from the image input should have extra state indicating that this is happening.  Something like a boolean arg, eg (which GetName() usually just propagates without changing and is set to false on entries into GetName and to true when calling it on an ancestor label, or a label in general, perhaps).

Comment 7

[Evan Yan]

Attachment: patch

Comment 8

[Aaron Leventhal]

Comment on attachment 282660 [details] [diff] [review]
patch

Please remove +  static PRBool isRecursing = PR_FALSE;
You don't need that variable, because aContent is always non-null when isRecursing is true.
The startContent == aContent check is enough.

Comment 9

[Aaron Leventhal]

Comment on attachment 282660 [details] [diff] [review]
patch

Actually I'm realizing this isn't complex enough, and passing an argument is necessary.

Like this:
<label>
<input style="display: table-cell;" type="image" aaa:labelledby="somelabel">
<input style="display: table-cell;" type="image" id="anotherlabel">
</label>

<div id="somelabel>
<input style="display: table-cell;" type="image" aaa:labelledby="anotherlabel">
</div>

Right?

Comment 10

[Evan Yan]

Attachment: patch v2

tested with the testcase in Aaron's last comment.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 283914 [details] [diff] [review]
patch v2

looks ok

Comment 12

[Evan Yan]

/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.326; previous revision: 1.325
done

Comment 13

[Martijn Wargers (dead)]

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