110115 - The font face will change on Sidebar/Toolbar after change the language for font in preferences

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Yuying Long]

Build: 11-14 trunk build on Mac OS 10.1

Steps to reproduce:
1. Launch browser, create a Japanese named bookmark.
2. Edit | Preferences | Fonts, change the default language from western to some
others, e.g. Japanese, then click on "OK" button.

Result:
1. The Japanese bookmark is garbled - a screen shot will be followed.
2. If you move the mouse over the tabs name in sidebar but don't click on them,
you will find the font is slightly changed, but not garbled though.

Note: if you quit browser and re-launch again, then the bookmark will display fine.

This problem is orignal found in our Japanese N6.2 build, bug:
http://bugscape/show_bug.cgi?id=10839

Since all the sidebar/toolbars are named in Japanese, after following the steps
above, all of them will display as garbage.

Comment 1

[Yuying Long]

Attachment: sreen shot for gabled bookmark name

Comment 2

[Yuying Long]

Nominating nsbeta1 - it cause a big problem on localized JA build.

It also can be reproduced on Mac9.1-Ja.

Comment 3

[Roy Yokoyama]

ylong: is this Mac only issue?

Comment 4

[Yuying Long]

Yes, Mac only.

Comment 5

[Yuying Long]

Please see the screen shot that attached in bug 10839 of bugscape:
http://bugscape/showattachment.cgi?attach_id=3641

All the double byte characters will be garbled once you move the mouse on the
top of them.

Comment 6

[nhottanscp]

Is there a way to correct the problem?
How about opening a new browser window, is that still garbled?

Comment 7

[Yuying Long]

Yes, if you open a new navigator window or exit application and re-launch again,
then will display properly. 

Comment 8

[nhottanscp]

Mac widget redraw problem, change component to xptoolkit/widgets and reassign to
hyatt.

Comment 9

[John Morrison]

-> future

Comment 10

[Frank Tang]

reassign back to nhotta. 
nhotta, this looks like a font rendering problem instead of a widget problem.
Please keep this bug in our group. Thanks.

Comment 11

[Frank Tang]

ylong, can we still reproduce this. I know some other mac text rendering issue
got fixed recently. 

Comment 12

[Frank Tang]

reassign back to ftang

Comment 13

[Yuying Long]

Yes, I still see it on 01-03 trunk build on Mac9.1JA and 01-02 trunk build on
Mac OS X.

Comment 14

[Frank Tang]

mass move unimportant m9.8 bug to m9.9 for now. 

Comment 15

[nhottanscp]

nsbeta1+ per i18n triage

Comment 16

[Frank Tang]

give to nhotta

Comment 17

[Frank Tang]

reassign to ftang

Comment 18

[Frank Tang]

1. I know the problem is we trash the content of the singleton of the
mFontMapping of nsFontMetricsMac after we click ok in the font pref (if we
change font)
2. sfraser gave me some light about this. the prefchange call back cause this
issue. 
If I temp comment out the nsUnicodeMappingUtil::GetSingleton()->Reset(); in 
nsUnicodeMappingUtil::PrefChangedCallback then the problem won't show up. So the
problem is probably inside the Reset code.

Comment 19

[Frank Tang]

what happen is nsFontMetricsMac hold a reference to the object but the object
are destroy by nsUnicodeMappingUtil::PrefChangedCallback

 when the cache is destory. 

The way to solve it is to add a nsIObserver interface to nsFontMetricsMac and
register itself to observer a event
and when before we remove the cache we notify the observer
the nsFontMetricsMac then clear out the refernce
patch will be attached.

Comment 20

[Frank Tang]

Attachment: v1 patch

Comment 21

[Frank Tang]

nhotta- can you r= this one ?

Comment 22

[nhottanscp]

Comment on attachment 74163 [details] [diff] [review]
v1 patch

r=nhotta

Please make sure nsFontMetricsMac::Observe is only called with
"remove-mac-font-mapping"for aTopic, otherwise you need to check aTopic.

Comment 23

[Frank Tang]

>Please make sure nsFontMetricsMac::Observe is only called with
>"remove-mac-font-mapping"for aTopic, otherwise you need to check aTopic.
we currently only observe one topic, there are no reason it will be called
unless we register to observe other topic. 

Comment 24

[Simon Fraser [no longer active]]

It seems somewhat heavy-weight to go through the observer service for this. I
need to look more closely at the patch.

Comment 25

[Frank Tang]

>It seems somewhat heavy-weight
I am open to other solution. we don't create too many nsIFontMetrics and the
performance impact should only be at the nsIFontMetrics create and destroy time
plus pref font change time. 

Comment 26

[Frank Tang]

the problem is we have a c++ object (not com object) which are refenced by the
nsFontMetricsMac and we clear the low level holding cache through pref. The
refeced need to be invalid before the cache destroyed. If we don't destroy the
cache, then we leak. If we implement the object as com object (add ref count)
then I think it is more heavier than the current proposal. I tried to add a pref
listner in the nsFontMetricsMac level to clear the reference, but I cannot
guarantee that they are called before the cache dstroyed if they listen to the
same pref change.

