Closed
Bug 324961
Opened 18 years ago
Closed 18 years ago
live bookmark is shown in UTF8 even if should be ISO-8859-1
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 2 alpha3
People
(Reporter: harald.langhammer, Assigned: masayuki)
References
()
Details
(4 keywords)
Attachments
(1 file)
6.34 KB,
patch
|
vlad
:
review+
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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•18 years ago
|
Version: unspecified → Trunk
Comment 1•18 years ago
|
||
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...
Comment 2•18 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?
Comment 3•18 years ago
|
||
*** Bug 325313 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•18 years ago
|
Keywords: intl,
jp-critical
Assignee | ||
Updated•18 years ago
|
Keywords: regression
Comment 4•18 years ago
|
||
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
Comment 5•18 years ago
|
||
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
Updated•18 years ago
|
OS: Windows XP → All
Hardware: PC → All
Assignee | ||
Comment 8•18 years ago
|
||
fix.
Is this bug only for live bookmarks or does it also have an effect on "normal" html content?
Comment 10•18 years ago
|
||
Just live bookmarks. If you see a problem with normal HTML, please file a separate bug and cc me.
Comment 11•18 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.
Assignee | ||
Updated•18 years ago
|
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+
Assignee | ||
Comment 13•18 years ago
|
||
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Target Milestone: Firefox 2 beta1 → Firefox 2 alpha2
Comment 14•18 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
Assignee | ||
Comment 15•18 years ago
|
||
(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•18 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•18 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?
Assignee | ||
Comment 18•18 years ago
|
||
(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.
Comment 19•18 years ago
|
||
*** Bug 337297 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 20•18 years ago
|
||
Vlad: We don't have any regression reports. Let's go to 1.8 branch too.
Comment 21•18 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•18 years ago
|
||
Branch is open again...
Updated•18 years ago
|
Attachment #219766 -
Flags: approval-branch-1.8.1?(vladimir) → approval-branch-1.8.1?(mconnor)
Updated•18 years ago
|
Attachment #219766 -
Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Assignee | ||
Comment 23•18 years ago
|
||
checked-in to branch.
Keywords: fixed1.8.1
Target Milestone: Firefox 2 alpha2 → Firefox 2 alpha3
Comment 24•18 years ago
|
||
*** 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.
Description
•