Closed Bug 324961 Opened 17 years ago Closed 16 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 2 alpha3

People

(Reporter: harald.langhammer, Assigned: masayuki)

References

()

Details

(4 keywords)

Attachments

(1 file)

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.
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?
(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. ***
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.
*** Bug 330028 has been marked as a duplicate of this bug. ***
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
Attached patch Patch rv1.0Splinter Review
fix.
Assignee: sayrer → masayuki
Status: NEW → ASSIGNED
Attachment #219766 - Flags: review?(vladimir)
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.
(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
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 2 beta1 → Firefox 2 alpha2
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.
(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 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.
(In reply to comment #20)
> Vlad:
> 
> We don't have any regression reports. Let's go to 1.8 branch too.

Branch is closed.
Branch is open again...
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.