55140 - Possible crash in Unicode font mapping routines

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P3)

Description

[Simon Fraser [no longer active]]

I'm seeing a consistent crash when I run with certain memory manager options 
(using the NewPtr malloc routines) that reveal a nasty bug in 
nsUnicodeFontMappingCache on Mac.

The problem is that nsUnicodeFontMappingMac has a static member which is a 
pointer to a nsUnicodeFontMappingCache, and this global is initialized the first 
time someone asks for the font mapping cache.

However, that nsUnicodeFontMappingCache object can be blown away and replaced in 
nsUnicodeMappingUtil::CleanUp(), which is called when "font.name.*" pref 
callbacks fire. So nsUnicodeFontMappingMac::GetCachedInstance() is problably 
normally running on a deleted object for most people.

Comment 1

[Simon Fraser [no longer active]]

rtm. this could be a serious stability problem, as well as causing unicode font 
mapping bugs. The fix is easy (patches coming).

Comment 2

[Simon Fraser [no longer active]]

Patches:

Rename gCache to mCache, since this is a member, not a global:

Index: mozilla/gfx/src/mac/nsUnicodeMappingUtil.h
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeMappingUtil.h,v
retrieving revision 1.5
diff -w -u -2 -r1.5 nsUnicodeMappingUtil.h
--- nsUnicodeMappingUtil.h	2000/06/14 23:40:17	1.5
+++ nsUnicodeMappingUtil.h	2000/10/04 01:49:07
@@ -70,5 +70,5 @@
 				return mGenericFontMapping[aScript][aType]; 
 	}
-    inline nsUnicodeFontMappingCache* GetFontMappingCache() { return gCache; };
+    inline nsUnicodeFontMappingCache* GetFontMappingCache() { return mCache; };
 	
   ScriptCode MapLangGroupToScriptCode(const char* aLangGroup);
@@ -87,5 +87,5 @@
 	short 	 mScriptFontMapping[smPseudoTotalScripts];
 	PRInt8   mBlockToScriptMapping[kUnicodeBlockSize];
-	nsUnicodeFontMappingCache*	gCache;
+	nsUnicodeFontMappingCache*	mCache;
 	
 	static nsUnicodeMappingUtil* gSingleton;

gCache -> mCache, also set mCache to null after deleting it:

Index: mozilla/gfx/src/mac/nsUnicodeMappingUtil.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeMappingUtil.cpp,v
retrieving revision 1.12
diff -w -u -2 -r1.12 nsUnicodeMappingUtil.cpp
--- nsUnicodeMappingUtil.cpp	2000/06/14 23:40:15	1.12
+++ nsUnicodeMappingUtil.cpp	2000/10/04 01:49:18
@@ -60,5 +60,5 @@
 	InitScriptFontMapping();
 	InitBlockToScriptMapping(); // this must be called after InitScriptEnabled()
-	gCache = new nsUnicodeFontMappingCache();
+	mCache = new nsUnicodeFontMappingCache();
 	++gUnicodeMappingUtilCount;
 }
@@ -71,6 +71,9 @@
 	  	}
 	}
-	if(gCache)
-		delete gCache;
+	if (mCache)
+	{
+		delete mCache;
+		mCache = nsnull;
+	}
 
 }


Remove the bogus gCache global, which is the one that gets stale:

Index: mozilla/gfx/src/mac/nsUnicodeFontMappingMac.h
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeFontMappingMac.h,v
retrieving revision 1.4
diff -w -u -2 -r1.4 nsUnicodeFontMappingMac.h
--- nsUnicodeFontMappingMac.h	2000/06/14 23:40:11	1.4
+++ nsUnicodeFontMappingMac.h	2000/10/04 01:49:38
@@ -58,5 +58,4 @@
    short  mScriptFallbackFontIDs [smPseudoTotalScripts] ;
    static nsUnicodeMappingUtil* gUtil;
-   static nsUnicodeFontMappingCache* gCache;
 };
 

Instead of using gCache here, get the cache each time from the gSingleton:

Index: mozilla/gfx/src/mac/nsUnicodeFontMappingMac.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/mac/nsUnicodeFontMappingMac.cpp,v
retrieving revision 1.11
diff -w -u -2 -r1.11 nsUnicodeFontMappingMac.cpp
--- nsUnicodeFontMappingMac.cpp	2000/08/19 21:32:38	1.11
+++ nsUnicodeFontMappingMac.cpp	2000/10/04 01:50:10
@@ -126,5 +126,4 @@
 //--------------------------------------------------------------------------
 nsUnicodeMappingUtil *nsUnicodeFontMappingMac::gUtil = nsnull;
-nsUnicodeFontMappingCache *nsUnicodeFontMappingMac::gCache = nsnull;
 
 //--------------------------------------------------------------------------
@@ -309,7 +308,9 @@
 	if(! gUtil)
 		gUtil = nsUnicodeMappingUtil::GetSingleton();
-	if(! gCache)
-		gCache = gUtil->GetFontMappingCache();	
 
+	nsUnicodeFontMappingCache* fontMappingCache = gUtil->GetFontMappingCache();	

+  NS_ASSERTION(fontMappingCache, "Should have a fontMappingCache here");
+  if (!fontMappingCache) return nsnull;
+  
 	nsUnicodeFontMappingMac* obj = nsnull;
 	nsAutoString key(aFont->name);
@@ -318,8 +319,8 @@
 	key.AppendWithConversion(":");
 	key.Append(aLANG);
-	if(! gCache->Get ( key, &obj )){
+	if(! fontMappingCache->Get ( key, &obj )){
 		obj = new nsUnicodeFontMappingMac(aFont, aDeviceContext, aLangGroup, 
aLANG);
 		if( obj )
-			gCache->Set ( key, obj);
+			fontMappingCache->Set ( key, obj);
 	}
 	NS_PRECONDITION(nsnull !=  obj, "out of memory");

Comment 3

[Simon Fraser [no longer active]]

These patches fix the problem for me. I can now run and get a browser window! 
Yay!

Comment 4

[Kathleen :Brade]

r=brade

Comment 5

[Frank Tang]

r=ftang. Looks like the right thing to do . We should take this for rtm.
reassign back to sfraser.

Comment 6

[Frank Tang]

pdt, please consider this as RTM++. sfraser have a fix for it. Both brade and I
review the code.

Comment 7

[Simon Fraser [no longer active]]

ftang, do you count as super-reviewer for this code? Or do I need to get someone 
else?

Comment 8

[Jim Roskind]

Crash bugs are Good Catches.  This looks small and safe.. bumping up to rtm
double plus.  Please land asap.

Comment 9

[Simon Fraser [no longer active]]

Fix checked in on branch and trunk.

Comment 10

[Teruko Kobayashi]

Simon, could you give me the instruction how to verify this bug?

Comment 11

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 12

[Simon Fraser [no longer active]]

This would be hard to verify; it should fix some occasional crashes, but I don't 
have steps to reproduce. Best verified as a source-level fix.

Comment 13

[Simon Fraser [no longer active]]

Verifying