673432 - Implement window.crypto.getRandomValues for FirefoxOS

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[David Dahl :ddahl]

In bug 440046, the whatwg proposed "window.crypto.getRandomValues" is being implemented. Need to implement this for mobile as well.

See: http://wiki.whatwg.org/wiki/Crypto#getRandomValues

Comment 1

[David Dahl :ddahl]

So this bug has a bigger scope apparently: http://img717.imageshack.us/img717/9330/selection029v.png

There is no crypto object in place at all on the contentWindow. I'm thinking I will have to dig into how to create a placeholder window property that only contains getRandomValues.

So we will end up with a somewhat bare crypto object:

window {
 interface crypto {
    void getRandomValues(inout ArrayBufferView)
  }
}

Are there any existing plans or patches that add the crypto object to window in fennec?

Comment 2

[David Dahl :ddahl]

Attachment: v 0.1 WIP, non-working

Saving work, I have based this patch on jdm's patch here: https://bug561244.bugzilla.mozilla.org/attachment.cgi?id=441447

Comment 3

[David Dahl :ddahl]

A wrinkle: We need jsval support in e10s message passing for this to work properly, as it is synchronous.

bsmedberg said it may be simpler and better to implement a window.crypto wrapper in JS that uses the messageManager to talk to nsIDOMCrypto.GetRandomValues (bug 440046)

Either way, not having jsvals is a probelm. I think bug 592017 blocks this. Maybe there is another way?

Comment 4

[Josh Matthews [:jdm]]

I do not believe that sending jsvals over the wire is a prerequisite to solving this bug. While I can see from a code-cleanliness point of view that it would be nice to simply execute the entirety of getRandomValues in the parent process, it is entirely possible to simplh perform the PK11_GenerateRandom call in the parent and pass a PRUint8 nsTArray back and forth, from which you can easily memcpy into the typed array buffer.

Comment 5

[David Dahl :ddahl]

(In reply to comment #4)
> from which you can easily memcpy into the typed array buffer.

I tried this approach (for desktop firefox) in bug 440046 and was r-'d in a feedback pass as the allocation and memcpy is considered extravagant when this is called in a loop 50000 times. Also, the WebKit implementation does not allocate, and is quite efficient.

Comment 6

[Josh Matthews [:jdm]]

Furthermore, bug 592017 is not the bug for serializing jsval values over IPDL. I don't think that is something we want to do.

Comment 7

[David Dahl :ddahl]

(In reply to comment #6)
> Furthermore, bug 592017 is not the bug for serializing jsval values over
> IPDL. I don't think that is something we want to do.

Is there a bug for serializing jsvals over IPDL?

Comment 8

[Josh Matthews [:jdm]]

Again, I don't believe we want to try to go down the route of serializing arbitrary jsvals. Instead, the jsval should be converted to some known type that can be serialized - in this instance, we could look into converting the typed array buffer into an nsTArray or nsCString, or perhaps creating a new TypedArrayBuffer type that IPDL understands to avoid extraneous memcopies.

Comment 9

[David Dahl :ddahl]

(In reply to comment #8)
> Again, I don't believe we want to try to go down the route of serializing
> arbitrary jsvals. Instead, the jsval should be converted to some known type
> that can be serialized - in this instance, we could look into converting the
> typed array buffer into an nsTArray or nsCString, or perhaps creating a new
> TypedArrayBuffer type that IPDL understands to avoid extraneous memcopies.

in which case, it sounds like we could not implement the crypto object as a getter added to the window in JS, correct?

Comment 10

[Josh Matthews [:jdm]]

If you sent a basic string/array over the wire, you could. Or if we had some way of differentiating a typed array from a regular JSObject, we could write specific serialization code for that.

Comment 11

[David Dahl :ddahl]

(In reply to comment #10)
> If you sent a basic string/array over the wire, you could. Or if we had some
> way of differentiating a typed array from a regular JSObject, we could write
> specific serialization code for that.

Perhaps this is a dumb question, but, I'm not sure how the end result of serializing a string or array to the chrome process would modify the existing chunk of memory that the original ArrayBufferView represented.

Comment 12

[Josh Matthews [:jdm]]

With regards to the message manager, there are not in/out parameters. Instead, in this instance you could just not pass anything over the wire to chrome, it could generate a string of randomness and send that back, and the result would be placed in the typed array.

Comment 13

[Josh Matthews [:jdm]]

Alternatively, if straight IPDL is used, then the serialization code can be written to copy the result from the IPDL message directly into the buffer.

Comment 14

[David Dahl :ddahl]

Just had a discussion with bsmedberg about whether we can help allocating in a method like this due to the fact that we do not allow NSS to run in a content process, and regardless of serialization, we will always allocate via IDPL  or messageManager.


<bsmedberg> ddahl: can you explain what the data load is for crypto.getRandomValues?
<bsmedberg> I don't quite understand why we're worrying about allocation costs, for example
<ddahl> bsmedberg: yeah - the original patch I put up used nsIRandomGenerator and allocated, but that was feeback- because of allocation. So i started using PK11_GenerateRandom. bsmith will eventually write a non-allocating api like nsIGenerateRandom
<ddahl> the fear is that a user of the API will run it in a loop 50000 times
<bsmedberg> ddahl: we have to allocate to pass things across the process boundary anyway...
<bsmedberg> so either 1) the concern is valid and we'd have to generate the randomness in the content process
<bsmedberg> or 2) the concern isn't valid and we should just pass the values as strings
<ddahl> bsmedberg: Ok, I will cc bsmith and adam barth to see what they think as well. perhaps we can come to a conclusion that way?
<bsmedberg> ddahl: note that currently we prevent NSS from being initialized in the content process
<ddahl> bsmedberg: yep, seems like a good policy - but i'm not sure. 
<bsmedberg> we could perhaps modify that policy in this case, as long as random data doesn't require touching files, for instance
<bsmedberg> but we definitely cannot touch the cert store or anything like that
<ddahl> bsmedberg: bsmith will know for sure 
<bsmedberg> ddahl: note that since this involves a synchronous RPC message, it will be rate limited by the time it takes to process messages anyway
<bsmedberg> and I'm allergic to prematurely optimizing something that looks quite simple

CCing bsmith and adam barth on this discussion.

Comment 15

[Adam Barth]

+1 to avoiding premature optimization.  We just didn't want to be locked into an API that required allocation because that would prevent future optimizations.

Comment 16

[David Dahl :ddahl]

Currently, in nsGlobalWindow.cpp, we do:

NS_IMETHODIMP
nsGlobalWindow::GetCrypto(nsIDOMCrypto** aCrypto)
{
#ifdef MOZ_DISABLE_DOMCRYPTO
  return NS_ERROR_NOT_IMPLEMENTED;
#else

  FORWARD_TO_OUTER(GetCrypto, (aCrypto), NS_ERROR_NOT_INITIALIZED);


to avoid using nsCrypto in the content process. I would like to replace this NS_ERROR_NOT_IMPLEMENTED behavior via a content script that adds a 'crypto' getter to the window. What is the clean way of making GetCrypto a noop in e10s land?

Comment 17

[David Dahl :ddahl]

Attachment: v 0.2 JS Patch, restores window.crypto + getRandomValues via messageManager

This patch restores window.crypto (all but getRandomValues return NS_ERROR_NOT_IMPLEMENTED) and shims in getRandomValues via the messageManager. Tests are being written next. There is a LEAK and I am hoping jdm might know what is leaking. My money is on an eventListener:)

Comment 18

[Josh Matthews [:jdm]]

My suspicion is that the leak is from the window object, which is passed as an (unused) argument to the crypto object in the content script. Event listeners are cleaned up automatically, so that's not the problem.

Comment 19

[David Dahl :ddahl]

(In reply to Josh Matthews [:jdm] from comment #18)
> My suspicion is that the leak is from the window object, which is passed as
> an (unused) argument to the crypto object in the content script. Event
> listeners are cleaned up automatically, so that's not the problem.

Ok, I actually removed the aWindow argument from the createCrypto() function, and I still have a leak. I wonder if I need to remove the crypto object from each window on dom-window-destroyed  - if so, is there a "clearinghouse" observer for such things in mobile/ ?

Comment 20

[David Dahl :ddahl]

Interesting issue here. I was testing manually with a fairly simple test using a single call to window.crypto.getRandomValues...

Now that I am working on the mochitests - i see that we always pass the first call to the method hosted by window.crypto, but any subsequent call throws:

[Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMWindow.crypto]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: http://127.0.0.1/dev/window.crypto.js :: load :: line 15"  data: no]

The code that throws is still indeed in nsCrypto. I apparently override it the first time getRandomValues is called. Obviously, I need to do something smart here as both regular Firefox and Fennec need to do this differently. Should I just have nsGlobalWindow::GetCrypto return null and not throw? Or return an empty JS object. Not sure what to do here. I am also wondering why this passes the first call to getRandomValues, but fails on subsequent calls.

Comment 21

[Alon Zakai (:azakai)]

If your goal is to just implement

window {
 interface crypto {
    void getRandomValues(inout ArrayBufferView)
  }
}

then my recommendation is to use nsIDOMGlobalPropertyInitializer, the messageManager, and the wakeupService.

More specifically,

* nsIDOMGlobalPropertyInitializer lets you define a JS component that is run when a property is accessed on a window object (and it doesn't exist). See existing code in ConsoleAPI.js for window.console, and bug 669240 is almost ready to land with similar code for window.InstallTrigger (the "Part 2" patch there). Basically the component creates the object that is returned when window.crypto is accessed.

* You can then remote the call to getRandomValues using the messageManager. Probably just tell it the type of random values and how many, something like that I think?

* Using the wakeup service (see bug 591052), you can define a component to be loaded only when a specific messagemanager message arrives. So you would create the parent-process part of this as such a component, and it can be loaded when it is actually needed. It would then generate the random numbers and return them over the messagemanager.

Note that all of this should work in the single-process case too. That is, this code can replace the (frontend part of the) current window.crypto.getRandomValues code.

Comment 22

[David Dahl :ddahl]

(In reply to Alon Zakai (:azakai) from comment #21)

> 
> * Using the wakeup service (see bug 591052), you can define a component to
> be loaded only when a specific messagemanager message arrives. So you would
> create the parent-process part of this as such a component, and it can be
> loaded when it is actually needed. It would then generate the random numbers
> and return them over the messagemanager.

Cool. I did not know about the wakeup service.

> 
> Note that all of this should work in the single-process case too. That is,
> this code can replace the (frontend part of the) current
> window.crypto.getRandomValues code.

Should I do anything at all about nsGlobalWindow::GetCrypto? Or just ignore it for now?

Comment 23

[Benjamin Smedberg]

If this were implemented in the message manager, you would remove it from the IDL interface and there wouldn't be a nsGlobalWindow::GetCrypto, I'm pretty sure.

Comment 24

[David Dahl :ddahl]

Attachment: v 0.3 Taking a new approach - WIP

Just need to figure out how to remove the crypto property from the window so we can add it back in via nsIDOMGlobalPropertyInitializer. This patch is quite a ways along. Am getting some NSS errors now:
nsCrypto.cpp
/home/ddahl/code/moz/mozilla-mobile/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp: In member function ‘nsresult nsNSSComponent::DispatchEventToWindow(nsIDOMWindow*, const nsAString_internal&, const nsAString_internal&)’:
/home/ddahl/code/moz/mozilla-mobile/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:555:16: error: ‘class nsIDOMWindow’ has no member named ‘GetCrypto’

Comment 25

[David Dahl :ddahl]

Ok, not sure if I have to implement certain methods in my wakeup-service component:

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "getHelperForLanguage"' when calling method: [nsIClassInfo::getHelperForLanguage]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: http://127.0.0.1/dev/testGetRandomVals.html :: <TOP_LEVEL> :: line 7"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "getInterfaces"' when calling method: [nsIClassInfo::getInterfaces]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: http://127.0.0.1/dev/testGetRandomVals.html :: <TOP_LEVEL> :: line 7"  data: no]
************************************************************

Comment 26

[Josh Matthews [:jdm]]

Sounds like you need to implement nsIClassInfo.

Comment 27

[David Dahl :ddahl]

(In reply to Alon Zakai (:azakai) from comment #21)
> If your goal is to just implement
> 
> window {
>  interface crypto {
>     void getRandomValues(inout ArrayBufferView)
>   }
> }
> 
> then my recommendation is to use nsIDOMGlobalPropertyInitializer, the
> messageManager, and the wakeupService.

> * Using the wakeup service (see bug 591052), you can define a component to
> be loaded only when a specific messagemanager message arrives. So you would
> create the parent-process part of this as such a component, and it can be
> loaded when it is actually needed. It would then generate the random numbers
> and return them over the messagemanager.
> 

The question I have now is what needs to be in the manifest for this line: http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageWakeupService.idl#52

Right now I have: category wakeup-request CryptoManager @mozilla.org/crypto-manager;1,nsIMessageWakeupService,getService,crypto:getRandomValues

I also tried to do: category wakeup-request CryptoManager @mozilla.org/crypto-manager;1,nsICryptoManager,getService,crypto:getRandomValues

but, when I tried to add an idl for the CryptoManager component, I got a a build error about not finding nsIMessageWakeupService.idl - I thought I would not need an IDL for a wakeup-request category component

Anyway, the problem appears to be an ASSERT when trying to load my wakeup-request component - however it is a cryptic xpconnect error:

###!!! ASSERTION: Non-global object has the wrong flags: '!(jsclazz->flags & JSCLASS_IS_GLOBAL)', file /home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp, line 1148
XPCWrappedNative::GetNewOrUsed(XPCCallContext&, xpcObjectHelper&, XPCWrappedNativeScope*, XPCNativeInterface*, int, XPCWrappedNative**) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:591)
XPCConvert::NativeInterface2JSObject(XPCLazyCallContext&, jsval_layout*, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, int, int, unsigned int*) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcconvert.cpp:1293)
XPCConvert::NativeData2JS(XPCLazyCallContext&, jsval_layout*, void const*, nsXPTType const&, nsID const*, unsigned int*) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcconvert.cpp:493)
XPCConvert::NativeData2JS(XPCCallContext&, jsval_layout*, void const*, nsXPTType const&, nsID const*, unsigned int*) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcprivate.h:3176)
CallMethodHelper::GatherAndConvertResults() (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:2637)
CallMethodHelper::Call() (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:2388)
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:2337)
XPC_WN_GetterSetter(JSContext*, unsigned int, jsval_layout*) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1649)
CallJSNative (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jscntxtinlines.h:282)
Invoke (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsinterp.h:163)
js::InvokeGetterOrSetter(JSContext*, JSObject*, js::Value const&, unsigned int, js::Value*, js::Value*) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsinterp.cpp:849)
js::Shape::get(JSContext*, JSObject*, JSObject*, JSObject*, js::Value*) const (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsscopeinlines.h:270)
js_NativeGetInline (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsobj.cpp:5169)
js_GetPropertyHelperInline (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsobj.cpp:5268)
js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsinterp.cpp:3522)
js::InvokeKernel(JSContext*, js::CallArgs const&, js::MaybeConstruct) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsinterp.cpp:687)
Invoke (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsinterp.h:163)
LAST_FRAME_CHECKS (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/jsapi.cpp:4418)
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1659)
nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/js/src/xpconnect/src/xpcwrappedjs.cpp:586)
PrepareAndDispatch (/home/ddahl/code/moz/mozilla-mobile/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:155)
SharedStub (xptcstubs_x86_64_linux.cpp:0)
getRandVals: true

