I'm prototyping some patches for this, leveraging sandboxes to avoid hand-coding a lot of infrastructure. It might not work out. Let's see.

I assume this is for bindings in content only, not for chrome stuff.

(In reply to Olli Pettay [:smaug] from comment #1) > I assume this is for bindings in content only, not for chrome stuff. yep.

So, I've got an initial sketch done to run content XBL in a separate scope: https://github.com/bholley/mozilla-central/commits/sbxblscope The most immediate snag I've hit has to do with fields. Fields are implemented by calling EvaluateStringWithValue with the bound node as the scope object. So I tried to enter the XBL scope, wrap the bound node for that compartment, and then pass the wrapper as the scope object. When I do, the JS engine "complain[s] bitterly", throwing JSMSG_NON_NATIVE_SCOPE: http://mxr.mozilla.org/mozilla-central/source/js/src/jsinterp.cpp#553 I think Luke is the right person to tell me what to do next.

Is there a list (or can it be easily created) of exactly which XBL bindings are being used in content?

(In reply to Justin Dolske [:Dolske] from comment #4) > Is there a list (or can it be easily created) of exactly which XBL bindings > are being used in content? In Gecko? I'd figured you'd know. ;-) There's also addons, of course. Flashblock runs bindings in content, I think.

So JSMSG_NON_NATIVE_SCOPE is an assertion-turned-dynamic-check (in bug 609990). The assertion itself is pretty old. Now, I know for a fact that we can have proxies on the scope chain: DebugScopeProxy. It looks like the assert comes from bug 566549 which had problems with non-native scope objects in JSOP_DEFVAR. But, surprise surprise, I had to generalize JSOP_DEFVAR for DebugScopeProxy so I think you should just loosen up this assertion: http://hg.mozilla.org/mozilla-central/diff/f45eec2bd4c7/js/src/jsinterpinlines.h#l1.13 to accept any isProxy(), not just isDebugScope() proxies and then remove the JSMSG_NON_NATIVE_SCOPE error (and the following !defineProperty assertion) entirely (from js.msg). Can you replace it with an assertion that every object (on the obj->enclosingScope() chain) is in the right compartment and that the chain terminates with an isGlobal?). You might want to try some of the flash examples that were hitting this (listed in bug 609990) and then try to land the change by itself (I can review) in case there are other issues.

These patches got significantly more complicated because of all the crazy stuff I had to do to XrayWrapper to make the XBL API show up on bound nodes. They pass XBL tests locally. Pushing to try. https://tbpl.mozilla.org/?tree=Try&rev=ae142e64815c

Hit a compartment mismatch as a result of an nsCxPusher saving the frame chain after the JSAutoEnterCompartment. Fixed and pushed again: https://tbpl.mozilla.org/?tree=Try&rev=cedd646b3170

Woohoo! This massive patch stack is green, modulo one b-c failure that I've fixed in a way that I hope is acceptable (I'll leave that up to the reviewers). There are three things left to do: 1 - Remove the XBL bit, which is now unused. 2 - Write thorough tests. 3 - Think about compartment overhead. Number 3 is a big one here, which only really dawned on me recently when smaug pointed it out. These patches cause each content scope to lazily generate an associated sandbox (whose principal is nsExpandedPrincipal(contentPrincipal)) whenever an XBL binding is bound in a given content scope. This isn't a big problem for stuff like <video>, but things like scrollbars are implemented in XBL. This means that we have the potential to create a whole lot of extra compartments in content, which could have a significant memory impact. I chose to create one XBL scope per content scope so that we could do the nsExpandedPrincipal(contentPrincipal) trick, which gives XBL asymmetric security relations with |contentPrincipal|, but (very importantly) nothing else. This maintains the long-understood security invariant that XBL code doesn't run with system principal. There are a few approaches I can think of, here: 1 - Run the XBL as system principal, giving us one and only one XBL scope. This is the most expedient, but runs the risk of security issues. It shouldn't be a huge deal for the most part, since XBL will now be better-insulated from content manipulation. However, XBL still needs to waive Xray vision for field access, so there's a small potential for exploits there. 2 - Audit and whitelist certain common XBL controls (like scrollbar) to run in a common system-principal compartment, and run the rest in individual content compartments. This is a bit more complexity, but might be a good balance. Scrollbar and such aren't that complicated, so we can probably be reasonable sure it's safe. On the other hand, there's also the issue that some addons (like flashblock?) might inject XBL into every content scope. This means that those addons would suddenly have a very large memory regression unless we let them whitelist their stuff somehow, which isn't something we necessarily want to encourage. 3 - Implement zones. I think this would mostly solve things while maintaining our good security invariants, and it sounds like there's already traction behind the idea. However, it's not clear to me how big of a task that is (hoisting XBL is a Q1 DOM goal). I'm also not sure what the residual memory footprint would be, since IIUC we still get an extra shapes arena. Hopefully njn and billm can weigh in here. Some other mitigation strategies: * Have one XBL scope per origin, rather than per content scope. This just involves some extra gross machinery, but isn't hard per se. * Invent a new XBL principal that has special security characteristics. The simplest implementation would be a principal that subsumes everything _but_ the system principal, meaning that an exploit would be a universal XSS but not arbitrary code execution. Curious to hear what people think here.

(1) feels very scary change. Don't know what (3) is about. What zones? But before anything, we need some data. How much does this increase memory usage? Does this affect to page load times?

Ok, so do I get it correctly that in worst case scenario for each content compartment we create a sandbox? (in case of every content compartment is using XBL) Because if that is the case almost all add-on does the same with content-scripts, so if it's a huge performance issue, then I just mention that add-ons on top of that even create a huge pile of sandboxes for the modules they are using. My point is that if this is a performance issue then we should deal with that regardless of this patch being landed. I was pushing it really hard before the CPG patch to make compartments as light-weighted as possible. Note that, CPG created an order of magnitude more extra compartments... That's being said, I'm with smaug: (1) is scary, and I don't know much about these zones either. "principal that subsumes everything _but_ the system principal" I think at some point I will need this anyway for add-ons, it does sound like a useful feature to me.

Zones (or compartment groups) are described in bug 759585, and have been discussed a bit on the recently dev-platform thread about memory overhead. The basic idea is to have certain compartments share arenas, which would significantly decrease the wasted space incurred by having multiple near-empty and related compartments. I think in general I agree with Nick's argument on the dev-platform thread: compartments are an important part of our infrastructure, and we should find ways to make them efficient rather than finding ways to avoid using them. Gabor's argument about jetpack is also a great point - any content script that attaches to '*' is already creating a new compartment for every scope. Hopefully one of the garbagemen can tell us more here. Are compartment groups feasible in the Q1 timeframe? As for measuring the memory footprint: I don't have a ton of experience doing this kind of thing, and I'm concerned that I'd get it wrong. Is there a memory-savvy person who's willing to help me out with the measurements? johns perhaps? Is there a way to do a trial run of AWSY or some other sort of automation with a set of patches applied?

Also - In the interest of speeding things along, I'm going to try to get this landed behind a pref.

(In reply to Bobby Holley (:bholley) from comment #16) > Gabor's argument about jetpack is also a great point - any content script > that attaches to '*' is already creating a new compartment for every scope. But it is not about this bug. Here we're talking about baseline memusage and performance > As for measuring the memory footprint: I don't have a ton of experience > doing this kind of thing, and I'm concerned that I'd get it wrong. Is there > a memory-savvy person who's willing to help me out with the measurements? > johns perhaps? Is there a way to do a trial run of AWSY or some other sort > of automation with a set of patches applied? AWSY runs would be good.

(In reply to Olli Pettay [:smaug] from comment #18) > (In reply to Bobby Holley (:bholley) from comment #16) > > Gabor's argument about jetpack is also a great point - any content script > > that attaches to '*' is already creating a new compartment for every scope. > But it is not about this bug. Here we're talking about baseline memusage and > performance Sure. I'm just saying that if we consider this kind of compartment overhead unacceptable, then we're in effect encouraging an unacceptable configuration for a large number of users (those that use addons developed with our own addon SDK). So given that JSMs, Jetpack, and now XBL are all running into the same overhead issues, it makes sense to try to tackle the problem at the source, rather than fighting the symptoms. :-)

(In reply to Bobby Holley (:bholley) from comment #16) > Is there a way to do a trial run of AWSY or some other sort > of automation with a set of patches applied? I can pretty easily AWSY test any set of patches (or try build with linux64-opt builds).

(In reply to John Schoenick [:johns] from comment #20) > (In reply to Bobby Holley (:bholley) from comment #16) > > Is there a way to do a trial run of AWSY or some other sort > > of automation with a set of patches applied? > > I can pretty easily AWSY test any set of patches (or try build with > linux64-opt builds). (johns and I talked on IRC and he kicked off a run) John, what do the results look like?

(In reply to Bobby Holley (:bholley) from comment #21) > (In reply to John Schoenick [:johns] from comment #20) > > (In reply to Bobby Holley (:bholley) from comment #16) > > > Is there a way to do a trial run of AWSY or some other sort > > > of automation with a set of patches applied? > > > > I can pretty easily AWSY test any set of patches (or try build with > > linux64-opt builds). > > (johns and I talked on IRC and he kicked off a run) > > John, what do the results look like? Sorry, I neglected to check on this before signing off yesterday. I queued this to run ten times: (the ten rightmost builds labeled f2891896079f are the try runs) https://www.areweslimyet.com/?series=bholley Aside from some noise, the majority of the runs show a explicit green line of ~344MiB, which is about the average for the past few weeks, so almost no change. The resident line seemingly regressed by a few megabytes, but looking at the past weeks of data, it doesn't look statistically significant (resident is very noisy)

bholley, how many extra compartments are being created? I imagine it varies with workload, but do you have any numbers, e.g. for popular sites?

(In reply to John Schoenick [:johns] from comment #22) > Aside from some noise, the majority of the runs show a explicit green line > of ~344MiB, which is about the average for the past few weeks, so almost no > change. The resident line seemingly regressed by a few megabytes, but > looking at the past weeks of data, it doesn't look statistically significant > (resident is very noisy) That's really great news! (In reply to Nicholas Nethercote [:njn] from comment #23) > bholley, how many extra compartments are being created? I imagine it varies > with workload, but do you have any numbers, e.g. for popular sites? Well, that's what I was hoping to grok from the AWSY run. We'd be creating an extra compartment for each content scope that contains one of: 1 - a <video> or <audio> control 2 - a scrollbar 3 - a <marquee> element I don't expect 1 or 3 to be very common, but I (and others) had feared that the scrollbar case would be a bigger issue. These numbers are pretty encouraging, though. Smaug, Kyle, what do you think? Are there other experiments we should do?

Do we have tp numbers?

(In reply to Olli Pettay [:smaug] from comment #25) > Do we have tp numbers? Nope. I'll do some runs soon.

about:memory reports the number of compartments under "js-compartment/system" and "js-compartments/user". I just looked for them but they seem to be missing on AWSY... johns, any idea why?

Made various changes to put the machinery behind a pref. Pref disabled: https://tbpl.mozilla.org/?tree=Try&rev=428e75fccab7 Pref enabled: https://tbpl.mozilla.org/?tree=Try&rev=3ded30290fd7

(In reply to Nicholas Nethercote [:njn] from comment #27) > about:memory reports the number of compartments under > "js-compartment/system" and "js-compartments/user". I just looked for them > but they seem to be missing on AWSY... johns, any idea why? AWSY was not handling memory reporters that didn't use "bytes" units :-/ I just pushed a fix for that, but only builds tested going forward will have the data. I can re-queue the last month of tinderbox builds, however.

> I can re-queue the last month of tinderbox builds, however. If it's not hard, sure. But it's not super important.

The TP5 numbers from comment 28 look pretty encouraging: enabled: tp5n_pbytes_paint: avg(499.5, 503.3, 502.4, 502.0, 498.8, 497.6) => 500.6 tp5n_main_rss_paint: avg(158.1, 157.8, 158.5, 157.8, 157.7, 157.6) => 157.9 disabled: tp5n_pbytes_paint: avg(499.5, 498.8, 488.5, 497.8, 497.3, 494.4) => 496.1 tp5n_main_rss_paint: avg(156.8, 157.1, 157.5, 157.5, 158, 157.6) => 157.4 In particular, this suggests that the regression is less than 1%. Combined with John's AWSY analysis, that gives us reasonable confidence that we're not looking at a massive memory hit here. The zone stuff will be a major help and we should get it in as soon as possible (in particular, I've signed on to do the non-SpiderMonkey bits), but IMO it doesn't block this landing. I'm going to put the patches up for bz's review. He said he may or may not get to them before London, and if not we can do it in a room when we get there.

This generally works with membrane semantics, but breaks when same-compartment security wrappers are involved. In particular, when Field{Getter,Setter} live in the XBL scope and operate on NAC via nativeCall, the this object can't be rewrapped, because otherwise a SOW will appear and break everything. It's not ideal to hardcode the index of |this|, but there's not really a great alternative. IIUC the layout isn't changing any time soon, and this code will hopefully be short-lived anyway, since SCSWs are on their way out.

We use XPCNativeWrapper.unwrap rather than .wrappedJSObject so that the tests are agnostic to whether there's an Xray wrapper or not. I converted test_tree_column_reorder.xul into a chrome test because it does all sorts of crazy introspection on the binding, and it really should be a chrome test anyway.

Otherwise the whole method borks when it discovers that the principal doesn't have a URI.

This lets us hook up the binding to the JSClass.

This confused me, because fields are, in fact, exposed via the prototype via the complicated two-step solution defined in InstallField. As it turns out, CompilePrototypeMembers throws if mClassObject ends up null.

Right now, DoInitJSClass only returns non-null if the class object is new (and thus needs to have members installed on it). The code then goes and unconditionally tries to install the members, null-checking and aborting each time. Instead, let's use an explicit boolean and one early return. This lets us get a reference to our JSClass no matter what, which we need.

Let's just pass a JSContext and do Requests/Compartments in the caller.

This is a pure cut/paste except for removing |static| from XBLResolve. XBLResolve will go away soon, so there's no need to put it in a header.

InstallXBLField knows how to handle cross-compartment |callee|. However, The current value will always be wrapped. We need to unwrap said wrappers, otherwise we'll end up with objects that aren't functions.

We make the class object readonly/non-configurable, as well as all the properties and methods on the class object.

Comment on attachment 707109 [details] [diff] [review] Part 2 - Don't rewrap |this| in nativeCall. v1 Review of attachment 707109 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jswrapper.cpp @@ +675,5 @@ > + // Handle |this| separately. We don't want to just rewrap it on the other > + // side of the membrane, because that might apply a same-compartment > + // security wrapper that will stymie this whole process. This logic can > + // go away when same-compartment security wrappers go away. > + Value *thisAddr = src + 1; &srcArgs.thisv()? Or do srcArgs.calleev()/.thisv() separately, and make the loop run over the arguments instead?

Comment on attachment 707111 [details] [diff] [review] Part 4 - Add infrastructure for lazily-created XBL scopes. v1 Review of attachment 707111 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCWrappedNativeScope.cpp @@ +194,5 @@ > + !strcmp(js::GetObjectClass(mGlobalJSObject)->name, "ChromeWindow")); > + > + // Get the scope principal. If it's system, there's no reason to make > + // a separate XBL scope. > + nsIPrincipal* principal = GetPrincipal(); * to the right :/ @@ +214,5 @@ > + options.proto = global; > + > + // Use an nsExpandedPrincipal to create asymmetric security. > + nsCOMPtr<nsIExpandedPrincipal> ep; > + MOZ_ASSERT(!(ep = do_QueryInterface(principal))); Ugh. Does MOZ_ASSERT(nsCOMPtr<nsIExpandedPrincipal>(do_QueryInterface(principal))); work? @@ +221,5 @@ > + ep = new nsExpandedPrincipal(principalAsArray); > + > + // Create the sandbox. > + JSAutoRequest ar(cx); > + js::RootedValue v(cx); Do you need to use RootedValue? @@ +222,5 @@ > + > + // Create the sandbox. > + JSAutoRequest ar(cx); > + js::RootedValue v(cx); > + nsresult rv = xpc_CreateSandboxObject(cx, v.address(), ep, options); One space before = ::: js/xpconnect/src/xpcprivate.h @@ +1739,5 @@ > XPCWrappedNativeScope(JSContext *cx, JSObject* aGlobal); > > nsAutoPtr<JSObject2JSObjectMap> mWaiverWrapperMap; > > + bool IsXBLScope() { return mIsXBLScope; } Five-space indentation?

Comment on attachment 707112 [details] [diff] [review] Part 5 - Check for XBL scopes in nsContentUtils::IsCallerXBL(). v1 Review of attachment 707112 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsContentUtils.cpp @@ +1769,5 @@ > JSScript *script; > JSContext *cx = GetCurrentJSContext(); > + if (!cx) > + return false; > + // New Hotness. Not a terribly helpful comment.

Comment on attachment 707115 [details] [diff] [review] Part 8 - Pass nsXBLBinding instead of nsIContent during implementation installation. v1 Review of attachment 707115 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xbl/src/nsXBLProtoImpl.cpp @@ +53,5 @@ > return NS_OK; // Nothing to do, so let's not waste time. > > // If the way this gets the script context changes, fix > // nsXBLProtoImplAnonymousMethod::Execute > + nsIDocument* document = aBinding->GetBoundElement()->OwnerDoc(); Can GetBoundElement() return null?

Comment on attachment 707126 [details] [diff] [review] Part 17 - Add API for lookup up implementation members on XBL bindings. v1 Review of attachment 707126 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xbl/src/nsXBLBinding.cpp @@ +1425,5 @@ > + > + // Get the string as an nsString before doing anything, so we can make > + // convenient comparisons during our search. > + if (!JSID_IS_STRING(aId)) > + return true; {}. More below.

Comment on attachment 707128 [details] [diff] [review] Part 19 - Dynamically waive Xray for field access by XBL script on bound nodes. v1 Review of attachment 707128 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1291,2 @@ > return false; > } if (!EnsureCompartmentPrivate(wrapper)->scope->IsXBLScope()) return false; nsCOMPtr<nsIContent> content = do_QueryInterfaceNative(cx, wrapper); if (!content) return false; js::RootedId id_(cx, id); return nsContentUtils::IsBindingField(cx, content, id_);

Comment on attachment 707129 [details] [diff] [review] Part 20 - Clone XBL into the XBL scope's compartment. v4 Review of attachment 707129 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xbl/src/nsXBLProtoImplMethod.cpp @@ +312,5 @@ > > JSAutoRequest ar(cx); > + JSAutoCompartment ac(cx, scopeObject); > + if (!JS_WrapObject(cx, &thisObject)) > + return NS_ERROR_OUT_OF_MEMORY; Indentation

To echo Ms2ger, less Rooted please.

Bobby, will this let us run the XBL scripts even if the user has turned off JS? That would be _really_ nice; we have bugs about video controls not working in that situation. Followup is fine if it needs additional work on top of this.

(In reply to Boris Zbarsky (:bz) from comment #61) > Bobby, will this let us run the XBL scripts even if the user has turned off > JS? That would be _really_ nice; we have bugs about video controls not > working in that situation. It gives us that ability, yeah. If we want that behavior, we'll need to make sure it actually happens, and that we don't bail early via some check for disabled scripts. But such work should be trivial, if it exists. > Followup is fine if it needs additional work on top of this. Can you mark those bugs as dependent on this one?

Sure thing. The general tracker for those problems is bug 236839, and there are a bunch of specific bugs about marquee, videocontrols, xml prettyprinter.

This seems like a good bug to land early in Fx22 rather than late in Firefox 21 -- pretty good chance of regressions, yes?

(In reply to Daniel Veditz [:dveditz] from comment #64) > This seems like a good bug to land early in Fx22 rather than late in Firefox > 21 -- pretty good chance of regressions, yes? Yeah. On the flip side, it's our only answer to an otherwise unfixable sg-crit that currently affects all branches. We'd be in a pretty impossible position if we ever had to chemspill over this, so I'm not wild about waiting another 6 weeks. Given that it's preffable, I'm going to land it, and then try flipping the pref on. If it becomes a problem, we can just pref it off on branch. Does that sound ok?

Er, r=me on that one with the addition of a test that runs in the standalone JS shell. It's easy to test, because the bug affects Proxies: js> m={} ({}) js> Object.defineProperty(m, 'p', {value: 3}); ({}) js> "use strict"; delete m.p typein:3:14 TypeError: property "p" is non-configurable and can't be deleted js> x = new Proxy(m, {}) ({}) js> x.p 3 js> "use strict"; delete x.p false

Comment on attachment 707109 [details] [diff] [review] Part 2 - Don't rewrap |this| in nativeCall. v1 Review of attachment 707109 [details] [diff] [review]: ----------------------------------------------------------------- Still thinking about this one. bholley and I have talked a little about this on IRC, but I need a little more. In particular, this exchange: <jorendorff> bholley: I think the problem applies equally to other parameters as to this; consider x.appendChild(y) where both x and y are NAC nodes [...bholley explained this is because appendChild doesn't go through callNative...] <jorendorff> but going back to appendChild() just for a second, when you call the function, we rewrap everything for the content compartment? why does that work? <bholley> jorendorff: so, for regular method access, we actually just do wrappedObject(wrapper) for |this| <bholley> jorendorff: we don't rewrap it <bholley> jorendorff: that's why it works I don't see this in the source code. I would feel more comfortable for this if the hack were: - rewrap for target compartment, as we do now, - but then if the result is a same-compartment security wrapper, unwrap it because then it would clearly only apply in the particular case where we need it, leaving other security-sensitive cases unaffected. Plus the hack will *certainly* go away when same-compartment security wrappers go away.

(In reply to Jason Orendorff [:jorendorff] from comment #67) > I don't see this in the source code. Oh yeah, you're totally right. I misread the code when we were talking a few hours ago. We unwrap the _call_, but not the |this|. So what probably ends up happening, in effect, is that we call the method with a proxy |this|, which means that we end up back in the nativeCall trap, which incidentally explains why this problem is fixed when only fixing the nativeCall situation. > > I would feel more comfortable for this if the hack were: > - rewrap for target compartment, as we do now, > - but then if the result is a same-compartment security wrapper, unwrap it > because then it would clearly only apply in the particular case where we > need it, leaving other security-sensitive cases unaffected. Plus the hack > will *certainly* go away when same-compartment security wrappers go away. Fine by me. Happy to do that.

Comment on attachment 707110 [details] [diff] [review] Part 3 - Make XBL-in-content tests Xray-safe. v2 r=me

Comment on attachment 707111 [details] [diff] [review] Part 4 - Add infrastructure for lazily-created XBL scopes. v1 >+MOZ_ASSERT(!strcmp(js::GetObjectClass(mGlobalJSObject)->name, "Window") || >+ !strcmp(js::GetObjectClass(mGlobalJSObject)->name, "ChromeWindow")); What about "ModalContentWindow"? Or do we no longer end up with those? Check with a showModalDialog? >+ if (nsContentUtils::IsSystemPrincipal(principal)) >+ return global; Does it make sense to set mXBLScope to global here, so we can avoid doing the system-principal check multiple times? >+ options.wantComponents = true; It's odd to set this and wantXHRConstructor to their default values explicitly but not do the same for wantXrays. Why, exactly? r=me with the above dealt with.

Comment on attachment 707112 [details] [diff] [review] Part 5 - Check for XBL scopes in nsContentUtils::IsCallerXBL(). v1 r=me

Comment on attachment 707113 [details] [diff] [review] Part 6 - Clean up security wrappers for NAC. v1 r=me

Comment on attachment 707114 [details] [diff] [review] Part 7 - Let nsExpandedPrincipal through CheckFunctionAccess. v1 r=me. Does this really not address the video-when-script-is-disabled issue?

Comment on attachment 707115 [details] [diff] [review] Part 8 - Pass nsXBLBinding instead of nsIContent during implementation installation. v1 r=me

Comment on attachment 707117 [details] [diff] [review] Part 9 - Remove bogus comment/check and replace with an assert. v1 r=me

Comment on attachment 707118 [details] [diff] [review] Part 10 - Make DoInitJSClass unconditionally fill in aClassObject. v1 Maybe add asserts that aTargetClassObject to the two InstallMember implementation?

Comment on attachment 707119 [details] [diff] [review] Part 11 - Store a strong ref to the JSClass in nsXBLBinding. v1 r=me

Comment on attachment 707120 [details] [diff] [review] Part 12 - Remove unused arguments from InstallMember and simplify calling convention. v1 r=me

Comment on attachment 707121 [details] [diff] [review] Part 13 - Hoist Field machinery into nsXBLProtoImplField. v1 Please add to the checkin comment that the code is just being moved with no changes. r=me

Comment on attachment 707123 [details] [diff] [review] Part 14 - Install XBL field accessors on prototype objects at setup time, and nuke XBLResolve. v2 >+// InstallXBLField below) reify a for the field onto the actual XBL-backed > element. "a property", right? >+ // Don't resolve it if the field is empty; see also InstallField which also s/resolve/install/ ? r=me

Comment on attachment 707124 [details] [diff] [review] Part 15 - Unwrap |callee| before passing it to InstallXBLField. v1 r=me

Comment on attachment 707125 [details] [diff] [review] Part 16 - Make XBL prototype setup tamper-proof for Xrays. v1 Hmm. This is slightly worrisome in terms of regression risk, but r=me

(In reply to Boris Zbarsky (:bz) from comment #71) > What about "ModalContentWindow"? Added it. > > >+ if (nsContentUtils::IsSystemPrincipal(principal)) > >+ return global; > > Does it make sense to set mXBLScope to global here, so we can avoid doing > the system-principal check multiple times? I'd prefer not to. I don't think the overhead is significant considering what else is going on here (it's basically just one virtual function call), and I'm concerned that we'd screw it up with tracing or get confused debugging. I'd rather have that member point either to a sandbox or null, depending on whether we're using a separate XBL scope or not. > >+ options.wantComponents = true; > > It's odd to set this and wantXHRConstructor to their default values > explicitly but not do the same for wantXrays. Why, exactly? I'd original had wantXrays=false (which is non-default) when I was experimenting with this stuff without nsEP. When I implemented nsEP, I removed it, to make sure that we were getting Xrays on account of the asymmetric security and not on account of wantXrays. I'll add options.wantXrays = false here to be explicit.

Comment on attachment 707126 [details] [diff] [review] Part 17 - Add API for lookup up implementation members on XBL bindings. v1 > Bug 821850 - Add API for lookup up implementation members on XBL bindings. v1 Extra "up" in there? >+ if (!mBoundElement && !mBoundElement->GetWrapper()) s/&&/||/ or something? >+ // Find our class object. It's permanent, so it should be there no matter >+ // what. Is that true if the node was adopted? Do we end up creating a new XBLBinding for it in the new global? Otherwise it seems like the class object would be on the old global but the aBoundScope would be the new global... r=me as long as that part is sorted out.

(In reply to Boris Zbarsky (:bz) from comment #74) > Does this really not address the video-when-script-is-disabled issue? Actually, it might!

(In reply to Boris Zbarsky (:bz) from comment #77) > Maybe add asserts that aTargetClassObject to the two InstallMember > implementation? It ends up implicitly asserted a few patches later via: MOZ_ASSERT(js::IsObjectInContextCompartment(aTargetClassObject, aCx));

(In reply to Boris Zbarsky (:bz) from comment #80) > Comment on attachment 707121 [details] [diff] [review] > Part 13 - Hoist Field machinery into nsXBLProtoImplField. v1 > > Please add to the checkin comment that the code is just being moved with no > changes. Already there - whenever I post a series of patches, the comments in the bug are also checkin comments in the patches. :-)

(In reply to Boris Zbarsky (:bz) from comment #85) > Is that true if the node was adopted? Do we end up creating a new > XBLBinding for it in the new global? Otherwise it seems like the class > object would be on the old global but the aBoundScope would be the new > global... Discussed in person. All good.

> Already there No, the comment in the patch doesn't make it quite clear that the only change is things being moved, via pure copy/paste. I _did_ look at the patch comment there... ;)

Comment on attachment 710762 [details] [diff] [review] Part 2 - Don't rewrap |this| in nativeCall. v2 /me narrows his eyes and peers closely at the patch. OK, let's do it.

Comment on attachment 707127 [details] [diff] [review] Part 18 - Expose XBL members via Xray wrappers. v1 >+ if (!aContent->IsInDoc()) Why? Nodes not in a document can have XBL bindings attached, generally... Please remove this check and use OwnerDoc() to get the binding manager. r=me with that.

Comment on attachment 707128 [details] [diff] [review] Part 19 - Dynamically waive Xray for field access by XBL script on bound nodes. v1 >+++ b/content/base/src/nsContentUtils.cpp >+ if (!aContent->IsInDoc()) >+ return false; Again, drop this and get the binding manager from OwnerDoc(). >+ const jschar* chars = JS_GetStringCharsZ(aCx, JSID_TO_STRING(aId)); >+ if (!chars) >+ return false; >+ nsDependentString name(chars); Why not nsDependentJSString? >+++ b/content/xbl/src/nsXBLBinding.cpp >+ if (mPrototypeBinding->FindField(aName)) >+ return true; >+ >+ // Try ancestor bindings. >+ return mNextBinding ? mNextBinding->HasField(aName) : false; Could just do: return mPrototypeBinding->FindField(aName) || (mNextBinding && mNextBinding->HasField(aName)); In either case, I'd prefer boolean and there to using ?:. r=me with that

Comment on attachment 707129 [details] [diff] [review] Part 20 - Clone XBL into the XBL scope's compartment. v4 >+ // FieldGetter and FieldSetter need to run in the XBL scope so that they can >+ // see throw any SOWs on their targets. s/throw/through/? >+ JSAutoCompartment ac (aCx, scopeObject); Nix the space before '(' please. >+ JSAutoCompartment c2(aCx, aTargetClassObject); "ac2"? r=me with those fixed. This stuff is ... finicky. :(

Comment on attachment 707130 [details] [diff] [review] Part 21 - Tests. v1 r=me

Final try push before landing preffed off: https://tbpl.mozilla.org/?tree=Try&rev=e5eb423ab5db

https://hg.mozilla.org/mozilla-central/rev/60c531ee1fbe https://hg.mozilla.org/mozilla-central/rev/7e7bd18931c1 https://hg.mozilla.org/mozilla-central/rev/cc884677ee40 https://hg.mozilla.org/mozilla-central/rev/580a65d9eb1e https://hg.mozilla.org/mozilla-central/rev/f168a07ce06f https://hg.mozilla.org/mozilla-central/rev/2c0fdd436173 https://hg.mozilla.org/mozilla-central/rev/dd78d9d83bdd https://hg.mozilla.org/mozilla-central/rev/34bc18d21cc5 https://hg.mozilla.org/mozilla-central/rev/bbad14489645 https://hg.mozilla.org/mozilla-central/rev/c092442efc7b https://hg.mozilla.org/mozilla-central/rev/a1ddf64db133 https://hg.mozilla.org/mozilla-central/rev/583ed5f34e84 https://hg.mozilla.org/mozilla-central/rev/05df50b4e43e https://hg.mozilla.org/mozilla-central/rev/30c448ff9ea7 https://hg.mozilla.org/mozilla-central/rev/507c15457176 https://hg.mozilla.org/mozilla-central/rev/5264dc88e9c2 https://hg.mozilla.org/mozilla-central/rev/d002170e2ba7 https://hg.mozilla.org/mozilla-central/rev/cd5c0556477c https://hg.mozilla.org/mozilla-central/rev/61fde4343620 https://hg.mozilla.org/mozilla-central/rev/411dcc78008e https://hg.mozilla.org/mozilla-central/rev/b191b0790a66

Bobby, this is crazy awesome! Great work!

To be clear to anybody trying to figure out what is going on here, this landed preffed off. The pref is going to be flipped in bug 834697, so technically the other dependent bugs here should be blocked by that.

If I understand correctly, the plan is to fuzz it for a few days, fix any fallout, then flip the pref.

(In reply to Andrew McCreight [:mccr8] from comment #100) > The pref is going to be flipped in bug 834697, so > technically the other dependent bugs here should be blocked by that. Good point. Moving the deps.