Expose TextEncoder TextDecoder in SDK

RESOLVED FIXED in mozilla26

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: irakli, Assigned: gkrizsanits)

Tracking

({dev-doc-needed})

unspecified
mozilla26
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

TextDecoder and TextEncoder are exposed to as globals in web workers and JSMs but they're not available in Cu.Sandbox'es or `bootstrap.js`. They should be exposed to avoid ugly hacks like this:

https://github.com/Gozala/addon-sdk/blob/hotfix/io/lib/sdk/io/buffer.js#L13
Gabor is this something that you can take a look at ?

I think it would make most sense to expose them as properties of `Components.utils`.
Can we just expose the same stuff in sandboxes as elsewhere? I think we had a bug on that somewhere.
Assignee

Comment 3

6 years ago
(In reply to Dave Townsend (:Mossop) from comment #2)
> Can we just expose the same stuff in sandboxes as elsewhere? I think we had
> a bug on that somewhere.

Yes we can. I can do it optionally (adding another option flag for sandbox creation) or add it to all sandbox without any conditions. I think it should be default for all sandbox, I don't think it could hurt anyone to have it.
I think exposing same stuff as elsewhere sounds good as long as it's not implying chrome capabilities.
Assignee

Comment 5

6 years ago
A few questions: is it OK to default this, or should this be optional?

Is it OK, to ignore it if there is a sandbox proto? (see comment)
Attachment #776436 - Flags: feedback?(bobbyholley+bmo)
I think these should probably be behind a default-off options in sandboxOptions, to avoid any compat issues. Probably some sort of 'wantDOMUtils' or something?

More generally, bz and blake - what do you guys think about exposing more and more random WebIDL stuff to sandboxes? And if so, should it be done piecemeal (or a la carte somehow), or should we just have one generic switch to turn on whatever we throw in there? Maybe people could pass a list of WebIDL constructors they want?

How likely are the bindings to break when the global isn't a window?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(mrbkap)
> More generally, bz and blake - what do you guys think about exposing more and more random
> WebIDL stuff to sandboxes?

Generally speaking, Peter and I have been talking about exposing all WebIDL things on all globals anyone interacts with...

> And if so, should it be done piecemeal (or a la carte somehow), or should we just have
> one generic switch to turn on whatever we throw in there?

What are the reasons to _not_ expose WebIDL stuff on sandboxes?

> How likely are the bindings to break when the global isn't a window?

The _bindings_ don't care, as long as the global has an nsISupports hanging off it.  Except for JS-implemented bindings, of course.

The _implementations_ can care, obviously; they have to be written to deal with non-Window globals as needed...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #7)
> Generally speaking, Peter and I have been talking about exposing all WebIDL
> things on all globals anyone interacts with...

Good to know.

> > And if so, should it be done piecemeal (or a la carte somehow), or should we just have
> > one generic switch to turn on whatever we throw in there?
> 
> What are the reasons to _not_ expose WebIDL stuff on sandboxes?

1 - General overhead. Sandboxes are supposed to be lightweight globals without all the DOM junk. I'm a bit concerned about throwing more and more stuff in there by default.

2 - Security. Sandboxes are supposed to be sandboxed. I think it would be very surprising behavior for sandboxes to suddenly start providing access to WebSockets and and Workers and whatnot to untrusted code.

3 - Embedding compat risk via name collisions. We probably don't care all that much in general, but probably want to be a bit more deliberate about introducing things into the global sandbox namespace than simply haphazardly adding whatever constructors the jetpack team wants this month.

4 - sandboxPrototype. Introducing Foopy on the sandbox global means that |new Foopy()| will now create a Foopy associated with the sandbox in situations where it previously would have created one associated with the sandboxPrototype. We can probably handle this by just disabling the constructors in situations where we have a window for a sandboxPrototype.

5 - Random breakage in the implementations. IIUC there's a pretty big surface of stuff that isn't going to work right with a non-Window global (we had to hack a bunch of stuff in to make XHR work in sandboxes, IIRC). 

> The _implementations_ can care, obviously; they have to be written to deal
> with non-Window globals as needed...

Right. So how much of this is there?
(In reply to Bobby Holley (:bholley) from comment #8)
> 2 - Security. Sandboxes are supposed to be sandboxed. I think it would be
> very surprising behavior for sandboxes to suddenly start providing access to
> WebSockets and and Workers and whatnot to untrusted code.

FTR in jetpack we're only really concerned with getting a new global for each module, one where messing with object prototypes can't impact other globals. The code we run in them is generally trusted. As far as we know the only thing we have to do that right now is to create a new sandbox per module.
(In reply to Dave Townsend (:Mossop) from comment #9)
> FTR in jetpack we're only really concerned with getting a new global for
> each module, one where messing with object prototypes can't impact other
> globals. The code we run in them is generally trusted. As far as we know the
> only thing we have to do that right now is to create a new sandbox per
> module.

Right, I'm just talking about why I don't want to add random constructors by default, especially to sandboxes with non-System principal. Jetpack can always just pass the flags it wants to the Sandbox() constructor.
> 1 - General overhead.

I think we'd want resolve hooks on the global or something, not predefining all the things... does that help?

> 2 - Security. Sandboxes are supposed to be sandboxed.

OK, fair.

> 3 - Embedding compat risk via name collisions. 

Given that assignment would override, is this really a problem?

> 4 - sandboxPrototype.

I think if sandboxPrototype is a DOM global we should avoid defining DOM stuff on the sandbox as well, yes.

> Right. So how much of this is there?

Presumably pretty much any constructor or static method that is not very carefully written.
(In reply to Boris Zbarsky (:bz) from comment #11)
> > 1 - General overhead.
> 
> I think we'd want resolve hooks on the global or something, not predefining
> all the things... does that help?

Somewhat, but I was also referring to complexity overhead. Especially when debugging random issues that addon developers file, I'd like to minimize the number of factors that might be involved when someone who doesn't know what they're doing does |new Cu.Sandbox(this)|.

This, along with (2), make me pretty sure that we want this to be default-off.
 
> > 3 - Embedding compat risk via name collisions. 
> 
> Given that assignment would override, is this really a problem?

It still could be, during detection (supposing an addon defined a TextEncoder polyfill on the sandbox global, and did |if (TextEncoder)| to determine if the init routine had been run). But it's probably not worth worrying about.

> > 4 - sandboxPrototype.
> 
> I think if sandboxPrototype is a DOM global we should avoid defining DOM
> stuff on the sandbox as well, yes.

Agreed.

> > Right. So how much of this is there?
> 
> Presumably pretty much any constructor or static method that is not very
> carefully written.

So then we're left with the question of how to move forward. I think we almost certainly want to audit and test each constructor that we add. Here's a strawman proposal:

We add a new sandboxOption, 'wantDOMConstructors', which defaults to false. When true, you get whatever set of whitelisted DOM constructors gabor has hooked up and tested (which will grow over time). Whenever we want a constructor, somebody goes in and:
(A) Audits the implementation for window-dependence, and fixes as necessary.
(B) Adds tests.
(C) Adds the constructor to the whitelist.

Thoughts?
Assignee

Comment 13

6 years ago
First I'd just leave this here: bug 892214

In general I already feel bad about the too many options on sandbox. Only reason I wanted to default TextEncoder is that in jsm modules they are default and even for workers they are default, so it seemed like the most consistent thing to do, to make it default on all global (as the comment says in nsXPConnect). But I don't have any strong opinion about this one, I'm fine with defaulting it off.

General wantDOMConstructors flag: sounds good to me, I already felt bad about the too many sandbox flags.

sandboxPrototype: I have to disagree here. Content-script sandbox has a content window as its proto, but content-script sandbox itself might have higher prvilege (nsEP). For XHR ctor for example shadowing is the right thing to do I'm afraid (so addons can do cross origin xhr). We could of course cherry pick when we need add an extra ctor and when is it enough to have one on the proto, but I'm not sure it worth it.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> sandboxPrototype: I have to disagree here. Content-script sandbox has a
> content window as its proto, but content-script sandbox itself might have
> higher prvilege (nsEP). For XHR ctor for example shadowing is the right
> thing to do I'm afraid (so addons can do cross origin xhr). We could of
> course cherry pick when we need add an extra ctor and when is it enough to
> have one on the proto, but I'm not sure it worth it.

That's a very good point. I guess as long as we're putting this behind wantDOMConstructors, it's opt-in, and it won't break any existing users of sandboxPrototype. So yeah, let's shadow.
To be honest I'd rather prefer to just have access to all the DOMConstructors in some way (not that TextEncoder / TextDecoder are not exposed in any ways other then on JS global) instead of polluting sandbox global, since that way have more control and can expose only constructors we need / make sense in specific cases. While `wantDOMConstructors` seems a lot less configurable. Preference also based of previous experience where we didn't wanted `Components` global in sandboxes but it was present never the less and was impossible to get rid off without platform changes.

That's also why original request suggested exposure of TextEncoder / TextDecoder constructors on Components.utils instead.

That being said `TextEncoder` / `TextDecoder` as sandbox globals would probably work just fine in practice, so I don't mind this option either.
Flags: needinfo?(gkrizsanits)
Flags: needinfo?(bobbyholley+bmo)
I have a spelling error I meant to say: "note that TextEncoder / TextDecoder are not exposed ..." instead of "not that TextEncoder / TextDecoder are..."
Assignee

Comment 17

6 years ago
Components.utils needs chrome privilege. Components are optional for sandboxes. So in the end I think polluting the sandbox global is the cleanest approach here (it's also consistent with the other globals). I also shared the same concern with you about the one flag for all the DOM constructor approach, but for now it's not really an issue. And I cannot really come up with a valid case where it can be a real issue.

For system modules in jetpack you have chrome privilege, so there this is kind of least of my worries.

For content-script, where you have expanded principal, I think it's fair to say that you either want DOM ctors with expanded principal or not. This is the part I guess where you might have a different opinion...

For (if we will have it) hidden frames with expanded principal, you already have all the DOM ctors.

My point is, if there is a use case where it makes sense to add more config options here, sure, let's do that. But just because at some point in the future we might needs something, I would not complicate an already far too complicated API further (sandbox ctor).

That being said. If there is a disagreement here, instead of adding more flags here is a proposal that gives you the freedom of choice without making the sandbox ctor even more uglier:

Cu.addDOMConstructors(sb, [])
where the second argument can be a name of a DOM ctor, an array of names, or if we leave it away all the DOM ctors are added.
Flags: needinfo?(gkrizsanits)
Irakli, can you describe you requirements a bit more? I'm a bit unclear on the following:

1 - Do you want to provide these to content scripts, or only chrome scripts?

2 - Do you want to be able to instantiate DOM objects and have them belong to the sandbox scope? This will require each constructor to be audited and tested per comment 12. A simpler approach would be to just sandboxPrototype your sandbox to a hidden iframe (though that has the disadvantages of memory overhead and the objects belonging to the wrong global).

3 - Do you want to be able to add them dynamically, or can you do it at Sandbox creation time? If the latter, we can just have wantDOMConstructors take an array of the constructors you want.
Flags: needinfo?(bobbyholley+bmo)
Flags: needinfo?(mrbkap)
Assignee

Updated

6 years ago
Flags: needinfo?(rFobic)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> Components.utils needs chrome privilege. 

But so does a Cu.Sandbox, I expect that those globals will be exposed by the code that set's up the sandbox. I kind of also expected that TextEncoder / Decoder won't leak capabilites in non privileged sandboxes but maybe I'm too optimistic.
Flags: needinfo?(rFobic)
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #19)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> > Components.utils needs chrome privilege. 
> 
> But so does a Cu.Sandbox, I expect that those globals will be exposed by the
> code that set's up the sandbox. I kind of also expected that TextEncoder /
> Decoder won't leak capabilites in non privileged sandboxes but maybe I'm too
> optimistic.

It seems kinda backwards to expose a DOM constructor on a chrome-only XPCOM object, and then manually bind that into a non-chrome scope, trying not to leak privileges. I think these constructors should be native to the sandbox they're operating in.
Assignee

Comment 21

6 years ago
(In reply to Bobby Holley (:bholley) from comment #20)
> It seems kinda backwards to expose a DOM constructor on a chrome-only XPCOM
> object, and then manually bind that into a non-chrome scope, trying not to
> leak privileges. I think these constructors should be native to the sandbox
> they're operating in.

Yeah, that's why I suggested a Cu.addDOMConstructors API if we want to add them dynamically. But frankly, I'm still missing the point of being able to do that, a wantDOMConstructors flag should be enough imo... And if it turns out not to be enough, we can let the wantDOMConstructors hold an array optionally.
(In reply to Bobby Holley (:bholley) from comment #18)
> Irakli, can you describe you requirements a bit more? I'm a bit unclear on
> the following:
> 
> 1 - Do you want to provide these to content scripts, or only chrome scripts?

At the moment we only need them in a chrome principled scripts, but won't be surprised if we'll end up needing them in content scripts in a future.

> 
> 2 - Do you want to be able to instantiate DOM objects and have them belong
> to the sandbox scope? 

I'm little confused by a terminology here. Are TextEncoder, TextDecoder DOM objects ? If so, yes I need to instantiate them and have them belong to sandbox scope. Also not quite sure what do you mean by belonging to a sandbox scope, but I'd like them to behave same as they do in JSMs.


> This will require each constructor to be audited and
> tested per comment 12. A simpler approach would be to just sandboxPrototype
> your sandbox to a hidden iframe (though that has the disadvantages of memory
> overhead and the objects belonging to the wrong global).
>

Hidden iframes are kind of pain & we constantly run into different bugs when doing this. I'd rather avoid them, if it goes to that I'd rather even stick to our current
approach of obtain them from a JSM:
https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/io/buffer.js#L13

> 
> 3 - Do you want to be able to add them dynamically, or can you do it at
> Sandbox creation time? If the latter, we can just have wantDOMConstructors
> take an array of the constructors you want.

Later will cover our use case just fine and I like idea of array with constructors to be exposed as long as those constructors don't have to be obtained from iframes or some other ugly hacks.
(In reply to Bobby Holley (:bholley) from comment #18)
> Irakli, can you describe you requirements a bit more? I'm a bit unclear on
> the following:
> 
> 1 - Do you want to provide these to content scripts, or only chrome scripts?

At the moment we only need them in a chrome principled scripts, but won't be surprised if we'll end up needing them in content scripts in a future.

> 
> 2 - Do you want to be able to instantiate DOM objects and have them belong
> to the sandbox scope? 

I'm little confused by a terminology here. Are TextEncoder, TextDecoder DOM objects ? If so, yes I need to instantiate them and have them belong to sandbox scope. Also not quite sure what do you mean by belonging to a sandbox scope, but I'd like them to behave same as they do in JSMs.


> This will require each constructor to be audited and
> tested per comment 12. A simpler approach would be to just sandboxPrototype
> your sandbox to a hidden iframe (though that has the disadvantages of memory
> overhead and the objects belonging to the wrong global).
>

Hidden iframes are kind of pain & we constantly run into different bugs when doing this. I'd rather avoid them, if it goes to that I'd rather even stick to our current
approach of obtain them from a JSM:
https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/io/buffer.js#L13

> 
> 3 - Do you want to be able to add them dynamically, or can you do it at
> Sandbox creation time? If the latter, we can just have wantDOMConstructors
> take an array of the constructors you want.

Later will cover our use case just fine and I like idea of array with constructors to be exposed as long as those constructors don't have to be obtained from iframes or some other ugly hacks.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #23)
> > 2 - Do you want to be able to instantiate DOM objects and have them belong
> > to the sandbox scope? 
> 
> I'm little confused by a terminology here. Are TextEncoder, TextDecoder DOM
> objects ?

Well, they're constructors, aka "Interface Objects". Usually you can |new| them, sometimes there are utility functions hanging off of them, and often both.

> If so, yes I need to instantiate them and have them belong to
> sandbox scope. Also not quite sure what do you mean by belonging to a
> sandbox scope, but I'd like them to behave same as they do in JSMs.

DOM objects have an observable 'home' global, which is most easily observable based on which Object.prototype the proto chain eventually leads to.

> Hidden iframes are kind of pain & we constantly run into different bugs when
> doing this. I'd rather avoid them

Yeah, me too.

> > 3 - Do you want to be able to add them dynamically, or can you do it at
> > Sandbox creation time? If the latter, we can just have wantDOMConstructors
> > take an array of the constructors you want.
> 
> Later will cover our use case just fine and I like idea of array with
> constructors to be exposed as long as those constructors don't have to be
> obtained from iframes or some other ugly hacks.

Ok, sounds like we should do wantDOMConstructors: [...], where the array takes a list of whitelisted DOM constructors that we've verified/tested as working properly. The set of whitelisted constructors will presumably grow over time.
Assignee

Comment 25

6 years ago
(In reply to Bobby Holley (:bholley) from comment #24)
> Ok, sounds like we should do wantDOMConstructors: [...], where the array
> takes a list of whitelisted DOM constructors that we've verified/tested as
> working properly. The set of whitelisted constructors will presumably grow
> over time.

Let's stick to this then.
Assignee: nobody → gkrizsanits
Assignee

Comment 26

6 years ago
Comment on attachment 776436 [details] [diff] [review]
TextEncoder / TextDecoder for sandboxes

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

New patch coming soon.
Attachment #776436 - Flags: feedback?(bobbyholley+bmo)
Priority: -- → P2
Assignee

Comment 27

6 years ago
Posted patch Sandbox option flags (obsolete) — Splinter Review
So I think we are going to add too many bool members with all these DOM ctors and other sandbox options to the SandboxOptions struct so I decided to switch to bit flags instead. What do you think? Do you mind it? It's the bits in the xpcprivate.h I'm talking about, you can ignore the rest of the patch for now (not really polished yet...), just wanted to get a quick feedback about this before I continue.
Attachment #794739 - Flags: feedback?(bobbyholley+bmo)
Comment on attachment 794739 [details] [diff] [review]
Sandbox option flags

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

Hm, I don't really see the value in this. Flags are a pain to use. Why do we care about saving a few bytes on a structure that only lives on the stack?
Attachment #794739 - Flags: feedback?(bobbyholley+bmo) → feedback-
Assignee

Comment 29

6 years ago
(In reply to Bobby Holley (:bholley) from comment #28)
> Flags are a pain to use.

Yes.

> care about saving a few bytes on a structure that only lives on the stack?

If it stays on the stack it really doesn't make sense. But I find it silly to gather all the sandbox options/info in one struct and then store at various places instead (like compartment private) where they really do not belong to. While we have already a native object glued to the sandbox (sandbox private) that could just store this thing, and any other info that belongs to the sandbox.

Anyway, even then it might not matter at all to save a few bytes so I think I'll just drop the idea.
Assignee

Comment 30

6 years ago
Attachment #776436 - Attachment is obsolete: true
Attachment #794739 - Attachment is obsolete: true
Attachment #797883 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 31

6 years ago
Maybe if we have a lot more DOM ctors we should do something smarter than this primitive if-else-strcmp tree... but probably does not matter a lot.
Attachment #797885 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 32

6 years ago
I don't think I should add too much testing for these two, workers do only primitive testing like I did in this patch.
Attachment #797888 - Flags: review?(bobbyholley+bmo)
Comment on attachment 797883 [details] [diff] [review]
part0: minor cleanups in sandbox

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

::: js/xpconnect/src/Sandbox.cpp
@@ +1167,2 @@
>  
> +    if (!(*found))

Why do we need the extra parens here?

@@ +1170,2 @@
>  
> +    NS_ENSURE_TRUE(JS_GetProperty(cx, from, name, prop), NS_ERROR_INVALID_ARG);

We should avoid putting functional code in NS_ENSURE_* macros. There are two options:

bool ok = JS_GetProperty(cx, from, name, prop);
NS_ENSURE_TRUE(ok, NS_ERROR_INVALID_ARG);
return NS_OK;

or

if (!JS_GetProperty(cx, from, name, prop))
  return NS_ERROR_INVALID_ARG;
return NS_OK;

The second is more JS idiomatic, but XPC is always a funny hybrid, especially with different return codes. The NS_ENSURE macros are helpful in giving us logging. Your call.

@@ +1183,5 @@
>  
>      RootedValue value(cx);
>      bool found;
> +    NS_ENSURE_SUCCESS(GetPropFromOptions(cx, from, name, &value, &found),
> +                      NS_ERROR_INVALID_ARG);

Same comment as above. At the very least, GetPropFromOptions shouldn't be in the macro.
Attachment #797883 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 797885 [details] [diff] [review]
part1: parsing DOMConstructors option

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

::: js/xpconnect/src/Sandbox.cpp
@@ +1242,5 @@
>  
>  /*
> + * Helper that tries to get a list of DOM constructors from the options object.
> + */
> +nsresult

static?

@@ +1243,5 @@
>  /*
> + * Helper that tries to get a list of DOM constructors from the options object.
> + */
> +nsresult
> +GetDOMConstructorsPropFromOptions(JSContext *cx, HandleObject from, SandboxOptions& options)

I'd just call this GetDOMConstructorsFromOptions

@@ +1258,5 @@
> +    RootedObject ctors(cx, &value.toObject());
> +    NS_ENSURE_TRUE(JS_IsArrayObject(cx, ctors), NS_ERROR_INVALID_ARG);
> +
> +    uint32_t length;
> +    JS_GetArrayLength(cx, ctors, &length);

Need to check for failure here.

@@ +1262,5 @@
> +    JS_GetArrayLength(cx, ctors, &length);
> +    for (uint32_t i = 0; i < length; i++) {
> +        RootedValue nameValue(cx);
> +        NS_ENSURE_TRUE(JS_GetElement(cx, ctors, i, &nameValue),
> +                       NS_ERROR_INVALID_ARG);

As noted earlier, the calls themselves shouldn't be in NS_ENSURE. Use |bool ok|

@@ +1265,5 @@
> +        NS_ENSURE_TRUE(JS_GetElement(cx, ctors, i, &nameValue),
> +                       NS_ERROR_INVALID_ARG);
> +        NS_ENSURE_TRUE(nameValue.isString(), NS_ERROR_INVALID_ARG);
> +        char *name = JS_EncodeString(cx, nameValue.toString());
> +        NS_ENSURE_TRUE(name, NS_ERROR_INVALID_ARG);

I don't think this is the appropriate return code for this. Probably NS_ERROR_OUT_OF_MEMORY.

@@ +1274,5 @@
> +            return NS_ERROR_INVALID_ARG;
> +        }
> +    }
> +    return NS_OK;
> +} 

Whitespace error.
Attachment #797885 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 797888 [details] [diff] [review]
part2: TextEncoder / TextDecoder

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

::: js/xpconnect/src/xpcprivate.h
@@ +3705,3 @@
>  IsSandbox(JSObject *obj);
>  
>  struct SandboxOptions {

I think we should make a new helper struct called SandboxDOMConstructors, with a set of booleans consistently named after the associated dom constructors. This could also encapsulate the parsing and defining of the different constructors. So something like:

struct SandboxDOMConstructors {
  SandboxOptions() { PodZero(this); }
  bool Parse(...);
  bool Define(...);
  bool XMLHttpRequest;
  bool TextEncoder;
  bool TextDecoder;
}

Basically, I want to encapsulate this stuff, so that it doesn't get unwieldy as we start to add more and more DOM constructors. Does that make sense?
Attachment #797888 - Flags: review?(bobbyholley+bmo) → review-
Assignee

Comment 37

6 years ago
(In reply to Bobby Holley (:bholley) from comment #34)
> > +    if (!(*found))
> 
> Why do we need the extra parens here?

I don't like !* together, but can remove it sure. Stupid old habits...

> 
> We should avoid putting functional code in NS_ENSURE_* macros. There are two
> options:
> 

Oh? I'd wonder why... is that a debugging thing, or is there any other practical reason for it? Anyway, I learned something new today.

(In reply to Bobby Holley (:bholley) from comment #36)
> I think we should make a new helper struct called SandboxDOMConstructors,

I like this.
 
> Basically, I want to encapsulate this stuff, so that it doesn't get unwieldy
> as we start to add more and more DOM constructors. Does that make sense?

Sure.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #37)
> > We should avoid putting functional code in NS_ENSURE_* macros. There are two
> > options:
> > 
> 
> Oh? I'd wonder why... is that a debugging thing, or is there any other
> practical reason for it? Anyway, I learned something new today.

It's just a stylistic thing. The macros are meant to convey "don't worry too much about this line, it's just handling exceptional behavior". Maintaining this pattern makes code easier to read, because you don't have to look at the NS_ENSURE lines too closely.
Assignee

Comment 39

6 years ago
Added static to the rest of the similar helper functions too. Was not sure if I should carry over the r+ or flag you r- again after these changes, so I just did the safe thing.
Attachment #797883 - Attachment is obsolete: true
Attachment #798831 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 40

6 years ago
Attachment #797885 - Attachment is obsolete: true
Attachment #798832 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 41

6 years ago
Posted patch TextEncoderSplinter Review
Attachment #797888 - Attachment is obsolete: true
Attachment #798833 - Flags: review?(bobbyholley+bmo)
Comment on attachment 798831 [details] [diff] [review]
part0: minor cleanups in sandbox

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

r=bholley assuming the change in XPCWrappedNativeScope was a mistake and will be removed.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ -241,5 @@
>      // So make sure to keep this set to true, here.
>      SandboxOptions options(cx);
>      options.wantXrays = true;
>      options.wantComponents = true;
> -    options.wantXHRConstructor = false;

er, what?
Attachment #798831 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 798832 [details] [diff] [review]
part1: parsing DOMConstructors option

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

Please make sure to get the relevant docs updated here.

In general, I think it would be better for all this parsing machinery to return bools rather than nsresults, since it's mostly just JSAPI stuff. But let's not worry about that now.
Attachment #798832 - Flags: review?(bobbyholley+bmo) → review+
Attachment #798833 - Flags: review?(bobbyholley+bmo) → review+
Assignee

Comment 44

6 years ago
(In reply to Bobby Holley (:bholley) from comment #42)
> >      SandboxOptions options(cx);
> >      options.wantXrays = true;
> >      options.wantComponents = true;
> > -    options.wantXHRConstructor = false;
> 
> er, what?

'false' has always been the default and will be in the future as well for all these DOM ctors.
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h#3705

What's the point of setting it false again here?
Assignee

Comment 45

6 years ago
(In reply to Bobby Holley (:bholley) from comment #43)
> In general, I think it would be better for all this parsing machinery to
> return bools rather than nsresults, since it's mostly just JSAPI stuff. But
> let's not worry about that now.

Probably... except the principal parsing part, but in general I came to the same conclusion.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #44)
> (In reply to Bobby Holley (:bholley) from comment #42)
> > >      SandboxOptions options(cx);
> > >      options.wantXrays = true;
> > >      options.wantComponents = true;
> > > -    options.wantXHRConstructor = false;
> > 
> > er, what?
> 
> 'false' has always been the default and will be in the future as well for
> all these DOM ctors.
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.
> h#3705
> 
> What's the point of setting it false again here?

Ah, ok. Nevermind then.
Assignee

Comment 48

6 years ago
These patches change Cu.Sandbox API slightly so the docs should be updated. Instead of the wantXHRConstructor options there is a wantDOMConstructors option now, that is an array of DOM constructor names the sandbox should have. Currently the supported constructors are: "XMLHttpRequest", "TextEncoder", "TextDecoder". More constructors will come. So the equivalent of the old wantXHRConstructor = true is now wantDOMConstructors = ["XMLHttpRequest"]
Keywords: dev-doc-needed
Assignee

Comment 50

6 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #48)
> These patches change Cu.Sandbox API slightly so the docs should be updated.
> Instead of the wantXHRConstructor options there is a wantDOMConstructors
> option now, that is an array of DOM constructor names the sandbox should
> have. Currently the supported constructors are: "XMLHttpRequest",
> "TextEncoder", "TextDecoder". More constructors will come. So the equivalent
> of the old wantXHRConstructor = true is now wantDOMConstructors =
> ["XMLHttpRequest"]

In Bug 892214 there is a patch that changes the name wantDOMConstructors to wantGlobalProperties. So in the documentation we should use wantGlobalProperties too.

Also, once I land that patch the SDK code should be updated too...
Depends on: 921399
You need to log in before you can comment on or make changes to this bug.