The default bug view has changed. See this FAQ.

[WebCryptoAPI] Enable Crypto in workers

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Workers
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: ddahl, Assigned: ttaubert)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla48
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, relnote-firefox 48+)

Details

Attachments

(8 attachments, 24 obsolete attachments)

6.46 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.85 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
2.83 KB, patch
keeler
: review+
Details | Diff | Splinter Review
7.79 KB, patch
baku
: review+
keeler
: review+
Details | Diff | Splinter Review
5.32 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
796 bytes, patch
jgraham
: review+
Details | Diff | Splinter Review
4.39 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.58 KB, patch
keeler
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 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;
};
(Reporter)

Updated

4 years ago
Depends on: 440046
(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.
(Reporter)

Comment 2

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

Comment 5

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

Updated

4 years ago
Assignee: nobody → ddahl
(Reporter)

Comment 7

4 years ago
Created attachment 724202 [details] [diff] [review]
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
Attachment #724202 - Flags: feedback?(Ms2ger)
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.
Attachment #724202 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 9

4 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?
dom/webidl/

Comment 11

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

Updated

4 years ago
Blocks: 865789
Keywords: dev-doc-needed
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.
I expect this is easier, yes, though the implementation will need to be taught to work off-thread, of course.  Kyle?
Flags: needinfo?(khuey)
(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 ...
Flags: needinfo?(khuey)
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?
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.
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...
(Reporter)

Comment 19

4 years ago
Created attachment 796294 [details] [diff] [review]
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.
Attachment #724202 - Attachment is obsolete: true
Attachment #796294 - Flags: feedback?(Ms2ger)
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 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.
Attachment #796294 - Flags: feedback?(Ms2ger) → feedback?(khuey)
(Reporter)

Comment 22

4 years ago
Created attachment 796917 [details] [diff] [review]
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
Attachment #796294 - Attachment is obsolete: true
Attachment #796294 - Flags: feedback?(khuey)
Attachment #796917 - Flags: feedback?(Ms2ger)
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.
(Reporter)

Comment 24

4 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?
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.
(Reporter)

Comment 26

4 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:)
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.
Attachment #796917 - Flags: feedback?(Ms2ger) → feedback?(khuey)
(Reporter)

Comment 28

4 years ago
Created attachment 797518 [details] [diff] [review]
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.
Attachment #796917 - Attachment is obsolete: true
Attachment #796917 - Flags: feedback?(khuey)
Attachment #797518 - Flags: feedback?(khuey)
> 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.
(Reporter)

Comment 30

4 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.
Created attachment 797865 [details] [diff] [review]
Fix example codegen to work with worker-only bindings

Give that a shot?
(Reporter)

Comment 32

4 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
> 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?
(Reporter)

Comment 34

4 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?
(Reporter)

Comment 35

4 years ago
Created attachment 798062 [details] [diff] [review]
Latest WIP

This patch attempts to generate random values via the runnable, not sure how to get the random data back to the worker.
Attachment #797518 - Attachment is obsolete: true
Attachment #797518 - Flags: feedback?(khuey)
Attachment #798062 - Flags: feedback?(khuey)
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.
Attachment #798062 - Flags: feedback?(khuey) → feedback-
(Reporter)

Comment 37

4 years ago
Created attachment 804765 [details] [diff] [review]
Latest WIP

Still need to re-name Crypto.h/.cpp to WorkerCrypto.h/cpp
Attachment #798062 - Attachment is obsolete: true
Duplicate of this bug: 1050491
I'm going to assume that ddahl isn't actively working on this.
Assignee: bugzilla → nobody

Comment 40

2 years ago
The IDL in the specification changed a bit. Due to us now supporting [Exposed] this should also be easier IDL-wise.
Summary: [WebCryptoAPI] Create the WorkerCrypto interface → [WebCryptoAPI] Enable Crypto in workers
Duplicate of this bug: 1111150
(Assignee)

Comment 42

2 years ago
Created attachment 8558119 [details] [diff] [review]
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?
Attachment #797865 - Attachment is obsolete: true
Attachment #804765 - Attachment is obsolete: true
(Assignee)

Comment 43

2 years ago
Created attachment 8558122 [details] [diff] [review]
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.
> 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.
Please let me know if you want me to raise the IDL issue.
Flags: needinfo?(ttaubert)

Comment 46

2 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
Ryan, that's the IDL we're talking about.  It just needs Exposed=(Window,Worker) added to GlobalCrypto.
(Assignee)

Comment 48

2 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.
Flags: needinfo?(ttaubert)
(Assignee)

Comment 49

2 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.
FWIW, the worker code already has infrastructure for forwarding certain pref values so that they can be read from worker threads.
(Assignee)

Comment 51

2 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?
TypedArrayCreator should work fine on a worker thread.  Which thread are you running them on, exactly?  What's the callstack to the assertion?
Flags: needinfo?(ttaubert)
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...
(Assignee)

Comment 54

2 years ago
Created attachment 8558564 [details] [diff] [review]
0002-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask.patch
Attachment #8558122 - Attachment is obsolete: true
Flags: needinfo?(ttaubert)
(Assignee)

Comment 55

2 years ago
Created attachment 8558565 [details] [diff] [review]
0003-Bug-842818-Run-WebCrypto-tests-in-Workers.patch
(Assignee)

Comment 56

2 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?
(Assignee)

Comment 57

2 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.
Attachment #8558564 - Flags: feedback?(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?
Attachment #8558564 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 59

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

Updated

2 years ago
Depends on: 1001691
(Assignee)

Comment 60

2 years ago
Created attachment 8663762 [details] [diff] [review]
0001-Bug-842818-Expose-WebCrypto-API-to-workers.patch, v2

Rebased.
Assignee: nobody → ttaubert
Attachment #8558119 - Attachment is obsolete: true
Attachment #8558564 - Attachment is obsolete: true
Attachment #8558565 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 61

2 years ago
Created attachment 8663763 [details] [diff] [review]
0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch, v2

Rebased.
(Assignee)

Comment 62

2 years ago
Created attachment 8664119 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch
(Assignee)

Comment 63

2 years ago
Created attachment 8671339 [details] [diff] [review]
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
Attachment #8663762 - Attachment is obsolete: true
Attachment #8671339 - Flags: review?(bugs)
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 ;) )
Attachment #8671339 - Flags: review?(bugs) → review?(amarchesini)
(Assignee)

