crash in nsNSSASN1Sequence::~nsNSSASN1Sequence @ NS_CycleCollectorSuspect3 with Certificate Patrol 2.0.14

VERIFIED FIXED in Firefox 25

Status

()

Core
Networking
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Scoobidiver (away), Assigned: smaug)

Tracking

({crash, regression, topcrash})

25 Branch
mozilla25
crash, regression, topcrash
Points:
---

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ verified)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
It first showed up in 25.0a1/20130711 and is currently #2 crasher in this build. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=04d8c309fe72&tochange=dde4dcd6fa46
It might be a regression from bug 889831.

Signature 	NS_CycleCollectorSuspect3 More Reports Search
UUID 	0ce6f3c7-7340-4b71-bd82-5fc042130711
Date Processed	2013-07-11 15:41:49.010506
Uptime	457
Last Crash	460 seconds before submission
Install Age 	5836 since version was first installed.
Install Time 	2013-07-11 14:06:24
Product 	Firefox
Version 	25.0a1
Build ID 	20130711030204
Release Channel 	nightly
OS 	Windows NT
OS Version 	6.1.7601 Service Pack 1
Build Architecture 	x86
Build Architecture Info 	AuthenticAMD family 15 model 107 stepping 2 | 2
Crash Reason 	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address 	0x0
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x053e, AdapterSubsysID: 1c41147b, AdapterDriverVersion: 8.15.11.9038
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
Adapter Vendor ID 	0x10de
Adapter Device ID 	0x053e
Total Virtual Memory 	4294836224
Available Virtual Memory 	3694338048
System Memory Use Percentage 	73
Available Page File 	4719177728
Available Physical Memory 	1114165248

Frame 	Module 	Signature 	Source
0 	xul.dll 	NS_CycleCollectorSuspect3 	xpcom/base/nsCycleCollector.cpp
1 	xul.dll 	nsNavHistoryFolderResultNode::Release() 	content/base/src/FragmentOrElement.cpp
2 	xul.dll 	nsNSSASN1Sequence::~nsNSSASN1Sequence() 	security/manager/ssl/src/nsNSSASN1Object.cpp
3 	xul.dll 	nsNSSASN1Sequence::`scalar deleting destructor'(unsigned int) 	
4 	xul.dll 	nsNSSASN1Sequence::Release() 	security/manager/ssl/src/nsNSSASN1Object.cpp
5 	xul.dll 	nsNSSCertificate::~nsNSSCertificate() 	security/manager/ssl/src/nsNSSCertificate.cpp
6 	xul.dll 	nsNSSCertificate::`scalar deleting destructor'(unsigned int) 	
7 	xul.dll 	nsNSSCertificate::Release() 	security/manager/ssl/src/nsNSSCertificate.cpp
8 	xul.dll 	nsSSLStatus::~nsSSLStatus() 	security/manager/ssl/src/nsSSLStatus.cpp
9 	xul.dll 	nsSSLStatus::`scalar deleting destructor'(unsigned int) 	
10 	xul.dll 	nsSSLStatus::Release() 	security/manager/ssl/src/nsSSLStatus.cpp
11 	xul.dll 	mozilla::psm::TransportSecurityInfo::~TransportSecurityInfo() 	security/manager/ssl/src/TransportSecurityInfo.cpp
12 	xul.dll 	nsNSSSocketInfo::`vector deleting destructor'(unsigned int) 	
13 	xul.dll 	mozilla::psm::TransportSecurityInfo::Release() 	security/manager/ssl/src/TransportSecurityInfo.cpp
14 	xul.dll 	nsRefPtr<mozilla::dom::Element>::~nsRefPtr<mozilla::dom::Element>() 	obj-firefox/dist/include/nsAutoPtr.h
15 	xul.dll 	nsSocketTransport::~nsSocketTransport() 	netwerk/base/src/nsSocketTransport2.cpp
16 	xul.dll 	nsSocketTransport::`vector deleting destructor'(unsigned int) 	
17 	xul.dll 	nsSocketTransport::Release() 	netwerk/base/src/nsSocketTransport2.cpp
18 	xul.dll 	nsSocketTransportService::DetachSocket(nsSocketTransportService::SocketContext *,nsSocketTransportService::SocketContext *) 	netwerk/base/src/nsSocketTransportService2.cpp
19 	xul.dll 	nsSocketTransportService::DoPollIteration(bool) 	netwerk/base/src/nsSocketTransportService2.cpp
20 	xul.dll 	nsSocketTransportService::Run() 	netwerk/base/src/nsSocketTransportService2.cpp
21 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
22 	xul.dll 	nsThread::ThreadFunc(void *) 	xpcom/threads/nsThread.cpp
23 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
24 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
25 	msvcr100.dll 	_callthreadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
26 	msvcr100.dll 	_threadstartex 	f:\\dd\\vctools\\crt_bld\\self_x86\\crt\\src\\threadex.c
27 	kernel32.dll 	BaseThreadInitThunk 	
28 	ntdll.dll 	__RtlUserThreadStart 	
29 	ntdll.dll 	_RtlUserThreadStart 	

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=NS_CycleCollectorSuspect3
(In reply to Scoobidiver from comment #0)
> It might be a regression from bug 889831.

Unless there was a typo in the patch, bug 889831 didn't change any functionality - OCSP stapling was pref'd on by default, so everyone was already running that code path.
Note, the code which used to use NS_CycleCollectorSuspect2 is now using NS_CycleCollectorSuspect3.
However the stack starts to look odd after ~nsSocketTransport2.cpp
Component: Security → Networking

Comment 3

4 years ago
Doug, do you think this is ready for any more stack analysis or do we have recommendations for how QA can investigate a regression window or STR here?
Assignee: nobody → doug.turner
tracking-firefox25: ? → +

Updated

4 years ago
Assignee: doug.turner → bugs
This is not for me. The stack looks ok until something odd happens in the network land.
It just happens to jump totally randomly to NS_CycleCollectorSuspect3 (which is a new method added 7/11). Or that is at least what the stack suggests.
Assignee: bugs → nobody
(Reporter)

Comment 5

4 years ago
Checking manually (bug 888219 not fixed) shows it's 100% correlated to Certificate Patrol (CertPatrol@PSYC.EU) 2.0.14.
Summary: crash in nsNSSASN1Sequence::~nsNSSASN1Sequence @ NS_CycleCollectorSuspect3 → crash in nsNSSASN1Sequence::~nsNSSASN1Sequence @ NS_CycleCollectorSuspect3 with Certificate Patrol 2.0.14

Comment 6

4 years ago
(In reply to Scoobidiver from comment #5)
> Checking manually (bug 888219 not fixed) shows it's 100% correlated to
> Certificate Patrol (CertPatrol@PSYC.EU) 2.0.14.

Is that an add-on? If so, does it trigger a problem on our side or do we need to block it?
(Reporter)

Comment 7

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #6)
> (In reply to Scoobidiver from comment #5)
> > Checking manually (bug 888219 not fixed) shows it's 100% correlated to
> > Certificate Patrol (CertPatrol@PSYC.EU) 2.0.14.
> Is that an add-on? If so, does it trigger a problem on our side or do we
> need to block it?
It's an extension as the ID suggests it. There's no binary in the stack trace so it's a Firefox issue but we can also contact the developers.

Comment 8

4 years ago
OK, then it looks like this add-on is triggering the crash, which already is quite helpful. I wonder if devs can reproduce with that?
So my guess is that the addon somehow causes us to create mASN1Objects of nsNSSASN1Sequence
in the main thread (does it call GetASN1Objects?) so that the mutable array is cycle collectable and
we still delete nsNSSASN1Sequence in non-main-thread. Deleting cycle collectable object in some other
thread it was created is just not supported.
nsIMutableArray is super error prone, since depending on the thread it is CCable or not.
Created attachment 778886 [details] [diff] [review]
possible patch

https://tbpl.mozilla.org/?tree=Try&rev=c756d7b36c52

There are perhaps more objects which shouldn't be thread safe, but changed
refcounting of nsNSSASN1Sequence only for now.
Assignee: nobody → bugs
Attachment #778886 - Flags: review?(brian)
Comment on attachment 778886 [details] [diff] [review]
possible patch

Hmm, no, somehow this breaks bug 480509.
Attachment #778886 - Flags: review?(brian)
Created attachment 778888 [details] [diff] [review]
v2

https://tbpl.mozilla.org/?tree=Try&rev=9ce0ab0dc7de

Not beautiful, but seems to pass the tests locally. Trying to keep the old behavior.
I guess someone just doesn't care about the rv value, but expects there to be
a real result.
Attachment #778886 - Attachment is obsolete: true
Attachment #778888 - Flags: review?(brian)
Oh, silly me. Didn't notice 'sequence' is reused later in the method. So this v2 should be ok.
Created attachment 778954 [details] [diff] [review]
alternative approach

https://tbpl.mozilla.org/?tree=Try&rev=d5a45d6f3e3a

I guess either one should be ok.
Attachment #778954 - Flags: review?(brian)
(Reporter)

Comment 15

4 years ago
(In reply to Scoobidiver from comment #5)
> Checking manually (bug 888219 not fixed)
Now that bug is fixed, here are the latest correlations:
     96% (45/47) vs.   2% (47/1931) CertPatrol@PSYC.EU (Certificate Patrol, https://addons.mozilla.org/addon/6415) (2.0.14)
Review ping
Comment on attachment 778954 [details] [diff] [review]
alternative approach

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

You should have an XPCOM peer review the XPCOM changes that add the new parameter to nsArray.

I prefer not to use this as the solution because PSM has never had to explicitly deal with cycle collection yet (AFAICT) and I would like to keep it this way if at all possible.
Attachment #778954 - Flags: review?(brian)
Comment on attachment 778888 [details] [diff] [review]
v2

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

::: security/manager/ssl/src/nsNSSASN1Object.cpp
@@ +13,2 @@
>  NS_IMPL_THREADSAFE_ISUPPORTS2(nsNSSASN1PrintableItem, nsIASN1PrintableItem,
>                                                        nsIASN1Object)

Please also change nsNSSASN1PrintableItem to non-thread-safe nsISupports.

This patch is bitrotted by the patch that removed NS_IMPL_THREADSAFE_ISUPPORTS* so you'll need to redo this part by changing the declarations of the classes.

::: security/manager/ssl/src/nsNSSCertHelper.cpp
@@ +2116,5 @@
>    if (isAlreadyShutDown())
>      return NS_ERROR_NOT_AVAILABLE;
>  
>    nsCOMPtr<nsIASN1Sequence> sequence = new nsNSSASN1Sequence();
> +  NS_ADDREF(*aRetVal = sequence.get());

Nit: No need for two addrefs. Just set *aRetValue = new nsNSSASN1Sequence() directly. (Move the declaration of sequence down to be right above the call to CreateTBSCertificateASN1Struct.)
Attachment #778888 - Flags: review?(brian) → review+
Created attachment 782627 [details] [diff] [review]
using .forget().get()

This way no extra addref/release.
retval is nsIASN1Object, but we need nsIASN1Sequence within the method, so couldn't just assign to retval.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb54b3e090da
https://hg.mozilla.org/mozilla-central/rev/cb54b3e090da
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox25: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Calling this verified fixed since I only see four reports in the last week with this signature on Firefox 25.0a2 and 26.0a1. Should there be a follow-up bug to address the new instances of this signature?
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
You need to log in before you can comment on or make changes to this bug.