Last Comment Bug 334515 - crash initialising iframe as html edit where html loaded contains a second iframe [@ nsQueryInterface::operator()]
: crash initialising iframe as html edit where html loaded contains a second if...
Status: VERIFIED FIXED
[camino-1.0.1]
: crash, fixed1.8.1, testcase, verified1.8.0.3, verified1.8.0.4
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks: 211348 299676 331981 337040
  Show dependency treegraph
 
Reported: 2006-04-18 11:54 PDT by Nick Mott
Modified: 2006-07-14 17:34 PDT (History)
21 users (show)
dveditz: blocking1.8.0.3+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reporter's testcase (742 bytes, text/html)
2006-04-18 12:45 PDT, Uri Bernstein (Google)
no flags Details
updated test case (765 bytes, text/html)
2006-04-18 13:16 PDT, Nick Mott
no flags Details
details of the crash (116.95 KB, text/plain)
2006-04-18 13:20 PDT, Nick Mott
no flags Details
Patch to fix (5.34 KB, patch)
2006-04-24 17:35 PDT, Jonas Sicking (:sicking) PTO Until July 5th
dbaron: review+
dbaron: superreview+
dbaron: approval‑branch‑1.8.1+
Details | Diff | Review
1.8 branch patch (includes bug 332132) (5.56 KB, patch)
2006-04-25 11:18 PDT, Daniel Veditz [:dveditz]
jonas: review+
Details | Diff | Review
Minimal patch for the 1.8.0 branch (5.28 KB, patch)
2006-04-25 15:03 PDT, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: approval1.8.0.3+
dveditz: approval1.8.0.4+
Details | Diff | Review
Unregress editing (5.04 KB, patch)
2006-04-26 16:39 PDT, Jonas Sicking (:sicking) PTO Until July 5th
dbaron: review+
dbaron: superreview+
dbaron: approval‑branch‑1.8.1+
Details | Diff | Review
Unregress editing v2 (5.59 KB, patch)
2006-04-26 16:52 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Review
Unregress editing, branch version (5.52 KB, patch)
2006-04-26 16:59 PDT, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.3+
dveditz: approval1.8.0.4+
Details | Diff | Review
Full combined patch (6.25 KB, patch)
2006-04-26 17:09 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Review

Description Nick Mott 2006-04-18 11:54:30 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

Setting an iframe as an html edit frame and loading html content which contains a second iframe crashes Firefox 1.5.0.2 (have not tried other versions)

The content seems to load, but then the iframe content disappears. Clicking on the iframe then causes Firefox to crash.



Reproducible: Always

Steps to Reproduce:
1. Create a html page with the following code:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<title>Untitled Document</title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<script>
function initIframe(){
var html;
html = "<html><head></head><body>Hello World!<br><iframe src=\"http://www.mozilla.org\">&nbsp;</iframe></body></html>";
var RTE_example = document.getElementById("example");
RTE_example.contentWindow.document.designMode = "on";
RTE_example.contentWindow.document.open();
RTE_example.contentWindow.document.write(html);
RTE_example.contentWindow.document.close();
};
</script>
</head>
<body onLoad="javascript: initIframe();">
<iframe id="example"><html><head></head><body>&nbsp;</body></html></iframe>
</body>
</html>

2. load the page
3. click on the iframe

Actual Results:  
the expected content is not displayed and Firefox crahses.

Expected Results:  
an editable iframe should have been produced.
Comment 1 Uri Bernstein (Google) 2006-04-18 12:45:02 PDT
Created attachment 218865 [details]
reporter's testcase
Comment 2 Nick Mott 2006-04-18 13:16:26 PDT
Created attachment 218871 [details]
updated test case

The previous test test missed a space between 'iframe' and 'src'.
Comment 3 Nick Mott 2006-04-18 13:20:51 PDT
Created attachment 218873 [details]
details of the crash
Comment 4 Adam Guthrie 2006-04-18 14:10:07 PDT
Could you please install Talkback and get a Talkback ID for the crash? http://kb.mozillazine.org/Talkback
Comment 5 Nickolay_Ponomarev 2006-04-18 14:17:03 PDT
No need for talkback id.

