230275 - [FIXr]XML RDF parser stops parsing at valid unicode char

Status: RESOLVED FIXED

(Core :: XML, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

The URL http://blogs.osafoundation.org/mitch/rss/1.0/ returns an RSS file
encoded in UTF-8 and containing the Unicode character #2013 (EN DASH), which is
encoded into UTF-8 as the sequence e2 80 93.  When the document is retrieved via
XMLHttpRequest and then passed to the XML RDF parser for parsing, the parser
stops parsing the document at the point it reaches the XML tag containing the
aforementioned character.

To see the problem firsthand, save the attached testcase (testcasedata.xml and
testcase.html) to your local system and load testcase.html.  A script will load
the data via XMLHttpRequest (you'll be asked to give the script
UniversalBrowserRead and UniversalXPConnect privileges) and display the
following data:

1. the URL being loaded;
2. the charset as defined by XMLHttpRequest.channel.URI.originCharset;
3. the data as obtained from XMLHttpRequest.responseText;
4. the data as serialized from XMLHttpRequest.responseXML;
5. after parsing by nsIRDFXMLParser, the RDF node containing the Unicode character;
6. after parsing by nsIRDFXMLParser, the data serialized by nsIRDFXMLSerializer.

Expected Results:

The RDF node exists, and the data serialized by nsIRDFXMLSerializer is a
complete copy of the original file.

Actual Results:

The RDF node does not exist, and the data serialized by nsIRDFXMLSerializer
contains only the data up to the point the parser encountered the Unicode character.

Builds Tested:

Linux Mozilla nightly 2004-01-06-07

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: test case data

A copy of the file located at the URL, just in case the data disappears from
that server.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: the test case script

The test case web page.  Note that this page expects the test case data file to
exist in its directory as testcasedata.xml.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Note the results of the following RSS validators, both of which say the RSS
itself is valid:

http://feedvalidator.org/check?url=http%3A%2F%2Fblogs.osafoundation.org%2Fmitch%2Frss%2F1.0%2F
http://aggregator.userland.com/validator?url=http%3A%2F%2Fblogs.osafoundation.org%2Fmitch%2Frss%2F1.0%2F

Comment 4

[Boris Zbarsky [:bzbarsky]]

What a mess...  The RDFParser code does:

113     parser->SetDocumentCharset(NS_LITERAL_CSTRING("UTF-8"),
114                                kCharsetFromDocTypeDefault);

126     nsCOMPtr<nsIInputStream> stream;
127     rv = NS_NewStringInputStream(getter_AddRefs(stream), aString);
128     if (NS_FAILED(rv)) return rv;

If you look at the impl of NS_NewStringInputStream(), it does:

324     char* data = ToNewCString(aStringToRead);

and ToNewCString does NOT convert to UTF-8 -- it just casts the PRUnichars to
char (using the LossyConvertEncoding converter).

So should NS_NewStringInputStream be using ToNewUTF8String instead?  What it
does now for nsAString is _totally_ not useful... ideally, it would either
convert to UTF-8 and then have a byte stream or have a byte stream of UTF-16
bytes, but not what it does....

In fact, see the XXX comment at
http://lxr.mozilla.org/seamonkey/source/dom/src/jsurl/nsJSProtocolHandler.cpp#279

I looked over the callers of NS_NewStringInputStream, and they are all either
dealing with an ascii string encoded in UTF-16 or look buggy.... (form
submission comes to mind in the latter category, I think; upgrading severity
because of dataloss potential there).

Comment 5

[Jungshik Shin]

Oh, my godness ! A while ago, I fixed a bug in mailnews with exactly the same
cause (bug 228543). I always wondered why we escape/unescape Unicode characters
into/out of RDF as opposed to just using UTF-8. I don't know what to say ...

Comment 6

[Jungshik Shin]

Attachment: a patch (not really working)

This is far far from complete. I'm just trying to see how it works. With this,
Unicode characters above U+0080 are not lost, but the parser on the receiving
end interprets them as in ISO-8859-1. 

I found that there's a separate nsIUnicharInputStream whose |Read| guarantees
that a byte sequence representing a single Unicode character is not broken
apart into two separate chunks, (which cannot be done with nsIInputStream).
However, it doesn't seem to be easy to use it where nsIInputStream is used.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> but the parser on the receiving end interprets them as in ISO-8859-1. 

Hmm... Odd.  What's the parser's mCharset when its OnStartRequest method is
called?  We set that to UTF-8 explicitly, no?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: more test case data

Here's another example containing the character #151 (hex 97), which also seems
to stuff up the RDF parser.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

According to http://en.wikipedia.org/wiki/ISO_8859-1, that character exists in
ISO-8859-1 (but not ISO 8859-1).  According to
http://en.wikipedia.org/wiki/Control_character it represents:

151	0x97	EPA	End of Protected Area

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The patch that went in for bug 250119 fixes this particular case; the test case
works for me with both trunk seamonkey and branch firefox.  It doesn't, however,
fix the other users of NewStringInputStream -- bug 230440 for that.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Vlad, this wasn't really fixed by bug 250119 as far as I can see.  If the XML has an encoding decl that specifies something that's not UTF8, we'll parse it in that encoding whereas we should be parsing in UTF8.  So reopening.  This still needs fixing.  Note that people had to work around this breakage at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mail/extensions/newsblog/content/feed-parser.js&rev=1.17&mark=62-66#62 (though they blamed the breakage on XMLHttpRequest when the real issue is the RDF parser being broken).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: I think this is the right fix

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 209501 [details] [diff] [review]
I think this is the right fix

r=mrbkap

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 209501 [details] [diff] [review]
I think this is the right fix

sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209501 [details] [diff] [review]
I think this is the right fix

This is also worth fixing on the branch... it's very safe, and tbird has nasty hackarounds for it...

Comment 17

[Harald Langhammer]

I'm not sure if the fix for this bug caused this but between nightly 20060125 and 20060127 the live bookmark from popular german news magazin Heise (c't) shows question marks instead of german special chars הצ�� on live bookmark dropdown menu

Feed: http://www.heise.de/newsticker/heise.rdf

Works still fine on an 20060125 build, so encoding hasn't changed. Another feed which is not an .rdf (http://www.zdnet.de/feeds/news/xml/rss_h20.xml.htm) works still fine.

Of course it's possible that the feed is wrong and was shown correctly "by accident" till now ;-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Please file a bug on that?  The live bookmark code is just broken -- it passes random data (ISO-88589-1) to a method that takes UTF8.  It used to "work" because the method was broken and didn't treat the data as UTF8, but this bug fixed that.

Really, I'd think the live bookmark code should be using ParseAsync anyway -- it fits their use case much better.

Please cc me on the bug you file?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on branch.