274777 - Update to Expat 1.95.8

Status: RESOLVED FIXED

(Core :: XML, defect, P3)

Description

[Peter Van der Beken [:peterv]]

Need to switch to the new blocking API that Expat provides instead of using our own.

Comment 1

[Benjamin Smedberg]

Beautiful! I don't know much of the code, but I'm willing to help. This is a
1.9-ish thing, right?

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Includes patches for
http://sourceforge.net/tracker/index.php?func=detail&aid=695407&group_id=10127&atid=110127
(aka https://bugzilla.mozilla.org/show_bug.cgi?id=274775) and
http://sourceforge.net/tracker/index.php?func=detail&aid=1090885&group_id=10127&atid=110127

I'm waiting for feedback from the Expat maintainers on those patches.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1 (diff against stock Expat)

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1.1

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.

Comment 5

[Benjamin Smedberg]

Comment on attachment 169716 [details] [diff] [review]
v1.1

I haven't spent years in this code, but from inspection this looks correct.

Comment 6

[Boris Zbarsky [: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....

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Updated to trunk, carrying forward r=bsmedberg. I'll also attach a diff -w of
just the changes to nsExpatDriver.cpp.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.1 (diff -w of nsExpatDriver.cpp)

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Peter?  Can you explain why those two cases work ok?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195539 [details] [diff] [review]
v1.1

sr- pending answers to the questions in comment 9.

Comment 12

[Peter Van der Beken [:peterv]]

(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).

Comment 13

[Boris Zbarsky [:bzbarsky]]

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 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195539 [details] [diff] [review]
v1.1

sr=bzbarsky

Comment 15

[Peter Van der Beken [:peterv]]

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.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Part of the patch that was backed out

Comment 17

[jag (Peter Annema)]

Any idea yet what's going on here?

Comment 18

[jag (Peter Annema)]

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?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Peter, would you mind attaching whatever the last thing you tried to check in is?

Comment 20

[Peter Van der Beken [:peterv]]

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.

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: Latest patch

Comment 22

[jag (Peter Annema)]

Very interesting that btek managed to go green in between two orange cycles.

Comment 23

[Boris Zbarsky [:bzbarsky]]

>   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...  :(

Comment 24

[Peter Van der Beken [:peterv]]

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).

Comment 25

[Peter Van der Beken [:peterv]]

Thanks to jst for finding bug 320375 this was finally checked in.

Comment 26

[jag (Peter Annema)]

Yay!
Woot!
Woohoo!

Comment 27

[Blu3]

bug 321460 seems to be a serious regression attached to this.