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

VERIFIED FIXED in Firefox 24

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: hectorz, Assigned: hectorz)

Tracking

({crash, regression})

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24 verified, firefox25 verified)

Details

(crash signature)

Attachments

(1 attachment)

+++ 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.
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
Blocks: 770840
Severity: normal → critical
Keywords: crash, regression
Posted patch PatchSplinter Review
I've tested my local build, nsIUDPServerSocket works w/o crash
Attachment #773802 - Flags: review?(mcmanus)
Crash Signature: [@ nsXPTCStubBase::AddRef()] → [@ nsXPTCStubBase::AddRef() ]
Attachment #773802 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b79bf992c475
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Attachment #773802 - Flags: approval-mozilla-aurora?
Comment on attachment 773802 [details] [diff] [review]
Patch

low risk patch fixing crash regression.
Attachment #773802 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
Is there a test to verify this is fixed?
Flags: needinfo?(bzhao)
(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.
Flags: needinfo?(bzhao)
Thanks Hector, flagging for verification with the steps in comment 9.
Keywords: verifyme
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.
QA Contact: ioana.budnar
Also verified on Firefox 25 beta 1, on Ubuntu 13.04 32bit and Mac OSX 10.8.4 64bit.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.