Closed Bug 683262 Opened 13 years ago Closed 11 years ago

window.crypto throws if MOZ_DISABLE_DOMCRYPTO is turned on

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: jdm, Assigned: ddahl)

References

Details

Attachments

(3 files, 25 obsolete files)

51.75 KB, patch
ddahl
: review+
jst
: review+
Details | Diff | Splinter Review
12.65 KB, patch
Details | Diff | Splinter Review
7.63 KB, text/plain
ddahl
: review+
jst
: review+
Details
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.
Blocks: 682416
Assignee: nobody → ddahl
Assignee: ddahl → bsmith
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.
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 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.
Attachment #633304 - Flags: feedback?(ddahl) → feedback+
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #633304 - Flags: review?(kaie)
Attachment #633304 - Flags: review?(doug.turner)
Attachment #633304 - Flags: review?(bugs)
(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 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
Attachment #633304 - Flags: review?(bugs) → review+
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.
Attachment #633304 - Flags: review?(doug.turner) → review?(blassey.bugs)
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
Attachment #633304 - Flags: review?(blassey.bugs) → review+
Attached patch Unbitrotted r+ Patch (obsolete) — Splinter Review
Just unbitrotted this patch for work on bug 440046
Attachment #633304 - Attachment is obsolete: true
Attachment #633304 - Flags: review?(kaie)
Attachment #640791 - Flags: review?(kaie)
Attachment #640791 - Flags: review+
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
> > 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.
Assignee: bsmith → david
Assignee: david → ddahl
Attached patch Unbitrotted, crash on start (obsolete) — Splinter Review
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
Attachment #640791 - Attachment is obsolete: true
Attachment #640791 - Flags: review?(kaie)
Attachment #659799 - Flags: feedback?(bsmith)
Attached file bt-1 (obsolete) —
Backtrace
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
(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?
(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.
gah! I found a few things that slipped past me. new patch asap.
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.
Attachment #659799 - Attachment is obsolete: true
Attachment #659799 - Flags: feedback?(bsmith)
Attachment #659902 - Flags: feedback?(bsmith)
Attached file debug-output (obsolete) —
debug output
Attached file backtrace (obsolete) —
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
Attachment #659801 - Attachment is obsolete: true
Attachment #659903 - Attachment is obsolete: true
Attachment #659903 - Attachment is obsolete: false
Attached patch Latest patch (obsolete) — Splinter Review
This error must be something about the number of properties not lining up correctly here in DOMWindow land - there are many #ifdefs here:)
Attachment #659902 - Attachment is obsolete: true
Attachment #659902 - Flags: feedback?(bsmith)
Attachment #675298 - Flags: feedback?(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 :)
Attachment #675298 - Flags: feedback?(jst) → feedback-
Just a heads up about bug 803964 which addressed this issue in another way.
(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?
I'm saying that on targets that define MOZ_DISABLE_CRYPTOLEGACY, I'm pretty sure that the window.crypto property is undefined.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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
Attached patch Latest Patch (obsolete) — Splinter Review
Attachment #675298 - Attachment is obsolete: true
Attachment #682261 - Flags: feedback?(jst)
Attached patch Fix assert (obsolete) — Splinter Review
David, this patch on top of yours fixes the assert you mentioned here. Also please see my XXX comment in this patch.
Comment on attachment 682261 [details] [diff] [review]
Latest Patch

Clearing feedback request per last comment.
Attachment #682261 - Flags: feedback?(jst)
Attached patch Latest Patch, builds and runs (obsolete) — Splinter Review
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?
Attachment #659903 - Attachment is obsolete: true
Attachment #675292 - Attachment is obsolete: true
Attachment #682261 - Attachment is obsolete: true
Attachment #682358 - Attachment is obsolete: true
Attachment #683426 - Flags: feedback?(josh)
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.
Attachment #683426 - Flags: feedback?(josh) → feedback+
Attached patch Latest patch unbitrot (obsolete) — Splinter Review
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.
Attachment #683426 - Attachment is obsolete: true
Attachment #686653 - Flags: feedback?(blassey.bugs)
(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 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
Attachment #686653 - Flags: feedback?(doug.turner)
Attachment #686653 - Flags: feedback?(blassey.bugs)
Attachment #686653 - Flags: feedback+
(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.
Flags: needinfo?(blassey.bugs)
Attached patch revert nsIWindowCrypto interface (obsolete) — Splinter Review
Attached patch Latest Patch Unbitrot (obsolete) — Splinter Review
Ok, this patch plus the (soon to be posted) patch in bug 440046 should be cleanly applicable against m-c
Attachment #686653 - Attachment is obsolete: true
Attachment #686653 - Flags: feedback?(doug.turner)
Attachment #691975 - Flags: feedback?(blassey.bugs)
(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?
Flags: needinfo?(blassey.bugs)
(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
Attached patch patch to fix build issues (obsolete) — Splinter Review
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?
(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
(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
Also, it looks like those tests pass with my build.
(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)
(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
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
Flags: needinfo?(bsmith)
Attachment #691975 - Flags: feedback?(blassey.bugs) → feedback+
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?
Attachment #692476 - Flags: feedback?(bsmith)
Comment on attachment 691589 [details] [diff] [review]
revert nsIWindowCrypto interface

Brian:

Will this nsIWindowCrypto revert patch have a negative impact on b2g?
Attachment #691589 - Flags: feedback?(bsmith)
(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.
Flags: needinfo?(bsmith)
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.
Attachment #692476 - Flags: feedback?(bsmith)
(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
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.
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/ddahl@mozilla.com-6e47f7d7eed6
Attached patch Latest patch, unbitrot (obsolete) — Splinter Review
jst: I was hoping you could suggest the right person to review this patch. This works on mobile and desktop.
Attachment #692476 - Attachment is obsolete: true
Attachment #699199 - Flags: review?(jst)
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
Attachment #691589 - Attachment is obsolete: true
Attachment #691589 - Flags: feedback?(bsmith)
Attachment #699201 - Flags: review?(jst)
Blocks: 827829
Attached patch Latest build fixes, unbitrot (obsolete) — Splinter Review
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
Attachment #691975 - Attachment is obsolete: true
Attachment #699206 - Flags: review?(bsmith)
Comment on attachment 699206 [details] [diff] [review]
Latest build fixes, unbitrot

># HG changeset patch
># User Brad Lassey <blassey@mozilla.com>
># 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"
>
Attachment #699206 - Attachment is obsolete: true
Attachment #699206 - Flags: review?(bsmith)
Attached patch Latest unbitrot (obsolete) — Splinter Review
Attachment #699199 - Attachment is obsolete: true
Attachment #699199 - Flags: review?(jst)
Attachment #709380 - Flags: review?(jst)
Attached patch revert nsIWindowCrypto unbitrot (obsolete) — Splinter Review
Attachment #699201 - Attachment is obsolete: true
Attachment #699201 - Flags: review?(jst)
Attachment #709381 - Flags: review?(jst)
Blocks: 673432
Attached patch Window.crypto revamp - unbitrot (obsolete) — Splinter Review
Attachment #709380 - Attachment is obsolete: true
Attachment #709380 - Flags: review?(jst)
Attachment #711842 - Flags: review?(jst)
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?
Attachment #711842 - Flags: review?(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
Attachment #711842 - Flags: review?(cviecco) → review-
(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.
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...
See bug 839465. I bet you have to clobber.
clobber shouldn't be needed, but the patch just doesn't apply cleanly these days.
Attached patch latest-window.crypto patch (obsolete) — Splinter Review
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?
Attachment #711842 - Attachment is obsolete: true
Attachment #711842 - Flags: review?(jst)
Attachment #714261 - Flags: feedback?(jst)
Flags: needinfo?(jst)
(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
Not built yet, hoping this is the correct approach
Attachment #714270 - Flags: feedback?(jst)
Attached file Window.crypto latest WIP (obsolete) —
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
Attachment #714270 - Attachment is obsolete: true
Attachment #714270 - Flags: feedback?(jst)
Attachment #714359 - Flags: feedback?(jst)
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.
Attachment #714667 - Flags: review?(ddahl)
Flags: needinfo?(jst)
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.
Attachment #709381 - Attachment is obsolete: true
Attachment #709381 - Flags: review?(jst)
Attachment #714261 - Attachment is obsolete: true
Attachment #714261 - Flags: feedback?(jst)
Attachment #714359 - Attachment is patch: false
Attachment #714359 - Flags: feedback?(jst)
Attachment #714359 - Attachment is obsolete: true
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.
Attachment #714667 - Flags: review+
Attachment #714671 - Flags: review?(ddahl)
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).
Attachment #714671 - Flags: review+
Yay! Thank you jst!!

try server results are coming in here: https://tbpl.mozilla.org/?tree=Try&rev=2c1719a39f6f
Comment on attachment 714667 [details] [diff] [review]
Updated window.crypto patch.

jst:

thanks for helping me with this. looks great.
Attachment #714667 - Flags: review?(ddahl) → review+
Comment on attachment 714671 [details]
revert nsIWindowCrypto more unbitrot

Thanks again for the deep dive on these patches.
Attachment #714671 - Flags: review?(ddahl) → review+
ok, try pushed after disabling crypto tests on b2g: https://tbpl.mozilla.org/?tree=Try&rev=c5bad072279b
https://hg.mozilla.org/mozilla-central/rev/73437c0d0091
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.