Last Comment Bug 516237 - nsTreeSelection::SetTree doesn't addref the tree
: nsTreeSelection::SetTree doesn't addref the tree
Status: RESOLVED FIXED
: [sg:critical?]
: crash, testcase, verified1.9.0.16, verified1.9.1
Product: Core
Classification: Components
Component: XUL
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: xptoolkit.widgets
:
: 516912
:
  Show dependency treegraph
 
Reported: 2009-09-13 04:55 PDT by Olli Pettay [:smaug]
Modified: 2010-04-01 20:17 PDT (History)
14 users (show)
dbaron: blocking1.9.2+
dveditz: blocking1.9.0.16+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta4-fixed
  .6+
  .6-fixed


Attachments
crashtest (1.31 KB, application/vnd.mozilla.xul+xml)
2009-09-14 03:38 PDT, Olli Pettay [:smaug]
no flags Details
possible patch (2.59 KB, patch)
2009-09-14 04:01 PDT, Olli Pettay [:smaug]
neil: superreview+
Details | Diff | Splinter Review
crashtest (2.01 KB, patch)
2009-09-14 06:49 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
+comment fixed (2.59 KB, patch)
2009-09-14 07:48 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
mTree == aTree (2.53 KB, patch)
2009-09-14 07:49 PDT, Olli Pettay [:smaug]
enndeakin: review+
dbaron: approval1.9.2+
Details | Diff | Splinter Review
for 1.9.1 (2.36 KB, patch)
2009-11-04 06:33 PST, Olli Pettay [:smaug]
enndeakin: review+
neil: superreview+
dveditz: approval1.9.1.6+
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review

Summon comment box

Description Olli Pettay [:smaug] 2009-09-13 04:55:38 PDT
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.
Comment 1 Olli Pettay [:smaug] 2009-09-14 03:38:49 PDT
Created attachment 400464 [details]
crashtest
Comment 2 Olli Pettay [:smaug] 2009-09-14 04:01:21 PDT
Created attachment 400468 [details] [review]
possible patch

Running tests first to see if this causes leaks or other problems.
Comment 3 Olli Pettay [:smaug] 2009-09-14 06:49:06 PDT
Created attachment 400487 [details] [review]
crashtest
Comment 4 neil@parkwaycc.co.uk 2009-09-14 07:42:36 PDT
(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 5 neil@parkwaycc.co.uk 2009-09-14 07:44:56 PDT
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\]\. // :-)
Comment 6 Olli Pettay [:smaug] 2009-09-14 07:48:11 PDT
Created attachment 400495 [details] [review]
+comment fixed

Oops :)
Comment 7 Olli Pettay [:smaug] 2009-09-14 07:49:46 PDT
Created attachment 400497 [details] [review]
mTree == aTree
Comment 8 Olli Pettay [:smaug] 2009-09-15 02:23:45 PDT
http://hg.mozilla.org/mozilla-central/rev/5b6c7bdfae3d
Comment 9 Olli Pettay [:smaug] 2009-09-15 07:37:50 PDT
Backed out to see if this caused mem usage regression on linux.
http://hg.mozilla.org/mozilla-central/rev/598eafb1b61f
Comment 10 Olli Pettay [:smaug] 2009-09-16 01:13:03 PDT
http://hg.mozilla.org/mozilla-central/rev/b501121884dd

Relanded
Comment 11 David Baron [:dbaron] 2009-09-17 16:20:18 PDT
Comment on attachment 400497 [details] [review]
mTree == aTree

a1.9.2=dbaron
Comment 12 David :Bienvenu 2009-10-14 13:33:59 PDT
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
Comment 13 Olli Pettay [:smaug] 2009-10-14 13:35:55 PDT
This should land on branches, at least in some form.
Maybe with https://bug516912.bugzilla.mozilla.org/attachment.cgi?id=403095
Comment 14 David :Bienvenu 2009-10-14 13:38:29 PDT
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?
Comment 15 Dan Mosedale (:dmose) 2009-10-16 09:55:59 PDT
Requesting blocking for 1.9.1.next, since given comment 13 from Smaug.
Comment 16 Daniel Veditz 2009-10-16 10:57:18 PDT
should this also land in 1.9.0?
Comment 17 Olli Pettay [:smaug] 2009-11-04 06:33:43 PST
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).
Comment 18 David :Bienvenu 2009-11-05 07:04:12 PST
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.
Comment 19 Olli Pettay [:smaug] 2009-11-05 07:09:52 PST
It shouldn't break TB, but if you could test to verify, that would be great.
Comment 20 Daniel Veditz 2009-11-06 11:33:25 PST
Comment on attachment 410230 [details] [review]
for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers
Comment 21 Olli Pettay [:smaug] 2009-11-06 12:06:17 PST
(In reply to comment #16)
> should this also land in 1.9.0?
I think so.
Comment 22 Olli Pettay [:smaug] 2009-11-06 12:18:31 PST
Comment on attachment 410230 [details] [review]
for 1.9.1

This applies to 1.9.0.
Comment 23 Olli Pettay [:smaug] 2009-11-06 13:08:29 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1bbadd6c8e4
Comment 24 Daniel Veditz 2009-11-09 14:17:01 PST
Comment on attachment 410230 [details] [review]
for 1.9.1

Approved for 1.9.0.16, a=dveditz for release-drivers
Comment 25 Al Billings [:abillings] 2009-11-10 16:52:40 PST
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.
Comment 26 Samuel Sidler (old account; do not CC) 2009-11-11 10:25:44 PST
Olli: Can we get this landed on 1.9.0 ASAP?
Comment 27 Olli Pettay [:smaug] 2009-11-11 11:02:29 PST
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
Comment 28 Al Billings [:abillings] 2009-11-20 17:09:43 PST
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?
Comment 29 Al Billings [:abillings] 2009-11-20 17:11:19 PST
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.
Comment 30 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-11-21 02:07:59 PST
Any particular reason this didn't land on 1.9.2 already? Looks like dbaron approved it in September.
Comment 31 Al Billings [:abillings] 2009-11-21 08:16:07 PST
Which makes me wonder if any of the other 1.9.1 security bugs didn't land on 1.9.2.
Comment 32 Olli Pettay [:smaug] 2009-11-22 12:11:46 PST
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.
Comment 33 Olli Pettay [:smaug] 2009-11-22 12:17:40 PST
(In reply to comment #32)
> ...and is safer than the one for trunk. 
I mean - "shouldn't cause regressions"
Comment 34 Olli Pettay [:smaug] 2009-11-24 07:49:39 PST
Comment on attachment 410230 [details] [review]
for 1.9.1

This bug became a blocker.
I'll push this patch to 1.9.2.
Comment 35 Olli Pettay [:smaug] 2009-11-24 07:51:45 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/70b578cc2cf7

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