This includes the decoupling from CryptoTask from bug 842818 and moves WebCryptoTasks to use a thread pool. Tests are passing. I wasn't sure whether WebCryptoThreadPool should have its own files but it wasn't too much code. Would happily move it out though if you think that's the right call.

Comment on attachment 8559122 [details] [diff] [review] 0001-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask-an.patch >+ NS_DECL_ISUPPORTS Doesn't this need to be NS_DECL_THREADSAFE_ISUPPORTS? Though see below. >+ rv = mPool->SetIdleThreadLimit(5); >+ rv = mPool->SetThreadLimit(5); Why those numbers? Might be worth documenting. >+ nsCOMPtr<nsIThreadPool> mPool; I'd prefer the data member were not intermixed with the member functions... Put it after them? >+ static WebCryptoThreadPool* GetOrCreate() This looks racy to me, in two ways. First, we're checking gInstance outside the mutex locked section. So consider two threads that both invoke WebCryptoThreadPool::Dispatch before it's ever been set up. They race into GetOrCreate, both see a null gInstance. Then one acquires the lock; the other blocks. The first one completes the function, sets gInstance. Then the second one unblocks and does the same and now you've got two WebCryptoThreadPools around. What you should probably do is recheck gInstance again after taking the lock. The second race is more annoying. Say a worker thread enters this function, checks gShutdown and gInstance, sees gInstance is set, goes to return it, then races with the main thread where the profile-before-change notification fires and the ref is dropped, nulling out gInstance. Is there a reason this can't happen? I'm told not. It might be better to actually keep a strong ref in gInstance, skip the observer service stuff entirely, and just shut things down in nsLayoutStatics::Release... Then you'll only need refcounting on this object, not QI. r- for these bits; I'd like to see an updated patch that fixes the races. >+WebCryptoTask::DispatchWithPromise(Promise* aResultPromise) >+ FailWithError(NS_ERROR_DOM_UNKNOWN_ERR); Shouldn't you also set mEarlyRv here? >+WebCryptoTask::Run() >+ // CryptoTasks have consistent behavior regardless of whether NSS is shut WebCryptoTasks? The rest looks fine, assuming ReleaseNSSResources() is OK to call from worker threads.

Oh, and thank you for picking this up!

