Bugzilla@Mozilla – Bug 479931
Crash [@ nsTreeContentView::InsertRow] with <xul:tree> and <option>
Last modified: 2010-04-06 17:15:35 PDT
Summon comment box
Created attachment 363834 [details] testcase (crashes Firefox when loaded) Firefox crashes trying to do something with memory at 0x1c6ffffc.
Created attachment 363835 [details] stack trace
Created attachment 364001 [details] [review] Like so?
Comment on attachment 364001 [details] [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".
This is all just xbl form controls stuff. We should just remove it from the tree code.
Created attachment 406901 [details] [review] 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 on attachment 406901 [details] [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.
Whoops, thanks for catching that!
Created attachment 406942 [details] [review] patch v2 Fixed Neil's comment.
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.
*** Bug 414170 has been marked as a duplicate of this bug. ***
Is there any reason this hasn't landed on mozilla-central for testing yet?
http://hg.mozilla.org/mozilla-central/rev/afd3d529bc14
Comment on attachment 406942 [details] [review] patch v2 This is blocking 1.9.2 now, so it doesn't need approval to land there.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/52713e37cfc6
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.
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.
Created attachment 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.
Comment on attachment 406942 [details] [review] patch v2 Approved for 1.9.1.5 and 1.9.0.16, a=dveditz for release-drivers
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c26d21a5b40
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
(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.
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.
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