427715 - nsCryptoHash apparently being called while NSS is in shutdown state [@ NSSRWLock_LockRead_Util]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[timeless]

Attachment: don't assume allocs won't fail and lock before reading

Signature	NSSRWLock_LockRead_Util
UUID	e34e0aba-04bf-11dd-8b73-001321b13766
Time	2008-04-07 09:29:16-07:00
Uptime	13
Product	Firefox
Version	3.0pre
Build ID	2008040606
OS	Windows NT
OS Version	6.0.6001 Service Pack 1
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 6
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x0
Comments	Just open and close firefox rapidly and it crash
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	nssutil3.dll 	NSSRWLock_LockRead_Util 	mozilla/security/nss/lib/util/nssrwlk.c:177
1 	nss3.dll 	PK11_GetAllTokens 	mozilla/security/nss/lib/pk11wrap/pk11slot.c:1776
2 	nss3.dll 	PK11_GetBestSlotMultiple 	mozilla/security/nss/lib/pk11wrap/pk11slot.c:1866
3 	nss3.dll 	PK11_GetBestSlot 	mozilla/security/nss/lib/pk11wrap/pk11slot.c:1927
4 	nss3.dll 	PK11_CreateDigestContext 	mozilla/security/nss/lib/pk11wrap/pk11cxt.c:410
5 	nss3.dll 	sha1_NewContext 	mozilla/security/nss/lib/cryptohi/sechash.c:91
6 	nss3.dll 	HASH_Create 	mozilla/security/nss/lib/cryptohi/sechash.c:327
7 	xul.dll 	nsCryptoHash::Init 	mozilla/security/manager/ssl/src/nsNSSComponent.cpp:2422
8 	xul.dll 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
9 	xul.dll 	XPCWrappedNative::CallMethod 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2369

while psm is doing something slightly nasty, i believe that it's using a public entry point. and afaict all the code I'm changing is wrong. I'm not sure about whitespace behavior, one place uses 3-tab;4-tab which seems wrong. i prefer no-tab and 4-4, but i'm trying to follow "conventions".

Comment 1

[timeless]

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

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 3

[Kai Engert [:KaiE:]]

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.

Comment 4

[Kai Engert [:KaiE:]]

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?

Comment 5

[timeless]

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?

Comment 6

[Robert Relyea]

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.

Comment 7

[timeless]

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 [details] [diff] [review]

Comment 8

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

See Bug 410897 Comment 7 (and 8)

Comment 9

[David Weitz]

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

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 11

[Samuel Sidler (old account; do not CC)]

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

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 13

[Samuel Sidler (old account; do not CC)]

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

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Kai Engert [:KaiE:]]

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.

Comment 16

[Kai Engert [:KaiE:]]

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.

Comment 17

[Kai Engert [:KaiE:]]

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.

Comment 18

[Honza Bambas (:mayhemer)]

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).

Comment 19

[Honza Bambas (:mayhemer)]

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.

Comment 20

[timeless]

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. :)

Comment 21

[Daniel Veditz [:dveditz]]

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.

Comment 22

[Kai Engert [:KaiE:]]

(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.

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: derivation from nss shutdown object

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

Comment 24

[chris hofmann]

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

Comment 25

[Honza Bambas (:mayhemer)]

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

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 323866 [details] [diff] [review]
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.

Comment 27

[Honza Bambas (:mayhemer)]

Attachment: derivation from nss shutdown object, 2

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 29

[Marco Zehe (:MarcoZ)]

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

Comment 30

[Samuel Sidler (old account; do not CC)]

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).

Comment 31

[Daniel Veditz [:dveditz]]

Would be really nice to get this into 1.9.0.2/1.8.1.17

Comment 32

[Samuel Sidler (old account; do not CC)]

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.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 34

[Kai Engert [:KaiE:]]

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

Comment 35

[Ngamer01]

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 36

[Samuel Sidler (old account; do not CC)]

Comment on attachment 314289 [details] [diff] [review]
don't assume allocs won't fail and lock before reading

Clearing review request per comment 34.

Comment 37

[Samuel Sidler (old account; do not CC)]

