Closed Bug 399227 Opened 14 years ago Closed 13 years ago

Crash @ nsTreeSelection::GetSingle, null mTree

Categories

(Toolkit :: XUL Widgets, defect)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: skrulx, Assigned: standard8)

References

Details

(Keywords: crash, verified1.9.0.15, verified1.9.1, Whiteboard: [sg:dos])

Attachments

(3 files, 2 obsolete files)

Attached file 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.
OS: Linux → All
(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.
FWIW SetCurrentColumn doesn't look as if it null-checks mTree either.
Severity: normal → critical
Keywords: crash
Attached patch Test cases and possible fix. (obsolete) — Splinter Review
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.
Attached patch Test case and fix. (obsolete) — Splinter Review
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.
Assignee: nobody → bugzilla
Attachment #350046 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #351701 - Flags: superreview?(neil)
Attachment #351701 - Flags: review?(enndeakin)
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]
Attachment #351701 - Flags: superreview?(neil) → superreview+
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.
Attachment #351701 - Flags: review?(enndeakin) → review+
(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.
With slight adjustment in SetCurrentColumn as previously mentioned. Carrying forward r/sr.
Attachment #351701 - Attachment is obsolete: true
Attachment #354168 - Flags: superreview+
Attachment #354168 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
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.
Attachment #354168 - Flags: approval1.9.1?
Comment on attachment 354168 [details] [diff] [review]
Test case and fix v2

a191=beltzner
Attachment #354168 - Flags: approval1.9.1? → approval1.9.1+
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
Status: RESOLVED → VERIFIED
1.9.0 !exploitable report:
PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsTreeSelection::GetSingle
Flags: blocking1.9.0.11?
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
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.
Flags: blocking1.9.0.11? → wanted1.9.0.x+
Whiteboard: [sg:dos]
Flags: blocking1.9.0.13?
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.)
Flags: blocking1.9.0.13?
> CCing Seth for Postbox coverage

Thanks for the heads up.
Duplicate of this bug: 489546
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 on attachment 354168 [details] [diff] [review]
Test case and fix v2

Approved for 1.9.0.15, a=dveditz
Attachment #354168 - Flags: approval1.9.0.15? → approval1.9.0.15+
Keywords: checkin-needed
Whiteboard: [sg:dos] → [sg:dos][checkin needed for 1.9.0.x]
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
Whiteboard: [sg:dos][checkin needed for 1.9.0.x] → [sg:dos]
... 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
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.
Verified based on testcases passing for 1.9.0.
You need to log in before you can comment on or make changes to this bug.