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.

Nominating nsbeta1 - it cause a big problem on localized JA build. It also can be reproduced on Mac9.1-Ja.

ylong: is this Mac only issue?

Yes, Mac only.

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.

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

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

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

-> future

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.

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

reassign back to ftang

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

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

nsbeta1+ per i18n triage

give to nhotta

reassign to ftang

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.

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.

nhotta- can you r= this one ?

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.

>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.

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

>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.

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.

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.

>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 }

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.

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

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

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 on attachment 76348 [details] [diff] [review] v3 , fix some tab sr=sfraser

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

Comment on attachment 76348 [details] [diff] [review] v3 , fix some tab a=asa (on behalf of drivers) for checkin to the 1.0 trunk

fixed and check in

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