Bugzilla@Mozilla – Bug 516237
nsTreeSelection::SetTree doesn't addref the tree
Last modified: 2010-04-01 20:17:32 PDT
Summon comment box
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.
Created attachment 400464 [details] crashtest
Created attachment 400468 [details] [review] possible patch Running tests first to see if this causes leaks or other problems.
Created attachment 400487 [details] [review] crashtest
(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] [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\]\. // :-)
Created attachment 400495 [details] [review] +comment fixed Oops :)
Created attachment 400497 [details] [review] mTree == aTree
http://hg.mozilla.org/mozilla-central/rev/5b6c7bdfae3d
Backed out to see if this caused mem usage regression on linux. http://hg.mozilla.org/mozilla-central/rev/598eafb1b61f
http://hg.mozilla.org/mozilla-central/rev/b501121884dd Relanded
Comment on attachment 400497 [details] [review] mTree == aTree a1.9.2=dbaron
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.
should this also land in 1.9.0?
Created attachment 410230 [details] [review] for 1.9.1 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).
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] [review] for 1.9.1 Approved for 1.9.1.6, a=dveditz for release-drivers
(In reply to comment #16) > should this also land in 1.9.0? I think so.
Comment on attachment 410230 [details] [review] for 1.9.1 This applies to 1.9.0.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1bbadd6c8e4
Comment on attachment 410230 [details] [review] for 1.9.1 Approved for 1.9.0.16, a=dveditz for release-drivers
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.
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
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?
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.
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] [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.
(In reply to comment #32) > ...and is safer than the one for trunk. I mean - "shouldn't cause regressions"
Comment on attachment 410230 [details] [review] for 1.9.1 This bug became a blocker. I'll push this patch to 1.9.2.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/70b578cc2cf7