xpcom_core.dll!nsQueryInterface::operator()(const nsID & aIID={...}, void * * answer=0x0012c368)  Line 47
editor.dll!nsCOMPtr<nsIEditor>::assign_from_qi(nsQueryInterface qi={...}, const nsID & aIID={...})  Line 1232
editor.dll!nsCOMPtr<nsIEditor>::nsCOMPtr<nsIEditor>(nsQueryInterface qi={...})  Line 646
editor.dll!nsUndoCommand::IsCommandEnabled(const char * aCommandName=0x04663148, nsISupports * aCommandRefCon=0x0483afe0, int * outCmdEnabled=0x0012c564)  Line 74
embedcomponents.dll!nsControllerCommandTable::IsCommandEnabled(const char * aCommandName=0x04663148, nsISupports * aCommandRefCon=0x0483afe0, int * aResult=0x0012c564)  Line 138
embedcomponents.dll!nsBaseCommandController::IsCommandEnabled(const char * aCommand=0x04663148, int * aResult=0x0012c564)  Line 118
xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x00000003, unsigned int methodIndex=2, unsigned int paramCount=1230164, nsXPTCVariant * params=0x00000000)  Line 102
xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=3)  Line 2155
xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2155
xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x03b47308, JSObject * obj=0x04442aa0, unsigned int argc=1, long * argv=0x048cda84, long * vp=0x0012c818)  Line 1444
js3250.dll!js_Invoke(JSContext * cx=0x03b47308, unsigned int argc=1, unsigned int flags=0)  Line 1246
js3250.dll!js_Interpret(JSContext * cx=0x03b47308, unsigned char * pc=0x03a87760, long * result=0x0012d2cc)  Line 3902
js3250.dll!js_Invoke(JSContext * cx=0x03b47308, unsigned int argc=1, unsigned int flags=2)  Line 1270
js3250.dll!js_InternalInvoke(JSContext * cx=0x03b47308, JSObject * obj=0x044ce850, long fval=73556936, unsigned int flags=0, unsigned int argc=1, long * argv=0x0012d558, long * rval=0x0012d560)  Line 1347
js3250.dll!JS_CallFunctionValue(JSContext * cx=0x03b47308, JSObject * obj=0x044ce850, long fval=73556936, unsigned int argc=1, long * argv=0x0012d558, long * rval=0x0012d560)  Line 4186
gklayout.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x044ce850, JSObject * aHandler=0x046263c8, unsigned int argc=1, long * argv=0x0012d558, long * rval=0x0012d560)  Line 1426
gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x0485a690)  Line 186
gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x0440ba00, nsIDOMEventListener * aListener=0x0440b958, nsIDOMEvent * aDOMEvent=0x0485a690, nsISupports * aCurrentTarget=0x0440b8a0, unsigned int aSubType=32, unsigned int aPhaseFlags=6)  Line 1636
gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x00cd06e0, nsEvent * aEvent=0x0012d9c4, nsIDOMEvent * * aDOMEvent=0x0012d77c, nsISupports * aCurrentTarget=0x0440b8a0, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012d780)  Line 1740
gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 335
gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 455
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 401
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x0440b8a0, nsPresContext * aPresContext=0x00cd06e0, nsEvent * aEvent=0x0012d9c4, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012d9c0, nsDispatchingCallback * aCallback=0x00000000, int aTargetIsChromeHandler=0)  Line 575
gklayout.dll!nsXULCommandDispatcher::UpdateCommands(const nsAString_internal & aEventName={...})  Line 415
gklayout.dll!nsGlobalWindow::UpdateCommands(const nsAString_internal & anAction={...})  Line 4472
gklayout.dll!nsFocusController::UpdateCommands()  Line 217
gklayout.dll!nsFocusController::Focus(nsIDOMEvent * aEvent=0x04637cf0)  Line 362
gklayout.dll!DispatchToInterface(nsIDOMEvent * aEvent=0x04637cf0, nsIDOMEventListener * aListener=0x03b4714c, unsigned int (nsIDOMEvent *)* aMethod=0x0187ce50, const nsID & aIID={...}, int * aHasInterface=0x0012dc34)  Line 143
gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x04892528, nsEvent * aEvent=0x0012e388, nsIDOMEvent * * aDOMEvent=0x0012dd18, nsISupports * aCurrentTarget=0x03a83454, unsigned int aFlags=4, nsEventStatus * aEventStatus=0x0012dd1c)  Line 1730
gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=4)  Line 335
gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 429
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 401
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392
gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x048d14f8, nsPresContext * aPresContext=0x04892528, nsEvent * aEvent=0x0012e388, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012e384, nsDispatchingCallback * aCallback=0x00000000, int aTargetIsChromeHandler=0)  Line 575
gklayout.dll!nsEventStateManager::PreHandleEvent(nsPresContext * aPresContext=0x04892528, nsEvent * aEvent=0x0012e6f8, nsIFrame * aTargetFrame=0x049376bc, nsEventStatus * aStatus=0x0012e50c, nsIView * aView=0x03b58228)  Line 639
gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012e6f8, nsIView * aView=0x03b58228, nsEventStatus * aStatus=0x0012e50c)  Line 6120
gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x03b58228, nsGUIEvent * aEvent=0x0012e6f8, nsEventStatus * aEventStatus=0x0012e50c)  Line 5901
gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x03b58228, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012e6f8, int aCaptured=0)  Line 1712
gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012e6f8, nsEventStatus * aStatus=0x0012e634)  Line 1665
gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012e6f8)  Line 174
gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012e6f8, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1100
gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012e6f8)  Line 1121
gkwidget.dll!nsWindow::DispatchFocus(unsigned int aEventType=105, int isMozWindowTakingFocus=1)  Line 6215
gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=7, unsigned int wParam=4981478, long lParam=0, long * aRetValue=0x0012eaf8)  Line 4752
gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x002f0566, unsigned int msg=7, unsigned int wParam=4981478, long lParam=0)  Line 1289
user32.dll!77d48734()
[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]
user32.dll!77d48816()
user32.dll!77d4b4c0()
user32.dll!77d4b50c()
ntdll.dll!7c90eae3()
user32.dll!77d4da6c()
gkwidget.dll!nsWindow::SetFocus(int aRaise=1)  Line 2156
gklayout.dll!nsEventStateManager::SendFocusBlur(nsPresContext * aPresContext=0x04892528, nsIContent * aContent=0x00000000, int aEnsureWindowHasFocus=1)  Line 4291
gklayout.dll!nsEventStateManager::SetContentState(nsIContent * aContent=0x00000000, int aState=2)  Line 3877
gklayout.dll!nsEventStateManager::PostHandleEvent(nsPresContext * aPresContext=0x04892528, nsEvent * aEvent=0x0012f654, nsIFrame * aTargetFrame=0x04950874, nsEventStatus * aStatus=0x0012f418, nsIView * aView=0x03b58228)  Line 1944
gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f654, nsIView * aView=0x03b58228, nsEventStatus * aStatus=0x0012f418)  Line 6148
gklayout.dll!PresShell::HandlePositionedEvent(nsIView * aView=0x03b58228, nsIFrame * aTargetFrame=0x04950874, nsGUIEvent * aEvent=0x0012f654, nsEventStatus * aEventStatus=0x0012f418)  Line 6005
gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x047eb720, nsGUIEvent * aEvent=0x0012f654, nsEventStatus * aEventStatus=0x0012f418)  Line 5826
gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x047eb720, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012f654, int aCaptured=0)  Line 1712
gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f654, nsEventStatus * aStatus=0x0012f540)  Line 1665
gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f654)  Line 174
gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f654, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1100
gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f654)  Line 1121
gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=302, unsigned int wParam=1, long lParam=393222)  Line 6095
gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int aEventType=302, unsigned int wParam=1, long lParam=393222)  Line 6278
gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=513, unsigned int wParam=1, long lParam=393222, long * aRetValue=0x0012fac0)  Line 4568
gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x001f0630, unsigned int msg=513, unsigned int wParam=1, long lParam=393222)  Line 1289
user32.dll!77d48734()
user32.dll!77d48816()
user32.dll!77d489cd()
user32.dll!77d49402()
user32.dll!77d48a10()
gkwidget.dll!nsAppShell::Run()  Line 135
tkitcmps.dll!nsAppStartup::Run()  Line 161
xul.dll!XRE_main(int argc=3, char * * argv=0x00ac7458, const nsXREAppData * aAppData=0x004036b0)  Line 2364
firefox.exe!main(int argc=3, char * * argv=0x00ac7458)  Line 61
firefox.exe!__tmainCRTStartup()  Line 586
firefox.exe!mainCRTStartup()  Line 403
kernel32.dll!7c816d4f()
kernel32.dll!7c8399f3()
Comment 6 Nickolay_Ponomarev 2006-04-18 14:42:36 PDT
The closest bug I could find was bug 299676.

