Last Comment Bug 374584 - because XULCommandDispatcher uses raw pointers, access to deleted object is possible
: because XULCommandDispatcher uses raw pointers, access to deleted object is p...
Status: RESOLVED FIXED
[sg:critical] need very different bra...
: crash, fixed1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (reviewing overload)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-20 02:04 PDT by Olli Pettay [:smaug] (reviewing overload)
Modified: 2008-07-31 03:23 PDT (History)
6 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase for nsXULCommandDispatcher::Updater::mElement (547 bytes, application/vnd.mozilla.xul+xml)
2007-03-20 02:04 PDT, Olli Pettay [:smaug] (reviewing overload)
no flags Details
fix for trunk, using cycle collector (15.20 KB, patch)
2007-03-20 09:03 PDT, Olli Pettay [:smaug] (reviewing overload)
peterv: review+
jst: superreview+
Details | Diff | Splinter Review
branch patch (11.46 KB, patch)
2007-04-03 11:26 PDT, Olli Pettay [:smaug] (reviewing overload)
jst: review+
jonas: superreview+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (reviewing overload) 2007-03-20 02:04:20 PDT
Created attachment 259079 [details]
testcase for nsXULCommandDispatcher::Updater::mElement

The testcase shows the bug with 
nsXULCommandDispatcher::Updater::mElement , 
but it is possible that also nsXULCommandDispatcher::mDocument
and nsXULCommandDispatcher::mFocusController are problematic.

Anyone willing to try to write testcases for the latter ones? ;)
Something where nsXULCommandDispatcher stays alive longer than document
or focuscontroller.
Comment 1 Olli Pettay [:smaug] (reviewing overload) 2007-03-20 02:06:04 PDT
Comment on attachment 259079 [details]
testcase for nsXULCommandDispatcher::Updater::mElement

You may need to click "test" few times before getting the crash.
Comment 2 Olli Pettay [:smaug] (reviewing overload) 2007-03-20 09:03:50 PDT
Created attachment 259092 [details] [diff] [review]
fix for trunk, using cycle collector

This is simple to fix using CC and removing mFocusController.
Running with XPCOM_MEM_LEAK_LOG didn't show any new leaks with the patch.

For branches the possible problems related to mDocument and mFocusController are quite easy fix, but I'm not yet quite sure what to 
do with mElement.
Comment 3 Peter Van der Beken [:peterv] 2007-03-23 06:30:15 PDT
Comment on attachment 259092 [details] [diff] [review]
fix for trunk, using cycle collector

Would be nice to convert mUpdaters and Updater::mNext to nsAutoPtr. Either way, r=peterv.
Comment 4 Olli Pettay [:smaug] (reviewing overload) 2007-03-23 09:58:42 PDT
Comment on attachment 259092 [details] [diff] [review]
fix for trunk, using cycle collector

mUpdater could become also nsTArray<Updater>, but that is different bug anyway.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2007-03-23 14:44:37 PDT
Comment on attachment 259092 [details] [diff] [review]
fix for trunk, using cycle collector

sr=jst
Comment 6 Olli Pettay [:smaug] (reviewing overload) 2007-03-24 04:24:55 PDT
Checked in. Leaks didn't go up.
Comment 7 chris hofmann 2007-03-29 14:20:19 PDT
> For branches the possible problems related to mDocument and mFocusController
> are quite easy fix, but I'm not yet quite sure what to do with mElement.

what are the next steps figure out for the branches as well, or is it only  feasable to try and fix with the cycle collector?  I just tested Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/2007030919 Firefox/2.0.0.3 and it appears to crash on the test case after a couple of clicks...