(In reply to Boris Zbarsky [:bz] from comment #2) > It might be better to actually keep a strong ref in gInstance, skip the > observer service stuff entirely, and just shut things down in > nsLayoutStatics::Release... Then you'll only need refcounting on this > object, not QI. Tried this and the problem seems to be that |nsThreadManager::get()->Shutdown()| is called before |nsLayoutStatistics::Shutdown()|. So we hang when calling the former on shutdown. Would listening for "xpcom-shutdown-threads" be the right thing like e.g. image/src/DecodePool.cpp does?

(In reply to Tim Taubert [:ttaubert] from comment #4) > Tried this and the problem seems to be that > |nsThreadManager::get()->Shutdown()| is called before > |nsLayoutStatistics::Shutdown()|. Meant to write nsLayoutStatics.

> Would listening for "xpcom-shutdown-threads" be the right thing I _think_ so. Ben, are we guaranteed this will run after workers have all shut down?

(In reply to Boris Zbarsky [:bz] from comment #2) > Doesn't this need to be NS_DECL_THREADSAFE_ISUPPORTS? Yeah, it does. Especially with the strong ref in gInstance now. > >+ rv = mPool->SetIdleThreadLimit(5); > >+ rv = mPool->SetThreadLimit(5); > > Why those numbers? Might be worth documenting. TBH, I'm not sure how to document this. I have no numbers backing it up but 5 threads sounds okay, especially if they're shut down after 30s idle time. I didn't see any better justification in other parts of the code that used similar parameters :/ > First, we're checking gInstance outside the mutex locked section. So > consider two threads that both invoke WebCryptoThreadPool::Dispatch before > it's ever been set up. They race into GetOrCreate, both see a null > gInstance. Then one acquires the lock; the other blocks. The first one > completes the function, sets gInstance. Then the second one unblocks and > does the same and now you've got two WebCryptoThreadPools around. What you > should probably do is recheck gInstance again after taking the lock. Good catch, thank you. > The second race is more annoying. Say a worker thread enters this function, > checks gShutdown and gInstance, sees gInstance is set, goes to return it, > then races with the main thread where the profile-before-change notification > fires and the ref is dropped, nulling out gInstance. Is there a reason this > can't happen? I'm told not. The new Observe() implementation should cover this, I hope. Threading is complicated *sigh*. > It might be better to actually keep a strong ref in gInstance, skip the > observer service stuff entirely, and just shut things down in > nsLayoutStatics::Release... Then you'll only need refcounting on this > object, not QI. Keeping a strong reference in gInstance now and listening for "xpcom-shutdown-threads" to shut down the thread pool in time. > The rest looks fine, assuming ReleaseNSSResources() is OK to call from > worker threads. The WebCrypto API uses ReleaseNSSResources() to dispose public and private keys (ScopedSECKEYPrivateKey) only. They have been allocated on the original thread (main or worker) so disposing them from the same should be fine?

> I have no numbers backing it up but 5 threads sounds okay OK. Maybe at least document why we don't want the default values? > Threading is complicated Yeah, indeed. :(

(In reply to Boris Zbarsky [:bz] from comment #8) > > I have no numbers backing it up but 5 threads sounds okay > > OK. Maybe at least document why we don't want the default values? I should have checked, I didn't know there are default values for that, sorry. Actually I'm really fine with using those. Just imagine the patch wouldn't set custom values when reviewing :)

Comment on attachment 8560437 [details] [diff] [review] 0001-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask-an.patch, v2 OK, so I asked around, and worker code can run until workers::RuntimeService::Cleanup, which is called off an xpcom-shutdown-threads observer. So it's only safe to assume worker code won't run in an xpcom-shutdown-threads observer if the workers::RuntimeService has been notified already. And the RuntimeService thing is only created if a worker is created. Oh, also, the observer service can only be used on the main thread, which is not what this patch is doing afaict. Seems to me like creation of a workers::RuntimeService (on the main thread) should set some boolean in WebCryptoThreadPool that makes it ignore the xpcom-shutdown-threads notification and that workers::RuntimeService should call some sort of shutdown function on WebCryptoThreadPool after it's cleaned up the workers. And that we should only register for "xpcom-shutdown-threads" if we're initially created on the main thread and the observer should no-op if the abovementioned boolean is set, otherwise call that same shutdown function. Or some other equivalent that will ensure that we're shut down even if no workers ever got created but that shutdown happens after the worker stuff if they did get created.

Thanks for your explanations. I must admit that after thinking about this more, it does sound rather complicated for the simple task of shutting down the thread pool. The RuntimeService shouldn't really need to know about the WebCryptoThreadPool ideally... > if (observerService) > observerService->NotifyObservers(nullptr, > NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID, > nullptr); > > gXPCOMThreadsShutDown = true; > NS_ProcessPendingEvents(thread); > > // Shutdown the timer thread and all timers that might still be alive before > // shutting down the component manager > nsTimerImpl::Shutdown(); > > NS_ProcessPendingEvents(thread); > > // Shutdown all remaining threads. This method does not return until > // all threads created using the thread manager (with the exception of > // the main thread) have exited. > nsThreadManager::get()->Shutdown(); There are two situations as you described above: 1) We have no workers and no instance of the RuntimeService. Using the "xpcom-shutdown-threads" notification to shut down the thread pool is fine and happens before the nsThreadManager as required. 2) We do have workers and an instance of the RuntimeService. Using the "xpcom-shutdown-threads" notification here means we could possibly shutdown our thread pool before RuntimeService::Cleanup() was called. It's clearly this case which is problematic. I think we have a few options here: 2a) Implement the solution proposed by you in comment #10. 2b) Send another notification right after "xpcom-shutdown-threads" that the WebCryptoThreadPool would then listen for. 2c) Directly call WebCryptoThreadPool::Shutdown() right after sending "xpcom-shutdown-threads". XPCOM should probably not need to know about our thread pool though. OTOH we wouldn't have to jump through any hoops to call AddObserver() on the main thread when a worker created the thread pool. 2d) Dispatch a runnable to the main thread when receiving "xpcom-shutdown-threads" that would then shut down the thread pool. This should probably work due to the |NS_ProcessPendingEvents(thread)| calls shown above, I assume this wouldn't be very safe to do though in case some "xpcom-shutdown-threads" observer processes events as well. None of these sound particularly great to me but with 2b-d we could avoid having to special case when the worker RuntimeService was instantiated. What do you think?

> The RuntimeService shouldn't really need to know about the WebCryptoThreadPool ideally So the fundamental problem is that anything workers rely on needs to not shut down until after RuntimeService is gone. Which means we need ways to check whether it's gone and if not wait for it to be gone, in general. Or hardcode things into RuntimeService to notify stuff when it's gone. > 2b) Send another notification right after "xpcom-shutdown-threads" that the WebCryptoThreadPool would then listen for. I doubt you can get the XPCOM module owners to agree to that, since it just gets into the "everyone tries to listen for a later notification than everyone else" whack-a-mole... > XPCOM should probably not need to know about our thread pool though. Yes, it's a lot better for workers to know about it than for XPCOM to. > I assume this wouldn't be very safe to do though in case some "xpcom-shutdown-threads" > observer processes events as well. And also I'm not sure we should depend on the exact structure of the XPCOM shutdown sequence here... So how about: 2e) We add a way to check whether any workers might still be running and if so a way to get notified when they're not (as in, when the workers::RuntimeService gets to Cleanup).

Talked about this with bz on irc today, I think we're in agreement that adding a 'dom-workers-shutdown' notification is a good idea. We'll make sure that it is always called at shutdown, regardless of whether or not anyone ever created a worker (and therefore created the RuntimeService). We'll also add some way of querying (from the main thread) whether or not this notification has already fired. Any crypto code that runs on a worker should be careful to hold the worker alive while it's doing async work. This includes keeping the worker alive while crypto code registers a listener for the new shutdown notification.

Turns out that RuntimeService::Cleanup() already sends a "web-workers-shutdown" notification. We can ensure this is always sent by just creating a RuntimeService instance on startup. I thought about only partially initializing it until it's actually used but it seems like the overhead is negligible and would just complicate the code? If we initialize the WebCryptoThreadPool like that too then we wouldn't need a method to check whether the notification was already sent. We can call AddObserver() on the main thread and just wait for workers to shut down. The two other patches I posted disentangle a few dependencies and allow including RuntimeService.h from outside dom/workers/.

I'm sorry for the lag here; I'm going to try to get to this tomorrow...

Comment on attachment 8563562 [details] [diff] [review] 0003-Bug-1001691-Always-create-a-RuntimeService.patch Hmm. So this mostly seems reasonable. In particular, GetOrCreateService doesn't start any threads or anything like that... The one problem I see is that workers::RuntimeService::Init reads some prefs that it doesn't seem to set up pref observers for (at least PREF_WORKERS_MAX_PER_DOMAIN). This means that calling it before the user prefs are read is not so great as things stand. We would probably need to add some pref observers or something. Sorry this took so long to get to. :(

In order to be able to include RuntimeService.h from nsLayoutStatics.cpp we need to move some things out of WorkerPrivate.h/cpp so that RuntimeService.h doesn't have to include WorkerPrivate.h. This patch moves WorkerPrivate::LoadInfo to WorkerLoadInfo defined in Workers.h. WorkerPrivate::InterfaceRequestor is moved to WorkerLoadInfo::InterfaceRequestor.

Comment on attachment 8563556 [details] [diff] [review] 0002-Bug-1001691-Move-WorkerType-out-of-WorkerPrivate.h.patch By moving the WorkerType enum out of WorkerPrivate.h we can now stop including it in RuntimeService.h.

Comment on attachment 8568662 [details] [diff] [review] 0001-Bug-1001691-WorkerPrivate-LoadInfo-WorkerLoadInfo.patch Needs review from someone who knows workers.

What actually improves the threading model? This looks like we're just moving code around.

That patch would presumably go on top of these... See the obsolete attachments.

(In reply to Not doing reviews right now from comment #23) > That patch would presumably go on top of these... See the obsolete > attachments. Yes, exactly. Patch 0003 needs to include RuntimeService.h to create it early to be notified about shutting down. On top of that I would continue improving attachment 8560437 [details] [diff] [review]. Just wanted to get the prerequisites reviewed earlier, and maybe landed so that they don't bitrot any further.

Comment on attachment 8568662 [details] [diff] [review] 0001-Bug-1001691-WorkerPrivate-LoadInfo-WorkerLoadInfo.patch Review of attachment 8568662 [details] [diff] [review]: ----------------------------------------------------------------- I'm mostly assuming that you copied things around correctly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea14f386df2 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e627eff6a6

Backed out for build failures :/ https://hg.mozilla.org/integration/mozilla-inbound/rev/071375dda3ea https://hg.mozilla.org/integration/mozilla-inbound/rev/53896c9e2e58

Had to replace a forward declaration with an actual #include. Need to get used to use debug builds for platform stuff, sorry... https://hg.mozilla.org/integration/mozilla-inbound/rev/a48fb48db853 https://hg.mozilla.org/integration/mozilla-inbound/rev/90a444476dcd

https://hg.mozilla.org/mozilla-central/rev/a48fb48db853 https://hg.mozilla.org/mozilla-central/rev/90a444476dcd

Comment on attachment 8563562 [details] [diff] [review] 0003-Bug-1001691-Always-create-a-RuntimeService.patch Review of attachment 8563562 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/build/nsLayoutStatics.cpp @@ +305,5 @@ > #ifdef MOZ_B2G > RequestSyncWifiService::Init(); > #endif > > + mozilla::dom::workers::RuntimeService::GetOrCreateService(); I think this is too early, RuntimeService::Init() does preferences stuff, and I think this comes before we've loaded a profile?

Any movement here? This seems to have stalled.

Sorry for the late response, I'll pick this up very soon again.

This patch introduces the WebCryptoThreadPool: 1) It's initialized very early by nsLayoutStatics so that we always see the xpcom-shutdown-threads notification. 2) The nsIThreadPool itself is created lazily, i.e. when the first async WebCrypto method is called. 3) RuntimeService::Cleanup() is called by xpcom-shutdown-threads, so we might still have workers active on shutdown for a tiny while after the WebCryptoThreadPool was shut down. If we simply make Dispatch() fail in that case we can get rid of a lot of complexity, after all calling an async WebCrypto method on shutdown doesn't make a lot of sense anyway.

This patch decouples WebCryptoTask and CryptoTask, and lets the WebCrypto API use the new thread pool.

Tim, thank you for picking this up! I'm not going to get to this review until at least Tuesday, unfortunately, and more likely Wednesday.

(In reply to Tim Taubert [:ttaubert] from comment #33) > 3) RuntimeService::Cleanup() is called by xpcom-shutdown-threads, so we > might still have workers active on shutdown for a tiny while after the > WebCryptoThreadPool was shut down. If we simply make Dispatch() fail in that > case we can get rid of a lot of complexity, after all calling an async > WebCrypto method on shutdown doesn't make a lot of sense anyway. Can you use the web-workers-shutdown notification here?

(In reply to Boris Zbarsky [:bz] from comment #35) > Tim, thank you for picking this up! I'm not going to get to this review > until at least Tuesday, unfortunately, and more likely Wednesday. No worries, I'll have to shake out a few things before anyway... This probably is more a feedback request now than a review :)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (UTC+8 and delayed responses until 9/21) from comment #36) > Can you use the web-workers-shutdown notification here? This notification is sent by RuntimeService::Shutdown(), which in turns is called by the xpcom-shutdown observer. So I can use that although it would notify even earlier than what we have now. Might not make a huge difference though as it's still the shutdown path. Do you only care about the semantics here or is there any other advantage of using that notification?

I was just wondering if it would make your life easier.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (UTC+8 and delayed responses until 9/21) from comment #39) > I was just wondering if it would make your life easier. I just remembered that it would make things actually slightly more complicated as the RuntimeService is created lazily. The web-workers-shutdown notification might not be sent at all if there's no RuntimeService and I'd end up listening for both notifications.

Blocked by bug 1152046 for a few days now unfortunately. There's a race condition I can only reproduce in my Linux VM, have to wait until I can run tests there again :( Or maybe I'll just work with a local backout, but it took me quite some time to figure out what's at fault.

(In reply to Tim Taubert [:ttaubert] from comment #41) > Blocked by bug 1152046 for a few days now unfortunately. There's a race > condition I can only reproduce in my Linux VM, have to wait until I can run > tests there again :( Or maybe I'll just work with a local backout, but it > took me quite some time to figure out what's at fault. Can you tell me how to reproduce it or at least I want to try to reproduce it? or you can tell me if you know what is it happening.

(In reply to Dragana Damjanovic [:dragana] from comment #42) > Can you tell me how to reproduce it or at least I want to try to reproduce > it? or you can tell me if you know what is it happening. Just trying to run './mach mochitest dom/crypto/' never starts the HTTP server in my Linux VM. The xpcshell binary segfaults in jemalloc.c:1620. Happy to help on IRC now or later if you want help fixing this!

(In reply to Tim Taubert [:ttaubert] from comment #43) > (In reply to Dragana Damjanovic [:dragana] from comment #42) > > Can you tell me how to reproduce it or at least I want to try to reproduce > > it? or you can tell me if you know what is it happening. > > Just trying to run './mach mochitest dom/crypto/' never starts the HTTP > server in my Linux VM. The xpcshell binary segfaults in jemalloc.c:1620. > Happy to help on IRC now or later if you want help fixing this! There was a problem reported in bug 1205016. Can you check if you have that patch in and if not if that solves your problem. Thanks.

(In reply to Dragana Damjanovic [:dragana] from comment #44) > There was a problem reported in bug 1205016. > Can you check if you have that patch in and if not if that solves your > problem. It does, thank you.

(In reply to Tim Taubert [:ttaubert] from comment #41) > There's a race condition I can only reproduce in my Linux VM Figured it out, will attach new patches soon.

Had to turn GenerateAsymmetricKeyTask::mKeyPair into an nsAutoPtr as it was crashing sometimes due to a race condition. The CryptoKey expects to be destroyed on the same thread as it was created. Sometimes, the thread pool held onto the CryptoTask longer than the main thread (reproducible with a single core on my Linux VM) and so ~WebCryptoTask() and ~CryptoKey() were called off the main thread. By using an nsAutoPtr and nulling it in WebCryptoTask::Cleanup() we can ensure the CryptoKey is destroyed on the right thread.

The thread pool works fine with the worker tests from bug 842818, except a few issues that are specific to workers and should be solved over in the other bug.

Comment on attachment 8664162 [details] [diff] [review] 0005-Bug-1001691-Make-GenerateAsymmetricKeyTask-mKeyPair-.patch Review of attachment 8664162 [details] [diff] [review]: ----------------------------------------------------------------- I feel pretty confident about reviewing this part if bz wants to save a tiny amount of time. ::: dom/crypto/WebCryptoTask.cpp @@ +2442,5 @@ > if (!mPrivateKey.get() || !mPublicKey.get()) { > return NS_ERROR_DOM_UNKNOWN_ERR; > } > > + mKeyPair->mPrivateKey.get()->SetPrivateKey(mPrivateKey); You should add MOZ_ASSERT(mKeyPair) to the start of DoCrypto() ::: dom/crypto/WebCryptoTask.h @@ +232,5 @@ > const ObjectOrString& aAlgorithm, bool aExtractable, > const Sequence<nsString>& aKeyUsages); > protected: > ScopedPLArenaPool mArena; > + nsAutoPtr<CryptoKeyPair> mKeyPair; I think that UniquePtr might be a better choice for this. You would then use .forget() when resolving.

(In reply to Martin Thomson [:mt:] from comment #51) > I feel pretty confident about reviewing this part if bz wants to save a tiny > amount of time. Thank you! > > ScopedPLArenaPool mArena; > > + nsAutoPtr<CryptoKeyPair> mKeyPair; > > I think that UniquePtr might be a better choice for this. You would then > use .forget() when resolving. Yeah, wondered if I should use that too. I think that's indeed better.

r=mt (In reply to Martin Thomson [:mt:] from comment #51) > I think that UniquePtr might be a better choice for this. You would then > use .forget() when resolving. UniquePtr doesn't have anything like .forget() - .release() would return the pointer but also leak it. So I stuck with calling Resolve(*mKeyPair) and nulling it in Cleanup().

Comment on attachment 8664160 [details] [diff] [review] 0003-Bug-1001691-Implement-WebCrypto-thread-pool.patch, v2 >+++ b/dom/crypto/WebCryptoThreadPool.cpp >+#include "WebCryptoThreadPool.h" Since it's exported, probably better to include as mozilla/dom/WebCryptoThreadPool.h. >+nsRefPtr<WebCryptoThreadPool> gInstance = nullptr; StaticRefPtr, please, to avoid the static constructor. But more on this below. >+WebCryptoThreadPool::DispatchInternal(nsIRunnable* aRunnable) >+ if (!mPool) { >+ // May be called from worker threads. >+ MutexAutoLock lock(mMutex); >+ >+ if (!mPool) { This seems fishy to me. Why are unsynchronized/unfenced reads of mPool ok here? https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_C.2B.2B11 suggests you need memory fences here... >+WebCryptoThreadPool::Shutdown() Should this be asserting mainthread? I guess the only caller does that... r=me modulo that DispatchInternal issue, which needs either comments explaining why it's safe or something.

Comment on attachment 8664161 [details] [diff] [review] 0004-Bug-1001691-Use-thread-pool-for-WebCrypto-operations.patch, v2 >+++ b/dom/crypto/WebCryptoTask.cpp > CloneData(JSContext* aCx, CryptoBuffer& aDst, JS::Handle<JSObject*> aSrc) >- MOZ_ASSERT(NS_IsMainThread()); Is this strictly needed as part of this patch, or just laying groundwork for webcrypto in workers? >+WebCryptoTask::DispatchWithPromise(Promise* aResultPromise) And same question for the assert that went away here. r=me

r=bz (In reply to Boris Zbarsky [:bz] from comment #55) > >+++ b/dom/crypto/WebCryptoTask.cpp > > CloneData(JSContext* aCx, CryptoBuffer& aDst, JS::Handle<JSObject*> aSrc) > >- MOZ_ASSERT(NS_IsMainThread()); > > Is this strictly needed as part of this patch, or just laying groundwork for > webcrypto in workers? The latter, we can leave it in for now. > >+WebCryptoTask::DispatchWithPromise(Promise* aResultPromise) > > And same question for the assert that went away here. Same here, re-added the assertion.

(In reply to Boris Zbarsky [:bz] from comment #54) > >+nsRefPtr<WebCryptoThreadPool> gInstance = nullptr; > > StaticRefPtr, please, to avoid the static constructor. Done. > >+WebCryptoThreadPool::DispatchInternal(nsIRunnable* aRunnable) > >+ if (!mPool) { > >+ // May be called from worker threads. > >+ MutexAutoLock lock(mMutex); > >+ > >+ if (!mPool) { > > This seems fishy to me. Why are unsynchronized/unfenced reads of mPool ok > here? > https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_C.2B.2B11 > suggests you need memory fences here... Yeah, this needs memory fences indeed. So what I did is use mozilla::Atomic<T, mozilla::ReleaseAcquire> to store the nsThreadPool instance. I wrapped the Atomic<> in a small class so that it's nicer to use and we can release the reference when mPool goes away. I hope this makes sense, it definitely needs another review. > >+WebCryptoThreadPool::Shutdown() > > Should this be asserting mainthread? I guess the only caller does that... Yeah, why not. Added an assertion.

Comment on attachment 8666702 [details] [diff] [review] 0003-Bug-1001691-Implement-WebCrypto-thread-pool.patch, v3 I _think_ this is good on memory fences now. I wish I understood this atomic/fence stuff better. :( We're assuming that ~AtomicThreadPool can't race with WebCryptoThreadPool::DispatchInternal, but that seems like an ok assumption to me. r=me

Comment on attachment 8666702 [details] [diff] [review] 0003-Bug-1001691-Implement-WebCrypto-thread-pool.patch, v3 Hey Nathan, I think you know a lot about Atomic, right? Can you please take a look and say whether this is sane?

Tim, is this really a case where double-checked locking is needed? I would have thought that this would be fine with a simple mutex. Heavy concurrent crypto on main and worker threads might generate contention, but mutexes aren't that expensive otherwise.

That said, the use of atomics looks correct to me.

Comment on attachment 8666702 [details] [diff] [review] 0003-Bug-1001691-Implement-WebCrypto-thread-pool.patch, v3 Review of attachment 8666702 [details] [diff] [review]: ----------------------------------------------------------------- The atomics usage looks correct here, though I'm inclined to agree with Martin that just acquiring the lock all the time wouldn't be that big of a deal. ::: dom/crypto/WebCryptoThreadPool.h @@ +62,5 @@ > + } > + > + operator bool() const { return mPool; } > + > + const nsIThreadPool* operator=(nsIThreadPool* aValue) I think it'd be better to pass already_AddRefed<nsIThreadPool>&& here, to encapsulate the details of refcounting inside this class.

Tim, could you perhaps put a bit of a comment on the block if you go ahead with the double-checked locking.

(In reply to Martin Thomson [:mt:] from comment #60) > Tim, is this really a case where double-checked locking is needed? I would > have thought that this would be fine with a simple mutex. Heavy concurrent > crypto on main and worker threads might generate contention, but mutexes > aren't that expensive otherwise. (In reply to Nathan Froyd [:froydnj] from comment #62) > The atomics usage looks correct here, though I'm inclined to agree with > Martin that just acquiring the lock all the time wouldn't be that big of a > deal. Thanks to all of you looking at the code! I think I will write into my diary that I managed to correctly implement double-checked locking but it seems like especially in terms of maintenance we'd be better off with a simple mutex. We should indeed not encounter heavy concurrency with different threads running WebCryptoTasks. We can always come back and revisit this decision should that situation ever arise. I'll work on a new version with a simple mutex.

Comment on attachment 8670196 [details] [diff] [review] 0003-Bug-1001691-Implement-WebCrypto-thread-pool-r-bz.patch, v4 r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/6df52c2b5c0f https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f5da761238 https://hg.mozilla.org/integration/mozilla-inbound/rev/773fdab3d54b

https://hg.mozilla.org/mozilla-central/rev/6df52c2b5c0f https://hg.mozilla.org/mozilla-central/rev/f8f5da761238 https://hg.mozilla.org/mozilla-central/rev/773fdab3d54b