Confirming using a trunk build from ~16-th.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-24 17:35:20 PDT
Created attachment 219692 [details] [diff] [review]
Patch to fix

This fixes the crash. I no longer crash from the testcases in this bug, bug 331981 and bug 211348.

I still see very strange behaviour from the testcase in bug 331981, but at least the security issue seems to be plugged.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-04-24 17:53:36 PDT
Comment on attachment 219692 [details] [diff] [review]
Patch to fix

sr=me, fwiw.  Thanks for doing this!
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-25 00:33:25 PDT
So this patch should be very safe to land with a minimal risk of regression. The patch basically makes us use safe mechanisms for avoiding access to deleted objects, rather than relying on that users clear out the pointer manually.

The only downside to the patch is that not all objects can be used as context, only objects that supports weak references. However all the objects that we use today already do so it should be no problem for existing code.

I havn't investigated yet how exploitable this crash is. I'll do that tomorrow.
Comment 10 Daniel Veditz [:dveditz] 2006-04-25 11:18:11 PDT
Created attachment 219775 [details] [diff] [review]
1.8 branch patch (includes bug 332132)

The trunk patch didn't apply cleanly to the 1.8 branch because bug 332132 recently landed on trunk, fixing an OOM crash and changing some whitespace. This patch applies cleanly and I went ahead and added the null checks from 332132.
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-25 12:03:21 PDT
Comment on attachment 219775 [details] [diff] [review]
1.8 branch patch (includes bug 332132)

