683262 - window.crypto throws if MOZ_DISABLE_DOMCRYPTO is turned on

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

This causes dom/tests/mochitest/general/test_windowProperties.html to fail, since window.crypto is an accessible window property. Perhaps instead we should set the outparam to NULL and return NS_OK.

Comment 1

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

Attachment: Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties

Comment 2

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

Comment on attachment 618132 [details] [diff] [review]
Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties

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

I would prefer

interface nsIDOMCrypto {
#ifndef MOZ_DISABLE_DOMCRYPTOLEGACY
  // Stuff
#endif
};

or, if that doesn't work, two IDL files that declare nsIDOMCrypto, and picking the right one from the Makefile.

::: dom/base/nsDOMCrypto.cpp
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + *
> + * ***** BEGIN LICENSE BLOCK *****

MPL2

@@ +39,5 @@
> +#include "nsDOMCrypto.h"
> +#include "nsIDOMClassInfo.h"
> +#include "nsString.h"
> +
> +namespace mozilla { namespace dom {

Two lines

::: dom/base/nsDOMCrypto.h
@@ +35,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +#ifndef _nsDOMCrypto_h_

mozilla_dom_Crypto_h

@@ +44,5 @@
> +#define NS_DOMCRYPTO_CLASSNAME "Crypto JavaScript Class"
> +#define NS_DOMCRYPTO_CID \
> +  {0x929d9320, 0x251e, 0x11d4, { 0x8a, 0x7c, 0x00, 0x60, 0x08, 0xc8, 0x44, 0xc3} }
> +
> +namespace mozilla { namespace dom {

Two lines

@@ +46,5 @@
> +  {0x929d9320, 0x251e, 0x11d4, { 0x8a, 0x7c, 0x00, 0x60, 0x08, 0xc8, 0x44, 0xc3} }
> +
> +namespace mozilla { namespace dom {
> +
> +class nsDOMCrypto : public nsIDOMCrypto

"Crypto"

::: dom/base/nsGlobalWindow.cpp
@@ +3363,5 @@
>    }
>  
> +  nsresult rv;
> +  nsCOMPtr<nsIDOMCrypto> crypto = do_QueryInterface(mCrypto, &rv);
> +  crypto.forget((nsISupports**)aCrypto);

Remove cast. And QI.

Comment 3

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

Is mozilla::dom the correct namespace?

> or, if that doesn't work, two IDL files that declare nsIDOMCrypto, and
> picking the right one from the Makefile.

I tried #ifdef before and it didn't work. I already asked jst if this was the best approach and he said that it seemed reasonable. I am concerned that if we have two IDLs, then we have to keep them in sync. And, it would be unclear what to do about UUID management in that situation. So, I'd prefer to keep the structure the way it is now.

> > +class nsDOMCrypto : public nsIDOMCrypto
> 
> "Crypto"

Do you mean that this class should be named just mozilla::dom::Crypto?

> > +  nsresult rv;
> > +  nsCOMPtr<nsIDOMCrypto> crypto = do_QueryInterface(mCrypto, &rv);
> > +  crypto.forget((nsISupports**)aCrypto);
> 
> Remove cast. And QI.

Thanks. I've already fixed this in my local copy. I am still cleaning this stuff up.

Comment 4

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

Attachment: Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties [v3]

Comment 5

[David Dahl :ddahl]

Comment on attachment 633304 [details] [diff] [review]
Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties [v3]

That is a lot of #ifdef - and a lot of magic DOM code. Works great.

Comment 6

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

Comment on attachment 633304 [details] [diff] [review]
Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties [v3]

Kai, please review the changes under security/.
Doug, please review the changes under mobile/.
Olli, please review the changes under dom/.

Our goal is to get this and bug 440046 and bug 673432 into Firefox 16, so it would be good to have these reviews in done in the next two weeks or so.


Currently, we do not expose window.crypto on mobile platforms, mostly because we never built UI for all those features, and because we don't have any e10s support in PSM. Now, we want to expose window.crypto.getRandomValues and soon other new crypto functionality that is being standardized in the W3C. We don't want to block the implementation of getRandomValues and the DOMCrypt API on the implementation of the legacy methods in window.crypto.

Accordingly, I have split the nsIDOMCrypto interface into two parts: nsIDOMCrypto and nsIDOMCryptoLegacy. nsIDOMCrypto will expose all the DOMCrypt API (initially, just getRandomValues). nsIDOMCryptoLegacy will expose all the legacy functionality.

On mobile/e10s platforms, window.crypto will only implement nsIDOMCrypto, not nsIDOMCryptoLegacy.

in PSM, there is some code for monitoring smartcards and doing crypto operations for the legacy window.crypto methods that can be discarded at compile-time with #ifdefs, to eliminate dead code. I have done so in this patch. Further, because there are no smart card events without the legacy API, I put the creation and management of the smart card monitoring threads in #ifdefs too.

Comment 7

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

(In reply to Brian Smith (:bsmith) from comment #6)
> Accordingly, I have split the nsIDOMCrypto interface into two parts:
> nsIDOMCrypto and nsIDOMCryptoLegacy. nsIDOMCrypto will expose all the
> DOMCrypt API (initially, just getRandomValues). nsIDOMCryptoLegacy will
> expose all the legacy functionality.

Kai, also, the implementation of nsIDOMCrypto is in dom/, while the implementation of nsIDOMCryptoLegacy is in security/manager/ssl/src/nsCrypto.cpp, just like before. For the most part, as far as PSM is concerned, we basically just renamed nsIDOMCrypto to nsIDOMCryptoLegacy and made some code compile conditionally depending on the platform (mobile/e10s or not).

Comment 8

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

Comment on attachment 633304 [details] [diff] [review]
Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties [v3]



>diff --git a/dom/interfaces/base/nsIDOMCrypto.idl b/dom/interfaces/base/nsIDOMCryptoLegacy.idl
>copy from dom/interfaces/base/nsIDOMCrypto.idl
>copy to dom/interfaces/base/nsIDOMCryptoLegacy.idl
>--- a/dom/interfaces/base/nsIDOMCrypto.idl
>+++ b/dom/interfaces/base/nsIDOMCryptoLegacy.idl
>@@ -1,17 +1,19 @@
> /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * 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 "domstubs.idl"
> 
>-[scriptable, uuid(12b6d899-2aed-4ea9-8c02-2223ab7ab592)]
>-interface nsIDOMCrypto : nsISupports
>+interface nsIDOMCRMFObject;
>+
>+[scriptable, uuid(b50312fa-06ec-460c-b5e9-5dbb009eb457)]
>+interface nsIDOMCryptoLegacy : nsISupports
This should be probably nsICryptoLegacy, so that
("CryptoLegacy" in window) doesn't become true.
The filename shouldn't have nsIDOM either.



>diff --git a/dom/tests/mochitest/Makefile.in b/dom/tests/mochitest/Makefile.in
>--- a/dom/tests/mochitest/Makefile.in
>+++ b/dom/tests/mochitest/Makefile.in
>@@ -13,16 +13,17 @@ include $(DEPTH)/config/autoconf.mk
> DIRS	+= \
> 	dom-level0 \
> 	dom-level1-core \
> 	dom-level2-core \
> 	dom-level2-html \
> 	ajax \
> 	bugs \
> 	chrome \
>+  crypto \
Odd indentation

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 633304 [details] [diff] [review]
Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties [v3]

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

this looks fine to me, but blassey should review this.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 633304 [details] [diff] [review]
Refactor window.crypto implementation so we can expose window.crypto on e10s without the unimplemented methods/properties [v3]

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

r=blassey for mobile/* parts

Comment 11

[David Dahl :ddahl]

Attachment: Unbitrotted r+ Patch

Just unbitrotted this patch for work on bug 440046

Comment 12

[David Dahl :ddahl]

Latest patch may be causing this error (from bug 440046):

> Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d59d5b1b8906

> seeing: 12308 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: CryptoLegacy

Comment 13

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

> > Unexpected interface name in global scope: CryptoLegacy

We need to rename the nsIDOMCryptoLegacy interface to nsICryptoLegacy to fix this.

David, I am assigning this bug to you because it will be a while before I can get back to this.

I know this has been pending review for a while. Please ping me if the review isn't done by September 1.

Comment 14

[David Dahl :ddahl]

Attachment: Unbitrotted, crash on start

I just got this patch building again, but it crashes on start:

  0x00007f0aac419c33 in ah_crap_handler (signum=11) at /home/ddahl/code/moz/getRandomValues/src/toolkit/xre/nsSigHandlers.cpp:87
#3  <signal handler called>
#4  xpcObjectHelper::GetScriptableFlags (this=0x7ffff86d8990) at /home/ddahl/code/moz/getRandomValues/src/js/xpconnect/src/xpcObjectHelper.h:103
#5  0x00007f0aad9f97ee in nsXPConnect::InitClassesWithNewWrappedGlobal (this=0x7f0a9e6dc400, aJSContext=0x7f0ab208b690, aCOMObj=0x7f0a9bbac480, aPrincipal=0x7f0a9e7990a0, aFlags=0, 
    _retval=0x7ffff86d8b68) at /home/ddahl/code/moz/getRandomValues/src/js/xpconnect/src/nsXPConnect.cpp:1171
#6  0x00007f0aad289fd4 in CreateNativeGlobalForInner (aCx=0x7f0ab208b690, aNewInner=0x7f0a9bbac400, aURI=0x7f0a9bbd41a0, aPrincipal=0x7f0a9e7990a0, aNativeGlobal=0x7f0a9bbac658, 
    aHolder=0x7f0a9bbaae10) at /home/ddahl/code/moz/getRandomValues/src/dom/base/nsGlobalWindow.cpp:1715
#7  0x00007f0aad2882ee in nsGlobalWindow::SetNewDocument (this=0x7f0a9bbaac00, aDocument=0x7f0a9bba0000, aState=0x0, aForceReuseInnerWindow=false)
    at /home/ddahl/code/moz/getRandomValues/src/dom/base/nsGlobalWindow.cpp:1920

I will attach the full bt

Comment 15

[David Dahl :ddahl]

Attachment: bt-1

Backtrace

Comment 16

[David Dahl :ddahl]

Whoops, did not see this assertion...
###!!! ASSERTION: The number of items in sClassInfoData doesn't match the number of nsIDOMClassInfo ID's, this is bad! Fix it!: 'Error', file /home/ddahl/code/moz/getRandomValues/src/dom/base/nsDOMClassInfo.cpp, line 4594

Comment 17

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

(In reply to David Dahl :ddahl from comment #14)
> Created attachment 659799 [details] [diff] [review]
> Unbitrotted, crash on start
> 
> I just got this patch building again, but it crashes on start:

When you un-bitrotted the patch, did you also rename nsIDOMCryptoLegacy to nsICryptoLegacy?

Comment 18

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #17)
> (In reply to David Dahl :ddahl from comment #14)
> When you un-bitrotted the patch, did you also rename nsIDOMCryptoLegacy to
> nsICryptoLegacy?

Yes.

Comment 19

[David Dahl :ddahl]

gah! I found a few things that slipped past me. new patch asap.

Comment 20

[David Dahl :ddahl]

Attachment: Unbitrotted, 2nd try, crashes on start

Fixed a few problems with DOMCryptoLegacy -> CryptoLegacy naming, still crashes on start after checking the length of the list of NS_DEFINE_CLASSINFO_DATA macros in sClassInfoData

###!!! ASSERTION: The number of items in sClassInfoData doesn't match the number of nsIDOMClassInfo ID's, this is bad! Fix it!: 'Error', file /home/ddahl/code/moz/getRandomValues/src/dom/base/nsDOMClassInfo.cpp, line 4594

I set a break point and have the output of the contents of the list.

Comment 21

[David Dahl :ddahl]

Attachment: debug-output

debug output

Comment 22

[David Dahl :ddahl]

Attachment: backtrace

backtrace after unbitrot:

during startup / shortly thereafter...
Assertion failure: false (The number of items in sClassInfoData doesn't match the number of nsIDOMClassInfo ID's, this is bad! Fix it!), at /home/ddahl/code/moz/getRandomValues/src/dom/base/nsDOMClassInfo.cpp:4482

Comment 23

[David Dahl :ddahl]

Attachment: Latest patch

This error must be something about the number of properties not lining up correctly here in DOMWindow land - there are many #ifdefs here:)

Comment 24

[Johnny Stenback (:jst)]

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

- In dom/base/nsDOMClassInfo.cpp:

+#ifndef MOZ_DISABLE_CRYPTOLEGACY
 DOMCI_DATA_NO_CLASS(CRMFObject)
 DOMCI_DATA_NO_CLASS(SmartCardEvent)
+#endif

...

+#ifndef MOZ_DISABLE_CRYPTOLEGACY
   NS_DEFINE_CLASSINFO_DATA(CRMFObject, nsDOMGenericSH,
                            DOM_DEFAULT_SCRIPTABLE_FLAGS)
+  NS_DEFINE_CLASSINFO_DATA(SmartCardEvent, nsDOMGenericSH,
+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
+#endif

...
 
+#ifndef MOZ_DISABLE_CRYPTOLEGACY
+  DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(Crypto, nsICryptoLegacy)
+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMCrypto)
+    DOM_CLASSINFO_MAP_ENTRY(nsICryptoLegacy)
+  DOM_CLASSINFO_MAP_END
+
+  DOM_CLASSINFO_MAP_BEGIN(CRMFObject, nsIDOMCRMFObject)
+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMCRMFObject)
+  DOM_CLASSINFO_MAP_END
+
+  DOM_CLASSINFO_MAP_BEGIN(SmartCardEvent, nsIDOMSmartCardEvent)
+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMSmartCardEvent)
+  DOM_CLASSINFO_MAP_END
+#else
   DOM_CLASSINFO_MAP_BEGIN(Crypto, nsIDOMCrypto)
     DOM_CLASSINFO_MAP_ENTRY(nsIDOMCrypto)
   DOM_CLASSINFO_MAP_END
-
-  DOM_CLASSINFO_MAP_BEGIN(CRMFObject, nsIDOMCRMFObject)
-    DOM_CLASSINFO_MAP_ENTRY(nsIDOMCRMFObject)
-  DOM_CLASSINFO_MAP_END
+#endif

- In dom/base/nsDOMClassInfoClasses.h

 // Crypto classes
 DOMCI_CLASS(Crypto)
+#ifndef MOZ_DISABLE_CRYPTOLEGACY
 DOMCI_CLASS(CRMFObject)
+DOMCI_CLASS(SmartCardEvent)
+#endif

All these places need to line up exactly as far as what code is active and what order things are in whether MOZ_DISABLE_CRYPTOLEGACY is defined or not, right now they don't, and thus the assertion. If you fix that, the assertion should go away. If not, let me know and I'll dig deeper :)

Comment 25

[Josh Matthews [:jdm]]

Just a heads up about bug 803964 which addressed this issue in another way.

Comment 26

[David Dahl :ddahl]

(In reply to Josh Matthews [:jdm] from comment #25)
> Just a heads up about bug 803964 which addressed this issue in another way.

Are you saying bug 803964 somehow fixes the issue of window.crypto being #ifdef'd out of mobile release targets?

Comment 27

[Josh Matthews [:jdm]]

I'm saying that on targets that define MOZ_DISABLE_CRYPTOLEGACY, I'm pretty sure that the window.crypto property is undefined.

Comment 28

[David Dahl :ddahl]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #24)

> All these places need to line up exactly as far as what code is active and
> what order things are in whether MOZ_DISABLE_CRYPTOLEGACY is defined or not,
> right now they don't, and thus the assertion. If you fix that, the assertion
> should go away. If not, let me know and I'll dig deeper :)

Is it the order in which they appear in the file? I also noticed that the configure.in did not have MOZ_DISABLE_CRYPTOLEGACY, it was still  MOZ_DISABLE_DOMCRYPTOLEGACY

Anyway, I tried to make the order the same, but am not sure how to align things the way they should be. Any more pointers would be helpful. I am attaching the updated / unbitrotten patch

Comment 29

[David Dahl :ddahl]

Attachment: Latest Patch

Comment 30

[Johnny Stenback (:jst)]

Attachment: Fix assert

David, this patch on top of yours fixes the assert you mentioned here. Also please see my XXX comment in this patch.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 682261 [details] [diff] [review]
Latest Patch

Clearing feedback request per last comment.

Comment 32

[David Dahl :ddahl]

Attachment: Latest Patch, builds and runs

This builds and runs on desktop, with the patch in bug 440046 applied, all getRandomValues tests pass. I will next try to get this running on FX mobile.

Josh: do you see any pitfalls with this patch on mobile?

Comment 33

[Josh Matthews [:jdm]]

Comment on attachment 683426 [details] [diff] [review]
Latest Patch, builds and runs

No, I don't see any reason this should cause problems for mobile, unless sites have started relying on a null window.crypto object to sniff us. That seems unlikely at this point.

Comment 34

[David Dahl :ddahl]

Attachment: Latest patch unbitrot

Would like to know how this patch, combined with the patch in bug 440046 might need to change in order to work on mobile. What we want is a crypto object with only the non-legacy "getRandomValues" in it - this builds and runs on Desktop, but on mobile, I understand I will need to support content process windows where we cannot run NSS code. Also the changes in bug 803964 seem to complicate what we want here. Should I write a component in JS that communicates over the message manager to get the random values? If this is the case, then we should probably formulate a solution that also works on Firefox OS, as that may be very similar code.

With both patches applied (see bug 440046), I get this error: [Child 18535] ###!!! ASSERTION: Trying to initialize PSM/NSS in a non-chrome process!: 'Error', file /home/ddahl/code/moz/mobile/src/security/manager/ssl/src/nsNSSComponent.cpp, line 262

[Child 18535] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/ddahl/code/moz/mobile/src/dom/base/nsGlobalWindow.cpp, line 3296
TEST-UNEXPECTED-FAIL | unknown test url | Unexpected Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWindowCrypto.crypto]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://mochi.test:8888/tests/dom/tests/mochitest/crypto/test_getRandomValues.html?autorun=1&closeWhenDone=1&logFile=%2Fhome%2Fddahl%2Fcode%2Fmoz%2Fmobile%2Fobjdir%2Fmochitest-plain.log&fileLevel=INFO&consoleLevel=INFO&failureFile=/home/ddahl/code/moz/mobile/objdir/_tests/testing/mochitest/makefailures.json :: testNsCryptoGetRandomValues :: line 47"  data: no]


Also, the interface referenced here is nsIWindowCrypto, which I don't quite understand if that is a placeholder object how is it calling into NSS? TBH, I am unsure what combination and values of the config vars should be, right now in mobile/android/confvars.sh I have:

MOZ_DISABLE_DOMCRYPTO=0
MOZ_DISABLE_CRYPTOLEGACY=1

Sorry for all the dumb questions, both DOM and mobile are not my bag.

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

(sorry for taking so long to reply)

We don't need to support content processes for Mobile, but we may need to for b2g

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 686653 [details] [diff] [review]
Latest patch unbitrot

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

::: dom/base/Makefile.in
@@ +75,5 @@
>    nsWrapperCacheInlines.h \
>    nsContentPermissionHelper.h \
>    nsStructuredCloneContainer.h \
>    nsWindowMemoryReporter.h \
> +	Crypto.h \

ws

@@ +113,5 @@
>  	nsStructuredCloneContainer.cpp \
>  	nsDOMNavigationTiming.cpp \
>  	nsPerformance.cpp	\
>  	nsWindowMemoryReporter.cpp \
> +  Crypto.cpp \

ws

Comment 37

[David Dahl :ddahl]

(In reply to Brad Lassey [:blassey] from comment #36)
> Comment on attachment 686653 [details] [diff] [review]
> Latest patch unbitrot
> 
I keep seeing this error, and nothing actually works on fennec, it seems like the window is being created in a child process, which I thought was not the case:

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


I also reverted the nsIWindowCrypto patch, which I will attach.

Comment 38

[David Dahl :ddahl]

Attachment: revert nsIWindowCrypto interface

Comment 39

[David Dahl :ddahl]

Attachment: Latest Patch Unbitrot

Ok, this patch plus the (soon to be posted) patch in bug 440046 should be cleanly applicable against m-c

Comment 40

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to David Dahl :ddahl from comment #37)
> (In reply to Brad Lassey [:blassey] from comment #36)
> > Comment on attachment 686653 [details] [diff] [review]
> > Latest patch unbitrot
> > 
> I keep seeing this error, and nothing actually works on fennec, it seems
> like the window is being created in a child process, which I thought was not
> the case:
> 
> [Child 11740] ###!!! ASSERTION: Trying to initialize PSM/NSS in a non-chrome
> process!: 'Error', file
> /home/ddahl/code/moz/mobile/src/security/manager/ssl/src/nsNSSComponent.cpp,
> line 262

Is this in a debug build?

Comment 41

[David Dahl :ddahl]

(In reply to Brad Lassey [:blassey] from comment #40)

> > [Child 11740] ###!!! ASSERTION: Trying to initialize PSM/NSS in a non-chrome
> > process!: 'Error', file
> > /home/ddahl/code/moz/mobile/src/security/manager/ssl/src/nsNSSComponent.cpp,
> > line 262
> 
> Is this in a debug build?
Yes

Comment 42

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to fix build issues

Building with those 2 patches, plus this one to fix some missing includes, I don't see any assertions. What are you doing to get these assertions?

Comment 43

[David Dahl :ddahl]

(In reply to Brad Lassey [:blassey] from comment #42)
> Created attachment 692476 [details] [diff] [review]
> patch to fix build issues
> 
> Building with those 2 patches, plus this one to fix some missing includes, I
> don't see any assertions. What are you doing to get these assertions?

After building with the patch from bug 440046, run this: 

TEST_PATH=dom/tests/mochitest/crypto/test_getRandomValues.html make -C . mochitest-plain

Comment 44

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to David Dahl :ddahl from comment #43)
> (In reply to Brad Lassey [:blassey] from comment #42)
> After building with the patch from bug 440046, run this: 
> 
> TEST_PATH=dom/tests/mochitest/crypto/test_getRandomValues.html make -C .
> mochitest-plain

well that's odd, because that just plain shouldn't work on Android. To run that test on android I run this from my object directory:

MOZ_HOST_BIN="/Volumes/source/mozilla-central/objdir-firefox/dist/bin/" TEST_PATH=dom/tests/mochitest/crypto/test_gRandomValues.html make mochitest-remote

Comment 45

[Brad Lassey [:blassey] (use needinfo?)]

Also, it looks like those tests pass with my build.

Comment 46

[David Dahl :ddahl]

(In reply to Brad Lassey [:blassey] from comment #45)
> Also, it looks like those tests pass with my build.

Did you have the 2nd patch on this bug applied as well? (revert nsIWindowCrypto)

Comment 47

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to David Dahl :ddahl from comment #46)
> (In reply to Brad Lassey [:blassey] from comment #45)
> > Also, it looks like those tests pass with my build.
> 
> Did you have the 2nd patch on this bug applied as well? (revert
> nsIWindowCrypto)

yes

Comment 48

[David Dahl :ddahl]

bsmith:

Still seeing this: https://tbpl.mozilla.org/php/getParsedLog.php?id=18035178&tree=Try

13581 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: CryptoLegacy

Comment 49

[Brad Lassey [:blassey] (use needinfo?)]

Add CryptoLegacy to https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html?force=1#355 ?

Comment 50

[David Dahl :ddahl]

Comment on attachment 692476 [details] [diff] [review]
patch to fix build issues

Brian,

Should this patch be merges with the one in bug 440046, or the main patch here?

Comment 51

[David Dahl :ddahl]

Comment on attachment 691589 [details] [diff] [review]
revert nsIWindowCrypto interface

Brian:

Will this nsIWindowCrypto revert patch have a negative impact on b2g?

Comment 52

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

(In reply to David Dahl :ddahl from comment #48)
> bsmith:
> 
> Still seeing this:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=18035178&tree=Try
> 
> 13581 ERROR TEST-UNEXPECTED-FAIL |
> /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected
> interface name in global scope: CryptoLegacy

See comment 13. We need to rename the nsIDOMCryptoLegacy interface to nsICryptoLegacy, because something in the DOM code treats interfaces with the nsIDOM* prefix specially.

(In reply to David Dahl :ddahl from comment #51)
> Comment on attachment 691589 [details] [diff] [review]
> revert nsIWindowCrypto interface
> 
> Will this nsIWindowCrypto revert patch have a negative impact on b2g?

What is the purpose of this patch? b2g uses e10s, so we need to fix the e10s bug for b2g.

(In reply to David Dahl :ddahl from comment #37)
> [Child 11740] ###!!! ASSERTION: Trying to initialize PSM/NSS in a non-chrome
> process!: 'Error', file
> /home/ddahl/code/moz/mobile/src/security/manager/ssl/src/nsNSSComponent.cpp,
> line 262

That seems exceedingly strange. Try doing a clobber build for your Android objdir (or just try pushing to try). Also, check your Android mozconfig to make sure you're not enabling e10s for Android and make sure you're not trying to build the de-supported XUL Fennec instead of the native Firefox for Android.

Comment 53

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

Comment on attachment 692476 [details] [diff] [review]
patch to fix build issues

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

If the patches in this bug fail to build successfully without the patch being folded into them, then fold it into this bug. If this bug's other patches build successfully without this patch folded into them, but the other bug's patches fail to build without this patch, then fold it into the other bug.

Comment 54

[David Dahl :ddahl]

(In reply to Brian Smith (:bsmith) from comment #52)
> (In reply to David Dahl :ddahl from comment #48)
> > bsmith:
> > 
> > Still seeing this:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=18035178&tree=Try
> > 
> > 13581 ERROR TEST-UNEXPECTED-FAIL |
> > /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected
> > interface name in global scope: CryptoLegacy
> 

Actually, I fixed all of these mobile build problems with help from blassey. Everything works on a proper-to-the-device build, all tests pass. The interface name was added to the test as well. I'm just doing some final clean up before asking review, and was wondering (if) where to fold in the blassey patch.

> See comment 13. We need to rename the nsIDOMCryptoLegacy interface to
> nsICryptoLegacy, because something in the DOM code treats interfaces with
> the nsIDOM* prefix specially.

This was done - the latest patch has the renamed interface.
> 
> (In reply to David Dahl :ddahl from comment #51)
> > Comment on attachment 691589 [details] [diff] [review]
> > revert nsIWindowCrypto interface
> > 
> > Will this nsIWindowCrypto revert patch have a negative impact on b2g?
> 
> What is the purpose of this patch? b2g uses e10s, so we need to fix the e10s
> bug for b2g.
This nsIWindowCrypto interface was added by the devtools team because of some failures in their remote debugging for android. See bug 803964

Comment 55

[David Dahl :ddahl]

Note:

from bug 803964:

(In reply to Mihai Sucan [:msucan] from comment #18)
> David: I applied all of the patches from bug 683262. 'window' object
> inspection still works. The only difference I see is that window.crypto is
> now an object, but it is not inspectable.  What matters here is that it
> doesn't throw when we read the property.

Comment 56

[Mozilla RelEng Bot]

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

Comment 57

[David Dahl :ddahl]

Attachment: Latest patch, unbitrot

jst: I was hoping you could suggest the right person to review this patch. This works on mobile and desktop.

Comment 58

[David Dahl :ddahl]

Attachment: Lates revert nsIWindowCrypto patch

This patch reverts the nsIWindowCrypto interface that devtools team added as there was a problem inspecting the window with an empty or missing crypto property. The other larger patch on this bug re-creates the crypto object on window and allows us to add getRandomValues to it and catch up with chrome's implementation in bug 440046

Comment 59

[David Dahl :ddahl]

Attachment: Latest build fixes, unbitrot

Some build fixes in this patch by blassey - not sure if this should be folded into one of these other patches or the patch in bug 440046

Comment 60

[David Dahl :ddahl]

Comment on attachment 699206 [details] [diff] [review]
Latest build fixes, unbitrot

># HG changeset patch
># User Brad Lassey <[e-mail]>
># Date 1355526688 18000
># Node ID 566ea1470ebe740aa37cc84ce0d8244661844a60
># Parent e327915a4686f51e56886d79745c1b3edd1c18b5
>try: -b do -p linux,linux64,macosx64,win32,android,android-armv6,android-noion -u mochitests -t none --post-to-bugzilla Bug 683262
>
>diff --git a/security/manager/ssl/src/nsCrypto.cpp b/security/manager/ssl/src/nsCrypto.cpp
>--- a/security/manager/ssl/src/nsCrypto.cpp
>+++ b/security/manager/ssl/src/nsCrypto.cpp
>@@ -67,16 +67,18 @@
> 
> #include "ssl.h" // For SSL_ClearSessionCache
> 
> #include "nsNSSCleaner.h"
> 
> #include "nsNSSCertHelper.h"
> #endif
> 
>+#include "nsNativeCharsetUtils.h"
>+
> using namespace mozilla;
> 
> /*
>  * These are the most common error strings that are returned
>  * by the JavaScript methods in case of error.
>  */
> 
> #define JS_ERROR       "error:"
>diff --git a/security/manager/ssl/src/nsNSSComponent.cpp b/security/manager/ssl/src/nsNSSComponent.cpp
>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>@@ -79,16 +79,18 @@
> #include "cms.h"
> #include "nssckbi.h"
> #include "base64.h"
> #include "secerr.h"
> #include "sslerr.h"
> #include "cert.h"
> 
> #include "nsXULAppAPI.h"
>+#include "nsIRunnable.h"
>+#include "nsThreadUtils.h"
> 
> #ifdef XP_WIN
> #include "nsILocalFileWin.h"
> #endif
> 
> #include "pkcs12.h"
> #include "p12plcy.h"
>

Comment 61

[David Dahl :ddahl]

Attachment: Latest unbitrot

Comment 62

[David Dahl :ddahl]

Attachment: revert nsIWindowCrypto unbitrot

Comment 63

[David Dahl :ddahl]

Attachment: Window.crypto revamp - unbitrot

Comment 64

[David Dahl :ddahl]

Comment on attachment 711842 [details] [diff] [review]
Window.crypto revamp - unbitrot

Camilo: There are some (minor) psm parts in this bug, can you review them?

Comment 65

[Camilo Viecco (:cviecco)]

Comment on attachment 711842 [details] [diff] [review]
Window.crypto revamp - unbitrot

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

Great work. Almost r+
- remove that unreachable #ifdef block. 
- put check into the setenableSmartCardEvents

Nits to do:
- Lots of whitespace issues and some alignment issues.

Nits that would only matter if another reviewer agree.
- I would prefer the #define to have another name -> disable_cryptolegacy feels wrong-> disable_desktop_crypto?

::: dom/base/Makefile.in
@@ +77,5 @@
>    nsWrapperCacheInlines.h \
>    nsContentPermissionHelper.h \
>    nsStructuredCloneContainer.h \
>    nsWindowMemoryReporter.h \
> +	Crypto.h \

align

@@ +115,5 @@
>  	nsStructuredCloneContainer.cpp \
>  	nsDOMNavigationTiming.cpp \
>  	nsPerformance.cpp	\
>  	nsWindowMemoryReporter.cpp \
> +  Crypto.cpp \

align

::: dom/base/nsDOMClassInfo.cpp
@@ +506,2 @@
>  #ifdef MOZ_DISABLE_DOMCRYPTO
>    static const bool domCryptoEnabled = false;

Since you are removing the def, this line would never be reached. Remove the #ifdefs or rename the const.

::: dom/base/nsGlobalWindow.cpp
@@ +2031,5 @@
>    // clear smartcard events, our document has gone away.
> +#ifndef MOZ_DISABLE_CRYPTOLEGACY
> +  nsCOMPtr<nsICryptoLegacy> legacy = do_QueryInterface(mCrypto);
> +  if (legacy) {
> +    legacy->SetEnableSmartCardEvents(false);

This can fail, check for failure.

@@ +3599,2 @@
>  #else
> +  mCrypto = new Crypto();

align

Comment 66

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

(In reply to Camilo Viecco (:cviecco) from comment #65)
> Nits that would only matter if another reviewer agree.
> - I would prefer the #define to have another name -> disable_cryptolegacy
> feels wrong-> disable_desktop_crypto?

I think the current name is fine. "desktop" is bad because we're going to end up needing to support at least some of the legacy functionality on mobile too.

Comment 67

[David Dahl :ddahl]

Apparently, nsSmartCardEvents.h/.cpp have been removed - trying to figure out some build errors here:

In file included from ../../dist/include/nsDOMClassInfoID.h:21:
 0:49.33 ../../dist/include/nsDOMClassInfoClasses.h:154:1: error: redefinition of enumerator 'eDOMClassInfo_SmartCardEvent_id'
 0:49.33 DOMCI_CLASS(SmartCardEvent)
 0:49.33 ^
 0:49.33 ../../dist/include/nsDOMClassInfoID.h:17:3: note: expanded from macro 'DOMCI_CLASS'
 0:49.33   eDOMClassInfo_##_dom_class##_id,
 0:49.33   ^
 0:49.33 <scratch space>:51:1: note: expanded from macro 'eDOMClassInfo_'
 0:49.33 eDOMClassInfo_SmartCardEvent_id
 0:49.33 ^
 0:49.33 ../../dist/include/GeneratedEvents.h:17:1: note: previous definition is here
 0:49.33 MOZ_GENERATED_EVENT(SmartCardEvent)
 0:49.33 ^
 0:49.33 ../../dist/include/nsDOMClassInfoClasses.h:44:47: note: expanded from macro 'MOZ_GENERATED_EVENT'
 0:49.33 #define MOZ_GENERATED_EVENT(_event_interface) DOMCI_CLASS(_event_interface)
 0:49.33                                               ^
 0:49.33 ../../dist/include/nsDOMClassInfoID.h:17:3: note: expanded from macro 'DOMCI_CLASS'
 0:49.33   eDOMClassInfo_##_dom_class##_id,
 0:49.33   ^
 0:49.33 <scratch space>:131:1: note: expanded from macro 'eDOMClassInfo_'
 0:49.33 eDOMClassInfo_SmartCardEvent_id
 0:49.33 ^
 0:49.33 1 error generated.

It looks like nsIDOMSmartCardEvents is still generated. Not sure what to do here. Still digging...

Comment 68

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

See bug 839465. I bet you have to clobber.

Comment 69

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

clobber shouldn't be needed, but the patch just doesn't apply cleanly these days.

Comment 70

[David Dahl :ddahl]

Attachment: latest-window.crypto patch

unbitrotten and working again. comments addressed. 

jst (or bsmith): based on jst's irc commnets today, I was looking at the patch, which already has 2 idls, the legacy idl does not define the getRandomValues method. I think I remember that the legacy interface inherits from the new nsIDOMCrypto, which only defines getRandomValues. How does this affect the new concept where we ifdef in the makefile and define only an nsIDOMCrypto interface?

Comment 71

[David Dahl :ddahl]

(In reply to Olli Pettay [:smaug] from comment #69)
> clobber shouldn't be needed, but the patch just doesn't apply cleanly these
> days.

Yep, fixed it once i found your patch from bug 839465

Comment 72

[David Dahl :ddahl]

Attachment: Latest patch window.crypto - WIP based on jst comments

Not built yet, hoping this is the correct approach

Comment 73

[David Dahl :ddahl]

Attachment: Window.crypto latest WIP

The last patch had a Makefile problem. I think it still does. in dom/interfaces/base/Makefile.in - this is where I try to swap out the idl based on MOZ_DISABLE_CRYPTOLEGACY

Comment 74

[Johnny Stenback (:jst)]

Attachment: Updated window.crypto patch.

I worked with ddahl on finishing this up. The main difference from the latest patch from ddahl and this one is that this does not expose any new constructor classes, and doesn't introduce any new interface classes either. Everything remains in nsIDOMCrypto, but the contents (and iid) of that interface varies depending on whether legacy crypto is enabled or not.

I'll upload an inter diff as well.

Comment 75

[Johnny Stenback (:jst)]

Attachment: inter diff to assist in reviewing

Comment 76

[Johnny Stenback (:jst)]

Attachment: revert nsIWindowCrypto more unbitrot

This is ddahl's patch with a few tweaks, namely bump the IID in nsIDOMWindow, remove some leftover commented out code, clean up some silly code scoping in nsNSSComponent::DispatchEventToWindow(), and fix a potential null pointer crash in that same code.

Comment 77

[Johnny Stenback (:jst)]

Comment on attachment 714667 [details] [diff] [review]
Updated window.crypto patch.

And this is r=jst for the parts that I didn't change myself.

Comment 78

[Johnny Stenback (:jst)]

Comment on attachment 714671 [details]
revert nsIWindowCrypto more unbitrot

r=jst for the parts of this that I didn't write (which is most of it).

Comment 79

[David Dahl :ddahl]

Yay! Thank you jst!!

try server results are coming in here: https://tbpl.mozilla.org/?tree=Try&rev=2c1719a39f6f

Comment 80

[David Dahl :ddahl]

Comment on attachment 714667 [details] [diff] [review]
Updated window.crypto patch.

jst:

thanks for helping me with this. looks great.

Comment 81

[David Dahl :ddahl]

Comment on attachment 714671 [details]
revert nsIWindowCrypto more unbitrot

Thanks again for the deep dive on these patches.

Comment 82

[David Dahl :ddahl]

https://hg.mozilla.org/integration/mozilla-inbound/rev/895a7efc978d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b7606d94a6

Comment 83

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d72db9163971 for b2g mochitest failures in https://tbpl.mozilla.org/php/getParsedLog.php?id=19797814&tree=Mozilla-Inbound

Comment 84

[David Dahl :ddahl]

ok, try pushed after disabling crypto tests on b2g: https://tbpl.mozilla.org/?tree=Try&rev=c5bad072279b

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73437c0d0091

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a7cf5d271cb