live bookmark is shown in UTF8 even if should be ISO-8859-1

RESOLVED FIXED in Firefox 2 alpha3

Status

()

RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: harald.langhammer, Assigned: masayuki)

Tracking

(4 keywords)

Trunk
Firefox 2 alpha3
fixed1.8.1, intl, jp-critical, regression
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9a1) Gecko/20060127 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9a1) Gecko/20060127 Firefox/1.6a1

This live bookmark worked fine until nightly 20060125. Broken in nightly 20060127. According to Boris Zbarsky the live bookmark code was wrong before and the checkin for bug 230275 is now causing this behavior here.

Reproducible: Always

Steps to Reproduce:
1. Add http://www.heise.de/newsticker/heise.rdf as a live bookmark
2. Open dropdown menu of this live bookmark


Actual Results:  
German special chars are replaced with boxes with question mark. Strings are wrongly handled as UTF8 but rss feed is declared being ISO-8859-1

Expected Results:  
Strings handled with declared char set

Quote Boris Zbarsky in bug 230275
| 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.
(Reporter)

Updated

13 years ago
Version: unspecified → Trunk
Vlad, this looks like your code originally.  In brief, we're calling ParseString on nsIXMLRDFParser which has the following IDL:

 65     void parseString(in nsIRDFDataSource aSink, in nsIURI aBaseURI, in AUTF8String aSource);

but you're passing it whatever bytes came off the network.  In particular, they may well not be UTF8 (and are not in this case).

Note that this used to "work" sometimes because of bug 324961, but that fix is needed for proper functioning of other code (and for actually having our API do what it claims to do in all cases, not just sometimes).  I would really like to land that patch on the 1.8 branch as a result, hence the blocking-firefox2 nomination for this bug.

I do wonder why this code is using ParseString instead of ParseAsync (which seems better suited to the data flow here), but given that it is it needs to do its own conversion (to UTF16 and then UTF8).

The relevant callsites are http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp&rev=1.11#390 and http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsBookmarksFeedHandler.cpp#332 (the latter seems to be a fork of the former or something).

Should there be a separate bug on Places here?

I should note that the intl handling of mBody in this code seems generally pretty broken to me.. for example the ParseFromBuffer call used elsewhere in nsBookmarksFeedHandler.cpp doesn't pass in useful charset information...
Blocks: 230275
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox2?

Comment 2

13 years ago
(In reply to comment #1)
> Vlad, this looks like your code originally.  In brief, we're calling
> ParseString on nsIXMLRDFParser...

I know this one from TBird and bz walking me through bug 230275 on IRC. I'll take it unless someone else wants it. 

Probably should be Places-only, right?
*** Bug 325313 has been marked as a duplicate of this bug. ***
Keywords: intl, jp-critical
Keywords: regression
Robert, if you could fix this, that would be great!  Also, if you could muster up a similar patch for 1.5.0.x this would almost certainly get fixed there.
Assignee: nobody → sayrer
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
This isn't a problem on the 1.8.0.x branch, since bug 230275 didn't land there.

Comment 6

13 years ago
*** Bug 330028 has been marked as a duplicate of this bug. ***

Comment 7

13 years ago
Can confirm this bug on 2.0a1 branch builds. Norwegian/Danish characters רזו/ֶ״ֵ are replaced with a black diamond with a white question mark inside.

Bug is present in both Windows and Mac builds, so it is not Windows XP specific.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060413 Firefox/2.0a1
OS: Windows XP → All
Hardware: PC → All
Created attachment 219766 [details] [diff] [review]
Patch rv1.0

fix.
Assignee: sayrer → masayuki
Status: NEW → ASSIGNED
Attachment #219766 - Flags: review?(vladimir)

Comment 9

13 years ago
Is this bug only for live bookmarks or does it also have an effect on "normal" html content?
Just live bookmarks.  If you see a problem with normal HTML, please file a separate bug and cc me.

Comment 11

13 years ago
(In reply to comment #10)
> Just live bookmarks.  If you see a problem with normal HTML, please file a
> separate bug and cc me.
> 

Thanks, but I just saw, that it seems to be fixed, I'll do that if I see that problem again.
Attachment #219766 - Flags: approval-branch-1.8.1?(vladimir)
Comment on attachment 219766 [details] [diff] [review]
Patch rv1.0

I don't really know the parser code all that well; but it looks ok in general.  Let it bake on the trunk for a few days before doing 1.8.
Attachment #219766 - Flags: review?(vladimir) → review+
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 2 beta1 → Firefox 2 alpha2

Comment 14

13 years ago
Problem still existing at e.g. http://www.laut.de/partner/allgemein/news.rdf
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060429 BonEcho/2.0a1
(In reply to comment #14)
> Problem still existing at e.g. http://www.laut.de/partner/allgemein/news.rdf
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060429
> BonEcho/2.0a1
> 

This is not fixed on 1.8branch, this is fixed only on Trunk. We'll check-in if there are no regressions on trunk.

Comment 16

13 years ago
(In reply to comment #15)
> (In reply to comment #14)
> > Problem still existing at e.g. http://www.laut.de/partner/allgemein/news.rdf
> > Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060429
> > BonEcho/2.0a1
> > 
> 
> This is not fixed on 1.8branch, this is fixed only on Trunk. We'll check-in if
> there are no regressions on trunk.
> 

Ooops, sorry, should have noticed that.

Comment 17

13 years ago
Comment on attachment 219766 [details] [diff] [review]
Patch rv1.0


>+    rv = rdfparser->ParseAsync(ds, mURI, getter_AddRefs(listener));
>     if (NS_FAILED(rv)) return rv;
>+    if (!listener) return NS_ERROR_FAILURE;

Is the last line necessary?
(In reply to comment #17)
> (From update of attachment 219766 [details] [diff] [review] [edit])
> 
> >+    rv = rdfparser->ParseAsync(ds, mURI, getter_AddRefs(listener));
> >     if (NS_FAILED(rv)) return rv;
> >+    if (!listener) return NS_ERROR_FAILURE;
> 
> Is the last line necessary?
> 

I don't know the detail of API(|ParseAsync|), therefore I checked it.
*** Bug 337297 has been marked as a duplicate of this bug. ***
Vlad:

We don't have any regression reports. Let's go to 1.8 branch too.

Comment 21

13 years ago
(In reply to comment #20)
> Vlad:
> 
> We don't have any regression reports. Let's go to 1.8 branch too.

Branch is closed.

Comment 22

13 years ago
Branch is open again...

Updated

13 years ago
Attachment #219766 - Flags: approval-branch-1.8.1?(vladimir) → approval-branch-1.8.1?(mconnor)
Attachment #219766 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
checked-in to branch.
Keywords: fixed1.8.1
Target Milestone: Firefox 2 alpha2 → Firefox 2 alpha3
*** Bug 253703 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.