529734 - Firefox 3.6b3 Crash [@ CMValidateProfile ]

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[chris hofmann]

from very early crash data with low number of beta 3 users

new signature not yet seen in 3.6b1 or b2.  could be a new crash or signature churn.   need to keep eye out, and maybe try to correlate to recent source changes.  no source changes jump out from the quick look I did.  stack traces mostly go through gfx and libpr0n but are varied.  here is one example

http://crash-stats.mozilla.com/report/index/55f81916-f237-404f-a5da-fb0fd2091117

Frame  	Module  	Signature [Expand]  	Source
0 	icm32.dll 	CMValidateProfile 	
1 	mscms.dll 	IsColorProfileValid 	
2 	mscms.dll 	GetProfileInfo 	
3 	mscms.dll 	DoesProfileMatchEnumRecord 	
4 	mscms.dll 	_allmul 	
5 	mscms.dll 	EnumColorProfilesW 	
6 	gdi32.dll 	IcmEnumColorProfile 	
7 	gdi32.dll 	IcmInitialize 	
8 	gdi32.dll 	GetICMProfileW 	
9 	xul.dll 	gfxWindowsPlatform::GetPlatformCMSOutputProfile 	gfx/thebes/src/gfxWindowsPlatform.cpp:915
10 	xul.dll 	info_callback 	modules/libpr0n/decoders/png/nsPNGDecoder.cpp:555

more reports at http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.6b3&query_search=signature&query_type=exact&date=&range_value=1&range_unit=weeks&do_query=1&signature=CMValidateProfile

strong correlation to startup
4 total crashes for CMValidateProfile on 20091117-crashdata.csv
3 start up crashes inside 3 minutes

os breakdown
   4 CMValidateProfile Windows NT 6.0.6001 Service Pack 1

no URLs are in any of the crash reports

--no reports before 2009 11 15
0   total crashes for CMValidateProfile on 20091116-crashdata.csv
4   total crashes for CMValidateProfile on 20091117-crashdata.csv


nominating until we get a bit more volume on b3 and/or know more.

Comment 1

[timeless]

Attachment: this is roughly what it would take

so, this should just be a bug in the os, there's basically absolutely nothing we could do about it.

We can add exception handling.

This needs to be sent to a try server, and it would be nicer if we could contact a victim so they could test w/ that build. I'm less familiar with the Windows CE bits.

Jesse was asking me about a bug for adding exception handling for Windows API bits. this is an example.

But someone should file a bug against Microsoft.

Comment 2

[[e-mail]]

Comment on attachment 413304 [details] [diff] [review]
this is roughly what it would take

Aren't we supposed to use __try/__except?

Comment 3

[Jonathan Kew [:jfkthame]]

It seems odd that this code is within "#ifndef MOZ_FT2_FONTS ... #endif", as color profile support is a separate issue from which font back-end we're using. Surely it would be more appropriate to use something like "#ifndef WINCE" here.

