Closed
Bug 842818
Opened 12 years ago
Closed 9 years ago
[WebCryptoAPI] Enable Crypto in workers
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: ddahl, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(8 files, 24 obsolete files)
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 |
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•12 years ago
|
||
(In reply to David Dahl :ddahl from comment #0) > The WorkerCrypto interface is needed for the WebCryptoAPI. The near-term > need is to support window.crypto.getRandomValues() in Workers > > http://lists.w3.org/Archives/Public/public-webcrypto/2012Oct/0076.html > > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview. > html#WorkerCrypto-interface > > Ryan Sleevi's proposal from the above mail thread: > > partial interface WorkerCrypto { > ArrayBufferView getRandomValues(ArrayBufferView array); > }; > > partial interface WorkerGlobalScope { > readonly attribute WorkerCrypto crypto; > }; This seems very reasonable to me, but should we be prefixing crypto as mozCrypto? IMO, this should be the next step in Mozilla's implementation of the web crypto API. Unlike some other parts of the W3C proposed API, getRandomValues is uncontroversial (at least, we resolved all the controversy). Having getRandomValues working in workers, on B2G, on Desktop, and (after this) in extensions sets the stage for rapid development of all the other features of the API and potential contributor involvement because there won't be any outstanding architectural issues to resolve, except for key management.
Reporter | ||
Comment 2•12 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•12 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•12 years ago
|
Assignee: nobody → ddahl
Reporter | ||
Comment 7•12 years ago
|
||
Ms2Ger: I almost have this thing building, some of the JS api stuff I know nothing about is confusing to me... Inside of WorkerCrypto.cpp I think I need a method like: static JSBool WorkerGetRandomValues(JSContext* aCx, unsigned aArgc, jsval* aVp) in order to make the function appear in the WorkerScope - any pointers there would be helpful, I was cribbing off of dom/workers/Navigator.cpp
Attachment #724202 -
Flags: feedback?(Ms2ger)
Comment 8•12 years ago
|
||
Comment on attachment 724202 [details] [diff] [review] Patch - WIP Review of attachment 724202 [details] [diff] [review]: ----------------------------------------------------------------- I'm afraid you chose the wrong implementation to crib off; look at TextEn/Decoder or XHR for something that uses WebIDL. That should end up being a lot simpler.
Attachment #724202 -
Flags: feedback?(Ms2ger)
Reporter | ||
Comment 9•12 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?
![]() |
||
Comment 10•12 years ago
|
||
dom/webidl/
Comment 11•11 years ago
|
||
Is there a master bug for "implement WebCryptoAPI http://www.w3.org/TR/WebCryptoAPI/ " that depends on this bug? I searched and couldn't find it and figured I'd ask before creating.
Comment 12•11 years ago
|
||
(In reply to comment #11) > Is there a master bug for "implement WebCryptoAPI > http://www.w3.org/TR/WebCryptoAPI/ " that depends on this bug? I searched and > couldn't find it and figured I'd ask before creating. Bug 865789.
Updated•11 years ago
|
Blocks: web-crypto
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 13•11 years ago
|
||
Is it easier to fix this bug now that bug 883741 (WebCrypto: Move Crypto to WebIDL) has landed? My interest is that the Gaia E-mail app does almost all of its work on the worker thread. While we are not going to land any crypto stuff (meta/discussion bug is bug 894817) for quite a while, there is community interest in prototyping and it would be very helpful if getRandomValues were available on the worker.
![]() |
||
Comment 14•11 years ago
|
||
I expect this is easier, yes, though the implementation will need to be taught to work off-thread, of course. Kyle?
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)
![]() |
||
Comment 16•11 years ago
|
||
We could just not expose the legacy stuff to worker JS by flagging them all individually with a [Func] annotation that checks for mainthread, no?
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•11 years ago
|
||
Actually, we won't. Per spec, for example, .location in a worker is a WorkerLocation. That implements URLUtilsReadOnly. On mainthread, .location is a Location. That implements URLUtils. Similarly for navigator: the worker one is a WorkerNavigator, which implements some other interfaces, but not all the ones Navigator implements. The only reason Crypto is a problem is that we don't want to bake the distinction between the worker and mainthread version into the toString on the objects by naming the worker-side interface something different, basically...
Reporter | ||
Comment 19•11 years ago
|
||
Trying to take another stab at this. Wondering if 'mozilla/dom/WorkerCryptoBinding.h' is something that will be auto-generated: In file included from /home/ddahl/code/moz/incoming/src/dom/workers/Crypto.cpp:11: 0:51.24 /home/ddahl/code/moz/incoming/src/dom/workers/DOMBindingInlines.h:14:10: fatal error: 'mozilla/dom/WorkerCryptoBinding.h' file not found 0:51.24 #include "mozilla/dom/WorkerCryptoBinding.h" 0:51.24 ^ 0:51.31 1 error generated.
Attachment #724202 -
Attachment is obsolete: true
Attachment #796294 -
Flags: feedback?(Ms2ger)
![]() |
||
Comment 20•11 years ago
|
||
Yes, WorkerCryptoBinding will be auto-generated when you add your .webidl file to WebIDL.mk. Note that there is already a definition of RandomSource, so you don't need to define it again (and in fact, attempting to define it twice would fail IDL parsing).
Comment 21•11 years ago
|
||
Comment on attachment 796294 [details] [diff] [review] WIP Patch - take 2 Review of attachment 796294 [details] [diff] [review]: ----------------------------------------------------------------- You forgot to add the header to dom/webidl/WebIDL.mk. Kyle knows more about the workers bindings, though.
Attachment #796294 -
Flags: feedback?(Ms2ger) → feedback?(khuey)
Reporter | ||
Comment 22•11 years ago
|
||
OK, this patch creates a crypto object inside workers with a getRandomValues noop method hanging off of it. The tests have also been stubbed out. Spoke with khuey and bz earlier about how to proxy the original dom method through to the worker. I am studying /dom/workers/URL.cpp as a 'model' to continue. <ddahl_> bz: I was trying to make the existing getRandomValues code work inside the worker as an experiment, sounds like this is not going to work this way <khuey> ddahl_: the usual way to do this is to have the worker impl proxy to the mainthread impl <ddahl_> khuey: is there an example of a proxy to learn from? <khuey> ddahl_: URL
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•11 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #23) > However if the backend that you need to get to, in this case I'm guessing > something in NSS, only works on the main thread, then you don't really have > much of a choice. > > But if you can call the relevant NSS functions from the worker thread, then > that's definitely preferable to proxying. Yeah, we can only call nsIRandomGenerator from the main thread. There is no way to use ipc from workers, huh?
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•11 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #25) > i*p*c? Isn't there only one process involved here? Do you mean > inter-thread-communication? Sorry, I was referring to they way we handle getRandomValues now on b2g: https://mxr.mozilla.org/mozilla-central/source/dom/base/Crypto.cpp#90 > It's definitely possible to use inter-thread-communication in workers. > Though you should talk to bent about how to do it. Not sure which is worse to deal with:)
Comment 27•11 years ago
|
||
Comment on attachment 796917 [details] [diff] [review] WorkerCrypto2 Review of attachment 796917 [details] [diff] [review]: ----------------------------------------------------------------- I don't think I know enough about the worker bindings to comment usefully here.
Attachment #796917 -
Flags: feedback?(Ms2ger) → feedback?(khuey)
Reporter | ||
Comment 28•11 years ago
|
||
This patch provides a crypto object to workers and attempts to use the same kind of proxy used in URL. This patch still needs to include dom/base/Crypto.h somehow to use our existing GetRandomValues - or - do we just go ahead and basically copy most of the GetRandomValues implementation (use of nsRandomGenerator, etc) into the RandomValuesRunnable? I am unsure what the Worker's crypto.GetRandomValues implementation's signature should be, naturally, the spec and webidl says it returns an ArrayBufferView.
Attachment #796917 -
Attachment is obsolete: true
Attachment #796917 -
Flags: feedback?(khuey)
Attachment #797518 -
Flags: feedback?(khuey)
![]() |
||
Comment 29•11 years ago
|
||
> I am unsure what the Worker's crypto.GetRandomValues implementation's signature should be
Run "make -C $objdir/dom/bindings/ WorkerCrypto-example" and then take a look at $objdir/dom/bindings/WorkerCrypto-example.h. So far for workers it can be a bit off in some cases, but I suspect in this case it will be correct.
Reporter | ||
Comment 30•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #29) > > I am unsure what the Worker's crypto.GetRandomValues implementation's signature should be > > Run "make -C $objdir/dom/bindings/ WorkerCrypto-example" and then take a > look at $objdir/dom/bindings/WorkerCrypto-example.h. When I do this with WorkerCrypto, the bindings generation fails - also WorkerLocation, WorkerNavigator fail the same way: http://pastebin.mozilla.org/2940574 The Crypto interface has getRandomValues as: JSObject* GetRandomValues(JSContext* cx, const ArrayBufferView& array, ErrorResult& aRv); Using URL as an example, I see there is also a GlobalObject argument that is used inside of the WorkerPrivate: static void CreateObjectURL(const GlobalObject& global, nsIDOMBlob* blob, const objectURLOptions& options, nsString& retval, ErrorResult& aRv); I am wondering if the WorkerCrypto signature will also require the GlobalObject arg.
![]() |
||
Comment 31•11 years ago
|
||
Give that a shot?
Reporter | ||
Comment 32•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #31) > Created attachment 797865 [details] [diff] [review] > Fix example codegen to work with worker-only bindings > > Give that a shot? Thank you, that worked: JSObject* GetRandomValues(JSContext* cx, const ArrayBufferView& array, ErrorResult& aRv); Looks like my WorkerCrypto arguments are correct. Since we have access to the window object inside the runnable, can we just call JSObject* randomValues = Crypto->GetRandomValues(aContext, aArray, aRv) directly? That seems like the DRY approach. Perhaps that is a naive approach? I tried to #include /mozilla/dom/Crypto.h to instanciate an nsIDOMCrypto object, but was unable to successfully include it. This made me think that I should not be trying to #include that inside dom/workers/Crypto.cpp
![]() |
||
Comment 33•11 years ago
|
||
> Perhaps that is a naive approach? Unfortunately, yes, if aArray actually represents a worker-side JS object, since we can't have the main thread operating on it directly. What you want to proxy to the main thread is not the whole API call, but just the "here is a memory buffer, write to it" bit or a "hand me some random data". The extraction of the memory buffer should happen worker-side. So basically, you want to get the buffer (ideally sharing code with Crypto::GetRandomValues(JSContext*, const ArrayBufferView&, ErrorResult&) in the process) and then post a runnable that just has the uint8_t*... or have the runnable return a uint8_t* to the worker (so the runnable would just call the Crypto::GetRandomValues(uint32_t) static method on the UI thread). > but was unable to successfully include it. LOCAL_INCLUDES issues?
Reporter | ||
Comment 34•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #33) > ... or have the runnable return a uint8_t* to the worker (so the > runnable would just call the Crypto::GetRandomValues(uint32_t) static method > on the UI thread). OK. Thanks for the guidance, I am getting closer. How is the uint8_t* data returned to the worker thread? I am passing it to the runnable's constructor, the runnable is calling GetRandomValues(uint32_t) on the main thread, just not sure how to make sure it gets back to the worker. Is there some way to do this via the ResponseRunnable object?
Reporter | ||
Comment 35•11 years ago
|
||
This patch attempts to generate random values via the runnable, not sure how to get the random data back to the worker.
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•11 years ago
|
||
Still need to re-name Crypto.h/.cpp to WorkerCrypto.h/cpp
Attachment #798062 -
Attachment is obsolete: true
I'm going to assume that ddahl isn't actively working on this.
Assignee: bugzilla → nobody
Comment 40•10 years ago
|
||
The IDL in the specification changed a bit. Due to us now supporting [Exposed] this should also be easier IDL-wise.
Summary: [WebCryptoAPI] Create the WorkerCrypto interface → [WebCryptoAPI] Enable Crypto in workers
Assignee | ||
Comment 42•10 years ago
|
||
Exposing the API with the latest IDL changes is indeed quite easy. > -[NoInterfaceObject] > +[NoInterfaceObject, Exposed=(Window,Worker)] > interface GlobalCrypto { Our codegen complains without those changes. Not sure if we need to fix that or the spec? > -[Pref="dom.webcrypto.enabled"] > +[Exposed=(Window,Worker)] We can't do that anymore when exposing to Workers unfortunately. It feels like we could probably get rid of the pref by now though?
Attachment #797865 -
Attachment is obsolete: true
Attachment #804765 -
Attachment is obsolete: true
Assignee | ||
Comment 43•10 years ago
|
||
It's probably a good idea to run all of the existing tests in Workers as well, so here's a patch for that. It works well but needs some cleanup. It should cause tests to run at least 2x as long so we probably have to request longer timeouts or split them up.
![]() |
||
Comment 44•10 years ago
|
||
> Our codegen complains without those changes. From the Web IDL spec: An interface's exposure set MUST also be a subset of the exposure set of all of the interface's consequential interfaces Since the GlobalCrypto interface is a consequential interface of WorkerGlobalScope and WorkerGlobalScope is exposed in "Worker", you need GlobalCrypto exposed in "Worker". If the webcrypto spec doesn't do that, the IDL in the spec is bogus and should be fixed. > We can't do that anymore when exposing to Workers unfortunately. You could replace with a Func that checks a pref value shipped from main thread on workers. Assuming you want to keep the pref, that is.
![]() |
||
Comment 45•10 years ago
|
||
Please let me know if you want me to raise the IDL issue.
Flags: needinfo?(ttaubert)
Comment 46•10 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
![]() |
||
Comment 47•10 years ago
|
||
Ryan, that's the IDL we're talking about. It just needs Exposed=(Window,Worker) added to GlobalCrypto.
Assignee | ||
Comment 48•10 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•10 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•10 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?
![]() |
||
Comment 52•10 years ago
|
||
TypedArrayCreator should work fine on a worker thread. Which thread are you running them on, exactly? What's the callstack to the assertion?
Flags: needinfo?(ttaubert)
![]() |
||
Comment 53•10 years ago
|
||
Oh, I bet your issue is that CryptoTask is a broken puppy which makes the following assumptions: 1) It's always dispatched from the main thread, so needs to reply on the main thread (see NS_DispatchToMainThread calls in there). 2) It's always dispatched from the main thread, so when running on a non-main thread it must be on its work thread and should do the work synchronously. Maybe we'll finally switch away from using CryptoTask here, which we should really do anyway...
Assignee | ||
Comment 54•10 years ago
|
||
Attachment #8558122 -
Attachment is obsolete: true
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 55•10 years ago
|
||
Assignee | ||
Comment 56•10 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•10 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 58•10 years ago
|
||
Comment on attachment 8558564 [details] [diff] [review] 0002-Bug-842818-Decouple-WebCryptoTask-from-CryptoTask.patch This seems like a fairly faithful transcription of CryptoTask, but without the mainthread assumption, right? That's OK as far as it goes, though it would be way better to fix bug 1001691 if we're going to be weaning ourselves off CryptoTask, either in a followup or before landing the patches to this bug... As for the failures in comment 56: 1) No idea offhand what's up with indexeddb. 2) You should have hit the MOZ_ASSERT in ToJSON! Were you checking in an opt build? In any case, the ToJSON methods generated for dictionaries are currently mainthread-only. That shouldn't be that difficult to fix. Just have to get the global from the workerprivate in that case. File a bug? I'm happy to fix it if you'd rather not deal with it. 3) Promise code all works on worker threads, in general. Try breakpointing on that line an seeing what the concrete type of aRunnable is and what the stack is?
Attachment #8558564 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 59•10 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 | ||
Comment 60•9 years ago
|
||
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•9 years ago
|
||
Rebased.
Assignee | ||
Comment 62•9 years ago
|
||
Assignee | ||
Comment 63•9 years ago
|
||
Here's the IDL for the Crypto interface: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface
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•9 years ago
|
||
Richard, can you please take a look at the tests? The patch doesn't do a lot other than running each test twice, once as usual and once in a Worker. It uses postMessage() to send the script to the worker and then execute it there. This works well for almost all tests, just had to rewrite the RSA JWK ones a little. They don't yet fully run as we're hitting some NS_IsMainThread() assertions and I'll sort that out in further patches.
Attachment #8663763 -
Attachment is obsolete: true
Attachment #8671345 -
Flags: review?(rlb)
![]() |
||
Comment 66•9 years ago
|
||
In particular, is getRandomValues threadsafe? Especially when it uses the contentchild singleton?
Assignee | ||
Comment 67•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #66) > In particular, is getRandomValues threadsafe? Especially when it uses the > contentchild singleton? It asserts at the moment that it's being called on the main thread. That's only one of the many things we'll have to fix/address.
Comment 68•9 years ago
|
||
Comment on attachment 8671339 [details] [diff] [review] 0001-Bug-842818-Expose-WebCrypto-API-to-workers.patch Review of attachment 8671339 [details] [diff] [review]: ----------------------------------------------------------------- This is good but it's not enough. I would like to see also the other patches.
Attachment #8671339 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 69•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #68) > This is good but it's not enough. I would like to see also the other patches. Sure, you will :) I'll r? you on them when they're ready.
Assignee | ||
Comment 70•9 years ago
|
||
I took a closer look at crypto.getRandomValues() and its history. Bug 673432 implemented it so that we wouldn't have to load NSS in a content process (see bug 673432 comment #14) and used the ContentChild singleton to send a message to the parent, that then responded with random bytes. We can't use that same singleton off the main thread so we could get rid of it and just use NSS in the content process as we already do on e10s anyway, none of the crypto.subtle.* methods is proxied to the parent. Bug 881761 landed the first code to use NSS in content, for WebRTC. It seems that the old policy referred to by bsmedberg was updated at that point. Bug 964455 is about a few difficulties this introduced for sandboxing but it looks like there's a solution that we just didn't have the time yet to implement. open() isn't whitelisted anymore since bug 930258 landed but afaict from the patches, open(/dev/urandom) is still allowed to properly initialize the NSS PRNG. So I think this should be fine?
Attachment #8664119 -
Attachment is obsolete: true
Attachment #8671762 -
Flags: feedback?(dkeeler)
Attachment #8671762 -
Flags: feedback?(benjamin)
Attachment #8671762 -
Flags: feedback?(amarchesini)
Comment 71•9 years ago
|
||
Comment on attachment 8671762 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch I don't know what the situation is. My understanding is that we still aren't using the full NSS due to performance/memory but we've used it for some of its utility functions such as checksums and randomness. But I'm not in that loop any more.
Attachment #8671762 -
Flags: feedback?(benjamin)
![]() |
||
Comment 72•9 years ago
|
||
Comment on attachment 8671762 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch Review of attachment 8671762 [details] [diff] [review]: ----------------------------------------------------------------- I believe you'll need to change the "nssEnsure" to "nssEnsureChromeOrContent" here: https://dxr.mozilla.org/mozilla-central/rev/23b7f289df923c01e692299fcd4be7029de8b155/security/manager/ssl/nsNSSModule.cpp#213 for this to work as intended. Unfortunately this doesn't get us exactly what we want, because that will cause EnsureNSSInitializedChromeOrContent() to be called, which is main-thread-only. Perhaps this bug would benefit from the PSM/NSS initialization refactoring we have planned for this quarter.
Attachment #8671762 -
Flags: feedback?(dkeeler) → feedback+
Comment 73•9 years ago
|
||
Comment on attachment 8671762 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch Review of attachment 8671762 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Crypto.cpp @@ +87,5 @@ > return; > } > > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > + do_GetService("@mozilla.org/security/random-generator;1"); are you sure that we can use this component in the child process? Why did we use PContent for this? @@ +96,5 @@ > > + uint8_t* data = aArray.Data(); > + nsresult rv = randomGenerator->GenerateRandomBytes(dataLen, &data); > + if (NS_FAILED(rv)) { > + aRv.Throw(NS_ERROR_DOM_OPERATION_ERR); any reason why this error code? What about: aRv = randomGenerator->GenerateRandomBytes(...); if (NS_WARN_IF(aRv.Failed()) { return; }
Attachment #8671762 -
Flags: feedback?(amarchesini) → feedback-
![]() |
||
Comment 74•9 years ago
|
||
Please prefer aRv.Throw(rv) to using ErrorResult::operator=.
Assignee | ||
Comment 75•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #73) > > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > > + do_GetService("@mozilla.org/security/random-generator;1"); > > Why did we use PContent for this? Please take a look at comment #70, when that code landed it seems the policy was to not use NSS in content. That's changed with WebRTC. > are you sure that we can use this component in the child process? Referring to comment #70 again, I think we can use it in child processes. The PRNG should be properly initialized as in the main process. > @@ +96,5 @@ > > > > + uint8_t* data = aArray.Data(); > > + nsresult rv = randomGenerator->GenerateRandomBytes(dataLen, &data); > > + if (NS_FAILED(rv)) { > > + aRv.Throw(NS_ERROR_DOM_OPERATION_ERR); > > any reason why this error code? It's an unexpected error occurring when calling crypto.getRandomValues(), the error code seems appropriate to me?
Assignee | ||
Comment 76•9 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #72) > I believe you'll need to change the "nssEnsure" to > "nssEnsureChromeOrContent" here: > https://dxr.mozilla.org/mozilla-central/rev/ > 23b7f289df923c01e692299fcd4be7029de8b155/security/manager/ssl/nsNSSModule. > cpp#213 for this to work as intended. Unfortunately this doesn't get us > exactly what we want, because that will cause > EnsureNSSInitializedChromeOrContent() to be called, which is > main-thread-only. Perhaps this bug would benefit from the PSM/NSS > initialization refactoring we have planned for this quarter. Ah, I didn't know we need it here too, but that makes sense. We unfortunately won't be able to expose the WebCrypto API on workers as long as we can't initialize NSS off the main thread. So yeah, this probably needs to wait until we can do that.
Assignee | ||
Comment 77•9 years ago
|
||
Comment on attachment 8671345 [details] [diff] [review] 0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch Martin, would you mind taking a look?
Attachment #8671345 -
Flags: review?(martin.thomson)
Comment 78•9 years ago
|
||
Comment on attachment 8671345 [details] [diff] [review] 0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch Review of attachment 8671345 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/test/test-array.js @@ +106,5 @@ > + > + var base = new Test(name, test); > + for (var method of ["draw", "setRow", "next", "complete"]) { > + this[method] = base[method].bind(this); > + } This is an odd way to do inheritance. Why not set the prototype? (I know why, but a comment would help some). ::: dom/crypto/test/test_WebCrypto_JWK.html @@ +217,3 @@ > TestArray.addTest( > "JWK import/export of an RSA private key where p < q", > + function () { I'm not especially happy about you duplicating all this code just so that you can use toSource() to transfer the test to the worker. I can't think of anything better though.
Attachment #8671345 -
Flags: review?(martin.thomson) → review+
Comment 79•9 years ago
|
||
Comment on attachment 8671345 [details] [diff] [review] 0002-Bug-842818-Run-WebCrypto-tests-in-Workers.patch Review of attachment 8671345 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/test/test-array.js @@ +99,5 @@ > + // Note the start time > + this.startTime = new Date(); > + > + worker.postMessage(test.toSource()); > + worker.onmessage = e => this.complete(e.data); This seems to assume that a message from the worker will always be a boolean. It would help to have a comment to make this explicit. ::: dom/crypto/test/test-worker.js @@ +38,5 @@ > + > + try { > + test.call({complete: finish}); > + } catch (err) { > + dump(`WORKER ERROR :: ${err}\n`); It seems like in this case, you would want to fire the onerror event for the worker.
Attachment #8671345 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 82•9 years 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•9 years ago
|
||
I see, thanks for the hints. Will give it another try.
Assignee | ||
Comment 86•9 years ago
|
||
(In reply to Martin Thomson [:mt:] from comment #78) > ::: dom/crypto/test/test-array.js > @@ +106,5 @@ > > + > > + var base = new Test(name, test); > > + for (var method of ["draw", "setRow", "next", "complete"]) { > > + this[method] = base[method].bind(this); > > + } > > This is an odd way to do inheritance. Why not set the prototype? (I know > why, but a comment would help some). Done. (In reply to Richard Barnes [:rbarnes] from comment #79) > ::: dom/crypto/test/test-array.js > @@ +99,5 @@ > > + worker.postMessage(test.toSource()); > > + worker.onmessage = e => this.complete(e.data); > > This seems to assume that a message from the worker will always be a > boolean. It would help to have a comment to make this explicit. Added. > ::: dom/crypto/test/test-worker.js > @@ +38,5 @@ > > + > > + try { > > + test.call({complete: finish}); > > + } catch (err) { > > + dump(`WORKER ERROR :: ${err}\n`); > > It seems like in this case, you would want to fire the onerror event for the > worker. Yes, good idea.
Assignee | ||
Comment 87•9 years ago
|
||
r=mt,rbarnes
Attachment #8671345 -
Attachment is obsolete: true
Attachment #8709628 -
Flags: review+
Assignee | ||
Comment 88•9 years ago
|
||
Comment on attachment 8671762 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch Andrea, can you please take another look at this patch and esp. comment #70 and #75? AFAICT the content process uses /dev/urandom to properly seed the PRNG. We'd otherwise have a few problems with our WebRTC implementation.
Attachment #8671762 -
Flags: review?(amarchesini)
Comment 89•9 years ago
|
||
Comment on attachment 8671762 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch Review of attachment 8671762 [details] [diff] [review]: ----------------------------------------------------------------- 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•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #89) > r+ for the dom/worker side. Ask for a review to some NSS peer too. Will do, thanks. > Plus, I guess that the next step is to work on bug 1187998. Yeah, that's one of the ~3 assertions I'm hitting when running webcrypto tests in workers.
Assignee | ||
Comment 91•9 years ago
|
||
Comment on attachment 8671762 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch David, Martin, would you both mind talking a look (again)?
Attachment #8671762 -
Flags: review?(martin.thomson)
Attachment #8671762 -
Flags: review?(dkeeler)
Comment 92•9 years ago
|
||
Comment on attachment 8671762 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch Review of attachment 8671762 [details] [diff] [review]: ----------------------------------------------------------------- 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•9 years ago
|
||
Sorry to bother everyone again, but turns out I killed the PRNG. We have tests that caught it but I didn't run them on my machine... The thing is that the random generator wants to allocate memory itself, so we can't just pass a pointer to the array buffer data into it, but have to use memcpy() and free() as the code did before.
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)
Updated•9 years ago
|
Attachment #8709935 -
Flags: review?(amarchesini) → review+
![]() |
||
Comment 94•9 years ago
|
||
Comment on attachment 8709935 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v2 Review of attachment 8709935 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Crypto.cpp @@ +93,5 @@ > return; > } > > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > + do_GetService("@mozilla.org/security/random-generator;1"); I think two things need to happen for this to work: s/nssEnsure/nssEnsureChromeOrContent/ here: https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/security/manager/ssl/nsNSSModule.cpp#210 EnsureNSSInitializedChromeOrContent will have to do the dispatch-to-main-thread-and-wait scheme described in comment 83. @@ +102,2 @@ > > + uint8_t* buf; Maybe use a scoped type here? Having to manually call free is a bummer.
Attachment #8709935 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 95•9 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #94) > > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > > + do_GetService("@mozilla.org/security/random-generator;1"); > > I think two things need to happen for this to work: > > s/nssEnsure/nssEnsureChromeOrContent/ here: > https://dxr.mozilla.org/mozilla-central/rev/ > b67316254602a63bf4e568198a5c7d3288a9db27/security/manager/ssl/nsNSSModule. > cpp#210 Not doing this for now as discussed on IRC. The sync-init functionality will live in a WebCrypto helper function until we see the need to move it into EnsureNSSInitializedChromeOrContent(). > EnsureNSSInitializedChromeOrContent will have to do the > dispatch-to-main-thread-and-wait scheme described in comment 83. Good catch, thanks. > @@ +102,2 @@ > > > > + uint8_t* buf; > > Maybe use a scoped type here? Having to manually call free is a bummer. We can't do this unfortunately, GenerateRandomBytes() takes a uint8_t** and allocates memory.
Assignee | ||
Comment 96•9 years ago
|
||
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•9 years ago
|
||
This patch makes all crypto.subtle.* methods actually callable from workers. It uses the new maybe-sync EnsureNSSInitialized() once when the thread pool is initialized.
Attachment #8710154 -
Flags: review?(dkeeler)
Assignee | ||
Comment 98•9 years ago
|
||
This one's basically bug 1187998, I think it's easier to just do it in here. It removes the main thread assertions in the structured cloning helper and adds two tests, sending/receiving keys to/from workers and then encrypting and decrypting data. The dom/crypto/test/test_WebCrypto.html change is related as IndexedDB uses structured cloning too. The test needed to close the database on the main thread so that it didn't block the worker test running right afterwards.
Attachment #8710156 -
Flags: review?(dkeeler)
Attachment #8710156 -
Flags: review?(amarchesini)
Assignee | ||
Comment 99•9 years ago
|
||
Just to be clear, *most* of the tests succeed, but there's some JWK code and at least one assertion that I still need to fix/remove. That means that at least one more patch will follow.
Assignee | ||
Comment 100•9 years ago
|
||
Boris, we have a key wrapping test that fails when run in a worker. Specifically, we hit the assertion at [1] called by [2]. [1] https://hg.mozilla.org/mozilla-central/rev/9c67ef1f9515#l3.17 [2] https://hg.mozilla.org/mozilla-central/annotate/b67316254602/dom/crypto/WebCryptoTask.cpp#l2897 Can you please explain why the NS_IsMainThread() assertion is needed here and how we can get ToJSON() to run on a worker? Thanks!
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 101•9 years ago
|
||
Comment on attachment 8710146 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v3 Review of attachment 8710146 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with comments addressed. ::: dom/base/Crypto.cpp @@ +100,3 @@ > > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > + do_GetService("@mozilla.org/security/random-generator;1"); I'm pretty sure this still requires the nsNSSModule.cpp change. ::: dom/crypto/WebCryptoCommon.cpp @@ +23,5 @@ > + // let's fire a SyncRunnable to do this synchronously. > + if (!initialized && !NS_IsMainThread()) { > + nsCOMPtr<nsIThread> mainThread; > + nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread)); > + NS_ENSURE_SUCCESS(rv, false); nit: NSS_ENSURE_SUCCESS hides a return, so it might be more clear to do this: if (NS_FAILED(rv)) { return false; } (or even NS_WARN_IF(NS_FAILED(rv))) @@ +27,5 @@ > + NS_ENSURE_SUCCESS(rv, false); > + > + mozilla::SyncRunnable::DispatchToThread(mainThread, > + new SyncRunnable(NS_NewRunnableFunction([&]() -> void { > + initialized = EnsureNSSInitializedChromeOrContent(); If we ever return true here, we should cache the result so subsequent non-main thread callers don't even have to dispatch to the main thread.
Attachment #8710146 -
Flags: review?(dkeeler) → review+
![]() |
||
Comment 102•9 years ago
|
||
That code had the IsMainThread assertion because GetSafeJSContextGlobal() can only be called on the main thread. The code has changed a bit, but UnprivilegedJunkScope is also only allowed to be called on the main thread. We would need some worker-thread-safe equivalent, just like we need one in bug 1241349... Bobby, I can add some binding utility function that returns the junk scope on mainthread and the worker global on a worker thread; does that seem reasonable?
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
Comment 103•9 years ago
|
||
Comment on attachment 8710146 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v3 Review of attachment 8710146 [details] [diff] [review]: ----------------------------------------------------------------- So EnsureNSSInitializedChromeOrContent() always returns false off the main thread, so if I get this right, all WebCrypto functions would synchronously dispatch every time on worker threads? If that's true, then I think that David is right about this: cache the result in a static atomic variable or something so that you can avoid unnecessary thread thrashing. Otherwise this will substantially affect performance of the crypto APIs. ::: dom/crypto/WebCryptoCommon.cpp @@ +26,5 @@ > + nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread)); > + NS_ENSURE_SUCCESS(rv, false); > + > + mozilla::SyncRunnable::DispatchToThread(mainThread, > + new SyncRunnable(NS_NewRunnableFunction([&]() -> void { Nit: Don't capture everything, just [&initialized]() {...}. You don't need the arrow for a void lambdas (don't know what the style guide says there...) @@ +32,5 @@ > + })) > + ); > + } > + > + return initialized; I would put the caching just before the return rather than in the lambda. That means that a call to this on the main thread might prime the value for all other threads, but I think that's OK.
Attachment #8710146 -
Flags: review?(martin.thomson) → review+
Comment 104•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #102) > We would need some worker-thread-safe equivalent, just like we need one in > bug 1241349... Bobby, I can add some binding utility function that returns > the junk scope on mainthread and the worker global on a worker thread; does > that seem reasonable? FWIW, there was a similar issue recently with chrome workers instantiating FileReader, in bug 1240365.
Comment 105•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #102) > That code had the IsMainThread assertion because GetSafeJSContextGlobal() > can only be called on the main thread. > > The code has changed a bit, but UnprivilegedJunkScope is also only allowed > to be called on the main thread. > > We would need some worker-thread-safe equivalent, just like we need one in > bug 1241349... Bobby, I can add some binding utility function that returns > the junk scope on mainthread and the worker global on a worker thread; does > that seem reasonable? Sure.
Flags: needinfo?(bobbyholley)
Updated•9 years ago
|
Attachment #8710156 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 106•9 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #101) > ::: dom/base/Crypto.cpp > @@ +100,3 @@ > > > > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > > + do_GetService("@mozilla.org/security/random-generator;1"); > > I'm pretty sure this still requires the nsNSSModule.cpp change. Done. > ::: dom/crypto/WebCryptoCommon.cpp > @@ +23,5 @@ > > + // let's fire a SyncRunnable to do this synchronously. > > + if (!initialized && !NS_IsMainThread()) { > > + nsCOMPtr<nsIThread> mainThread; > > + nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread)); > > + NS_ENSURE_SUCCESS(rv, false); > > nit: NSS_ENSURE_SUCCESS hides a return, so it might be more clear to do this: > > if (NS_FAILED(rv)) { > return false; > } Ok, done. > @@ +27,5 @@ > > + NS_ENSURE_SUCCESS(rv, false); > > + > > + mozilla::SyncRunnable::DispatchToThread(mainThread, > > + new SyncRunnable(NS_NewRunnableFunction([&]() -> void { > > + initialized = EnsureNSSInitializedChromeOrContent(); > > If we ever return true here, we should cache the result so subsequent > non-main thread callers don't even have to dispatch to the main thread. I moved the EnsureNSSInitialized() call to the thread pool as the thread pool is initialized only once per process. And so this function would be called only once too. But you're right in that it would cause sync inits on every .getRandomValues() call from a worker in a content process. I missed that we check for PSM_COMPONENT_CONTRACTID only in the parent process, not in content.
Assignee | ||
Comment 107•9 years ago
|
||
(In reply to Martin Thomson [:mt:] from comment #103) > So EnsureNSSInitializedChromeOrContent() always returns false off the main > thread, so if I get this right, all WebCrypto functions would synchronously > dispatch every time on worker threads? Only .getRandomValues() would do that if called by a worker in a content process. But yes, that needs to be fixed too. > If that's true, then I think that David is right about this: cache the > result in a static atomic variable or something so that you can avoid > unnecessary thread thrashing. Otherwise this will substantially affect > performance of the crypto APIs. Done. > ::: dom/crypto/WebCryptoCommon.cpp > @@ +26,5 @@ > > + nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread)); > > + NS_ENSURE_SUCCESS(rv, false); > > + > > + mozilla::SyncRunnable::DispatchToThread(mainThread, > > + new SyncRunnable(NS_NewRunnableFunction([&]() -> void { > > Nit: Don't capture everything, just [&initialized]() {...}. You don't need > the arrow for a void lambdas (don't know what the style guide says there...) Replaced the call to EnsureNSSInitializedChromeOrContent() with a recursive call to EnsureNSSInitialized() to simply forward non-main-thread calls to the main thread. The lambda thus doesn't need to capture any variable anymore. > @@ +32,5 @@ > > + })) > > + ); > > + } > > + > > + return initialized; > > I would put the caching just before the return rather than in the lambda. > That means that a call to this on the main thread might prime the value for > all other threads, but I think that's OK. Restructured the function so that now the static atomic bool is read from any thread but only written to from the main thread. I think that's a little nicer than before.
Assignee | ||
Comment 108•9 years ago
|
||
r=baku,keeler,mt
Attachment #8710146 -
Attachment is obsolete: true
Attachment #8710403 -
Flags: review+
![]() |
||
Comment 109•9 years ago
|
||
> We would need some worker-thread-safe equivalent, just like we need one in bug 1241349 Except not; I'll discuss this a bit more at length in bug 1241349.
![]() |
||
Updated•9 years ago
|
Attachment #8710154 -
Flags: review?(dkeeler) → review+
![]() |
||
Comment 110•9 years ago
|
||
Comment on attachment 8710156 [details] [diff] [review] 0005-Bug-842818-Enable-structured-cloning-for-CryptoKeys-.patch Review of attachment 8710156 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/test/test_WebCrypto_Workers.html @@ +1,1 @@ > +<!DOCTYPE html> Would it be worth it to also run all of the other webcrypto tests in a worker?
Attachment #8710156 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 111•9 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #110) > Comment on attachment 8710156 [details] [diff] [review] > 0005-Bug-842818-Enable-structured-cloning-for-CryptoKeys-.patch > > Review of attachment 8710156 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/crypto/test/test_WebCrypto_Workers.html > @@ +1,1 @@ > > +<!DOCTYPE html> > > Would it be worth it to also run all of the other webcrypto tests in a > worker? Yes, definitely. That's what part 2 (attachment #8709628 [details] [diff] [review]) is about :)
Assignee | ||
Comment 112•9 years ago
|
||
This patch removes the NS_IsMainThread() assertion from the CloneData() helper method. Richard, you added this back when you landed the initial WebCrypto patches. I talked to :till and there seems to be no reason for CloneData() to only work on the main thread. I assume you added this only to ensure SetKeyData() is called from the main thread only? Which is a fair assumption in our current threading model.
Attachment #8716246 -
Flags: review?(rlb)
Comment 113•9 years ago
|
||
Comment on attachment 8716246 [details] [diff] [review] 0006-Bug-842818-Remove-NS_IsMainThread-assertion-from-Clo.patch Review of attachment 8716246 [details] [diff] [review]: ----------------------------------------------------------------- So I'm not entirely sure what the right answer is, but I don't think this is it :) On the one hand, I seem to recall CloneData being used for other things in WebCrypto. It's an implementation of the "clone the data" algorithm [1], but AFAICT it's only used in the algorithm normalization process [2]. I think our normalization code handles this part of the process with ATTEMPT_BUFFER_INIT, though. On the other hand, if CloneData really shouldn't be used anywhere besides SetKeyData, we should probably just inline it there. Could you please take a look at the spec and see if there are other places we should be using CloneData, and if not, then kill it and put it inline in SetKeyData? [1] https://www.w3.org/TR/WebCryptoAPI/#terminology [2] https://www.w3.org/TR/WebCryptoAPI/#algorithm-normalization-normalize-an-algorithm
Attachment #8716246 -
Flags: review?(rlb) → review-
Comment 114•9 years ago
|
||
Might I ask a potentially unrelated question? Can this bug #1250930 be a consequence of this current bug not being resolved? Sandboxes, workers, it's kind of related, right?
Assignee | ||
Comment 115•9 years ago
|
||
(In reply to wyohknott from comment #114) > Can this bug #1250930 be a consequence of this current bug not being > resolved? Sandboxes, workers, it's kind of related, right? Good question. I'll take a look at it and will respond in the bug.
![]() |
||
Comment 116•9 years ago
|
||
> Sandboxes, workers, it's kind of related, right?
I don't think it actually is, for what it's worth.
Assignee | ||
Comment 117•9 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #113) > On the one hand, I seem to recall CloneData being used for other things in > WebCrypto. It's an implementation of the "clone the data" algorithm [1], > but AFAICT it's only used in the algorithm normalization process [2]. I > think our normalization code handles this part of the process with > ATTEMPT_BUFFER_INIT, though. Yeah, we don't really use CloneData where the spec defines it, as you mention above. Inlining CloneData is good but there's a little more we can do. The import task initialization routines (constructor, Init, SetKeyData) are quite confusing. I checked the call sites and here's what I did: 1) Inlined CloneData() into SetKeyData(). The function behaves slightly differently now as I removed the SetJwkFromKeyData() calls, The only four places that call it are the Import*KeyTask constructors that take a keyData argument, that means those are from actual crypto.subtle.importKey() calls. (The "parse a JWK" algorithm as implemented by SetJwkFromKeyData() should only be used for unwrapKey.) 2) Added SetRawKeyData() that supports only raw key data, called by DeriveKeyTask::Resolve(). 3) Added SetKeyDataMaybeParseJWK() that accepts a CryptoBuffer argument and has the inlined code from SetJwkFromKeyData(). This is only called from UnwrapKeyTask::Resolve() and implements the "parse a JWK" algorithm as described in the spec.
Attachment #8716246 -
Attachment is obsolete: true
Attachment #8728846 -
Flags: review?(rlb)
Comment 118•9 years ago
|
||
Comment on attachment 8728846 [details] [diff] [review] 0006-Bug-842818-Inline-CloneData-and-clean-up-ImportKeyTa.patch, v2 Review of attachment 8728846 [details] [diff] [review]: ----------------------------------------------------------------- r=me modulo the test question ::: dom/crypto/test/test_WebCrypto_Wrap_Unwrap.html @@ -178,5 @@ > ); > > // ----------------------------------------------------------------------------- > TestArray.addTest( > - "JWK wrap/unwrap round-trip, with AES-GCM", Why are we removing this test? Seems unrelated to this patch.
Attachment #8728846 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 119•9 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #118) > > // ----------------------------------------------------------------------------- > > TestArray.addTest( > > - "JWK wrap/unwrap round-trip, with AES-GCM", > > Why are we removing this test? Seems unrelated to this patch. Oops, that accidentally found its way into the patch. Removed it only for testing while bz is looking into it, will resurrect.
Assignee | ||
Comment 120•9 years ago
|
||
r=rbarnes
Attachment #8728846 -
Attachment is obsolete: true
Attachment #8731621 -
Flags: review+
Assignee | ||
Comment 121•9 years ago
|
||
This patch removes testing/web-platform/meta/WebCryptoAPI/getRandomValues.worker.js.ini and thereby re-enables the WPT for crypto.getRandomValues() on workers.
Attachment #8738177 -
Flags: review?(james)
Assignee | ||
Comment 122•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8738177 -
Flags: review?(james) → review+
Attachment #8738178 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 123•9 years ago
|
||
Sorry, forgot to update test_worker_interfaces.js.
Attachment #8738178 -
Attachment is obsolete: true
Attachment #8738217 -
Flags: review?(bugs)
Attachment #8738217 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 124•9 years ago
|
||
Had to revise this patch once again due to WPT failures with workers in e10s. If we try to create an instance of nsRandomGenerator from a non-main thread in a content process this fails because NS_NSS_GENERIC_FACTORY_CONSTRUCTOR still calls EnsureNSSInitializedChromeOrContent() that then returns false. This patch now introduces nssEnsureChromeOrContentSync, used only for nsRandomGenerator, that when instantiated initializes NSS properly - no matter the thread or process it's called from. The "Sync" part indicates that it may block the main thread while doing so because we have to use a SyncRunnable. I mostly moved the code from v4 around, so the mechanisms stay the same, they're just called from different places.
Attachment #8710403 -
Attachment is obsolete: true
Attachment #8738730 -
Flags: review?(dkeeler)
Assignee | ||
Comment 125•9 years ago
|
||
Small amendment.
Attachment #8738730 -
Attachment is obsolete: true
Attachment #8738730 -
Flags: review?(dkeeler)
Attachment #8738737 -
Flags: review?(dkeeler)
Assignee | ||
Comment 126•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=739569a91ef9
![]() |
||
Comment 127•9 years ago
|
||
Comment on attachment 8738737 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v6 Review of attachment 8738737 [details] [diff] [review]: ----------------------------------------------------------------- I think I was actually wrong the other day when I said it wouldn't be a good idea to put the dispatch-to-main-thread-and-wait code in EnsureNSSInitializedChromeOrContent itself. It should be ok to do that, and it will simplify the code changes here. ::: dom/base/Crypto.cpp @@ -100,5 @@ > - if (!XRE_IsParentProcess()) { > - InfallibleTArray<uint8_t> randomValues; > - // Tell the parent process to generate random values via PContent > - ContentChild* cc = ContentChild::GetSingleton(); > - if (!cc->SendGetRandomValues(dataLen, &randomValues) || I think we still need this - currently NSS initialization in the child process doesn't cause any entropy to be collected, if I'm reading things correctly. ::: security/manager/ssl/nsNSSComponent.cpp @@ +199,5 @@ > + } > + > + // Forward to the main thread synchronously. > + bool initialized = false; > + mozilla::SyncRunnable::DispatchToThread(mainThread, Now that I think about this more, it should be safe to move this into EnsureNSSInitializedChromeOrContent and have non-main threads dispatch to the main thread and wait (particularly since all current callers are main-thread anyway). Let's be sure and document it, though. Also, we should cache the result for subsequent calls to avoid an unnecessary dispatch.
Attachment #8738737 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 128•9 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #127) > I think I was actually wrong the other day when I said it wouldn't be a good > idea to put the dispatch-to-main-thread-and-wait code in > EnsureNSSInitializedChromeOrContent itself. It should be ok to do that, and > it will simplify the code changes here. Done. > ::: dom/base/Crypto.cpp > @@ -100,5 @@ > > - if (!XRE_IsParentProcess()) { > > - InfallibleTArray<uint8_t> randomValues; > > - // Tell the parent process to generate random values via PContent > > - ContentChild* cc = ContentChild::GetSingleton(); > > - if (!cc->SendGetRandomValues(dataLen, &randomValues) || > > I think we still need this - currently NSS initialization in the child > process doesn't cause any entropy to be collected, if I'm reading things > correctly. As per result of the NSS call and discussion on IRC earlier today we should postpone this and tackle/remove feeding entropy to NSS in a follow-up. (Bug 1263015) > ::: security/manager/ssl/nsNSSComponent.cpp > @@ +199,5 @@ > > + } > > + > > + // Forward to the main thread synchronously. > > + bool initialized = false; > > + mozilla::SyncRunnable::DispatchToThread(mainThread, > > Now that I think about this more, it should be safe to move this into > EnsureNSSInitializedChromeOrContent and have non-main threads dispatch to > the main thread and wait (particularly since all current callers are > main-thread anyway). Let's be sure and document it, though. Also, we should > cache the result for subsequent calls to avoid an unnecessary dispatch. Done.
Attachment #8738737 -
Attachment is obsolete: true
Attachment #8739242 -
Flags: review?(dkeeler)
![]() |
||
Comment 129•9 years ago
|
||
Comment on attachment 8739242 [details] [diff] [review] 0003-Bug-842818-Make-Crypto-GetRandomValues-work-off-the-.patch, v7 Review of attachment 8739242 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8739242 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 130•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cb1b2946566
Comment 131•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a98b4fa6de54 https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcb400c1a48 https://hg.mozilla.org/integration/mozilla-inbound/rev/f83f79281511 https://hg.mozilla.org/integration/mozilla-inbound/rev/fdded53dc92f https://hg.mozilla.org/integration/mozilla-inbound/rev/9262f09b3322 https://hg.mozilla.org/integration/mozilla-inbound/rev/17e4c84b299d https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe4eae85573 https://hg.mozilla.org/integration/mozilla-inbound/rev/deef69da0f29
Comment 132•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a98b4fa6de54 https://hg.mozilla.org/mozilla-central/rev/ffcb400c1a48 https://hg.mozilla.org/mozilla-central/rev/f83f79281511 https://hg.mozilla.org/mozilla-central/rev/fdded53dc92f https://hg.mozilla.org/mozilla-central/rev/9262f09b3322 https://hg.mozilla.org/mozilla-central/rev/17e4c84b299d https://hg.mozilla.org/mozilla-central/rev/4fe4eae85573 https://hg.mozilla.org/mozilla-central/rev/deef69da0f29
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 133•9 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: long-standing request by web devs [Suggested wording]: Web Crypto API is now available in Workers [Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto
relnote-firefox:
--- → ?
You need to log in
before you can comment on or make changes to this bug.
Description
•