360936 - XML parsing regression?

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[chris hofmann]

Starting last week we began getting XML parsing errors on minimo start up, due to some strange response to parsing comments in minimo UI files.

Marcio has the test case.

Comment 1

[/\/\arcio Galli]

Attachment: Screen shot with the parser error

I hope the screenshot helps, it shows that an XML error. The actual error was eliminated when I removed the License Block Comment section.

Comment 2

[/\/\arcio Galli]

BTW a small size comment would not cause any problem. As in <!-- foo --> but if you keep adding characters, when the error happens.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Starting when last week?  And did it start last week on _trunk_ as the version field says?  Or on one of the branches?  If so, which one(s)?

In any case, if there's a testcase, can you attach the testcase instead of a screenshot of the testcase?

Comment 4

[Peter Van der Beken [:peterv]]

Will try to figure this out tomorrow with Marcio, apparently Firefox loads the file without a problem.

Comment 5

[/\/\arcio Galli]

It started with Minimo using Branch 1.8 on Nov 11th. 

Comment 6

[/\/\arcio Galli]

REMINDER: 

I checked in a workaround to some XUL minimo source files. the size of the XML license block was reduced and points to a separated license file. 

As this bug gets fixed, then we have to remove that workaround and probably use the standard license block again to each of the source files. 

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Until we understand what's causing this bug and whether or not it can be reproduced on the branch, we can't block on it. If we can reliably trace this to the XML parser, please set the wanted? flag.

Comment 8

[Jay Patel [:jay]]

Has anyone confirmed whether or not this bug is a regression from bug 335047?  Chofmann made a note of it there on 11/16.

Comment 9

[chris hofmann]

definitely a regression, but its not clear about how widespread the problem might be.  Its possible that it is limited to WinCE.  Maybe related to some difference in the file system or some other OS quirk on that platform that confuses the new changes in the parser.

On 11/28/06, Doug Turner <[e-mail]> wrote:
> I can reproduce the problem.  backing out the changes to the xmlparse.c
> does fix the problem.  I tried backing out of the performance based
> preference but that did not work. 

Comment 10

[Peter Van der Beken [:peterv]]

I am going to try to write some logging code for the branch, so that we can figure out what is going on here and in bug 361180. It is hard to fix these bugs if I can't reproduce them myself.

Comment 11

[Doug Turner (:dougt)]

post a patch and I can try it (ideally, printf's are the way to go... but I will understand if you use NSPR Logging)

Regards,
Doug

Comment 12

[Joshua Randall]

It is not limited to WinCE.  I can reproduce on a Linux handset (kernel 2.4.19) running on armv5tel (XScale-Bulverde rev 7).  Perhaps the problem is ARM related?  

pointer alignment?  signed vs unsigned chars?  

(In reply to comment #9)
> definitely a regression, but its not clear about how widespread the problem
> might be.  Its possible that it is limited to WinCE.  Maybe related to some
> difference in the file system or some other OS quirk on that platform that
> confuses the new changes in the parser.
> 
> On 11/28/06, Doug Turner <[e-mail]> wrote:
> > I can reproduce the problem.  backing out the changes to the xmlparse.c
> > does fix the problem.  I tried backing out of the performance based
> > preference but that did not work. 
> 

Comment 13

[Doug Turner (:dougt)]

are you running minimo on arm, or ff?

Comment 14

[Joshua Randall]

I'm running minimo on arm-linux.  I don't think I'd be able to fit the whole FF on my handset, or I would try it.  

If the problem is with pointer alignment, perhaps it would show up in a Sparc build of FF?


(In reply to comment #13)
> are you running minimo on arm, or ff?
> 

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Possible patch v1

Could someone try this patch and report whether it fixes the bug?

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Logging

This patch adds logging code to nsExpatDriver on the branch. If attachment 247068 [details] [diff] [review] doesn't fix the bug, then I'll need the prlog output (NSPR_LOG_MODULES=expatdriver:5).

Comment 17

[Doug Turner (:dougt)]

patch 247068 seams to have fix the problem.  Could you explain this fix?  

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 247068 [details] [diff] [review]
Possible patch v1

On the branch when we resume the parser after blocking it for a stylesheet load we pass in the same buffer again, but starting at the position just after the content that blocked the parser. Expat always keeps the remainder of the buffer in its internal buffer when returning, and so we end up with two copies of that part. This patch makes Expat clear its buffer when blocked. It seems odd that this hasn't caused bigger problems before, it's a good thing that we cleaned all this up on trunk.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 247068 [details] [diff] [review]
Possible patch v1

Oh, right.  r+sr=bzbarsky.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 247068 [details] [diff] [review]
Possible patch v1

This fixes a regression from a security fix that was landed on the branches (bug 335047). It's not very high-risk (there's always a risk in mucking with these buffers).

Comment 21

[Jay Patel [:jay]]

Comment on attachment 247068 [details] [diff] [review]
Possible patch v1

Approved for 1.8 and 1.8.0 branches, a=jay for drivers.

Comment 22

[Peter Van der Beken [:peterv]]

*** Bug 361180 has been marked as a duplicate of this bug. ***

Comment 23

[Hussam Al-Tayeb]

Has this been checked into MOZILLA_1_8_BRANCH ?

Comment 24

[Hussam Al-Tayeb]

(In reply to comment #23)
> Has this been checked into MOZILLA_1_8_BRANCH ?
> 

Sorry for that. I realize now that this is in 1.8.1.1