Closed Bug 987111 Opened 11 years ago Closed 10 years ago

Implement Xrays to Object objects

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

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?
(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.
Depends on: 987110
> 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?
(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...
(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!
Blocks: 787070
Depends on: 992958
(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.
(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.
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)
Depends on: 1011084
Depends on: 1015380
Depends on: 1015396
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.
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)
Properties are supposed to be enumerable by default. It's unfortunate that the default is reversed in SpiderMonkey.
Attachment #8428057 - Flags: review?(efaustbmo)
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)
Attachment #8428064 - Flags: review?(gkrizsanits)
Attachment #8428065 - Flags: review?(gkrizsanits)
Attachment #8428066 - Flags: review?(gkrizsanits)
Attached patch Part 12 - Tests. v1 (deleted) — Splinter Review
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+
(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+
(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.
(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. ;-)
(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.
(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.
Final all-platform try push, including jed's patch from bug 1011084 (the last remaining blocker). https://tbpl.mozilla.org/?tree=Try&rev=2389a9b5113f
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)
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)
Depends on: 1021244
Try push with the conflict handled: https://tbpl.mozilla.org/?tree=Try&rev=7cc2e09b0e79
Flags: needinfo?(bobbyholley)
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?
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).
Depends on: 1021994
I'm working on some docs.
Flags: needinfo?(wbamberg)
Depends on: 1022016
Depends on: 1022208
Blocks: 856067
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: