The semantics of XBL fields dictate that they must be accessible to code running in the bound scope, which made things tricky when hoisting XBL into a separate compartment. We ended up defining the field as a property on the bound node (as before), and then waiving Xray whenever the XBL code accesses the field. Various in-content bindings make heavy use of <field>s to store state. Because we're waiving Xray here, this ends up losing us a lot of the protections introduced by XBL scopes. Talking to dolske and jaws, it doesn't sound like there's no real reason we need to do things this way, and could instead define them in the constructor as expandos on the Xray-ed |this|. In bug 843829, I'm trying to reduce the attack surface of XBL scopes by making all content objects without an explicit Xray waiver opaque. But this involves applying a full Xray waiver whenever accessing fields, which, in the current field-happy world, causes this waiver to propagate just about everywhere, losing our Xray vision. But really, this is nothing that a clever attacker couldn't do anyway. All bets are off once Xray is waived, and content can lead us wherever it wants. Marking this s-s because I don't want to make too much noise about this XBL stuff for the time being.

Comment on attachment 722632 [details] [diff] [review] Part 3 - De-field marquee. v1 Review of attachment 722632 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to get a sign-off from Neil that these changes are OK too.

(In reply to Jared Wein [:jaws] from comment #5) > Comment on attachment 722632 [details] [diff] [review] > Part 3 - De-field marquee. v1 > > Review of attachment 722632 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd like to get a sign-off from Neil that these changes are OK too. Where these refers to all three patches.

Once we do this, can we kill off the automatic Xray waiver for fields altogether?

Comment on attachment 722632 [details] [diff] [review] Part 3 - De-field marquee. v1 I don't know anything about marquee elements nor what this bug is even talking about.

(In reply to Boris Zbarsky (:bz) from comment #7) > Once we do this, can we kill off the automatic Xray waiver for fields > altogether? It would be green in our tree, sure. But it seems like the main point of all of our field shenanigans is to preserve compat as much as possible, right? It seems like it would require changes to every in-content XBL binding that uses <field>s, since most all of them would depend on manipulating that field from XBL in some way, right?

Jared, in light of comment 9: Am I green to land, or do you want input from someone else?

It would be good to get another set of eyes looking at this too. Maybe Dao can help?

What info do you need? Would you like me to review a patch? All patches?

(In reply to Dão Gottwald [:dao] from comment #12) > What info do you need? Would you like me to review a patch? All patches? Yeah, if you could.

> It seems like it would require changes to every in-content XBL binding that uses <field>s Oh, I guess the issue would be extensions (remote XBL is handled by just not using a separate scope there is the plan, iirc). Yeah, ok.

Comment on attachment 722631 [details] [diff] [review] Part 2 - De-field videocontrols.xml. v1 > <constructor> > <![CDATA[ >+ this.scrubberNameFormat = "&scrubberScale.nameFormat;"; <![CDATA[... &scrubberScale.nameFormat; ...]]> isn't going to work as intended.

(In reply to Dão Gottwald [:dao] from comment #15) > Comment on attachment 722631 [details] [diff] [review] > <![CDATA[... &scrubberScale.nameFormat; ...]]> isn't going to work as > intended. Can you suggest a fix? This isn't my area of expertise.

You can break out of the CDATA section like this: ]]>&scrubberScale.nameFormat;<![CDATA[

(In reply to Dão Gottwald [:dao] from comment #15) > Comment on attachment 722631 [details] [diff] [review] > Part 2 - De-field videocontrols.xml. v1 > > > <constructor> > > <![CDATA[ > >+ this.scrubberNameFormat = "&scrubberScale.nameFormat;"; > > <![CDATA[... &scrubberScale.nameFormat; ...]]> isn't going to work as > intended. Thank you for catching that.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a174d3a0ce2e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a7ac3aee3d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/da71d1eb9521

Comment on attachment 722632 [details] [diff] [review] Part 3 - De-field marquee. v1 Flagging for aurora approval. This applies to all three patches. One of the major features that's now on aurora is the ability to run in-content XBL in a separate compartment. This grants us a number of security wins, but the security invariants are weakened each time we enter the content compartment, which we have to do for <field>. A number of our in-content XBL bindings use <field>, though there's no effective difference in doing that as opposed to just defining expandos on the target object. This patch is low-ish risk, insofar as everything that touches XBL bindings has an inherent risk. I suggest baking this for a few days on m-c, and then landing it on m-a if there's no fallout.

(In reply to Bobby Holley (:bholley) from comment #20) > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a7ac3aee3d Backed out the videocontrols.xml patch for mochitest-1 orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a247547c35 https://tbpl.mozilla.org/php/getParsedLog.php?id=20613002&tree=Mozilla-Inbound

Backed out the rest at bholley's request. https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca452c42502

Bobby, the test failure looks to come from this line: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#707 Does that usage of document.getAnonymousElementByAttribute not work in this scenario?

Comment on attachment 722632 [details] [diff] [review] Part 3 - De-field marquee. v1 Removing approval for now.

(In reply to Bobby Holley (:bholley) from comment #20) > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a174d3a0ce2e > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a7ac3aee3d > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/da71d1eb9521 Merge of first landing: https://hg.mozilla.org/mozilla-central/rev/da71d1eb9521 https://hg.mozilla.org/mozilla-central/rev/e0a7ac3aee3d https://hg.mozilla.org/mozilla-central/rev/a174d3a0ce2e Leaving open for remaining work.

Ugh, so I just sunk a number of hours into this. After a very very long trail of breadcrumbs, I've figured out what's happening. The videoControls binding has a <field> called randomID. This is turns out to be the culprit, and the error goes away when I undo the change from <field> to Xray expando. This was somewhat baffling at first, because none of the code seemed to depend on that property living on the underlying object. After some time, however, I discovered that the property was actually being undefined on the underlying object by content/xbl when the binding was stripped off. And the videoControls code actually depends on randomID becoming |undefined| when the element is unbound and it encounters this element in an event handler. Yuck. So anyway. The most straightforward solution is to replace access to randomID with a function that checks whether the element is bound and returns undefined if it isn't. What's the cleanest way to detect whether an element is bound or not?

I decided to hack something up with document.getAnonymousNodes, which seems to work. I also de-fielded TouchUtils, which I missed last time: https://tbpl.mozilla.org/?tree=Try&rev=ba48f66d7c25

Looks green modulo intermittent orange. Flagging jaws for review.

(In reply to Bobby Holley (:bholley) from comment #27) > videoControls code actually depends on randomID becoming |undefined| when > the element is unbound and it encounters this element in an event handler. Sounds like you could just delete randomID in the XBL <destructor>?

(In reply to Dão Gottwald [:dao] from comment #30) > (In reply to Bobby Holley (:bholley) from comment #27) > > videoControls code actually depends on randomID becoming |undefined| when > > the element is unbound and it encounters this element in an event handler. > > Sounds like you could just delete randomID in the XBL <destructor>? Yes, but AFAICT XBL <destructor>s aren't reliably called, per bug 230086.

(In reply to Bobby Holley (:bholley) from comment #31) > (In reply to Dão Gottwald [:dao] from comment #30) > > (In reply to Bobby Holley (:bholley) from comment #27) > > > videoControls code actually depends on randomID becoming |undefined| when > > > the element is unbound and it encounters this element in an event handler. > > > > Sounds like you could just delete randomID in the XBL <destructor>? > > Yes, but AFAICT XBL <destructor>s aren't reliably called, per bug 230086. Because the binding wouldn't be detached in that case, so we'd get the same behavior as with <field>.

Well, ClearDocument doesn't appear to fire destructors, for example: http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsBindingManager.cpp#858 If you prefer though, I can try implementing it with a <destructor> and see if anything breaks.

Yes, I'd prefer that.

Comment on attachment 726420 [details] [diff] [review] Part 2 - De-field videocontrols.xml. v4 >@@ -1628,18 +1625,19 @@ > // If the video is not at the start, then we probably just > // transitioned into or out of fullscreen mode, and we don't want > // the controls to remain visible. this.controlsTimeout is a full > // 5s, which feels too long after the transition. > if (video.currentTime !== 0) { > this.delayHideControls(this.Utils.HIDE_CONTROLS_TIMEOUT_MS); > } > } >- }) ]]> >- </field> >+ }; >+ ]]> >+ </constructor> > > </implementation> nit: indent ]]> and </constructor> with two more spaces

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/34e30f1b46d8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/01207a5deca5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/254cde8d3974

Backed out scale and videocontrols in https://hg.mozilla.org/integration/mozilla-inbound/rev/a58872e5c1bb for Android bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=20809560&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=20809468&tree=Mozilla-Inbound

Marquee patch: https://hg.mozilla.org/mozilla-central/rev/254cde8d3974

Doh, stupid typo in TouchControls, which only affects android (the call to init needs to come after the property is defined). https://tbpl.mozilla.org/?tree=Try&rev=db513a6851de

So, _this_ one is orange, because it looks like XBL bindings with inheritance don't execute the <destructor> of their super-binding. Indeed, I don't see anywhere in the code that would make that happen. :-( bz, is this a known bug? I don't see one on file...

Not known offhand to me, but XBL destructors are generally so broken I haven't really looked into them much....

This just duplicates the destructor. Let me know if you want me to go back to my old approach pre comment 31 and comment 33.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/43d0932a0b9c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/24c95448c549

https://hg.mozilla.org/mozilla-central/rev/24c95448c549 https://hg.mozilla.org/mozilla-central/rev/43d0932a0b9c

(In reply to Bobby Holley (busy) from comment #4) > Created attachment 722632 [details] [diff] [review] > Part 3 - De-field marquee. v1 This makes the certain marquee properties undefined while the constructor of the binding hasn't been called yet.