892340 - Remove Off-Main-Thread XPCWrappedJS refcounting from (UDP)ServerSocketListenerProxy

Status: VERIFIED FIXED

Description

[Hector Zhao [:hectorz]]

+++ This bug was initially created as a clone of Bug #850246 +++

Bug 870916 comment 23:
> (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 1

[Scoobidiver (away)]

Report link, bp-909944d6-c958-44af-b8fe-411de2130710, and stack trace:
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsXPTCStubBase::AddRef() 	xpcom/reflect/xptcall/src/xptcall.cpp
1 	xul.dll 	nsRefPtr<mozilla::dom::CDATASection>::nsRefPtr<mozilla::dom::CDATASection>(mozilla::dom::CDATASection *) 	obj-firefox/dist/include/nsAutoPtr.h
2 	xul.dll 	`anonymous namespace'::ServerSocketListenerProxy::OnPacketReceivedRunnable::OnPacketReceivedRunnable(nsIUDPServerSocketListener *,nsIUDPServerSocket *,nsIUDPMessage *) 	netwerk/base/src/nsUDPServerSocket.cpp
3 	xul.dll 	`anonymous namespace'::ServerSocketListenerProxy::OnPacketReceived(nsIUDPServerSocket *,nsIUDPMessage *) 	netwerk/base/src/nsUDPServerSocket.cpp
4 	xul.dll 	nsUDPServerSocket::OnSocketReady(PRFileDesc *,short) 	netwerk/base/src/nsUDPServerSocket.cpp
5 	xul.dll 	nsSocketTransportService::DoPollIteration(bool) 	netwerk/base/src/nsSocketTransportService2.cpp
6 	xul.dll 	nsSocketTransportService::Run() 	netwerk/base/src/nsSocketTransportService2.cpp
7 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
8 	xul.dll 	nsThread::ThreadFunc(void *) 	xpcom/threads/nsThread.cpp
9 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
10 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
11 	msvcr100.dll 	_callthreadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
12 	msvcr100.dll 	_threadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
13 	kernel32.dll 	BaseThreadInitThunk 	
14 	ntdll.dll 	__RtlUserThreadStart 	
15 	ntdll.dll 	_RtlUserThreadStart

Comment 2

[Hector Zhao [:hectorz]]

Attachment: Patch

I've tested my local build, nsIUDPServerSocket works w/o crash

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Hector Zhao [:hectorz]]

Comment on attachment 773802 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 770840
User impact if declined: Firefox will crash when nsIUDPServerSocket receive any packet
Testing completed (on m-c, etc.): Same patch for nsIServerSocket landed in bug 850246
Risk to taking this patch (and alternatives if risky): Low risk, see above
String or IDL/UUID changes made by this patch: None

Comment 6

[bhavana bajaj [:bajaj]]

Comment on attachment 773802 [details] [diff] [review]
Patch

low risk patch fixing crash regression.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/300800156548

Comment 8

[Anthony Hughes (:ashughes) [QA]]

Is there a test to verify this is fixed?

Comment 9

[Hector Zhao [:hectorz]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #8)
> Is there a test to verify this is fixed?

To manually verify the fix:

Run the following code in browser console:

> let udpServer = {
>   _server: null,
>   init: function() {
>     this._server = Cc["@mozilla.org/network/server-socket-udp;1"]
>                      .createInstance(Ci.nsIUDPServerSocket);
>     this._server.init(49941, false);
>     this._server.asyncListen(this);
>   },
>   uninit: function() {
>     this._server.close();
>   },
>   onStopListening: function(aServ, aStatus) {},
>   onPacketReceived: function(aServ, aMessage) {
>     console.log(aMessage.data);
>   }
> };
> 
> udpServer.init();

Then send a udp packet to it, e.g. in the terminal

> nc -u 127.0.0.1 49941

Type some text in the prompt and then hit Enter

Before the fix Firefox will crash, after the fix the text send using nc should appear in the browser console.

I hope this is helpful. FYI, the code snippet above (using nsIUDPServerSocket) will stop working once bug 869869 is landed, IIUC.

Comment 10

[Anthony Hughes (:ashughes) [QA]]

Thanks Hector, flagging for verification with the steps in comment 9.

Comment 11

[Ioana (away)]

Verified as fixed on Firefox 24 RC (20130910201120). I verified this on Ubuntu 13.04 32bit and Mac OS X 10.7.5 64bit. I couldn't verify it on my Windows 7 because no netcat I got worked on it.

Comment 12

[Ioana (away)]

Also verified on Firefox 25 beta 1, on Ubuntu 13.04 32bit and Mac OSX 10.8.4 64bit.