Comment 27

[Simon Fraser [no longer active]]

I looked some more at the code. Boy, we have some knarly code in there.

The reason I don't like this change is that you are not addressing the real
issue; it's a band-aid patch.

The real issue is that you have a method,
nsUnicodeFontMappingMac::GetCachedInstance(), that returns a pointer to
something that is in a cache, and that cache may get cleared out at any time.
There is no way to indicate to callers that this cache item is going to go away
(which is why the bug occurs). You're imposing on the caller the burden of
knowing this, and installing an observer for this purpose.

I think the cleanest solution would be to eliminate the mFontMapping member on
nsFontMetricsMac, and just get it every time. It's pretty cheap; just a hash
lookup. That way, you can never be holding onto a bad object. You should run the
pageload tests with this change, but I bet the perf difference is imperceptible.

Comment 28

[Frank Tang]

>I think the cleanest solution would be to eliminate the mFontMapping member on
>nsFontMetricsMac, and just get it every time. It's pretty cheap; just a hash
>lookup. 
I have consider that too. I don't think that is THAT cheap. Basically, we need
this for every GetTextDimension and DrawString if don't hold it in the
nsFontMetricsMac. And to access it, we need to call 
nsUnicodeFontMappingMac::GetCachedInstance every time. The funcion is not that
light right now
321 nsUnicodeFontMappingMac* nsUnicodeFontMappingMac::GetCachedInstance(
322 nsFont* aFont, nsIDeviceContext *aDeviceContext, const nsString& aLangGroup,
const nsString& aLANG) 
323 {
324 if(! gUtil)
325 gUtil = nsUnicodeMappingUtil::GetSingleton();
326 327 nsUnicodeFontMappingCache* fontMappingCache = gUtil->GetFontMappingCache(); 
328 NS_ASSERTION(fontMappingCache, "Should have a fontMappingCache here");
329 if (!fontMappingCache) return nsnull;
330 331 nsUnicodeFontMappingMac* obj = nsnull;
332 nsAutoString key(aFont->name);
333 key.Append(NS_LITERAL_STRING(":"));
334 key.Append(aLangGroup);
335 key.Append(NS_LITERAL_STRING(":"));
336 key.Append(aLANG);
337 if(! fontMappingCache->Get ( key, &obj )){
338 obj = new nsUnicodeFontMappingMac(aFont, aDeviceContext, aLangGroup, aLANG);
339 if( obj )
340 fontMappingCache->Set ( key, obj);
341 }
342 NS_PRECONDITION(nsnull !=  obj, "out of memory");
343 return obj;
344 }

Comment 29

[Frank Tang]

the other way is to create one nsUnicodeFontMappingMac for each nsFontMetricsMac
instead of share them in the cache. But if we do that, we MAY need to register a
pref listener in nsFontMetricsMac to listen to font pref changes. 

Comment 30

[Simon Fraser [no longer active]]

Why not do some perf tests with my suggestion? A few pageload tests should tell
us the answer.

Comment 31

[Frank Tang]

we have two approach for this bug and I will show sfraser the performacne
number, I think we could decide today.

Comment 32

[Frank Tang]

Attachment: approach 2

Comment 33

[Frank Tang]

Without changes
http://cowtools.mcom.com/page-loader/report.pl?id=3CA0A7168C
Test id: 3CA0A7168C
Avg. Median : 1832 msec		Minimum     : 592 msec
Average     : 1860 msec		Maximum     : 6552 msec


AFTER v1 patch (http://bugzilla.mozilla.org/attachment.cgi?id=74163&action=view )
http://cowtools.mcom.com/page-loader/report.pl?id=3CA09AE27E
Test id: 3CA09AE27E
Avg. Median : 1836 msec		Minimum     : 593 msec
Average     : 1865 msec		Maximum     : 6563 msec

AFTER approach 2 ( http://bugzilla.mozilla.org/attachment.cgi?id=76212&action=view )
http://cowtools.mcom.com/page-loader/report.pl?id=3CA0AA67E1
Test id: 3CA0AA67E1
Avg. Median : 1844 msec		Minimum     : 589 msec
Average     : 1874 msec		Maximum     : 6549 msec

If we look at the Avg. Median value. 
v1 patch is 0.22% slower than without change
approach 2 is 0.65% slower than without change

sfraser- feel free to pick either approach.

Comment 34

[Frank Tang]

Attachment: v2 of approach 2. remove unnecessary cache code.

Comment 35

[Frank Tang]

Attachment: v3 , fix some tab

Comment 36

[Simon Fraser [no longer active]]

Comment on attachment 76348 [details] [diff] [review]
v3 , fix some tab

sr=sfraser

Comment 37

[nhottanscp]

Comment on attachment 76348 [details] [diff] [review]
v3 , fix some tab

r=nhotta

Comment 38

[Asa Dotzler [:asa]]

Comment on attachment 76348 [details] [diff] [review]
v3 , fix some tab

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 39

[Frank Tang]

fixed and check in

Comment 40

[Yuying Long]

Fixed veeified on 03-08 trunk build with Mac 9.1-JA and 10.1.3.