Last Comment Bug 455647 - [Indic] Firefox displays garbage Indic characters on parts of some English webpages
: [Indic] Firefox displays garbage Indic characters on parts of some English we...
Status: VERIFIED FIXED
: fixed1.9.1, testcase, verified1.9.0.6
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Linux
P3 normal with 10 votes (vote)
: mozilla1.9.1b2
Assigned To: Karl Tomlinson (:karlt)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-16 22:36 PDT by Parag Nemade
Modified: 2009-01-06 14:22 PST (History)
14 users (show)
vladimir: blocking1.9.1+
samuel.sidler+old: wanted1.9.0.x-
karlt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
snapshot for webpage http://news.bbc.co.uk/2/hi/business/7620127.stm (175.98 KB, image/png)
2008-09-16 22:37 PDT, Parag Nemade
no flags Details
output for http://fedoraproject.org/static/firefox/ (27.91 KB, image/png)
2008-09-16 22:40 PDT, Parag Nemade
no flags Details
while reporting bug to mozilla bugzilla I got this snapshot (95.76 KB, image/png)
2008-09-16 22:41 PDT, Parag Nemade
no flags Details
simple testcase with Lohit Punjabi, DejaVu Sans (524 bytes, text/html)
2008-09-17 20:34 PDT, Karl Tomlinson (:karlt)
no flags Details
replace old glyph run and add an assertion (2.40 KB, patch)
2008-09-17 21:10 PDT, Karl Tomlinson (:karlt)
pavlov: review+
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review
fallback reftest (2.70 KB, patch)
2008-09-17 23:29 PDT, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review

Description User image Parag Nemade 2008-09-16 22:36:30 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) 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.
Comment 1 User image Parag Nemade 2008-09-16 22:37:57 PDT
Created attachment 339012 [details]
snapshot for webpage http://news.bbc.co.uk/2/hi/business/7620127.stm
Comment 2 User image Parag Nemade 2008-09-16 22:40:07 PDT
Created attachment 339013 [details]
output for http://fedoraproject.org/static/firefox/
Comment 3 User image Parag Nemade 2008-09-16 22:41:02 PDT
Created attachment 339014 [details]
while reporting bug to mozilla bugzilla I got this snapshot
Comment 4 User image Parag Nemade 2008-09-16 22:42:13 PDT
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
Comment 5 User image Karl Tomlinson (:karlt) 2008-09-17 19:31:34 PDT
Still an issue on trunk.
Comment 6 User image Karl Tomlinson (:karlt) 2008-09-17 20:34:09 PDT
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.)
Comment 7 User image Karl Tomlinson (:karlt) 2008-09-17 21:02:19 PDT
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).
Comment 8 User image Karl Tomlinson (:karlt) 2008-09-17 21:10:02 PDT
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.
Comment 9 User image Karl Tomlinson (:karlt) 2008-09-17 21:14:45 PDT
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.)
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-09-17 21:25:36 PDT
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 11 User image Karl Tomlinson (:karlt) 2008-09-17 21:34:38 PDT
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.
Comment 12 User image Simon Montagu :smontagu 2008-09-17 22:57:40 PDT
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.
Comment 13 User image Karl Tomlinson (:karlt) 2008-09-17 23:29:15 PDT
Created attachment 339197 [details] [diff] [review]
fallback reftest
Comment 14 User image Karl Tomlinson (:karlt) 2008-09-24 20:57:32 PDT
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.
Comment 15 User image Samuel Sidler (old account; do not CC) 2008-10-04 18:45:18 PDT
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.
Comment 16 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-10-28 18:03:07 PDT
Stuart, can we get a review here?
Comment 18 User image Parag Nemade 2008-11-05 19:53:21 PST
is there any build available to test? Is this patch included in latest nightly builds 3.1b1 ?
Comment 19 User image Karl Tomlinson (:karlt) 2008-11-05 20:02:41 PST
Not 3.1b1, but in nightly builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
Comment 20 User image Parag Nemade 2008-11-06 02:15:41 PST
Thanks Karl for your help and patch here. I see that latest nightly builds fixed above reported problem.
Comment 21 User image Parag Nemade 2008-11-09 21:11:47 PST
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?
Comment 22 User image Martin Stránský 2008-11-10 02:12:31 PST
Yep, fix in 1.9.0.x would be great.
Comment 23 User image Karl Tomlinson (:karlt) 2008-11-12 20:06:13 PST
Comment on attachment 339182 [details] [diff] [review]
replace old glyph run and add an assertion

Requesting approval for 1.9.0.6.  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.
Comment 24 User image Karl Tomlinson (:karlt) 2008-11-12 20:16:11 PST
Fedora bug (so I don't lose it):
https://bugzilla.redhat.com/show_bug.cgi?id=460865
Comment 25 User image Parag Nemade 2008-11-12 20:25:45 PST
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.
Comment 26 User image A S Alam 2008-11-28 04:40:33 PST
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?
Comment 27 User image Jens Petersen 2008-11-30 15:38:40 PST
(In reply to comment #23)
> Requesting approval for 1.9.0.6.  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.
Comment 28 User image Axel Hecht [:Pike] 2008-12-01 03:45:14 PST
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?
Comment 29 User image Samuel Sidler (old account; do not CC) 2008-12-01 05:59:51 PST
Yes, this has baked on trunk long enough and we'll look at the approval request when we start doing 1.9.0.6 triage.
Comment 30 User image Daniel Veditz [:dveditz] 2008-12-10 15:37:00 PST
Comment on attachment 339182 [details] [diff] [review]
replace old glyph run and add an assertion

Approved for 1.9.0.6, a=dveditz for release-drivers.
Comment 32 User image Al Billings [:abillings] 2009-01-06 14:22:06 PST
Verified fixed in 1.9.0.6 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6pre) Gecko/2009010604 GranParadiso/3.0.6pre.

Note You need to log in before you can comment on or make changes to this bug.