646534 - crash [@ _atof_l] under nsCRLManager::ImportCrl

Status: RESOLVED INVALID

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

Signature	_atof_l
UUID	c163c396-0241-47ad-9c33-7b0002110330
Time 	2011-03-30 02:14:17.206590
Uptime	14
Last Crash	56 seconds before submission
Install Age	32857 seconds (9.1 hours) since version was first installed.
Product	Firefox
Version	4.2a1pre
Build ID	20110329030437
Branch	2.2
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
CPU	x86
CPU Info	GenuineIntel family 6 model 28 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x1
User Comments	
App Notes 	AdapterVendorID: 8086, AdapterDeviceID: a011, AdapterDriverVersion: 8.14.10.2230
D3D10 Layers? D3D10 Layers-
D3D9 Layers? D3D9 Layers-
Processor Notes 	WARNING: No 'client_crash_date' could be determined from the Json file
EMCheckCompatibility	False
Bugzilla - Report this Crash
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	mozcrt19.dll 	_atof_l 	obj-firefox/memory/jemalloc/crtsrc/atof.c:59
1 	mozcrt19.dll 	atof 	obj-firefox/memory/jemalloc/crtsrc/atof.c:71
2 	xul.dll 	nsCRLManager::ImportCrl 	security/manager/ssl/src/nsCRLManager.cpp:262
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
4 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1613
5 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:4799
6 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:740
7 	mozjs.dll 	js::ExternalInvoke 	js/src/jsinterp.cpp:863
8 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5173
9 	xul.dll 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1672
10 	xul.dll 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:588
11 	xul.dll 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:114
12 	xul.dll 	SharedStub 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:141
13 	xul.dll 	nsStreamLoader::OnStopRequest 	netwerk/base/src/nsStreamLoader.cpp:125

This is basically because the nsCRLManager code doesn't use the pref service properly. I have a patch series which tries to rewrite this. I'll post it shortly.

Comment 1

[timeless]

Attachment: Correct misspellings in source code: downloads

this belongs to bug 577266, but it's part of my series and thus it's best to have it pushed from here.

Comment 2

[timeless]

Attachment: use doxygen style comment blocks

this is just comment cleanup

Comment 3

[timeless]

Attachment: cleanup: use return instead of goto

while nss uses goto's, c++ really shouldn't.

Comment 4

[timeless]

Attachment: cleanup whitespace

Comment 5

[timeless]

Attachment: fix pref handling, refactor, avoid manual string management

this fixes the bug

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ea6a053c57b0
http://hg.mozilla.org/mozilla-central/rev/0dcdf1c5a7fa

Brian, do you think you can take over the review on the remaining patches here, please?  Thanks!

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 523917 [details] [diff] [review]
cleanup whitespace

r=kaie

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 523918 [details] [diff] [review]
fix pref handling, refactor, avoid manual string management

This patch has too many changes.
It's too difficult to review it.
I don't know how I am supposed to guarantee that your changes doesn't introduce semantic changes.

Why didn't you simply fix the crash first?

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 523918 [details] [diff] [review]
fix pref handling, refactor, avoid manual string management

r-

I'm trying to morph it into changes that result in a readable patch.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 523918 [details] [diff] [review]
fix pref handling, refactor, avoid manual string management

This patch changes error result handling in several scenarios

Comment 11

[Kai Engert (:KaiE:)]

Timeless, please, it's good that you're trying to fix bugs, but it's really unproductive if you produce unreadable patches. I already took me more than 3 hours to un-refactor your patch and trying to get it into a state that is readable.

I'm really sure it was less work for you to write the initial patch.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Part 5 patch v2

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Part 5 patch v2 - ignoring whitespace

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Part 5 patch v3

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Part 5 patch v3 - ignoring whitespace

Comment 16

[Kai Engert (:KaiE:)]

This is a crash bug and should get some attention.

I spent hours on this already, I'd appreciate some help.

But I have no idea whether this crash is really reproducible or if the patch even still applies.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This was fixed by removing the feature. See bug 867465.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 525906 [details] [diff] [review]
Part 5 patch v3 - ignoring whitespace

The feature was removed so I am dropping this review request.