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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

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][[e-mail]]]

Attachment: 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]]

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][[e-mail]]]

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)]

Comment on attachment 259092 [details] [diff] [review]
fix for trunk, using cycle collector

sr=jst

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Checked in. Leaks didn't go up.

Comment 7

[chris hofmann]

> 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][[e-mail]]]

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][[e-mail]]]

Attachment: 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][[e-mail]]]

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)]

Comment on attachment 260485 [details] [diff] [review]
branch patch

r=jst

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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]]

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]]

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.