Closed Bug 516237 Opened 10 years ago Closed 10 years ago

nsTreeSelection::SetTree doesn't addref the tree

Categories

(Core :: XUL, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta4-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

(4 keywords, Whiteboard: [sg:critical?])

Attachments

(4 files, 2 obsolete files)

This needs a testcase, but it seems that nsITreeSelection::tree handling
is wrong. The interface is scriptable, so a script may set the .tree.
nsTreeSelection then later uses that object, but nothing guarantees that
the object is alive.
Attached file crashtest
Attached patch possible patch (obsolete) — Splinter Review
Running tests first to see if this causes leaks or other problems.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Whiteboard: [sg:ciritical?]
Whiteboard: [sg:ciritical?] → [sg:critical?]
Attached patch crashtestSplinter Review
Attachment #400468 - Flags: superreview?(neil)
Attachment #400468 - Flags: review?(enndeakin)
(In reply to comment #2)
> Running tests first to see if this causes leaks or other problems.
Hopefully nsTreeBoxObject::Clear(), nsTreeBodyFrame::Destroy etc. will help in breaking potential cycles.
Comment on attachment 400468 [details] [diff] [review]
possible patch

>+  NS_ENSURE_STATE(mTree);
NS_ENSURE_STATE(mTree == aTree); also works in the null case too ;-)

>+  nsCOMPtr<nsITreeBoxObject> mTree; // [Weak]. The tree will hold on to us through the view and let go when it dies.
s/\[Weak\]\. // :-)
Attachment #400468 - Flags: superreview?(neil) → superreview+
Attached patch +comment fixed (obsolete) — Splinter Review
Oops :)
Attachment #400468 - Attachment is obsolete: true
Attachment #400495 - Flags: review?(enndeakin)
Attachment #400468 - Flags: review?(enndeakin)
Attached patch mTree == aTreeSplinter Review
Attachment #400495 - Attachment is obsolete: true
Attachment #400497 - Flags: review?(enndeakin)
Attachment #400495 - Flags: review?(enndeakin)
Attachment #400497 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/5b6c7bdfae3d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #400497 - Flags: approval1.9.2?
Backed out to see if this caused mem usage regression on linux.
http://hg.mozilla.org/mozilla-central/rev/598eafb1b61f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/b501121884dd

Relanded
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 400497 [details] [diff] [review]
mTree == aTree

a1.9.2=dbaron
Attachment #400497 - Flags: approval1.9.2? → approval1.9.2+
Are there plans to land a version of this on the 1.9.1 branch? I'm trying to figure out if I need to fix TB's bug 516912
This should land on branches, at least in some form.
Maybe with https://bug516912.bugzilla.mozilla.org/attachment.cgi?id=403095
that patch doesn't seem like it can get past the reviewers...is my understanding correct that if it did get landed, though, we wouldn't have to do anything else?
Requesting blocking for 1.9.1.next, since given comment 13 from Smaug.
blocking1.9.1: --- → ?
should this also land in 1.9.0?
blocking1.9.1: ? → .5+
Keywords: crash, testcase
Attached patch for 1.9.1Splinter Review
This is even safer for 1.9.1. Contains the patch for bug 516912.

I was a bit worried about causing reference cycles in 1.9.1, but seems like
we clean up mTree reference when element is removed from document or when
the frame for it is destroyed.

Pushed this to tryserver (1.9.1 hg repo).
Attachment #410230 - Flags: superreview?(neil)
Attachment #410230 - Flags: review?(enndeakin)
Attachment #410230 - Flags: review?(enndeakin) → review+
Attachment #410230 - Flags: superreview?(neil) → superreview+
Attachment #410230 - Flags: approval1.9.1.6?
Olli, so this patch doesn't break TB's tab implementation, right? Our plan is to ship with 1.9.1.5, but there's always a chance that we might slip to 1.9.1.6.
It shouldn't break TB, but if you could test to verify, that would be great.
Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers
Attachment #410230 - Flags: approval1.9.1.6? → approval1.9.1.6+
(In reply to comment #16)
> should this also land in 1.9.0?
I think so.
Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

This applies to 1.9.0.
Attachment #410230 - Flags: approval1.9.0.16?
Attachment #410230 - Flags: approval1.9.0.16? → approval1.9.0.16+
Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

Approved for 1.9.0.16, a=dveditz for release-drivers
Flags: blocking1.9.0.16+
Verified this for 1.9.1.6 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091110 Shiretoko/3.5.6pre (.NET CLR 3.5.30729) and the attached testcase. The crash occurs on 1.9.1.5 on the same machine.
Keywords: verified1.9.1
Olli: Can we get this landed on 1.9.0 ASAP?
Checking in layout/xul/base/src/tree/src/nsTreeSelection.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp,v  <--  nsTreeSelection.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in layout/xul/base/src/tree/src/nsTreeSelection.h;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.h,v  <--  nsTreeSelection.h
new revision: 1.20; previous revision: 1.19
done
Keywords: fixed1.9.0.16
I notice that the crashtest on this crashes my nightly 1.9.2 build on OS X (I accidentally clicked). Shouldn't this be fixed for 1.9.2?
Flags: blocking1.9.2?
Verified for 1.9.0.16 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 the crashtest after verifying crash in 1.9.0.15.
Whiteboard: [sg:critical?] → [sg:critical?][needs 192 landing]
Any particular reason this didn't land on 1.9.2 already? Looks like dbaron approved it in September.
Which makes me wonder if any of the other 1.9.1 security bugs didn't land on 1.9.2.
Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

This applies cleanly to 1.9.2 and is safer than the one for trunk. 
(The patch for trunk doesn't actually apply cleanly to 1.9.2 because of cycle collection changes)

And sorry I forgot to get this to 1.9.2.
Attachment #410230 - Flags: approval1.9.2?
(In reply to comment #32)
> ...and is safer than the one for trunk. 
I mean - "shouldn't cause regressions"
blocking1.9.1: .6+ → ---
blocking1.9.1: --- → .6+
Flags: blocking1.9.2? → blocking1.9.2+
Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

This bug became a blocker.
I'll push this patch to 1.9.2.
Attachment #410230 - Flags: approval1.9.2?
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/70b578cc2cf7
Whiteboard: [sg:critical?][needs 192 landing] → [sg:critical?]
Group: core-security
You need to log in before you can comment on or make changes to this bug.