223470 - [FIXr]Crash with <ATTLIST foo size CDATA #FIXED "3">...<foo/>

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Alex Vincent [:WeirdAl]]

Program mozilla-debug/dist/bin/mozilla-bin (pid = 5507) received signal 11.
Stack:
_ZN13nsProfileLock18FatalSignalHandlerEi+0x000000D0
[/home/ajvincent/mozilla-debug/dist/bin/components/libprofile.so +0x00032F3A]
UNKNOWN [/lib/libpthread.so.0 +0x0000CC2D]
UNKNOWN [/lib/libc.so.6 +0x00027D58]
_ZN8nsCOMPtrI11nsINodeInfoE16begin_assignmentEv+0x00000020
[/home/ajvincent/mozilla-debug/dist/bin/components/libgklayout.so +0x00377C26]
_ZN8nsCOMPtrI11nsINodeInfoE15StartAssignmentEv+0x0000001E
[/home/ajvincent/mozilla-debug/dist/bin/components/libgklayout.so +0x003775EC]
_ZN15nsGetterAddRefsI11nsINodeInfoEcvPPS0_Ev+0x00000020
[/home/ajvincent/mozilla-debug/dist/bin/components/libgklayout.so +0x00376AA6]
_ZN18XULContentSinkImpl13AddAttributesEPPKtjP21nsXULPrototypeElement+0x000000F8
[/home/ajvincent/mozilla-debug/dist/bin/components/libgklayout.so +0x0082F6A2]
_ZN18XULContentSinkImpl7OpenTagEPPKtjjP11nsINodeInfo+0x00000147
[/home/ajvincent/mozilla-debug/dist/bin/components/libgklayout.so +0x0082EBCF]
_ZN18XULContentSinkImpl18HandleStartElementEPKtPS1_jjj+0x0000019E
[/home/ajvincent/mozilla-debug/dist/bin/components/libgklayout.so +0x0082CC08]
_ZN13nsExpatDriver18HandleStartElementEPKtPS1_+0x000000A5
[/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so +0x0003ABF3]
UNKNOWN [/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so
+0x000399D3]
UNKNOWN [/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so
+0x0007179B]
UNKNOWN [/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so
+0x00070BAF]
UNKNOWN [/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so
+0x000739FD]
UNKNOWN [/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so
+0x00073551]
UNKNOWN [/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so
+0x000734FD]
XML_Parse+0x000001FF
[/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so +0x00070472]
_ZN13nsExpatDriver11ParseBufferEPKcji+0x00000085
[/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so +0x0003C2DB]
_ZN13nsExpatDriver12ConsumeTokenER9nsScannerRi+0x000000C5
[/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so +0x0003C6BB]
_ZN8nsParser8TokenizeEi+0x00000146
[/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so +0x0005DB6C]
_ZN8nsParser11ResumeParseEiii+0x000001B3
[/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so +0x0005BA0D]
_ZN8nsParser15OnDataAvailableEP10nsIRequestP11nsISupportsP14nsIInputStreamjj+0x000001AA
[/home/ajvincent/mozilla-debug/dist/bin/components/libhtmlpars.so +0x0005D71A]
_ZN18nsDocumentOpenInfo15OnDataAvailableEP10nsIRequestP11nsISupportsP14nsIInputStreamjj+0x0000005E
[/home/ajvincent/mozilla-debug/dist/bin/components/libdocshell.so +0x0009B826]
_ZN13nsFileChannel15OnDataAvailableEP10nsIRequestP11nsISupportsP14nsIInputStreamjj+0x00000055
[/home/ajvincent/mozilla-debug/dist/bin/components/libnecko.so +0x00137BB9]
_ZN17nsInputStreamPump15OnStateTransferEv+0x000001FD
[/home/ajvincent/mozilla-debug/dist/bin/components/libnecko.so +0x000A29A1]
_ZN17nsInputStreamPump18OnInputStreamReadyEP19nsIAsyncInputStream+0x00000096
[/home/ajvincent/mozilla-debug/dist/bin/components/libnecko.so +0x000A25AC]
_ZN23nsInputStreamReadyEvent12EventHandlerEP7PLEvent+0x0000007D
[/home/ajvincent/mozilla-debug/dist/bin/libxpcom.so +0x000E04B5]
PL_HandleEvent+0x00000054 [/home/ajvincent/mozilla-debug/dist/bin/libxpcom.so
+0x00105D2C]
PL_ProcessEventsBeforeID+0x0000016A
[/home/ajvincent/mozilla-debug/dist/bin/libxpcom.so +0x0010647A]
_Z12processQueuePvS_+0x0000002D
[/home/ajvincent/mozilla-debug/dist/bin/components/libwidget_gtk.so +0x00032F99]
_ZN11nsVoidArray17EnumerateForwardsEPFiPvS0_ES0_+0x00000050
[/home/ajvincent/mozilla-debug/dist/bin/libxpcom.so +0x000B8B90]
_ZN10nsAppShell15ProcessBeforeIDEm+0x00000034
[/home/ajvincent/mozilla-debug/dist/bin/components/libwidget_gtk.so +0x00032FDA]
_Z16handle_gdk_eventP9_GdkEventPv+0x00000626
[/home/ajvincent/mozilla-debug/dist/bin/components/libwidget_gtk.so +0x000426A8]
UNKNOWN [/usr/lib/libgdk-1.2.so.0 +0x0001917F]
UNKNOWN [/usr/lib/libglib-1.2.so.0 +0x00014199]
g_main_run+0x00000044 [/usr/lib/libglib-1.2.so.0 +0x00013174]
Sleeping for 5 minutes.
Type 'gdb mozilla-debug/dist/bin/mozilla-bin 5507' to attach your debugger to
this thread.

