Closed
Bug 987111
Opened 11 years ago
Closed 10 years ago
Implement Xrays to Object objects
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bholley, Assigned: bholley)
References
(Blocks 1 open bug)
Details
(Keywords: addon-compat, dev-doc-complete)
Attachments
(13 files, 3 obsolete files)
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jedp
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•11 years ago
|
||
(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.
Comment 2•11 years ago
|
||
> 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•11 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.
Comment 4•11 years ago
|
||
> > 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...
Comment 5•11 years ago
|
||
> 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•11 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.
Comment 7•11 years ago
|
||
(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.
Comment 8•11 years ago
|
||
> 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•11 years ago
|
Blocks: CVE-2014-8636
Comment 9•11 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•11 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 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
This is almost done. Gabor, how do you feel about reviewing this? Peter is really busy these days.
Flags: needinfo?(gkrizsanits)
Assignee | ||
Comment 13•11 years ago
|
||
Comment 14•11 years ago
|
||
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 | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Comment 17•11 years ago
|
||
Assignee | ||
Comment 18•11 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 19•11 years ago
|
||
Assignee | ||
Comment 20•11 years ago
|
||
Assignee | ||
Comment 21•11 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 22•11 years ago
|
||
Attachment #8428055 -
Flags: review?(terrence)
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #8428056 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 24•11 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•11 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 26•11 years ago
|
||
Attachment #8428059 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #8428061 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #8428062 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 29•11 years ago
|
||
Attachment #8428064 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #8428065 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 31•11 years ago
|
||
Attachment #8428066 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #8428067 -
Flags: review?(gkrizsanits)
Comment 33•11 years ago
|
||
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 34•11 years ago
|
||
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 35•11 years ago
|
||
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•11 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.
Updated•11 years ago
|
Attachment #8428056 -
Flags: review?(gkrizsanits) → review+
Comment 37•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8428059 -
Flags: review?(gkrizsanits) → review+
Comment 38•11 years ago
|
||
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 39•11 years ago
|
||
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+
Comment 40•11 years ago
|
||
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 41•11 years ago
|
||
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 42•11 years ago
|
||
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 43•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8428065 -
Flags: review?(gkrizsanits) → review+
Comment 44•11 years ago
|
||
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 45•11 years ago
|
||
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•11 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•11 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•11 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•11 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.
Assignee | ||
Comment 50•11 years ago
|
||
Comment 51•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #47)
> Is that clearer?
Yepp, thanks, everything looks great now.
Assignee | ||
Comment 52•10 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•10 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)
Comment 54•10 years ago
|
||
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+
Assignee | ||
Comment 55•10 years ago
|
||
No longer depends on: 1011084
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 | ||
Comment 57•10 years ago
|
||
Try push with the conflict handled:
https://tbpl.mozilla.org/?tree=Try&rev=7cc2e09b0e79
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 58•10 years ago
|
||
Assignee | ||
Comment 59•10 years ago
|
||
Comment 60•10 years ago
|
||
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
Comment 61•10 years ago
|
||
Comment 62•10 years ago
|
||
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.
Comment 63•10 years ago
|
||
(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?
Updated•10 years ago
|
Keywords: addon-compat
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 64•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/500b82107c18
https://hg.mozilla.org/mozilla-central/rev/0e0a7e21a765
https://hg.mozilla.org/mozilla-central/rev/7133191966b6
https://hg.mozilla.org/mozilla-central/rev/4fdeccffdf55
https://hg.mozilla.org/mozilla-central/rev/c4d288e2a0a6
https://hg.mozilla.org/mozilla-central/rev/3a1f67c7f223
https://hg.mozilla.org/mozilla-central/rev/3d93bce9a456
https://hg.mozilla.org/mozilla-central/rev/d9788144b427
https://hg.mozilla.org/mozilla-central/rev/40b5dff1e985
https://hg.mozilla.org/mozilla-central/rev/5951773bbbe3
https://hg.mozilla.org/mozilla-central/rev/5c0a8d16c227
https://hg.mozilla.org/mozilla-central/rev/9592f7abff55
https://hg.mozilla.org/mozilla-central/rev/8501686e4923
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 65•10 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)
Comment 66•10 years ago
|
||
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).
Comment 68•10 years ago
|
||
There are now some docs for Object Xrays here: https://developer.mozilla.org/en-US/docs/Xray_vision, and in particular here: https://developer.mozilla.org/en-US/docs/Xray_vision#Xrays_for_JavaScript_objects.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•