271130 - editting observatoryscope, removing framesets repeatedly finally croaks [@ nsHTMLEditor::GetCSSBackgroundColorState]

Status: RESOLVED WONTFIX

(Core :: DOM: Editor, defect)

Description

[timeless]

this page is supposed to crash print preview in 1.7.3 or something, but it's
stubborn and refuses to crash my trunk build, so i decided to take it for a spin
through composer.

note that i have some patches that alter how <noframes> content is handled, but
i don't think that really relates too much to my crash (well, it'll be a crash
if i don't do something about the assert).

An error occurred updating the cmd_ul command:
[Exception... "Component returned failure code: 0x80004003
(NS_ERROR_INVALID_POINTER) [nsICommandController.getCommandStateWithParams]" 
nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
chrome://editor/content/ComposerCommands.js :: goUpdateCommandState :: line 258"
 data: no]
An error occurred updating the cmd_ol command:
[Exception... "Component returned failure code: 0x80004003
(NS_ERROR_INVALID_POINTER) [nsICommandController.getCommandStateWithParams]" 
nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
chrome://editor/content/ComposerCommands.js :: goUpdateCommandState :: line 258"
 data: no]
An error occurred updating the cmd_paragraphState command:
[Exception... "Component returned failure code: 0x80004003
(NS_ERROR_INVALID_POINTER) [nsICommandController.getCommandStateWithParams]" 
nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
chrome://editor/content/ComposerCommands.js :: goUpdateCommandState :: line 258"
 data: no]
WARNING: NS_ENSURE_TRUE(aNode) failed, file
r:/mozilla/editor/libeditor/html/nsHTMLCSSUtils.cpp, line 1409
###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().:
'mRawPtr != 0', file ../../../dist/include/xpcom\nsCOMPtr.h, line 712
Break: at file ../../../dist/include/xpcom\nsCOMPtr.h, line 712

 	xpcom_core.dll!nsDebug::Assertion(const char * aStr=0x041a8b44, const char *
aExpr=0x041a8b88, const char * aFile=0x041a8b98, int aLine=0x000002c8)  Line 109	C++
 	editor.dll!nsCOMPtr<nsIDOMNode>::operator->()  Line 712 + 0x22	C++
>	editor.dll!nsHTMLEditor::GetCSSBackgroundColorState(int * aMixed=0x0012cf34,
nsAString & aOutColor={...}, int aBlockLevel=0x00000001)  Line 2498 + 0x8	C++
 	editor.dll!nsHTMLEditor::GetBackgroundColorState(int * aMixed=0x0012cf34,
nsAString & aOutColor={...})  Line 2399 + 0x18	C++
 	composer.dll!nsBackgroundColorStateCommand::GetCurrentState(nsIEditor *
aEditor=0x0518bdb0, nsICommandParams * aParams=0x050cc0e8)  Line 1004 + 0x2b	C++
 	composer.dll!nsMultiStateCommand::GetCommandStateParams(const char *
aCommandName=0x052f3948, nsICommandParams * aParams=0x050cc0e8, nsISupports *
refCon=0x0518bdb0)  Line 681 + 0x18	C++
 	embedcomponents.dll!nsControllerCommandTable::GetCommandState(const char *
aCommandName=0x052f3948, nsICommandParams * aParams=0x050cc0e8, nsISupports *
aCommandRefCon=0x0518bdb0)  Line 226 + 0x23	C++
 	embedcomponents.dll!nsBaseCommandController::GetCommandStateWithParams(const
char * aCommand=0x052f3948, nsICommandParams * aParams=0x050cc0e8)  Line 148	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x0518b8ac, unsigned int
methodIndex=0x00000003, unsigned int paramCount=0x00000002, nsXPTCVariant *
params=0x0012d0f4)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2037 + 0x1e	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x03f6eba8, JSObject *
obj=0x04ff0118, unsigned int argc=0x00000002, long * argv=0x050a9008, long *
vp=0x0012d3c0)  Line 1287 + 0xb	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x03f6eba8, unsigned int argc=0x00000002,
unsigned int flags=0x00000000)  Line 1286 + 0x20	C
 	js3250.dll!js_Interpret(JSContext * cx=0x03f6eba8, long * result=0x0012de80) 
