Robert Longson [:longsonr] Reporter
	Description • 17 years ago

      No description provided.

	Robert Longson [:longsonr] Reporter
	Comment 1 • 17 years ago

If the xmlns parts are removed the file loads as an XML file.

	Robert Longson [:longsonr] Reporter
	Comment 2 • 17 years ago

If you make the file small enough by hacking out sections that also makes it load.

	Robert Longson [:longsonr] Reporter
	Comment 3 • 17 years ago

It loads OK in firefox 2.0 but since that does not have incremental XML I guess that's not surprising.

	tor
	Comment 4 • 17 years ago

I don't see an xml error here with the svg version, but if it's a incremental XML thing this could timing dependent.

FWIW, the person who wrote the original needs to fix their SVG as they're trying to use "-1" and "1" as the values of flags of the path "A" command, when the allowed values are "0" and "1".

	Robert Longson [:longsonr] Reporter
	Comment 5 • 17 years ago

It is timing dependent as I get different parse errors each time I refresh the page.

Google found it for me :-) I don't know who the original author is although investigating the source of the jpegs might lead to an answer I suppose.

	Robert Longson [:longsonr] Reporter
	Comment 7 • 17 years ago

I tried this on another computer and I can't get it to break there so maybe it's

a) Vista only, or
b) requires a fast computer to break

	Jonathan Watt [:jwatt]
	Comment 8 • 17 years ago

I've been getting intermittent XML Parsing Error that disappear on refresh etc. for ages. Never reliably reproducible though. Perhaps if you can consistently reproduce the problem you could dig into it a bit?

	Robert Longson [:longsonr] Reporter
	Comment 9 • 17 years ago

Jonathan, can you try taking the attachment and use cut/paste to make it bigger. Just take everything in the <svg> tags and paste that back into the document. If you make it big enough (try 5x as big) it fails for me on my Windows XP computer too.

	Jonathan Watt [:jwatt]
	Comment 10 • 17 years ago

Yeah, making it even just twice the size makes it much more reproducible.

	Jonathan Watt [:jwatt]
	Comment 11 • 17 years ago

Putting a breakpoint here catches the error:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/parser/htmlparser/src/nsExpatDriver.cpp&rev=3.105&mark=807-809#796

I need to continue with my reviews right now, but I can help with this a bit later.

	tor
	Comment 13 • 17 years ago

I see the following assertion before go things south:

###!!! ASSERTION: Inconsistent expat suspension state.: 'status != XML_STATUS_SUSPENDED || (mInternalState == NS_ERROR_HTMLPARSER_BLOCK || mInternalState == NS_ERROR_HTMLPARSER_INTERRUPTED)', file c:/home/moz/trunk/debug/parser/htmlparser/src/../../../../mozilla/parser/htmlparser/src/nsExpatDriver.cpp, line 1008

	alice nodelman [:alice] [:anode]
	Comment 14 • 17 years ago

I added the blocking bug 401146 because this error is being throw on several of our talos testing boxes - causing constantly burning trees (see qm-mini-xp01 and qm-mini-xp04 on MozillaTest tinderbox).  I would rather not push burning trees to the Firefox tinderbox page so this is blocking moving talos machines to production.

	tor
	Comment 15 • 17 years ago

If I add back the "wallpaper" that was removed in the development of the bug 18333 patch, I can't reproduce the XML parse error.  Not sure how we're getting into this state, though.

	Robert Longson [:longsonr] Reporter
	Comment 16 • 17 years ago

I think processing the long d attributes takes so long that the parser returns XML_STATUS_SUSPENDED as per this comment in nsParser.cpp.

This capability is currently used in CNavDTD and nsHTMLContentSink. The
nsHTMLContentSink is notified by CNavDTD when a chunk of tokens is going to be
processed and when each token is processed. The nsHTML content sink records
the time when the chunk has started processing and will return
NS_ERROR_HTMLPARSER_INTERRUPTED if the token processing time has exceeded a
threshold called max tokenizing processing time. This allows the content sink
to limit how much data is processed in a single chunk which in turn gates how
much time is spent away from the event loop. Processing smaller chunks of data
also reduces the time spent in subsequent reflows.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 17 years ago