(That wouldn't have any effect on this issue, it just seems wrong on principle. And could be a source of future breakage when we modify the font stuff.)

Comment 4

[timeless]

jonathan: oh sorry, i was just looking for /a/ place which was 'if win and not wince'. It can go anywhere.

neil: probably, but for whatever reason the plugins code doesn't. Until it does, I'd rather be consistent (and when it does, I expect whomever fixes it to fix all occurrences in the tree).

Comment 5

[Marcia Knous [:marcia]]

Are the crashes in [NormalizeColor] (example http://crash-stats.mozilla.com/report/index/43691918-0e8d-4cf8-9cc6-d91b62091123) similar to this crash (is a simiilar OS interaction happening)? Slightly different signature but in the same general areas.

Comment 6

[[e-mail]]

(In reply to comment #4)
> neil: probably, but for whatever reason the plugins code doesn't. Until it
> does, I'd rather be consistent (and when it does, I expect whomever fixes it to
> fix all occurrences in the tree).
What about the 171 occurrences of __except( in accessibility, or the other 13 occurrences in mozilla-central?

Comment 7

[timeless]

CMValidateProfile
UUID	55f81916-f237-404f-a5da-fb0fd2091117
Crash Reason	EXCEPTION_ACCESS_VIOLATION

marcia: yes and no, yes it's the same module, yes it's crashing where it shouldn't, but, no since our entry point is different, the specific fix i was adding wouldn't fix it, it'd need a similar fix at a different point.

your crash (for my/microsoft's reference):
Signature	NormalizeColor
UUID	43691918-0e8d-4cf8-9cc6-d91b62091123
Time 	2009-11-23 11:33:07.516401
Uptime	570
Last Crash	573 seconds before submission
Product	Firefox
Version	3.6b3
Build ID	20091115182845
Branch	1.9.2
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 13
Crash Reason	EXCEPTION_FLT_DIVIDE_BY_ZERO
Crash Address	0x5acf4268
User Comments	
Processor Notes 	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	icm32.dll 	NormalizeColor 	
1 	icm32.dll 	MyNewAbstractW 	
2 	icm32.dll 	CMCreateProfileW 	
3 	mscms.dll 	InternalCreateProfileFromLCS 	
4 	mscms.dll 	CreateProfileFromLogColorSpaceW 	
5 	gdi32.dll 	IcmCreateProfileFromLCS 	
6 	gdi32.dll 	mulff3_c 	
7 	gdi32.dll 	mulff3_c 	
8 	xul.dll 	nsDragService::CreateDragImage 	widget/src/windows/nsDragService.cpp:160
9 	xul.dll 	nsDragService::InvokeDragSession 	widget/src/windows/nsDragService.cpp:255
10 	xul.dll 	nsBaseDragService::InvokeDragSessionWithImage 	widget/src/xpwidgets/nsBaseDragService.cpp:291

Note that the crash reason is also different.

neil: alright.

Comment 8

[Marcia Knous [:marcia]]

(In reply to comment #7)

I filed Bug 532141 to track the Normalize Color crash since it appears to need a different fix and gives a different reason for crashing.

> CMValidateProfile
> bp-55f81916-f237-404f-a5da-fb0fd2091117
> Crash Reason    EXCEPTION_ACCESS_VIOLATION
> 
> marcia: yes and no, yes it's the same module, yes it's crashing where it
> shouldn't, but, no since our entry point is different, the specific fix i was
> adding wouldn't fix it, it'd need a similar fix at a different point.
> 
> your crash (for my/microsoft's reference):
> Signature    NormalizeColor
> bp-43691918-0e8d-4cf8-9cc6-d91b62091123
> Time     2009-11-23 11:33:07.516401
> Uptime    570
> Last Crash    573 seconds before submission
> Product    Firefox
> Version    3.6b3
> Build ID    20091115182845
> Branch    1.9.2
> OS    Windows NT
> OS Version    5.1.2600 Service Pack 2
> CPU    x86
> CPU Info    GenuineIntel family 6 model 15 stepping 13
> Crash Reason    EXCEPTION_FLT_DIVIDE_BY_ZERO
> Crash Address    0x5acf4268
> User Comments    
> Processor Notes     
> Crashing Thread
> Frame     Module     Signature [Expand]     Source
> 0     icm32.dll     NormalizeColor     
> 1     icm32.dll     MyNewAbstractW     
> 2     icm32.dll     CMCreateProfileW     
> 3     mscms.dll     InternalCreateProfileFromLCS     
> 4     mscms.dll     CreateProfileFromLogColorSpaceW     
> 5     gdi32.dll     IcmCreateProfileFromLCS     
> 6     gdi32.dll     mulff3_c     
> 7     gdi32.dll     mulff3_c     
> 8     xul.dll     nsDragService::CreateDragImage    
> widget/src/windows/nsDragService.cpp:160
> 9     xul.dll     nsDragService::InvokeDragSession    
> widget/src/windows/nsDragService.cpp:255
> 10     xul.dll     nsBaseDragService::InvokeDragSessionWithImage    
> widget/src/xpwidgets/nsBaseDragService.cpp:291
> 
> Note that the crash reason is also different.
> 
> neil: alright.

Comment 9

[chris hofmann]

is the patch close enough to make this a blocker rather than just wanted?

Comment 10

[timeless]

Attachment: SEH [Checkin: Comment 13]

Comment 11

[[e-mail]]

Comment on attachment 415590 [details] [diff] [review]
SEH
[Checkin: Comment 13]

Hmm, should we be using ::GetICMProfileW and ::GetExceptionCode?

Comment 12

[timeless]

filed bug 532863 asking to maybe get more information in breakpad.

Comment 13

[Nochum Sossonko [:Natch]]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/033c21be6f86.

Comment 14

[Jacek Caban]

Attachment: mingw fix

This patch broke mingw builds. MinGW doesn't support __try/__except. I've ifdefed its use. Also, while I was at it, str size should be MAX_PATH instead of a magic number, there is no need for extra character not present in size. There is also no guarantee that str will be set to empty string on GetICMProfileW failure, so we should check its return code.

Comment 15

[[e-mail]]

Comment on attachment 420349 [details] [diff] [review]
mingw fix

>+    BOOL res;
...
>     __try {
>-        GetICMProfileW(dc, &size, (LPWSTR)&str);
>+        res = GetICMProfileW(dc, &size, (LPWSTR)&str);
>     } __except(GetExceptionCode() == EXCEPTION_ILLEGAL_INSTRUCTION) {
>         str[0] = 0;
>     }
...
>+    if(!res)
>+        str[0] = 0;
I know str[0] is already 0 in this case, but it looks odd to use res uninitialised in the case of an exception. Would it make more sense simply to directly return nsnull in the failure cases?

Comment 16

[Jacek Caban]

Attachment: mingw fix v1.1 [Checkin: Comment 17]

I'm sorry for late response, I forgot to CC myself. I've attached a version fixed with your suggestions.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 423179 [details] [diff] [review]
mingw fix v1.1
[Checkin: Comment 17]


http://hg.mozilla.org/mozilla-central/rev/fb61277403e3