Comment on attachment 325550 [details] [diff] [review]
derivation from nss shutdown object, 2

Clearing review request per comment 34.

Comment 38

[Daniel Veditz [:dveditz]]

(In reply to comment #35)
> I know this bug was originally filed as a Windows Vista bug,

Vista vs. XP shouldn't make a difference for this crash. You're in the right place, we don't need a separate bug, and thanks for the offer of stack traces but we should have all we need.

What we really need is a way to reproduce the crash at will so we can debug it.

Comment 39

[David Maza]

We if it helps, for me it often happens when restarting after installing extensions.

Comment 40

[Honza Bambas (:mayhemer)]

(In reply to comment #39)
> We if it helps, for me it often happens when restarting after installing
> extensions.

Are you 100% sure it is this crash? If so, are you able to attach a debugger to FF? Best, _before_ it crash?

...

Actually, what would be very useful is a JS stack - the source of call. It could be one of these, but not limited to:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/src/nsUpdateService.js.in#2506

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginInstallerService.js#165

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/multi-querier.js#133
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/multi-querier.js#142

Personally I most suspect the url classifier.

Comment 41

[David Maza]

The crash tends to occur when I'm using the restart function after installing or updating an extension for me at least, but it can occur when Firefox is shutting down.

I've opened 12 tabs at crash-stats although after 3 hours none of them have been processed yet. crash-stats really needs a server upgrade...

Here's a list of all the crash reports relating to this bug which have occurred on my computer since 3.0.0:

http://crash-stats.mozilla.com/report/index/4b43e099-9464-11dd-be25-001cc45a2ce4
http://crash-stats.mozilla.com/report/index/d5c55c40-8d6f-11dd-938e-001a4bd43e5c
http://crash-stats.mozilla.com/report/index/3efa89a8-86c3-11dd-a6e7-0013211cbf8a
http://crash-stats.mozilla.com/report/index/f60be78c-8591-11dd-8d30-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/55330e85-8517-11dd-a2e0-0013211cbf8a
http://crash-stats.mozilla.com/report/index/e6b61ed7-83c4-11dd-8168-001cc45a2ce4
http://crash-stats.mozilla.com/report/index/7d50ab2e-8230-11dd-8c0c-001cc45a2c28
http://crash-stats.mozilla.com/report/index/63b4604f-8165-11dd-b872-001cc45a2c28
http://crash-stats.mozilla.com/report/index/24b0b1c2-7bb4-11dd-883a-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/03d38a78-80c1-11dd-ac75-001cc45a2c28
http://crash-stats.mozilla.com/report/index/7b3ae52e-7636-11dd-acbd-001cc45a2c28
http://crash-stats.mozilla.com/report/index/acfd6c0a-7305-11dd-8857-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/112915bd-6b5e-11dd-9da5-0013211cbf8a
http://crash-stats.mozilla.com/report/index/effe29c0-6add-11dd-a69d-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/3f78b8e1-6a0b-11dd-99cc-001cc45a2c28
http://crash-stats.mozilla.com/report/index/c40fb91e-69d9-11dd-9cb9-001321b13766
http://crash-stats.mozilla.com/report/index/a4307f35-6932-11dd-9f79-001cc45a2ce4
http://crash-stats.mozilla.com/report/index/98ffc88a-6921-11dd-9c64-001cc45a2ce4
http://crash-stats.mozilla.com/report/index/23e60c82-691a-11dd-a7bc-001a4bd43e5c
http://crash-stats.mozilla.com/report/index/f02a3f23-6867-11dd-a17d-001cc4e2bf68
http://crash-stats.mozilla.com/report/index/b38c2663-6848-11dd-8709-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/3b39cdca-6108-11dd-83d7-001321b13766
http://crash-stats.mozilla.com/report/index/718bcbf5-60ab-11dd-957d-0013211cbf8a
http://crash-stats.mozilla.com/report/index/92950acc-609c-11dd-8eac-001cc45a2c28
http://crash-stats.mozilla.com/report/index/99269882-5fcd-11dd-9b74-0013211cbf8a
http://crash-stats.mozilla.com/report/index/ad2b8205-5fc6-11dd-b565-0013211cbf8a
http://crash-stats.mozilla.com/report/index/c59ee811-5fb5-11dd-9f25-001cc4e2bf68



The first 12 haven't been processed yet so they may relate to other bugs although the problem seems to be coming from

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/nssrwlk.c&rev=1.8&mark=177#177

Comment 42

[Honza Bambas (:mayhemer)]

Thanks for this report. I'm interested in complete stack trace on the main thread at least to see if it were called from OnStopRequest of incremental download (to confirm culprit is the update service). Is there a way to see the complete stack from the crash reporter and not just first 10 frames?

David, are you willing to attach debugger and try to reproduce the crash? We can join at IRC (mayhemer @ #developers) to discuss how.

Comment 43

[David Maza]

Sure, I'll be waiting there.

Comment 44

[Benjamin Smedberg]

Topcrash, blocking+... kaie, are you going to be able to look at this?

Comment 45

[Diego]

Seems to be more triggered in OS X current nightly (20081119) according to:

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&version=Firefox%3A3.1b2pre&signature=NSSRWLock_LockRead_Util

People are stating that the crash occurs in current build when restarting with saved tabs and when Weave is enabled.

Comment 46

[Honza Bambas (:mayhemer)]

I can reproduce that in a debugger. It seems it is quit different problem because this happens during start of firefox and not during quit. Filed new bug 465974.

Comment 47

[Honza Bambas (:mayhemer)]

During fixing of bug 456705 I figured out that URL classifier uses always_async proxy to invoke its callback that uses crypto hash. When proxy is used right before shutdown the callback could potentially be invoked after nss has been shutdown. After that bug lands this bug could also be fixed. See bug 456705 comment 59.

CC'ing Dave Camp as author of the URL classifier.

Comment 48

[Tom Parker]

We've just seen what appears to be an instance of this bug at http://bugzilla.gnome.org/show_bug.cgi?id=571228 but in Evolution, not Firefox. Evolution is using the NSS libraries however, and given the reporter was closing down Evolution at the time, it looks like a similar problem.

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

This may be going off topic, but ...
Does Evolution use Mozilla's nsCryptoHash class?

Comment 50

[Kai Engert [:KaiE:]]

Tom Parker, I guess the question in comment 49 was addressed to you, but I just made a source-level-search:

evolution-2.24.4]$ grep -ril cryptohash .
=> nothing

...

I propose, let's wait for Honza's fix in bug 456705 to land, then have another look at crash reports.

Comment 51

[chris hofmann]

looks like it landed a while ago. whats next?

Comment 52

[Honza Bambas (:mayhemer)]

This bug will probably be fixed with bug 456705 that has been landed on trunk and 1.9.1. It should also land on 1.9.0 and 1.8.1.

Comment 53

[chris hofmann]

ok,

wanted 1.9.0.x  and wanted 1.8.0.x  flags should get it on the radar for those releases.  

I'll mark this fixed.   change the bug again if this is not right.

Comment 54

[(not currently active) Ted Mielczarek]

Sounds like this isn't fixed:
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.5b99&platform=windows&query_search=signature&query_type=exact&query=NSSRWLock_LockRead_Util&date=&range_value=1&range_unit=weeks&do_query=1&signature=NSSRWLock_LockRead_Util#table

Same stack, 3.5b99.

Comment 55

[Honza Bambas (:mayhemer)]

There are two stacks:
http://crash-stats.mozilla.com/report/index/568b34ff-ed68-4dc6-8c89-14c112090622
http://crash-stats.mozilla.com/report/index/57aae0d3-19a0-46ce-adec-676bf2090622

Good news is that initWithString is invoked only here:
http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/mozapps/plugins/content/pluginInstallerService.js#62

This crash is 95% of all reports.

Comment 56

[Honza Bambas (:mayhemer)]

In the former case I suspect this: http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/mozapps/update/src/nsUpdateService.js.in#2415

What hash algorithm is used for application updates signing? Is it SHA256?

Comment 57

[Mike Beltzner [:beltzner, not reading bugmail]]

Honza: how do you figure on 95% of crashes? This is the #2 topcrash by my queries.

Is the assertion here that our first fix was so insufficient that this needs to block again? That's what's happened.

Comment 58

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #56)
> In the former case I suspect this:
> http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/mozapps/update/src/nsUpdateService.js.in#2415
> 
> What hash algorithm is used for application updates signing? Is it SHA256?

We switched from SHA1 to SHA512 for app. update hashes when 3.5b99 was released.

Comment 59

[Honza Bambas (:mayhemer)]

(In reply to comment #57)
> Honza: how do you figure on 95% of crashes? This is the #2 topcrash by my
> queries.

95% = my guess. For this signature I can see a lot of stacks with call to initWithString. Only few stack are with call to init.

> 
> Is the assertion here that our first fix was so insufficient that this needs to
> block again? That's what's happened.

Seems so. I'll revisit the patch ones again or try to reproduce.

Comment 60

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #58)
> (In reply to comment #56)
> > In the former case I suspect this:
> > http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/mozapps/update/src/nsUpdateService.js.in#2415
> > 
> > What hash algorithm is used for application updates signing? Is it SHA256?
> 
> We switched from SHA1 to SHA512 for app. update hashes when 3.5b99 was
> released.

So does this mean that we can mitigate this crash by using a different SHA algorithm until we fix this bug? Most of the comments in the crash reports indicate that the crash happens on browser-close, which isn't as concerning to me.

Comment 61

[Honza Bambas (:mayhemer)]

I may know the cause: when PSM is shutting down in "profile-before-change", we call nsNSSComponent::ShutdownNSS that calls ::NSS_Shutdown(). But, the nsNSSComponent service may not be released immediately after we do that and as there are many other observers processing "profile-before-change" after we shutdown nss, event loop may be triggered and one of the stacks I provide may get called after we have already shut nss down.

I'll provide a patch for this.

Comment 62

[Mike Beltzner [:beltzner, not reading bugmail]]

Honza: so does that mean that you also think this is happening on shutdown only?

Comment 63

[Honza Bambas (:mayhemer)]

(In reply to comment #60)
> So does this mean that we can mitigate this crash by using a different SHA
> algorithm until we fix this bug? 

Absolutely not :) I was just trying to find out if the code I suspect is the
one that really crashes. You say you are using sha512, isn't it really a sha256? If not, comes to your mind anything that could be using sha256?

One more interesting stack:
http://crash-stats.mozilla.com/report/index/929cf9da-0e8b-4dfb-9421-6196e2090621
It confirms comment 61.

(In reply to comment #62)
> Honza: so does that mean that you also think this is happening on shutdown
> only?

I certainly do.

Comment 64

[Mike Beltzner [:beltzner, not reading bugmail]]

As per IRC discussion with bsmedberg, wanted1.9.1.x (we should take it for 3.5.1) but not blocking final release.

Comment 65

[Mike Beltzner [:beltzner, not reading bugmail]]

(ugh, fixing flags)

Comment 66

[Nick Thomas [:nthomas] (UTC+12)]

I don't understand why switching from SHA1 to SHA512 would matter, we call nsCryptoHash either way AFAICT. There have been changes to the updater, CC'ing Rob Strong and Mossop.

It really is SHA512 btw,
https://aus2.mozilla.org/update/1/Firefox/3.5b99/20090605162636/WINNT_x86-msvc/en-US/beta/update.xml?force=1

Comment 67

[Robert Strong (they/them - no direct email)]

(In reply to comment #66)
> I don't understand why switching from SHA1 to SHA512 would matter, we call
> nsCryptoHash either way AFAICT. There have been changes to the updater, CC'ing
> Rob Strong and Mossop.
Per comment #63 it doesn't matter. I don't think there were any changes to the update service that would cause this and this bug has been around since at least 2008-04-08

Comment 68

[Honza Bambas (:mayhemer)]

Attachment: v1 for trunk [Checkin comment 73]

Preliminary patch, I have to test it with use case from bug 456705 to check it still prevents the original crash.

Comment 69

[Honza Bambas (:mayhemer)]

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

Tested with test case from bug 456705 and with SeaMonkey.

Comment 70

[Honza Bambas (:mayhemer)]

Attachment: v1 for 1.9.0 (identical, just merged)

Same as attachment 384744 [details] [diff] [review].

Comment 71

[Kai Engert [:KaiE:]]

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

r=kaie

Comment 72

[Honza Bambas (:mayhemer)]

Kai, thanks for your quick review! I think I can start landing this.

Comment 73

[Honza Bambas (:mayhemer)]

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

http://hg.mozilla.org/mozilla-central/rev/50a10946e41f

Comment 74

[Honza Bambas (:mayhemer)]

Attachment: v1 for 1.8.1

Comment 75

[Honza Bambas (:mayhemer)]

Attachment: v1 for 1.9.0 (fixed build bustage on linux)

Comment 76

[Nelson Bolyard (seldom reads bugmail)]

I recently did some investigation into stacks showing crashes in NSS while
NSS was not initialized.  I found at least 5 distinct stacks.  See 
bug 470500 comment 24.

Comment 78

[Honza Bambas (:mayhemer)]

The patch is not complete. I didn't realize that we bypass the new code checking the nss component being up because haveLoaded flag is still up. This patch doesn't prevent the crash.

Comment 79

[Chris Ilias [:cilias]]

I'd like to get a user doc created for this, but I'm having trouble understanding the comments on this bug. What is the cause of this (in terms that the user would understand), and is there anything the user can do for now to prevent this crash?

Comment 80

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 81

[Honza Bambas (:mayhemer)]

Attachment: 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 [details] [diff] [review].

Comment 82

[Honza Bambas (:mayhemer)]

Attachment: v2 for 1.8.1

Comment 83

[Nelson Bolyard (seldom reads bugmail)]

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 .

Comment 85

[Mike Beltzner [:beltzner, not reading bugmail]]

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 86

[Samuel Sidler (old account; do not CC)]

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

I'm assuming these two are obsolete...

Comment 87

[Honza Bambas (:mayhemer)]

Comment on attachment 325550 [details] [diff] [review]
derivation from nss shutdown object, 2

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

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

Comment 88

[Samuel Sidler (old account; do not CC)]

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

Comment 89

[Daniel Veditz [:dveditz]]

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

Comment 90

[Kai Engert [:KaiE:]]

Comment on attachment 325550 [details] [diff] [review]
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 91

[Kai Engert [:KaiE:]]

Comment on attachment 387852 [details] [diff] [review]
v2 for trunk, 1.9.1 

r=kaie

Comment 92

[Honza Bambas (:mayhemer)]

Comment on attachment 387852 [details] [diff] [review]
v2 for trunk, 1.9.1 

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

Comment 93

[Honza Bambas (:mayhemer)]

Attachment: v2, as laned on m-c [Checkin comment 92]

Comment 94

[Honza Bambas (:mayhemer)]

Attachment: v1+v2 merged for 1.9.1

attachment 384744 [details] [diff] [review] and attachment 391336 [details] [diff] [review] folded and merged to 1.9.1.

Comment 95

[Honza Bambas (:mayhemer)]

Attachment: v1+v2 merged for 1.9.1 [Checkin comment 101]

This is the correct one...

Comment 96

[Samuel Sidler (old account; do not CC)]

Comment on attachment 391368 [details] [diff] [review]
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.

Comment 97

[Honza Bambas (:mayhemer)]

Attachment: v1+v2 merged for 1.9.0 [Checkin comment 102]

Comment 98

[Honza Bambas (:mayhemer)]

Attachment: v1+v2 merged for 1.8.1

Comment 99

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.1.3, a=dveditz for release-drivers

Comment 100

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.0.14, a=dveditz for release-drivers

Comment 101

[Honza Bambas (:mayhemer)]

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

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

Comment 102

[Honza Bambas (:mayhemer)]

Comment on attachment 391397 [details] [diff] [review]
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

Comment 103

[Jesse Ruderman]

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

Comment 104

[Remko de Keijzer]

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

Comment 105

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 106

[Honza Bambas (:mayhemer)]

Flag clean up.

Comment 107

[Honza Bambas (:mayhemer)]

Comment on attachment 391554 [details] [diff] [review]
v1+v2 merged for 1.8.1

Flag clean up.