870916 - crash in nsSocketTransport::BuildSocket @ nsXPTCStubBase::AddRef

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Scoobidiver (away)]

It first showed up in 23.0a1/20130510. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ea059733677c&tochange=08be63954b6b
It might be a regression from bug 870579.

Signature 	nsXPTCStubBase::AddRef() More Reports Search
UUID	75fca066-dec0-4052-bfd1-f827c2130510
Date Processed	2013-05-10 17:55:07
Uptime	4
Last Crash	6 seconds before submission
Install Age	17 seconds since version was first installed.
Install Time	2013-05-10 17:54:27
Product	Firefox
Version	23.0a1
Build ID	20130510041606
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 42 stepping 7
Crash Reason	EXCEPTION_BREAKPOINT
Crash Address	0x5922f12f
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x0116, AdapterSubsysID: 16821043, AdapterDriverVersion: 9.17.10.2932
Has dual GPUs. GPU #2: AdapterVendorID2: 0x10de, AdapterDeviceID2: 0x1050, AdapterSubsysID2: 16821043, AdapterDriverVersion2: 9.18.13.1144D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	sp-processor05_phx1_mozilla_com_3440:2012
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x0116
Total Virtual Memory	4294836224
Available Virtual Memory	3823108096
System Memory Use Percentage	42
Available Page File	12738912256
Available Physical Memory	4845830144

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsXPTCStubBase::AddRef 	xpcom/reflect/xptcall/src/xptcall.cpp:30
1 	xul.dll 	nsCOMPtr_base::assign_with_AddRef 	obj-firefox/xpcom/build/nsCOMPtr.cpp:48
2 	msvcr100.dll 	zzz_AsmCodeRange_End 	
3 	xul.dll 	nsSocketTransport::BuildSocket 	netwerk/base/src/nsSocketTransport2.cpp:1016
4 	xul.dll 	nsSocketTransport::InitiateSocket 	netwerk/base/src/nsSocketTransport2.cpp:1094
5 	xul.dll 	nsSocketTransport::OnSocketEvent 	netwerk/base/src/nsSocketTransport2.cpp:1518
6 	xul.dll 	nsSocketEvent::Run 	netwerk/base/src/nsSocketTransport2.cpp:66
7 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
8 	xul.dll 	NS_ProcessNextEvent 	obj-firefox/xpcom/build/nsThreadUtils.cpp:238
9 	xul.dll 	nsSocketTransportService::Run 	netwerk/base/src/nsSocketTransportService2.cpp:648
10 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
11 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:265
12 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
13 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
14 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
15 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
16 	kernel32.dll 	BaseThreadInitThunk 	
17 	ntdll.dll 	__RtlUserThreadStart 	
18 	ntdll.dll 	_RtlUserThreadStart 	

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsXPTCStubBase%3A%3AAddRef%28%29

Comment 1

[Josh Matthews [:jdm]]

This is almost certainly triggered by bug 770840, meaning that bug 850252 wasn't enough.

Comment 2

[Josh Matthews [:jdm]]

If I'm right, we probably need to change http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSocketTransport2.cpp#1012 and http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/TransportSecurityInfo.h#87.

Comment 3

[Josh Matthews [:jdm]]

Does this look likely to you, Bobby?

Comment 4

[Jason Duell]

