Closed Bug 582297 Opened 14 years ago Closed 9 years ago

e10s: forward JS 'crypto' calls from content->chrome (including <keygen>)

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m4+ ---
blocking2.0 --- -

People

(Reporter: dougt, Assigned: mrbkap)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

+++ 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.
Keywords: checkin-needed
tracking-fennec: 2.0a1+ → 2.0+
This isn't an issue for Firefox 4, but yeah, should be fixed for Fennec.
blocking2.0: ? → -
Assignee: nobody → mkristoffersen
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
this is the FF bug.
Status: RESOLVED → REOPENED
tracking-fennec: 2.0+ → ---
Resolution: DUPLICATE → ---
Assignee: mkristoffersen → nobody
Why is this fennec/FF specific?
I guess this is also the reason why window.crypto is giving a js error in Fennec, right?
Yep.
Blocks: 673432
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.
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.
Component: DOM: Other → DOM
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)
I'll make it happen.
Flags: needinfo?(josh)
This bug is unrelated to bug 926547.
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 ;_;
Blocks: 901848
Assignee: nobody → mrbkap
Priority: -- → P2
We can probably just close this. All the legacy crypto functions have been removed in bug 1030963. I am not sure about <keygen>?
<keygen> (in the HTML spec) is not going anywhere.
Just to confirm, StartSSL is broken with e10s due to <keygen> not working. Hit this bug today.
Moving old M2 P2 bugs to M4.
Move old M2's low-priority bugs to M6 milestone.
oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.
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+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=feacc6bbdf74

It turned out that there were quite a few tests disabled for <keygen> on e10s.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a4e5137c55

I'll file a followup on the rest of the crypto API.
https://hg.mozilla.org/mozilla-central/rev/b8a4e5137c55
Status: REOPENED → RESOLVED
Closed: 14 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Target Milestone: mozilla37 → mozilla36
I filed bug 1113313 because I forgot to rename the IPDL functions.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.