Many (most) certificate and crypto operations need to happen off the main thread. One reason is that asymmetric crypto operations are slow. (Note that many existing PSM APIs do not follow this.) Another reason is that certificate operations may do disk I/O and/or network I/O behind the schemes. When doing async operations, we need to handle race conditions between NSS shutdown and the execution and/or completion of our async operation. The nsNSSShutdownObject interface helps for this, but for any operations, a simpler interface makes more sense.

These are the utility changes needed for B2G signature verification and also BrowserID signature verification. Suggestions welcome on a better place to put the new utilities that I stuffed in ScopedNSSTypes.h.

This version fixes a potential race condition at shutdown. I think that this type of race condition (destructor running on a background thread while NSS is looping through the nsNSSShutDownList on the main thread) is also the cause of the other NSS shutdown crashes.

Comment on attachment 678233 [details] [diff] [review] Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v2] Review of attachment 678233 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/CryptoTask.cpp @@ +23,5 @@ > +{ > + nsCOMPtr<nsIThread> thread; > + nsresult rv = NS_NewThread(getter_AddRefs(thread), this); > + if (thread) { > + NS_SetThreadName(thread, taskThreadName); If there is a chance this can go in parallel, could we somehow use the pool naming here? @@ +64,5 @@ > + "virtualDestroyNSSReference called off the main thread"); > + if (!mReleasedNSSResources) { > + mReleasedNSSResources = true; > + ReleaseNSSResources(); > + } I think you should follow [1]. Adding the shutdown lock to dtor is a nice addition. [1] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSShutDown.h#203 ::: security/manager/ssl/src/CryptoTask.h @@ +29,5 @@ > + * except CalculateResult might be skipped if NSS is shut down before it can > + * be called; in that case ReleaseNSSResources will be called and then > + * CallCallback will be called with an error code. > + */ > +class CryptoTask : public nsRunnable, public nsNSSShutDownObject each on a new line please.

Comment on attachment 678233 [details] [diff] [review] Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v2] Dropping r since I don't think the shutdown implementation is 100% correct.

(In reply to Honza Bambas (:mayhemer) from comment #3) > ::: security/manager/ssl/src/CryptoTask.cpp > @@ +23,5 @@ > > +{ > > + nsCOMPtr<nsIThread> thread; > > + nsresult rv = NS_NewThread(getter_AddRefs(thread), this); > > + if (thread) { > > + NS_SetThreadName(thread, taskThreadName); > > If there is a chance this can go in parallel, could we somehow use the pool > naming here? Presumably, each different type of crypto task should have its own name. But, we'd have to extend the pooling API to support that. Right now, because of time constraints, I think it isn't worth doing that. But, that would not affect this patch anyway; it would affect the patches that use CryptoTask. > > @@ +64,5 @@ > > + "virtualDestroyNSSReference called off the main thread"); > > + if (!mReleasedNSSResources) { > > + mReleasedNSSResources = true; > > + ReleaseNSSResources(); > > + } > > I think you should follow [1]. Adding the shutdown lock to dtor is a nice > addition. The pattern in [1] doesn't make sense for this implementation because CryptoTask always makes sure that the NSS resources are released before the destructor is ever called. (This is asserted in the destructor.) Therefore, we don't need a "destructor-safe" cleanup operation. In fact, this is one of the key reasons why I created this helper class, to reduce the boilerplate that subclasses need to implement compared to normal nsNSSShutDownObject. > [1] > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > nsNSSShutDown.h#203 > > ::: security/manager/ssl/src/CryptoTask.h > @@ +29,5 @@ > > + * except CalculateResult might be skipped if NSS is shut down before it can > > + * be called; in that case ReleaseNSSResources will be called and then > > + * CallCallback will be called with an error code. > > + */ > > +class CryptoTask : public nsRunnable, public nsNSSShutDownObject > > each on a new line please. Fixed.

Comment on attachment 680367 [details] [diff] [review] Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v3] Review of attachment 680367 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab Thanks.

I switched the usage of HASH_* to PK11_* because HASH_* functions not return error codes from all the operations, whereas PK11_* do, so I think the PK11_* functions are safer/better.

Backed out of m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/58b9ad5a8043

This bug has been called out as likely having risk to non-B2G platforms. Given that, marking as P1, and moving into the C2 milestone. We should prioritize this landing to mozilla-beta as soon as possible, to prevent late-breaking regressions to other platforms.

Any updates here, Brian?

I think its unlikely we should take an NSS update at this late date.

(In reply to JP Rosevear [:jpr] from comment #12) > I think its unlikely we should take an NSS update at this late date. Please see bug 816392 comment 0. I have already attached the patch to backport the three *build-time-only* NSS patches to mozilla-beta, so that we will not have to update NSS in mozilla-beta.

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e09d599229c

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f24c48b21e3 https://hg.mozilla.org/releases/mozilla-aurora/rev/36f6c5a2c0d5 https://hg.mozilla.org/releases/mozilla-beta/rev/5c4883456c4e