516237 - nsTreeSelection::SetTree doesn't addref the tree

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Attachment: crashtest

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

Running tests first to see if this causes leaks or other problems.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: crashtest

Comment 4

[[e-mail]]

(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

[[e-mail]]

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\]\. // :-)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +comment fixed

Oops :)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: mTree == aTree

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/5b6c7bdfae3d

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/b501121884dd

Relanded

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 400497 [details] [diff] [review]
mTree == aTree

a1.9.2=dbaron

Comment 12

[David :Bienvenu]

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][[e-mail]]]

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]

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 (:dmosedale, :dmose)]

Requesting blocking for 1.9.1.next, since given comment 13 from Smaug.

Comment 16

[Daniel Veditz [:dveditz]]

should this also land in 1.9.0?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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]

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][[e-mail]]]

It shouldn't break TB, but if you could test to verify, that would be great.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> should this also land in 1.9.0?
I think so.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

This applies to 1.9.0.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1bbadd6c8e4

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 410230 [details] [diff] [review]
for 1.9.1

Approved for 1.9.0.16, a=dveditz for release-drivers

Comment 25

[Al Billings [:abillings - ex-MoCo]]

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)]

Olli: Can we get this landed on 1.9.0 ASAP?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

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 - ex-MoCo]]

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 - ex-MoCo]]

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) (email my personal email if necessary)]

Any particular reason this didn't land on 1.9.2 already? Looks like dbaron approved it in September.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

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][[e-mail]]]

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.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #32)
> ...and is safer than the one for trunk. 
I mean - "shouldn't cause regressions"

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/70b578cc2cf7