Closed Bug 374584 Opened 17 years ago Closed 17 years ago

because XULCommandDispatcher uses raw pointers, access to deleted object is possible

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

Details

(Keywords: crash, fixed1.8.0.12, verified1.8.1.4, Whiteboard: [sg:critical] need very different branch patch)

Attachments

(3 files)

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 on attachment 259079 [details]
testcase for nsXULCommandDispatcher::Updater::mElement

You may need to click "test" few times before getting the crash.
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.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #259092 - Flags: review?(peterv)
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.
Attachment #259092 - Flags: review?(peterv) → review+
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.
Attachment #259092 - Flags: superreview?(jst)
Comment on attachment 259092 [details] [diff] [review]
fix for trunk, using cycle collector

sr=jst
Attachment #259092 - Flags: superreview?(jst) → superreview+
Checked in. Leaks didn't go up.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
> 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)
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
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...
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Keywords: crash
Whiteboard: [sg:critical] need very different branch patch
Flags: blocking1.9?
Attached patch branch patchSplinter Review
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
Attachment #260485 - Flags: review?(jst)
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 on attachment 260485 [details] [diff] [review]
branch patch

r=jst
Attachment #260485 - Flags: review?(jst) → review+
Comment on attachment 260485 [details] [diff] [review]
branch patch

I'd like to get 2 reviews for this branch patch too.
Attachment #260485 - Flags: superreview?(peterv)
Attachment #260485 - Flags: superreview?(peterv) → superreview+
Attachment #260485 - Flags: approval1.8.1.4?
Attachment #260485 - Flags: approval1.8.0.12?
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
Attachment #260485 - Flags: approval1.8.1.4?
Attachment #260485 - Flags: approval1.8.1.4+
Attachment #260485 - Flags: approval1.8.0.12?
Attachment #260485 - Flags: approval1.8.0.12+
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.
Group: security
Flags: in-testsuite?
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.