firefox crashed with SIGSEGV in NSSRWLock_LockRead_Util()

oh right, from my naive perspective, this is a topcrash.
http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Firefox%3A3.0pre&range_value=2&signature=NSSRWLock_LockRead_Util

Explain how it is that this process is not initializing NSS until after it
has run so long that it is already in an out-of-memory condition.

Nelson, Mozilla software will not access PSM unless it has a need to. And only when PSM gets accessed the first time it will init NSS.

According to my understanding, it seems reasonable to start by getting a lock, and only afterwards try to get the module list.

But I don't know! I don't know this code well.

Looking at http://lxr.mozilla.org/seamonkey/ident?i=SECMOD_GetDefaultModuleList
it appears that usually the module list is obtained first, and only afterwards the lock is obtained and locked.

I suspect that the lock is only required for modifying the contents of the lock?

The crash report comment quoted here and the reports i read show this probably *is* early in startup.

I think we use the hashing bits as part of the update service and it seems likely it'd be nearly the first crypto bit to be hit.

please note that i use systems where memory is often but not always scarce. It isn't legal to assume early allocs will succeed any more than it is to assume late ones will.

is there a public contract for "HASH_Create" somewhere?

Ouch...

The NSS contract is NO nss calls are made until NSS is initialized (period). We guarrentee you a crash if you do.

To date, PSM has been the only user of NSS inside a mozilla app.

If someone outside PSM is calling NSS directly, they, at the very least, need to do the minimal xpcom calls to access the crypto object (exported by PSM). Simply setting up that object will guarrentee NSS is initialized properly.

That does not necessarily mean you are home free (PSM keeps track of the NSS objects it creates to make sure it closes them all down if mozilla wants to switch users, for instance, as well as keeps track that mozilla isn't in the middle of shutting down (to make sure no calls are made after NSS is shutdown).

bob

> I suspect that the lock is only required for modifying the contents of the
> lock?

No, the lock is required for both reading and writing. Looking at the code, it's clear it should be acquired before getting the list (as adding, and removal could in theory change the pointer).

In practice the first module is the softoken, which is never removed (though it may change character), so such a bug is unlikely to show up. (Actually changes to the module list is extremely rarer anyway. Adding and deleting pkcs #11 modules in mozilla are the only way it happens in any real application that I know of.

right, I claim that the patch I'm providing here is correct based on your comments. And I'd like it to be taken.

I think that PSM itself may be violating the NSS initialization constraint, but this bug is about flaws in the NSS code. I'll deal w/ any PSM errors in another bug.

Kaie: i think comment 6 should be sufficient for you to r+ attachment 314290

See Bug 410897 Comment 7 (and 8)

Just got this crash: http://crash-stats.mozilla.com/report/index/7db16dc5-3100-11dd-9e8b-0013211cbf8a?p=1

Bob's comment 6 should be interpreted as saying this:

Crashing is the DEFINED BEHAVIOR of NSS when NSS functions are called
before NSS is initialized. It is the caller's responsibility to ensure
that NSS is initialized before calling other NSS functions (besides
NSS initialization functions).

So, by definition, this is NOT an NSS bug.
TWO NSS module owners have now spoken on this subject.

Nelson, where should we move this bug to to get the needed attention (and an acceptable patch)?

Samuel: I think I just moved it to the right place when I added comment 10.

Oh, sorry. I missed that in the Bug Activity. Thanks!

I think we've got the same topcrash on branch, would be nice to fix this there as well if the patch gets reviews and such.

nsNSSModule.cpp has its own copied+modified XPCOM constructors.
Those are designed to do the following:
- if the interface requested means we create an instance of
  class nsNSSComponent, then proceed (because that class
  implements the code that is needed to init NSS)
- if the above is not true, then we first jump through function
  EnsureNSSInitialized and if necessary create the nsNSSComponent singleton.

The crashing code is in nsCryptoHash, which uses the correct macro that tries to achieve the latter.

If this bug is because NSS was not initialized, it means either
- the EnsureNSSInitialized code does not work (or regressed)
or
- you have shut NSS. You might have sent out a profile changed notification, but not yet switched to a new profile

The existing PSM code assumes that you'll never use any of PSM services while you are in the middle of a profile switch.

We should trace the calls to
  NSS_InitReadWrite
  NSS_Shutdown
  HASH_Create

If we see calls in the above order, then we've found the cause of the bug.

We must find a way to ensure it never happens.

Oops. We missed something when nsICryptoHash got implemented in bug 415799.

=> nsNSSShutDownObject

nsCryptoHash and nsCryptoHMAC contain a handle to an NSS object that gets invalid at time of NSS shutdown.

We should have made these classes derive from nsNSSShutDownObject, in order to track/invalidate the references when necessary.

timeless: it is really pity you didn't get the JS stack when it crashed. Do you know about a way to reproduce it?

The theory with the update service seems to me right. I experienced some crashes while closing firefox and its autoupdate was in progress (there were active download).

Just for report: I created fake update file on my local server and I emulate the auto update being in progress in background. When I shutdown Minefield it does NOT crash - tried 10 times...

The theory with update service will probably be wrong because: nsUpdateService.js!_verifyDownload that creates cryptohash instance is called only from OnStopRequest with status NS_OK. When shutdown with download in progress, the download request is canceled from "xpcom-shutdown" notification with NS_ERROR_ABORT status - so _verifyDownload is not called. Anyway, NSS is shutdown by nsNSSComponent in "profile-before-change" event that comes earlier then "xpcom-shutdown". If the download would succeed between these two events and OnStopRequest would be passed to JS somehow we theoretically may crash. But it is highly unlikely to happen.

this was a crash report based bug. i didn't experience it. i saw it in reports and picked a series of paths which aren't correct to patch. they're still not correct. fwiw, it's easy to tell if a crash isn't mine. I only run XP, so if the crash report i pick is vista (6.*) then it can't be mine :).

that psm is broken is quite likely (mentioned in comment 0).

do note that the crash report i picked has a very nice comment in the comments field. :)

