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

RESOLVED FIXED in mozilla1.9.2

Status

()

defect
P1
critical
RESOLVED FIXED
11 years ago
Last year

People

(Reporter: jruderman, Assigned: tnikkel)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla1.9.2
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
blocking1.9.0.16 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta2-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

11 years ago
Firefox crashes trying to do something with memory at 0x1c6ffffc.
Reporter

Comment 1

11 years ago
Posted file stack trace

Comment 2

11 years ago
Posted patch Like so? (obsolete) — Splinter Review
Attachment #364001 - Flags: superreview?(neil)
Attachment #364001 - Flags: review?(neil)
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".
Attachment #364001 - Flags: superreview?(neil)
Attachment #364001 - Flags: superreview-
Attachment #364001 - Flags: review?(neil)
Attachment #364001 - Flags: review-

Comment 4

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

Updated

10 years ago
Attachment #364001 - Attachment is obsolete: true
Reporter

Updated

10 years ago
Whiteboard: [sg:critical?]
Reporter

Updated

10 years ago
Blocks: 414170
Assignee

Comment 5

10 years ago
Posted patch patchSplinter Review
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.
Assignee: nobody → tnikkel
Attachment #406901 - Flags: review?(neil)
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.
Attachment #406901 - Flags: review?(neil) → review+
Assignee

Comment 7

10 years ago
Whoops, thanks for catching that!
Assignee

Comment 8

10 years ago
Posted patch patch v2Splinter Review
Fixed Neil's comment.
Assignee

Comment 9

10 years ago
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.
blocking1.9.1: --- → ?
Flags: blocking1.9.0.16?
Assignee

Updated

10 years ago
Attachment #406942 - Flags: approval1.9.2?
Attachment #406942 - Flags: approval1.9.1.5?
Attachment #406942 - Flags: approval1.9.0.16?
Assignee

Updated

10 years ago
Duplicate of this bug: 414170
blocking1.9.1: ? → .5+
Flags: wanted1.9.0.x+
Flags: blocking1.9.2?
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.16+
Is there any reason this hasn't landed on mozilla-central for testing yet?
Assignee

Comment 12

10 years ago
http://hg.mozilla.org/mozilla-central/rev/afd3d529bc14
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.3a1
Assignee

Updated

10 years ago
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?] → [sg:critical?][needs 1.9.2 approval or blocking]
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
Whiteboard: [sg:critical?][needs 1.9.2 approval or blocking] → [sg:critical?][needs 1.9.2 landing]
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.
Attachment #406942 - Flags: approval1.9.2?
Assignee

Comment 14

10 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/52713e37cfc6
Whiteboard: [sg:critical?][needs 1.9.2 landing] → [sg:critical?]
Target Milestone: mozilla1.9.3a1 → mozilla1.9.2
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.
Assignee

Comment 16

10 years ago
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.
Assignee

Comment 17

10 years ago
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] [diff] [review]
patch v2

Approved for 1.9.1.5 and 1.9.0.16, a=dveditz for release-drivers
Attachment #406942 - Flags: approval1.9.1.6?
Attachment #406942 - Flags: approval1.9.1.6+
Attachment #406942 - Flags: approval1.9.0.16?
Attachment #406942 - Flags: approval1.9.0.16+
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
Keywords: fixed1.9.0.16
(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.
Keywords: verified1.9.1
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.
Group: core-security
Crash Signature: [@ nsTreeContentView::InsertRow]
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.