Closed
Bug 305023
Opened 20 years ago
Closed 19 years ago
crash when accessing nsITreeView reference from javascript after hiding the tree [@ nsTreeSelection::SetCurrentIndex]
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: toddf, Assigned: mozilla)
References
Details
Attachments
(3 files, 1 obsolete file)
886 bytes,
application/x-zip-compressed
|
Details | |
3.39 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
3.38 KB,
patch
|
neil
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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()
Reporter | ||
Updated•20 years ago
|
Component: XP Apps: GUI Features → XUL Widgets
Product: Mozilla Application Suite → Toolkit
Updated•20 years ago
|
Assignee: guifeatures → Jan.Varga
Component: XUL Widgets → XP Toolkit/Widgets: Trees
Product: Toolkit → Core
QA Contact: xptoolkit.trees
Version: unspecified → 1.0 Branch
Updated•20 years ago
|
Version: 1.0 Branch → Trunk
Reporter | ||
Comment 1•20 years ago
|
||
This patch protects calls made on nsTreeSelection::Select, when the mTree
pointer is NULL.
Summary: crash when accessing nsITreeView reference from javascript after hidding the tree → crash when accessing nsITreeView reference from javascript after hidding the tree [@ nsTreeSelection::SetCurrentIndex]
Attachment #193012 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #193012 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 2•20 years ago
|
||
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.
Updated•20 years ago
|
Summary: crash when accessing nsITreeView reference from javascript after hidding the tree [@ nsTreeSelection::SetCurrentIndex] → crash when accessing nsITreeView reference from javascript after hiding the tree [@ nsTreeSelection::SetCurrentIndex]
![]() |
||
Comment 3•20 years ago
|
||
Todd, do you plan to update the patch to Neil's review comments, or should someone else try to look into this?
Reporter | ||
Comment 4•20 years ago
|
||
(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 :-)
Assignee | ||
Comment 5•19 years ago
|
||
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.
Assignee | ||
Comment 6•19 years ago
|
||
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.
Assignee | ||
Comment 7•19 years ago
|
||
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•19 years ago
|
||
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
Attachment #227565 -
Flags: review?(neil)
Updated•19 years ago
|
Attachment #227206 -
Attachment mime type: application/octet-stream → application/x-zip-compressed
Comment 9•19 years ago
|
||
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.
Attachment #227565 -
Flags: review?(neil) → review+
Assignee | ||
Comment 10•19 years ago
|
||
Attachment #227679 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Attachment #227679 -
Flags: review? → review?(neil)
Updated•19 years ago
|
Attachment #227679 -
Flags: review?(neil) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #227679 -
Flags: superreview?(hyatt)
![]() |
||
Comment 11•19 years ago
|
||
Comment on attachment 227679 [details] [diff] [review]
updated patch
Hyatt is inactive...
Attachment #227679 -
Flags: superreview?(hyatt) → superreview+
![]() |
||
Updated•19 years ago
|
Assignee: Jan.Varga → jobor
Status: UNCONFIRMED → NEW
Ever confirmed: true
![]() |
||
Updated•19 years ago
|
Attachment #193012 -
Attachment is obsolete: true
Attachment #193012 -
Flags: superreview?(neil)
Attachment #193012 -
Flags: review?(neil)
![]() |
||
Comment 12•19 years ago
|
||
Checked in on trunk. Thanks for the patch!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•