Closed Bug 274777 Opened 15 years ago Closed 14 years ago

Update to Expat 1.95.8

Categories

(Core :: XML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: peterv, Assigned: peterv)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

Need to switch to the new blocking API that Expat provides instead of using our own.
Beautiful! I don't know much of the code, but I'm willing to help. This is a
1.9-ish thing, right?
Attached patch v1 (diff against stock Expat) (obsolete) — Splinter Review
Attached patch v1.1 (obsolete) — Splinter Review
The only real changes are the integration of a patch that just went into the
Expat trunk
(http://cvs.sourceforge.net/viewcvs.py/expat/expat/lib/xmlparse.c?r1=1.140&r2=1.141)
and the changes in nsExpatDriver.cpp.
The new blocking API has slightly different semantics than our API.
XML_ResumeParser used to just flip the blocked flag and then we called
XML_Parse again to parse the remainder of the buffer. In the new API,
XML_ResumeParser flips the blocked flag and parses the remainder of the buffer.

All the other changes are from the update of Expat from 1.95.7 to 1.95.8.
Attachment #169512 - Attachment is obsolete: true
Attachment #169514 - Attachment is obsolete: true
Attachment #169716 - Flags: review?(bsmedberg)
Comment on attachment 169716 [details] [diff] [review]
v1.1

I haven't spent years in this code, but from inspection this looks correct.
Attachment #169716 - Flags: review?(bsmedberg) → review+
Attachment #169716 - Flags: superreview?(bzbarsky)
Peter, would you mind posting a patch updated to tip?  ParseBuffer has changed a
good bit and I want to try to sort out how the new logic works with it....
Attached patch v1.1Splinter Review
Updated to trunk, carrying forward r=bsmedberg. I'll also attach a diff -w of
just the changes to nsExpatDriver.cpp.
Attachment #169716 - Attachment is obsolete: true
Attachment #195539 - Flags: superreview?(bzbarsky)
Attachment #195539 - Flags: review+
Blocks: 158400
OK, there are two things that concern me about the nsExpatDriver changes:

1)  Consider a call to nsExpatDriver::ParseBuffer that passes the following
    buffer (this is the first call to the method):

    "<root"

    (note the missing '>').  I assume expat will return XML_STATUS_OK, right?
    If not, what does it return?  If it does, then in this case, don't we still
    need to update mBytePosition and mBytesParsed?  And most importantly update
    mLastLine?  I don't see where we do that...

2)  Consider a call to nsExpatDriver::ParseBuffer that happens while expat is
    suspended (so we want to be unsuspending).  We call XML_ResumeParser and
    then if, say, XML_STATUS_SUSPENDED is returned start mucking with aBuffer,
    which we never used.  Worse yet, if XML_STATUS_OK is returned we just return
    out of this method without ever passing aBuffer to expat.  Is that really
    correct?  It looks to me like we might lose aBuffer in these cases.
Attachment #169716 - Flags: superreview?(bzbarsky)
Blocks: entities
Peter?  Can you explain why those two cases work ok?
Comment on attachment 195539 [details] [diff] [review]
v1.1

sr- pending answers to the questions in comment 9.
Attachment #195539 - Flags: superreview?(bzbarsky) → superreview-
(In reply to comment #9)
> 1)  Consider a call to nsExpatDriver::ParseBuffer that passes the following
>     buffer (this is the first call to the method):
> 
>     "<root"
> 
>     (note the missing '>').  I assume expat will return XML_STATUS_OK, right?
>     If not, what does it return?  If it does, then in this case, don't we still
>     need to update mBytePosition and mBytesParsed?  And most importantly update
>     mLastLine?  I don't see where we do that...

