Richard Barnes [:rbarnes] Reporter
	Description • 10 years ago

      No description provided.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 1 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 10 years ago

Oh, and thank you for picking this up!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 4 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

> 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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 7 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 11 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 13 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 16 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 19 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 20 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 10 years ago

Comment on attachment 8568662 [details] [diff] [review]
0001-Bug-1001691-WorkerPrivate-LoadInfo-WorkerLoadInfo.patch

Needs review from someone who knows workers.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 22 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 24 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 25 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 26 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 27 • 10 years ago

Backed out for build failures :/

https://hg.mozilla.org/integration/mozilla-inbound/rev/071375dda3ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/53896c9e2e58

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 28 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 10 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 30 • 10 years ago

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?

	Martin Thomson [:mt:]
	Comment 31 • 9 years ago

Any movement here?  This seems to have stalled.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 32 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 33 • 9 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 34 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 9 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 36 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 37 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 38 • 9 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 39 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 40 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 41 • 9 years ago

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.

	Dragana Damjanovic [:dragana]
	Comment 42 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 43 • 9 years ago

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

	Dragana Damjanovic [:dragana]
	Comment 44 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 45 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 46 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 49 • 9 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 50 • 9 years ago

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.

	Martin Thomson [:mt:]
	Comment 51 • 9 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 52 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 53 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 54 • 9 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 56 • 9 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 57 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 59 • 9 years ago

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?

	Martin Thomson [:mt:]
	Comment 60 • 9 years ago

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.

	Martin Thomson [:mt:]
	Comment 61 • 9 years ago

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

	Nathan Froyd [:froydnj]
	Comment 62 • 9 years ago

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.

	Martin Thomson [:mt:]
	Comment 63 • 9 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 64 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 66 • 9 years ago

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

r=me

	Pulsebot
	Comment 67 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 68 • 9 years ago

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