re: comment 2: we also pass the callbacks into PSM, so presumably we also need to change these?

   http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp#153

   http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp#145  
   (I assume we'd want to assert main-thread only for gets?)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Josh Matthews [:jdm] from comment #3)
> Does this look likely to you, Bobby?

Likely in what sense? I don't know this code at all.

Comment 6

[Josh Matthews [:jdm]]

Likely in that this is nsXPTCStubBase, not nsXPCWrappedJS.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Josh Matthews [:jdm] from comment #6)
> Likely in that this is nsXPTCStubBase, not nsXPCWrappedJS.

Oh, sorry. They are one in the same.

Comment 8

[Bobby Holley (:bholley)]

(nsXPTCStubBase is the glue that allows XPCWrappedJS to masquerade as arbitrary XPCOM interface types).

Comment 9

[Scoobidiver (away)]

(In reply to Jason Duell (:jduell) from comment #4)
> re: comment 2: we also pass the callbacks into PSM, so presumably we also
> need to change these?
The stack trace on Mac is:
Frame 	Module 	Signature 	Source
0 	XUL 	nsXPCWrappedJS::AddRef 	js/xpconnect/src/XPCWrappedJS.cpp:155
1 	XUL 	nsCertVerificationJob::Run 	obj-firefox/x86_64/dist/include/nsCOMPtr.h:566
2 	XUL 	nsCertVerificationThread::Run 	security/manager/ssl/src/nsCertVerificationThread.cpp:142
3 	libnss3.dylib 	null_signal_handler 	
4 	libnss3.dylib 	_pt_root 	
5 	libsystem_c.dylib 	libsystem_c.dylib@0x147a2 	
6 	libsystem_c.dylib 	libsystem_c.dylib@0x11e1 	
7 	libnss3.dylib 	null_signal_handler

See also bug 870887 for Calendar.

Comment 10

[Josh Matthews [:jdm]]

Actually, I'm really confused about this abort. nsSocketTransport's mCallbacks is implicated, but all the callers of nsSocketTransport::SetSecurityCallbacks pass pointers to C++ types which should not trigger this. I can't figure out where nsSocketTransport would obtain a JS-implemented callbacks object.

Comment 11

[Josh Matthews [:jdm]]

AWOEI#U%(*#QAJFQO#@EEEEEEEEEKDWKMD

The user comments on the related crash reports tipped me off to the KeeFox addon, and I found this: https://github.com/luckyrat/KeeFox/blob/master/Firefox%20addon/KeeFox/modules/session.js#L109

Comment 12

[Josh Matthews [:jdm]]

Attachment: Make socket transport avoid off-main-thread usage of wrapped JS components.

Comment 13

[Bobby Holley (:bholley)]

Does this patch make what that addon does a non-issue? We could also just break the addon and make sure not to crash. Does this need to be IDL-exposed? If not, let's unexpose it. If so, let's make SetSecurityCallbacks check if its argument is a wrappedJS and throw if so. Easiest way to do this is probably to move IsWrappedJS from xpcprivate.h to xpcpublic.h and call it from necko.

Comment 14

[Josh Matthews [:jdm]]

This should make what the addon does a non-issue and continue to work.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 751163 [details] [diff] [review]
Make socket transport avoid off-main-thread usage of wrapped JS components.

https://tbpl.mozilla.org/?tree=Try&rev=12be5cb02e9d shows this isn't ready yet.

Comment 16

[Josh Matthews [:jdm]]

I've kicked off another run with a much simpler patch: https://tbpl.mozilla.org/?tree=Try&rev=65ee375598c9

Comment 17

[Josh Matthews [:jdm]]

Attachment: Make socket transport avoid off-main-thread usage of wrapped JS components.

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 753910 [details] [diff] [review]
Make socket transport avoid off-main-thread usage of wrapped JS components.

Try appears to like this one.

Comment 19

[Patrick McManus [:mcmanus]]

Comment on attachment 753910 [details] [diff] [review]
Make socket transport avoid off-main-thread usage of wrapped JS components.

Review of attachment 753910 [details] [diff] [review]:
-----------------------------------------------------------------

ugh.

Comment 20

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be5476e167e4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be5476e167e4

Comment 22

[Hector Zhao [:hectorz]]

This crash still happen when I play with nsIUDPServerSocket, while nsIServerSocket works fine for me. Sample crash report: https://crash-stats.mozilla.com/report/index/909944d6-c958-44af-b8fe-411de2130710 , should I file a new bug?

Comment 23

[Hector Zhao [:hectorz]]

(In reply to Hector Zhao [:hectorz] from comment #22)
> This crash still happen when I play with nsIUDPServerSocket, while
> nsIServerSocket works fine for me. Sample crash report:
> https://crash-stats.mozilla.com/report/index/909944d6-c958-44af-b8fe-
> 411de2130710 , should I file a new bug?

Seems like bug 850246 should also be done for /netwerk/base/src/nsUDPServerSocket.cpp, I'll give it a try.

Comment 24

[Josh Matthews [:jdm]]

Please do, but in a new bug :)