Closed
Bug 561244
Opened 15 years ago
Closed 14 years ago
Remove window.crypto for e10s
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b2+ | --- |
People
(Reporter: jduell.mcbugs, Assigned: MikeK)
References
Details
Attachments
(4 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
We're turning off NSS in the content process. We thus need to forward any calls to https://developer.mozilla.org/en/JavaScript_crypto to chrome. This is needed for fennec 2.0, so must land by mid-June. Any takers?
Updated•15 years ago
|
Assignee: nobody → josh
Comment 2•15 years ago
|
||
This is completely untested, but theoretically complete. I'd like an executive decision as to whether it's worth splitting nsCrypto into separate chrome and content objects, or whether I can leave it as the #ifdef/conditional setup I have right now. Any other general comments appreciated/what's the best way to test whether this still works?
Attachment #441447 -
Flags: feedback?(jduell.mcbugs)
Comment 3•15 years ago
|
||
So when for example signText is called, it opens a modal dialog, right? Content process would then send a sync message to chrome which opens a dialog. This would mean that anything in the content process doesn't get painted while the dialog is open in chrome.
Updated•15 years ago
|
Attachment #441447 -
Flags: feedback?(jduell.mcbugs) → feedback?(kaie)
Comment 4•15 years ago
|
||
Comment on attachment 441447 [details] [diff] [review] WIP Kai, same feedback request from comment 2 applies.
Comment 5•15 years ago
|
||
I understand e10s is separate-process-per-tab. This patch doesn't appear to be complete, for example I see no caller to RecvSetEnableSmartCardEvents. (In reply to comment #2) > > This is completely untested, but theoretically complete. If you want to change things, I'll request that you do full regression testing of all touched features. > I'd like an executive > decision as to whether it's worth splitting nsCrypto into separate chrome and > content objects, or whether I can leave it as the #ifdef/conditional setup I > have right now. I'm not picky on this detail, making sure it still works is more important. > Any other general comments appreciated/what's the best way to > test whether this still works? You need special setup. For smartcards, you need to have a smartcard that's configured to work with encryption. Receive an encrypted smartcard. Go to thunderbird. Make sure that inserting a smartcard will trigger decrypting of a message. For CRMF request, the ultimate test is to test against a certificate system (such as dogtag) that uses this request to enroll for certificates. You might be able to test using a simple test case that I created on kuix.de, we could dig it up. It is mentioned in some bug around crmf. Importing user certificates? That's probably when you have enrolled for a certificate via web, and when it's ready, you pick up the public part of your new user certificate from a web download. You could test this using Verisign's testdrive enrollment available at https://digitalid.verisign.com/client/class1Netscape.htm You probably also must handle the <keygen> code that's also seen in Verisign's enrollment. The content processing of <keygen> tag triggers NSS to create a new public/private key pair.
Comment 6•15 years ago
|
||
PSM has too few tests, this is an area were we seriously need help. If you want to change the core of PSM, please add new tests for the functionality you touch.
Comment 7•15 years ago
|
||
jdm, so you're asking, is it find to have the initial-forwarding-code in the body of each existing function, or do you need a separate class for the forwarding stubs? IMHO, if it's limited to such an initial block in each forwarder, which exits after forwarding and doesn't interact with the remainder of the function, I'm absolutely find with having the forwarder inside the same function (for simplicity)
Updated•15 years ago
|
Attachment #441447 -
Flags: feedback?(kaie)
Reporter | ||
Comment 9•15 years ago
|
||
This blocks turning off NSS in content, not vice versa, unless I'm missing something.
Reporter | ||
Comment 10•15 years ago
|
||
Note that we can implement this in parts: fennec isn't going to need smartcard support, so we can leave that for a spinoff bug. Right now the entire JS crypto interface is unsupported from content processes under e10s, so any patches that implement features should probably be considered progress.
Comment 11•15 years ago
|
||
I'm focusing on other bugs right now, so I won't get upset if somebody else wants to run with this.
Assignee: josh → nobody
Comment 14•14 years ago
|
||
Hey, i just got a first look on that wip. actually i'm not 100% sure whats holding it up for fennec. Is it about missing tests? How should we proceed here?
Comment 15•14 years ago
|
||
tests+try server, then we can get a review request going. jeremias, could you also take a first pass through the code for correctness?
Comment 16•14 years ago
|
||
One major issue: nobody ever checked to ensure that the changes worked correctly.
Comment 19•14 years ago
|
||
Found a crash while manual testing with the script found on https://developer.mozilla.org/en/javascript_crypto mRemote wasn't initialized to nsnull. thats fixed with V3 If there are more tests around please point me to it.
Attachment #459258 -
Attachment is obsolete: true
Comment 21•14 years ago
|
||
Thanks i checked out i.e. http://kuix.de/misc/test3/crmf-rsa.php which is generating nothing in e10s at all, even with the patch. It ends up with NS_ERROR_NOT_AVAILABLE because xpc->GetCurrentNativeCallContext(&ncc); is putting ncc to be 0. Is something like that a known issue? How to handle that?
Comment 22•14 years ago
|
||
The callstack: #0 nsXPConnect::GetCurrentNativeCallContext (this=0xb7b56f20, aCurrentNativeCallContext=0xbfff86d0) at /scratchbox/users/jbos/home/jbos/mozilla-central/js/src/xpconnect/src/nsXPConnect.cpp:1824 #1 0xb5fb73d8 in nsCrypto::GenerateCRMFRequest (this=0xaaefe9a0, aReturn=0xbfff8794) at /scratchbox/users/jbos/home/jbos/mozilla-central/security/manager/ssl/src/nsCrypto.cpp:1854 #2 0xb6200ab2 in mozilla::dom::TabParent::RecvGenerateCRMFRequest (this=0xb623ef2a, rv=0xbfff884c, request=0xbfff8810) at /scratchbox/users/jbos/home/jbos/mozilla-central/dom/ipc/TabParent.cpp:593 #3 0xb623ef2a in mozilla::dom::PBrowserParent::OnMessageReceived (this=0xaae51830, __msg=..., __reply=@0xbfff89bc) at PBrowserParent.cpp:1107 #4 0xb62453c0 in mozilla::dom::PContentParent::OnMessageReceived (this=0xb7b2c280, __msg=..., __reply=@0xbfff89bc) at PContentParent.cpp:408 #5 0xb6223b99 in mozilla::ipc::SyncChannel::OnDispatchMessage (this=0xb7b2c288, msg=...) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/glue/SyncChannel.cpp:169 #6 0xb6220140 in mozilla::ipc::RPCChannel::OnMaybeDequeueOne (this=0xb7b2c288) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/glue/RPCChannel.cpp:436 #7 0xb6220b52 in DispatchToMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)()> (arg=<value optimized out>, method=<value optimized out>, obj=<value optimized out>) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/tuple.h:383 #8 RunnableMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)(), Tuple0>::Run (arg=<value optimized out>, method=<value optimized out>, obj=<value optimized out>) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/task.h:307 #9 0xb6220bbb in Run (this=<value optimized out>) at ../../dist/include/mozilla/ipc/RPCChannel.h:448 #10 mozilla::ipc::RPCChannel::DequeueTask::Run (this=<value optimized out>) at ../../dist/include/mozilla/ipc/RPCChannel.h:473 #11 0xb638b18a in MessageLoop::RunTask (this=0xb7b1b980, task=0xaae1eee0) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:339 #12 0xb638b562 in MessageLoop::DeferOrRunPendingTask (this=0x0, pending_task=...) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:347 #13 0xb638b7d5 in MessageLoop::DoWork (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:447 #14 0xb621ddc7 in mozilla::ipc::MessagePump::Run (this=0xb7ba46a0, aDelegate=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/glue/MessagePump.cpp:122 #15 0xb638bb01 in MessageLoop::RunInternal (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:219 #16 0xb638bbb0 in RunHandler (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:202 #17 MessageLoop::Run (this=0xb7b1b980) at /scratchbox/users/jbos/home/jbos/mozilla-central/ipc/chromium/src/base/message_loop.cc:176 #18 0xb612996e in nsBaseAppShell::Run (this=0xb17f6600) at /scratchbox/users/jbos/home/jbos/mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:175 #19 0xb5f69304 in nsAppStartup::Run (this=0xb14d2760) at /scratchbox/users/jbos/home/jbos/mozilla-central/toolkit/components/startup/src/nsAppStartup.cpp:191 #20 0xb54b4ddf in XRE_main (argc=1, argv=0xbfffe334, aAppData=0xb7b1fb40) at /scratchbox/users/jbos/home/jbos/mozilla-central/toolkit/xre/nsAppRunner.cpp:3630 #21 0x08049ff8 in main (argc=1, argv=0xbfffe334) at /scratchbox/users/jbos/home/jbos/mozilla-central/xulrunner/stub/nsXULStub.cpp:583
Comment 23•14 years ago
|
||
Ok I looked more into that and its getting complicated. The issue is, that the context is in the content process but the work done is in the chrome. Basically I think whats need to be done is: The parsing of the parameters must happen within in the content process, giving them into the chrome and doing there all the crypto work. Since i.e. for GenerateCRMFRequest there can also be a callback back into the javascript this must happen again within the content process. Whats the best way to do that?
Reporter | ||
Comment 24•14 years ago
|
||
> Whats the best way to do that? When we need to go back to content from chrome, we'll need to send more IPDL traffic. Chrome can't block, so the messages need to be async. For dialogs, we have one case where we've handled that already (HTTP auth: see the patch for bug 537782), so maybe you can get what you need from there. Round-trip IPDL traffic is expensive time-wise, so if there's a way to avoid it, we should do it. For instance, if the callbacks in GenerateCRMFRequest are just to grab values from the JS engine, we should grab those earlier if possible, and send them as part of the initial IPDL msg from content->chrome, and save an IPDL round trip.
Comment 25•14 years ago
|
||
It looks to me like GenerateCRMFRequest returns a solitary object: > nsCRMFObject *newObject = new nsCRMFObject(); > if (newObject == nsnull) { > JS_ReportError(cx, "%s%s\n", JS_ERROR, "could not create crmf JS object"); > > nsFreeKeyPairInfo(keyids,numRequests); > return NS_ERROR_OUT_OF_MEMORY; > } > newObject->SetCRMFRequest(encodedRequest); > *aReturn = newObject; If the chrome does the crypto generation and passes back the encodedRequest, which is just a string, then it should be easy to return the proper object. For the callback, you should have everything but the principals, which are serializable which gives me hope. dwitte or somebody else certainly knows more about those than I do, however. You should be able to factor out the bit starting after the xpc->WrapNative into a separate function that could run in chrome with some help. Note that cryptojs_ReadArgsAndGenerateKey just turns the js args into things like strings and then calls cryptojs_generateOneKeyPair which doesn't actually do anything with the JS context it receives. This gives me hope that you can just send some strings over the wire synchronously, do some work and just get back the final encoded value. But man, this is not fun code to read.
OS: Linux → Windows Server 2003
Comment 26•14 years ago
|
||
While doing this patch I was realizing that its way more complicated than I thought. The goal - which is getting rid of the nss lib in the child process isn't easily done by just ripping the object apart, basically to achieve that you need two classes, one in xul and one in the nss. its also not just ripping the function in two parts, this cut goes way deeper and you need carefully decide which parts can end up in the one and which in the other side. Since i have no Idea about the internals of the nss lib and not the time to get into them and because its just a fennec alpha blocker because of the child process loading the nss lib, i propose this patch to just remove the crypto object from global window and returning NS_ERROR_NOT_IMPLEMENTED for now. We should take care about bringing the crypto back in another bug, to a later time.
Attachment #459574 -
Attachment is obsolete: true
Attachment #459912 -
Flags: review?(doug.turner)
Comment 27•14 years ago
|
||
Comment on attachment 459912 [details] [diff] [review] Removes Crypto Object from nsGlobalWindow jst - can you review? I do not think anyone on mobile is going to cry to long about not having access to the crypto object in 2.0.
Attachment #459912 -
Flags: superreview?(jst)
Attachment #459912 -
Flags: review?(jst)
Attachment #459912 -
Flags: review?(doug.turner)
Updated•14 years ago
|
Attachment #459912 -
Flags: superreview?(jst)
Attachment #459912 -
Flags: superreview+
Attachment #459912 -
Flags: review?(jst)
Attachment #459912 -
Flags: review+
Updated•14 years ago
|
Keywords: checkin-needed
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
tracking-fennec: --- → 2.0a1+
Comment 28•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2335c842c06c marking fixed. Bug 582297 can track bring crypto back.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 29•14 years ago
|
||
test_windowProperties.html needs window.crypto also. MOZ_IPC is probably the wrong #define because firefox desktop has it set. backing this out for now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•14 years ago
|
tracking-fennec: 2.0a1+ → 2.0+
Comment 30•14 years ago
|
||
sorry, removing window.crypto is not ok. it's used by the web. please note that the n900 and potential successors want to be able to do certificate enrollment in enterprise environments. for information about window.crypto, you can read: https://developer.mozilla.org/en/JavaScript_crypto
Comment 32•14 years ago
|
||
fwiw, we have an activclient smart card reader + the software for it here. i specifically acquired it in order to be able to deal w/ this stuff.
OS: Windows Server 2003 → All
Comment 33•14 years ago
|
||
Comment on attachment 459912 [details] [diff] [review] Removes Crypto Object from nsGlobalWindow MOZ_IPC is enabled also in FF. :(
Attachment #459912 -
Flags: superreview-
Attachment #459912 -
Flags: superreview+
Attachment #459912 -
Flags: review-
Attachment #459912 -
Flags: review+
Updated•14 years ago
|
Keywords: checkin-needed
Comment 34•14 years ago
|
||
Technically this doesn't block Firefox, though it does block fennec. I think that means it doesn't need the blocking2.0 flag.
blocking2.0: ? → ---
Updated•14 years ago
|
Assignee: honzab.moz → mkristoffersen
Assignee | ||
Comment 36•14 years ago
|
||
Assignee | ||
Comment 37•14 years ago
|
||
Comment 38•14 years ago
|
||
Comment on attachment 478093 [details] [diff] [review] Part 1/3 - Adding uniq define to disable crypto object jst, similar to the last patch, but using a better #define.
Attachment #478093 -
Flags: review?(jst)
Updated•14 years ago
|
Attachment #478094 -
Flags: review?(khuey)
Updated•14 years ago
|
Attachment #478095 -
Flags: review+
Comment 39•14 years ago
|
||
Comment on attachment 478093 [details] [diff] [review] Part 1/3 - Adding uniq define to disable crypto object - In nsGlobalWindow::GetCrypto(nsIDOMCrypto** aCrypto): +#ifdef MOZ_DISABLE_DOMCRYPTO + return NS_ERROR_NOT_IMPLEMENTED; +#else Use 2-space indentation here. r=jst
Attachment #478093 -
Flags: review?(jst) → review+
Comment on attachment 478094 [details] [diff] [review] Part 2/3 - Making it possible to control the define by options Does this really need to be an actual configure option? Isn't having it exposed to an app's confvars enough? Also, lose the AC_SUBST.
Assignee | ||
Comment 41•14 years ago
|
||
(In reply to comment #40) > Comment on attachment 478094 [details] [diff] [review] > Part 2/3 - Making it possible to control the define by options > > Does this really need to be an actual configure option? Isn't having it > exposed to an app's confvars enough? What does it hurt to have it as a configure option? > > Also, lose the AC_SUBST. ok
We don't like adding configure options that aren't necessary. They just cause trouble when somebody decides to set a bunch of bizarre ones and then comes on #developers wondering why their build is broke.
Assignee | ||
Comment 43•14 years ago
|
||
(In reply to comment #42) > We don't like adding configure options that aren't necessary. They just cause > trouble when somebody decides to set a bunch of bizarre ones and then comes on > #developers wondering why their build is broke. When I finish biting my tongue I'll remove it...
Assignee | ||
Comment 44•14 years ago
|
||
(In reply to comment #43) > When I finish biting my tongue I'll remove it... btw, that comment was about process and group behavior, not about Kyle, he is doing the right thing by pointing this out to me, and as I said, I'll update it :)
Assignee | ||
Updated•14 years ago
|
Attachment #478094 -
Flags: review?(khuey)
Comment 46•14 years ago
|
||
Comment on attachment 479425 [details] [diff] [review] Part 1/3 - Adding uniq define to disable crypto object (v2 - updated after review) basically the same as the patch jst reviewed -- the difference here is one ws line @ 3097.
Attachment #479425 -
Flags: review+
Assignee | ||
Comment 48•14 years ago
|
||
(In reply to comment #47) > mikek - do we just need the patches named something 1/3 and 3/3? No we need 2/3 too - I have one that is updated with the comments from the review, just doing a sanity check on it - will upload it soon.
Assignee | ||
Updated•14 years ago
|
Attachment #479516 -
Flags: review?(khuey)
Attachment #479516 -
Flags: review?(khuey) → review+
Updated•14 years ago
|
tracking-fennec: 2.0+ → 2.0b2+
Comment 50•14 years ago
|
||
fennec bit: http://hg.mozilla.org/mobile-browser/rev/6596de4dd091 http://hg.mozilla.org/mozilla-central/rev/2b512173219b http://hg.mozilla.org/mozilla-central/rev/4c2cf1947818
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 52•13 years ago
|
||
This bug ended up being about removing window.crypto on e10s. Bug 582297 is about bringing this functionality back.
Summary: e10s: forward JS 'crypto' calls from content->chrome → Remove window.crypto for e10s
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•