0x1030eae3
nsXULElement::QueryInterface  [mozilla/content/xul/content/src/nsXULElement.cpp, line 420]
nsXULCommandDispatcher::UpdateCommands  [mozilla/content/xul/document/src/nsXULCommandDispatcher.cpp, line 382]
XPTC_InvokeByIndex  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2169]
XPC_WN_CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1455]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1396]
js_Interpret  [mozilla/js/src/jsinterp.c, line 3975]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1415]
js_InternalInvoke  [mozilla/js/src/jsinterp.c, line 1490]
JS_CallFunctionValue  [mozilla/js/src/jsapi.c, line 4356]
nsJSContext::CallEventHandler  [mozilla/dom/src/base/nsJSEnvironment.cpp, line 1493]
nsJSEventListener::HandleEvent  [mozilla/dom/src/events/nsJSEventListener.cpp, line 195]
nsEventListenerManager::HandleEventSubType  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1655]
nsEventListenerManager::HandleEvent  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1762]
nsXULElement::HandleDOMEvent  [mozilla/content/xul/content/src/nsXULElement.cpp, line 2230]
PresShell::HandleDOMEventWithTarget  [mozilla/layout/base/nsPresShell.cpp, line 6524]
nsButtonBoxFrame::DoMouseClick  [mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp, line 182]
nsButtonBoxFrame::MouseClicked  [mozilla/layout/xul/base/src/nsButtonBoxFrame.h, line 61]
PresShell::HandleEventInternal  [mozilla/layout/base/nsPresShell.cpp, line 6466]
PresShell::HandleEventWithTarget  [mozilla/layout/base/nsPresShell.cpp, line 6323]
nsEventStateManager::CheckForAndDispatchClick  [mozilla/content/events/src/nsEventStateManager.cpp, line 3207]
nsEventStateManager::PostHandleEvent  [mozilla/content/events/src/nsEventStateManager.cpp, line 2170]
PresShell::HandleEventInternal  [mozilla/layout/base/nsPresShell.cpp, line 6497]
PresShell::HandleEvent  [mozilla/layout/base/nsPresShell.cpp, line 6261]
nsViewManager::HandleEvent  [mozilla/view/src/nsViewManager.cpp, line 2559]
nsViewManager::DispatchEvent  [mozilla/view/src/nsViewManager.cpp, line 2246]
HandleEvent  [mozilla/view/src/nsView.cpp, line 174]
nsWindow::DispatchEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 1389]
nsWindow::DispatchMouseEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 6442]
ChildWindow::DispatchMouseEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 6689]
nsWindow::WindowProc  [mozilla/widget/src/windows/nsWindow.cpp, line 1577]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0x89cd (0x77d489cd)
USER32.dll + 0x8a10 (0x77d48a10)
nsAppShell::Run  [mozilla/widget/src/windows/nsAppShell.cpp, line 159]
nsAppStartup::Run  [mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 152]
main  [mozilla/browser/app/nsBrowserApp.cpp, line 61]
kernel32.dll + 0x16fd7 (0x7c816fd7)
Comment 8 Olli Pettay [:smaug] (reviewing overload) 2007-03-29 14:46:55 PDT
Any good suggestions how to fix this on branches are welcome.
Using nsIBoxObject as a weak object might work well enough here,
though if someone passes an element which isn't in document 
to AddCommandUpdater, nsIBoxObject would be null...
Comment 9 Olli Pettay [:smaug] (reviewing overload) 2007-04-03 11:26:06 PDT
Created attachment 260485 [details] [diff] [review]
branch patch

This is ugly. Using the same boxObject hack that has been used in XBL.
This breaks the case where someone is using AddCommandUpdater with an 
element which is not in a document. Or if someone is moving the element in document, because in that case boxObject mContent member is cleared.
In Mozilla AddCommandUpdater is used only here
http://lxr.mozilla.org/mozilla1.8/source/content/xul/templates/src/nsXULContentUtils.cpp#510
Comment 10 Olli Pettay [:smaug] (reviewing overload) 2007-04-03 11:36:21 PDT
nsXULContentUtils.cpp#510 is called only from there
http://lxr.mozilla.org/mozilla1.8/source/content/xul/document/src/nsXULDocument.cpp#1914,
which is called whenever xul element is added to xul document.
So at least in case of xul, things should be ok, I think.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2007-04-11 16:45:38 PDT
Comment on attachment 260485 [details] [diff] [review]
branch patch

r=jst
Comment 12 Olli Pettay [:smaug] (reviewing overload) 2007-04-11 17:07:33 PDT
Comment on attachment 260485 [details] [diff] [review]
branch patch

I'd like to get 2 reviews for this branch patch too.
Comment 13 Daniel Veditz [:dveditz] 2007-04-18 16:29:16 PDT
Comment on attachment 260485 [details] [diff] [review]
branch patch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Comment 14 Marcia Knous [:marcia - use ni] 2007-05-08 15:20:42 PDT
verified on the 1.8 branch using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/2007050804 BonEcho/2.0.0.4pre. The testcase in Comment 1 does not crash. Adding branch verified keyword.

Note You need to log in before you can comment on or make changes to this bug.