399227 - Crash @ nsTreeSelection::GetSingle, null mTree

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Steve Krulewitz]

Attachment: linux stack

Currently, one of Songbird's top crashing bugs happens when nsTreeSelection::GetSingle is called on an nsTreeSelection instance that happens to have a null mTree member.  The crash reports can be found here:

Most frequently on windows:
http://tinyurl.com/3dopn9

Happened once on mac and once on linux:
http://tinyurl.com/34xrxv

I believe the js that causes the crash is here:

http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tree.xml#999

I've only been able to reproduce this once by chance, I'll attach the stack.

We do one strange thing where we dynamically create our "playlist" binding JavaScript.  This binding contains a xul:tree whose view is eventually set with our custom nsITreeView implementation.  However, I have my doubts that this crash is happening after we set the view since our custom view implementation also returns a custom nsITreeSelection which is not appearing on the stack.

Another possibility is that it is perfectly normal to have a null mTree in nsTreeSelection, and there is just a missing check for this in the GetSingle() method.  We've patched our custom xulrunner build to add this check and return NS_ERROR_UNEXPECTED, as per Niel.

Comment 1

[[e-mail]]

(In reply to comment #0)
>Another possibility is that it is perfectly normal to have a null mTree
It depends on what you mean by perfectly normal. The only way I can think of of triggering a null mTree from that code is by manually dispatching a click event to a hidden tree. Another way to create a selection with a null mTree is to clear the view on a tree; the old view's selection will then have a null mTree.

Comment 2

[[e-mail]]

FWIW SetCurrentColumn doesn't look as if it null-checks mTree either.

Comment 4

[Mark Banner (:standard8)]

Attachment: Test cases and possible fix.

We've been seeing this on Thunderbird/SeaMonkey (bug 463560). There is a case where we can hide the contacts panel (= remove tree object), then due to the nsITreeView implementation being active and trying to update the selection, we crash.

The test cases replicate this action for the rangedSelect and currentColumns functions where we are crashing.

I'm not quite sure I'm doing the right thing with return values for the GetSingle functions. I think I've probably got the currentColumns function right.

Comments welcome, otherwise I'll poke this through the try servers in the next few days.

Comment 5

[Mark Banner (:standard8)]

Attachment: Test case and fix.

This seems to pass all the standard test machine tests on Mac, and didn't appear to regress perf on Talos. Therefore time to request reviews.

Comment 6

[[e-mail]]

Comment on attachment 351701 [details] [diff] [review]
Test case and fix.

>+    var treecolumn0 = tree.columns[0];
>+    var treecolumn1 = tree.columns[1];
[it seems the bogus JS strict warning for column indices been fixed]

Comment 7

[Neil Deakin]

Comment on attachment 351701 [details] [diff] [review]
Test case and fix.

>-  if (mCurrentColumn) {
>+  if (mTree && mCurrentColumn) {
>     if (mFirstRange)
>       mTree->InvalidateCell(mFirstRange->mMin, mCurrentColumn);
>     if (mCurrentIndex != -1)
>@@ -671,7 +680,7 @@ NS_IMETHODIMP nsTreeSelection::SetCurren
>   
>   mCurrentColumn = aCurrentColumn;
>   
>-  if (mCurrentColumn) {
>+  if (mTree && mCurrentColumn) {

Shouldn't we just return early if mTree is null rather than setting mCurrentColumn to something invalid? Otherwise, this looks ok.

Also, not a big deal, but you could combine the two tests into one.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> (From update of attachment 351701 [details] [diff] [review])
> >-  if (mCurrentColumn) {
> >+  if (mTree && mCurrentColumn) {
> >     if (mFirstRange)
> >       mTree->InvalidateCell(mFirstRange->mMin, mCurrentColumn);
> >     if (mCurrentIndex != -1)
> >@@ -671,7 +680,7 @@ NS_IMETHODIMP nsTreeSelection::SetCurren
> >   
> >   mCurrentColumn = aCurrentColumn;
> >   
> >-  if (mCurrentColumn) {
> >+  if (mTree && mCurrentColumn) {
> 
> Shouldn't we just return early if mTree is null rather than setting
> mCurrentColumn to something invalid? Otherwise, this looks ok.

Sorry for not getting back to this earlier, my thought was that we should still set the column for when the tree is valid again. However, looking at SetCurrentIndex and others, I think you're right, we should just return early with NS_ERROR_UNEXPECTED.

I'll attach the checkin patch in a few minutes.

Comment 9

[Mark Banner (:standard8)]

Attachment: Test case and fix v2

With slight adjustment in SetCurrentColumn as previously mentioned. Carrying forward r/sr.

Comment 10

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/mozilla-central/rev/eab42a3de6c6

I had to do a couple of alterations to the tests because although I'd run them, I hadn't understood about how to get at the crashtest/reftest output. I added a try/catch round the functions that now (correctly) fail, I also added a removeAttribute for the class so that the reftest-wait class is removed from the test:

http://hg.mozilla.org/mozilla-central/rev/411a1637b560
http://hg.mozilla.org/mozilla-central/rev/8a601ed6bc4c

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 354168 [details] [diff] [review]
Test case and fix v2

Requesting 1.9.1 approval for small crash fix. Has crashtests, should be low-risk. Has been checked into mozilla-central.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354168 [details] [diff] [review]
Test case and fix v2

a191=beltzner

Comment 13

[Mark Banner (:standard8)]

Checked into 1.9.1 branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1785be4a786c

Comment 14

[Tony Chung [:tchung]]

Verified tests are in the fixes checked in: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090122 Minefield/3.2a1pre 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre)
Gecko/20090122 Shiretoko/3.1b3pre

Comment 15

[Bob Clary [:bc] (inactive)]

1.9.0 !exploitable report:
PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsTreeSelection::GetSingle

Comment 16

[Bob Clary [:bc] (inactive)]

sorry for the spam. the previous is for the -1 test. layout/xul/base/src/tree/src/crashtests/399227-2.xul gives on 1.9.0:


PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsTreeSelection::SetCurrentColumn

Comment 17

[Daniel Veditz [:dveditz]]

This is a null-deref (!exploitable is being paranoid). It'd be nice to have this patch on 1.9.0, but it's not blocking.

Comment 18

[Samuel Sidler (old account; do not CC)]

Dan flagged this because it affects Thunderbird, but since Thunderbird isn't built on 1.9.0, we're not going to block on this. (CCing Seth for Postbox coverage.)

Comment 19

[Seth Spitzer]

> CCing Seth for Postbox coverage

Thanks for the heads up.

Comment 21

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

Comment on attachment 354168 [details] [diff] [review]
Test case and fix v2

Should we take this to 1.9.0.x? The patch applies cleanly.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 354168 [details] [diff] [review]
Test case and fix v2

Approved for 1.9.0.15, a=dveditz

Comment 23

[Reed Loden [:reed]]

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.60; previous revision: 1.59
done
RCS file: /cvsroot/mozilla/layout/xul/base/src/tree/src/crashtests/399227-1.xul,v
done
Checking in layout/xul/base/src/tree/src/crashtests/399227-1.xul;
/cvsroot/mozilla/layout/xul/base/src/tree/src/crashtests/399227-1.xul,v  <--  399227-1.xul
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/xul/base/src/tree/src/crashtests/399227-2.xul,v
done
Checking in layout/xul/base/src/tree/src/crashtests/399227-2.xul;
/cvsroot/mozilla/layout/xul/base/src/tree/src/crashtests/399227-2.xul,v  <--  399227-2.xul
initial revision: 1.1
done
Checking in layout/xul/base/src/tree/src/crashtests/crashtests.list;
/cvsroot/mozilla/layout/xul/base/src/tree/src/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.9; previous revision: 1.8
done

Comment 24

[Reed Loden [:reed]]

... and follow-up commits because the tests attached weren't what was committed.

Checking in layout/xul/base/src/tree/src/crashtests/399227-1.xul;
/cvsroot/mozilla/layout/xul/base/src/tree/src/crashtests/399227-1.xul,v  <--  399227-1.xul
new revision: 1.2; previous revision: 1.1
done
Checking in layout/xul/base/src/tree/src/crashtests/399227-2.xul;
/cvsroot/mozilla/layout/xul/base/src/tree/src/crashtests/399227-2.xul,v  <--  399227-2.xul
new revision: 1.2; previous revision: 1.1
done

Comment 25

[Reed Loden [:reed]]

In the future, if you land something that isn't what is attached to the bug, please attach a new patch with what was actually committed. It will make life much easier if (such as this case) a patch needs to be landed elsewhere.

Comment 26

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

Verified based on testcases passing for 1.9.0.

Comment 27

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 patch