241058 - DEBUG-only crash when visiting hixie's blog (http://ln.hixie.ch/) [@ nsINodeInfo::Equals]

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[(not reading, please use [e-mail] instead)]

crash when visiting hixie's blog (http://ln.hixie.ch/)

this is with the trunk, I have a stack.

look like we might be de-references a null nsCOMPtr.

Comment 1

[(not reading, please use [e-mail] instead)]

first I assert:

NTDLL! 77fa144b()
nsDebugImpl::Assertion(nsDebugImpl * const 0x00267df0, const char * 0x01a8b7cc
`string', const char * 0x01a8b810 `string', const char * 0x01aba828 `string',
int 711) line 287
nsDebug::Assertion(const char * 0x01a8b7cc `string', const char * 0x01a8b810
`string', const char * 0x01aba828 `string', int 711) line 109
nsCOMPtr<nsINodeInfo>::operator->() line 711 + 34 bytes
nsHTMLSharedListElement::QueryInterface(nsHTMLSharedListElement * const
0x049929c0, const nsID & {...}, void * * 0x0012e94c) line 133 + 18 bytes
nsQueryInterface::operator()(const nsID & {...}, void * * 0x0012e94c) line 47 +
23 bytes
nsCOMPtr<nsIDOMHTMLOListElement>::assign_from_qi(nsQueryInterface {...}, const
nsID & {...}) line 1030 + 17 bytes
nsCOMPtr<nsIDOMHTMLOListElement>::nsCOMPtr<nsIDOMHTMLOListElement>(nsQueryInterface
{...}) line 572
nsCOMPtr<nsIDOMHTMLOListElement>::Assert_NoQueryNeeded() line 520
nsCOMPtr<nsIDOMHTMLOListElement>::nsCOMPtr<nsIDOMHTMLOListElement>(nsIDOMHTMLOListElement
* 0x049929e0) line 556
nsHTMLSharedListElement::CloneNode(nsHTMLSharedListElement * const 0x049453d0,
int 1, nsIDOMNode * * 0x0012ea1c) line 156
nsGenericHTMLElement::CopyInnerTo(nsGenericElement * 0x04992840, int 1) line 337
+ 69 bytes
nsHTMLDivElement::CloneNode(nsHTMLDivElement * const 0x049442a8, int 1,
nsIDOMNode * * 0x0012eb5c) line 151
nsGenericElement::CopyInnerTo(nsGenericElement * 0x048c4d48, int 1) line 3193 +
67 bytes
nsXMLElement::CloneNode(nsXMLElement * const 0x04941bd8, int 1, nsIDOMNode * *
0x0012ee78) line 480
nsXBLBinding::GenerateAnonymousContent(nsXBLBinding * const 0x04907300) line 577
+ 67 bytes
nsXBLService::LoadBindings(nsXBLService * const 0x02b97df8, nsIContent *
0x04933360, nsIURI * 0x048ba690, int 0, nsIXBLBinding * * 0x0012f248, int *
0x0012f234) line 629
nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell * 0x048a48a0,
nsIPresContext * 0x0466b158, nsFrameConstructorState & {...}, nsIContent *
0x04933360, nsIFrame * 0x0492d594, nsIAtom * 0x00ea4718, int 3, nsStyleContext *
0x045d24e4, nsFrameItems & {...}, int 0) line 7069 + 55 bytes
nsCSSFrameConstructor::ConstructFrame(nsIPresShell * 0x048a48a0, nsIPresContext
* 0x0466b158, nsFrameConstructorState & {...}, nsIContent * 0x04933360, nsIFrame
* 0x0492d594, nsFrameItems & {...}) line 7026 + 51 bytes
nsCSSFrameConstructor::ContentInserted(nsIPresContext * 0x0466b158, nsIContent *
0x049284e0, nsIFrame * 0x00000000, nsIContent * 0x04933360, int 1,
nsILayoutHistoryState * 0x00000000, int 0) line 8921
PresShell::ContentInserted(nsIDocument * 0x04899028, nsIContent * 0x049284e0,
nsIContent * 0x04933360, int 1) line 5252
nsXBLResourceLoader::NotifyBoundElements() line 284
nsXBLResourceLoader::StyleSheetLoaded(nsXBLResourceLoader * const 0x0489ddc8,
nsICSSStyleSheet * 0x0489e150, int 1) line 205
CSSLoaderImpl::SheetComplete(SheetLoadData * 0x0467b448, int 1) line 1541
CSSLoaderImpl::ParseSheet(nsIUnicharInputStream * 0x0495aca8, SheetLoadData *
0x0467b448, int & 1) line 1483
SheetLoadData::OnStreamComplete(SheetLoadData * const 0x0467b448,
nsIUnicharStreamLoader * 0x046b2768, nsISupports * 0x00000000, unsigned int 0,
nsIUnicharInputStream * 0x0495aca8) line 821 + 23 bytes
nsUnicharStreamLoader::OnStopRequest(nsUnicharStreamLoader * const 0x046b276c,
nsIRequest * 0x0467b4e0, nsISupports * 0x00000000, unsigned int 0) line 196
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x0495a8d0,
nsIRequest * 0x0467b4e0, nsISupports * 0x00000000, unsigned int 0) line 66
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x0467b4e8, nsIRequest *
0x0497fb50, nsISupports * 0x00000000, unsigned int 0) line 3623
nsInputStreamPump::OnStateStop() line 506
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x0497fb54,
nsIAsyncInputStream * 0x046b2fdc) line 340 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x048dfb04) line 119
PL_HandleEvent(PLEvent * 0x048dfb04) line 692 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ed7cf0) line 627 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0028072c, unsigned int 49553, unsigned int 0,
long 15564016) line 1433 + 9 bytes
USER32! 77e3a2d0()
USER32! 77e145e5()
USER32! 77e1a816()
nsAppShellService::Run(nsAppShellService * const 0x00f74d60) line 524
main1(int 1, char * * 0x00264f70, nsISupports * 0x00eb32b0) line 1302 + 32 bytes
main(int 1, char * * 0x00264f70) line 1779 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c5987e7()

