892214 - Expose `atob` and `btoa` functions in SDK

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

atob and btoa function are exposed to as globals in web workers and JSMs and dom windows but they're not available for scripts in Cu.Sandbox'es. They should be exposed to avoid ugly hacks like this:

https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/base64.js#L13-L14


I think it would make most sense to expose them as properties of `Components.utils`

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Gabor, Eddy is this something one of you could look into ?

Comment 2

[Gabor Krizsanits (INACTIVE)]

Why would you want it on Components.utils if both JSM and workers (as well as regular DOM windows) have them injected in their global scope? Shouldn't we do the same? Like adding some flags to sandbox options, or simply by default adding all these functions to sandboxes the same way? I think that's the right thing to do here.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Yeah I think exposing as globals sounds fine too, just not big fun of constantly increasing number of globals, also node does not has these as globals but rather as module so I thought of doing same.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> constantly increasing number of globals
By globals, you mean global variables? But you would do that anyway, no? Like:
var atob = loadAToB();

> rather as module
So your point is that you want more control over this, and want to add them optionally? See my proposal in the other bug, and let me know if that would suit you...

Comment 5

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=c0b5f8de52d5

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: Part1: Rename DOMConstructors to GlobalProperties in SandboxOptions. v1

Comment 7

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: Exporting atob to xpcprivate. v1

Comment 8

[Gabor Krizsanits (INACTIVE)]

Attachment: part3: Atob for Sandbox. v1

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 807139 [details] [diff] [review]
part2: Exporting atob to xpcprivate. v1

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

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +145,5 @@
>  #endif
>  }
>  
> +bool
> +xpc::Atob(JSContext *cx, unsigned argc, jsval *vp)

Can you move these  to nsXPConnect.cpp while you're at it?

::: js/xpconnect/src/xpcprivate.h
@@ +3566,5 @@
>  }
>  
>  namespace xpc {
>  
> +// Atob and Btoa helpers for xpc globals.

I'd say:

// JSNatives to expose atob and btoa in various non-DOM XPConnect scopes.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 807140 [details] [diff] [review]
part3: Atob for Sandbox. v1

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

::: js/xpconnect/tests/unit/test_sandbox_atob.js
@@ +4,5 @@
> +                      { wantGlobalProperties: ["atob", "btoa"] });
> +  sb.do_check_eq = do_check_eq;
> +  Cu.evalInSandbox('var dummy = "Dummy test.";' +
> +                   'do_check_eq(dummy, atob(btoa(dummy)));',
> +                   sb);

I think we should also test a known value of btoa conversion to make sure that the transformations actually do something (in addition to verifying the composition results in an identity transformation).

How about:

Cu.evalInSandbox('do_check_eq(btoa("budapest", "YnVkYXBlc3Q=");', sb)

?

@@ +5,5 @@
> +  sb.do_check_eq = do_check_eq;
> +  Cu.evalInSandbox('var dummy = "Dummy test.";' +
> +                   'do_check_eq(dummy, atob(btoa(dummy)));',
> +                   sb);
> +} 

Whitespace error.

Comment 11

[Gabor Krizsanits (INACTIVE)]

Attachment: Rename DOMConstructors to GlobalProperties in SandboxOptions

Just to be on the safe side, I will land this patch to fx-team with the SDK changes and uplift to aurora, in the meanwhile add the SDK related changes to the SDK github repo too...

Dave, could you please review the SDK part of this patch? Bobby already r+-ed the rest of it.

Comment 12

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/044237be11e2
remote:   https://hg.mozilla.org/integration/fx-team/rev/2d42170cc6cc
remote:   https://hg.mozilla.org/integration/fx-team/rev/d5e7dc7d33c3

Comment 13

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 807774 [details] [diff] [review]
Rename DOMConstructors to GlobalProperties in SandboxOptions

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 892203
User impact if declined: There was a small API change that mainly affects the add-on SDK. As it turns out a rename is needed in that new API, but that patch is already on aurora. If I don't uplift this patch that will cause a mess around this API.
Testing completed (on m-c, etc.): I've just pushed it to the fx-team branch (because there are changes in the Addon SDK...) 
Risk to taking this patch (and alternatives if risky): none 
String or IDL/UUID changes made by this patch: none

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/3af7cccb2f5b8282d8c3bd2eea23e721488436d5
Bug 892214 - Rename DOMConstructors to GlobalProperties in SandboxOptions. r=bholley, r=mossop

https://github.com/mozilla/addon-sdk/commit/7b19502d4d9546ac2e194f78658f8e6d1221e484
Merge pull request #1242 from krizsa/master

Bug 892214 - Rename DOMConstructors to GlobalProperties in SandboxOptions. r=@mossop

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/044237be11e2
https://hg.mozilla.org/mozilla-central/rev/2d42170cc6cc
https://hg.mozilla.org/mozilla-central/rev/d5e7dc7d33c3

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17069a490319

