305023 - crash when accessing nsITreeView reference from javascript after hiding the tree [@ nsTreeSelection::SetCurrentIndex]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Todd A. Fisher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050816 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050816 Firefox/1.0+

start by creating a xul tree in a xul window.  on load or when convient get a
reference to the tree.view storing in a global object, for example: "var gobj =
new Object(); gobj.view = tree.view;".  

To crash, hide the tree, by doing something like
"tree.setAttribute('hidden','true');", then do something with your global view
reference like: 
"gobj.view.selection.select( next )"

Reproducible: Always

Steps to Reproduce:




The actual source that caused this can be found in my xulmusic player revision
80 - 90, if you leave the playlist collaspsed and it gets to the next song the
player will crash. source:(https://updates.simosoftware.com/repos/xulmusic/)

Also, i'm working on a reduced test case.

Below is the stack trace:
>	xul.dll!nsTreeSelection::SetCurrentIndex(int aIndex=1)  Line 602 + 0x10	C++
 	xul.dll!nsTreeSelection::Select(int aIndex=1)  Line 350	C++
 	xul.dll!XPTC_InvokeByIndex(nsISupports * that=0x02366b50, unsigned int
methodIndex=8, unsigned int paramCount=1, nsXPTCVariant * params=0x0012e7bc) 
Line 102	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2119 + 0x1d	C++
 	xul.dll!XPC_WN_CallMethod(JSContext * cx=0x021defa0, JSObject *
obj=0x023d3708, unsigned int argc=1, long * argv=0x023cc078, long *
vp=0x0012ea90)  Line 1401 + 0xe	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x021defa0, unsigned int argc=1, unsigned
int flags=0)  Line 1173 + 0x20	C
 	js3250.dll!js_Interpret(JSContext * cx=0x021defa0, unsigned char *
pc=0x022a0f15, long * result=0x0012f58c)  Line 3463 + 0xf	C
 	js3250.dll!js_Invoke(JSContext * cx=0x021defa0, unsigned int argc=1, unsigned
int flags=2)  Line 1193 + 0x13	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x021defa0, JSObject *
obj=0x016bc280, long fval=37565848, unsigned int flags=0, unsigned int argc=1,
long * argv=0x023d2a78, long * rval=0x0012f728)  Line 1270 + 0x14	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x021defa0, JSObject *
obj=0x016bc280, long fval=37565848, unsigned int argc=1, long * argv=0x023d2a78,
long * rval=0x0012f728)  Line 3919 + 0x1f	C
 	xul.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x016bc280, JSObject
* aHandler=0x023d3598, unsigned int argc=1, long * argv=0x023d2a78, long *
rval=0x0012f728)  Line 1414 + 0x21	C++
 	xul.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x023d5860)  Line 6030	C++
 	xul.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer=0x023d58d8, void *
aClosure=0x023d5860)  Line 6398	C++
 	xul.dll!nsTimerImpl::Fire()  Line 394 + 0x11	C++
 	xul.dll!nsTimerManager::FireNextIdleTimer()  Line 628	C++
 	xul.dll!nsAppShell::Run()  Line 142	C++
 	xul.dll!nsAppStartup::Run()  Line 145 + 0x1a	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x003d5e9c, const nsXREAppData *
aAppData=0x0012fd80)  Line 2322 + 0x23	C++
 	xulrunner.exe!main(int argc=1, char * * argv=0x003d5e9c)  Line 412 + 0x15	C++
 	xulrunner.exe!mainCRTStartup()  Line 398 + 0x11	C
 	kernel32.dll!7c816d4f() 	
 	kernel32.dll!7c8399f3()

Comment 1

[Todd A. Fisher]

Attachment: Protect against accessing a null mTree pointer

This patch protects calls made on nsTreeSelection::Select, when the mTree
pointer is NULL.

Comment 2

[[e-mail]]

Comment on attachment 193012 [details] [diff] [review]
Protect against accessing a null mTree pointer

>@@ -356,17 +358,19 @@ NS_IMETHODIMP nsTreeSelection::Select(PR
>         // We need to deselect everything but our item.
>         mFirstRange->RemoveAllBut(aIndex);
>         FireOnSelectHandler();
>       }
>       return NS_OK;
>     }
>-    else {
>+    else if( mTree ){
>       // Clear out our selection.
>       mFirstRange->Invalidate();
>       delete mFirstRange;
>     }
>+    else
>+      return NS_OK;
This looks wrong, there was no return in the else case before (else after
return is of course bad style [speaking of which, space required between ) and
{] but now I know why!) also you've already null-checked Invalidate.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Todd, do you plan to update the patch to Neil's review comments, or should someone else try to look into this?

Comment 4

[Todd A. Fisher]

(In reply to comment #3)
> Todd, do you plan to update the patch to Neil's review comments, or should
> someone else try to look into this?
> 

I probably won't be able to get to this for awhile... would really like to do more, but if you have the time to fix it correctly by all means don't let me stop you :-)

Comment 5

[Joerg Bornemann]

The proposed patch seems to be somehow incomplete.
"select" is not the only function that leads to a crash due to a null pointer reference. Another example is "selectAll". I will create a simple test case for this bug and then try to fix it.

Comment 6

[Joerg Bornemann]

Attachment: XUL test case

This zip archive contains a xul and a js file.
Load treetest.xul into mozilla and press "hide".
Then press "select" f.e. and watch the crash.

Comment 7

[Joerg Bornemann]

Attachment: mTree null pointer checks

This patch prevents crashes when accessing the view object of a hidden tree.
Unfortunatly will SelectAll not work (as the other methods do) on the hidden tree.
See the test case. Please review.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 227565 [details] [diff] [review]
mTree null pointer checks

So in general you want to request review from the module owner...  See http://www.mozilla.org/owners.html

Comment 9

[[e-mail]]

Comment on attachment 227565 [details] [diff] [review]
mTree null pointer checks

> NS_IMETHODIMP nsTreeSelection::SelectAll()
> {
>   nsCOMPtr<nsITreeView> view;
>+  if (!mTree)
>+    return NS_OK;
>+
Should be before the nsCOMPtr. With that, r=me.

Comment 10

[Joerg Bornemann]

Attachment: updated patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 227679 [details] [diff] [review]
updated patch

Hyatt is inactive...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.  Thanks for the patch!