Comment 28

[David Dahl :ddahl]

Attachment: v 0.4 Current patch

Comment 29

[Alon Zakai (:azakai)]

Comment on attachment 556976 [details] [diff] [review]
v 0.4 Current patch

Ok, I debugged this a bit before I realized what the problem is. The issue is that the wakeup service only listens to processmessagemanager messages - not the per-window ones. This makes sense, since we want the wakeup service to only listen once for all of its messages, during initial load - we don't want it to do additional work for each new window opened and closed. Put another way, the wakeup service wakes up services, and it works at the level of services, not individual windows.

So, the solution should be, to make the messageManager you use be the per-process one. See the code in nsContentPrefService.js for example, or just mxr for parentprocessmessagemanager and childprocessmessagemanager (you need the latter in the child process of course). Should be a very small change, just which messagemanager you use. As a side effect, you will no longer need to rely on bug 669240 (and that assertion will go away, coincidentally).

Comment 30

[David Dahl :ddahl]

Attachment: v 0.5 Working patch, still a bug with messageManager

Very strange thing going on here: 

The first time I call messageManager.sendSyncMessage() I get back the result just fine. The second time it is called, I get a result, but the result array has 2 results as if I called it twice. The component that receives the message is only executing its messageHandler once. Very strange. I tried to bring all of this up in gdb, but my gdb foo is weak and I have a feeling I am just doing something very wrong. There are only a couple of other examples of sendSyncMessage in mxr, so I have not much to go on.

You need to build this for mobile and also build with the 2 dependent patches. In bug 440046, the patch is the latest one.

Also, the 2 errors I am testing for only ever return an array full of nulls, as if all of the previous result objects were nulled out.

Comment 31

[Alon Zakai (:azakai)]

Comment on attachment 557627 [details] [diff] [review]
v 0.5 Working patch, still a bug with messageManager

That sounds like a bug with the wakeup service, as if it isn't unlistening to the messages it wakes services up for. You can put some debug dumps in the wakeup service to see if that is the case, or I can take a look myself later today.

Comment 32