Comment 2

[(not reading, please use [e-mail] instead)]

and then I crash very soon after (the top of this stack looks bogus?)

nsINodeInfo::Equals(nsIAtom * 0x00ea4930) line 301 + 3 bytes
nsHTMLSharedListElement::QueryInterface(nsHTMLSharedListElement * const
0x049929c0, const nsID & {...}, void * * 0x0012e94c) line 133 + 25 bytes
nsQueryInterface::operator()(const nsID & {...}, void * * 0x0012e94c) line 47 +
23 bytes
nsCOMPtr<nsIDOMHTMLOListElement>::assign_from_qi(nsQueryInterface {...}, const
nsID & {...}) line 1030 + 17 bytes
nsCOMPtr<nsIDOMHTMLOListElement>::nsCOMPtr<nsIDOMHTMLOListElement>(nsQueryInterface
{...}) line 572
nsCOMPtr<nsIDOMHTMLOListElement>::Assert_NoQueryNeeded() line 520
nsCOMPtr<nsIDOMHTMLOListElement>::nsCOMPtr<nsIDOMHTMLOListElement>(nsIDOMHTMLOListElement
* 0x049929e0) line 556
nsHTMLSharedListElement::CloneNode(nsHTMLSharedListElement * const 0x049453d0,
int 1, nsIDOMNode * * 0x0012ea1c) line 156
nsGenericHTMLElement::CopyInnerTo(nsGenericElement * 0x04992840, int 1) line 337
+ 69 bytes
nsHTMLDivElement::CloneNode(nsHTMLDivElement * const 0x049442a8, int 1,
nsIDOMNode * * 0x0012eb5c) line 151
nsGenericElement::CopyInnerTo(nsGenericElement * 0x048c4d48, int 1) line 3193 +
67 bytes
nsXMLElement::CloneNode(nsXMLElement * const 0x04941bd8, int 1, nsIDOMNode * *
0x0012ee78) line 480
nsXBLBinding::GenerateAnonymousContent(nsXBLBinding * const 0x04907300) line 577
+ 67 bytes
nsXBLService::LoadBindings(nsXBLService * const 0x02b97df8, nsIContent *
0x04933360, nsIURI * 0x048ba690, int 0, nsIXBLBinding * * 0x0012f248, int *
0x0012f234) line 629
nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell * 0x048a48a0,
nsIPresContext * 0x0466b158, nsFrameConstructorState & {...}, nsIContent *
0x04933360, nsIFrame * 0x0492d594, nsIAtom * 0x00ea4718, int 3, nsStyleContext *
0x045d24e4, nsFrameItems & {...}, int 0) line 7069 + 55 bytes
nsCSSFrameConstructor::ConstructFrame(nsIPresShell * 0x048a48a0, nsIPresContext
* 0x0466b158, nsFrameConstructorState & {...}, nsIContent * 0x04933360, nsIFrame
* 0x0492d594, nsFrameItems & {...}) line 7026 + 51 bytes
nsCSSFrameConstructor::ContentInserted(nsIPresContext * 0x0466b158, nsIContent *
0x049284e0, nsIFrame * 0x00000000, nsIContent * 0x04933360, int 1,
nsILayoutHistoryState * 0x00000000, int 0) line 8921
PresShell::ContentInserted(nsIDocument * 0x04899028, nsIContent * 0x049284e0,
nsIContent * 0x04933360, int 1) line 5252
nsXBLResourceLoader::NotifyBoundElements() line 284
nsXBLResourceLoader::StyleSheetLoaded(nsXBLResourceLoader * const 0x0489ddc8,
nsICSSStyleSheet * 0x0489e150, int 1) line 205
CSSLoaderImpl::SheetComplete(SheetLoadData * 0x0467b448, int 1) line 1541
CSSLoaderImpl::ParseSheet(nsIUnicharInputStream * 0x0495aca8, SheetLoadData *
0x0467b448, int & 1) line 1483
SheetLoadData::OnStreamComplete(SheetLoadData * const 0x0467b448,
nsIUnicharStreamLoader * 0x046b2768, nsISupports * 0x00000000, unsigned int 0,
nsIUnicharInputStream * 0x0495aca8) line 821 + 23 bytes
nsUnicharStreamLoader::OnStopRequest(nsUnicharStreamLoader * const 0x046b276c,
nsIRequest * 0x0467b4e0, nsISupports * 0x00000000, unsigned int 0) line 196
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x0495a8d0,
nsIRequest * 0x0467b4e0, nsISupports * 0x00000000, unsigned int 0) line 66
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x0467b4e8, nsIRequest *
0x0497fb50, nsISupports * 0x00000000, unsigned int 0) line 3623
nsInputStreamPump::OnStateStop() line 506
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x0497fb54,
nsIAsyncInputStream * 0x046b2fdc) line 340 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x048dfb04) line 119
PL_HandleEvent(PLEvent * 0x048dfb04) line 692 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ed7cf0) line 627 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0028072c, unsigned int 49553, unsigned int 0,
long 15564016) line 1433 + 9 bytes
USER32! 77e3a2d0()
USER32! 77e145e5()
USER32! 77e1a816()
nsAppShellService::Run(nsAppShellService * const 0x00f74d60) line 524
main1(int 1, char * * 0x00264f70, nsISupports * 0x00eb32b0) line 1302 + 32 bytes
main(int 1, char * * 0x00264f70) line 1779 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c5987e7()

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, this is interesting... First of all, this crash is debug-only.

