Possible crash in Unicode font mapping routines

VERIFIED FIXED

Status

()

Core
Internationalization
P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Simon Fraser, Assigned: Simon Fraser)

Tracking

({crash})

Trunk
PowerPC
Mac System 8.5
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: patch [rtm++])

(Assignee)

Description

18 years ago
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.
(Assignee)

Comment 1

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

Comment 2

18 years ago
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");

Keywords: crash
Whiteboard: patche
(Assignee)

Comment 3

18 years ago
These patches fix the problem for me. I can now run and get a browser window! 
Yay!
Whiteboard: patche → patch

Comment 4

18 years ago
r=brade

Comment 5

18 years ago
r=ftang. Looks like the right thing to do . We should take this for rtm.
reassign back to sfraser.
Assignee: ftang → sfraser

Comment 6

18 years ago
pdt, please consider this as RTM++. sfraser have a fix for it. Both brade and I
review the code.
Whiteboard: patch → patch [rtm+]
(Assignee)

Comment 7

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

Comment 8

18 years ago
Crash bugs are Good Catches.  This looks small and safe.. bumping up to rtm
double plus.  Please land asap.
Whiteboard: patch [rtm+] → patch [rtm++]
(Assignee)

Comment 9

18 years ago
Fix checked in on branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 10

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

Comment 11

18 years ago
Changed QA contact to sfraser@netscape.com.
QA Contact: teruko → sfraser
(Assignee)

Comment 12

18 years ago
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.
(Assignee)

Comment 13

18 years ago
Verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.