Implement Xrays to Object objects

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug, {addon-compat, dev-doc-complete})

unspecified
mozilla32
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 3 obsolete attachments)

1.36 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.36 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.44 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.19 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.65 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.25 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.51 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
10.06 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
8.60 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
4.60 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
674 bytes, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
9.55 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.95 KB, patch
jedp
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
For the XrayToJS project, I've been pondering what to do about pure Object instances ( {foo: 42} ). The purist in me wants to just make them opaque, but I fear that this will lead to a lot of unnecessary waiving. In practice, I think something simple might actually be pretty useful.

So I'm considering implementing Object Xrays that do all (or some) of the following:
* Only resolve |own| properties
* Only resolve non-accessor (i.e. value) properties
* Only resolve properties that do not shadow anything on Object.prototype.
* Only resolve non-callable properties.
* Only resolve properties that are either primitive or same-origin with the object.

The main goal is to make simple Dictionary-like objects work, while eschewing all of the dangerous stuff.

What are peoples' thoughts on this one? Are any of these too restrictive/lenient? Other thoughts on things to disallow?
(In reply to Bobby Holley (:bholley) from comment #0)
> * Only resolve |own| properties

What is the risk in walking up on the proto-chain? Is this about shadowing?

> * Only resolve non-callable properties.

I wonder if it would make sense to put options on the waiver about what to waive. On one hand I don't want JS code being able to fake something native. Like if we expect an Element as an argument, and we only check name, coordinates or something simple on it, currently content cannot trick our code just by passing in a JS object instead with some fake data properties on it, but if we enable Xray on JS object by default it could...

On the other hand just because I want to read some data from a JS object I don't want to turn off the whole xray protection and cross fingers either... Or do extra dance with isProxy/defineProperty etc... So I'm thinking about a very restrictive default behavior for pure Objects (even opaque) and some options about what to waive explicitly.
> What is the risk in walking up on the proto-chain?

The risk is as follows.  Say you have a DOM API like so:

  dictionary Foo {
    DOMString name;
  };
  interface Bar {
    Foo getNameInfo();
  };

here the returned object might look like { name: "something" } or it might look like {} and the caller should be able to tell those cases apart.

Now say we have an Xray to such a dictionary and the web page has done Object.prototype.name = "I am evil".  What should the Xray see?

> I wonder if it would make sense to put options on the waiver about what to waive.

We're not talking about waivers here.  We're talking all-up Xrays.

> currently content cannot trick our code just by passing in a JS object instead

Uh.. it can, since we have no Xrays to random objects at all right now.  You mean it couldn't if such Xrays were opaque, right?

Note also that the right way to handle "expect an Element as an argument" is to have WebIDL saying so!

That said, the "opaque by default" setup might be ok as long as bindings can auto-create the right waivers or something?
Assignee

Comment 3

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #2)
> > What is the risk in walking up on the proto-chain?

More specifically -  the idea is to make these appear as much like simple { foo: 42 } objects as possible. An Xray to a Date object will appear to have Date.prototype as its __proto__ regardless of whether or not the underlying object has some other funny __proto__. So we should do the same for Object, and therefore we can't resolve any non-own content-side properties.

Basically, I just want to handle the simple cases that would constitute 90% of the pain if we just made these opaque. Those seem pretty sane to implement securely, which is why I'm proposing this.
> > I wonder if it would make sense to put options on the waiver about what to waive.
> 
> Uh.. it can, since we have no Xrays to random objects at all right now.  You
> mean it couldn't if such Xrays were opaque, right?

Ah sorry, I confused things a bit here. But yes, so I was thinking making xrays on JS object opaque by default and let the consumer choose what to allow to be seen on it. And for implementing that we could use custom waivers (or flags on the current waiver). But we can just add these flags to the xray itself. Whatever is simpler.

> 
> Note also that the right way to handle "expect an Element as an argument" is
> to have WebIDL saying so!

For WebIDL that is fine. But we use Xrays for add-ons without any WebIDL around.

> 
> That said, the "opaque by default" setup might be ok as long as bindings can
> auto-create the right waivers or something?

Yes, something like that. I'm not sure if it's feasible or the right thing to do
just an idea. I'm not a big fan of making the full transparent the default
behavior as it is now...
> But we use Xrays for add-ons without any WebIDL around.

Maybe we shouldn't?  If we were serious about this sort of thing, addons would say what they expect, and we'd automatically enforce it somehow...
Assignee

Comment 6

5 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Ah sorry, I confused things a bit here. But yes, so I was thinking making
> xrays on JS object opaque by default and let the consumer choose what to
> allow to be seen on it.

That sounds too complicated, IMO. Just having to views (Xray vs Waived) is already a major source of confusion.
(In reply to Boris Zbarsky [:bz] from comment #5)
> > But we use Xrays for add-ons without any WebIDL around.
> 
> Maybe we shouldn't?  If we were serious about this sort of thing, addons
> would say what they expect, and we'd automatically enforce it somehow...

I don't know how hard that would be in practice, but in general I like the
idea to use WebIDL for add-ons too.

(In reply to Bobby Holley (:bholley) from comment #6)
> That sounds too complicated, IMO. Just having to views (Xray vs Waived) is
> already a major source of confusion.

Probably you're right. I just wish I had some behavior like this for waivers then...
Something that does waive the xray on the native but not on the expando object of it.
So a safer alternative for window.wrappedJSObject.someBooleanProperty.
> I don't know how hard that would be in practice

We'd need to write either a JS version or a C++ version that operates on IDL data directly of the glue code.

I suspect doing the JS version might be simpler, honestly.  Still some work, though.  Esp. if the addon just ships IDL and we have to parse it!
Assignee

Updated

5 years ago
Assignee

Updated

5 years ago
Blocks: 787070
Assignee

Updated

5 years ago
Depends on: 992958

Comment 9

5 years ago
(In reply to Bobby Holley (:bholley) from comment #0)
> * Only resolve non-callable properties.

What is the point of forbidding callable properties? It makes sense to create an API (e.g. in an extension) that accepts callbacks in a dictionary, and this does not seem to be dangerous.
Assignee

Comment 10

5 years ago
(In reply to Gábor Molnár from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #0)
> > * Only resolve non-callable properties.
> 
> What is the point of forbidding callable properties? It makes sense to
> create an API (e.g. in an extension) that accepts callbacks in a dictionary,
> and this does not seem to be dangerous.

Fundamentally, Xrays are designed to avoid accidentally executing content code. So I don't think we should make it easy for chrome code to accidentally get its hands on a content function.

For code that ships in the browser, dictionary-like stuff should go through WebIDL anyway, which gives us the benefits of type enforcement. Addons can't use WebIDL, so they'd have a more compelling use case for something like what Gábor suggests. But I think that the benefits of addons using Xrays (instead of waivers) in this situation is outweighed by the benefits of preventing such a free flow of content functions across the content<->chrome boundary throughout the platform.
Assignee

Comment 12

5 years ago
This is almost done. Gabor, how do you feel about reviewing this? Peter is really busy these days.
Flags: needinfo?(gkrizsanits)
I think I'm familiar with xrays enough to do most parts for sure. If I find something I don't get we can ask for an sr for that part from someone.
Flags: needinfo?(gkrizsanits)
Assignee

Updated

5 years ago
Depends on: 1011084
Assignee

Updated

5 years ago
Depends on: 1015380
Assignee

Updated

5 years ago
Depends on: 1015396
Assignee

Comment 18

5 years ago
It's unfortunate the we need to operate on the raw JSPropertyDescriptor for
|other|, but the specialization that makes Handle<JSPropertyDescriptor> work is
declared later in the file, which isn't kosher.
Assignee

Comment 21

5 years ago
It's unfortunate the we need to operate on the raw JSPropertyDescriptor for
|other|, but the specialization that makes Handle<JSPropertyDescriptor> work is
declared later in the file, which isn't kosher.
Attachment #8428050 - Attachment is obsolete: true
Attachment #8428051 - Attachment is obsolete: true
Attachment #8428052 - Attachment is obsolete: true
Attachment #8428054 - Flags: review?(terrence)
Assignee

Comment 24

5 years ago
Properties are supposed to be enumerable by default. It's unfortunate that
the default is reversed in SpiderMonkey.
Attachment #8428057 - Flags: review?(efaustbmo)
Assignee

Comment 25

5 years ago
This gives us strictly more information than we had before, which turns out to
be useful. We can still get the old behavior by testing the identity of
desc.object(), which I've done in one of the two existing uses for existing_desc.
The other (in DOMXrayTraits::defineProperty) is actually more correct with the
full (non-own) lookup.
Attachment #8428058 - Flags: review?(gkrizsanits)
Assignee

Comment 31

5 years ago
Attachment #8428066 - Flags: review?(gkrizsanits)
Assignee

Comment 32

5 years ago
Attachment #8428067 - Flags: review?(gkrizsanits)
What do we do with an object that has a regular js object as proto (other than Object.prototype)

I'm not sure what IdentifyStandardInstanceOrPrototype returns for them, are those xrayables or not? I would think they are not but have not seen any test for it...
Comment on attachment 8428055 [details] [diff] [review]
Part 2 - Introduce a method to determine whether a given PropertyDescriptor is an accessor prop. v1

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

r=me
Attachment #8428055 - Flags: review?(terrence) → review+
Comment on attachment 8428054 [details] [diff] [review]
Part 1 - Add an assign() method to MutablePropertyDescriptorOperations. v1

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

r=me
Attachment #8428054 - Flags: review?(terrence) → review+
Assignee

Comment 36

5 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #33)
> What do we do with an object that has a regular js object as proto (other
> than Object.prototype)

It doesn't matter - the XrayWrapper always sees the Xrayed Object.prototype as the proto. This is similar to what happens for WebIDL objects.
 
> I'm not sure what IdentifyStandardInstanceOrPrototype returns for them, are
> those xrayables or not? I would think they are not but have not seen any
> test for it...

There is test coverage for making sure the protos are correct. I can make it more useful by setting one of the protos on the object passed to testXray to something arbitrary. Please include that in your review comments for part 12 and I'll add it.
Attachment #8428056 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8428058 [details] [diff] [review]
Part 5 - Fill out existing_desc with all properties, not just |own| ones. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2025,3 @@
>          return false;
>  
> +    if (existing_desc.object() == wrapper && existing_desc.isPermanent())

As we discussed, this check should be explained in a comment what it _should_ mean and maybe mention the current side effect that might come with it since we sometimes lie about this...
Attachment #8428058 - Flags: review?(gkrizsanits) → review+
Attachment #8428059 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8428061 [details] [diff] [review]
Part 7 - Make JSProto_Object COWs take precedence over Xrays. v1

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

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +126,5 @@
> +static bool
> +ForceCOWBehavior(JSObject *obj)
> +{
> +    if (IdentifyStandardInstanceOrPrototype(obj) == JSProto_Object) {
> +        MOZ_ASSERT(GetXrayType(obj) == XrayForJSObject);

Please add some text to MOZ_ASSERT.
Attachment #8428061 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8428062 [details] [diff] [review]
Part 8 - Implement resolveOwnProperty and enumerateNames for Object instances. v1

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

Everything makes sense except that JSITER_HIDDEN flag could you explain me that one?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +446,5 @@
> +    // Disallow accessor properties.
> +    if (desc.hasGetterOrSetter())
> +        return SilentFailure(cx, id, "Property has accessor");
> +
> +    // Apply extra scrutiny to objects.

I pick this comment to point this out... I really appreciate your commenting work, it helped me a lot over the years and I'm mighty thankful for them. One think I would like to ask is to be a bit more generous to us non-native speakers, because as much I like to learn new words, I feel like I have to use the dictionary way too often to interpret your comments :) Thinking about stuff like "ephemeral" ;) (you can leave this as it is)

@@ +512,5 @@
>          return ok;
>  
> +    RootedObject target(cx, getTargetObject(wrapper));
> +    if (!isPrototype(holder)) {
> +        switch (getProtoKey(holder)) {

I guess you need the switch here because this part will be extended in some future patches? In any case this section could use some more comment, or even better getProtoKey could enumerated the cases and what they mean in a comment, which ever you prefer.

@@ +646,5 @@
> +            MOZ_ASSERT(props.empty());
> +            {
> +                JSAutoCompartment ac(cx, target);
> +                AutoIdVector targetProps(cx);
> +                if (!js::GetPropertyNames(cx, target, JSITER_OWNONLY | JSITER_HIDDEN, &targetProps))

JSITER_HIDDEN ? I don't get that one...
Attachment #8428062 - Flags: review?(gkrizsanits) → review+
It's getting late here... I'll do the rest tomorrow. I haven't found any big issue in them so it should be straightforward from here.
Comment on attachment 8428057 [details] [diff] [review]
Part 4 - Proxy::set should create enumerable properties. v1

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

That's silly. r=me
Attachment #8428057 - Flags: review?(efaustbmo) → review-
Comment on attachment 8428057 [details] [diff] [review]
Part 4 - Proxy::set should create enumerable properties. v1

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

r=me with the right flags this time.
Attachment #8428057 - Flags: review- → review+
Comment on attachment 8428064 [details] [diff] [review]
Part 9 - Implement defineProperty for Object Xrays. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +641,5 @@
> +    // Object instances are special. For that case, we forward property
> +    // definitions to the underlying object if the following conditions are met:
> +    // * The property being defined is a value-prop.
> +    // * The property being defined is either a primitive or subsumed by the target.
> +    // * As seen from the Xray, any existing property is an |own| value-prop.

I don't fully understand this last condition.

@@ +655,5 @@
> +        }
> +        if (desc.value().isObject() &&
> +            !AccessCheck::subsumes(target, js::UncheckedUnwrap(&desc.value().toObject())))
> +        {
> +            JS_ReportError(cx, "Not allowed to define privileged object as property on [Object] XrayWrapper");

It's not necessary privileged, it's enough to be cross origin too, no?

@@ +663,5 @@
> +            JS_ReportError(cx, "Not allowed to overwrite accessor property on [Object] XrayWrapper");
> +            return false;
> +        }
> +        if (existingDesc.object() && existingDesc.object() != wrapper) {
> +            JS_ReportError(cx, "Not allowed to shadow non-own property on [Object] XrayWrapper");

Well technically xray might ignore several members of the proto chain (with several non-own properties that can be totally shadowed) so I find this message somewhat misleading. Especially that own properties are not shadowed either since we don't have expandos.
Attachment #8428064 - Flags: review?(gkrizsanits) → review+
Attachment #8428065 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8428066 [details] [diff] [review]
Part 11 - Flip on Object Xrays. v1

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

\o/
Attachment #8428066 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8428067 [details] [diff] [review]
Part 12 - Tests. v1

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

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +165,5 @@
>      ok(protoMethods.length > 0, "Need something to test");
>      is(xrayProto, iwin[classname].prototype, "Xray proto is correct");
>      is(xrayProto, xray.__proto__, "Proto accessors agree");
> +    var protoProto = classname == "Object" ? null : iwin.Object.prototype;
> +    is(Object.getPrototypeOf(xrayProto), protoProto, "proto proto is correct");

There are two proto tests I'm missing, one is an object with a vanila JS Object as its proto, the other is an instance of Date let's say. Some basic checks on them that reflects how we handle proto in these cases would be great for people who don't want to dive into XrayWrapper.cpp to figure out how this all works.

@@ +211,5 @@
> +    testXray('Object', new iwin.Object(), new iwin.Object(), []);
> +
> +    // Construct an object full of tricky things.
> +    var trickyObject =
> +      iwin.eval('new Object({ \

Not sure why you need new Object here, but cannot hurt I guess...

@@ +212,5 @@
> +
> +    // Construct an object full of tricky things.
> +    var trickyObject =
> +      iwin.eval('new Object({ \
> +                   primitiveProp: 42, objectProp: { foo: 2}, \

{ foo: 2 }

@@ +218,5 @@
> +                   get getterProp() { return 2; }, \
> +                   set setterProp(x) { }, \
> +                   get getterSetterProp() { return 3; }, \
> +                   set getterSetterProp(x) { }, \
> +                   callableProp: function() {}, \

{ }

@@ +234,5 @@
> +    // we placed on the xray proto.
> +    var propCount = 0;
> +    for (let prop in trickyObject) {
> +      if (prop != 'expando')
> +        trickyObject[prop] = trickyObject[prop];

This way we don't see the difference between success and silent failure... I know that we throw for most of the important cases, but not for the actual setting the property part.

@@ +251,5 @@
> +    is(Object.getOwnPropertyDescriptor(trickyObject, 'primitiveProp').value, 42, "getOwnPropertyDescriptor works");
> +    is(Object.getOwnPropertyDescriptor(trickyObject, 'xoProp'), undefined, "filtering works with getOwnPropertyDescriptor");
> +
> +    //
> +    // Test defineProperty.

uber NIT: why is this comment different than the rest? (feel free to ignore)
Attachment #8428067 - Flags: review?(gkrizsanits) → review+
Assignee

Comment 46

5 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #39)
> Comment on attachment 8428062 [details] [diff] [review]
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +446,5 @@
> > +    // Disallow accessor properties.
> > +    if (desc.hasGetterOrSetter())
> > +        return SilentFailure(cx, id, "Property has accessor");
> > +
> > +    // Apply extra scrutiny to objects.
> 
> I pick this comment to point this out... I really appreciate your commenting
> work, it helped me a lot over the years and I'm mighty thankful for them.
> One think I would like to ask is to be a bit more generous to us non-native
> speakers, because as much I like to learn new words, I feel like I have to
> use the dictionary way too often to interpret your comments :) Thinking
> about stuff like "ephemeral" ;) (you can leave this as it is)

Thanks! I'll try. ;-)
 
> @@ +512,5 @@
> >          return ok;
> >  
> > +    RootedObject target(cx, getTargetObject(wrapper));
> > +    if (!isPrototype(holder)) {
> > +        switch (getProtoKey(holder)) {
> 
> I guess you need the switch here because this part will be extended in some
> future patches? In any case this section could use some more comment, or
> even better getProtoKey could enumerated the cases and what they mean in a
> comment, which ever you prefer.

I'll add some comments.

> JSITER_HIDDEN ? I don't get that one...

You're right - this should be flags | JSITER_OWNONLY, since in the XrayWrapper::enumerate path, we don't want to iterate over non-enumerable properties. Good catch.
Assignee

Comment 47

5 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #43)
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +641,5 @@
> > +    // Object instances are special. For that case, we forward property
> > +    // definitions to the underlying object if the following conditions are met:
> > +    // * The property being defined is a value-prop.
> > +    // * The property being defined is either a primitive or subsumed by the target.
> > +    // * As seen from the Xray, any existing property is an |own| value-prop.
> 
> I don't fully understand this last condition.

I changed it to:

    // * As seen from the Xray, any existing property that we would overwrite is an
    //   |own| value-prop.

Is that clearer?

> > +            JS_ReportError(cx, "Not allowed to define privileged object as property on [Object] XrayWrapper");
> 
> It's not necessary privileged, it's enough to be cross origin too, no?

Good point. Fixed.


> @@ +663,5 @@
> > +            JS_ReportError(cx, "Not allowed to overwrite accessor property on [Object] XrayWrapper");
> > +            return false;
> > +        }
> > +        if (existingDesc.object() && existingDesc.object() != wrapper) {
> > +            JS_ReportError(cx, "Not allowed to shadow non-own property on [Object] XrayWrapper");
> 
> Well technically xray might ignore several members of the proto chain (with
> several non-own properties that can be totally shadowed) so I find this
> message somewhat misleading.

It was meant to refer to properties that are resolved on the Xray, rather than properties of the underlying object.

I changed the message to:

Not allowed to shadow non-own Xray-resolved property on [Object] XrayWrapper

> Especially that own properties are not shadowed
> either since we don't have expandos.

Fair point, but in this case I'm referring to 'shadowing' in the proto-vs-derived sense. I wish we had more names. ;-)
Assignee

Comment 48

5 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #45)
> Comment on attachment 8428067 [details] [diff] [review]
> Part 12 - Tests. v1
> 
> Review of attachment 8428067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/tests/chrome/test_xrayToJS.xul
> @@ +165,5 @@
> >      ok(protoMethods.length > 0, "Need something to test");
> >      is(xrayProto, iwin[classname].prototype, "Xray proto is correct");
> >      is(xrayProto, xray.__proto__, "Proto accessors agree");
> > +    var protoProto = classname == "Object" ? null : iwin.Object.prototype;
> > +    is(Object.getPrototypeOf(xrayProto), protoProto, "proto proto is correct");
> 
> There are two proto tests I'm missing, one is an object with a vanila JS
> Object as its proto, the other is an instance of Date let's say. Some basic
> checks on them that reflects how we handle proto in these cases would be
> great for people who don't want to dive into XrayWrapper.cpp to figure out
> how this all works.

Ok. I'll call testXray a couple of times for objects and pass in different prototype configurations.

> @@ +234,5 @@
> > +    // we placed on the xray proto.
> > +    var propCount = 0;
> > +    for (let prop in trickyObject) {
> > +      if (prop != 'expando')
> > +        trickyObject[prop] = trickyObject[prop];
> 
> This way we don't see the difference between success and silent failure... I
> know that we throw for most of the important cases, but not for the actual
> setting the property part.

Ok, I'll try setting a property to something different and checking it later.

> > +
> > +    //
> > +    // Test defineProperty.
> 
> uber NIT: why is this comment different than the rest? (feel free to ignore)

Because it refers to multiple blocks. I'll remove the triple-// and keep the newline.
Assignee

Comment 49

5 years ago
(In reply to Bobby Holley (:bholley) from comment #48)
> Ok. I'll call testXray a couple of times for objects and pass in different
> prototype configurations.

Actually, the date thing is hard to test, because objects generally don't behave well when prototyped to different specialized objects (valueOf called on incomaptible object, etc). So I'll just test the case of an arbitrary proto and make sure that Xrays show the standard proto.
(In reply to Bobby Holley (:bholley) from comment #47) 
> Is that clearer?

Yepp, thanks, everything looks great now.
Assignee

Comment 52

5 years ago
Final all-platform try push, including jed's patch from bug 1011084 (the last remaining blocker).

https://tbpl.mozilla.org/?tree=Try&rev=2389a9b5113f
Assignee

Comment 53

5 years ago
Bug 1011084 is getting hung up on various things, and we're currently blocked on
it because Object Xrays filter out callables. So let's just turn off Xrays here
for that case, so that we can enable them everywhere else.
Attachment #8433763 - Flags: review?(jparsons)
Assignee

Updated

5 years ago
Blocks: 1020609
Comment on attachment 8433763 [details] [diff] [review]
Part 0 - Temporarily waive Xrays on the aOptions argument passed to mozId.watch and mozId.request. v1

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

Thanks, Bobby.  This is very helpful.
Attachment #8433763 - Flags: review?(jparsons) → review+
This disagreed with something that came in from a merge of b2g-inbound and fx-team, and that disagreement broke a lot of stuff: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=41084683&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41082279&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41082502&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41082016&tree=Mozilla-Inbound


Since I couldn't tell what caused the disagreement from the other side, I had to back out all 13 changesets from this bug in https://hg.mozilla.org/integration/mozilla-inbound/rev/fe14647a422d to hopefully get the tree back into a somewhat good state.


A suspect on the b2g-inbound side is bug 899260 since it touched the webapps stuff.
A suspect on the fx-team side is bug 1019643, since it touches the browser element stuff.
Flags: needinfo?(bobbyholley)
Assignee

Updated

5 years ago
Depends on: 1021244
Assignee

Comment 57

5 years ago
Try push with the conflict handled:

https://tbpl.mozilla.org/?tree=Try&rev=7cc2e09b0e79
Flags: needinfo?(bobbyholley)
Assignee

Updated

5 years ago
Depends on: 1021312
After conducting a regression test to see why an add-on I use broke, it lead me to this patch.  The add-on that broke can be found at https://addons.mozilla.org/en-US/firefox/addon/flashstopper/?src=ss

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
What is on the js error console? What is broken exactly?

It's probably related to the sandbox the addon is using. If evalInSandbox returns a js object you will have to do unwrappedJSObject on it. Similarily, if you place a JS object on the |this| from the scope of the sandbox, from outside that object will be opaque by default and you will have to call wrappedJSObject on it.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #62)
> What is on the js error console? What is broken exactly?
> 
> It's probably related to the sandbox the addon is using. If evalInSandbox
> returns a js object you will have to do unwrappedJSObject on it. Similarily,
> if you place a JS object on the |this| from the scope of the sandbox, from
> outside that object will be opaque by default and you will have to call
> wrappedJSObject on it.

It stops Flash videos from automatically playing. The link I posted to FS has all the info about it.

My concern is to what extent will this patch 'break' other add-ons?
Assignee

Comment 65

5 years ago
Yeah, we should probably expedite documentation of this setup, because people are going to want to understand the breakage.

FWIW, in a debug build, you'll get console spew telling you if a property access was silently denied.
Flags: needinfo?(wbamberg)
Dev docs would be very useful, since I believe this change is causing a bunch of Travis failures on Gaia for the ringtones app. It's easy enough to fix, but I imagine there may be a better way to fix this than what I'm planning on (which is using the private variables of my object instead of the public getters).

Updated

5 years ago
Depends on: 1021994
I'm working on some docs.
Flags: needinfo?(wbamberg)
Assignee

Updated

5 years ago
Depends on: 1022016

Updated

5 years ago
Depends on: 1022208
Assignee

Updated

5 years ago
Blocks: 856067
You need to log in before you can comment on or make changes to this bug.