Closed
Bug 582297
Opened 14 years ago
Closed 10 years ago
e10s: forward JS 'crypto' calls from content->chrome (including <keygen>)
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: dougt, Assigned: mrbkap)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
keeler
:
review+
billm
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #561244 +++ We're turning off NSS in the content process. We thus need to forward any calls to https://developer.mozilla.org/en/JavaScript_crypto to chrome. In the previsiou bug, we removed the crypto object from the namespace. This bug is to track the work to correctly forward the calls.
Updated•14 years ago
|
Keywords: checkin-needed
Updated•14 years ago
|
tracking-fennec: 2.0a1+ → 2.0+
Comment 1•14 years ago
|
||
This isn't an issue for Firefox 4, but yeah, should be fixed for Fennec.
blocking2.0: ? → -
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → mkristoffersen
Reporter | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 3•14 years ago
|
||
this is the FF bug.
Status: RESOLVED → REOPENED
tracking-fennec: 2.0+ → ---
Resolution: DUPLICATE → ---
Reporter | ||
Updated•14 years ago
|
Assignee: mkristoffersen → nobody
Why is this fennec/FF specific?
Comment 6•14 years ago
|
||
I guess this is also the reason why window.crypto is giving a js error in Fennec, right?
Comment 8•13 years ago
|
||
How do we feel about a patch that brings back the crypto object but just returns NS_ERROR_FAILURE for all existing methods if in a non-chrome process? David's work in bug 673432 apparently is urgently required by the identity team.
Updated•13 years ago
|
Summary: e10s: forward JS 'crypto' calls from content->chrome → e10s: forward JS 'crypto' calls from content->chrome (including <keygen>)
Now that Jesse's fuzzers are hitting this, comment 8 sounds like a reasonable short-term band-aid.
Updated•12 years ago
|
Component: DOM: Other → DOM
Comment 12•11 years ago
|
||
Is this bug on someone's backlog? jdm (comment #8)? The reason I ask is because it's affecting b2g debug builds (specifically bug 926547).
Flags: needinfo?(josh)
Comment 15•11 years ago
|
||
Er, nevermind. If we go by the original request in the summary, it's related. Comment 8 is unrelated, and the original request is much more work ;_;
Updated•11 years ago
|
tracking-e10s:
--- → +
Updated•11 years ago
|
Blocks: old-e10s-m2
Updated•10 years ago
|
Assignee: nobody → mrbkap
Priority: -- → P2
Comment 18•10 years ago
|
||
We can probably just close this. All the legacy crypto functions have been removed in bug 1030963. I am not sure about <keygen>?
Comment 20•10 years ago
|
||
Just to confirm, StartSSL is broken with e10s due to <keygen> not working. Hit this bug today.
Assignee | ||
Comment 24•10 years ago
|
||
This is straightforward, I simply forward the keygen interface through IPDL. I cleaned up a bit of trailing whitespace as I went (as well as a couple of other minor cleanups). I think there are other APIs to deal with, which I'll get to in a bit.
Attachment #8527079 -
Flags: review?(wmccloskey)
Attachment #8527079 -
Flags: review?(dkeeler)
Comment on attachment 8527079 [details] [diff] [review] Make <keygen> work in e10s. Review of attachment 8527079 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with nits addressed (I only looked at the PSM parts). ::: security/manager/ssl/src/nsKeygenHandler.cpp @@ +465,5 @@ > } > > nsresult > +nsKeygenFormProcessor::GetPublicKey(const nsAString& aValue, const nsAString& aChallenge, > + const nsAFlatString& aKeyType, nsAString& aOutPublicKey, Actually, can you wrap these lines so they're <80 chars? @@ +794,5 @@ > aElement->GetAttribute(NS_LITERAL_STRING("challenge"), challengeValue); > +} > + > +nsresult > +nsKeygenFormProcessor::ProcessValue(nsIDOMHTMLElement *aElement, nit: nsIDOMHTMLElement* aElement ::: security/manager/ssl/src/nsKeygenHandler.h @@ +25,3 @@ > nsresult Init(); > > + virtual nsresult ProcessValue(nsIDOMHTMLElement *aElement, nit: nsIDOMHTMLElement* aElement ::: security/manager/ssl/src/nsKeygenHandlerContent.cpp @@ +5,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsKeygenHandlerContent.h" > +#include "nsIFormProcessor.h" nit: as far as I understand the #include guidelines, nsKeygenHandlerContent.h would be first, then a newline, then the other two in alphabetical order @@ +22,5 @@ > +{ > +} > + > +nsresult > +nsKeygenFormProcessorContent::ProcessValue(nsIDOMHTMLElement *aElement, nit: nsIDOMHTMLElement* aElement @@ +33,5 @@ > + nsKeygenFormProcessor::ExtractParams(aElement, challengeValue, keyTypeValue, keyParamsValue); > + > + ContentChild* child = ContentChild::GetSingleton(); > + if (!child) > + return NS_ERROR_FAILURE; nit: braces around conditional bodies Also, two-space indents would be appreciated. @@ +62,5 @@ > + nsTArray<nsString>& aContent, > + nsAString& aAttribute) > +{ > + nsString attribute; > + unused << ContentChild::GetSingleton()->SendFormProvideContent(&attribute, &aContent); Is it possible for GetSingleton to fail here (as it appears to be the case in ProcessValue)? If so, we should null-check it first here as well. ::: security/manager/ssl/src/nsKeygenHandlerContent.h @@ +5,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef _NSKEYGENHANDLERCONTENT_H_ > +#define _NSKEYGENHANDLERCONTENT_H_ nit: my understanding of the #include guard guidelines is that this would be 'nsKeygenHandlerContent_h' @@ +11,5 @@ > +class nsKeygenFormProcessorContent : public nsIFormProcessor { > +public: > + nsKeygenFormProcessorContent(); > + > + virtual nsresult ProcessValue(nsIDOMHTMLElement *aElement, nit: nsIDOMHTMLElement* aElement
Attachment #8527079 -
Flags: review?(dkeeler) → review+
Comment on attachment 8527079 [details] [diff] [review] Make <keygen> work in e10s. Review of attachment 8527079 [details] [diff] [review]: ----------------------------------------------------------------- Do we have tests for this stuff? It doesn't seem like it would be too hard to make one (or enable an existing one for e10s). ::: dom/ipc/PContent.ipdl @@ +786,5 @@ > + * Keygen requires us to call it after a <keygen> element is parsed and > + * before one is submitted. This is urgent because an extension might use > + * a CPOW to synchronously submit a keygen element. > + */ > + prio(urgent) sync FormProcessValue(nsString oldValue, The names FormProcessValue and FormProvideContent aren't very descriptive. Is there anything better we can use? The FormProcessValue code calls GetPublicKey, which seems like a much better name. For the other one maybe GetKeyList (if that's what it does; I have no idea). ::: security/manager/ssl/src/nsKeygenHandler.cpp @@ +269,5 @@ > > nsresult > nsKeygenFormProcessor::Create(nsISupports* aOuter, const nsIID& aIID, void* *aResult) > { > + if (GeckoProcessType_Default != XRE_GetProcessType()) { Perhaps check specifically for Type_Content here since that's all we really support (because everything is over PContent). ::: security/manager/ssl/src/nsKeygenHandlerContent.cpp @@ +32,5 @@ > + nsAutoString keyParamsValue; > + nsKeygenFormProcessor::ExtractParams(aElement, challengeValue, keyTypeValue, keyParamsValue); > + > + ContentChild* child = ContentChild::GetSingleton(); > + if (!child) Should be able to remove this with the change above.
Attachment #8527079 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 27•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=feacc6bbdf74 It turned out that there were quite a few tests disabled for <keygen> on e10s.
Assignee | ||
Comment 28•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a4e5137c55 I'll file a followup on the rest of the crypto API.
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b8a4e5137c55
Status: REOPENED → RESOLVED
Closed: 14 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•10 years ago
|
Target Milestone: mozilla37 → mozilla36
Assignee | ||
Comment 30•10 years ago
|
||
I filed bug 1113313 because I forgot to rename the IPDL functions.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•