Line 3619 + 0xf	C
 	js3250.dll!js_Invoke(JSContext * cx=0x03f6eba8, unsigned int argc=0x00000001,
unsigned int flags=0x00000002)  Line 1306 + 0xd	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x03f6eba8, JSObject *
obj=0x04f08d88, long fval=0x04f08d98, unsigned int flags=0x00000000, unsigned
int argc=0x00000001, long * argv=0x0012e180, long * rval=0x0012e184)  Line
1383 + 0x14	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x03f6eba8, JSObject *
obj=0x04f08d88, long fval=0x04f08d98, unsigned int argc=0x00000001, long *
argv=0x0012e180, long * rval=0x0012e184)  Line 3794 + 0x1f	C
 	gklayout.dll!nsJSContext::CallEventHandler(JSObject * aTarget=0x04f08d88,
JSObject * aHandler=0x04f08d98, unsigned int argc=0x00000001, long *
argv=0x0012e180, long * rval=0x0012e184)  Line 1361 + 0x21	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x0539ddd8) 
Line 205 + 0x2d	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct *
aListenerStruct=0x04dd4560, nsIDOMEvent * aDOMEvent=0x0539ddd8,
nsIDOMEventTarget * aCurrentTarget=0x051a2a90, unsigned int aSubType=0x00000020,
unsigned int aPhaseFlags=0x00000007)  Line 1524 + 0x14	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext *
aPresContext=0x03f961f0, nsEvent * aEvent=0x0012e714, nsIDOMEvent * *
aDOMEvent=0x0012e6b4, nsIDOMEventTarget * aCurrentTarget=0x051a2a90, unsigned
int aFlags=0x00000007, nsEventStatus * aEventStatus=0x0012e710)  Line 1618	C++
 	gklayout.dll!nsXULElement::HandleDOMEvent(nsPresContext *
aPresContext=0x03f961f0, nsEvent * aEvent=0x0012e714, nsIDOMEvent * *
aDOMEvent=0x0012e6b4, unsigned int aFlags=0x00000007, nsEventStatus *
aEventStatus=0x0012e710)  Line 2820	C++
 	gklayout.dll!nsXULCommandDispatcher::UpdateCommands(const nsAString &
aEventName={...})  Line 384	C++
 	gklayout.dll!GlobalWindowImpl::UpdateCommands(const nsAString &
anAction={...})  Line 3647	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x03d133c4, unsigned int
methodIndex=0x00000052, unsigned int paramCount=0x00000001, nsXPTCVariant *
params=0x0012e9cc)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2037 + 0x1e	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x018df598, JSObject *
obj=0x03dd5be8, unsigned int argc=0x00000001, long * argv=0x05274078, long *
vp=0x0012ec98)  Line 1287 + 0xb	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x018df598, unsigned int argc=0x00000001,
unsigned int flags=0x00000000)  Line 1286 + 0x20	C
 	js3250.dll!js_Interpret(JSContext * cx=0x018df598, long * result=0x0012f758) 
Line 3619 + 0xf	C
 	js3250.dll!js_Invoke(JSContext * cx=0x018df598, unsigned int argc=0x00000003,
unsigned int flags=0x00000002)  Line 1306 + 0xd	C
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *
wrapper=0x04fea028, unsigned short methodIndex=0x0003, const nsXPTMethodInfo *
info=0x0114c988, nsXPTCMiniVariant * nativeParams=0x0012fa54)  Line 1413 + 0x14	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=0x0003,
const nsXPTMethodInfo * info=0x0114c988, nsXPTCMiniVariant * params=0x0012fa54)
 Line 450	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x04fea028, unsigned
int methodIndex=0x00000003, unsigned int * args=0x0012fb18, unsigned int *
stackBytesToPop=0x0012fb08)  Line 117 + 0x1a	C++
 	xpcom_core.dll!SharedStub()  Line 147	C++
 	embedcomponents.dll!nsCommandManager::CommandStatusChanged(const char *
aCommandName=0x02d77a6c)  Line 115 + 0x39	C++
 	composer.dll!nsComposerCommandsUpdater::UpdateCommandGroup(const nsAString &
aCommandGroup={...})  Line 323	C++
 	composer.dll!nsComposerCommandsUpdater::TimerCallback()  Line 286 + 0x18	C++
 	composer.dll!nsComposerCommandsUpdater::Notify(nsITimer * timer=0x0534dd58)
 Line 400	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 387	C++
 	xpcom_core.dll!nsTimerManager::FireNextIdleTimer()  Line 617	C++
 	gkwidget.dll!nsAppShell::Run()  Line 142	C++
 	appcomps.dll!nsAppStartup::Run()  Line 216	C++
 	mozilla.exe!main1(int argc=0x00000001, char * * argv=0x00347b88, nsISupports *
nativeApp=0x01106140)  Line 1321 + 0x20	C++
 	mozilla.exe!main(int argc=0x00000001, char * * argv=0x00347b88)  Line 1813 +
