Closed Bug 883741 Opened 11 years ago Closed 11 years ago

WebCrypto: Move Crypto to WebIDL

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 9 obsolete files)

Interdiff, fwiw
24.26 KB, patch
Details | Diff | Splinter Review
Patch v7.
50.17 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Part 2: Test for null. v2
1.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Currently in m-c, nsIDOMCrypto is still of XPIDL. For implementing webcrypto, we should start moving it to WebIDL.
Comment on attachment 769645 [details] [diff] [review] WIP - Patch Hi, smaug I'd like to request for your feedback here, in case I did something wrong already. Also there's one problem I am having now " ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../dist/include/nsCOMPtr.h, line 547" when running my test Gaia app. The assertion is from mCrypto in nsGlobalWindow::GetCrypto. But I am not sure what went wrong in my patch, I though NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION will define 'QueryInterface' method for me. Can you help to check my patch and kindly help to provide some tips to me? Thank you.
Attachment #769645 - Flags: feedback?(bugs)
Hmm, I can't see where the problem is. (It shouldn't matter that ISUPPORTS methods aren't DECL'ed first)
Comment on attachment 769645 [details] [diff] [review] WIP - Patch >+JSObject * >+Crypto::GetRandomValues(JSContext *aCx, ArrayBufferView& aArray) JSObject* Crypto::GetRandomValues(JSContext* aCx, ArrayBufferView& aArray) >+NS_IMETHODIMP >+Crypto::GetRandomValues(uint8_t *aData, uint32_t aDataLen) uint8_t* aData
Attachment #769645 - Flags: feedback?(bugs)
Hmm, does the patch work at all when we enter #ifndef MOZ_DISABLE_CRYPTOLEGACY in GetCrypto? Looks like we have two nsIDOMCrypto interfaces o_O' http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMCrypto.idl#9 http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMCryptoLegacy.idl#11 Wouldn't be surprised if that causes some issues.
Thanks for the tip, smaug. :) I'll check why I met the assertion and try my patch on Desktop (which crypto legacy is enabled).
> But I am not sure what went wrong in my patch, You're getting hit by bug 798188. Don't put NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY at the end of your QI impl; put it at the beginning.
(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #7) > You're getting hit by bug 798188. Don't put > NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY at the end of your QI impl; put it at > the beginning. Great thanks Boriz, that did fixed the assertion. :)
Hi Boriz I found DOMCameraManager.cpp [1] and nsGlobalWindow.cpp [2] also puts NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY at the end of QI impl. Is this a bug ? [1] :http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraManager.cpp#26 [2] :http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#1583 Thanks
Hi, ddahl Thanks for smaug's and bz's help, I already finish most pars of converting nsIDOMCrypto to WebIDL. However when I try to test my patch on desktop (which uses CRYPTO_LEGACY), I am not sure what should be done next. Should I add a compile option in WebIDL.mk? Like ifdef MOZ_DISABLE_CRYPTOLEGACY webidl_files += \ WebCrypto.webidl \ $(NULL) endif In that case I need to add many #ifdef MOZ_DISABLE_CRYPTOLEGACY in my patch because on desktop it's still using nsIDOMCryptoLegacy, which is still XPIDL. I am not sure whether I should add those legacy things into WebIDL since you mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=865789#c10 Or should I seperate the impl to two DOM objects, mozilla::dom::Crypto for nsIDOMCrypto, and mozilla::dom::CryptoLegacy for the older nsIDOMCryptoLegacy. Thanks
Flags: needinfo?(ddahl)
Both should be moved at the same time, IMO.
nsGlobalWindow is not a problem because the NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY does not come last. The cameramanager code seems buggy at first glance.
(In reply to :Ms2ger from comment #11) > Both should be moved at the same time, IMO. In that case I think I should file another bug for converting nsIDOMCryptoLegacy for WebIDL. The implementation is located in another file nsCrypto.cpp in NSS. And it should take quite a huge effort. But I don't know if it's worth or not to WebIDLize it. I think those code are related to DOMCrypto before, which seems deprecated by WebCrypto.
Flags: needinfo?(ddahl)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10) > I am not sure whether I should add those legacy things into WebIDL since you > mentioned in > https://bugzilla.mozilla.org/show_bug.cgi?id=865789#c10 > I was really only talking about any new Web Crypto API implementation work - not the conversion to WebIDL of existing crypto object. I assume we will continue to keep legacy methods in window.crypto for the foreseeable future - as much as we would like to nuke them. > Or should I seperate the impl to two DOM objects, > mozilla::dom::Crypto for nsIDOMCrypto, > and mozilla::dom::CryptoLegacy for the older nsIDOMCryptoLegacy. > I would ask bsmith about this.
(In reply to David Dahl :ddahl from comment #14) > > I was really only talking about any new Web Crypto API implementation work - > not the conversion to WebIDL of existing crypto object. I assume we will > continue to keep legacy methods in window.crypto for the foreseeable future > - as much as we would like to nuke them. > One point I'd like to use WebIDL is WebCrypto uses Promises, which is already WebIDLized. Also WebCrypto uses some string enumerations, which XPIDL might not be able to do it.(I think XPIDL cannot use string constants, but maybe I am wrong). The other point is Mozilla is starting moving XPIDL to WebIDL. I check nsIDOMCrypto.idl, and it's just one method. I think it's totally do-able, and one day nsIDOMCrypto will be moved to WebIDL anyway. So I try to do it in this bug. > > Or should I seperate the impl to two DOM objects, > > mozilla::dom::Crypto for nsIDOMCrypto, > > and mozilla::dom::CryptoLegacy for the older nsIDOMCryptoLegacy. > > > > I would ask bsmith about this. Needinfo? to Brian, Brian, how do you think if I create another CryptoLegacy.cpp (or perhaps using existing nsCrypto.cpp) which implements nsIDOMCryptoLegacy? Otherwise I have to add many #ifdef inside Crypto.cpp, and actually the implementation of nsIDOMCryptoLegacy is in another file nsCrytpo.cpp Thanks
Flags: needinfo?(brian)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #15) > Needinfo? to Brian, > Brian, how do you think if I create another CryptoLegacy.cpp (or perhaps > using existing nsCrypto.cpp) which implements nsIDOMCryptoLegacy? > > Otherwise I have to add many #ifdef inside Crypto.cpp, and actually the > implementation of nsIDOMCryptoLegacy is in another file nsCrytpo.cpp. I would do whatever is easier and whatever the DOM team things is reasonable. Since I never deal with DOM stuff except for this esoteric crypto API, I don't have a good sense of what the best way to deal with this WebIDL stuff is. In fact, I'm mostly learning from you. When we e10s-ify the window.crypto code, I would like to move the DOM-specific stuff to the DOM module (dom/), leaving the permission prompting and crypto logic in PSM. That is, I want to split the code that deals with nsIDOM* stuff and JS API stuff (which need to be in the child process) from the code that deals with NSS-related stuff and the permission prompts (which need to be in the parent process). This code is going to get heavily refactored in the process of doing that. So again I would say that you should do whatever the DOM team recommends, keeping in mind the future goal of splitting the code up that way.
Flags: needinfo?(brian)
Thanks for the feedback, Brian. Hi, bz, smaug, and Ms2ger Since Ms2ger has mentioned moving both (nsIDOMCrypto.idl and nsIDOMCryptoLegacy.idl) to WebIDL, I'll try that first. But welcome to provide some comments/feedbacks here.
What's on WebIDL bindings is an _object_ not an interface. So if the nsCrypto object is on WebIDL bindings, then that means WebIDL equivalents for all the interfaces it implements. In practice, you could condition whether it's on WebIDL bindings on MOZ_DISABLE_CRYPTOLEGACY, but that might lead to different behavior for getRandomValues on our different platforms, which seems undesirable. So I think the best approach is to unconditionally put nsCrypto on WebIDL bindings.
Attached patch WIP - Patch (obsolete) — — Splinter Review
Attachment #769645 - Attachment is obsolete: true
Comment on attachment 772606 [details] [diff] [review] WIP - Patch Hi, bz This is my latest WIP patch. Can you help to check this and give some feedback here? Thank you :)
Attachment #772606 - Flags: feedback?(bzbarsky)
Comment on attachment 772606 [details] [diff] [review] WIP - Patch >+NS_IMETHODIMP >+Crypto::GetRandomValues(uint8_t* aData, uint32_t aDataLen) Why NS_IMETHODIMP? Also, should the caller really ignore failures from this method? Seems like this should be a void method taking an ErrorResult instead.... This is changing the behavior of Get/SetEnableSmartCardEvents, GenerateCRMFRequest, GetVersion, ImportUserCertificates, etc on dom::Crypto to no-op instead of throwing. Why? For that matter, why are these methods even needed on this object? What ends up calling them? My current guess based on code inspection is that this patch totally breaks all those methods because it will call the dom::Crypto versions, not the nsCrypto ones. Which is why you had to add the static-cast hackery in nsGlobalWindow::SetNewDocument... You should remove mentions of nsIDOMCrypto from dom_quickstubs.qsconf. The ifdef bits in the WebIDL directory are OK for now until we can do ifdefs inside a WebIDL file. nsCrypto needs to have an init method or something so it will actually set the right window on dom::Crypto. nsCrypto::SetEnableSmartCardEvents used to throw in some cases but now suddenly swallows exceptions, no? >+nsCrypto::generateCRMFRequest(nsIDOMCRMFObject** aReturn) This method is using XPConnect stuff that will no longer exist on WebIDL bindings; it needs to be changed to actually take its arguments as arguments. And no need for NS_IMETHODIMP and two separate methods here, imo. >+ NS_ENSURE_SUCCESS(rv, nullptr); Swallowing exceptions is generally bad. Lots more of that in this patch (e.g. nsCrypto::PopChallengeResponse, Random, Logout, etc). >+nsCrypto::SignText(const nsAString& aStringToSign, Again, this needs to not use xpconnect stuff.
Attachment #772606 - Flags: feedback?(bzbarsky) → feedback-
Comment on attachment 772606 [details] [diff] [review] WIP - Patch Er, I meant feedback+. Though for future, it would help to say which aspects you're looking for feedback on...
Attachment #772606 - Flags: feedback- → feedback+
(In reply to Boris Zbarsky (:bz) from comment #21) Hi, Boriz Really appreciate for your quick feedback. And sorry for not mentioning which part needs your feedback, I was trying to request for your feedback on the DOM LEGACY part. Next time I'll keep this in mind. > Comment on attachment 772606 [details] [diff] [review] > WIP - Patch > > >+NS_IMETHODIMP > >+Crypto::GetRandomValues(uint8_t* aData, uint32_t aDataLen) > > Why NS_IMETHODIMP? Also, should the caller really ignore failures from this > method? > > Seems like this should be a void method taking an ErrorResult instead.... > Sure, I'll update this. > This is changing the behavior of Get/SetEnableSmartCardEvents, > GenerateCRMFRequest, GetVersion, ImportUserCertificates, etc on dom::Crypto > to no-op instead of throwing. Why? I'll ask bsmith for this. The reason I didn't add [Throw] in the WebIDL because I saw some comments in nsCrypto::SignText // XXX This code should return error codes, but we're keeping this // backwards compatible with NS4.x and so we can't throw exceptions. > For that matter, why are these methods > even needed on this object? What ends up calling them? > Do you mean I should try to remove these methods in dom::Crypto? I'll try this later. WebCrypto.webidl and CryptoLegacy.webidl use the same interface 'Crypto'. And its nativeType is dom::Crypto. I thought I have to put those interfaces in dom::Crypto when compiling CryptoLegacy.webidl.(Although the implementation is actually in nsCrypto.cpp) I haven't tested these functions yet. (Make it compile did already spent a lot of time). Will do more serious tests now. > My current guess based on code inspection is that this patch totally breaks > all those methods because it will call the dom::Crypto versions, not the > nsCrypto ones. Which is why you had to add the static-cast hackery in > nsGlobalWindow::SetNewDocument... > There's some background in this. When I was doing CryptoLegacy.webidl, There is one attribute called 'version'. readonly attribute DOMString version; In XPIDL, its signature is NS_IMETHOD GetVersion(nsAString & aVersion) Whereas in WebIDL, it's void GetVersion(nsAString& aVersion) (The return type is different) Therefore I have to remove inheritance from nsIDOMCrypto, and this also changes the code in nsGlobalWindow.cpp. > > nsCrypto::SetEnableSmartCardEvents used to throw in some cases but now > suddenly swallows exceptions, no? > Like above I'll also ask bsmith about throwing exception. > >+nsCrypto::generateCRMFRequest(nsIDOMCRMFObject** aReturn) > > This method is using XPConnect stuff that will no longer exist on WebIDL > bindings; it needs to be changed to actually take its arguments as > arguments. And no need for NS_IMETHODIMP and two separate methods here, imo. > Oh this method was changed to private and rename to 'generateCRMFRequest' with lower case 'g'. My intent is to try to keep this method not modified, and the new method (for WebIDL) nsCrypto::GenerateCRMFRequest() could call the original one (private one, nsCrypto::generateCRMFRequest). I'll re-check my patch again to address your comments. Thanks for the detail feedback. :)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #23) > (In reply to Boris Zbarsky (:bz) from comment #21) > >+nsCrypto::generateCRMFRequest(nsIDOMCRMFObject** aReturn) > > > > This method is using XPConnect stuff that will no longer exist on WebIDL > > bindings; it needs to be changed to actually take its arguments as > > arguments. And no need for NS_IMETHODIMP and two separate methods here, imo. > > > Oh this method was changed to private and rename to 'generateCRMFRequest' > with lower case 'g'. > My intent is to try to keep this method not modified, and the new method > (for WebIDL) nsCrypto::GenerateCRMFRequest() could call the original one > (private one, nsCrypto::generateCRMFRequest). > Please ignore my previous comment :) I understand what you meant now.
> void GetVersion(nsAString& aVersion) Or void GetVersion(nsString& aVersion) if that makes things simpler. Of course if it _doesn't_ throw then WebIDL can just use the XPIDL getter, and if it _does_ then it should be [Throws] in the IDL and then it has a different signature... > My intent is to try to keep this method not modified, and the new method (for WebIDL) > nsCrypto::GenerateCRMFRequest() could call the original one (private one, > nsCrypto::generateCRMFRequest). That can't work because the current code in GenerateCRMFRequest assumes it's called from XPConnect, which will no longer be true. You actually need to change the implementation to work without depending on XPConnect. Note that that this should not be too bad: WebIDL allows variadic arguments.
(In reply to Boris Zbarsky (:bz) from comment #25) > > void GetVersion(nsAString& aVersion) > > Or void GetVersion(nsString& aVersion) if that makes things simpler. > > Of course if it _doesn't_ throw then WebIDL can just use the XPIDL getter, > and if it _does_ then it should be [Throws] in the IDL and then it has a > different signature... > Hi bz: So if I add [Throws] in the WebIDL to make the signature is different, then I can make dom::Crypto still inherits nsIDOMCrypto and remove the static_cast in nsGlobalWindow. But now I have to keep the XPIDL implementation methods (since it inherits nsIDOMCrypto), so in that case I'll have two sets of methods, one for XPIDL, and the other is for WebIDL. (And I could make WebIDL ones to call XPIDL ones) Does this sound okay to you?
For now, keeping both sets of methods is fine; the xpidl ones will simply not be called from web code. You can't generally call the xpidl ones from the webidl ones because the former expect to be called from XPConnect. But for some of them you can do it, I'd think.
Though I would recommend making the XPIDL methods call the WebIDL ones, generally.
(In reply to Boris Zbarsky (:bz) from comment #27) > For now, keeping both sets of methods is fine; the xpidl ones will simply > not be called from web code. > > You can't generally call the xpidl ones from the webidl ones because the > former expect to be called from XPConnect. But for some of them you can do > it, I'd think. Yes for some getters/setters I could do calling XPIDL ones. But for the XPConnect (GenerateCRMFRequest as you mentioned comment 21) I think I will do it in another part 2 patch to smooth the review. Thanks for the quick answer.
Attached patch WIP - Patch. v3 (obsolete) — — Splinter Review
Attachment #772606 - Attachment is obsolete: true
Comment on attachment 773198 [details] [diff] [review] WIP - Patch. v3 Hi, bz Thanks for your previous suggestions, and now I did a shorter patch which is based on comment 21. (Although I am not quite sure what you meant in comment 28, the opposite way XPIDL->WebIDL) And this time I'd like to request for your feedback on the #ifndef MOZ_DISABLE_CRYPTOLEGACY #undef IMETHOD_VISIBILITY #define IMETHOD_VISIBILITY NS_VISIBILITY_DEFAULT #endif in dom::Crypto.h, In this way, I don't have to copy/paste the WebIDL->XPIDL code again in nsCrypto.cpp. Also I will remove the XPC code in my part 2 patch(Ongoing). Thanks again for your valuable feedback. :)
Attachment #773198 - Flags: feedback?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #21) > nsCrypto needs to have an init method or something so it will actually set > the right window on dom::Crypto. > Sorry I forgot to address this, will update it in next version.
> Although I am not quite sure what you meant in comment 28, the opposite way XPIDL->WebIDL Usually looks like this: ErrorResult rv; CallWebIDLMethod(args, rv); return rv.ErrorCode(); >#define IMETHOD_VISIBILITY NS_VISIBILITY_DEFAULT Why are we doing that??? >In this way, I don't have to copy/paste the WebIDL->XPIDL code again in nsCrypto.cpp. Let's back up a bit. WebIDL thinks that the Crypto interface maps to the Crypto object, right? The nsCrypto object is a subclass. Are Crypto objects ever instantiated directly when MOZ_DISABLE_CRYPTOLEGACY is not set? Because the way I see it, what should happen is that the non-legacy stuff should just live on Crypto. The legacy stuff should be defined (ifndef MOZ_DISABLE_CRYPTOLEGACY) as virtual functions on Crypto and overridden in nsCrypto. The functions on Crypto can either assert not reached if Crypto is directly instantiated or be pure virtual if it's not. Then you won't need any duplication, and it should all just work....
Hi bsmith In nsCrypto::SignText there's a comment // XXX This code should return error codes, but we're keeping this // backwards compatible with NS4.x and so we can't throw exceptions. But I cannot find any code which calling this method, and the patch comes from a very old bug Bug 29152. Do you think we can make it Throw now? Thanks
Flags: needinfo?(brian)
(In reply to Boris Zbarsky (:bz) from comment #33) Yeah your comment makes sense, I guess I was confused yesterday. Will address your comments right away. Thank you for your feedback again.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32) > (In reply to Boris Zbarsky (:bz) from comment #21) > > nsCrypto needs to have an init method or something so it will actually set > > the right window on dom::Crypto. > > > > Sorry I forgot to address this, will update it in next version. Actually I am not sure how to fix this nicely. In nsGlobalWindow.h, mCrypto is of type nsCOMPtr<nsIDOMCrypto>. If I add a Init, which means I need to update the IDL to add a 'Init' method. But if I change mCrypto to nsRefPtr<mozilla::dom::Crypt>, in do_CreateInstance I need to do the static cast you mentioned in comment 21. Like nsCOMPtr<nsIDOMCrypto> crypto; crypto = do_CreateInstance(NS_CRYPTO_CONTRACTID); mCrypto = static_cast<mozilla::dom::Crypto*(crypto.get()); ... mCrypto->Init(this); Do you have some suggestion for this? Thanks
> If I add a Init, which means I need to update the IDL to add a 'Init' method. Sure. Why is this a problem? Just add a noscript (and notxpcom, probably) init method. Doing static_cast on something you got by contractid is generally not an OK thing to do, because contracts can be overridden by extensions. If you plan to static_cast, pass a classid to createInstance.
Attachment #773198 - Flags: feedback?(bzbarsky) → feedback-
Attached patch WIP - Patch. v4 (obsolete) — — Splinter Review
Attachment #773198 - Attachment is obsolete: true
Comment on attachment 775578 [details] [diff] [review] WIP - Patch. v4 Hi, BZ Thanks for your previous comments. This patch I've done 1. Add init in the IDL. 2. Remove XPConnect stuff from nsCrypto.cpp 3. Add variadic arguments for CryptoLegacy.webidl 4. use nsWeakPtr in nsCrypto to avoid memory leak (Perhaps I should update the cycle collection macros, but seems using nsWeakPtr just did the trick) 5. Using XPIDL calling WebIDL functions in nsCrypto, and for the GenenateCRMFRequest and SignText, I think XPIDL ones are not neccesary anymore so I just return UNIMPLEMENTED. Can you kindly help to provide some tips or comments on these things? Also there's one thing I am not sure, I don't know the correct way to get the JS::RootedObject in nsCrypto.cpp#1959 Can you also help me here? Thanks a lot.
Attachment #775578 - Flags: feedback?(bzbarsky)
Flags: needinfo?(brian)
Comment on attachment 775578 [details] [diff] [review] WIP - Patch. v4 Review of attachment 775578 [details] [diff] [review]: ----------------------------------------------------------------- Please create a patch with 8 lines of context. ::: dom/webidl/CryptoLegacy.webidl @@ +19,5 @@ > + [SetterThrows] > + attribute boolean enableSmartCardEvents; > + > + [Throws] > + CRMFObject generateCRMFRequest(DOMString... args); Should be 'ByteString reqDN, ByteString regToken, ByteString authenticator, ByteString eaCert, ByteString jsCallback, any... args'
Attached patch WIP - Patch. v4 (obsolete) — — Splinter Review
Thanks for Ms2ger's reminding. Re-Format it to hg patch. Hi BZ, Please see my comment 39 for this version of patch. Also I'd also address Ms2ger's comment 40 later. Thanks
Attachment #775578 - Attachment is obsolete: true
Attachment #775578 - Flags: feedback?(bzbarsky)
Attachment #775592 - Flags: feedback?(bzbarsky)
(In reply to :Ms2ger from comment #40) > Comment on attachment 775578 [details] [diff] [review] > WIP - Patch. v4 > > Review of attachment 775578 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please create a patch with 8 lines of context. > > ::: dom/webidl/CryptoLegacy.webidl > @@ +19,5 @@ > > + [SetterThrows] > > + attribute boolean enableSmartCardEvents; > > + > > + [Throws] > > + CRMFObject generateCRMFRequest(DOMString... args); > > Should be 'ByteString reqDN, ByteString regToken, ByteString authenticator, > ByteString eaCert, ByteString jsCallback, any... args' Hi, Ms2ger But from Bug 327524, it seems it could have just one argument?
The code has... 1865 if (argc < 5 || ((argc-5) % 3) != 0) { 1866 JS_ReportError(cx, "%s%s", JS_ERROR, 1867 "incorrect number of parameters"); 1868 return NS_ERROR_FAILURE; 1869 } so it seems like we need at least 5 arguments.
Just wanted to mention some cleanup that might make sense in this bug or parallel bugs dealing with window.crypto: https://bugzilla.mozilla.org/show_bug.cgi?id=824652#c8
(In reply to David Dahl :ddahl from comment #44) > Just wanted to mention some cleanup that might make sense in this bug or > parallel bugs dealing with window.crypto: > https://bugzilla.mozilla.org/show_bug.cgi?id=824652#c8 You mean JS_ReportError? It seems easy and I'd try to remove them if necessary.
Comment on attachment 775592 [details] [diff] [review] WIP - Patch. v4 Why is GetVersion marked as throwing? Doesn't look to me like it can throw.... Also, drop the .get() there. GenerateCRMFRequest is introducing a whole bunch of behavior changes (e.g. in terms of how null vs "" is handled). Are those intentional? Why are they being made? Why are we interning these strings? That seems pretty odd. Also, I see absolutely no reason to do it. What's the goal? > I think XPIDL ones are not neccesary anymore so I just return UNIMPLEMENTED. Why not just remove them from the xpidl? What's the story with the exception stuff in signText? Either we're throwing exceptions (and then we should do things like use a WebIDL enum for the enumerated argument), or we're not and then we shouldn't be throwing them. > I don't know the correct way to get the JS::RootedObject in nsCrypto.cpp#1959 GetWrapper() should work.
Attachment #775592 - Flags: feedback?(bzbarsky) → feedback+
Hi, BZ Thanks for the feedback (In reply to Boris Zbarsky (:bz) from comment #46) > Comment on attachment 775592 [details] [diff] [review] > WIP - Patch. v4 > > Why is GetVersion marked as throwing? Doesn't look to me like it can > throw.... Also, drop the .get() there. > The signature between XPIDL and WebIDL are the same (except the return type), and you commented in Comment 25 suggested I'd could use Throws to make the signature different. > GenerateCRMFRequest is introducing a whole bunch of behavior changes (e.g. > in terms of how null vs "" is handled). Are those intentional? Why are > they being made? > My mistake. Will check nsString API for checking null again. > Why are we interning these strings? That seems pretty odd. Also, I see > absolutely no reason to do it. What's the goal? > The variadic arguments are of type 'nsString', but the original code uses JSAutoByteString.encodeLatin1', so I intern those strings to do nsString->JSAutoBytesString Or do you have better suggestion here? > Why not just remove them from the xpidl? Those are from Comment 26 and Comment 28. Also XPIDL ones are remained to implement methods from nsIDOMCrypto. So nsGlobalWindow.cpp could do less 'static_cast' code. I'll try to make the most XPIDL methods return unimplemented to simply my patch. Thanks
> and you commented in Comment 25 suggested I'd could use Throws to make the signature > different. Er... that's not what I said. I said that _if_ the method ever throws is should be [Throws] and if not then WebIDL can just call the XPIDL getter. > Or do you have better suggestion here? Comment 40 covered it, except use "ByteString?" if you want to allow null as a value, of course. > Also XPIDL ones are remained to implement methods from nsIDOMCrypto. Yes, but why not change nsIDOMCrypto to remove the unimplemented methods?
(In reply to Boris Zbarsky (:bz) from comment #48) > > Also XPIDL ones are remained to implement methods from nsIDOMCrypto. > > Yes, but why not change nsIDOMCrypto to remove the unimplemented methods? Sorry for asking this again. Do you mean I should remove 'random', 'popChallengeResponse' and 'disableRightClick' those methods from nsIDOMCryptoLegacy.idl? Or I should remove the 3 methods above, with including 'generateCRMFRequest' and 'signText', total 5 methods, from nsIDOMCryptoLegacy.idl? And how about also removing 'random', 'popChallengeResponse' and 'disableRightClick' in CryptoLegacy.webidl? Also how about removing all the methods in nsIDOMCrypto.idl ? Since XPIDL methods won't be called. Thanks
Generally speaking, anything that you're just making return NS_ERROR_NOT_IMPLEMENTED should just be removed. Certainly, generateCRMFRequest and signText should go away from nsIDOMCryptoLegacy, since that's only usable from c++ after this patch and those methods make no sense from C++. > And how about also removing 'random', 'popChallengeResponse' and 'disableRightClick' in > CryptoLegacy.webidl? No, since that changes web-facing API, right? > Also how about removing all the methods in nsIDOMCrypto.idl That seems fine to me if there are no C++ consumers.
(In reply to Boris Zbarsky (:bz) from comment #48) > > Or do you have better suggestion here? > > Comment 40 covered it, except use "ByteString?" if you want to allow null as > a value, of course. Hi, BZ But even using ByteString in the WebIDL, it seems I still have to intern or new a JSString to call the JSAutoByteString.encodeLatin1. Otherwise I think I have to include CharacterEncoding.h to construct to LatinCharsZ. Which one is better? Or you got another suggestion? Thanks
Note that now that bug 838146 has landed you can just use #ifdef in your WebIDL (if you add it to preprocessed_webidl_files) instead of having two different WebIDL files. > But even using ByteString in the WebIDL, it seems I still have to intern or new a > JSString to call the JSAutoByteString.encodeLatin1. Why do you need to call JSAutoByteString.encodeLatin1? What's the goal?
(In reply to Boris Zbarsky (:bz) from comment #52) > > But even using ByteString in the WebIDL, it seems I still have to intern or new a > > JSString to call the JSAutoByteString.encodeLatin1. > > Why do you need to call JSAutoByteString.encodeLatin1? What's the goal? > Hi, BZ Thanks for pointing out the #ifdef tip. And for the encodeLatin1, the original code uses it already. http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCrypto.cpp#1887
> And for the encodeLatin1, > the original code uses it already. Yes, I know that the original code uses it. But _why_ does it use it? What is the output of the encodeLatin1 call?
Sorry for asking stupied question before, I think encodeLatin1 should be unnecessary now if we already got nsCString here. Thanks
That was sort of my feeling: as far as I can tell ByteString does all the work for you here.
Attached patch Patch v5 (obsolete) — — Splinter Review
Attachment #775592 - Attachment is obsolete: true
Comment on attachment 777054 [details] [diff] [review] Patch v5 Hi, BZ Thanks for your valuable feedback. I think I've finished the patch. This patch is done with some extra modifications - Using #ifdef in WebCrypto.webidl - removed unused methods in nsIDOMCrypto and nsIDOMCrypoLegacy, and hence removed those XPIDL methods in mozilla::dom::Crypto and nsCrypto. - Remove JSString in nsCrypto - Update the signature of generateCrmfRequest by Ms2ger's suggestions. Can you kindly help to review this for me ? Thanks
Attachment #777054 - Flags: review?(bzbarsky)
Attached patch Patch v5 (obsolete) — — Splinter Review
forgot to qref
Attachment #777054 - Attachment is obsolete: true
Attachment #777054 - Flags: review?(bzbarsky)
Comment on attachment 777614 [details] [diff] [review] Patch v5 Hi, BZ Please see comment 58 for this patch. Thanks
Attachment #777614 - Flags: review?(bzbarsky)
Comment on attachment 777614 [details] [diff] [review] Patch v5 >+++ b/dom/base/Crypto.cpp >+Crypto::Init(nsIDOMWindow* aWindow) >+ mWindow = static_cast<nsPIDOMWindow*>(aWindow); Probably safer to do: mWindow = do_QueryInterface(aWindow); MOZ_ASSERT(mWindow); Alternately, you could declare init() as taking an nsPIDOMWindow in the IDL, but that's a bit annoying to do. >+Crypto::GetRandomValues(JSContext* aCx, ArrayBufferView& aArray, ErrorResult& aRv) >+ uint8_t* data = reinterpret_cast<uint8_t*>(aArray.Data()); You don't need the cast. dom::ArrayBufferView::Data returns uint8_t*. ;) >+ return aArray.Obj(); I would prefer "return view;" here until we get our rooting story sorted out for DOM typed array structs. >+Crypto::GetRandomValues(uint8_t* aData, uint32_t aDataLen) Why do we need to split this out into a separate method? It's only called from the WebIDL getRandomValues now, as far as I can tell. Please merge it back into that method unless there's a good reason for the split. > Crypto::GetEnableSmartCardEvents(bool *aEnableSmartCardEvents) Please mark this attribute noscript in the xpidl. In fact, maybe the entire nsIDOMCrypto interface can stop being scriptable... >+++ b/dom/base/Crypto.h >+ virtual JSObject * >+ GetRandomValues(JSContext* aCx, ArrayBufferView& aArray, ErrorResult& aRv); This doesn't need to be virtual, I don't think. >+ GenerateCRMFRequest( JSContext* aContext, For what it's worth, I think lining things up with the bit after "const " both here and in other methods on this interface, is more confusing than it's worth. I'd just remove the extra spaces. >+ GetParentObject() const >+ NS_WARNING("Crypto:GetParentObject"); Please take the warning out. There's no reason to warn here. >+++ b/dom/bindings/Bindings.conf >+ 'headerFile': 'Crypto.h' The other option is to export Crypto.h to mozilla/dom.... This is OK for now, but maybe a followup on exporting it and removing this annotation? >+++ b/dom/interfaces/base/nsIDOMCrypto.idl >+ [noscript, notxpcom] void init(in nsIDOMWindow window); Drop the extra space before "void". >+++ b/dom/interfaces/base/nsIDOMCryptoLegacy.idl >+ [noscript, notxpcom]void init(in nsIDOMWindow window); Actually, move that space here. And drop the extra spaces before "init". ;) >+++ b/dom/webidl/WebCrypto.webidl >+Crypto implements RandomSource; Please put that right after the RandomSource interface definition, before the MOzilla-specific ifdefed parts start, since this part is actually defined by the spec. Also, I think this file should be called Crypto.webidl... >+++ b/security/manager/ssl/src/nsCrypto.cpp >@@ -68,16 +68,17 @@ > nsCrypto::nsCrypto() : >+ mWindow(nullptr) Why do we need this mWindow member? We have a perfectly reasonable mWindow on the superclass... Please take it out. >+nsCrypto::Init(nsIDOMWindow* aWindow) This should presumably just call Crypto::Init. >+nsCrypto::GenerateCRMFRequest( JSContext* aContext, >+ if (argc > 0 && argc % 3 != 0) { No need for the > 0 test. If argc == 0, then certainly argc % 3 == 0. >+ NS_WARNING("incorrect number of parameters"); >+ aRv.Throw(NS_ERROR_FAILURE); How about taking out the NS_WARNING and using aRv.ThrowNotEnoughArgsError() to give a better exception? >- if (JSVAL_IS_NULL(argv[0])) { >+ if (!aReqDN.Data()) { That's not the same thing at all. In particular, aReqDN.Data() will _never_ be null here. If you want to keep the test as it was, make the reqDN argument nullable in the WebIDL and use aReqDN.IsVoid() to test for null. >+ if (!aJsCallback.Data()) { Similar issue here. if (aJsCallback.IsVoid()) That said, there was no test like this before (throwing on null) that I can see... why are we adding one now? What was the old behavior when null was passed in? >+ if (aEaCert.Data()) { if (!aEaCert.IsVoid()) { >+ SECStatus srv = ATOB_ConvertAsciiToItem(&certDer, aEaCert.Data()); .get(), please, so if the string ever becomes non-null-terminated this will fail to compile instead of doing the wrong thing. >+ if (!okay) { >+ return nullptr; The IDL says this function never returns null. Either this codepath needs to throw, or to preserve the old behavior of returning null without throwing you can leave this code as-is but change the IDL return value to "CRMFObject?". >+ const_cast<char*>(aReqDN.Data()), >+ const_cast<char*>(aRegToken.Data()), >+ const_cast<char*>(aAuthenticator.Data()), .get() instead of .Data(). >+ args->m_jsCallback.Adopt(aJsCallback.Data() ? nsCRT::strdup(aJsCallback.Data()) : 0); That doesn't have the same behavior as before. The old behavior was that aJSCallback being null would cause m_jsCallback to be null. You probably want the following (after making aJSCallback nullable in the IDL): if (!aJSCallback.isVoid()) { args->m_jsCallback = aJSCallback; } >+nsCrypto::PopChallengeResponse(const nsAString& aChallenge, >+nsCrypto::Random(int32_t aNumBytes, nsAString& aReturn, ErrorResult& aRv) >+nsCrypto::DisableRightClick(ErrorResult& aRv) Please file a followup bug to remove these methods that we don't actually implement? >+ caNames[i] = const_cast<char*>(aArgs[i].Data()); .get() >+++ b/security/manager/ssl/src/nsCrypto.h >+using namespace mozilla; >+using namespace mozilla::dom; No, please don't. That will force those using declarations on anyone including the header. You can put those in the .cpp, but in the header you'll have to stick with the fully qualified names. Incentives to put nsCrypto in the mozilla::dom namespace. ;) >+ virtual bool EnableSmartCardEvents(); MOZ_OVERRIDE here and on the various other virtual methods being added. >+ nsWeakPtr mWindow; Like I said, I think having this is wrong. r- because I'd like to see the updated patch, but the above is looking pretty good!
Attachment #777614 - Flags: review?(bzbarsky) → review-
Comment on attachment 777614 [details] [diff] [review] Patch v5 Review of attachment 777614 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsCrypto.cpp @@ +1952,5 @@ > PK11SlotInfo *slot = nullptr; > // Go through all of the arguments and generate the appropriate key pairs. > + for (i=0,keyInfoIndex=0; i<argc; i+=3,keyInfoIndex++) { > + nrv = cryptojs_ReadArgsAndGenerateKey(aContext, > + const_cast<JS::Value*>(&aArgs[i]), This sure looks fishy...
You mean the const_cast? It sure is fishy, but OK in this case. In a sane world, cryptojs_ReadArgsAndGenerateKey would take a Handle<Value> or something....
Thanks for the review, BZ. (In reply to Boris Zbarsky (:bz) from comment #61) > Comment on attachment 777614 [details] [diff] [review] > Patch v5 > > > >+ return aArray.Obj(); > > I would prefer "return view;" here until we get our rooting story sorted out > for DOM typed array structs. > Can you show me the bug discussing this? > >+++ b/dom/bindings/Bindings.conf > >+ 'headerFile': 'Crypto.h' > > The other option is to export Crypto.h to mozilla/dom.... This is OK for > now, but maybe a followup on exporting it and removing this annotation? > Filed Bug 897358 for this. > >+ if (!aJsCallback.Data()) { > > Similar issue here. > > if (aJsCallback.IsVoid()) > > That said, there was no test like this before (throwing on null) that I can > see... why are we adding one now? What was the old behavior when null was > passed in? > It was in the original file line 1906, checking null for argv[4]. > >+nsCrypto::PopChallengeResponse(const nsAString& aChallenge, > >+nsCrypto::Random(int32_t aNumBytes, nsAString& aReturn, ErrorResult& aRv) > >+nsCrypto::DisableRightClick(ErrorResult& aRv) > > Please file a followup bug to remove these methods that we don't actually > implement? > Filed Bug 897359 for this. Thanks for the great comments here, I'll address them in my next patch.
> That doesn't have the same behavior as before. The old behavior was that > aJSCallback being null would cause m_jsCallback to be null. You probably > want the following (after making aJSCallback nullable in the IDL): > > if (!aJSCallback.isVoid()) { > args->m_jsCallback = aJSCallback; > } > But from my previous comment, aJSCallback cannot be null. So I'll take your suggestion here, but 'without' adding aJSCallback nullable.
(In reply to Boris Zbarsky (:bz) from comment #61) > Comment on attachment 777614 [details] [diff] [review] > Patch v5 > > > >+ if (aEaCert.Data()) { > > if (!aEaCert.IsVoid()) { > > >+ SECStatus srv = ATOB_ConvertAsciiToItem(&certDer, aEaCert.Data()); > > .get(), please, so if the string ever becomes non-null-terminated this will > fail to compile instead of doing the wrong thing. > > > >+ const_cast<char*>(aReqDN.Data()), > >+ const_cast<char*>(aRegToken.Data()), > >+ const_cast<char*>(aAuthenticator.Data()), > > .get() instead of .Data(). > > >+ caNames[i] = const_cast<char*>(aArgs[i].Data()); > > .get() Hi, BZ One question for these s/Data()/get()/ Their type is nsACString, not nsCString, so they don't have the 'get()' method, so I use Data() to get the 'char*'. Do you mean that I need to convert the type from nsACString to nsCString? Thanks
(In reply to Boris Zbarsky (:bz) from comment #61) > >+++ b/security/manager/ssl/src/nsCrypto.h > >+using namespace mozilla; > >+using namespace mozilla::dom; > > No, please don't. That will force those using declarations on anyone > including the header. > > You can put those in the .cpp, but in the header you'll have to stick with > the fully qualified names. Incentives to put nsCrypto in the mozilla::dom > namespace. ;) > I choose to use fully qualfied names here, adding namespace mozilla::dom in nsCrypto seems wird to me because it's in security/ not in dom/(although I am not a DOM peer here and you definitely knows more than I do :D ), also I still have to update NSSModule.cpp. But if you insist on that I'd file a bug for it.
> Can you show me the bug discussing this? Bug 868799. > It was in the original file line 1906, checking null for argv[4]. Ah, indeed. Well, in that case this argument needs to be nullable, with throw-on-null. > So I'll take your suggestion here, but 'without' adding aJSCallback nullable. Sounds good. > Their type is nsACString, not nsCString Make it nsCString. The bindings are passing in an nsCString. > because it's in security/ not in dom/ Well, why is a DOM object in security/ ? ;) Anyway, followup is fine for that no matter what.
(In reply to Boris Zbarsky (:bz) from comment #61) > >@@ -68,16 +68,17 @@ > > nsCrypto::nsCrypto() : > >+ mWindow(nullptr) > > Why do we need this mWindow member? We have a perfectly reasonable mWindow > on the superclass... Please take it out. > > >+ nsWeakPtr mWindow; > > Like I said, I think having this is wrong. > Hi,BZ I now recall why I use nsWeakPtr in my patch, from Comment 39 ---- 4. use nsWeakPtr in nsCrypto to avoid memory leak (Perhaps I should update the cycle collection macros, but seems using nsWeakPtr just did the trick) ----
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #69) > (In reply to Boris Zbarsky (:bz) from comment #61) > > >@@ -68,16 +68,17 @@ > > > nsCrypto::nsCrypto() : > > >+ mWindow(nullptr) > > > > Why do we need this mWindow member? We have a perfectly reasonable mWindow > > on the superclass... Please take it out. > > > > >+ nsWeakPtr mWindow; > > > > Like I said, I think having this is wrong. > > > Hi,BZ > > I now recall why I use nsWeakPtr in my patch, from Comment 39 > > ---- > 4. use nsWeakPtr in nsCrypto to avoid memory leak > (Perhaps I should update the cycle collection macros, but seems using > nsWeakPtr just did the trick) > ---- When running mochitest, after the test is terminated, there's some log 0:24.58 TEST-UNEXPECTED-FAIL | leakcheck | 436739 bytes leaked (AtomImpl, Crypto, DOMStorageObserver, DR_State, DecodeRequest, ...) 0:24.58 INFO | runtests.py | Running tests: end. 0:24.58 TEST-UNEXPECTED-FAIL | leakcheck | 436739 bytes leaked (AtomImpl, Crypto, DOMStorageObserver, DR_State, DecodeRequest, ...) At that time I found out using nsWeakPtr could prevent this leak.
> 4. use nsWeakPtr in nsCrypto to avoid memory leak Why does nsCrypto have an mWindow member at all? It shouldn't!
(In reply to Boris Zbarsky (:bz) from comment #61) > You can put those in the .cpp, but in the header you'll have to stick with > the fully qualified names. Incentives to put nsCrypto in the mozilla::dom > namespace. ;) Filed Bug 898285 for this.
(In reply to Boris Zbarsky (:bz) from comment #71) > > 4. use nsWeakPtr in nsCrypto to avoid memory leak > > Why does nsCrypto have an mWindow member at all? It shouldn't! I'll check where's the leak from. Thanks.
If you have an mWindow member there that holds a strong ref and don't CC it, it will leak. But again, you shouldn't have an mWindow there at all.
Thanks for the tip, now I found in dom/base/nsGlobalWindow.cpp nsGlobalWindow::CleanUp, I added mCrypto = nullptr; Then the leak is gone now.
Again, you shouldn't need that, I would think...
Attached patch Patch v6 (obsolete) — — Splinter Review
Addressed bz's comments (Comment 61)
Attachment #777614 - Attachment is obsolete: true
Comment on attachment 781588 [details] [diff] [review] Patch v6 Hi, BZ Thanks for providing great comments before, this patch is base on your Comment 61, could you help to review this again for me ? Thanks
Attachment #781588 - Flags: review?(bzbarsky)
Would you mind posting an interdiff against v5 so its clearer which parts you changed?
Flags: needinfo?(allstars.chh)
Comment on attachment 781588 [details] [diff] [review] Patch v6 >+++ b/security/manager/ssl/src/nsCrypto.cpp >+ if (aReqDN.IsVoid()) { That will never test true given the way the WebIDL is written. The type in the IDL should presumably be "ByteString?", no? >+ if (aJsCallback.IsVoid()) { Likewise. r=me with those nits fixed.
Attachment #781588 - Flags: review?(bzbarsky) → review+
Oh, and it makes me sad that we don't have test coverage for the null handling here. :(
Hi, BZ Thanks for help of the interdiff. I was not able to connect to network these few days. Next time I'll keep this in mind. Also I'll address your comments and add some test case here. Thanks
Flags: needinfo?(allstars.chh)
now met error in try server, Bug 899946
Depends on: 899946
Attached patch Patch v7. — — Splinter Review
Addressed BZ's comments: Change 'ByteString' to 'ByteString?' for the parameters 'reqDN' and 'jsCallback' from generateCRMFRequest of Crypto.webidl Also add NS_ENSURE_SUCCESS in nsGlobalWindow::GetCrypto, (I found without this crashtest-ipc will fail due to do_CreateInstance(NS_CRYPTO_CONTRACTID) will fail in that test)
Attachment #781588 - Attachment is obsolete: true
Attachment #784250 - Flags: review+
Comment on attachment 784251 [details] [diff] [review] Part 2: Test for null. Hi, BZ This is the test script to test the null handling. Can you help to review this? Thanks
Attachment #784251 - Flags: review?(bzbarsky)
Comment on attachment 784251 [details] [diff] [review] Part 2: Test for null. Given the second test this is adding. how do we know the first test tests the ReqDN and not the jsCallback bits?
Attachment #784251 - Flags: review?(bzbarsky) → review-
Hi, BZ Thanks for pointing that out, so in this version I update the 1st test with supplying jsCallback. Thanks
Attachment #784251 - Attachment is obsolete: true
Attachment #784753 - Flags: review?(bzbarsky)
Comment on attachment 784753 [details] [diff] [review] Part 2: Test for null. v2 r=me
Attachment #784753 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/8aafb671f1e9 https://hg.mozilla.org/mozilla-central/rev/af5f5cf7bd3d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 927901
Depends on: 930827
Component: Security → DOM: Security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: