842818 - [WebCryptoAPI] Enable Crypto in workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[David Dahl :ddahl]

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;
};

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 2

[David Dahl :ddahl]

(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

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[David Dahl :ddahl]

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

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[David Dahl :ddahl]

Attachment: Patch - WIP

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

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 9

[David Dahl :ddahl]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

dom/webidl/

Comment 11

[Tantek Çelik]

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.

Comment 12

[(no longer active)]

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

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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?

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[David Dahl :ddahl]

Attachment: WIP Patch - take 2

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 22

[David Dahl :ddahl]

Attachment: WorkerCrypto2

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

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 24

[David Dahl :ddahl]

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

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 26

[David Dahl :ddahl]

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

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 28

[David Dahl :ddahl]

Attachment: Proxy Patch - WIP

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[David Dahl :ddahl]

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix example codegen to work with worker-only bindings

Give that a shot?

Comment 32

[David Dahl :ddahl]

(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

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[David Dahl :ddahl]

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

Comment 35

[David Dahl :ddahl]

Attachment: Latest WIP

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

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 37

[David Dahl :ddahl]

Attachment: Latest WIP

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

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 40

[Anne (:annevk)]

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

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-842818-Expose-WebCrypto-API-to-workers.patch

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?

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch (WIP)

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.

Comment 44

[Boris Zbarsky [:bzbarsky]]

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

Comment 45

[Boris Zbarsky [:bzbarsky]]

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

Comment 46

[Ryan Sleevi]

*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

Comment 47

[Boris Zbarsky [:bzbarsky]]

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

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 49

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 51

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 52

[Boris Zbarsky [:bzbarsky]]

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

Comment 53

[Boris Zbarsky [:bzbarsky]]

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

Comment 54

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask.patch

Comment 55

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Run-WebCrypto-tests-in-Workers.patch

Comment 56

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 58

[Boris Zbarsky [:bzbarsky]]

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?

Comment 59

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 60

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-842818-Expose-WebCrypto-API-to-workers.patch, v2

Rebased.

Comment 61

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch, v2

Rebased.

Comment 62

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

Comment 63

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-842818-Expose-WebCrypto-API-to-workers.patch

Here's the IDL for the Crypto interface:

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

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 65

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch

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.

Comment 66

[Boris Zbarsky [:bzbarsky]]

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

Comment 67

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 68

[Andrea Marchesini [:baku]]

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.

Comment 69

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 70

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch

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?

Comment 71

[Benjamin Smedberg]

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.

Comment 72

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 73

[Andrea Marchesini [:baku]]

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;
}

Comment 74

[Boris Zbarsky [:bzbarsky]]

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

Comment 75

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 76

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 77

[Tim Taubert [:ttaubert] (inactive)]

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

Martin, would you mind taking a look?

Comment 78

[Martin Thomson [:mt:]]

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.

Comment 79

[Richard Barnes [:rbarnes]]

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.

Comment 81

[Martin Thomson [:mt:]]

Any report on progress?

Comment 82

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 83

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 84

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 85

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 86

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 87

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-842818-Run-WebCrypto-tests-in-Workers-r-mt-rbarn.patch, v2

r=mt,rbarnes

Comment 88

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 89

[Andrea Marchesini [:baku]]

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.

Comment 90

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 91

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 92

[Martin Thomson [:mt:]]

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.

Comment 93

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v2

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.

Comment 94

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 95

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 96

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v3

Carrying over r=baku.

Comment 97

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-842818-Allow-calling-WebCryptoTask-DispatchWithP.patch

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.

Comment 98

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-842818-Enable-structured-cloning-for-CryptoKeys-.patch

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.

Comment 99

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 100

[Tim Taubert [:ttaubert] (inactive)]

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!

Comment 101

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 102

[Boris Zbarsky [:bzbarsky]]

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?

Comment 103

[Martin Thomson [:mt:]]

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.

Comment 104

[Andrew McCreight [:mccr8]]

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

Comment 105

[Bobby Holley (:bholley)]

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

Comment 106

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 107

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 108

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v4

r=baku,keeler,mt

Comment 109

[Boris Zbarsky [:bzbarsky]]

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

Comment 110

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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?

Comment 111

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 112

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-842818-Remove-NS_IsMainThread-assertion-from-Clo.patch

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.

Comment 113

[Richard Barnes [:rbarnes]]

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

Comment 114

[wyohknott]

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?

Comment 115

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 116

[Boris Zbarsky [:bzbarsky]]

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

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

Comment 117

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-842818-Inline-CloneData-and-clean-up-ImportKeyTa.patch, v2

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

Comment 118

[Richard Barnes [:rbarnes]]

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.

Comment 119

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 120

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-842818-Inline-CloneData-and-clean-up-ImportKeyTa.patch, v3

r=rbarnes

Comment 121

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0007-Bug-842818-Enable-getRandomValues-web-platform-tests.patch

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

Comment 122

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0008-Bug-842818-Add-Crypto-and-SubtleCrypto-to-expected-s.patch

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

Comment 123

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0008-Bug-842818-Add-Crypto-and-SubtleCrypto-to-expected-s.patch, v2

Sorry, forgot to update test_worker_interfaces.js.

Comment 124

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v5

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.

Comment 125

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v6

Small amendment.

Comment 126

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 127

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 128

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v7

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

Comment 129

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 130

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 131

[Pulsebot]

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

Comment 132

[Ryan VanderMeulen [:RyanVM]]

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

Comment 133

[Florian Bender]

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

Comment 134

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx48 Aurora release notes