Peter, is the attached patch the right thing to do?

	Peter Van der Beken [:peterv] Assignee
	Comment 18 • 17 years ago

(In reply to comment #17)
> Peter, is the attached patch the right thing to do?

Nope, we need to find the root cause. I can't reproduce it, but the problem is probably an EndElement call clobbering mInternalState. Need to be careful to handle this without dropping errors.

	Robert Longson [:longsonr] Reporter
	Comment 19 • 17 years ago

If you make the file bigger using cut/paste to duplicate everything within the <svg> tags several times it seems to be easier to reproduce.

	Peter Van der Beken [:peterv] Assignee
	Comment 21 • 17 years ago

I've been able to reproduce this once. Could you guys try this patch and let me know if it seems to fix the problem for you?

	Robert Longson [:longsonr] Reporter
	Comment 22 • 17 years ago

Your patch fixes things for me Peter.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 23 • 17 years ago

https://bugs.maemo.org/show_bug.cgi?id=2040
Same issue?

	Peter Van der Beken [:peterv] Assignee
	Comment 24 • 17 years ago

The issue is that after blocking Expat from the start element handler it can still call at least the end element and the end namespace declaration handlers for empty elements (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/parser/expat/lib/expat.h&rev=1.11#763). When that happens the end element handler shouldn't clobber mInternalState. I thought I could assert mInternalState != NS_ERROR_HTMLPARSER_BLOCK in HandleEndElement/HandleEndNamespaceDecl, but it looks like at least nsCrossSiteListenerProxy::HandleStartElement returns NS_ERROR_HTMLPARSER_BLOCK.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 17 years ago

If we think that it's reasonable to disallow blocking from HandleStartElement, we should do it.  In the case of nsCrossSiteListenerProxy, it should be using NS_ERROR_HTMLPARSER_STOPPARSING anyway, no?  The only issue is that as far as I can see nsExpatDriver doesnt treat that return value as meaning "terminate now".  I think it should.  Of course your patch changes it so that it does!  So want to go ahead and change the proxy to return STOPPARSING and then add that assert?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 26 • 17 years ago

If it's not much extra code to allow blocking from HandleStartElement I think we should allow it. I would be surprised if we'd never end up needing that.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 27 • 17 years ago

So basically, if peters patch already does allow it, I think we should check it in. If not, we should at least add assertions and warnings to the relevant headers.

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 17 years ago

Well, whatever we do here we should change the proxy to terminate the parser, not just pause it, right?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 29 • 17 years ago

Isn't the mParser->Terminate() call enough?

http://lxr.mozilla.org/mozilla/source/content/base/src/nsCrossSiteListenerProxy.cpp#78

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 17 years ago

Ah, indeed.  But in that case nsExpatDriver should probably not clobber a STOPPARSING error could with BLOCK_PARSER either (which is what I think it's doing right now), right?

	Peter Van der Beken [:peterv] Assignee
	Comment 31 • 17 years ago

So maybe we should just return early in HandleEndElement and HandleEndNamespaceDecl if mInternalState == NS_ERROR_HTMLPARSER_STOPPARSING? And change the nsCrossSiteListenerProxy to return NS_ERROR_HTMLPARSER_STOPPARSING.

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 17 years ago

That would make sense to me.  Should we also stop expat if a callback returns STOPPARSING?

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 17 years ago

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

Pending that update.  The rest of this looks reasonable to me.

	Peter Van der Beken [:peterv] Assignee
	Comment 35 • 17 years ago

That wasn't the right patch :-/.

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 17 years ago

Comment on attachment 289172 [details] [diff] [review]
v1.2

Looks reasonable.  Do we want to also call Terminate() if we get back STOPPARSING?

	Peter Van der Beken [:peterv] Assignee
	Comment 37 • 17 years ago

(In reply to comment #36)
> (From update of attachment 289172 [details] [diff] [review])
> Looks reasonable.  Do we want to also call Terminate() if we get back
> STOPPARSING?

nsParser calls Terminate if it gets back STOPPARSING from ConsumeToken, so I don't think there's a need to do it in nsExpatDriver.

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 17 years ago

Ah, ok.  Very good!