Closed
Bug 223470
Opened 21 years ago
Closed 21 years ago
[FIXr]Crash with <ATTLIST foo size CDATA #FIXED "3">...<foo/>
Categories
(Core :: XUL, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.6beta
People
(Reporter: WeirdAl, Assigned: bzbarsky)
Details
(Keywords: crash)
Attachments
(3 files, 1 obsolete file)
323 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
3.80 KB,
text/plain
|
Details | |
17.80 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•21 years ago
|
||
Comment 2•21 years ago
|
||
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.
Reporter | ||
Comment 3•21 years ago
|
||
per jshin's comments, OS -> All.
FYI, a generic XML-based testcase does not crash, it loads cleanly.
OS: Linux → All
Assignee | ||
Comment 4•21 years ago
|
||
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?
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.
Assignee | ||
Comment 6•21 years ago
|
||
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...
Assignee: hyatt → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.6beta
Assignee | ||
Comment 7•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #134067 -
Flags: superreview?(peterv)
Attachment #134067 -
Flags: review?(bugmail)
Assignee | ||
Updated•21 years ago
|
Summary: Crash with <ATTLIST foo size CDATA #FIXED "3">...<foo/> → [FIX]Crash with <ATTLIST foo size CDATA #FIXED "3">...<foo/>
Assignee | ||
Comment 8•21 years ago
|
||
> 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.
Attachment #134067 -
Flags: review?(bugmail) → review+
Comment 9•21 years ago
|
||
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 /
Attachment #134067 -
Flags: superreview?(peterv) → superreview+
Assignee | ||
Comment 10•21 years ago
|
||
I don't get a warning on that for loop, no.
Attachment #134067 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Summary: [FIX]Crash with <ATTLIST foo size CDATA #FIXED "3">...<foo/> → [FIXr]Crash with <ATTLIST foo size CDATA #FIXED "3">...<foo/>
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.
Assignee | ||
Comment 12•21 years ago
|
||
I'll just use {} instead of ;
Assignee | ||
Comment 13•21 years ago
|
||
Patch checked in for 1.6b (with {} and a comment instead of ';').
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 14•21 years ago
|
||
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.
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•