Kai: is a patch based on comment 17 likely soon, and if so would it be small? I guess we won't 'block' the current release (since we're trying to wrap it up) but we'd really like to make this crash go away if we can.

(In reply to comment #21)
> Kai: is a patch based on comment 17 likely soon, and if so would it be small? I
> guess we won't 'block' the current release (since we're trying to wrap it up)
> but we'd really like to make this crash go away if we can.

We don't know if implementing what I mention in comment 17 will cure the crash.
It might only help us to understand more.

Created attachment 323866
derivation from nss shutdown object

Tested using firefox restart from add-ons dialog. This won't fix the crash.

I see this popping up to #6 on the topcrash list using a one day sample for RC2.

If anyone can reproduce this, please, list the JS callstack using xpc3250!DumpJSStack(). This prints it to stdout. This should help us.

Comment on attachment 323866
derivation from nss shutdown object

Honza, thanks for working on this patch.

Could you please follow the style that other code implementing nsNSSShutdownObject is using?

For example, please look at nsNSSCertCache.

In the destructor, you should use the locker, check for isAlreadyShutDown.

In virtualDestroyNSSReference all other code checks for isAlreadyShutDown, too. This prevents destroying objects that belong to a shutdown NSS session.

I would recommend you rearrance the code so that you have the same functions called virtualDestroyNSSReference, destructorSafeDestroyNSSReference. As a result, you'll have your calls to destroy the objects only once.

Created attachment 325550
derivation from nss shutdown object, 2

Not blocking at this point, but topcrash+ (#1 topcrash on OSX) so let's see if we can get a safe patch in.

Got this crash on Windows once today:
bp-e34de6cf-428e-11dd-862f-001cc45a2ce4

Kai, does this patch look good? This is currently the #4 topcrash and we'd love to see it fixed in 3.0.2 (and, really 2.0.0.x).

Would be really nice to get this into 1.9.0.2/1.8.1.17

Not going to block on this for 1.9.0.2 (and likely 1.8.1.17). The patches aren't reviewed or landed and this will likely need bake time.

Note that Bug 450468 has a rather similar stack, but not identical.
That appears to be in PRNG code, while this bug is in hash code.

update: we still don't know what causes the crash, the patches in this bug are not a cure for the crash.

I know this bug was originally filed as a Windows Vista bug, but I'm getting [@ NSSRWLock_LockRead_Util] crashes on Windows XP. Namely XP SP 3.

If it's alright, I can provide my crash reports (though I don't have any stack traces available =/ ). If I'm in the wrong place or you'd like a separate bug to be filed, do let me know.