The reason it's happening is that when you assign into an nsCOMPtr in a debug
build it QIs to the type it expects and asserts pointer equality.  So in this
case, the line:

  nsCOMPtr<nsIDOMHTMLOListElement> kungFuDeathGrip(it);

in nsHTMLSharedListElement::CloneNode will trigger a QI of "it" to
nsIDOMHTMLOListElement.

But "it" is a nsHTMLSharedListElement that has just been created and hence does
not have a nodeinfo yet.  And the QI impl of this class looks at the mNodeInfo
(the tagname, to be exact) to decide whether it QIs to a particular interface. 
But the mNodeInfo nsCOMPtr is still null when QI is called here (it's only set
in the next line of CloneNode(), so we crash.  The stack in comment 2 is not
bogus at all -- it's right on the money.

We really shouldn't be using that nsCOMPtr here; the best we can do with it is
make it only assert, not crash (when what's being cloned is a <ul>, say).  How
about we just use nsRefPtr<nsHTMLSharedListElement> or use a manual addref?

Comment 4

[Stefan Borggraefe]

Also crashes a 10 hour old Linux Gtk2 debug build.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm... The ideal solution would probably be to pass the nodeinfo to the ctor,
though that is a bit unCOM-ish. Using nsRefPtr sounds like the best idea to me.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would anyone have the cycles to come up with a patch for this one? Even though
it's debug only, it's a pretty serious crasher and could cause problems for
developers. I don't have a development environment currently (and won't until
after 1.7 is out the door) but the patch should be simple enough for me to review.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Use nsRefPtr<>.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148221 [details] [diff] [review]
Use nsRefPtr<>.

r+sr=bzbarsky

Comment 9

[Johnny Stenback (:jst)]

Attachment: The "real" fix, no more nsGenericElement::Init().

This is a boring change, it's the same thing in all the nsHTML*Element classes
(with some silly differences in a few). This saves us some code, and should
make us a tad faster too (due to fewer calls on elements during construction,
and less refcounting when cloning). Not for the 1.7 branch, but good for the
trunk, IMO.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 148265 [details] [diff] [review]
The "real" fix, no more nsGenericElement::Init().

bz, wanna stamp this one too? :-)

