175.98 KB, image/png
27.91 KB, image/png
95.76 KB, image/png
524 bytes, text/html
2.40 KB, patch
Stuart Parmenter: review+
|Details | Diff | Splinter Review|
2.70 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:22.214.171.124) Gecko/2008070206 Firefox/3.0.1 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:126.96.36.199) Gecko/2008070206 Firefox/3.0.1 Localized builds are working fine. e.g. when firefox downloaded for Punjabi or Gujarati locales but when started English firefox in pa_IN or gu_IN locales, English web pages are showing characters in Non-English font. Reproducible: Always Steps to Reproduce: 1. download tarball 2. start firefox as "LANG=mr_IN.UTF-8 ./firefox" or "LANG=pa_IN.UTF-8 ./firefox" 3. open following English webpage links http://fedoraproject.org/static/firefox/ http://news.bbc.co.uk/2/hi/business/7620127.stm https://bugzilla.mozilla.org/show_bug.cgi?id=381967 Actual Results: English pages are showing characters from locale in which firefox started where no translations are exist for that locale. Expected Results: English pages should be rendering in English characters only.
Created attachment 339012 [details] snapshot for webpage http://news.bbc.co.uk/2/hi/business/7620127.stm
All attached snapshots are taken with firefox binary English tarball firefox-3.0.1.tar.bz2 and started firefox on fedora linux as LANG=pa_IN.UTF-8 ./firefox
Still an issue on trunk.
Created attachment 339173 [details] simple testcase with Lohit Punjabi, DejaVu Sans Simpler testcase. You wont need to use any particular locale to reproduce, but you will need lohit-fonts for this testcase. (I think different testcases could show the same problem with different fonts though.)
The problem is in creating TextRuns. First the fast path is attempted, and it calls AddGlyphRun with aStartCharIndex == 0. The fast path fails and falls back to the itemizing path. The itemizing path also calls AddGlyphRun with aStartCharIndex == 0 and a different font. The would be fine except that aForceNewRun == PR_TRUE is passed, which means that two different glyph runs with different fonts are recorded for the first run. The comparator used for the sort in SortGlyphRuns says the order of GlyphRuns with the same character offset is not important so the order becomes random and a random font gets used to render the glyphs (the indices of which were selected from a specific font).
Created attachment 339182 [details] [diff] [review] replace old glyph run and add an assertion The documentation for pango_itemize() says "Each byte of text will be contained in exactly one of the items in the returned list; the generated list of items will be in logical order (the start offsets of the items are ascending)." So there is no need to force a new run and sort the runs.
For those with builds without a patch the workaround (as discovered by smontagu) is to set the preference "browser.display.auto_quality_min_font_size" to 0. (My initial hunch of a problem with lohit fonts was wrong.)
It does look like we shouldn't need to sort, but the code you're removing was deliberately added for bug 362682, so I think you should get review from Behdad and/or Stuart.
Comment on attachment 339182 [details] [diff] [review] replace old glyph run and add an assertion Stuart, we only ever increase utf16Offset so I can't think of any reason why we'd need to sort.
By the way, there are two bugs involved here: apart from the textrun bug that Karl found, the fact that we are trying to use the Punjabi font for English text in the first place is an example of bug 91190.
I think the effects of this bug are significant enough that it should be fixed for 1.9.1, and ported to 1.9.0.x after sufficient bake-time. The bug would be expected to affect users in non-Latin locales viewing pages with Latin text in a non-Latin-specific charset without any lang attribute and without any Content-Language header. Whether users see the bug depends on which Latin characters are supported in the font for their native language. Many fonts support most Latin characters, so the effects would only be seen either with fonts designed for a specific script/language or with less common (Latin-1 supplement) characters.
Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.
Stuart, can we get a review here?
Fixed: http://hg.mozilla.org/mozilla-central/rev/8feeaada71eb In testsuite: http://hg.mozilla.org/mozilla-central/rev/c8b770c2fe89 http://hg.mozilla.org/mozilla-central/rev/f404bcc1b7f6
is there any build available to test? Is this patch included in latest nightly builds 3.1b1 ?
Not 3.1b1, but in nightly builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
Thanks Karl for your help and patch here. I see that latest nightly builds fixed above reported problem.
Will be good if this patch will be available in 3.0.x branch also. Can we have this fix applied to 3.0.5 branch?
Yep, fix in 1.9.0.x would be great.
Comment on attachment 339182 [details] [diff] [review] replace old glyph run and add an assertion Requesting approval for 188.8.131.52. This patch changes two lines of code in one function, plus adds an assertion. A reftest would land with it. See comment 14 for discussion on what this bug effects and attachment 339013 [details] for an example of the problem. The small number of changes makes risk very low.
Fedora bug (so I don't lose it): https://bugzilla.redhat.com/show_bug.cgi?id=460865
Thanks Karl, I have tested your patch on 1.9.0.x branch also. Patch is working fine and this bug looks like get fixed on 1.9.0.x branch also along with 1.9.1 branch.
I tested nightly build for 3.1 (1.9.1) and it is working for my language. But Will it be possible to backport to 3.0.0.x?
(In reply to comment #23) > Requesting approval for 184.108.40.206. This patch changes two lines of code in one > function, plus adds an assertion. A reftest would land with it. : > The small number of changes makes risk very low. Thank you Karl for your work on this. I just want to chime in in agreement that it would be very good to get this patch included on the stable branch - the consequences for Indic users are quite serious: a lot of English pages are quite unreadable.
Based on comments here, in irc and on .l10n.in, marking this VERIFIED for trunk. Sam, is having this not shown up any beta 2 blockers good enough as baking?
Yes, this has baked on trunk long enough and we'll look at the approval request when we start doing 220.127.116.11 triage.
Comment on attachment 339182 [details] [diff] [review] replace old glyph run and add an assertion Approved for 18.104.22.168, a=dveditz for release-drivers.
Verified fixed in 22.214.171.124 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:126.96.36.199pre) Gecko/2009010604 GranParadiso/3.0.6pre.