479931 - Crash [@ nsTreeContentView::InsertRow] with <xul:tree> and <option>

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Firefox crashes trying to do something with memory at 0x1c6ffffc.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Mats Palmgren (inactive)]

Attachment: Like so?

Comment 3

[[e-mail]]

Comment on attachment 364001 [details] [diff] [review]
Like so?

It's not the parent index that's wrong, it's the child index, and that's wrong because we're not allowing for the <div>. Mind you, I think the code for <optgroup> is wrong too, but that just fails "safe".

Comment 4

[Neil Deakin]

This is all just xbl form controls stuff. We should just remove it from the tree code.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Ah, there is already a function that takes into account content nodes that the tree ignores in order to determine proper indices, so use it here.

Comment 6

[[e-mail]]

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

>-    PRInt32 count = InsertRow(parentIndex, aIndexInContainer, aChild);
>+    PRInt32 index = 0;
>+    GetIndexInSubtree(aContainer, aChild, &index);
>+    PRInt32 count = InsertRow(parentIndex, index, aChild);
>     if (mBoxObject)
>       mBoxObject->RowCountChanged(parentIndex + aIndexInContainer + 1, count);
You meant to use index here too, right? r=me with that fixed.

Comment 7

[Timothy Nikkel (:tnikkel)]

Whoops, thanks for catching that!

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Fixed Neil's comment.

Comment 9

[Timothy Nikkel (:tnikkel)]

Even though 1.9.0 and 1.9.1 don't crash on the testcase, the same vulnerable code is on all branches, so I think we should take this on all branches.

Comment 11

[Daniel Veditz [:dveditz]]

Is there any reason this hasn't landed on mozilla-central for testing yet?

Comment 12

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/afd3d529bc14

Comment 13

[Samuel Sidler (old account; do not CC)]

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

This is blocking 1.9.2 now, so it doesn't need approval to land there.

Comment 14

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/52713e37cfc6

Comment 15

[Daniel Veditz [:dveditz]]

Is it possible to write a testcase that exercises the bug on the 1.9.1/1.9.0 branches? Otherwise QA won't be able to verify this.

Comment 16

[Timothy Nikkel (:tnikkel)]

I think the reason is doesn't crash on 1.9.0 and 1.9.1 is the switch from a nsVoidArray to a nsTArray. I haven't looked any more closely.

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: testcase with observable difference on 1.9.1/1.9.0

If you just need something that has observable differences before/after this patch on 1.9.1/1.9.0 this testcase will do.

Clicking the button should insert an x between the a and b. Without the patch the x is inserted between the b and c.

Comment 18

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.1.5 and 1.9.0.16, a=dveditz for release-drivers

Comment 19

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c26d21a5b40

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checking in nsTreeContentView.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeContentView.cpp,v  <--  nsTreeContentView.cpp
new revision: 1.71; previous revision: 1.70

Comment 21

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #17)
> Created an attachment (id=410163) [details]
> testcase with observable difference on 1.9.1/1.9.0
> 
> If you just need something that has observable differences before/after this
> patch on 1.9.1/1.9.0 this testcase will do.
> 
> Clicking the button should insert an x between the a and b. Without the patch
> the x is inserted between the b and c.

Verified using this testcase on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5. I see the difference in behavior between 1.9.1.5 and 1.9.1.6.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Verified using the same testcase on 1.9.0 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729) and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.0.16pre) Gecko/2009111916 GranParadiso/3.0.16pre.

Comment 23

[Timothy Nikkel (:tnikkel)]

Added crashtest to 1.9.1, 1.9.2, and mozilla-central.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/976ec8c4d911
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/914ae05fe0a2
http://hg.mozilla.org/mozilla-central/rev/260d768e55f4

Comment 24

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336