Comment 11

[Johnny Stenback (:jst)]

The first patch is now checked in, btw.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 148221 [details] [diff] [review]
Use nsRefPtr<>.

Requesting approval. No real code change, fixes a *debug only* crash.

Comment 13

[Boris Zbarsky [:bzbarsky]]

er.. It'll take me a few days to get to this one....  It's long... ;)

Comment 14

[chris hofmann]

Comment on attachment 148221 [details] [diff] [review]
Use nsRefPtr<>.

a=chofmann for 1.7

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 148265 [details] [diff] [review]
The "real" fix, no more nsGenericElement::Init().

Please fix SVG too

Comment 16

[Johnny Stenback (:jst)]

Attachment: Fix SVG as well.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148360 [details] [diff] [review]
Fix SVG as well.

>Index: content/base/src/nsContentUtils.cpp
>@@ -156,19 +160,19 @@ nsContentUtils::Init()
>-  
>+

Pointless whitespace change.

>Index: content/html/content/src/nsHTMLFormElement.cpp

> nsFormControlList::Item(PRUint32 aIndex, nsIDOMNode** aReturn)

>-  *aReturn = nsnull;
>+
> 
>   return NS_OK;

er... that could leave *aReturn pointing to garbage with a success return, no? 
Please undo that change....

> nsFormControlList::NamedItem(const nsAString& aName,
>                              nsIDOMNode** aReturn)
> {
>-  NS_ENSURE_ARG_POINTER(aReturn);
>-  *aReturn = nsnull;

Same.

Up through beginning of nsHTMLHeadElement...

It occurs to me that this would be a lot more pleasant to review if we just had
a macro (or two) that took the element name and implemented clodeNode and
NS_NewWhatever.  That way I'd only have to review the code once.... Most
elements could just use that; the few that do something else would not use the
macro.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ooh, that sounds very neat indeed. We could possibly even allow for a macro 
that took a functionname that got called and passed the old and the new 
element (as appropriate datatype). Speed won't be an issue, an extra 
functioncall in these cases won't matter at all. 

Comment 19

[Johnny Stenback (:jst)]

Attachment: Add macros for NS_NewHTML* and NS_NewSVG*, and also for CloneNode

Yeah, that's a good idea. Here's a start. It's more than just one macro, but it
should help with reviewing. If we want to combine these, we can do that, or if
we want to generalize them more (to be able to use the same ones for HTML and
SVG element classes) we can do that too, though I'd rather do that as a
separate patch.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148716 [details] [diff] [review]
Add macros for NS_NewHTML* and NS_NewSVG*, and also for CloneNode

>Index: content/base/src/nsContentUtils.cpp
>@@ -156,19 +160,19 @@ nsContentUtils::Init()
>-  
>+

That's still here... kill it, please?

>Index: content/html/content/src/nsGenericHTMLElement.h

>+ * A macro to implement the nsHTMLXXXElement::CloneNode().

s/the //, please.

>Index: content/html/content/src/nsHTMLSpanElement.cpp

> NS_NewHTMLUnknownElement(nsIHTMLContent** aInstancePtrResult,
>                          nsINodeInfo *aNodeInfo)

> nsHTMLUnknownElement::CloneNode(PRBool aDeep, nsIDOMNode** aReturn)

was there a reason not to use the macros for these?  If so, I don't see what it
was...

>Index: content/svg/content/src/nsSVGElement.h

>+ * A macro to implement the nsSVGXXXElement::CloneNode().

I don't really see why SVG is using this CopyNode (which looks identical to
nsGenericElement::CopyInnerTo.	File a followup bug to eliminate that and have
a single clonenode macro that works for svg and HTML both?

r+sr=bzbarsky.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er, we may not be able to reuse the macro because of the Init() thing, but
CopyNode should still go, methinks.

Comment 22

[Peter Van der Beken [:peterv]]

Could we prefix the macros with NS_... so they resemble the naming of our other
macros?

Comment 23

[Johnny Stenback (:jst)]

(In reply to comment #21)
> Er, we may not be able to reuse the macro because of the Init() thing, but
> CopyNode should still go, methinks.

Well, the inconsistency is lame, but IMO CopyNode() should stay, and
CopyInnerTo() should go since that name is from the good ol' days of
nsHTMLXXXElement::mInner (long time ago) where the HTML element classes didn't
inherit nsGeneric*Element, but had a pointer to a separately malloc'ed one. So
now that there's no "inner" to copy, CopyInnerTo() isn't all that good of a name :-)

Filed bug 243967 on renaming CopyInnerTo().

Oh, and I prefixed the macros with "NS_", and added "HTML" to the name of the
HTML CloneNode macro for consistency's sake.

Comment 24

[Johnny Stenback (:jst)]

Fix checked in.

Comment 25

[R.K.Aa.]

i wonder if this caused bug 244050 and bug 244051

Comment 26

[Johnny Stenback (:jst)]

Yes it did :-(

Comment 27

[[e-mail]]

Comment on attachment 148265 [details] [diff] [review]
The "real" fix, no more nsGenericElement::Init().

>+  nsCOMPtr<nsIDOMNode> kungFuDeathGrip =
>+    NS_STATIC_CAST(nsIDOMHTMLParamElement *, this);
When trying to split a mail cite I get four assertions, of which the first is
here in nsHTMLSharedElement::CloneNode, then I get two notreached errors in
nsRange::IsIncreasing, then editor just gives up :-(

Comment 28

[Scott MacGregor]

I'm seeing some pretty nasty regressions in mail compose that I think may have
been caused by these changes but I'm not sure.  I just filed:

Bug #244576

(I see Neil must be seeing the same thing)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Whoa.  The code cited in comment 27 should be taking a ref to "it", not
"this".... Does fixing that help?

Comment 30

[Johnny Stenback (:jst)]

Yup, that was it :-( Thanks for catching that! I just landed the fix.