180848 - nsNSSComponent instance refcounting is broken.

Status: RESOLVED WORKSFORME

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

So, i triggered this assertion:
  NS_ASSERTION( (0 == mInstanceCount), "nsNSSComponent is a singleton, but
instantiated multiple times!");

The problem is that nsNSSComponent::InitializeNSS() fails:
    rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
                                getter_AddRefs(profilePath));
    if (NS_FAILED(rv)) {
but the mInstanceCount was already incremented

nsNSSComponent::InitializeNSS() line 915
nsNSSComponent::Init(nsNSSComponent * const 0x03b4f278) line 1117 + 8 bytes
nsNSSComponentConstructor(nsISupports * 0x00000000, const nsID & {...}, void * *
0x0012f148) line 137 + 139 bytes
nsGenericFactory::CreateInstance(nsGenericFactory * const 0x03ea8fc0,
nsISupports * 0x00000000, const nsID & {...}, void * * 0x0012f148) line 84 + 21
bytes
nsComponentManagerImpl::CreateInstanceByContractID(nsComponentManagerImpl *
const 0x0027d760, const char * 0x028f0d70, nsISupports * 0x00000000, const nsID
& {...}, void * * 0x0012f148) line 1878 + 24 bytes
nsComponentManagerImpl::GetServiceByContractID(nsComponentManagerImpl * const
0x0027d764, const char * 0x028f0d70, const nsID & {...}, void * * 0x0012f1ac)
line 2277 + 50 bytes
nsGetServiceByContractID::operator()(const nsID & {...}, void * * 0x0012f1ac)
line 121 + 38 bytes
nsCOMPtr<nsISignatureVerifier>::assign_from_helper(const nsCOMPtr_helper &
{...}, const nsID & {...}) line 922 + 18 bytes
nsCOMPtr<nsISignatureVerifier>::nsCOMPtr<nsISignatureVerifier>(const
nsCOMPtr_helper & {...}) line 554
nsJAR::GetCertificatePrincipal(nsJAR * const 0x03910404, const char *
0x03de9ef0, nsIPrincipal * * 0x0012f2b8) line 367 + 30 bytes
nsJARChannel::GetOwner(nsJARChannel * const 0x03c65bc8, nsISupports * *
0x0012f2f4) line 495 + 63 bytes
nsDocument::Reset(nsDocument * const 0x04ea9ba8, nsIChannel * 0x03c65bc8,
nsILoadGroup * 0x038b3890) line 748
nsDocument::StartDocumentLoad(nsDocument * const 0x04ea9ba8, const char *
0x021b6b18, nsIChannel * 0x03c65bc8, nsILoadGroup * 0x038b3890, nsISupports *
0x038aa7bc, nsIStreamListener * * 0x0012fb34, int 1, nsIContentSink *
0x00000000) line 852 + 23 bytes
nsImageDocument::StartDocumentLoad(nsImageDocument * const 0x04ea9ba8, const
char * 0x021b6b18, nsIChannel * 0x03c65bc8, nsILoadGroup * 0x038b3890,
nsISupports * 0x038aa7bc, nsIStreamListener * * 0x0012fb34, int 1,
nsIContentSink * 0x00000000) line 247 + 37 bytes
nsContentDLF::CreateDocument(const char * 0x021b6b18, nsIChannel * 0x03c65bc8,
nsILoadGroup * 0x038b3890, nsISupports * 0x038aa7bc, const nsID & {...},
nsIStreamListener * * 0x0012fb34, nsIContentViewer * * 0x0012f8a4) line 435 + 47
bytes
nsContentDLF::CreateInstance(nsContentDLF * const 0x04e59788, const char *
0x021b6b18, nsIChannel * 0x03c65bc8, nsILoadGroup * 0x038b3890, const char *
0x0012fa38, nsISupports * 0x038aa7bc, nsISupports * 0x00000000,
nsIStreamListener * * 0x0012fb34, nsIContentViewer * * 0x0012f8a4) line 283 + 37
bytes
nsDocShell::NewContentViewerObj(nsDocShell * const 0x038aa798, const char *
0x0012fa38, nsIRequest * 0x03c65bc8, nsILoadGroup * 0x038b3890,
nsIStreamListener * * 0x0012fb34, nsIContentViewer * * 0x0012f8a4) line 4543 +
101 bytes
nsDocShell::CreateContentViewer(nsDocShell * const 0x038aa798, const char *
0x0012fa38, nsIRequest * 0x03c65bc8, nsIStreamListener * * 0x0012fb34) line 4413
+ 60 bytes
nsDSURIContentListener::DoContent(nsDSURIContentListener * const 0x038ac668,
const char * 0x0012fa38, int 0, nsIRequest * 0x03c65bc8, nsIStreamListener * *
0x0012fb34, int * 0x0012fa24) line 110 + 33 bytes
nsDocumentOpenInfo::DispatchContent(nsIRequest * 0x03c65bc8, nsISupports *
0x00000000) line 430 + 96 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x04df3f40,
nsIRequest * 0x03c65bc8, nsISupports * 0x00000000) line 229 + 16 bytes
nsJARChannel::OnStartRequest(nsJARChannel * const 0x03c65bcc, nsIRequest *
0x04e02a5c, nsISupports * 0x00000000) line 583
nsOnStartRequestEvent::HandleEvent() line 161 + 53 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x04f04e44) line 116
PL_HandleEvent(PLEvent * 0x04f04e44) line 644 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00fc6c00) line 574 + 9 bytes
_md_TimerProc(HWND__ * 0x016f00de, unsigned int 275, unsigned int 0, unsigned
long 449321890) line 930 + 9 bytes
USER32! 77e13eb0()
USER32! 77e14092()
USER32! 77e13f0f()
nsAppShellService::Run(nsAppShellService * const 0x0105f2c8) line 472
main1(int 1, char * * 0x002847a0, nsISupports * 0x00276eb8) line 1541 + 32 bytes
main(int 1, char * * 0x002847a0) line 1902 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