It returns XML_STATUS_OK. So we don't hit |if (status == XML_STATUS_SUSPENDED)
{| nor |if (status == XML_STATUS_ERROR) {| and fall through to the block:

    if (!aIsFinal && buffer) {
      mLastLine.Append(Substring(buffer + startOffset, buffer + length));
    }
    mBytesParsed += aLength;
    mBytePosition = 0;

I didn't actually change any of this (just changed |if (!parsedAll) {| to |if
(status == XML_STATUS_SUSPENDED) {| and |if (status == XML_STATUS_ERROR) {|).

> 2)  Consider a call to nsExpatDriver::ParseBuffer that happens while expat is
>     suspended (so we want to be unsuspending).  We call XML_ResumeParser and
>     then if, say, XML_STATUS_SUSPENDED is returned start mucking with aBuffer,
>     which we never used.  Worse yet, if XML_STATUS_OK is returned we just return
>     out of this method without ever passing aBuffer to expat.  Is that really
>     correct?  It looks to me like we might lose aBuffer in these cases.

In a call after suspending, aBuffer contains what the internal Expat buffer
contains (which is the data starting from just after we blocked in the previous
call). So there's no problem when we suspend again, we just keep the position in
aBuffer in sync with the position in Expat's internal buffer. If it returns
XML_STATUS_OK we consumed everything from the fragment that made us block, so we
don't want to handle aBuffer anyway (it contains everything that Expat just
consumed from its internal buffer).
Attachment #195539 - Flags: superreview- → superreview?
Ah, ok.  Gotcha.

Last question.  nsExpatDriver has:

866     // we assume that if expat failed to consume some bytes last time it won't
867     // consume them this time without also consuming some bytes from aBuffer.
868     // Note that if expat consumed everything we passed it, it will have nulled
869     // out all its internal pointers to data, so parserBytesConsumed will come
870     // out -1 in that case.

Is that assumption still true?  I guess it is because if it gets suspended we'll
pass the same data in again next time, right?  Please document that, ok?
Comment on attachment 195539 [details] [diff] [review]
v1.1

sr=bzbarsky
Attachment #195539 - Flags: superreview? → superreview+
I tried checking this in today, and the tree went orange. I'm a bit at a loss
why. My own builds (debug and opt) work fine, I can run the pageload tests, etc.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
Any idea yet what's going on here?
I've run with this without a problem. I wonder if perhaps it's the compiler on btek.

Could we put something in place that lets us get a stack, before killing it?
Peter, would you mind attaching whatever the last thing you tried to check in is?
Here's a stack from balsa Dep (gcc 3.4) on SeamonkeyPorts:

  Stack:
  nsProfileLock::FatalSignalHandler(int)+0x0000013D [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libprofile.so +0x00028417]
  UNKNOWN [/lib/i686/libpthread.so.0 +0x000098D5]
  UNKNOWN [/lib/i686/libc.so.6 +0x0002E848]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libhtmlpars.so +0x000306D1]
  nsParser::Tokenize(int)+0x0000015C [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libhtmlpars.so +0x0004875A]
  nsParser::ResumeParse(int, int, int)+0x000001B7 [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libhtmlpars.so +0x0004699B]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libhtmlpars.so +0x000482E8]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/librdf.so +0x00042A31]
  RDFXMLDataSourceImpl::BlockingParse(nsIURI*, nsIStreamListener*)+0x0000034C [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/librdf.so +0x000411D6]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/librdf.so +0x000421A7]
  nsChromeRegistry::LoadDataSource(nsACString_internal const&, nsIRDFDataSource**, int, char const*)+0x00000434 [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libchrome.so +0x0001CC68]
  nsChromeRegistry::GetDynamicDataSource(nsIURI*, int, int, int, nsIRDFDataSource**)+0x00000553 [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libchrome.so +0x0001C015]
  nsChromeRegistry::GetDynamicInfo(nsIURI*, int, nsISimpleEnumerator**)+0x0000008F [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libchrome.so +0x0001C4E7]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libchrome.so +0x0001C0D1]
  nsXULDocument::AddChromeOverlays()+0x000001AB [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00742E21]
  nsXULDocument::PrepareToWalk()+0x00000160 [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00742694]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00749E96]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libdocshell.so +0x00092AD8]
  nsCachedChromeChannel::HandleStopLoadEvent(PLEvent*)+0x0000009E [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libchrome.so +0x00037F0C]
  PL_HandleEvent+0x00000055 [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000F5045]
  PL_ProcessPendingEvents+0x000000C3 [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000F4EFA]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x000F81C1]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0002D212]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0002CBE9]
  UNKNOWN [/usr/lib/libglib-1.2.so.0 +0x0000FF9E]
  UNKNOWN [/usr/lib/libglib-1.2.so.0 +0x00011773]
  UNKNOWN [/usr/lib/libglib-1.2.so.0 +0x00011D39]
  g_main_run+0x0000008C [/usr/lib/libglib-1.2.so.0 +0x00011EEC]
  gtk_main+0x000000D3 [/usr/lib/libgtk-1.2.so.0 +0x00094333]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0002D4F0]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libappcomps.so +0x000AE4F4]
  UNKNOWN [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/seamonkey-bin +0x00010CF6]
  main+0x00000374 [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/seamonkey-bin +0x00011B04]
  __libc_start_main+0x00000093 [/lib/i686/libc.so.6 +0x0001C507]
  Sleeping for 5 minutes.
Attached patch Latest patchSplinter Review
Attachment #199788 - Attachment is obsolete: true
Very interesting that btek managed to go green in between two orange cycles.
>   UNKNOWN
> [/builds/tinderbox/SeaMonkey-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libhtmlpars.so
+0x000306D1]

That's the part we really care about, of course...  :(
Jst found an issue that might cause the orange, I filed it as bug 320375 since it's a bug in the current code (on trunk and branch).
Thanks to jst for finding bug 320375 this was finally checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Yay!
Woot!
Woohoo!
bug 321460 seems to be a serious regression attached to this.
You need to log in before you can comment on or make changes to this bug.