Btw, this is already landed on the 1.8 branch. Still needs to go on the 1.8.0.3 branch
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-25 15:03:40 PDT
Created attachment 219799 [details] [diff] [review]
Minimal patch for the 1.8.0 branch

This is a minimal patch for the 1.8.0 branch that only includes the parts neccesary to fix the crash.

(This is also what I landed yesterday on the 1.8.1 branch)
Comment 13 Daniel Veditz [:dveditz] 2006-04-25 15:54:49 PDT
Comment on attachment 219799 [details] [diff] [review]
Minimal patch for the 1.8.0 branch

approved for the 1.8.0 branch, a=dveditz for drivers.
Comment 14 Tim Riley [:timr] 2006-04-25 16:36:40 PDT
Comment on attachment 219799 [details] [diff] [review]
Minimal patch for the 1.8.0 branch

Does this patch still fix the crash in  bugs that the current bug depends on: bug
331981 and bug 211348.  See comment #7 regarding attachment 219692 [details] [diff] [review]
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-25 17:06:40 PDT
Yes it does
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-25 17:29:26 PDT
So I'm beginning to wonder if any extensions could depend on this object -- it's got a contractID, so they *could* be using it, which means this *could* break them.

Any thoughts on what, if anything, we should do about that?


For the suite, according to sicking, we apparently also need to do stuff for the editor XBL binding, since editor elements are passed as context by the editor code.
Comment 17 Daniel Veditz [:dveditz] 2006-04-25 17:33:37 PDT
Comment on attachment 219799 [details] [diff] [review]
Minimal patch for the 1.8.0 branch

approved for 1.8.0.3 mini-branch
Comment 18 Benjamin Smedberg [:bsmedberg] 2006-04-25 17:44:51 PDT
nsBaseCommandController is not a pseudo-interface: we aren't worrying about changes to concrete classes, just interfaces and pseudo-interfaces such as nsIDocument/nsIContent
Comment 19 Brendan Eich [:brendan] 2006-04-25 17:54:58 PDT
I've said this in person to everyone involved, but for the record:

* We are incompatibly changing an API to require nsISupportsWeakReference be implemented by the nsISupports * doodad.

* That fixes a potentially exploitable security bug, good.  Alternative of holding a strong ref looks like a leak hazard.

* An API is a contract, which requires all of our code to comply (or be sued!).

* We therefore ought to take the second patch that Jonas has developed, to fix the editor XBL to comply with the new API.

/be
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-25 18:16:44 PDT
It is a relatively important semantic change to nsIControllerContext, even though the signature is unchanged.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-25 18:18:46 PDT
Speaking of which, we should probably document that change in nsIControllerContext.idl.
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-25 18:24:29 PDT
I have a patch that changes nsIControllerContext.idl that I used to verify that all internal callers were ok. I'll file a new bug on that and attach the patch there.


I am worried about extensions too, but at the time I don't see another solution than the current one. Unfortunatly the code in question is too unowned :(
Comment 23 Daniel Veditz [:dveditz] 2006-04-25 19:00:41 PDT
Just thought I should note this does not affect Firefox 1.0.x or Suite 1.7.x: Martijn Wargers notes in bug 331981 comment 0:

   This is a regression.
   It doesn't crash in a 2005-04-05 build, it does crash in a 2005-04-06 build,
   so my bet is on bug 283897.

That's a year after the aviary/moz1.7 branches, and I've confirmed that Firefox 1.0.8 is not affected.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-04-25 19:44:10 PDT
So one thing we could do wrt extensions is that if the nsISupports passed in doesn't support nsISupportsWeakReference then we store an nsISupports* like we used to.  This does mean that extensions that use this stuff will suffer from issues like this bug if we fail to unset contexts, of course....

