David Dahl :ddahl Reporter
	Description • 11 years ago

The WorkerCrypto interface is needed for the WebCryptoAPI. The near-term need is to support window.crypto.getRandomValues() in Workers

http://lists.w3.org/Archives/Public/public-webcrypto/2012Oct/0076.html

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#WorkerCrypto-interface

Ryan Sleevi's proposal from the above mail thread:

partial interface WorkerCrypto {
   ArrayBufferView getRandomValues(ArrayBufferView array);
};

partial interface WorkerGlobalScope {
  readonly attribute WorkerCrypto crypto;
};

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 1 • 11 years ago

(In reply to David Dahl :ddahl from comment #0)
> The WorkerCrypto interface is needed for the WebCryptoAPI. The near-term
> need is to support window.crypto.getRandomValues() in Workers
> 
> http://lists.w3.org/Archives/Public/public-webcrypto/2012Oct/0076.html
> 
> https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.
> html#WorkerCrypto-interface
> 
> Ryan Sleevi's proposal from the above mail thread:
> 
> partial interface WorkerCrypto {
>    ArrayBufferView getRandomValues(ArrayBufferView array);
> };
> 
> partial interface WorkerGlobalScope {
>   readonly attribute WorkerCrypto crypto;
> };

This seems very reasonable to me, but should we be prefixing crypto as mozCrypto?

IMO, this should be the next step in Mozilla's implementation of the web crypto API. Unlike some other parts of the W3C proposed API, getRandomValues is uncontroversial (at least, we resolved all the controversy).

Having getRandomValues working in workers, on B2G, on Desktop, and (after this) in extensions sets the stage for rapid development of all the other features of the API and potential contributor involvement because there won't be any outstanding architectural issues to resolve, except for key management.

	David Dahl :ddahl Reporter
	Comment 2 • 11 years ago

(In reply to Brian Smith (:bsmith) from comment #1)
> (In reply to David Dahl :ddahl from comment #0)
> > The WorkerCrypto interface is needed for the WebCryptoAPI. The near-term
> > need is to support window.crypto.getRandomValues() in Workers
> > 
> > http://lists.w3.org/Archives/Public/public-webcrypto/2012Oct/0076.html
> > 
> > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.
> > html#WorkerCrypto-interface
> > 
> > Ryan Sleevi's proposal from the above mail thread:
> > 
> > partial interface WorkerCrypto {
> >    ArrayBufferView getRandomValues(ArrayBufferView array);
> > };
> > 
> > partial interface WorkerGlobalScope {
> >   readonly attribute WorkerCrypto crypto;
> > };
> 
> This seems very reasonable to me, but should we be prefixing crypto as
> mozCrypto?
>
I would think so, then other non-finalized methods can be added without any trouble - taking the other approach, since this object does not exist yet, is it necessary to prefix it? 
 
> IMO, this should be the next step in Mozilla's implementation of the web
> crypto API. Unlike some other parts of the W3C proposed API, getRandomValues
> is uncontroversial (at least, we resolved all the controversy).
> 
Agreed.

> Having getRandomValues working in workers, on B2G, on Desktop, and (after
> this) in extensions sets the stage for rapid development of all the other
> features of the API and potential contributor involvement because there
> won't be any outstanding architectural issues to resolve, except for key
> management.
+1

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 3 • 11 years ago

Given that window.crypto already exists, and the plan is to extend it, I don't think it makes sense to add window.mozCrypto or workerGlobal.mozCrypto.

In general, we're trying to avoid introducing prefixed properties and instead limit "experimental features" to nightly/aurora channels instead.

So instead add the new API on window.crypto and workerGlobal.crypto and if you are worried that it's too early to release the API into the wild, make it pref-off-able and disable it on the beta/release branches (I think we have tools to make that automatic)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 11 years ago

Though note that

* If you feel pretty confident that the API is right, and I think having discussed the API with other browser vendors as well as developers and gotten positive feedback is a requirement for that.
* And are prepared to iterate on the API quickly, i.e. have the time to quickly act on feedback to the API, both in spec and in implementation.

then I think it would be ok to let the API ride all the way to release. Though having it be pref-offable might still be a good idea in case some big issues (like security issues or known API problems) come up late in the game.

	David Dahl :ddahl Reporter
	Comment 5 • 11 years ago

I see here: http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp#384 

That we add the navigator object to each Worker by creating a navigator object inside the GetNavigator getter.

In the case of a Crypto object, we could do the same thing, but we will end up with all of the legacy properties as well.

Can we use an instance of nsIDOMCrypto, which only has the getRandomValues property on it?
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMCrypto.idl#9

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 11 years ago

We don't use the normal main-thread nsIDOMNavigator object on the worker thread. It's very unlikely that the nsIDOMNavigator object is fully threadsafe. Also we only want to expose a very limited number of properties on the navigator object in workers.

http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#the-workernavigator-object

So you should be able to use the same approach for crypto.

	David Dahl :ddahl Reporter
	Comment 7 • 11 years ago

Ms2Ger: I almost have this thing building, some of the JS api stuff I know nothing about is confusing to me... Inside of WorkerCrypto.cpp I think I need a method like:   

static JSBool
WorkerGetRandomValues(JSContext* aCx, unsigned aArgc, jsval* aVp)

in order to make the function appear in the WorkerScope - any pointers there would be helpful, I was cribbing off of dom/workers/Navigator.cpp

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 11 years ago

Comment on attachment 724202 [details] [diff] [review]
Patch - WIP

Review of attachment 724202 [details] [diff] [review]:
-----------------------------------------------------------------

I'm afraid you chose the wrong implementation to crib off; look at TextEn/Decoder or XHR for something that uses WebIDL. That should end up being a lot simpler.

	David Dahl :ddahl Reporter
	Comment 9 • 11 years ago

(In reply to :Ms2ger from comment #8)

> I'm afraid you chose the wrong implementation to crib off; look at
> TextEn/Decoder or XHR for something that uses WebIDL. That should end up
> being a lot simpler.


Thanks! That does look much simpler. Where are the IDL files for these implementations?

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

dom/webidl/

	Tantek Çelik
	Comment 11 • 11 years ago

Is there a master bug for "implement WebCryptoAPI http://www.w3.org/TR/WebCryptoAPI/ " that depends on this bug? I searched and couldn't find it and figured I'd ask before creating.

	(no longer active)
	Comment 12 • 11 years ago

(In reply to comment #11)
> Is there a master bug for "implement WebCryptoAPI
> http://www.w3.org/TR/WebCryptoAPI/ " that depends on this bug? I searched and
> couldn't find it and figured I'd ask before creating.

Bug 865789.

	Andrew Sutherland [:asuth] (he/him)
	Comment 13 • 11 years ago

Is it easier to fix this bug now that bug 883741 (WebCrypto: Move Crypto to WebIDL) has landed?  My interest is that the Gaia E-mail app does almost all of its work on the worker thread.  While we are not going to land any crypto stuff (meta/discussion bug is bug 894817) for quite a while, there is community interest in prototyping and it would be very helpful if getRandomValues were available on the worker.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

I expect this is easier, yes, though the implementation will need to be taught to work off-thread, of course.  Kyle?

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

(In reply to Boris Zbarsky [:bz] from comment #14)
> I expect this is easier, yes, though the implementation will need to be
> taught to work off-thread, of course.  Kyle?

It might be mildly complicated on non-b2g.  Right now we have:

interface Crypto {
};

and

[NoInterfaceObject]
interface RandomSource {
  [Throws]
  ArrayBufferView getRandomValues(ArrayBufferView array);
};

Crypto implements RandomSource;

for the standards track stuff and

[NoInterfaceObject]
interface CryptoLegacy {
  // legacy crap
};

Crypto implements CryptoLegacy;

for the ancient stuff.  On workers we would want just the RandomSource bits, and not the CryptoLegacy bits.  How to teach the parser that we only want implements to apply to the main thread implementation ...

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 11 years ago

We could just not expose the legacy stuff to worker JS by flagging them all individually with a [Func] annotation that checks for mainthread, no?

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

Yeah, I guess so.  Might even be worth making a shorthand that apply it to an entire interface (or an implements statement, or something).  We'll need to do similar things to convert location and navigator to WebIDL in workers.

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

Actually, we won't.  Per spec, for example, .location in a worker is a WorkerLocation.  That implements URLUtilsReadOnly.  On mainthread, .location is a Location.  That implements URLUtils.

Similarly for navigator: the worker one is a WorkerNavigator, which implements some other interfaces, but not all the ones Navigator implements.

The only reason Crypto is a problem is that we don't want to bake the distinction between the worker and mainthread version into the toString on the objects by naming the worker-side interface something different, basically...

	David Dahl :ddahl Reporter
	Comment 19 • 11 years ago

Trying to take another stab at this. Wondering if  'mozilla/dom/WorkerCryptoBinding.h' is something that will be auto-generated:

In file included from /home/ddahl/code/moz/incoming/src/dom/workers/Crypto.cpp:11:
 0:51.24 /home/ddahl/code/moz/incoming/src/dom/workers/DOMBindingInlines.h:14:10: fatal error: 'mozilla/dom/WorkerCryptoBinding.h' file not found
 0:51.24 #include "mozilla/dom/WorkerCryptoBinding.h"
 0:51.24          ^
 0:51.31 1 error generated.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 11 years ago

Yes, WorkerCryptoBinding will be auto-generated when you add your .webidl file to WebIDL.mk.

Note that there is already a definition of RandomSource, so you don't need to define it again (and in fact, attempting to define it twice would fail IDL parsing).

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 21 • 11 years ago

Comment on attachment 796294 [details] [diff] [review]
WIP Patch - take 2

Review of attachment 796294 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to add the header to dom/webidl/WebIDL.mk. Kyle knows more about the workers bindings, though.

	David Dahl :ddahl Reporter
	Comment 22 • 11 years ago

OK, this patch creates a crypto object inside workers with a getRandomValues noop method hanging off of it. The tests have also been stubbed out. 

Spoke with khuey and bz earlier about how to proxy the original dom method through to the worker. I am studying /dom/workers/URL.cpp as a 'model' to continue.

<ddahl_> bz: I was trying to make the existing getRandomValues code work inside the worker as an experiment, sounds like this is not going to work this way
<khuey> ddahl_: the usual way to do this is to have the worker impl proxy to the mainthread impl
<ddahl_> khuey: is there an example of a proxy to learn from?
<khuey> ddahl_: URL

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 11 years ago

If the API is synchronous, we should always avoid proxying to the main thread if possible. The proxying definitely adds a lot of performance overhead.

However if the backend that you need to get to, in this case I'm guessing something in NSS, only works on the main thread, then you don't really have much of a choice.

But if you can call the relevant NSS functions from the worker thread, then that's definitely preferable to proxying.

URL was proxied just because nsIURI only works on the main thread.

	David Dahl :ddahl Reporter
	Comment 24 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #23)
> However if the backend that you need to get to, in this case I'm guessing
> something in NSS, only works on the main thread, then you don't really have
> much of a choice.
> 
> But if you can call the relevant NSS functions from the worker thread, then
> that's definitely preferable to proxying.
Yeah, we can only call nsIRandomGenerator from the main thread. There is no way to use ipc from workers, huh?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 25 • 11 years ago

i*p*c? Isn't there only one process involved here? Do you mean inter-thread-communication?

It's definitely possible to use inter-thread-communication in workers. Though you should talk to bent about how to do it.

	David Dahl :ddahl Reporter
	Comment 26 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #25)
> i*p*c? Isn't there only one process involved here? Do you mean
> inter-thread-communication?
Sorry, I was referring to they way we handle getRandomValues now on b2g: https://mxr.mozilla.org/mozilla-central/source/dom/base/Crypto.cpp#90

> It's definitely possible to use inter-thread-communication in workers.
> Though you should talk to bent about how to do it.
Not sure which is worse to deal with:)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 27 • 11 years ago

Comment on attachment 796917 [details] [diff] [review]
WorkerCrypto2

Review of attachment 796917 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I know enough about the worker bindings to comment usefully here.

	David Dahl :ddahl Reporter
	Comment 28 • 11 years ago

This patch provides a crypto object to workers and attempts to use the same kind of proxy used in URL. This patch still needs to include dom/base/Crypto.h somehow to use our existing GetRandomValues - or - do we just go ahead and basically copy most of the GetRandomValues implementation (use of nsRandomGenerator, etc) into the RandomValuesRunnable?

I am unsure what the Worker's crypto.GetRandomValues implementation's signature should be, naturally, the spec and webidl says it returns an ArrayBufferView.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 11 years ago

> I am unsure what the Worker's crypto.GetRandomValues implementation's signature should be

Run "make -C $objdir/dom/bindings/ WorkerCrypto-example" and then take a look at $objdir/dom/bindings/WorkerCrypto-example.h.  So far for workers it can be a bit off in some cases, but I suspect in this case it will be correct.

	David Dahl :ddahl Reporter
	Comment 30 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #29)
> > I am unsure what the Worker's crypto.GetRandomValues implementation's signature should be
> 
> Run "make -C $objdir/dom/bindings/ WorkerCrypto-example" and then take a
> look at $objdir/dom/bindings/WorkerCrypto-example.h.

When I do this with WorkerCrypto, the bindings generation fails - also WorkerLocation, WorkerNavigator fail the same way: http://pastebin.mozilla.org/2940574
The Crypto interface has getRandomValues as:   

JSObject* GetRandomValues(JSContext* cx, const ArrayBufferView& array, ErrorResult& aRv);

Using URL as an example, I see there is also a GlobalObject argument that is used inside of the WorkerPrivate:
  static void CreateObjectURL(const GlobalObject& global, nsIDOMBlob* blob, const objectURLOptions& options, nsString& retval, ErrorResult& aRv);

I am wondering if the WorkerCrypto signature will also require the GlobalObject arg.

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 11 years ago

Give that a shot?

	David Dahl :ddahl Reporter
	Comment 32 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #31)
> Created attachment 797865 [details] [diff] [review]
> Fix example codegen to work with worker-only bindings
> 
> Give that a shot?

Thank you, that worked:
  JSObject* GetRandomValues(JSContext* cx, const ArrayBufferView& array, ErrorResult& aRv);

Looks like my WorkerCrypto arguments are correct. 

Since we have access to the window object inside the runnable, can we just call JSObject* randomValues = Crypto->GetRandomValues(aContext, aArray, aRv) directly? That seems like the DRY approach. Perhaps that is a naive approach?

I tried to #include /mozilla/dom/Crypto.h to instanciate an nsIDOMCrypto object, but was unable to successfully include it. This made me think that I should not be trying to #include that inside dom/workers/Crypto.cpp

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 11 years ago

> Perhaps that is a naive approach?

Unfortunately, yes, if aArray actually represents a worker-side JS object, since we can't have the main thread operating on it directly.

What you want to proxy to the main thread is not the whole API call, but just the "here is a memory buffer, write to it" bit or a "hand me some random data".  The extraction of the memory buffer should happen worker-side.  So basically, you want to get the buffer (ideally sharing code with Crypto::GetRandomValues(JSContext*, const ArrayBufferView&, ErrorResult&) in the process) and then post a runnable that just has the uint8_t*... or have the runnable return a uint8_t* to the worker (so the runnable would just call the Crypto::GetRandomValues(uint32_t) static method on the UI thread).

> but was unable to successfully include it.

LOCAL_INCLUDES issues?

	David Dahl :ddahl Reporter
	Comment 34 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #33)
> ... or have the runnable return a uint8_t* to the worker (so the
> runnable would just call the Crypto::GetRandomValues(uint32_t) static method
> on the UI thread).

OK. Thanks for the guidance, I am getting closer. How is the uint8_t* data returned to the worker thread? I am passing it to the runnable's constructor, the runnable is calling GetRandomValues(uint32_t) on the main thread, just not sure how to make sure it gets back to the worker.

Is there some way to do this via the ResponseRunnable object?

	David Dahl :ddahl Reporter
	Comment 35 • 11 years ago

This patch attempts to generate random values via the runnable, not sure how to get the random data back to the worker.

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

Comment on attachment 798062 [details] [diff] [review]
Latest WIP

Review of attachment 798062 [details] [diff] [review]:
-----------------------------------------------------------------

In general 'workers': True bindings are something we're trying not to add more of.  If you remove that you'll get stuff that looks more familiar (no inheritance from DOMBindingBase, no DOMBindingInlines.h entry, no trace/finalize hooks, etc).

Please name your files WorkerCrypto.h/cpp

::: dom/workers/Crypto.cpp
@@ +20,5 @@
> +#include "nsPIDOMWindow.h"
> +#include "nsGlobalWindow.h"
> +
> +#include "nsIDocument.h"
> +#include "nsIRandomGenerator.h" // XXXddahl:  adding static GetRandomValues method below until #includes are sorted out

You already included this above.

@@ +28,5 @@
> +BEGIN_WORKERS_NAMESPACE
> +
> +using mozilla::dom::GlobalObject;
> +
> +#include "mozilla/dom/TypedArray.h"

Don't put includes after namespace declarations o.O

@@ +86,5 @@
> +  bool
> +  Dispatch(JSContext* aCx)
> +  {
> +    mWorkerPrivate->AssertIsOnWorkerThread();
> +    mSyncQueueKey = mWorkerPrivate->CreateNewSyncLoop();

Please use AutoSyncLoopHolder.  It'll help ensure you don't fuck things up if dispatching fails.

@@ +126,5 @@
> +  uint8_t* mRandomValues;
> +  uint32_t mLength;
> +
> +public:
> +  RandomValuesRunnable(WorkerPrivate* aWorkerPrivate, 

nit: whitespace at EOL

@@ +146,5 @@
> +
> +    nsCOMPtr<nsIPrincipal> principal;
> +    nsIDocument* doc = nullptr;
> +
> +    nsCOMPtr<nsPIDOMWindow> window = mWorkerPrivate->GetWindow();

Not every worker will have a window (e.g. workers created by chrome JSMs/components and soon content shared workers).

@@ +156,5 @@
> +
> +      principal = doc->NodePrincipal();
> +    } else {
> +      MOZ_ASSERT_IF(!mWorkerPrivate->GetParent(), mWorkerPrivate->IsChromeWorker());
> +      principal = mWorkerPrivate->GetPrincipal();

You should just use mWorkerPrivate->GetPrincipal() unconditionally unless you have a really good reason.

@@ +208,5 @@
> +  // NS_ABORT_IF_FALSE(NS_IsMainThread(), "Called on the wrong thread");
> +  JS::Rooted<JSObject*> view(aCx, aArray.Obj());
> +
> +  // Throw if the wrong type of ArrayBufferView is passed in
> +  // (Part of the Web Crypto API spec)

This is unfortunate.  It's really something that should be handled at the WebIDL layer.

	David Dahl :ddahl Reporter
	Comment 37 • 11 years ago

Still need to re-name Crypto.h/.cpp to WorkerCrypto.h/cpp

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

I'm going to assume that ddahl isn't actively working on this.

	Anne (:annevk)
	Comment 40 • 10 years ago

The IDL in the specification changed a bit. Due to us now supporting [Exposed] this should also be easier IDL-wise.

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

Exposing the API with the latest IDL changes is indeed quite easy.

> -[NoInterfaceObject]
> +[NoInterfaceObject, Exposed=(Window,Worker)]
>  interface GlobalCrypto {

Our codegen complains without those changes. Not sure if we need to fix that or the spec?

> -[Pref="dom.webcrypto.enabled"]
> +[Exposed=(Window,Worker)]

We can't do that anymore when exposing to Workers unfortunately. It feels like we could probably get rid of the pref by now though?

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

It's probably a good idea to run all of the existing tests in Workers as well, so here's a patch for that. It works well but needs some cleanup. It should cause tests to run at least 2x as long so we probably have to request longer timeouts or split them up.

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

> Our codegen complains without those changes.

From the Web IDL spec:

  An interface's exposure set MUST also be a subset of the exposure set of all of the
  interface's consequential interfaces

Since the GlobalCrypto interface is a consequential interface of WorkerGlobalScope and WorkerGlobalScope is exposed in "Worker", you need GlobalCrypto exposed in "Worker".

If the webcrypto spec doesn't do that, the IDL in the spec is bogus and should be fixed.

> We can't do that anymore when exposing to Workers unfortunately.

You could replace with a Func that checks a pref value shipped from main thread on workers.  Assuming you want to keep the pref, that is.

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

Please let me know if you want me to raise the IDL issue.

	Ryan Sleevi
	Comment 46 • 9 years ago

*sigh* IDL is hard.

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface tried to address this before. Please let me know if it fails to address. This was in change https://dvcs.w3.org/hg/webcrypto-api/rev/810285715051 for https://www.w3.org/Bugs/Public/show_bug.cgi?id=25390

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

Ryan, that's the IDL we're talking about.  It just needs Exposed=(Window,Worker) added to GlobalCrypto.

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

(In reply to Boris Zbarsky [:bz] from comment #45)
> Please let me know if you want me to raise the IDL issue.

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27942.

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

(In reply to Boris Zbarsky [:bz] from comment #44)
> > We can't do that anymore when exposing to Workers unfortunately.
> 
> You could replace with a Func that checks a pref value shipped from main
> thread on workers.  Assuming you want to keep the pref, that is.

Ah, right. So we should find out whether we still want that or not.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 50 • 9 years ago

FWIW, the worker code already has infrastructure for forwarding certain pref values so that they can be read from worker threads.

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

The tests do currently not run because we have assertions everywhere that check we're on the main thread when calling SubtleCrypto/WebCryptoTask methods. The same goes for the CryptoTask implementation.

Without those NS_IsMainThread() assertions I can run a few tests successfully but am stopped by:

Assertion failure: js::CurrentThreadCanAccessRuntime(runtime_), at ../../../dist/include/js/HeapAPI.h:132

This is caused by a TypedArrayCreator calling JS_NewArrayBuffer() off the main thread. What's the best way to proceed here? Should we have a WorkerSubtleCrypto that forwards calls to the main thread and then moves the heavy work off the main thread again? That doesn't sound like the best way. And why exactly do ArrayBuffers fail here?

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

TypedArrayCreator should work fine on a worker thread.  Which thread are you running them on, exactly?  What's the callstack to the assertion?

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

Oh, I bet your issue is that CryptoTask is a broken puppy which makes the following assumptions:

1)  It's always dispatched from the main thread, so needs to reply on the main thread (see NS_DispatchToMainThread calls in there).

2)  It's always dispatched from the main thread, so when running on a non-main thread it must be on its work thread and should do the work synchronously.

Maybe we'll finally switch away from using CryptoTask here, which we should really do anyway...

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

(In reply to Tim Taubert [:ttaubert] from comment #54)
> Created attachment 8558564 [details] [diff] [review]
> 0002-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask.patch

With that patch all but two tests succeed when run in workers.

1) The first failing test is storing a key in IndexedDB, I get a DataCloneError when trying to store.

2) The second failing test is doing some JWK (un)wrapping with JWK. It crashes with the following message:

>    264 	        // XPConnect off the main thread. If you're an extension developer hitting
>    265 	        // this, you need to change your code. See bug 716167.
>    266 	        if (!MOZ_LIKELY(NS_IsMainThread()))
> -> 267 	            MOZ_CRASH();
>    268
>    269 	        return gSelf;
>    270 	    }

>   * frame #0: 0x00000001015cb541 XUL`xpc::UnprivilegedJunkScope() [inlined] nsXPConnect::XPConnect(this=<unavailable>) at xpcprivate.h:267
>     frame #1: 0x00000001015cb541 XUL`xpc::UnprivilegedJunkScope() [inlined] XPCJSRuntime::Get(this=<unavailable>) at xpcprivate.h:444
>     frame #2: 0x00000001015cb541 XUL`xpc::UnprivilegedJunkScope() + 33 at XPCJSRuntime.cpp:542
>     frame #3: 0x0000000101e1d648 XUL`mozilla::dom::JsonWebKey::ToJSON(this=0x00000001280c54a0, aJSON=0x000000012be007c8) const + 72 at SubtleCryptoBinding.cpp:3409
>     frame #4: 0x00000001024b7fd5 XUL`mozilla::dom::WrapKeyTask<mozilla::dom::AesTask>::AfterCrypto(this=0x00000001280c5400) + 133 at WebCryptoTask.cpp:2817

3) The third problem is that with a debug build I constantly run into:

>     frame #0: 0x0000000105069ffb XUL`mozilla::dom::workers::WorkerThread::Dispatch(this=0x000000011acd5b20, aRunnable=0x000000011a93a840, aFlags=0) + 379 at WorkerThread.cpp:220
>    217
>    218 	      // Only enforce cancelable runnables after we've started the worker loop.
>    219 	      if (!mAcceptingNonWorkerRunnables) {
> -> 220 	        MOZ_ASSERT(cancelable,
>    221 	                   "Only nsICancelableRunnable may be dispatched to a worker!");
>    222 	      }
>    223 	    }

The WebCryptoTask itself is a nsICancelableRunnable now. I wonder if some promise code tries to dispatch a runnable on the worker thread?

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

Comment on attachment 8558564 [details] [diff] [review]
0002-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask.patch

Boris, I'd love to get some feedback from you on the general approach here. I also hope that you can provide some more insights into the aforementioned failures.

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

Comment on attachment 8558564 [details] [diff] [review]
0002-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask.patch

This seems like a fairly faithful transcription of CryptoTask, but without the mainthread assumption, right?

That's OK as far as it goes, though it would be way better to fix bug 1001691 if we're going to be weaning ourselves off CryptoTask, either in a followup or before landing the patches to this bug...

As for the failures in comment 56:

1)  No idea offhand what's up with indexeddb.

2)  You should have hit the MOZ_ASSERT in ToJSON!  Were you checking in an opt build?  In any case, the ToJSON methods generated for dictionaries are currently mainthread-only.  That shouldn't be that difficult to fix.  Just have to get the global from the workerprivate in that case.  File a bug?  I'm happy to fix it if you'd rather not deal with it.

3)  Promise code all works on worker threads, in general.  Try breakpointing on that line an seeing what the concrete type of aRunnable is and what the stack is?

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

(In reply to Boris Zbarsky [:bz] from comment #58)
> Comment on attachment 8558564 [details] [diff] [review]
> This seems like a fairly faithful transcription of CryptoTask, but without
> the mainthread assumption, right?

Yes, indeed.

> That's OK as far as it goes, though it would be way better to fix bug
> 1001691 if we're going to be weaning ourselves off CryptoTask, either in a
> followup or before landing the patches to this bug...

Didn't know about that bug. It would probably make sense to decouple from CryptoTask and move to a new threading model at the same time. Let's continue with that one for now then.

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

Rebased.

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

Rebased.

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

Here's the IDL for the Crypto interface:

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 64 • 9 years ago

Comment on attachment 8671339 [details] [diff] [review]
0001-Bug-842818-Expose-WebCrypto-API-to-workers.patch

Perhaps baku could review this?
(one needs to make sure everything Crypto implementation uses is thread agnostic. I assume some changes will happen in other patches, and baku might be good reviewer for those too ;) )

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

Richard, can you please take a look at the tests? The patch doesn't do a lot other than running each test twice, once as usual and once in a Worker. It uses postMessage() to send the script to the worker and then execute it there. This works well for almost all tests, just had to rewrite the RSA JWK ones a little. They don't yet fully run as we're hitting some NS_IsMainThread() assertions and I'll sort that out in further patches.

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

In particular, is getRandomValues threadsafe?  Especially when it uses the contentchild singleton?

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

(In reply to Boris Zbarsky [:bz] from comment #66)
> In particular, is getRandomValues threadsafe?  Especially when it uses the
> contentchild singleton?

It asserts at the moment that it's being called on the main thread. That's only one of the many things we'll have to fix/address.

	Andrea Marchesini [:baku]
	Comment 68 • 9 years ago

Comment on attachment 8671339 [details] [diff] [review]
0001-Bug-842818-Expose-WebCrypto-API-to-workers.patch

Review of attachment 8671339 [details] [diff] [review]:
-----------------------------------------------------------------

This is good but it's not enough. I would like to see also the other patches.

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

(In reply to Andrea Marchesini (:baku) from comment #68)
> This is good but it's not enough. I would like to see also the other patches.

Sure, you will :) I'll r? you on them when they're ready.

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

I took a closer look at crypto.getRandomValues() and its history.

Bug 673432 implemented it so that we wouldn't have to load NSS in a content process (see bug 673432 comment #14) and used the ContentChild singleton to send a message to the parent, that then responded with random bytes. We can't use that same singleton off the main thread so we could get rid of it and just use NSS in the content process as we already do on e10s anyway, none of the crypto.subtle.* methods is proxied to the parent.

Bug 881761 landed the first code to use NSS in content, for WebRTC. It seems that the old policy referred to by bsmedberg was updated at that point. Bug 964455 is about a few difficulties this introduced for sandboxing but it looks like there's a solution that we just didn't have the time yet to implement.

open() isn't whitelisted anymore since bug 930258 landed but afaict from the patches, open(/dev/urandom) is still allowed to properly initialize the NSS PRNG. So I think this should be fine?

	Benjamin Smedberg
	Comment 71 • 9 years ago

Comment on attachment 8671762 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

I don't know what the situation is. My understanding is that we still aren't using the full NSS due to performance/memory but we've used it for some of its utility functions such as checksums and randomness. But I'm not in that loop any more.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 72 • 9 years ago

Comment on attachment 8671762 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

Review of attachment 8671762 [details] [diff] [review]:
-----------------------------------------------------------------

I believe you'll need to change the "nssEnsure" to "nssEnsureChromeOrContent" here: https://dxr.mozilla.org/mozilla-central/rev/23b7f289df923c01e692299fcd4be7029de8b155/security/manager/ssl/nsNSSModule.cpp#213 for this to work as intended. Unfortunately this doesn't get us exactly what we want, because that will cause EnsureNSSInitializedChromeOrContent() to be called, which is main-thread-only. Perhaps this bug would benefit from the PSM/NSS initialization refactoring we have planned for this quarter.

	Andrea Marchesini [:baku]
	Comment 73 • 9 years ago

Comment on attachment 8671762 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

Review of attachment 8671762 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Crypto.cpp
@@ +87,5 @@
>      return;
>    }
>  
> +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> +    do_GetService("@mozilla.org/security/random-generator;1");

are you sure that we can use this component in the child process?
Why did we use PContent for this?

@@ +96,5 @@
>  
> +  uint8_t* data = aArray.Data();
> +  nsresult rv = randomGenerator->GenerateRandomBytes(dataLen, &data);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);