Steps to reproduce:
(1) Open the attached testcase.

Expected Result: Empty XUL page.
Actual Result: Crash.

Testcase coming up.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: crash testcase

Comment 2

[Jungshik Shin]

Attachment: stack trace (Win32)

I've just sent a talkback (I forgot how to get the talkback id #. let me know
how and I'll get back with the talkback id) from Win32 1.5 on Win2k. Attached
is the stack trace.

Comment 3

[Alex Vincent [:WeirdAl]]

per jshin's comments, OS -> All.

FYI, a generic XML-based testcase does not crash, it loads cleanly.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the problem here is that nsExpatDriver is simply buggy (or nsIExpatSink too
demanding, as you will).  Here is a summary of the problem:

1)  nsIExpatSink defines HandleStartElement to take a 
    "[array, size_is (aAttsCount)] in wstring aAtts" arg as well as a 
    "in unsigned long aAttsCount" arg.
2)  The XUL content sink assumes that this actually means something and uses
    aAttsCount to allocate the array of nsXULPrototypeAttribute objects that it
    writes the attr data into.
3)  nsExpatDriver::HandleStartElement, which calls HandleStartElement on the
    sink, passes "XML_GetSpecifiedAttributeCount(mExpatParser) / 2" as
    aAttsCount.
4)  This number does not include attributes that are defaulted from a DTD.  See
    http://lxr.mozilla.org/seamonkey/source/expat/xmlparse/xmlparse.h#390
5)  There is no api in xmlparse.h to get the number of default attributes for
    the current element.  In fact, this information is only gettable via some
    internal apis in xmlparse.c (looking up the tag name in a table, getting a
    type descriptor from there, etc).

We have a couple of options:

A)  Change nsIExpatSink to reflect the current reality and document this
    interface (it needs it sorely).  Change XUL to count the default attrs by
    hand if |aAttributes[aAttrLen] != 0| (rare case, no real speed hit).
B)  Change nsExpatDriver to follow the nsIExpatSink interface.  No changes
    needed to XUL; this can also use the |aAtts[attrlen] != 0| optimization,
    thus no real speed hit here either.  Document nsIExpatSink anyway.

heikki? sicking?  Thoughts?  Will there come a time when we do care about
specified vs default attrs in nsIExpatSink implementations?

Comment 5

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

My guess is yes, there will come a time when we care, but IMHO we should deal
with that then and not overengineer now.

Making the change in the nsExpatDriver seems like the best solution since that
will fix all sinks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and in any case the interface needs changing, since the count is the count
of attributes and the array is twice that big.

In any case, I'll take this...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

>                     Otherwise, aIndex
>+   *        is |unsigned long(-1)|

I've changed that to just "-1" instead of "|unsigned long(-1)|" in my build to
match the idl change.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 134067 [details] [diff] [review]
Proposed patch

>Index: htmlparser/public/nsIExpatSink.idl
>===================================================================

> interface nsIExpatSink : nsISupports 
> {
>+  /**
>+   * Called to handle the opening tag of an element.
>+   * @param aName the fully qualified tagname if the element

s/if/of/

>+  /**
>+   * Called to handle the closing tag of an element.
>+   * @param aName the fully qualified tagname if the element

s/if/of/

>Index: htmlparser/src/nsExpatDriver.cpp
>===================================================================

>+  PRUint32 attrArrayLength;
>+  for (attrArrayLength = XML_GetSpecifiedAttributeCount(mExpatParser);
>+       aAtts[attrArrayLength];
>+       attrArrayLength += 2);

Will this give a warning?

>Index: extensions/transformiix/source/xslt/txMozillaStylesheetCompiler.cpp
>===================================================================

>     nsresult rv =
>-        mCompiler->startElement(aName, aAtts, aAttsCount, (PRInt32)aIndex);
>+        mCompiler->startElement(aName, aAtts, aAttsCount/2, aIndex);

Add spaces around /

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Address the comments

I don't get a warning on that for loop, no.

Comment 11

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

i noticed that warning-thing too but forgot to comment on it. IIRC some
platforms warn for that which can be fixed by inserting a comment between the
|for| and the |;|, or by adding an empty |{}| rather then the |;|.

I could be wrong though and only |while| loops warn. But if you don't do this
please keep an eye on warnings after you check in.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'll just use {} instead of ;

Comment 13

[Boris Zbarsky [:bzbarsky]]

Patch checked in for 1.6b (with {} and a comment instead of ';').

Comment 14

[Scott MacGregor]

This patch caused a crash in mailnews now when clicking on news accounts. 

See BUg #224313:

crash occurs at:
XULContentSinkImpl::AddAttributes

backing out this patch apparently fixes the crash.