Setting status-firefox26 to fixed while noting that only the renaming patch was uplifted, not the other 2 patches.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Is there any documentation for the wantGlobalProperties thing on sandbox?  I don't see any at https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox

Comment 18

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #17)
> Is there any documentation for the wantGlobalProperties thing on sandbox?  I
> don't see any at
> https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox

I think we don't have any docs yet. Will, do you think you could have some time for this?

In a nutshell it is sandbox option, that takes an array of string. The strings are names of constructors that we install on the sandbox global. Currently supported ones can be found here:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#732

I have no idea about the "-Promise" magic there, but it seems like by default Promise are installed and with this "-Promise" you can turn it off.

Comment 19

[Gabor Krizsanits (INACTIVE)]

Forgot to flag Will for moreInfo on the previous comment.

Comment 20

[Boris Zbarsky [:bzbarsky]]

And just as importantly, the documented wantXHRConstructor option is gone.

Comment 21

[Will Bamberg [:wbamberg]]

(In reply to Gabor Krizsanits [:krizsa :gabor](PTO until 27th) from comment #18)
> (In reply to Boris Zbarsky [:bz] from comment #17)
> > Is there any documentation for the wantGlobalProperties thing on sandbox?  I
> > don't see any at
> > https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox
> 
> I think we don't have any docs yet. Will, do you think you could have some
> time for this?

Yes! Just looking at it now.

> 
> In a nutshell it is sandbox option, that takes an array of string. The
> strings are names of constructors that we install on the sandbox global.
> Currently supported ones can be found here:
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.
> cpp#732
> 
> I have no idea about the "-Promise" magic there, but it seems like by
> default Promise are installed and with this "-Promise" you can turn it off.

Comment 22

[Will Bamberg [:wbamberg]]

I've added some docs for wantGlobalProperties: https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox#Sandbox_options. Let me know if it's all right.

Comment 23

[Gabor Krizsanits (INACTIVE)]

(In reply to Will Bamberg [:wbamberg] from comment #22)
> I've added some docs for wantGlobalProperties:
> https://developer.mozilla.org/en-US/docs/Components.utils.
> Sandbox#Sandbox_options. Let me know if it's all right.

Looks all good to me. One small thing, could you try it if the -Promise thing really does what I think? I've never actually tried out my educated guess about it...

Comment 24

[Will Bamberg [:wbamberg]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> (In reply to Will Bamberg [:wbamberg] from comment #22)
> > I've added some docs for wantGlobalProperties:
> > https://developer.mozilla.org/en-US/docs/Components.utils.
> > Sandbox#Sandbox_options. Let me know if it's all right.
> 
> Looks all good to me. One small thing, could you try it if the -Promise
> thing really does what I think? I've never actually tried out my educated
> guess about it...

Yes, as long as the following chrome code does what I think it does, then -Promise does what you think it does :)

***
var sandbox1 = Components.utils.Sandbox("https://example.org/");
console.log(sandbox1.Promise);
// function()

var options = {
  wantGlobalProperties: ["-Promise"]
}

var sandbox2 = Components.utils.Sandbox("https://example.org/", options);
console.log(sandbox2.Promise);
// undefined
***

I found that similar code using importGlobalProperties did not do what I thought it would, but unfortunately I didn't understand the explanation for why (bug 920553, comment 24).

Comment 25

[Gabor Krizsanits (INACTIVE)]

(In reply to Will Bamberg [:wbamberg] from comment #24)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> Yes, as long as the following chrome code does what I think it does, then
> -Promise does what you think it does :)

lgtm

> I found that similar code using importGlobalProperties did not do what I
> thought it would, but unfortunately I didn't understand the explanation for
> why (bug 920553, comment 24).

I don't think that -Promise flag is something that makes a lot of sense for
importGlobalProperties. It does not "uninstal" Promises, it's only used for
the sandbox case to prevent it being installed on the first place when the
sandbox is created. So your example is not working because once it is defined
for sandbox importGlobalProperties will not even try to undefine it, it's basically
just a noop. Actually it's even worse than that because it adds the side effect that
whenever someone is trying to call importGlobalProperties without passing in an explicit
-Promise we are trying to install Promise on the global by default, which seems like an
unwanted side effect. Bobby, can you confirm that this is a bug?

Comment 26

[Bobby Holley (:bholley)]

I think we should get rid of -Promise. I don't think it buys us very much - my original thinking was that we might have concerns about giving untrusted sandboxed code access to the event loop, but we effectively already have that on the web with <iframe sandbox>. Moreover, this feature will be a pain to keep working when we move Promises into the JS engine.

Gabor, would you mind writing up a quick patch to remove it?