Comment on attachment 314289
don't assume allocs won't fail and lock before reading

Clearing review request per comment 34.

I wonder if going off-line several seconds before shutting down the browser
would help.

Created attachment 387852
v2 for trunk, 1.9.1

Marking haveLoaded = false when we shutdown nss from nsNSSComponent. This really should prevent to get any psm component while nss is not at the moment initialized.

We should also apply the attachment 325550.

Created attachment 387853
v2 for 1.8.1

Bug 505223 is almost certainly a duplicate of this bug. It contains a stack showing that nsCryptoHash::Init is being called while in NS_ShutdownXPCOM_P .

*** Bug 505223 has been marked as a duplicate of this bug. ***

Kai: any ETA on a fix here? This is blocking our next release which we're trying to square away earlier rather than later.

Comment on attachment 384744
v1 for trunk [Checkin comment 73]

I'm assuming these two are obsolete...

Comment on attachment 325550
derivation from nss shutdown object, 2

(In reply to comment #86)
> (From update of attachment 384744 [details])
> I'm assuming these two are obsolete...

No, it is not, I point out in comment 81 we should also take it.

Uh... then we should really do a combined patch instead of separate ones...

Nominating this topcrash for 1.9.0.13 since it looks like these fixes will land on 1.9.1 before then.

Comment on attachment 325550
derivation from nss shutdown object, 2

This patch is good. It adds protection for the "NSS shutdown race" to the nsCryptoHash and nsCryptoHMAC objects, the same protection we already use for other PSM objects that store references to NSS objects.

r=kaie

Comment on attachment 387852
v2 for trunk, 1.9.1

r=kaie

Comment on attachment 387852
v2 for trunk, 1.9.1

http://hg.mozilla.org/mozilla-central/rev/4679ac688c56

Created attachment 391336
v2, as laned on m-c [Checkin comment 92]

Created attachment 391359
v1+v2 merged for 1.9.1

attachment 384744 and attachment 391336 folded and merged to 1.9.1.

Created attachment 391368
v1+v2 merged for 1.9.1 [Checkin comment 101]

This is the correct one...

Comment on attachment 391368
v1+v2 merged for 1.9.1 [Checkin comment 101]

This *just* landed on mozilla-central. I don't think we can take it in 1.9.1.2.

Created attachment 391397
v1+v2 merged for 1.9.0 [Checkin comment 102]

Created attachment 391554
v1+v2 merged for 1.8.1

Comment on attachment 391368
v1+v2 merged for 1.9.1 [Checkin comment 101]

Approved for 1.9.1.3, a=dveditz for release-drivers

Comment on attachment 391397
v1+v2 merged for 1.9.0 [Checkin comment 102]

Approved for 1.9.0.14, a=dveditz for release-drivers

Comment on attachment 391368
v1+v2 merged for 1.9.1 [Checkin comment 101]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/11bdfa7b7c58

Comment on attachment 391397
v1+v2 merged for 1.9.0 [Checkin comment 102]

Checking in security/manager/ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v <-- nsNSSComponent.cpp
new revision: 1.170; previous revision: 1.169
done
Checking in security/manager/ssl/src/nsNSSComponent.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.h,v <-- nsNSSComponent.h
new revision: 1.55; previous revision: 1.54

This fixed the topcrash; there have been no reports of crashes at NSSRWLock_LockRead_Util since July 25 builds.

http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.6a1pre&query_search=signature&query_type=exact&query=&date=&range_value=4&range_unit=weeks&do_query=1&signature=NSSRWLock_LockRead_Util

It still occurs in 3.5.2
Currently #8 in 3.5.2 topcrashes
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.5.2&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=NSSRWLock_LockRead_Util

It was fixed for Gecko 1.9.1.3 per the flags above, meaning that Firefox 3.5.3 should no longer show the issue.

Flag clean up.

Comment on attachment 391554
v1+v2 merged for 1.8.1

Flag clean up.