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

Status: RESOLVED FIXED

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

Description

[Doug Turner (:dougt)]

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

Comment 1

[Johnny Stenback (:jst)]

This isn't an issue for Firefox 4, but yeah, should be fixed for Fennec.

Comment 3

[Doug Turner (:dougt)]

this is the FF bug.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Why is this fennec/FF specific?

Comment 6

[Martijn Wargers (dead)]

I guess this is also the reason why window.crypto is giving a js error in Fennec, right?

Comment 7

[Josh Matthews [:jdm]]

Yep.

Comment 8

[Josh Matthews [:jdm]]

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.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Now that Jesse's fuzzers are hitting this, comment 8 sounds like a reasonable short-term band-aid.

Comment 12

[Andrew Overholt [:overholt]]

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

Comment 13

[Josh Matthews [:jdm]]

I'll make it happen.

Comment 14

[Josh Matthews [:jdm]]

This bug is unrelated to bug 926547.

Comment 15

[Josh Matthews [:jdm]]

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

Comment 18

[Tom S [:evilpie]]

We can probably just close this. All the legacy crypto functions have been removed in bug 1030963. I am not sure about <keygen>?

Comment 19

[Florian Bender]

<keygen> (in the HTML spec) is not going anywhere.

Comment 20

[Anne (:annevk)]

Just to confirm, StartSSL is broken with e10s due to <keygen> not working. Hit this bug today.

Comment 21

[Chris Peterson [:cpeterson]]

Moving old M2 P2 bugs to M4.

Comment 22

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 23

[Chris Peterson [:cpeterson]]

oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make <keygen> work in e10s.

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.

Comment 25

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

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

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

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

It turned out that there were quite a few tests disabled for <keygen> on e10s.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a4e5137c55

I'll file a followup on the rest of the crypto API.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8a4e5137c55

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 1113313 because I forgot to rename the IPDL functions.