1116269 - Add WebCrypto to GlobalProperties for import

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Martin Thomson [:mt:]]

I'm looking to implement bug 975144, there is a good chance that the sandboxed code will need to run some crypto functions.  WebCrypto is nicely self-contained, so this should be straightforward to expose.

Comment 1

[Martin Thomson [:mt:]]

Attachment: MozReview Request: bz://1116269/mt

Comment 2

[Martin Thomson [:mt:]]

/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties

Pull down these commits:

hg pull review -r cde3bd84767b1c25c885fbc02ad88814325b2054

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8542372 [details]
MozReview Request: bz://1116269/mt

Gabor is probably the best alternative xpconnect reviewer these days.

This looks good at first glance, but I didn't have time to audit things very carefully (especially the crypto-side changes), so bouncing to gabor.

Comment 4

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/1789/#review1213

::: js/xpconnect/src/Sandbox.cpp
(Diff revision 1)
> +    return JS_DefineProperty(cx, obj, "crypto", wrapped, JSPROP_ENUMERATE);

Please be consistent with naming here. Either call it WebCrypto or crypto everywhere (not sure which is the name of the ctor on a regular DOM window but you should use the same here too)

Patches look great but I'm missing a simple test at least from it. Could you please add one? (I would do an xpcshell test under js/xpconnect)

Comment 5

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8542372 [details]
MozReview Request: bz://1116269/mt

I've did the review on review board, please flag me back once you added the test and fixed the naming issue I mentioned there.

Comment 6

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/1789/#review1215

> Please be consistent with naming here. Either call it WebCrypto or crypto everywhere (not sure which is the name of the ctor on a regular DOM window but you should use the same here too)

The name of the API is WebCrypto; the name of the property on window is "crypto".  I'm trying to ensure that the property is named consistently.  I can't call the flag "Crypto", since that conflicts with the name of the Crypto class that this instantiates.  Do you have any suggestions?

I don't know how to link to files in reviewboard (I'm only just learning still), but I guess that you missed [test_crypto.js](https://reviewboard.mozilla.org/r/1789/diff/7).

Comment 7

[Martin Thomson [:mt:]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> I've did the review on review board, please flag me back once you added the
> test and fixed the naming issue I mentioned there.

It looks like reviewboard is adding comments to bugzilla (I'm only just learning how this works myself).  It doesn't quote properly though.

Comment 8

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=96065fa5b34f - all green

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Martin Thomson [:mt] from comment #7)
> It looks like reviewboard is adding comments to bugzilla (I'm only just
> learning how this works myself).  It doesn't quote properly though.

Right, I'm totally new to this too...

(In reply to Martin Thomson [:mt] from comment #6)
> The name of the API is WebCrypto; the name of the property on window is
> "crypto".  I'm trying to ensure that the property is named consistently.  I
> can't call the flag "Crypto", since that conflicts with the name of the
> Crypto class that this instantiates.  Do you have any suggestions?

The flag should not have capital C, just simply "crypto" as the property on the global. And then it shouldn't conflict with the class name, or am I missing something? The idea is that those bool flags (atob/File/Blob/...) named exactly as the properties will appear on the global (all lower case in this case). Not a regular coding style, but quite useful there, so I wanted to preserve it... And sorry about the test, probably I just looked it over as I'm not too familiar with this review-board yet. It looks great!

Comment 10

[Martin Thomson [:mt:]]

/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject, r=rbarnes
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties, r=gabor

Pull down these commits:

hg pull review -r 6c2ea4e0ab880c38c90828ce42eca784b2791a77

Comment 11

[Martin Thomson [:mt:]]

/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject, r=rbarnes
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties, r=gabor

Pull down these commits:

hg pull review -r 6c2ea4e0ab880c38c90828ce42eca784b2791a77

Comment 12

[Martin Thomson [:mt:]]

/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject, r=rbarnes
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties, r=gabor
/r/1865 - Bug 975144 - WebIDL changes for RTC identity sandbox
/r/1867 - Bug 975144 - Implementation of RTC identity DOM component
/r/1869 - Bug 975144 - Rework RTC identity to use JS sandbox
/r/1871 - Bug 975144 - Updating test IdP for new API
/r/1873 - Bug 975144 - Updating RTC identity tests
/r/1875 - Bug 975144 - Adding rtcIdentityProvider to sandbox global scope
/r/1877 - Bug 975144 - Tests for rtcIdentityProvider property

Pull down these commits:

hg pull review -r 8c84952758cc934e83e42129b5e57938f3642e5d

Comment 13

[Martin Thomson [:mt:]]

OK, Richard, please disregard reviewboard being stupid.  Can you look at https://reviewboard.mozilla.org/r/1791/ please?

Comment 14

[Martin Thomson [:mt:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b206ea15cb59
https://hg.mozilla.org/integration/mozilla-inbound/rev/da03d6aff37d

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b206ea15cb59
https://hg.mozilla.org/mozilla-central/rev/da03d6aff37d

Comment 16

[Gabor Krizsanits (INACTIVE)]

One new Sandbox global property: crypto

Comment 17

[Jean-Yves Perrier [:teoli]]

I've updated https://developer.mozilla.org/en-US/docs/Components.utils.importGlobalProperties

Comment 19

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1116269 - Add 'crypto' to sandbox global properties, r=gabor

Comment 20

[Martin Thomson [:mt:]]

Attachment: MozReview Request:

Comment 21

[Martin Thomson [:mt:]]

Attachment: MozReview Request:

Comment 22

[Martin Thomson [:mt:]]

Attachment: MozReview Request:

Comment 23

[Martin Thomson [:mt:]]

Attachment: MozReview Request:

Comment 24

[Martin Thomson [:mt:]]

Attachment: MozReview Request:

Comment 25

[Martin Thomson [:mt:]]

Attachment: MozReview Request:

Comment 26

[Martin Thomson [:mt:]]

Attachment: MozReview Request:

Comment 27

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject, r=rbarnes