Comment 1

[timeless]

Attachment: move reference counting

Comment 2

[Kai Engert (:KaiE:)]

The assertion means you tried to create another instance of nsNSSComponent.
Why did it fail in the first place?
It should not fail.

Comment 3

[Kai Engert (:KaiE:)]

The code we are talking about is in PSM, not NSS.

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 106820 [details] [diff] [review]
move reference counting

The destructor would decrease the reference count, so if it is still at one,
you did free the previous instance.

Comment 5

[Kai Engert (:KaiE:)]

The previous comment was missing a word, i meant you did NOT free the previous
instace.

I don't see the point in fixing this reference counting.
You said on IRC PSM init fails because you don't have a special directory.
Obviously your environment is not appropriate for PSM.
If you changed the reference counting like you suggested, the assertion might no
longer show up, but you didn't really fix the problem.
Instead, you will cause another attempt to init NSS whenever another crypto
portion is triggered, which will fail again.

Since PSM can not work without a sane environment, and you seem to work on some
very special kind of environment, I suggest you do not build or use PSM.

Suggesting wontfix.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106820 [details] [diff] [review]
move reference counting

OK.  I'm rescinding the sr after a careful look at the code.  kaie, thanks for
pointing out that this just hides the undelying issue.

However, I do not think this should be wontfixed.  There _is_ a real problem
here.  If Init() fails, the object gets released and should get destroyed,
decrementing mInstanceCount.  This seems to not be happening.

Further, Init() can fail for a bunch of reasons.  eg, getting the bundle could
fail with an out of memory error.  In such cases, the proper thing to do is to
fail out to the caller, who may want to free up some memory and try again.  And
this is the key part.  Just because we fail to initialize once, does not mean
we will continue to fail in the future and we should deal gracefully with an
init failure.  The fact that this assertion fires shows that we are not dealing
gracefully (and we should keep the refcounting right where it is and address
the real problem, it seems).

Comment 7

[John G. Myers]

Mass reassign ssaux bugs to nobody

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I imagine people aren't hitting this assertion, or they would have mentioned it sometime in the past ~14 years.