Comment 65

2 years ago
Created attachment 8671345 [details] [diff] [review]
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.
Attachment #8663763 - Attachment is obsolete: true
Attachment #8671345 - Flags: review?(rlb)
In particular, is getRandomValues threadsafe?  Especially when it uses the contentchild singleton?
(Assignee)

Comment 67

2 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.
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.
Attachment #8671339 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 69

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

Comment 70

2 years ago
Created attachment 8671762 [details] [diff] [review]
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?
Attachment #8664119 - Attachment is obsolete: true
Attachment #8671762 - Flags: feedback?(dkeeler)
Attachment #8671762 - Flags: feedback?(benjamin)
Attachment #8671762 - Flags: feedback?(amarchesini)
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.
Attachment #8671762 - Flags: feedback?(benjamin)
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.
Attachment #8671762 - Flags: feedback?(dkeeler) → feedback+
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;
}
Attachment #8671762 - Flags: feedback?(amarchesini) → feedback-
Please prefer aRv.Throw(rv) to using ErrorResult::operator=.
(Assignee)

Comment 75

a year 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?
(Assignee)

Comment 76

a year 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.
(Assignee)

Comment 77

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

Martin, would you mind taking a look?
Attachment #8671345 - Flags: review?(martin.thomson)
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.
Attachment #8671345 - Flags: review?(martin.thomson) → review+
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.
Attachment #8671345 - Flags: review?(rlb) → review+
Duplicate of this bug: 1120930
Any report on progress?
Flags: needinfo?(ttaubert)
(Assignee)

Comment 82

a year ago
No progress, we unfortunately can't do much here until we can call EnsureNSSInitializedChromeOrContent() from a non-main thread.
Flags: needinfo?(ttaubert)
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.
Yeah there's nothing wrong with blocking the worker to go to the main thread here.
(Assignee)

Comment 85

a year ago
I see, thanks for the hints. Will give it another try.
(Assignee)

Comment 86

a year 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.
(Assignee)

Comment 87

a year ago
Created attachment 8709628 [details] [diff] [review]
0002-Bug-842818-Run-WebCrypto-tests-in-Workers-r-mt-rbarn.patch, v2

r=mt,rbarnes
Attachment #8671345 - Attachment is obsolete: true
Attachment #8709628 - Flags: review+
(Assignee)

Comment 88

a year 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.
Attachment #8671762 - Flags: review?(amarchesini)
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.
Attachment #8671762 - Flags: review?(amarchesini)
Attachment #8671762 - Flags: review+
Attachment #8671762 - Flags: feedback-
(Assignee)

Comment 90

a year 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.
(Assignee)

Comment 91

a year 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)?
Attachment #8671762 - Flags: review?(martin.thomson)
Attachment #8671762 - Flags: review?(dkeeler)
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.
Attachment #8671762 - Flags: review?(martin.thomson) → review+
(Assignee)

Comment 93

a year ago
Created attachment 8709935 [details] [diff] [review]
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.
Attachment #8671762 - Attachment is obsolete: true
Attachment #8671762 - Flags: review?(dkeeler)
Attachment #8709935 - Flags: review?(martin.thomson)
Attachment #8709935 - Flags: review?(dkeeler)
Attachment #8709935 - Flags: review?(amarchesini)
Attachment #8709935 - Flags: review?(amarchesini) → review+
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.
Attachment #8709935 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 95

a year 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.
(Assignee)

Comment 96

a year ago
Created attachment 8710146 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v3

Carrying over r=baku.
Attachment #8709935 - Attachment is obsolete: true
Attachment #8709935 - Flags: review?(martin.thomson)
Attachment #8710146 - Flags: review?(martin.thomson)
Attachment #8710146 - Flags: review?(dkeeler)
(Assignee)

Comment 97

a year ago
Created attachment 8710154 [details] [diff] [review]
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.
Attachment #8710154 - Flags: review?(dkeeler)
(Assignee)

Comment 98

a year ago
Created attachment 8710156 [details] [diff] [review]
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.
Attachment #8710156 - Flags: review?(dkeeler)
Attachment #8710156 - Flags: review?(amarchesini)
(Assignee)

Comment 99

a year 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.
(Assignee)

Updated

a year ago
Blocks: 1187998
(Assignee)

Comment 100

a year 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!
Flags: needinfo?(bzbarsky)
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.
Attachment #8710146 - Flags: review?(dkeeler) → review+
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?
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
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.
Attachment #8710146 - Flags: review?(martin.thomson) → review+
(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.
(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.
Flags: needinfo?(bobbyholley)
Attachment #8710156 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 106

a year 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.
(Assignee)

Comment 107

a year 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.
(Assignee)

Comment 108

a year ago
Created attachment 8710403 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v4

r=baku,keeler,mt
Attachment #8710146 - Attachment is obsolete: true
Attachment #8710403 - Flags: review+
> 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.
Attachment #8710154 - Flags: review?(dkeeler) → review+
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?
Attachment #8710156 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 111

a year 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 :)
Depends on: 1241349
(Assignee)

Comment 112

a year ago
Created attachment 8716246 [details] [diff] [review]
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.
Attachment #8716246 - Flags: review?(rlb)
Depends on: 1246153
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
Attachment #8716246 - Flags: review?(rlb) → review-

Comment 114

a year 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?
(Assignee)

Comment 115

a year 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.
> Sandboxes, workers, it's kind of related, right?

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

Comment 117

a year ago
Created attachment 8728846 [details] [diff] [review]
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.
Attachment #8716246 - Attachment is obsolete: true
Attachment #8728846 - Flags: review?(rlb)
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.
Attachment #8728846 - Flags: review?(rlb) → review+
(Assignee)

Comment 119

a year 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.
(Assignee)

Comment 120

a year ago
Created attachment 8731621 [details] [diff] [review]
0006-Bug-842818-Inline-CloneData-and-clean-up-ImportKeyTa.patch, v3

r=rbarnes
Attachment #8728846 - Attachment is obsolete: true
Attachment #8731621 - Flags: review+
(Assignee)

Comment 121

a year ago
Created attachment 8738177 [details] [diff] [review]
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.
Attachment #8738177 - Flags: review?(james)
(Assignee)

Comment 122

a year ago
Created attachment 8738178 [details] [diff] [review]
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
Attachment #8738178 - Flags: review?(bugs)
Attachment #8738177 - Flags: review?(james) → review+

Updated

a year ago
Attachment #8738178 - Flags: review?(bugs) → review+
(Assignee)

Comment 123

a year ago
Created attachment 8738217 [details] [diff] [review]
0008-Bug-842818-Add-Crypto-and-SubtleCrypto-to-expected-s.patch, v2

Sorry, forgot to update test_worker_interfaces.js.
Attachment #8738178 - Attachment is obsolete: true
Attachment #8738217 - Flags: review?(bugs)

Updated

a year ago
Attachment #8738217 - Flags: review?(bugs) → review+
(Assignee)

Comment 124

a year ago
Created attachment 8738730 [details] [diff] [review]
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.
Attachment #8710403 - Attachment is obsolete: true
Attachment #8738730 - Flags: review?(dkeeler)
(Assignee)

Comment 125

a year ago
Created attachment 8738737 [details] [diff] [review]
0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v6

Small amendment.
Attachment #8738730 - Attachment is obsolete: true
Attachment #8738730 - Flags: review?(dkeeler)
Attachment #8738737 - Flags: review?(dkeeler)
(Assignee)

Comment 126

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=739569a91ef9
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.
Attachment #8738737 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 128

a year ago
Created attachment 8739242 [details] [diff] [review]
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.
Attachment #8738737 - Attachment is obsolete: true
Attachment #8739242 - Flags: review?(dkeeler)
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.
Attachment #8739242 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 130

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

Comment 131

a year 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

Comment 132

a year 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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 133

a year 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
relnote-firefox: --- → ?

Comment 134

11 months ago
Added to Fx48 Aurora release notes
relnote-firefox: ? → 48+
You need to log in before you can comment on or make changes to this bug.