[David Dahl :ddahl]

(In reply to Alon Zakai (:azakai) from comment #31)
> I can take a look myself later today.

No worries. I think smaug figured out the services issue. I will be cleaning this up and rebasing before asking for review.

Comment 33

[David Dahl :ddahl]

Attachment: v 0.6 rebased for m-c

Strange wrapper problem here, I think. If I get a TypedArray passed to me from content and try to get an instance of nsIDOMCrypto, the TypedArray is not perceived as one.

If I do the same thing except the TypedArray is constructed in chrome scope, it all works. I tried unwrapping the TypedArray to no avail.

Comment 34

[David Dahl :ddahl]

The failure happens on line 99 of dom/base/CryptoAPI.js:  

[Exception... "The type of an object is incompatible with the expected type of the parameter associated to the object"  code: "17" nsresult: "0x80530011 (NS_ERROR_DOM_TYPE_MISMATCH_ERR)"  location: "file:///home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/dist/bin/components/CryptoAPI.js Line: 99"

Comment 35

[David Dahl :ddahl]

Also, we do this error checking inside of nsCrypto::GetRandomValues:

  // Is it an object?
  if (JSVAL_IS_PRIMITIVE(aData)) {
    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
  }

  JSObject* view = JSVAL_TO_OBJECT(aData);

  // Is it an ArrayBufferView?
  if (!js_IsTypedArray(view)) {
    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
  }

  // Throw if the wrong type of ArrayBufferView is passed in
  // (Part of the WHATWG spec)
  switch (JS_GetTypedArrayType(view)) {
    case js::TypedArray::TYPE_FLOAT32:
    case js::TypedArray::TYPE_FLOAT64:
      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
    default:
      break;
  }

Comment 36

[David Dahl :ddahl]

Attachment: v 0.7 broken on e10s

This patch has a crude attempt at disabling the 'normal' way to 'GetCrypto' in an nsGlobalWindow. Just want you too let me know if this is anywhere near correct. The new dom/base/CryptoAPI.js and dom/base/CryptoManager.js attach an object to the content process window and make the calls over to the chrome process where we lazily init an instance of nsCrypto.

Comment 37

[David Dahl :ddahl]

Attachment: v 0.8 Test passing

Patch is working again, proper (TYPE_MISMATCH_ERR) exception is thrown. Still have some question on how to handle nsGlobalWindow's GetCrypto/crypto property

Comment 38

[David Dahl :ddahl]

Attachment: v 0.9 tests pass, a couple of unneeded changes removed

This patch works on both e10s and desktop... finally.

Comment 39

[David Dahl :ddahl]

Attachment: v 0.10 Fixed for jsapi changes

Comment 40

[David Dahl :ddahl]

Attachment: v 11 removed all crypto bits and fixed exception handling over e10s

Mark:

Would like to get a mobile peer to review this before a super review for the interface changes. this patch adds window.crypto back to fennec, as well as altering the way we handle nsIDOMCrypto in desktop firefox. If you do not have time, can you recommend someone?

Comment 41

[David Dahl :ddahl]

Attachment: v 12 forgot to hg qref

whoops, forgot to qref

Comment 42

[David Dahl :ddahl]

I forgot to mention that the test is here: security/manager/ssl/tests/mochitest/bugs/test_440046.html. My build just finished and all of the tests are not running for some reason. 

Should I also copy my test over to mobile/ since I am going to assume the tests in security/manager/ssl/tests/mochitest are not run during mobile builds

Comment 43

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to David Dahl :ddahl from comment #42)
> I forgot to mention that the test is here:
> security/manager/ssl/tests/mochitest/bugs/test_440046.html. My build just
> finished and all of the tests are not running for some reason. 
> 
> Should I also copy my test over to mobile/ since I am going to assume the
> tests in security/manager/ssl/tests/mochitest are not run during mobile
> builds

Only tests in /browser won't be run, iirc. Just make sure the tests are active/enabled for mobile.

Comment 44

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 568233 [details] [diff] [review]
v 12 forgot to hg qref

Not sure I am a good enough review here, so switching to feedback.

Overall this looks good to me. You are using the watcher to handle the parent/child process issues. The code for CryptoAPI and Manager looks sound to me as well.

Comment 45

[David Dahl :ddahl]

(In reply to Mark Finkle (:mfinkle) from comment #43)
> Only tests in /browser won't be run, iirc. Just make sure the tests are
> active/enabled for mobile.

Dumb question: Is there a configuration switch I need flip to enable them for mobile? 

Thanks, Mark. I'm going to figure out these tests before getting a review from a DOM peer

Comment 46

[David Dahl :ddahl]

Attachment: v 13 all tests pass mobile and desktop

smaug: let me know if you have someone else in mind to review this. I have excised all of the crypto bits from this patch, so it is mainly a dom patch. I was unsure where else to put this code other than dom/base like the web console property code.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 568546 [details] [diff] [review]
v 13 all tests pass mobile and desktop

I think someone else more familiar with this kind of JS scripts should
review this.

nsICryptoManager.idl certainly needs some comments why it is basically
just a dummy interface.

I don't understand what XPCOMUtils.defineLazyGetter(this, "messageManager", ...
is trying to do, I mean the parentprocessmessagemanager part.

For content scripts, please wrap everything inside
(function() { ...})() until bug 673569 is fixed.

Comment 48

[David Dahl :ddahl]

(In reply to Olli Pettay [:smaug] from comment #47)
> Comment on attachment 568546 [details] [diff] [review] [diff] [details] [review]
> v 13 all tests pass mobile and desktop
> 
> I think someone else more familiar with this kind of JS scripts should
> review this.
> 

Thanks for looking at this, who might be a better reviewer?

Comment 49

[David Dahl :ddahl]

Comment on attachment 568546 [details] [diff] [review]
v 13 all tests pass mobile and desktop

Looking for a mobile-savvy/JS reviewer that a dom peer can bless:)

Comment 50

[Doug Turner (:dougt)]

Comment on attachment 568546 [details] [diff] [review]
v 13 all tests pass mobile and desktop

Review of attachment 568546 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/package-manifest.in
@@ +259,5 @@
>  @BINPATH@/components/ConsoleAPI.manifest
>  @BINPATH@/components/ConsoleAPI.js
> +@BINPATH@/components/CryptoAPI.manifest
> +@BINPATH@/components/CryptoAPI.js
> +@BINPATH@/components/CryptoManager.manifest

ooc, why don't we have one .manifest for this instead of the two you have?

::: dom/base/CryptoAPI.js
@@ +45,5 @@
> +XPCOMUtils.defineLazyGetter(this, "inContentProcess", function () {
> +  let appInfo = Cc["@mozilla.org/xre/app-info;1"];
> +  if (!appInfo || appInfo.getService(Ci.nsIXULRuntime).
> +        processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> +    return false;

Cc["@mozilla.org/xre/app-info;1"] should never fail, and if it does you should just throw and die.  change to:

if (Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT)
  return false;

@@ +57,5 @@
> +  if (!inContentProcess) {
> +    return Cc["@mozilla.org/security/crypto;1"].createInstance(Ci.nsIDOMCrypto);
> +  }
> +  else {
> +    return null;

non-sequitur:

if (!inContentProcess) {
return Cc["@mozilla.org/security/crypto;1"].createInstance(Ci.nsIDOMCrypto);
}

// no else

return null;

@@ +64,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "messageManager", function (){
> +  var mm;
> +  if (inContentProcess) {
> +    mm = Cc["@mozilla.org/childprocessmessagemanager;1"].getService(Components.interfaces.nsISyncMessageSender);

Components.interfaces -> Ci

@@ +69,5 @@
> +  }
> +  else {
> +    // TODO: only need this if we use the message manager for all API calls
> +    mm = Cc["@mozilla.org/parentprocessmessagemanager;1"].
> +           getService(Components.interfaces.nsIFrameMessageManager);

Components.interfaces -> Ci

@@ +76,5 @@
> +});
> +
> +function CryptoAPI()
> +{
> +  if (!inContentProcess) {

maybe this should be gInContentProcess.  I keep thinking it is going to be a local variable.

@@ +118,5 @@
> +        }
> +
> +        let length = aTypedArr.byteLength;
> +        let constructor = aTypedArr.__proto__.
> +          toString().split(" ")[1].split("]")[0];

This pains me.  wtf is doing.

@@ +126,5 @@
> +                                                      constructor: constructor })[0];
> +        if (!result) {
> +          let exception =
> +            new Components.Exception("TYPE_MISMATCH_ERR",
> +                                     17,

why 17?  Magic number.

@@ +142,5 @@
> +      enableSmartCardEvents: false,
> +
> +      signText: function cC_signText()
> +      {
> +        self.handleAPICall("signText", arguments);

you follow this pattern for each of the methods.  I wonder if there is a smarter way of doing it.

::: dom/interfaces/base/nsICryptoManager.idl
@@ +45,5 @@
> +};
> +
> +%{C++
> +// The contractID
> +#define NS_CRYPTO_MANAGER_CONTRACTID "@mozilla.org/crypto-manager;1"

This probably belongs somewhere outside the IDL.

::: mobile/chrome/tests/Makefile.in
@@ +42,5 @@
>  relativesrcdir  = mobile/chrome/tests
>  TESTXPI  = $(CURDIR)/$(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)/addons
>  ADDONSRC = $(srcdir)/addons
>  
> +DIRS = mochitest

Honorable, but not tests are attached.

Comment 51

[David Dahl :ddahl]

Doug: 

I think I attached the wrong patch. Crap! I know I updated that constructor string hack.

Comment 52

[David Dahl :ddahl]

Attachment: v 14 Correct patch

I totally owe you some booze. That was indeed the wrong patch. Sorry. A lot of what you reviewed still applies, I will fix that stuff and ask for review again.

Comment 53

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

ddahl, which person or people are supposed to review this patch? The r? flag is not set.

Comment 54

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #53)
> ddahl, which person or people are supposed to review this patch? The r? flag
> is not set.

dougt took a look and I am working on the changes. Another curveball came in today as we need to use another method other than the messageManager for this patch - the messageManager is going to be backed out at some future point

Comment 55

[David Dahl :ddahl]

I thought this patch was a non-starter with all of the changes to mobile, however, I have turned back to this patch again as Andreas told me that we might still be using e10s in the future.

It looks like we still have the MessageManager in the codebase, but now, after unbitrotting this patch, I get the following assertion when trying to use window.crypto:

###!!! ASSERTION: Trying to initialize PSM/NSS in a non-chrome process!: 'Error', file /home/ddahl/code/moz/mozilla-mobile/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp, line 299

The content javascript was merely doing: alert(window.crypto);

IS there something I am missing here? I don't understand why gecko thinks I am trying to load nss in a 'non-chrome' process.

Also, in order to disable MOZ_DISABLE_DOMCRYPTO I just removed it from the /configure.in completely, which cannot be correct.

Uploading the cleaned up patch now.

Comment 56

[David Dahl :ddahl]

Attachment: v 15 unbitrotten

see previous comment

Comment 57

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Implement an empty window.crypto on mobile

Here is a (completely untested) patch that does the following:

1. Splits nsIDOMCrypto into two interfaces: nsIDOMCrypto and nsIDOMCryptoLegacy. nsIDOMCrypto is currently empty. It is intended to be the interface that getRandomValues and DOMCrypt stuff get added to. nsIDOMCryptoLegacy is the old nsIDOMCrypto; it defines all the existing methods. nsIDOMCryptoLegacy extends nsIDOMCrypto.

2. Implements the empty nsIDOMCrypto interface in dom/base/nsDOMCrypto.cpp.

3. On Android (native and XUL), window.crypto implements only nsIDOMCrypto. On other platforms, window.crypto implements nsIDOMCryptoLegacy.

The idea here is that we may very well *never* implement the legacy methods on mobile. One reason is that we have no resources to do a mobile UI for them. The second reason is that these methods have bad security and usability properties. (See bug 680008 for example.) The third reason is that work on to-be-standard replacement methods for the old functionality (e.g. signText) is underway. Also, the old "version" property doesn't fit with how web applications do feature detection.

This patch doesn't actually implement getRandomValues. It is designed to have the patch that implements getRandomValues landed on top of it. It shouldn't be checked in (even after being reviewed) until the new nsIDOMCrypto interface has at least one member. (This first member will be getRandomValues, most likely.)

To implement getRandomValues on top of this patch, you should probably:
0. Declare getRandomValues in the new nsIDOMCrypto interface.
1. Move the core part of the implementation from nsCrypto to nsRandomGenerator.
2. In dom/base/nsDOMCrypto.cpp, implement getRandomValues with a method that basically just forwards the call to the new method in nsRandomGenerator.
3. nsDOMCrypto should be the class that gets split into child/parent process classes to handle e10s-related stuff, so that nsRandomGenerator and the rest of PSM can remain ignorant of e10s. This is the part I don't quite know how to do but I can get you some help here.

Feedback appreciated.

Comment 58

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

One other addition to the previous comment: dom/base/nsDOMCrypto.cpp is also the place where all DOM- and JS- related processing should happen, so that PSM doesn't need to know about the DOM or JS at all (except for the legacy dom.crypto method implementations, of course). Similarly, all access to NSS functionality must be done in PSM classes and not anywhere in dom/*. Such a split of responsibilities will allow us to implement the DOMCrypt methods and getRandomValues in a way that will work for e10s. This is why I put nsDOMCrypto.cpp in dom/base instead of under security/.

Comment 59

[David Dahl :ddahl]

Comment on attachment 585269 [details] [diff] [review]
Implement an empty window.crypto on mobile

The separation looks good. A question: What is the status of e10s vis a vis the native Android build of fennec? Will native Android function like Desktop under the hood?

In order to get this landed sooner rather than later, should we just ignore e10s until this is on m-c - then work out the parent-child e10s bits and however this should be activated for native Android?

Comment 60

[Alon Zakai (:azakai)]

(In reply to David Dahl :ddahl from comment #59)
> Comment on attachment 585269 [details] [diff] [review]
> Implement an empty window.crypto on mobile
> 
> The separation looks good. A question: What is the status of e10s vis a vis
> the native Android build of fennec?

Native Android Fennec does not use e10s. (However, Boot to Gecko Fennec does use e10s.)

Comment 61

[Johnny Stenback (:jst)]

Comment on attachment 585269 [details] [diff] [review]
Implement an empty window.crypto on mobile

This approach looks good to me.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

This patch presumably depends on bug 726864, given its use of the wake up service.

Comment 63

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #57)
> Created attachment 585269 [details] [diff] [review]
> Implement an empty window.crypto on mobile

In trying to build this patch, I see the following errors:

nsCertTree.cpp
../../security/manager/ssl/src/nsSmartCardMonitor.o:(.bss.gPIPNSSLog+0x0): multiple definition of `gPIPNSSLog'
../../security/manager/ssl/src/nsNSSComponent.o:(.bss.gPIPNSSLog+0x0): first defined here
../../security/manager/ssl/src/nsNSSComponent.o:(.data.rel.ro._ZTV14nsNSSComponent[vtable for nsNSSComponent]+0xb8): undefined reference to `nsNSSComponent::PostEvent(nsAString_internal const&, nsAString_internal const&)'
../../security/manager/ssl/src/nsNSSComponent.o:(.data.rel.ro._ZTV14nsNSSComponent[vtable for nsNSSComponent]+0xc0): undefined reference to `nsNSSComponent::DispatchEvent(nsAString_internal const&, nsAString_internal const&)'
../../security/manager/ssl/src/nsNSSComponent.o:(.data.rel.ro._ZTV14nsNSSComponent[vtable for nsNSSComponent]+0x1a8): undefined reference to `non-virtual thunk to nsNSSComponent::PostEvent(nsAString_internal const&, nsAString_internal const&)'
../../security/manager/ssl/src/nsNSSComponent.o:(.data.rel.ro._ZTV14nsNSSComponent[vtable for nsNSSComponent]+0x1b0): undefined reference to `non-virtual thunk to nsNSSComponent::DispatchEvent(nsAString_internal const&, nsAString_internal const&)'
../../security/manager/ssl/src/nsCrypto.o: In function `nsCrypto::AddRef()':
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:243: undefined reference to `mozilla::dom::nsDOMCrypto::AddRef()'
../../security/manager/ssl/src/nsCrypto.o: In function `nsCrypto::Release()':
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:244: undefined reference to `mozilla::dom::nsDOMCrypto::Release()'
../../security/manager/ssl/src/nsCrypto.o: In function `nsCrypto':
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:277: undefined reference to `mozilla::dom::nsDOMCrypto::nsDOMCrypto()'
../../security/manager/ssl/src/nsCrypto.o: In function `~nsCrypto':
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:281: undefined reference to `mozilla::dom::nsDOMCrypto::~nsDOMCrypto()'
/usr/bin/ld.bfd.real: libxul.so: hidden symbol `mozilla::dom::nsDOMCrypto::nsDOMCrypto()' isn't defined
/usr/bin/ld.bfd.real: final link failed: Bad value

Comment 64

[David Dahl :ddahl]

Attachment: v 2 Implement empty window.crypto on mobile

Now it builds, next rebase the patch from bug 440046 on top of this

Comment 65

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #57)
> Created attachment 585269 [details] [diff] [review]

> To implement getRandomValues on top of this patch, you should probably:
> 0. Declare getRandomValues in the new nsIDOMCrypto interface.
> 1. Move the core part of the implementation from nsCrypto to
> nsRandomGenerator.
> 2. In dom/base/nsDOMCrypto.cpp, implement getRandomValues with a method that
> basically just forwards the call to the new method in nsRandomGenerator.

In the middle of this move-around, and am seeing this:

/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsNSSModule.cpp: In function ‘nsresult nsCryptoConstructor(nsISupports*, const nsIID&, void**)’:
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsNSSModule.cpp:236:1: error: cannot allocate an object of abstract type ‘nsCrypto’
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.h:80:7: note:   because the following virtual functions are pure within ‘nsCrypto’:
../../../../dist/include/nsIDOMCrypto.h:33:28: note: 	virtual nsresult nsIDOMCrypto::GetRandomValues(const JS::Value&, JS::Value*)
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsNSSModule.cpp:236:1: error: cannot allocate an object of abstract type ‘nsCrypto’
/home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.h:80:7: note:   since type ‘nsCrypto’ has pure virtual functions

Comment 66

[David Dahl :ddahl]

Attachment: v 1 Alternate Patch for bug 440046

Patch in use when above build errors were produced

Comment 67

[David Dahl :ddahl]

(In reply to David Dahl :ddahl from comment #65)
> In the middle of this move-around, and am seeing this:
> 
> /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsNSSModule.
> cpp: In function ‘nsresult nsCryptoConstructor(nsISupports*, const nsIID&,
> void**)’:
> /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsNSSModule.
> cpp:236:1: error: cannot allocate an object of abstract type ‘nsCrypto’
> /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.h:80:
> 7: note:   because the following virtual functions are pure within
> ‘nsCrypto’:
> ../../../../dist/include/nsIDOMCrypto.h:33:28: note: 	virtual nsresult
> nsIDOMCrypto::GetRandomValues(const JS::Value&, JS::Value*)
> /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsNSSModule.
> cpp:236:1: error: cannot allocate an object of abstract type ‘nsCrypto’
> /home/ddahl/code/moz/mozilla-central/security/manager/ssl/src/nsCrypto.h:80:
> 7: note:   since type ‘nsCrypto’ has pure virtual functions

Smaug told me on irc this is due to nsCrypto not implementing all of the inherited classes methods, which I think bsmith wanted to do via:

class nsCrypto: public mozilla::dom::nsDOMCrypto, public nsIDOMCryptoLegacy
{
public:
  nsCrypto();
  virtual ~nsCrypto();
  
  NS_DECL_ISUPPORTS_INHERITED
  NS_DECL_NSIDOMCRYPTOLEGACY
...

I'll keep digging

Comment 68

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Get David's patches to build.

David, here are the modifications I had to make to get this to build. I didn't try to run the tests yet.

Comment 69

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Get David's patches to build (new-crypto-object)

Comment 70

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Implement empty window.crypto object on e10s

This should build, and the updated version of your getRandomValues patch that I will post next should build on top of it. I haven't tested it though.

Comment 71

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Fixup David's patch to implement getRandomValues on top of empty window.crypto patch

David, please test this out to see if it actually works. I have only built it; I didn't try to run it.

Also, please update the patch to take into account the DOMException stuff that already landed and move the xpcshell tests to be under dom/ instead of security/manager/ssl.

Comment 72

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 602533 [details] [diff] [review]
Fixup David's patch to implement getRandomValues on top of empty window.crypto patch

Review of attachment 602533 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed only the NSS/PSM-related parts for now.

Please merge the changes from bug 440046 into this patch. I suggest that you replace the existing patch bug 440046 with this one (i.e. attach the updated version of this patch to bug 440046).

I will finish up the empty window.crypto patch and add tests for it and request review from the dom team.

::: dom/base/nsDOMCrypto.cpp
@@ +116,5 @@
> +  // // bytes have been requested.
> +  // if (srv != SECSuccess) {
> +  //   return PR_GetError() == SEC_ERROR_INVALID_ARGS 
> +  //     ? NS_ERROR_DOM_QUOTA_EXCEEDED_ERR : NS_ERROR_FAILURE;
> +  // }

Remove this commented-out code.

@@ +120,5 @@
> +  // }
> +
> +  nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();
> +
> +  nsresult rv = randomGen->GetRandomValues(dataLen, data);

nsresult rv = nsRandomGenerator::GenerateRandomBytes(data, dataLen);

@@ +122,5 @@
> +  nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();
> +
> +  nsresult rv = randomGen->GetRandomValues(dataLen, data);
> +  
> +  // TODO: handle various errors

Handle the errors.

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +71,5 @@
>    return NS_OK;
>  }
> +
> +NS_IMETHODIMP
> +nsRandomGenerator::GetRandomValues(PRUint32 aLength, unsigned char* aValues)

Rename to GenerateRandomBytes and change this to be a static member function of nsRandomGenerator.

Document the error handling of GenerateRandomBytes. In particular, what does GenerateRandomBytes return when too many random bytes have been requested?

Check that aValues is not null.

We will need to file a follow-up bug that nsRandomGenerator does not implement nsNSSShotdownObject.

@@ +75,5 @@
> +nsRandomGenerator::GetRandomValues(PRUint32 aLength, unsigned char* aValues)
> +{
> +  if (aLength == 0) {
> +    return NS_ERROR_FAILURE;
> +  }

Why not return NS_OK?

@@ +80,5 @@
> +
> +  SECStatus srv = PK11_GenerateRandom(aValues, aLength);
> +
> +  if (srv != SECSuccess) {
> +    return PR_GetError();

You must map the PRErrorCode into a nsresult.

::: security/manager/ssl/tests/mochitest/bugs/test_bug440046.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

The tests should be moved from security/manager/ssl to dom/.

I did not review this file yet. I need to see the interdiff between it and the last version I reviewed. (I will do the interdiff myself.)

Comment 73

[David Dahl :ddahl]

Attachment: crash after unbitrot

I will attach the new unbitrot patches soon

Comment 74

[David Dahl :ddahl]

The crash was on startup:

Program ./dist/bin/firefox (pid = 9447) received signal 6.
Stack:
raise+0x0000002B [/lib/x86_64-linux-gnu/libpthread.so.0 +0x0000FF2B]
MOZ_Crash+0x0000000E [./dist/bin/firefox +0x0001DD66]
MOZ_Assert+0x00000056 [./dist/bin/firefox +0x0001DDBE]
UNKNOWN [/home/ddahl/code/moz/mozilla-share/obj-x86_64-unknown-linux-gnu-debug/dist/bin/libxul.so +0x01848955]
UNKNOWN [/home/ddahl/code/moz/mozilla-share/obj-x86_64-unknown-linux-gnu-debug/dist/bin/libxul.so +0x0184CDFC]
...
XRE_main+0x00001C7C [/home/ddahl/code/moz/mozilla-share/obj-x86_64-unknown-linux-gnu-debug/dist/bin/libxul.so +0x008697C1]
UNKNOWN [./dist/bin/firefox +0x0000237A]
UNKNOWN [./dist/bin/firefox +0x000025D0]
__libc_start_main+0x000000ED [/lib/x86_64-linux-gnu/libc.so.6 +0x0002130D]
UNKNOWN [./dist/bin/firefox +0x00001BB9]

Comment 75

[David Dahl :ddahl]

a closer look at the assertion dump output makes me think this might be a build problem that does not manifest until it is run:

###!!! ASSERTION: Class info data out of sync, you forgot to update nsDOMClassInfo.h and nsDOMClassInfo.cpp! Fix this, mozilla will not work without this fixed!: 'Error', file /home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMClassInfo.cpp, line 4417

Comment 76

[David Dahl :ddahl]

Attachment: New Crypto Object Patch, unbitrotted

This one is crashing die to some include ordering in nsClassInfo.cpp ?

Comment 77

[David Dahl :ddahl]

Attachment: New Crypto Object patch, errors

I am not sure what nsIDOMClass* goop is missing here:

In file included from /home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMClassInfo.cpp:457:0:
/home/ddahl/code/moz/mozilla-share/src/content/base/src/FileIOObject.h:79:3: warning: ‘virtual nsresult mozilla::dom::FileIOObject::Notify(nsITimer*)’ was hidden [-Woverloaded-virtual]
/home/ddahl/code/moz/mozilla-share/src/content/base/src/nsDOMFileReader.h:94:14: warning:   by ‘virtual nsresult nsDOMFileReader::Notify(const char*, nsDetectionConfident)’ [-Woverloaded-virtual]
/home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMClassInfo.cpp:1012:1: error: ‘eDOMClassInfo_Crypto_id’ was not declared in this scope
/home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMClassInfo.cpp: In static member function ‘static nsresult nsDOMClassInfo::Init()’:
/home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMClassInfo.cpp:3094:1: error: ‘eDOMClassInfo_Crypto_id’ was not declared in this scope

Comment 78

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

DOMCI_CLASS(CryptoLegacy) should just be DOMCI_CLASS(Crypto)

Comment 79

[David Dahl :ddahl]

(In reply to Ms2ger from comment #78)
> DOMCI_CLASS(CryptoLegacy) should just be DOMCI_CLASS(Crypto)

I still have the same crash. I must be missing some other bit of magic code.

Comment 80

[David Dahl :ddahl]

Attachment: New Crypto Object patch, scaffolding still not correct

I added the sCrypto_id, as a stab in the dark, still borked.

Comment 81

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 606266 [details] [diff] [review]
New Crypto Object patch, scaffolding still not correct

The sCrypto_id stuff is wrong. Do you need me to review this patch further?

Comment 82

[David Dahl :ddahl]

(In reply to Ms2ger from comment #81)
> Comment on attachment 606266 [details] [diff] [review]
> New Crypto Object patch, scaffolding still not correct
> 
> The sCrypto_id stuff is wrong. Do you need me to review this patch further?

Last week in SF, Kyle and Brian and I made some progress. I will upload a new patch tomorrow.

Comment 83

[David Dahl :ddahl]

Attachment: New Crypto Obj Builds and runs, but...

So this patch now builds and runs, but fails if you try to access the crypto object due to:

###!!! ASSERTION: Class name and proto chain interface name mismatch!: 'nsCRT::strcmp(CutPrefix(name), mData->mName) == 0', file /home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMClassInfo.cpp, line 4977

Apparently you cannot use nsIDOMCryptoLegacy as the impl if the window object is named "Crypto"

Comment 84

[Josh Matthews [:jdm]]

>  DOM_CLASSINFO_MAP_BEGIN(Crypto, nsIDOMCryptoLegacy)

This needs to be DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF to avoid that assertion.

Comment 85

[David Dahl :ddahl]

(In reply to Josh Matthews [:jdm] from comment #84)
> >  DOM_CLASSINFO_MAP_BEGIN(Crypto, nsIDOMCryptoLegacy)
> 
> This needs to be DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF to avoid that assertion.

Ok, that helps. Now I am seeing some kind of missing/wrong interface error:

[17:19:05.535] [Exception... "Component does not have requested interface arg 0 [nsIDOMWindow.crypto]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: Web Console :: <TOP_LEVEL> :: line 1"  data: no]

Is there further confusion here since we have nsIDOMCryptoLegacy ?

Comment 86

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Without really understanding what you're doing here, mozilla::dom::nsCrypto (ew double namespacing) doesn't implement class info, so you can't use it as a DOM object.

Comment 87

[David Dahl :ddahl]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #86)
> Without really understanding what you're doing here, mozilla::dom::nsCrypto
> (ew double namespacing) doesn't implement class info, so you can't use it as
> a DOM object.

I think we are trying to subclass nsCrypto from nsCryptoLegacy in the case of Desktop Firefox, but not do so with any mobile release, only adding new APIs to the mobile releases.

Is this approach possible? Can we implement classinfo? I'm thinking khuey and bsmith should pow wow on this problem.

Comment 88

[David Dahl :ddahl]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #86)
> Without really understanding what you're doing here, mozilla::dom::nsCrypto
> (ew double namespacing) doesn't implement class info, so you can't use it as
> a DOM object.

Is it possible to make the new nsCrypto implement class info? Or does the underlying re-org patch need to be changed back to just a bunch of #ifdefs ?

Comment 89

[David Dahl :ddahl]

It looks like we do not have:

DOMCI_DATA(Crypto, nsCrypto)

in nsCrypto.cpp

Adding this produces a build error:

/home/ddahl/code/moz/mozilla-share/src/security/manager/ssl/src/nsCrypto.cpp:237:11: error: expected constructor, destructor, or type conversion before ‘(’ token

Comment 90

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, DOMCI_DATA is supposed to be used just in dom/ and content/.

Comment 91

[David Dahl :ddahl]

Attachment: Latest new crypto object

Comment 92

[David Dahl :ddahl]

Attachment: Latest getRandomValues

jst:

We need a plan here as we are not DOM guys. How should this new dynamic crypto Object be attached to the window? nsDOMCryptoLegacy will be used by desktop firefox, but nsDOMCrypto will be used by mobile, b2g.

Comment 93

[Johnny Stenback (:jst)]

Attachment: Interdiff needed to get "Latest new crypto object" to work on desktop

This fixes the classinfo goop to work on desktop builds (i.e. legacy crypto stuff enabled), with this set of changes I can use the crypto object, including calling the new getRandomValues method.

Comment 94

[Johnny Stenback (:jst)]

Attachment: "Latest new crypto object" updated to trunk, and includes the fixes in the previous interdiff

Comment 95

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks jst!

ddahl, since this patch doesn't implement the actual e10s stuff, when you produce the final version (commented-out code removed, etc), please attach it to the main getRandomValues bug, bug 440046.

If not done already, let's try to do add the ArrayBuffer support too, including the test with a raw ArrayBuffer (should be really easy).

We can fix the e10s version in this bug as a follow-up, as Fennec doesn't need e10s any more, but B2G might in the future. We should find a B2G Web API tracking bug to mark as depending on this bug.

Comment 96

[David Dahl :ddahl]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #94)
> Created attachment 617165 [details] [diff] [review]
> "Latest new crypto object" updated to trunk, and includes the fixes in the
> previous interdiff

jst:

Thanks so much for this!

Comment 97

[David Dahl :ddahl]

With the latest patch applied, I am getting this error:

/home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMCrypto.cpp:44:11: error: expected constructor, destructor, or type conversion before ‘(’ token
/home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMCrypto.cpp:64:3: error: expected ‘}’ at end of input
/home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMCrypto.cpp:64:3: error: expected ‘}’ at end of input

on this line: 

DOMCI_DATA(Crypto, nsDOMCrypto)

Comment 98

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: New crypto object

(In reply to David Dahl :ddahl from comment #97)
> With the latest patch applied, I am getting this error:
> 
> /home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMCrypto.cpp:44:11:
> error: expected constructor, destructor, or type conversion before ‘(’ token
> /home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMCrypto.cpp:64:3: error:
> expected ‘}’ at end of input
> /home/ddahl/code/moz/mozilla-share/src/dom/base/nsDOMCrypto.cpp:64:3: error:
> expected ‘}’ at end of input
> 
> on this line: 
> 
> DOMCI_DATA(Crypto, nsDOMCrypto)

You tried to build the "new-crypto-object" patch without the getRandomValues patch, but that patch doesn't build on its own due to a missing #include. I have updated jst's updated patch to fix that.

Comment 99

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Bug 582297 is about forwarding calls from the existing methods from content to chrome. This bug is about forwarding just window.crypto.getRandomValues() from content to chrome, and after the refactoring in bug 683262, we can do those things independently. It will actually be easier to fix bug 582297 after we fix this one, because we'll have learned mostly how to do it (sans UI issues).

Comment 100

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 618131 [details] [diff] [review]
New crypto object

Patch moved to bug 683262.

Comment 101

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 616847 [details]
Latest getRandomValues

Updated patch moved to bug 440046. This bug is just about e10s, but this patch implements only the non-e10s case.

Thanks jst for your help!

Comment 102

[David Dahl :ddahl]

Attachment: Patch 1

I have a feeling I am missing something here, however, is this approach on the right track?

Comment 103

[David Dahl :ddahl]

Attachment: Patch 2

changed a type, I know you said to use nsCString, just curious if the conversion from unsigned char* to nsCString is unneeded? Also, I am unsure if unsigned char* will make it through ipc.

Comment 104

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 634614 [details] [diff] [review]
Patch 2

Review of attachment 634614 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, that seems very simple. Do the tests actually pass?

::: dom/base/Crypto.cpp
@@ +77,4 @@
>  
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    if (!NS_IsMainThread()) {
> +      NS_ERROR("Cannot instantiate nsRandomGenerator in child processes from non-main thread.");

We should not be instantiating nsRandomGenerator from child processes AT ALL.

Comment 105

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 634614 [details] [diff] [review]
Patch 2

>@@ -63,24 +67,33 @@ Crypto::GetRandomValues(const jsval& aDa
>       return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
>   }
> 
>   PRUint32 dataLen = JS_GetTypedArrayByteLength(view, cx);
>   // Abort if the ArrayBufferView is requesting too few values
>   if (dataLen == 0) {
>     return NS_OK;
>   }
>+  nsresult rv;
>+  unsigned char* data = static_cast<unsigned char*>(JS_GetArrayBufferViewData(view, cx));
> 
>-  unsigned char* data = static_cast<unsigned char*>(JS_GetArrayBufferViewData(view, cx));
>-  
>-  nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();
>-
>-  nsresult rv = randomGen->GetRandomValues(dataLen, data);
>-  
>-  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+  if (XRE_GetProcessType() != GeckoProcessType_Default) {
>+    if (!NS_IsMainThread()) {
>+      NS_ERROR("Cannot instantiate nsRandomGenerator in child processes from non-main thread.");
>+      return NS_ERROR_FAILURE;
>+    }
>+    // Tell the chrome process to generate random values via PContent
>+    ContentChild* cc = ContentChild::GetSingleton();
>+    data = cc->GetRandomValues(dataLen, data);
So, you don't actually do anything with data.


> 
> bool
>+ContentParent::RecvGetRandomValues(PRUint32 length, unsigned char* data)
>+{
>+    nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();
>+    rv = randomGen->GetRandomValues(length, data);
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+
>+    return true;
>+}
does this compile?
You have RecvGetRandomValues(PRUint32 length, unsigned char* data) here
...
but the following in the header:
> 
>+    virtual bool RecvGetRandomValues(PRUint32 length, char randomValues);

> 
>+    sync RandomValues(PRInt32 length, char randomValues) returns (char randomValues);
This would send and receive just one char.
You want either (PRInt32 length, char[] randomValues) (not sure how the memory is managed in that case), or
just use nsCString as a container for char data.


But yes, something like this.

Comment 106

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #104)
> Comment on attachment 634614 [details] [diff] [review]
> Patch 2
> 
> Review of attachment 634614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow, that seems very simple. Do the tests actually pass?
No.

> We should not be instantiating nsRandomGenerator from child processes AT ALL.

I was just seeing if I was on the right track with my approach.

Comment 107

[David Dahl :ddahl]

Attachment: Patch 3 - builds - does not work yet

I know this code does not work but am still unsure how the content child talks to the parent to run the GenerateRandomBytes method and pass the resulting char* to the child.

Comment 108

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 635501 [details] [diff] [review]
Patch 3 - builds - does not work yet

Review of attachment 635501 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Crypto.cpp
@@ +11,5 @@
> +#include "mozilla/dom/ContentParent.h"
> +#include "mozilla/dom/ContentChild.h"
> +
> +using mozilla::dom::ContentParent;
> +using mozilla::dom::ContentChild;

Shouldn't need this

@@ +86,5 @@
> +    ContentChild* cc = ContentChild::GetSingleton();
> +    data = cc->RecvGetRandomValues(dataLen, data);
> +  } else {
> +    nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();
> +    rv = randomGen->GenerateRandomBytes(dataLen, data);

declare the rv here

::: dom/ipc/PContent.ipdl
@@ +131,5 @@
>      PAudio(PRInt32 aNumChannels, PRInt32 aRate, PRInt32 aFormat);
>  
>      sync PCrashReporter(NativeThreadId tid, PRUint32 processType);
>  
> +    sync GetRandomValues(PRInt32 length, char[] randomValues) returns (char[] randomValues);

I suspect that having randomValues as argument *and* return value isn't what you want, but I have no experience with IPDL. Talk to cjones or someone else who does?

Comment 109

[Josh Matthews [:jdm]]

Comment on attachment 635501 [details] [diff] [review]
Patch 3 - builds - does not work yet

Review of attachment 635501 [details] [diff] [review]:
-----------------------------------------------------------------

https://developer.mozilla.org/en/IPDL/Tutorial might explain some IPDL stuff that is confusing you.

::: dom/ipc/ContentChild.cpp
@@ +783,5 @@
>  
> +unsigned char*
> +ContentChild::RecvGetRandomValues(PRUint32 length, unsigned char* randomValues)
> +{
> +    // XXXddahl: what do I do here? Not sure how this will call the parent method.

The way to call the parent method is via SendGetRandomValues.

::: dom/ipc/ContentParent.cpp
@@ +1041,5 @@
>      return true;
>  }
>  
> +unsigned char*
> +ContentParent::RecvGetRandomValues(PRUint32 length, unsigned char* data)

I'm surprised this builds. RecvGetRandomValues returns a bool indicating success or failure. The return value for the IPDL method is an outparam.

::: dom/ipc/PContent.ipdl
@@ +131,5 @@
>      PAudio(PRInt32 aNumChannels, PRInt32 aRate, PRInt32 aFormat);
>  
>      sync PCrashReporter(NativeThreadId tid, PRUint32 processType);
>  
> +    sync GetRandomValues(PRInt32 length, char[] randomValues) returns (char[] randomValues);

Ms2ger is correct. Only include the randomValues arg if you actually want to pass data inside of it to the receiver.

Comment 112

[David Dahl :ddahl]

Attachment: Unbitrotten patch

Unbitrotten on top of the patches in bug 683262 and bug 440046

Comment 113

[David Dahl :ddahl]

Attachment: Latest non-working patch

Ian:

I was hoping you might know more about native code ipc and can give me some pointers on how to make this patch work.

Comment 114

[Josh Matthews [:jdm]]

Comment on attachment 711840 [details] [diff] [review]
Latest non-working patch

>+unsigned char*
>+ContentChild::RecvGetRandomValues(PRUint32 length, unsigned char* randomValues)
>+{
>+    // XXXddahl: what do I do here? Not sure how this will call the parent method.
>+    // This is a bit magickal to me.
>+    return randomValues;
>+}

RecvFoo should only be defined in the parent actor, not the child.

>+unsigned char*
>+ContentParent::RecvGetRandomValues(uint32_t length, unsigned char* data)
>+{
>+    nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();
>+    nsresult rv = randomGen->GenerateRandomBytes(length, data);
>+    // NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+    // XXXddahl: how do I return the values to the child content process? 
>+    return data; // XXXddahl: This is obvioulsy not right, just try to get it to build 
>+}

The return type should be bool, to indicate success or failure. Based on the IPDL declaration:

>sync GetRandomValues(uint32_t length, char[] randomValues) returns (char[] randomValues);

(in which the first randomValues is unnecessary) your method will take an InfallibleTArray<char>& argument in which you'll store the output of GetRandomValues. You'll want to call SetCapacity on the array to ensure its the size you need, then you can pass its buffer to GetRandValues and it will be serialized automatically by IPDL on the return trip.

Comment 115

[Ian Melven :imelven]

Comment on attachment 711840 [details] [diff] [review]
Latest non-working patch

(In reply to David Dahl :ddahl from comment #113)
> Created attachment 711840 [details] [diff] [review]
> Latest non-working patch
> 
> Ian:
> 
> I was hoping you might know more about native code ipc and can give me some
> pointers on how to make this patch work.

I actually know very little about native code ipc, but learned quite a bit more from looking at your patch and the IPDL tutorial jdm linked in comment 109, I'll try to help going forward though.

Comment 116

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 711840 [details] [diff] [review]
Latest non-working patch

Review of attachment 711840 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +1136,5 @@
>      return true;
>  }
>  
> +unsigned char*
> +ContentChild::RecvGetRandomValues(PRUint32 length, unsigned char* randomValues)

No PR types, please...

Comment 117

[David Dahl :ddahl]

Attachment: Another WIP patch

Since bug 440046 bounced, we need this patch to be working ASAP. How do you get the InfallibleTArray char data to pass to the randomgenerator?

Comment 118

[Johnny Stenback (:jst)]

Attachment: Compiles, and should in theory work (but untested).

ddahl, can you test this? Should be testable with a remote iframe in mochitest, as the indexedDB tests are done. Also, should work in b2g tests, but those may be hard to run out of process locally...

Comment 119

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 714827 [details] [diff] [review]
Compiles, and should in theory work (but untested).

Review of attachment 714827 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Crypto.cpp
@@ +69,5 @@
>        return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
>    }
>  
>    uint32_t dataLen = JS_GetTypedArrayByteLength(view);
> +  unsigned char* data;

Declare this when you need it

@@ +98,2 @@
>  
> +    memcpy(data, static_cast<uint8_t*>(randomValues.Elements()), dataLen);

Don't think you need the cast

::: dom/ipc/ContentParent.cpp
@@ +2179,5 @@
> +                                       &buf);
> +    if (NS_FAILED(rv)) {
> +        return false;
> +    }
> + 

Trailing whitespace

@@ +2183,5 @@
> + 
> +    randomValues->SetCapacity(length);
> +    randomValues->SetLength(length);
> +
> +    memcpy(static_cast<uint8_t*>(randomValues->Elements()), buf, length);

Don't think you need the cast here either

::: dom/ipc/PContent.ipdl
@@ +384,5 @@
>      PDeviceStorageRequest(DeviceStorageParams params);
>  
>      sync PCrashReporter(NativeThreadId tid, uint32_t processType);
>  
> +    sync GetRandomValues(uint32_t length) 

Trailing whitespace

Comment 120

[Josh Matthews [:jdm]]

Comment on attachment 714827 [details] [diff] [review]
Compiles, and should in theory work (but untested).

Review of attachment 714827 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +2170,5 @@
>  bool
> +ContentParent::RecvGetRandomValues(const uint32_t& length,
> +                                   InfallibleTArray<uint8_t>* randomValues)
> +{
> +    nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();

Use the same do_GetService stuff as Crypto, please.

@@ +2174,5 @@
> +    nsRefPtr<nsRandomGenerator> randomGen = new nsRandomGenerator();
> +    uint8_t *buf;
> +
> +    nsresult rv =
> +        randomGen->GenerateRandomBytes(randomValues->Length(),

You should use length here instead.

Comment 121

[David Dahl :ddahl]

Attachment: Latest patch, unbitrot

Builds fine, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e78416f0e809

[I will also refresh my FxOS build to test locally]

Comment 122

[Mozilla RelEng Bot]

Try run for 3f1a7c729354 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3f1a7c729354
Results (out of 6 total builds):
    exception: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3f1a7c729354

Comment 123

[David Dahl :ddahl]

Looks like the latest patch works on b2g!
https://tbpl.mozilla.org/?tree=Try&rev=4b167a1ee47a  (mochitest-2)

Comment 124

[David Dahl :ddahl]

Attachment: Latest Patch, builds, all tests pass

Kyle: Do you have time to review this small patch that provides randomvalues for b2g? It is mainly an IPC change.

Comment 125

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 716380 [details] [diff] [review]
Latest Patch, builds, all tests pass

Review of attachment 716380 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +2169,5 @@
>  
>  bool
> +ContentParent::RecvGetRandomValues(const uint32_t& length,
> +                                   InfallibleTArray<uint8_t>* randomValues)
> +{

It's kind of lame that you have this code duplicated.  Can you stick it in a static function on the crypto object and call it from both places?

@@ +2172,5 @@
> +                                   InfallibleTArray<uint8_t>* randomValues)
> +{
> +    nsCOMPtr<nsIRandomGenerator> randomGenerator;
> +    nsresult rv;
> +    randomGenerator = 

extra whitespace

@@ +2180,5 @@
> +    rv = randomGenerator->GenerateRandomBytes(randomValues->Length(), &buf);
> +    if (NS_FAILED(rv)) {
> +        return false;
> +    }
> + 

extra whitespace

::: dom/ipc/PContent.ipdl
@@ +384,5 @@
>      PDeviceStorageRequest(DeviceStorageParams params);
>  
>      sync PCrashReporter(NativeThreadId tid, uint32_t processType);
>  
> +    sync GetRandomValues(uint32_t length) 

extra whitespace

Comment 126

[David Dahl :ddahl]

Attachment: Updated patch, does not work

Naturally, I am doing it wrong. Not sure how to make the static function visible in the ContentParent side. Also, unsure what to call this static function and what it's signature should be.

Comment 127

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 718176 [details] [diff] [review]
Updated patch, does not work

Review of attachment 718176 [details] [diff] [review]:
-----------------------------------------------------------------

It needs to go in Crypto.h, and then ContentParent.cpp needs to #include "Crypto.h"

Comment 128

[David Dahl :ddahl]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #127)
> Comment on attachment 718176 [details] [diff] [review]
> Updated patch, does not work
> 
> Review of attachment 718176 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It needs to go in Crypto.h, and then ContentParent.cpp needs to #include
> "Crypto.h"

Should the function return a uint8_t* ? also, in the case of an error, what do I return?

Comment 129

[David Dahl :ddahl]

Attachment: Working patch, comments addressed

Ok, thanks to you and mmc this patch works. Would love another pass from you before pushing to try, thanks again

Comment 130

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This is what I had in mind.

Comment 131

[David Dahl :ddahl]

Comment on attachment 718451 [details] [diff] [review]
Patch

Review of attachment 718451 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! I'll also fix the whitespace and push to try.

::: dom/base/Crypto.cpp
@@ +183,5 @@
> +  nsresult rv;
> +  randomGenerator = 
> +    do_GetService("@mozilla.org/security/random-generator;1", &rv);
> +  // XXXkhuey null check randomGenerator?
> +

Yep, I must have left that out on accident. will fix.

@@ +187,5 @@
> +
> +  uint8_t* buf;
> +  rv = randomGenerator->GenerateRandomBytes(aLength, &buf);
> +
> +  // XXXkhuey are we sure this always succeeds?

Won't the MOZ_ASSERT catch a failure?

Comment 132

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to David Dahl :ddahl from comment #131)
> @@ +187,5 @@
> > +
> > +  uint8_t* buf;
> > +  rv = randomGenerator->GenerateRandomBytes(aLength, &buf);
> > +
> > +  // XXXkhuey are we sure this always succeeds?
> 
> Won't the MOZ_ASSERT catch a failure?

Not in an opt build.

Comment 133

[David Dahl :ddahl]

Attachment: Latest patch review comments addressed

Changed the GetRandomValues function to return nullptr in case the randomGenerator is not able to function correctly

Comment 134

[David Dahl :ddahl]

Attachment: Latest patch

hmm, maybe I uploaded before qref completed?

Comment 135

[David Dahl :ddahl]

Attachment: Latest patch

I must be losing it, ran hg qref again. sorry about that.

Comment 136

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 718488 [details] [diff] [review]
Latest patch

Review of attachment 718488 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Crypto.cpp
@@ +93,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +    NS_ASSERTION(dataLen == randomValues.Length(),
> +                 "Invalid length returned from parent process!");
> +    memcpy(data, static_cast<uint8_t*>(randomValues.Elements()), dataLen);

Shouldn't need the cast

@@ +100,2 @@
>  
> +    if (buf == nullptr) {

'!buf'

@@ +189,5 @@
> +    do_GetService("@mozilla.org/security/random-generator;1", &rv);
> +
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("unable to continue without random number generator");
> +    MOZ_ASSERT(!NS_FAILED(rv));

NS_ENSURE_SUCCESS(rv, nullptr);

@@ +194,5 @@
> +  }
> +  uint8_t* buf;
> +  rv = randomGenerator->GenerateRandomBytes(aLength, &buf);
> +
> +  MOZ_ASSERT(!NS_FAILED(rv));

NS_ENSURE_SUCCESS(rv, nullptr);

::: dom/ipc/ContentParent.cpp
@@ +2176,5 @@
> +
> +    randomValues->SetCapacity(length);
> +    randomValues->SetLength(length);
> +
> +    memcpy(static_cast<uint8_t*>(randomValues->Elements()), buf, length);

No cast

::: dom/ipc/PContent.ipdl
@@ +384,5 @@
>      PDeviceStorageRequest(DeviceStorageParams params);
>  
>      sync PCrashReporter(NativeThreadId tid, uint32_t processType);
>  
> +    sync GetRandomValues(uint32_t length) 

Whitespace

Comment 137

[David Dahl :ddahl]

Attachment: Latest patch, comments addressed, tests pass

Comment 138

[Mozilla RelEng Bot]

Try run for 1b3a7c89bb68 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1b3a7c89bb68
Results (out of 28 total builds):
    success: 28
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1b3a7c89bb68

Comment 139

[David Dahl :ddahl]

All tests pass on B2G and Desktop: https://tbpl.mozilla.org/?tree=Try&rev=1b3a7c89bb68

Comment 140

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 718689 [details] [diff] [review]
Latest patch, comments addressed, tests pass

Review of attachment 718689 [details] [diff] [review]:
-----------------------------------------------------------------

wfm

::: dom/ipc/ContentParent.cpp
@@ +2171,5 @@
>  bool
> +ContentParent::RecvGetRandomValues(const uint32_t& length,
> +                                   InfallibleTArray<uint8_t>* randomValues)
> +{
> +    uint8_t *buf = Crypto::GetRandomValues(length);

* to the left.

@@ +2172,5 @@
> +ContentParent::RecvGetRandomValues(const uint32_t& length,
> +                                   InfallibleTArray<uint8_t>* randomValues)
> +{
> +    uint8_t *buf = Crypto::GetRandomValues(length);
> + 

Whitespace

::: security/manager/ssl/src/nsRandomGenerator.cpp
@@ +34,5 @@
>    }
>    SECStatus srv = PK11_GenerateRandomOnSlot(slot, buf, aLength);
>  
>    if (SECSuccess != srv) {
> +    *aBuffer = nullptr;

Either always set *aBuffer on failure (at the start of the function), or never do it.

Comment 141

[David Dahl :ddahl]

Attachment: Latest Patch, comments addressed

Followed up with Ms2Ger's comments. Would like to land this asap.

Comment 142

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 719126 [details] [diff] [review]
Latest Patch, comments addressed

Review of attachment 719126 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Crypto.cpp
@@ +187,5 @@
> +  nsresult rv;
> +  randomGenerator =
> +    do_GetService("@mozilla.org/security/random-generator;1", &rv);
> +
> +  NS_ENSURE_SUCCESS(rv, nullptr);

Drop the rv from do_GetService and just do

NS_ENSURE_TRUE(randomGenerator, nullptr);

Comment 143

[David Dahl :ddahl]

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

Comment 144

[Ryan VanderMeulen [:RyanVM]]

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