any reason why this error code?
What about:

aRv = randomGenerator->GenerateRandomBytes(...);
if (NS_WARN_IF(aRv.Failed()) {
  return;
}

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

Please prefer aRv.Throw(rv) to using ErrorResult::operator=.

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

(In reply to Andrea Marchesini (:baku) from comment #73)
> > +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> > +    do_GetService("@mozilla.org/security/random-generator;1");
> 
> Why did we use PContent for this?

Please take a look at comment #70, when that code landed it seems the policy was to not use NSS in content. That's changed with WebRTC.

> are you sure that we can use this component in the child process?

Referring to comment #70 again, I think we can use it in child processes. The PRNG should be properly initialized as in the main process.

> @@ +96,5 @@
> >  
> > +  uint8_t* data = aArray.Data();
> > +  nsresult rv = randomGenerator->GenerateRandomBytes(dataLen, &data);
> > +  if (NS_FAILED(rv)) {
> > +    aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);
> 
> any reason why this error code?

It's an unexpected error occurring when calling crypto.getRandomValues(), the error code seems appropriate to me?

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

(In reply to David Keeler [:keeler] (use needinfo?) from comment #72)
> I believe you'll need to change the "nssEnsure" to
> "nssEnsureChromeOrContent" here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 23b7f289df923c01e692299fcd4be7029de8b155/security/manager/ssl/nsNSSModule.
> cpp#213 for this to work as intended. Unfortunately this doesn't get us
> exactly what we want, because that will cause
> EnsureNSSInitializedChromeOrContent() to be called, which is
> main-thread-only. Perhaps this bug would benefit from the PSM/NSS
> initialization refactoring we have planned for this quarter.

Ah, I didn't know we need it here too, but that makes sense. We unfortunately won't be able to expose the WebCrypto API on workers as long as we can't initialize NSS off the main thread. So yeah, this probably needs to wait until we can do that.

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

Comment on attachment 8671345 [details] [diff] [review]
0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch

Martin, would you mind taking a look?

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

Comment on attachment 8671345 [details] [diff] [review]
0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch

Review of attachment 8671345 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/test/test-array.js
@@ +106,5 @@
> +
> +  var base = new Test(name, test);
> +  for (var method of ["draw", "setRow", "next", "complete"]) {
> +    this[method] = base[method].bind(this);
> +  }

This is an odd way to do inheritance.  Why not set the prototype?  (I know why, but a comment would help some).

::: dom/crypto/test/test_WebCrypto_JWK.html
@@ +217,3 @@
>  TestArray.addTest(
>    "JWK import/export of an RSA private key where p < q",
> +  function () {

I'm not especially happy about you duplicating all this code just so that you can use toSource() to transfer the test to the worker.  I can't think of anything better though.

	Richard Barnes [:rbarnes]
	Comment 79 • 9 years ago

Comment on attachment 8671345 [details] [diff] [review]
0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch

Review of attachment 8671345 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/test/test-array.js
@@ +99,5 @@
> +    // Note the start time
> +    this.startTime = new Date();
> +
> +    worker.postMessage(test.toSource());
> +    worker.onmessage = e => this.complete(e.data);

This seems to assume that a message from the worker will always be a boolean.  It would help to have a comment to make this explicit.

::: dom/crypto/test/test-worker.js
@@ +38,5 @@
> +
> +  try {
> +    test.call({complete: finish});
> +  } catch (err) {
> +    dump(`WORKER ERROR :: ${err}\n`);

It seems like in this case, you would want to fire the onerror event for the worker.

	Martin Thomson [:mt:]
	Comment 81 • 8 years ago

Any report on progress?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 82 • 8 years ago

No progress, we unfortunately can't do much here until we can call EnsureNSSInitializedChromeOrContent() from a non-main thread.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 83 • 8 years ago

Unless you do a sync call from worker to the main thread when EnsureNSSInitializedChromeOrContent() call is needed. Such setup is used often in the worker code.
I would expect there to be some bool flag on worker side to hint whether the slow sync call is actually needed.

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

Yeah there's nothing wrong with blocking the worker to go to the main thread here.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 85 • 8 years ago

I see, thanks for the hints. Will give it another try.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 86 • 8 years ago

(In reply to Martin Thomson [:mt:] from comment #78)
> ::: dom/crypto/test/test-array.js
> @@ +106,5 @@
> > +
> > +  var base = new Test(name, test);
> > +  for (var method of ["draw", "setRow", "next", "complete"]) {
> > +    this[method] = base[method].bind(this);
> > +  }
> 
> This is an odd way to do inheritance.  Why not set the prototype?  (I know
> why, but a comment would help some).

Done.

(In reply to Richard Barnes [:rbarnes] from comment #79)
> ::: dom/crypto/test/test-array.js
> @@ +99,5 @@
> > +    worker.postMessage(test.toSource());
> > +    worker.onmessage = e => this.complete(e.data);
> 
> This seems to assume that a message from the worker will always be a
> boolean.  It would help to have a comment to make this explicit.

Added.

> ::: dom/crypto/test/test-worker.js
> @@ +38,5 @@
> > +
> > +  try {
> > +    test.call({complete: finish});
> > +  } catch (err) {
> > +    dump(`WORKER ERROR :: ${err}\n`);
> 
> It seems like in this case, you would want to fire the onerror event for the
> worker.

Yes, good idea.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 87 • 8 years ago

r=mt,rbarnes

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 88 • 8 years ago

Comment on attachment 8671762 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

Andrea, can you please take another look at this patch and esp. comment #70 and #75? AFAICT the content process uses /dev/urandom to properly seed the PRNG. We'd otherwise have a few problems with our WebRTC implementation.

	Andrea Marchesini [:baku]
	Comment 89 • 8 years ago

Comment on attachment 8671762 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

Review of attachment 8671762 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the dom/worker side. Ask for a review to some NSS peer too.
Plus, I guess that the next step is to work on bug 1187998.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 90 • 8 years ago

(In reply to Andrea Marchesini (:baku) from comment #89)
> r+ for the dom/worker side. Ask for a review to some NSS peer too.

Will do, thanks.

> Plus, I guess that the next step is to work on bug 1187998.

Yeah, that's one of the ~3 assertions I'm hitting when running webcrypto tests in workers.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 91 • 8 years ago

Comment on attachment 8671762 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

David, Martin, would you both mind talking a look (again)?

	Martin Thomson [:mt:]
	Comment 92 • 8 years ago

Comment on attachment 8671762 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

Review of attachment 8671762 [details] [diff] [review]:
-----------------------------------------------------------------

Not entirely happy about doing this on the current thread, but that is a fault in the spec not the implementation.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 93 • 8 years ago

Sorry to bother everyone again, but turns out I killed the PRNG. We have tests that caught it but I didn't run them on my machine...

The thing is that the random generator wants to allocate memory itself, so we can't just pass a pointer to the array buffer data into it, but have to use memcpy() and free() as the code did before.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 94 • 8 years ago

Comment on attachment 8709935 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v2

Review of attachment 8709935 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Crypto.cpp
@@ +93,5 @@
>      return;
>    }
>  
> +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> +    do_GetService("@mozilla.org/security/random-generator;1");

I think two things need to happen for this to work:

s/nssEnsure/nssEnsureChromeOrContent/ here: https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/security/manager/ssl/nsNSSModule.cpp#210

EnsureNSSInitializedChromeOrContent will have to do the dispatch-to-main-thread-and-wait scheme described in comment 83.

@@ +102,2 @@
>  
> +  uint8_t* buf;

Maybe use a scoped type here? Having to manually call free is a bummer.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 95 • 8 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #94)
> > +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> > +    do_GetService("@mozilla.org/security/random-generator;1");
> 
> I think two things need to happen for this to work:
> 
> s/nssEnsure/nssEnsureChromeOrContent/ here:
> https://dxr.mozilla.org/mozilla-central/rev/
> b67316254602a63bf4e568198a5c7d3288a9db27/security/manager/ssl/nsNSSModule.
> cpp#210

Not doing this for now as discussed on IRC. The sync-init functionality will live in a WebCrypto helper function until we see the need to move it into EnsureNSSInitializedChromeOrContent().

> EnsureNSSInitializedChromeOrContent will have to do the
> dispatch-to-main-thread-and-wait scheme described in comment 83.

Good catch, thanks.

> @@ +102,2 @@
> >  
> > +  uint8_t* buf;
> 
> Maybe use a scoped type here? Having to manually call free is a bummer.

We can't do this unfortunately, GenerateRandomBytes() takes a uint8_t** and allocates memory.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 96 • 8 years ago

Carrying over r=baku.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 97 • 8 years ago

This patch makes all crypto.subtle.* methods actually callable from workers. It uses the new maybe-sync EnsureNSSInitialized() once when the thread pool is initialized.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 98 • 8 years ago

This one's basically bug 1187998, I think it's easier to just do it in here.

It removes the main thread assertions in the structured cloning helper and adds two tests, sending/receiving keys to/from workers and then encrypting and decrypting data.

The dom/crypto/test/test_WebCrypto.html change is related as IndexedDB uses structured cloning too. The test needed to close the database on the main thread so that it didn't block the worker test running right afterwards.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 99 • 8 years ago

Just to be clear, *most* of the tests succeed, but there's some JWK code and at least one assertion that I still need to fix/remove. That means that at least one more patch will follow.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 100 • 8 years ago

Boris, we have a key wrapping test that fails when run in a worker. Specifically, we hit the assertion at [1] called by [2].

[1] https://hg.mozilla.org/mozilla-central/rev/9c67ef1f9515#l3.17
[2] https://hg.mozilla.org/mozilla-central/annotate/b67316254602/dom/crypto/WebCryptoTask.cpp#l2897

Can you please explain why the NS_IsMainThread() assertion is needed here and how we can get ToJSON() to run on a worker? Thanks!

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 101 • 8 years ago

Comment on attachment 8710146 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v3

Review of attachment 8710146 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with comments addressed.

::: dom/base/Crypto.cpp
@@ +100,3 @@
>  
> +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> +    do_GetService("@mozilla.org/security/random-generator;1");

I'm pretty sure this still requires the nsNSSModule.cpp change.

::: dom/crypto/WebCryptoCommon.cpp
@@ +23,5 @@
> +  // let's fire a SyncRunnable to do this synchronously.
> +  if (!initialized && !NS_IsMainThread()) {
> +    nsCOMPtr<nsIThread> mainThread;
> +    nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread));
> +    NS_ENSURE_SUCCESS(rv, false);

nit: NSS_ENSURE_SUCCESS hides a return, so it might be more clear to do this:

if (NS_FAILED(rv)) {
  return false;
}

(or even NS_WARN_IF(NS_FAILED(rv)))

@@ +27,5 @@
> +    NS_ENSURE_SUCCESS(rv, false);
> +
> +    mozilla::SyncRunnable::DispatchToThread(mainThread,
> +      new SyncRunnable(NS_NewRunnableFunction([&]() -> void {
> +        initialized = EnsureNSSInitializedChromeOrContent();

If we ever return true here, we should cache the result so subsequent non-main thread callers don't even have to dispatch to the main thread.

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

That code had the IsMainThread assertion because GetSafeJSContextGlobal() can only be called on the main thread.

The code has changed a bit, but UnprivilegedJunkScope is also only allowed to be called on the main thread.

We would need some worker-thread-safe equivalent, just like we need one in bug 1241349...  Bobby, I can add some binding utility function that returns the junk scope on mainthread and the worker global on a worker thread; does that seem reasonable?

	Martin Thomson [:mt:]
	Comment 103 • 8 years ago

Comment on attachment 8710146 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v3

Review of attachment 8710146 [details] [diff] [review]:
-----------------------------------------------------------------

So EnsureNSSInitializedChromeOrContent() always returns false off the main thread, so if I get this right, all WebCrypto functions would synchronously dispatch every time on worker threads?

If that's true, then I think that David is right about this: cache the result in a static atomic variable or something so that you can avoid unnecessary thread thrashing.  Otherwise this will substantially affect performance of the crypto APIs.

::: dom/crypto/WebCryptoCommon.cpp
@@ +26,5 @@
> +    nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread));
> +    NS_ENSURE_SUCCESS(rv, false);
> +
> +    mozilla::SyncRunnable::DispatchToThread(mainThread,
> +      new SyncRunnable(NS_NewRunnableFunction([&]() -> void {

Nit: Don't capture everything, just [&initialized]() {...}. You don't need the arrow for a void lambdas (don't know what the style guide says there...)

@@ +32,5 @@
> +      }))
> +    );
> +  }
> +
> +  return initialized;

I would put the caching just before the return rather than in the lambda.  That means that a call to this on the main thread might prime the value for all other threads, but I think that's OK.

	Andrew McCreight [:mccr8]
	Comment 104 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #102)
> We would need some worker-thread-safe equivalent, just like we need one in
> bug 1241349...  Bobby, I can add some binding utility function that returns
> the junk scope on mainthread and the worker global on a worker thread; does
> that seem reasonable?

FWIW, there was a similar issue recently with chrome workers instantiating FileReader, in bug 1240365.

	Bobby Holley (:bholley)
	Comment 105 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #102)
> That code had the IsMainThread assertion because GetSafeJSContextGlobal()
> can only be called on the main thread.
> 
> The code has changed a bit, but UnprivilegedJunkScope is also only allowed
> to be called on the main thread.
> 
> We would need some worker-thread-safe equivalent, just like we need one in
> bug 1241349...  Bobby, I can add some binding utility function that returns
> the junk scope on mainthread and the worker global on a worker thread; does
> that seem reasonable?

Sure.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 106 • 8 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #101)
> ::: dom/base/Crypto.cpp
> @@ +100,3 @@
> >  
> > +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> > +    do_GetService("@mozilla.org/security/random-generator;1");
> 
> I'm pretty sure this still requires the nsNSSModule.cpp change.

Done.

> ::: dom/crypto/WebCryptoCommon.cpp
> @@ +23,5 @@
> > +  // let's fire a SyncRunnable to do this synchronously.
> > +  if (!initialized && !NS_IsMainThread()) {
> > +    nsCOMPtr<nsIThread> mainThread;
> > +    nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread));
> > +    NS_ENSURE_SUCCESS(rv, false);
> 
> nit: NSS_ENSURE_SUCCESS hides a return, so it might be more clear to do this:
> 
> if (NS_FAILED(rv)) {
>   return false;
> }

Ok, done.

> @@ +27,5 @@
> > +    NS_ENSURE_SUCCESS(rv, false);
> > +
> > +    mozilla::SyncRunnable::DispatchToThread(mainThread,
> > +      new SyncRunnable(NS_NewRunnableFunction([&]() -> void {
> > +        initialized = EnsureNSSInitializedChromeOrContent();
> 
> If we ever return true here, we should cache the result so subsequent
> non-main thread callers don't even have to dispatch to the main thread.

I moved the EnsureNSSInitialized() call to the thread pool as the thread pool is initialized only once per process. And so this function would be called only once too. But you're right in that it would cause sync inits on every .getRandomValues() call from a worker in a content process. I missed that we check for PSM_COMPONENT_CONTRACTID only in the parent process, not in content.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 107 • 8 years ago

(In reply to Martin Thomson [:mt:] from comment #103)
> So EnsureNSSInitializedChromeOrContent() always returns false off the main
> thread, so if I get this right, all WebCrypto functions would synchronously
> dispatch every time on worker threads?

Only .getRandomValues() would do that if called by a worker in a content process. But yes, that needs to be fixed too.

> If that's true, then I think that David is right about this: cache the
> result in a static atomic variable or something so that you can avoid
> unnecessary thread thrashing.  Otherwise this will substantially affect
> performance of the crypto APIs.

Done.

> ::: dom/crypto/WebCryptoCommon.cpp
> @@ +26,5 @@
> > +    nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread));
> > +    NS_ENSURE_SUCCESS(rv, false);
> > +
> > +    mozilla::SyncRunnable::DispatchToThread(mainThread,
> > +      new SyncRunnable(NS_NewRunnableFunction([&]() -> void {
> 
> Nit: Don't capture everything, just [&initialized]() {...}. You don't need
> the arrow for a void lambdas (don't know what the style guide says there...)

Replaced the call to EnsureNSSInitializedChromeOrContent() with a recursive call to EnsureNSSInitialized() to simply forward non-main-thread calls to the main thread. The lambda thus doesn't need to capture any variable anymore.

> @@ +32,5 @@
> > +      }))
> > +    );
> > +  }
> > +
> > +  return initialized;
> 
> I would put the caching just before the return rather than in the lambda. 
> That means that a call to this on the main thread might prime the value for
> all other threads, but I think that's OK.

Restructured the function so that now the static atomic bool is read from any thread but only written to from the main thread. I think that's a little nicer than before.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 108 • 8 years ago

r=baku,keeler,mt

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

> We would need some worker-thread-safe equivalent, just like we need one in bug 1241349

Except not; I'll discuss this a bit more at length in bug 1241349.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 110 • 8 years ago

Comment on attachment 8710156 [details] [diff] [review]
0005-Bug-842818-Enable-structured-cloning-for-CryptoKeys-.patch

Review of attachment 8710156 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/test/test_WebCrypto_Workers.html
@@ +1,1 @@
> +<!DOCTYPE html>

Would it be worth it to also run all of the other webcrypto tests in a worker?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 111 • 8 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #110)
> Comment on attachment 8710156 [details] [diff] [review]
> 0005-Bug-842818-Enable-structured-cloning-for-CryptoKeys-.patch
> 
> Review of attachment 8710156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/crypto/test/test_WebCrypto_Workers.html
> @@ +1,1 @@
> > +<!DOCTYPE html>
> 
> Would it be worth it to also run all of the other webcrypto tests in a
> worker?

Yes, definitely. That's what part 2 (attachment #8709628 [details] [diff] [review]) is about :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 112 • 8 years ago

This patch removes the NS_IsMainThread() assertion from the CloneData() helper method.

Richard, you added this back when you landed the initial WebCrypto patches. I talked to :till and there seems to be no reason for CloneData() to only work on the main thread. I assume you added this only to ensure SetKeyData() is called from the main thread only? Which is a fair assumption in our current threading model.

	Richard Barnes [:rbarnes]
	Comment 113 • 8 years ago

Comment on attachment 8716246 [details] [diff] [review]
0006-Bug-842818-Remove-NS_IsMainThread-assertion-from-Clo.patch

Review of attachment 8716246 [details] [diff] [review]:
-----------------------------------------------------------------

So I'm not entirely sure what the right answer is, but I don't think this is it :)

On the one hand, I seem to recall CloneData being used for other things in WebCrypto.  It's an implementation of the "clone the data" algorithm [1], but AFAICT it's only used in the algorithm normalization process [2].  I think our normalization code handles this part of the process with ATTEMPT_BUFFER_INIT, though.

On the other hand, if CloneData really shouldn't be used anywhere besides SetKeyData, we should probably just inline it there.

Could you please take a look at the spec and see if there are other places we should be using CloneData, and if not, then kill it and put it inline in SetKeyData? 

[1] https://www.w3.org/TR/WebCryptoAPI/#terminology
[2] https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an-algorithm

	wyohknott
	Comment 114 • 8 years ago

Might I ask a potentially unrelated question?

Can this bug #1250930 be a consequence of this current bug not being resolved? Sandboxes, workers, it's kind of related, right?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 115 • 8 years ago

(In reply to wyohknott from comment #114)
> Can this bug #1250930 be a consequence of this current bug not being
> resolved? Sandboxes, workers, it's kind of related, right?

Good question. I'll take a look at it and will respond in the bug.

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

> Sandboxes, workers, it's kind of related, right?

I don't think it actually is, for what it's worth.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 117 • 8 years ago

(In reply to Richard Barnes [:rbarnes] from comment #113)
> On the one hand, I seem to recall CloneData being used for other things in
> WebCrypto.  It's an implementation of the "clone the data" algorithm [1],
> but AFAICT it's only used in the algorithm normalization process [2].  I
> think our normalization code handles this part of the process with
> ATTEMPT_BUFFER_INIT, though.

Yeah, we don't really use CloneData where the spec defines it, as you mention above.

Inlining CloneData is good but there's a little more we can do. The import task initialization routines (constructor, Init, SetKeyData) are quite confusing. I checked the call sites and here's what I did:


1) Inlined CloneData() into SetKeyData(). The function behaves slightly differently now as I removed the SetJwkFromKeyData() calls, The only four places that call it are the Import*KeyTask constructors that take a keyData argument, that means those are from actual crypto.subtle.importKey() calls.

(The "parse a JWK" algorithm as implemented by SetJwkFromKeyData() should only be used for unwrapKey.)

2) Added SetRawKeyData() that supports only raw key data, called by DeriveKeyTask::Resolve().

3) Added SetKeyDataMaybeParseJWK() that accepts a CryptoBuffer argument and has the inlined code from SetJwkFromKeyData(). This is only called from UnwrapKeyTask::Resolve() and implements the "parse a JWK" algorithm as described in the spec.

	Richard Barnes [:rbarnes]
	Comment 118 • 8 years ago

Comment on attachment 8728846 [details] [diff] [review]
0006-Bug-842818-Inline-CloneData-and-clean-up-ImportKeyTa.patch, v2

Review of attachment 8728846 [details] [diff] [review]:
-----------------------------------------------------------------

r=me modulo the test question

::: dom/crypto/test/test_WebCrypto_Wrap_Unwrap.html
@@ -178,5 @@
>  );
>  
>  // -----------------------------------------------------------------------------
>  TestArray.addTest(
> -  "JWK wrap/unwrap round-trip, with AES-GCM",

Why are we removing this test?  Seems unrelated to this patch.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 119 • 8 years ago

(In reply to Richard Barnes [:rbarnes] from comment #118)
> >  // -----------------------------------------------------------------------------
> >  TestArray.addTest(
> > -  "JWK wrap/unwrap round-trip, with AES-GCM",
> 
> Why are we removing this test?  Seems unrelated to this patch.

Oops, that accidentally found its way into the patch. Removed it only for testing while bz is looking into it, will resurrect.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 120 • 8 years ago

r=rbarnes

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 121 • 8 years ago

This patch removes testing/web-platform/meta/WebCryptoAPI/getRandomValues.worker.js.ini and thereby re-enables the WPT for crypto.getRandomValues() on workers.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 122 • 8 years ago

This patch adds Crypto and SubtleCrypto to the list of expected interfaces in dom/workers/test/serviceworkers/test_serviceworker_interfaces.js

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 123 • 8 years ago

Sorry, forgot to update test_worker_interfaces.js.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 124 • 8 years ago

Had to revise this patch once again due to WPT failures with workers in e10s. If we try to create an instance of nsRandomGenerator from a non-main thread in a content process this fails because NS_NSS_GENERIC_FACTORY_CONSTRUCTOR still calls EnsureNSSInitializedChromeOrContent() that then returns false.

This patch now introduces nssEnsureChromeOrContentSync, used only for nsRandomGenerator, that when instantiated initializes NSS properly - no matter the thread or process it's called from. The "Sync" part indicates that it may block the main thread while doing so because we have to use a SyncRunnable.

I mostly moved the code from v4 around, so the mechanisms stay the same, they're just called from different places.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 125 • 8 years ago

Small amendment.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 126 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=739569a91ef9

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 127 • 8 years ago

Comment on attachment 8738737 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v6

Review of attachment 8738737 [details] [diff] [review]:
-----------------------------------------------------------------

I think I was actually wrong the other day when I said it wouldn't be a good idea to put the dispatch-to-main-thread-and-wait code in EnsureNSSInitializedChromeOrContent itself. It should be ok to do that, and it will simplify the code changes here.

::: dom/base/Crypto.cpp
@@ -100,5 @@
> -  if (!XRE_IsParentProcess()) {
> -    InfallibleTArray<uint8_t> randomValues;
> -    // Tell the parent process to generate random values via PContent
> -    ContentChild* cc = ContentChild::GetSingleton();
> -    if (!cc->SendGetRandomValues(dataLen, &randomValues) ||

I think we still need this - currently NSS initialization in the child process doesn't cause any entropy to be collected, if I'm reading things correctly.

::: security/manager/ssl/nsNSSComponent.cpp
@@ +199,5 @@
> +      }
> +
> +      // Forward to the main thread synchronously.
> +      bool initialized = false;
> +      mozilla::SyncRunnable::DispatchToThread(mainThread,

Now that I think about this more, it should be safe to move this into EnsureNSSInitializedChromeOrContent and have non-main threads dispatch to the main thread and wait (particularly since all current callers are main-thread anyway). Let's be sure and document it, though. Also, we should cache the result for subsequent calls to avoid an unnecessary dispatch.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 128 • 8 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #127)
> I think I was actually wrong the other day when I said it wouldn't be a good
> idea to put the dispatch-to-main-thread-and-wait code in
> EnsureNSSInitializedChromeOrContent itself. It should be ok to do that, and
> it will simplify the code changes here.

Done.

> ::: dom/base/Crypto.cpp
> @@ -100,5 @@
> > -  if (!XRE_IsParentProcess()) {
> > -    InfallibleTArray<uint8_t> randomValues;
> > -    // Tell the parent process to generate random values via PContent
> > -    ContentChild* cc = ContentChild::GetSingleton();
> > -    if (!cc->SendGetRandomValues(dataLen, &randomValues) ||
> 
> I think we still need this - currently NSS initialization in the child
> process doesn't cause any entropy to be collected, if I'm reading things
> correctly.

As per result of the NSS call and discussion on IRC earlier today we should postpone this and tackle/remove feeding entropy to NSS in a follow-up. (Bug 1263015)

> ::: security/manager/ssl/nsNSSComponent.cpp
> @@ +199,5 @@
> > +      }
> > +
> > +      // Forward to the main thread synchronously.
> > +      bool initialized = false;
> > +      mozilla::SyncRunnable::DispatchToThread(mainThread,
> 
> Now that I think about this more, it should be safe to move this into
> EnsureNSSInitializedChromeOrContent and have non-main threads dispatch to
> the main thread and wait (particularly since all current callers are
> main-thread anyway). Let's be sure and document it, though. Also, we should
> cache the result for subsequent calls to avoid an unnecessary dispatch.

Done.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 129 • 8 years ago

Comment on attachment 8739242 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v7

Review of attachment 8739242 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 130 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cb1b2946566

	Pulsebot
	Comment 131 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a98b4fa6de54
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcb400c1a48
https://hg.mozilla.org/integration/mozilla-inbound/rev/f83f79281511
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdded53dc92f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9262f09b3322
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e4c84b299d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe4eae85573
https://hg.mozilla.org/integration/mozilla-inbound/rev/deef69da0f29

	Ryan VanderMeulen [:RyanVM]
	Comment 132 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a98b4fa6de54
https://hg.mozilla.org/mozilla-central/rev/ffcb400c1a48
https://hg.mozilla.org/mozilla-central/rev/f83f79281511
https://hg.mozilla.org/mozilla-central/rev/fdded53dc92f
https://hg.mozilla.org/mozilla-central/rev/9262f09b3322
https://hg.mozilla.org/mozilla-central/rev/17e4c84b299d
https://hg.mozilla.org/mozilla-central/rev/4fe4eae85573
https://hg.mozilla.org/mozilla-central/rev/deef69da0f29

	Florian Bender
	Comment 133 • 8 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]: long-standing request by web devs
[Suggested wording]: Web Crypto API is now available in Workers
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 134 • 8 years ago

Added to Fx48 Aurora release notes