The reason bug 283897 was likely relevant here is that it changed editor's behavior when doing a load in an iframe which is in designMode.  Maybe we're not clearing contexts in that case properly for some reason?  Say we try to work with a window's controllers after it's already dropped them or something?
Comment 25 Bob Clary [:bc:] 2006-04-26 08:23:22 PDT
No crash in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3 with attachment 218871 [details]
Comment 26 Kathleen Brade 2006-04-26 14:21:26 PDT
The patch in attachment 219775 [details] [diff] [review] also fixes a crash in my company's Pearl Comments extension.
Comment 27 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-26 16:39:19 PDT
Created attachment 219951 [details] [diff] [review]
Unregress editing
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-26 16:46:12 PDT
Comment on attachment 219951 [details] [diff] [review]
Unregress editing

You should initialize mCommandContextRawPtr to nsnull in the constructor, and maybe propagate errors from GetWeakReference.  With that, r+sr=dbaron.
Comment 29 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-26 16:52:25 PDT
Created attachment 219953 [details] [diff] [review]
Unregress editing v2

Fixes review comments
Comment 30 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-26 16:59:40 PDT
Created attachment 219954 [details] [diff] [review]
Unregress editing, branch version

This one applies to the 1.8.* branches
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-26 17:09:37 PDT
Created attachment 219958 [details] [diff] [review]
Full combined patch

This is the full combined patch for distributors and private trees and the like.
Comment 32 Daniel Veditz [:dveditz] 2006-04-26 17:31:38 PDT
Comment on attachment 219953 [details] [diff] [review]
Unregress editing v2

post facto a=dveditz for Firefox 1.5.0.3 minibranch, 1.8.0 branch, and 1.8 branch
Comment 33 Daniel Veditz [:dveditz] 2006-04-26 17:33:18 PDT
Comment on attachment 219953 [details] [diff] [review]
Unregress editing v2

oops, wrong one.
Comment 34 Daniel Veditz [:dveditz] 2006-04-26 17:33:41 PDT
Comment on attachment 219954 [details] [diff] [review]
Unregress editing, branch version

a=dveditz for branch landing (for real this time)
Comment 35 Daniel Veditz [:dveditz] 2006-04-26 17:34:32 PDT
Comment on attachment 219775 [details] [diff] [review]
1.8 branch patch (includes bug 332132)

ignore this one, handle bug 332132 elsewhere
Comment 36 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-26 18:52:49 PDT
Marking this bug fixed. I'll file separate bugs on remaining work to do on the trunk.
Comment 37 Stephen Donner [:stephend] 2006-04-27 19:55:56 PDT
Verified FIXED using 04-27-16 build of SeaMonkey trunk on Windows XP.
Comment 38 Daniel Veditz [:dveditz] 2006-04-28 17:46:51 PDT
This bug has the same root cause as the Firefox DoS that hit the press this past week. Dupe-finding fodder:

MozillaZine:      http://forums.mozillazine.org/viewtopic.php?t=408603
Vuln PoC home:    http://www.securident.com/vuln/ff.txt
Bugtraq posting:  http://www.securityfocus.com/archive/1/431878/30/0/
Secunia:          http://secunia.com/advisories/19802/
Comment 39 Daniel Veditz [:dveditz] 2006-04-28 18:07:46 PDT
Also http://securitytracker.com/alerts/2006/Apr/1015981.html
Comment 40 Nick Mott 2006-05-03 01:27:13 PDT
Just updated to 1.5.0.3

Firefox no longer crashes, but the iframe still does not function properly. 

The contents of the outer iframe still do not load correctly. I should be able to edit text inside around the embedded iframe.

Nick 
 
Comment 41 Juha-Matti Laurio 2006-05-03 07:16:32 PDT
This has been assigned to 
http://www.securityfocus.com/bid/17671
http://www.securiteam.com/windowsntfocus/5FP0Q20IBY.html
and
http://www.frsirt.com/english/advisories/2006/1614 (Critical Risk 4/4)
too.
US-CERT VU#866300 at http://www.kb.cert.org/vuls/id/866300 is public as well.
Comment 42 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-05-03 07:50:34 PDT
Nick, could you please file a followup bug on that?  Thanks!
Comment 43 Robert Stopp (textrinum) 2006-05-07 09:46:02 PDT
the Problem is the function document.close()
if an iframe is in the written html (maybe it closes that content and gif back a editable reference to that)
if the html is later added everthing works fine
Comment 44 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-07 09:52:43 PDT
Robert (Nick?), please file follow-up bugs for the issues you're seeing (preferably with a testcase), and mention the bug number here.
Comment 45 Robert Stopp (textrinum) 2006-05-07 11:50:46 PDT
for "document.close()" with written iframe an behavior discussed
the new created bug is: bug 337040 
(with testcase and description)

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