0x25	C++
 	mozilla.exe!mainCRTStartup()  Line 400 + 0x11	C
 	kernel32.dll!TermsrvAppInstallMode()  + 0x269	

+	blockParent	{mRawPtr=0x00000000 }	nsCOMPtr<nsIDOMNode>
	res	0x00000000	unsigned int
+	this	0x0518bdb0 {mIgnoreSpuriousDragEvent=0x00000000 mContentFilters={...}
mTypeInState=0x051ae188 {mRefCnt={mValue=0x00000002 }
_mOwningThread={mThread=0x00345280 } mSetArray={mImpl=0x00000000 {mBits=???
mCount=??? mArray=0x00000008 } } ...} ...}	nsHTMLEditor * const
+	tmp	{mRawPtr=0x00000000 }	nsCOMPtr<nsIDOMNode>
	isBlock	0x00000000	int

      res = tmp->GetParentNode(getter_AddRefs(blockParent));

steps:
edit the page
select view all tags mode
click a frameset marker
right click in the tag hierarchy in the status area and select remove this tag.
repeat until it asserts.

reproducable: unsure.

Comment 1

[timeless]

the code uses a bunch of different pointers
the code gets a new |blockParent| each time through the loop
and it sets htmlElement to the old blockParent each time through the loop
and it null checks the old blockParent each time through the loop
but it uses the new blockParent each time through the loop
so, it can crash, one time through the loop, right? :)

<smontagu> it null checks after using it?
<Neil> glazou will be pleased :-)

yes
one loop after using it
at least, that's how i read the code

Comment 2

[Oleg Romashin (:romaxa)]

Probably it related to this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=167543

Comment 3

[Oleg Romashin (:romaxa)]

Also probably this fix will help... 
https://bugzilla.mozilla.org/attachment.cgi?id=170483

from bug https://bugzilla.mozilla.org/show_bug.cgi?id=277306

But I need to check.

Comment 4

[Oleg Romashin (:romaxa)]

Not reproducible:
seamonkey/nightly/2006-11-03-01-trunk


Reproducible:
seamonkey/nightly/2006-11-04-01-trunk

Comment 5

[Oleg Romashin (:romaxa)]

Some debug log from current build... there are no crash anymore, because page load failed at all.

++DOMWINDOW == 8
--DOMWINDOW == 7
--DOMWINDOW == 6
###!!! ASSERTION: wasDirty lied: 'mDirtyRoots.IndexOf(f) == -1', file /mnt/other_opts/do_it_here/romaxa/officials_trunk/mozilla/layout/base/nsPresShell.cpp, line 3500
++WEBSHELL 0xb21ee600 == 4
++DOMWINDOW == 7
++DOMWINDOW == 8
++DOMWINDOW == 9
CSS Error (http://terra.tuparada.com/tarjetas.css :2.9): Error in parsing value for property 'border'.  Declaration dropped.
CSS Error (http://terra.tuparada.com/tarjetas.css :35.26): Expected color but found '1px'.  Expected end of value for property but found '1px'.  Error in parsing value for property 'border-color'.  Declaration dropped.
--WEBSHELL 0xb21ee600 == 3
--DOMWINDOW == 8

Comment 6

[(no longer active)]

The only possibility that I see here for a crash is when the text node here does not have a parent: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#2301>, and we try to dereference the null parent here: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#2324>.  The rest of nsCOMPtr dereferences seem safe here.

Comment 7

[(no longer active)]

Attachment: Patch (v1)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How can a text node in the selection not have a parent? Surely the selection should not include nodes that are not in the document?

Can you actually reproduce this bug? Are we sure it still crashes on trunk?

Comment 9

[(no longer active)]

(In reply to comment #8)
> How can a text node in the selection not have a parent? Surely the selection
> should not include nodes that are not in the document?

Normally it shouldn't.  However, this is the only assumption in this function which is not actually tested, as far as I can see.

> Can you actually reproduce this bug? Are we sure it still crashes on trunk?

No, I can't reproduce the problem, especially that detailed STRs are not available.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Then we may just be patching around some deeper bug. I recommend not fixing this until we have STR (if ever).

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.