Last Comment Bug 479931 - Crash [@ nsTreeContentView::InsertRow] with <xul:tree> and <option>
: Crash [@ nsTreeContentView::InsertRow] with <xul:tree> and <option>
Status: RESOLVED FIXED
: [sg:critical?]
: crash, testcase, verified1.9.0.16, verified1.9.1
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL
: Trunk
: x86 Mac OS X
: P1 critical (vote)
: mozilla1.9.2
Assigned To: Timothy Nikkel (:tn)
: XPToolKit XUL
:
:
: 343943 414170
  Show dependency treegraph
 
Reported: 2009-02-23 21:15 PST by Jesse Ruderman
Modified: 2010-04-06 17:15 PDT (History)
11 users (show)
mbeltzner: blocking1.9.2+
dveditz: blocking1.9.0.16+
dveditz: wanted1.9.0.x+
tnikkel: in‑testsuite+
See Also:
Crash Signature:
[@ nsTreeContentView::InsertRow]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta2-fixed
  .6+
  .6-fixed


Attachments
testcase (crashes Firefox when loaded) (449 bytes, application/xhtml+xml)
2009-02-23 21:15 PST, Jesse Ruderman
no flags Details
stack trace (7.71 KB, text/plain)
2009-02-23 21:16 PST, Jesse Ruderman
no flags Details
Like so? (2.88 KB, patch)
2009-02-24 17:06 PST, Mats Palmgren [:mats]
neil: review-
neil: superreview-
Details | Diff | Splinter Review
patch (1.12 KB, patch)
2009-10-17 23:45 PDT, Timothy Nikkel (:tn)
neil: review+
Details | Diff | Splinter Review
patch v2 (1.32 KB, patch)
2009-10-18 14:19 PDT, Timothy Nikkel (:tn)
dveditz: approval1.9.1.6+
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review
testcase with observable difference on 1.9.1/1.9.0 (940 bytes, application/vnd.mozilla.xul+xml)
2009-11-03 23:16 PST, Timothy Nikkel (:tn)
no flags Details

Summon comment box

Description Jesse Ruderman 2009-02-23 21:15:32 PST
Created attachment 363834 [details]
testcase (crashes Firefox when loaded)

Firefox crashes trying to do something with memory at 0x1c6ffffc.
Comment 1 Jesse Ruderman 2009-02-23 21:16:02 PST
Created attachment 363835 [details]
stack trace
Comment 2 Mats Palmgren [:mats] 2009-02-24 17:06:13 PST
Created attachment 364001 [details] [review]
Like so?
Comment 3 neil@parkwaycc.co.uk 2009-02-25 04:33:51 PST
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".
Comment 4 Neil Deakin 2009-02-25 06:29:03 PST
This is all just xbl form controls stuff. We should just remove it from the tree code.
Comment 5 Timothy Nikkel (:tn) 2009-10-17 23:45:53 PDT
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 6 neil@parkwaycc.co.uk 2009-10-18 09:09:31 PDT
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.
Comment 7 Timothy Nikkel (:tn) 2009-10-18 14:06:33 PDT
Whoops, thanks for catching that!
Comment 8 Timothy Nikkel (:tn) 2009-10-18 14:19:12 PDT
Created attachment 406942 [details] [review]
patch v2

Fixed Neil's comment.
Comment 9 Timothy Nikkel (:tn) 2009-10-18 14:20:13 PDT
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 10 Timothy Nikkel (:tn) 2009-10-19 07:45:15 PDT
*** Bug 414170 has been marked as a duplicate of this bug. ***
Comment 11 Daniel Veditz 2009-10-23 10:31:46 PDT
Is there any reason this hasn't landed on mozilla-central for testing yet?
Comment 12 Timothy Nikkel (:tn) 2009-10-24 02:13:02 PDT
http://hg.mozilla.org/mozilla-central/rev/afd3d529bc14
Comment 13 Samuel Sidler (old account; do not CC) 2009-10-30 11:56:57 PDT
Comment on attachment 406942 [details] [review]
patch v2

This is blocking 1.9.2 now, so it doesn't need approval to land there.
Comment 14 Timothy Nikkel (:tn) 2009-10-31 00:49:57 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/52713e37cfc6
Comment 15 Daniel Veditz 2009-11-02 14:52:40 PST
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 (:tn) 2009-11-02 14:57:32 PST
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 (:tn) 2009-11-03 23:16:30 PST
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 18 Daniel Veditz 2009-11-04 15:14:06 PST
Comment on attachment 406942 [details] [review]
patch v2

Approved for 1.9.1.5 and 1.9.0.16, a=dveditz for release-drivers
Comment 19 Timothy Nikkel (:tn) 2009-11-09 01:01:33 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c26d21a5b40
Comment 20 Boris Zbarsky (:bz) 2009-11-09 09:21:23 PST
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] 2009-11-10 16:50:33 PST
(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] 2009-11-20 16:55:33 PST
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.

Note You need to log in before you can comment on or make changes to this bug.