Closed Bug 892214 Opened 6 years ago Closed 6 years ago

Expose `atob` and `btoa` functions in SDK

Categories

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

x86
macOS
defect

Tracking

(firefox26 fixed, firefox27 fixed)

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: irakli, Assigned: gkrizsanits)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [only the renaming patch landed on Fx26])

Attachments

(3 files, 1 obsolete file)

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`
Gabor, Eddy is this something one of you could look into ?
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.
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.
(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...
Priority: -- → P2
Assignee: nobody → gkrizsanits
Status: NEW → ASSIGNED
Attachment #807138 - Flags: review?(bobbyholley+bmo)
Attachment #807138 - Attachment description: Rename DOMConstructors to GlobalProperties in SandboxOptions → Part1: Rename DOMConstructors to GlobalProperties in SandboxOptions. v1
Attachment #807139 - Flags: review?(bobbyholley+bmo)
Attachment #807138 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #807139 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #807140 - Flags: review+
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.
Attachment #807138 - Attachment is obsolete: true
Attachment #807774 - Flags: review?(dtownsend+bugmail)
Attachment #807774 - Flags: review?(dtownsend+bugmail) → review+
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
Attachment #807774 - Flags: approval-mozilla-aurora?
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
Attachment #807774 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Whiteboard: [only the renaming patch landed on Fx26]
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
Flags: needinfo?(gkrizsanits)
Keywords: dev-doc-needed
(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.
Flags: needinfo?(gkrizsanits)
Forgot to flag Will for moreInfo on the previous comment.
Flags: needinfo?(wbamberg)
And just as importantly, the documented wantXHRConstructor option is gone.
(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.
Flags: needinfo?(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.
Flags: needinfo?(gkrizsanits)
(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...
Flags: needinfo?(gkrizsanits)
(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).
(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?
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.