Right now browser code and objects created by addons is not distinguished from other parts of the browser. It would be nice to improve this so that we could associate scripts and objects with addons, by annotating their compartments. This would be nice for several reasons --- e10s addon compatibility issues, cpu / memory profiling, maybe telemetry, maybe crash diagnostics. Note that this is not the same as sandboxing the addon: the addon can still modify the contents of other compartments to its heart's content, though any objects it creates will still be marked as coming from the addon. The attached patch makes this change and seems to do the right thing (correctly labeling both addon and non-addon code) for several addons: adblock, flashblock, and video download helper. Another addon, tree style tabs, mostly works. The main difficulty with this is handling cases where addon code runs in a compartment that is not part of the addon, e.g. event handlers added for some non-addon window. Normally this case is handled by cloning or compiling the code in the non-addon compartment and then running in that one. This patch tries instead to run in an addon compartment with a scope chain that is a wrapper for some object in the non-addon compartment. This approach has several problems. - cx->global() returns the global for the addon compartment rather than the non-addon compartment, which breaks things in various ways --- implicit 'this' computation, binding names for lookups and assignments, some XPConnect native-wrapping stuff. - There are lots of places where addon code can run from and they all need special preparation to make sure the right compartments are available. - Handling this involves lots of wrapping, unwrapping, and tests for wrappers which could potentially fire on wrappers unrelated to addons. It would be good to have a clean notion for when addon code is wrapping a value in a non-addon compartment it would normally be running directly in. This is all hacked around in the patch but there are some cases that are still just plain hard to handle. For example, when an addon does an indirect eval we enter the non-addon compartment and no longer have access to an addon compartment from which to run the eval'ed code. I think a way to improve this situation would be to lazily create addon compartments which are more explicit wrappers for particular non-addon compartments. Instead of having their own global these addon compartments would have a wrapper for the non-addon compartment's global. These compartments can be kept internal to the JS engine and creating and entering them would be done at the JSAPI boundary rather than being scattered through the browser, and the rest of the browser shouldn't have to worry about entering and leaving them.

Skimming this, I really think this is the wrong approach, and that we should be doing something similar to what we do with XBL scopes (Sandbox with sandboxPrototype). In particular, I don't think changes to the JS engine should be necessary at all.

This bug may be relevent to khuey's interests.

(In reply to Bobby Holley (:bholley) from comment #1) > Skimming this, I really think this is the wrong approach, and that we should > be doing something similar to what we do with XBL scopes (Sandbox with > sandboxPrototype). In particular, I don't think changes to the JS engine > should be necessary at all. Well, regardless of the mechanism this seems different from XBL because there are so many ways in which addon code can execute --- pretty much any way in which the browser can execute JS code can also be done as part of an addon. Keeping the JS engine clean would require adding logic to every one of these entry points on the browser side, whereas keeping things internal to the JS engine only requires adding logic to a few pinchpoints where scripts are compiled or cloned. Doing things inside the JS engine should make it easier to handle cases where the addon code enters new compartments as it executes, which we might also want to wrap. This would definitely hold in the indirect eval case, but could be extended to other cases (when an addon calls an existing function in another compartment, should that function be considered to be executing as part of the addon?). The current patch doesn't meet the goal of keeping the compartment logic inside the JS engine but I think with the changes described at the end of comment 0 it could.

Is the problem that we have code like this? http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#4736 I can see why that would cause issues--even if the add-on script is in its own compartment, we still might try to run it in some random scope, and cloning the script into that random scope's compartment isn't what we want to do for this bug. Are there other cases like that? I noticed you also modified JS_CloneFunctionObject in sort of a similar way. Does that have a similar problem?

(In reply to Bill McCloskey (:billm) from comment #4) > Is the problem that we have code like this? > http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#4736 > > I can see why that would cause issues--even if the add-on script is in its > own compartment, we still might try to run it in some random scope, and > cloning the script into that random scope's compartment isn't what we want > to do for this bug. > > Are there other cases like that? I noticed you also modified > JS_CloneFunctionObject in sort of a similar way. Does that have a similar > problem? Yeah, JS_CloneFunctionObject gets used to clone addon scripts into non-addon compartments so they can run in that non-addon compartment's global. Besides this and JS_ExecuteScript, scripts can also be directly compiled in non-addon compartments, though I've only seen this in XBL.

I just looked at this in a bit more detail. I'm sorry about the stop-energy here, but this isn't going to fly. :-( Storing a fuzzy notion of "addon-ness" in stack-scoped TLS might give the right answer often enough to be useful for prototyping and measuring, but just isn't careful and robust enough to be something that we actually land. There are lots of ways in which execution control can transfer from an addon to something else - or that an addon can asynchronously trigger more execution to happen on its behalf. We need to carefully track this information, and be very explicit about when we propagate it. There aren't actually that many places where we compile and execute scripts, so if we track this state appropriately, entering the right scope at those entry points shouldn't end up being all that bad. (In reply to Brian Hackett (:bhackett) from comment #3) > Doing things inside the JS engine should > make it easier to handle cases where the addon code enters new compartments > as it executes, which we might also want to wrap. Do we? I don't think we do, necessarily. And if we do, we need very clear rules about when that happens. Let's back up, and enumerate the types of changes we want to make here. Take <overlay>s, for example. What are the desired semantics? Presumably we want the script to run in a separate scope. What about the content reflectors? Do we want to hoist those into the other scope as well? I'm guessing not, but we should think about it, particular with respect to event handlers and such. I'm happy to help work on the design. Boris (and Blake?) should also probably participate to some degree as well.

I thought we were already doing something here in a postprocessing step.

If I understand what we do for XBL scopes, I think it's pretty similar to what Brian is proposing in comment 0. Whenever we want to run some XBL stuff in some global, we check if that global already has an associated "XBL compartment" and create one if it doesn't. Then we run the XBL code in the associated XBL compartment. I think the only difference is where the code actually lives--in XPConnect or the JS engine. Bobby, how does the XBL code avoid problems like the ones in comment 4? Does it just make sure to pass the right XBL scope into JSAPI every time? Or is there some better pinch point?

(In reply to Bill McCloskey (:billm) from comment #8) > If I understand what we do for XBL scopes, I think it's pretty similar to > what Brian is proposing in comment 0. Whenever we want to run some XBL stuff > in some global, we check if that global already has an associated "XBL > compartment" and create one if it doesn't. Then we run the XBL code in the > associated XBL compartment. > > I think the only difference is where the code actually lives No, my concern is with the way that this patch tracks whether we're associated with an addon or not. --in XPConnect > or the JS engine. Bobby, how does the XBL code avoid problems like the ones > in comment 4? Does it just make sure to pass the right XBL scope into JSAPI > every time? Yes. Though in general, we're very rarely executing script via JSAPI (in XBL or anywhere in Gecko, for that matter). Normally we're just installing methods and accessors and whatnot.

(In reply to Bobby Holley (:bholley) from comment #6) > I just looked at this in a bit more detail. I'm sorry about the stop-energy > here, but this isn't going to fly. :-( > > Storing a fuzzy notion of "addon-ness" in stack-scoped TLS might give the > right answer often enough to be useful for prototyping and measuring, but > just isn't careful and robust enough to be something that we actually land. > There are lots of ways in which execution control can transfer from an addon > to something else - or that an addon can asynchronously trigger more > execution to happen on its behalf. > > We need to carefully track this information, and be very explicit about when > we propagate it. There aren't actually that many places where we compile and > execute scripts, so if we track this state appropriately, entering the right > scope at those entry points shouldn't end up being all that bad. Yeah, as discussed on IRC (and which I should have mentioned in comment 0) the JSAutoEnterAddon stuff is a hack and will need to be replaced with explicit tracking of bits related to addons via stack and heap state. The patch already does this to some degree, e.g. with nsJARChannel.

Remove the TLS scoping stuff, JSAutoEnterAddon and JS_GetActiveAddonId(). Instead of passing this state around implicitly it is stored in heap structures, stack arguments etc. I've only tested this with the tree style tabs addon (the most complicated one) which is still just mostly working. I think the approach in this patch needs further revision. The main way in which addon-ness is passed around is through URIs. If we decide a URI is part of an addon then a new nsAddonURI instance is created wrapping that URI and associating it with the addon. The URI is passed around and eventually when compartments are created based off it the compartment is tagged with that addon. This has a couple problems: - There are lots of places URIs are created and many may need to account for whether they are creating an addon URI. - When creating a URI it may not be immediately apparent that the URI is associated with an addon. If part of the addon includes some other resource (a script, XML file, CSS file etc.) that will just be a bare string ('chrome://treestyletabs/whatever.css') and we'd like to distinguish resources that are other parts of the addon from ones that are just generic parts of the browser and happen to be loaded by the addon. When resolving URIs we can tell whether a given resource:// (and chrome:// I think) URI is associated with an addon and I think this mechanism can be used to fix the above problems. Instead of worrying about whether a URI is associated with an addon when creating the URI, we can create, modify and pass around the URIs as normal and wait until we need to know the URI's addon (because of a compartment being created, or whatever) to resolve the URI to that addon, which will mainly be a hashtable lookup on the string following the resource:// or chrome:// in the URI.

(In reply to Brian Hackett (:bhackett) from comment #11) > When resolving URIs we can tell whether a given resource:// (and chrome:// I > think) URI is associated with an addon and I think this mechanism can be > used to fix the above problems. Instead of worrying about whether a URI is > associated with an addon when creating the URI, we can create, modify and > pass around the URIs as normal and wait until we need to know the URI's > addon (because of a compartment being created, or whatever) to resolve the > URI to that addon, which will mainly be a hashtable lookup on the string > following the resource:// or chrome:// in the URI. Yeah, this sounds like a better way to go. We can add a GetAddonId to nsIURI (or the appropriate subclass), which generally returns null, and does a dynamic lookup for the kinds of URIs that might be addon-related. We can implement some kind of caching+flushing mechanism if the perf turns out to be a problem, though it probably won't. This is probably more maintainable in the long run, since we don't have to update the wrapper to forward new nsIURI functionality as it's added.

This patch computes URIs as normal and figures out what addon they are associated with as needed, according to the registration of chrome:// and resource:// domains when the addon was loaded. With tree style tabs this is able to correctly distinguish which scripts are part of the addon, except for a few scripts created via indirect eval on a content global. Assuming this looks good the next step is to fix the compartment wrapping mechanism to more closely match that used in XBL, rather than use the rather nasty and invasive engine hacks present in this patch. That can wait for bug 993438 though I think.

Comment on attachment 8403661 [details] [diff] [review] WIP v3 Review of attachment 8403661 [details] [diff] [review]: ----------------------------------------------------------------- A cursory looks over the URI stuff looks good, though I don't know that code all that well. ::: content/xul/document/src/nsXULPrototypeDocument.cpp @@ +755,1 @@ > mJSObject = JS_NewGlobalObject(cx, &gSharedGlobalClass, Note that in bug 993772, I'm merging all of the jillion compilation globals in Gecko into one compilation global. So we'll want to track this at the pinch-point where we clone scripts out of the compilation scope and run them in a given Window. This happens in XULDocument::ExecuteScript, where we'll want to pull the addon id out of the URI on the nsXULPrototypeScript, and use that to pick the execution scope. My patches (which should land in a day or two) are here: https://github.com/bholley/gecko-dev/commits/compilationglobal ::: dom/xbl/nsXBLDocumentInfo.cpp @@ +176,1 @@ > mJSObject = JS_NewGlobalObject(cx, &gSharedGlobalClass, Same thing here. We'll want to do it in all the various places where we currently select between the XBL scope and the global. I'm totally down to have an xpc::GetScopeForXBLScript(cx, global, addonId) that internally forwards to either xpc::GetXBLScope or xpc::GetAddonScope or identity, as appropriate - so long as we leave the underlying mechanisms separate, and only create addon scopes in the System Principal case. Note that I'm also rejiggering a lot of the XBL code significantly in bug 990290, which should land within the hour. ::: js/xpconnect/loader/mozJSComponentLoader.cpp @@ +655,5 @@ > > CompartmentOptions options; > options.setZone(SystemZone) > + .setVersion(JSVERSION_LATEST) > + .setAddonId(GetAddonIdForURI(aURI)); we should probably turn this off in the aReuseLoaderGlobal case, just in case.

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Hey Brian, just wanted to see if you're still working on this.

Hey, I was waiting on bug 993772 to land before doing more work. Since that's landed now I'll start back up on this later this week (I've been working on the irregexp port and want to get that into a semi-working state before changing gears).

This patch seems to do a pretty good job on the addons mentioned in comment 0. Using the various addons we are able to associate running scripts with their addon correctly except in a couple cases --- the indirect evals performed by tree-style-tabs, and content XBL stuff done by flashblock. This uses the strategy outlined earlier in the bug of creating addon sandbox compartments in basically the same way as is done for XBL sandboxes. I had to extend this a bit though --- using the sandboxes as is was causing most of the tested addons to break pretty much entirely, as they were still able to observe the presence of the sandbox. If they did 'var foo = whatever' in the sandbox's scope and later did a window.foo they wouldn't see the foo property because it had been defined on the sandbox rather than the window. The patch works around this by setting a bit on the sandbox/compartment so that top level var defines and sets jump to the sandbox's prototype (a wrapper for the target global). This is sufficient to fix the addons I tested, though maybe we should have special handling for implicit |this| construction which would fix all the typical ways that code running in the sandbox can modify or get a handle on the sandbox itself (there would still be harebrained ways I'm sure but whatever).

Thanks very much Brian. This is tons of fun to play around with. One thought I have is that this will have to be behind a pref. I'll be away the next two days, so I won't be able to get to this until next week. The code looks pretty clean. I do wonder if there's a nicer way to do the writeToGlobalPrototype stuff. Would it work to have a class hook on the addon global that would forward to the proto? That seems like a more localized change than having this extra flag on all compartments. Also, the logic of GetXBLScope seems a little odd to me. It seems like we should allow compartments to be both XBL scopes and addon scopes simultaneously. Would that make Flashblock work?

(In reply to Bill McCloskey (:billm) from comment #20) > I'll be away the next two days, so I won't be able to get to this until next > week. The code looks pretty clean. I do wonder if there's a nicer way to do > the writeToGlobalPrototype stuff. Would it work to have a class hook on the > addon global that would forward to the proto? That seems like a more > localized change than having this extra flag on all compartments. Yeah, it would, though I think the semantics should stay similarly well constrained and not e.g. be applicable to the generic define property paths or to non-global objects. > Also, the logic of GetXBLScope seems a little odd to me. It seems like we > should allow compartments to be both XBL scopes and addon scopes > simultaneously. Would that make Flashblock work? Earlier patches had scopes that were both an XBL and addon scope but bholley was pretty opposed to this due to the extra complexity this would entail (which doesn't seem that bad to me, but I don't know the associated code at all). I feel like we should be handling and not punting on these sorts of oddball cases (also including indirect eval).

Yeah, that was my call - Let's talk about it on Monday Bill?

I am actively reviewing this. The XBL stuff has quite a lot of implications that I need to think about and research more.

After some thought, I've realized that I hadn't fully thought through the implications of separating addon XBL that gets applied to XUL documents. If we try to do that, we're going to have to jump the shark on most of the stuff I was worried about in comment 22. If we do go that route, then I think you guys are right that it makes more sense to use a tuple in the XPConnect machinery. However, getting that right is going to be a lot of work, and involve much more heavy surgery on dom/xbl than this patch performs (this all dawned on me when I was reviewing that part). Given that, I think it would be wise to try an incremental approach - land this

(In reply to Bobby Holley (:bholley) from comment #24) > Given that, I think it would be wise to try an incremental approach - land > this (Err, accidentally hit submit) I was going to say - ...land this (possibly even preffed off) with addon-provided XBL running straight in the target's compartment, and file a followup bug to hoist the XBL. Thoughts?

(In reply to Bobby Holley (:bholley) from comment #24) > If we do go that route, then I think you guys are right that it makes more > sense to use a tuple in the XPConnect machinery. (Actually, I don't think this is the case. For a chrome scopes, we'd have sandbox-per-addon, where each sandbox contains both xbl and overlay code). For content scopes, we'd have sandbox-per-addon (where each sandbox contains only xbl code), plus one extra sandbox for system XBL injected into content. So in the end, the representation ends up looking more or less the way it does in the current patch.)

Well, I'm not really sure what you're proposing here, but most addons I've looked at do use XBL to some degree so I don't think there would be much value in landing this without isolation for addon provided XBL that runs against chrome compartments (those where we don't create sandboxes currently). I'd like to do the tuple thing (or anything that allows us to isolate addon XBL running against content) and am fine with landing everything pref'ed off, and with the pref off there shouldn't be any change in the browser's semantics so couldn't we just land with the tuple thing even if it's only correct to a first approximation? i.e. tested addons don't seem to break with the pref on. What are these implications you're worried about though? This patch *will* change the visible behavior of addons, and not just XBL ones, just hopefully not in ways that cause them to break. It's all kind of squishy.

Can you be more specific about your XBL concerns, Bobby? It would really be much better to support XBL applied to chrome, even if it's incomplete support. Either way, I think it makes sense to land it preffed off. Maybe we could send out a message to dev-platform asking people to try it and watch for addon breakage. If that goes smoothly for a few days, we can enable it on nightly.

While trying this out, I ran into a case with the Video Download Helper add-on where we weren't loading some code in an add-on compartment. It turns out that components that are registered through chrome.manifest (using component/contract directives) get loaded as normal file:// URIs. The URI -> addon ID mapping code doesn't have any support for those. I hacked around it a little, but then I ran into this code: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3704 I'm guessing that Kyle was referring to this code in comment 7. It's used by about:memory to map compartments to add-on IDs. One nice thing that it does is to resolve all URIs down to file:// URIs and then check if it's from an add-on based on the path alone. That wa I think it probably makes sense to use this code. My only concern is that it might not be fast enough. This patch does two things: 1. Makes it easier to call the addon manager code from C++ and avoids calling into the addon manager for URIs that clearly aren't addons. 2. Uses a trie data structure to keep track of addon paths, which should be faster than the linear scan that we do now.

Here's Brian's patch but without the addon ID mapping stuff. I'll leave the original one alone in case Bobby has review comments that apply to it.

(In reply to Bill McCloskey (:billm) from comment #28) > Can you be more specific about your XBL concerns, Bobby? Supporting XBL in this patch has far-reaching correctness implications that neither you, Brian, nor I fully understand yet. I had been hoping to avoid these issues (hence my insistence on not intermingling XBL scopes and addon scopes), but I realized in comment 24 that we're going to have to address them if we want to hoist XBL-in-XUL (which I think we want). The final patch may end up looking somewhat similar to what we have now. But it's going to take me longer to review, we're going to have to discuss various issues, and I'm probably going to to insist on some tests for the corner-cases that will feel like a distraction in the context of this patch. > It would really be > much better to support XBL applied to chrome I agree that we should support it. I just think it makes more sense to do as a separate followup patch. This patch already does a lot of stuff (the JS engine hackery, the component registry stuff, and the overlay hoisting), which I think would be better to land on its own. That'll let you start working on interpositions in parallel with the XBL stuff. > even if it's incomplete support. I'm not worried about it being incomplete. I'm worried that it will crash in corner cases. > Either way, I think it makes sense to land it preffed off. Agreed. (In reply to Brian Hackett (:bhackett) from comment #27) > I don't think there would be much > value in landing this without isolation for addon provided XBL that runs > against chrome compartments (those where we don't create sandboxes > currently). I think there's a lot of value from the perspective of compartmentalizing risk and keeping the patch manageable. Is there a disadvantage? > I'd like to do the tuple thing The tuple thing won't work. See comment 26. > (or anything that allows us to > isolate addon XBL running against content) and am fine with landing > everything pref'ed off, and with the pref off there shouldn't be any change > in the browser's semantics so couldn't we just land with the tuple thing > even if it's only correct to a first approximation? i.e. tested addons > don't seem to break with the pref on. > What are these implications you're worried about though? Mostly issues stemming around there being N XBL scopes for a given global. I don't want to get too much into it in this bug, but the issues include anonymous content reflectors (which get hoisted into "the" XBL scope), Xrays (which expect to find <implementation> members for bound elements in "the" XBL scope), Aggregated QI (which makes use of the Xray functionality above to support <implements>), and the WeakMaps we use to store XBL interface objects (which live in "the" XBL scope, and expect things which live in the XBL scope to be same-compartment with each other). Some of these may turn out to be non-issues, but we need to consider them carefully, and I'd rather not block the unrelated parts of this patch on that. Does this sound ok? If so, I'll finish off my review comments for the non-XBL parts and we can split the XBL stuff into a separate bug where I'll comment in more detail.

Brian asked me to take over the bug. I guess I'm okay splitting off the XBL stuff. Do you want to talk about it tomorrow, Bobby?

(In reply to Bill McCloskey (:billm) from comment #32) > Brian asked me to take over the bug. I guess I'm okay splitting off the XBL > stuff. Ok. > Do you want to talk about it tomorrow, Bobby? Tomorrow's not great. Here's an order of events that I'd suggest: * I'll finish up the review sans XBL right now. * You address that feedback, get the necessary reviews from jorendorff (or similar, but I think he should at least know about it) and bsmedberg, and get this landed. * You file a followup bug on the XBL stuff, and needinfo me to do a preliminary audit and come up with a proposed plan, which I will do by the end of this week at the latest. * We talk about the plan on Monday (or sooner if it happens sooner). Sound good?

Comment on attachment 8414160 [details] [diff] [review] patch Review of attachment 8414160 [details] [diff] [review]: ----------------------------------------------------------------- This review covers the DOM and XPConnect bits. The registry stuff and the SM stuff still need review. I have some concerns about the js-engine hackery, which we discussed various solutions to. Please run the final scheme for that stuff past jorendorff, just so he's aware that it's happening. It's also worth running this briefly by bz, who is intimately familiar with the hacked up prototype scheme that the spec uses for Window, and might have some thoughts on this. This stuff all needs to go behind a pref. This patch doesn't properly handle the case where an overlay does "window.setTimeout('some script')". Do we care? Per previous discussion, all the stuff on dom/xbl should be dropped. ::: js/xpconnect/src/Sandbox.cpp @@ +1089,5 @@ > .setTrace(TraceXPCGlobal); > > + // Try to figure out any addon this sandbox should be associated with. > + // The addon could have been passed in directly, as part of the metadata, > + // or by being constructed from an addon's code. Why is this on the metadata? It should be a SandboxOption. Hooking it up as such also means that all of the nasty JSAPI stuff is done automatically. @@ +1106,5 @@ > + NS_ENSURE_TRUE(options.addonId, NS_ERROR_FAILURE); > + } > + } > + } > + if (JSObject *obj = JS::GetScriptedCallerGlobal(cx)) { Hm. Why do we need to use the caller global here? Can't we just use the global of the compartment? Or is the issue that addon code sometimes ends up using the Cu from the XUL document's global? If that's the reason, please at least use GetIncumbentGlobal() here, which handles various edge cases that GetScriptedCallerGlobal doesn't. ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +526,5 @@ > + JSCompartment *compartment = js::GetObjectCompartment(obj); > + CompartmentPrivate *priv = GetCompartmentPrivate(compartment); > + if (!priv || !priv->scope) > + return false; > + return priv->scope->IsAddonScope(); This can all boil down to: GetObjectScope(obj)->IsAddonScope(); @@ +618,5 @@ > MOZ_ASSERT(aObj); > MOZ_ASSERT(!js::IsWrapper(aObj)); > > + if (IsSandbox(aObj)) > + aObj = js::GetPrototypeNonProxy(aObj); This is a pretty big behavior change. Can you elaborate on the breakage we get without this? It's possible that we may want to go this route, but so far we haven't needed to (and have done localized fixes, like that in nsGlobalWindow::CallerInnerWindow). If we _do_ do this, it should be a separate patch, and I'll need to do an audit. ::: js/xpconnect/src/XPCWrappedNativeScope.cpp @@ +258,5 @@ > + scope = nativeScope->EnsureXBLScope(cx); > + else if (addonId) > + scope = nativeScope->EnsureAddonScope(cx, addonId); > + else > + scope = JS_GetGlobalForObject(cx, contentScope); All of the changes here can go away for now. @@ +313,5 @@ > + return &v.toObject(); > +} > + > +JSObject * > +xpc::GetAddonScope(JSContext *cx, JSObject *contentScopeArg, JSAddonId *addonId) Can you add an assertion to this and to GetXBLScope that we're not already that kind of scope? ::: js/xpconnect/src/xpcprivate.h @@ +1154,5 @@ > // This reference is wrapped into the compartment of mGlobalJSObject. > JS::ObjectPtr mXBLScope; > > + // Lazily created sandboxes for addon code. > + AutoInfallibleTArray<JS::ObjectPtr, 0> mAddonScopes; Isn't this just equivalent to: nsTArray<JS::ObjectPtr> mAddonScopes? ::: js/xpconnect/src/xpcpublic.h @@ +92,3 @@ > > inline JSObject * > +GetXBLScopeOrGlobal(JSContext *cx, JSObject *obj, JSAddonId *addonId = nullptr) { These changes can go away for now too. @@ +115,5 @@ > +GetAddonScope(JSContext *cx, JSObject *contentScope, JSAddonId *addonId); > + > +inline JSObject * > +GetAddonScopeOrGlobal(JSContext *cx, JSObject *obj, JSAddonId *addonId) { > + if (!addonId || IsInAddonScope(obj)) I think that thing most consistent with the present infrastructure would be to only check IsAddonScope() here, and and check for a null addonId in xpc::GetAddonScope (which is kinda the analog of the UseXBLScope() check).

(In reply to Bobby Holley (:bholley) from comment #34) > ::: js/xpconnect/src/Sandbox.cpp > @@ +1089,5 @@ > > .setTrace(TraceXPCGlobal); > > > > + // Try to figure out any addon this sandbox should be associated with. > > + // The addon could have been passed in directly, as part of the metadata, > > + // or by being constructed from an addon's code. > > Why is this on the metadata? It should be a SandboxOption. Hooking it up as > such also means that all of the nasty JSAPI stuff is done automatically. The addon ID is already being passed in by existing JS via the sandbox metadata, see source/addon-sdk/source/lib/toolkit/loader.js > @@ +1106,5 @@ > > + NS_ENSURE_TRUE(options.addonId, NS_ERROR_FAILURE); > > + } > > + } > > + } > > + if (JSObject *obj = JS::GetScriptedCallerGlobal(cx)) { > > Hm. Why do we need to use the caller global here? Can't we just use the > global of the compartment? Or is the issue that addon code sometimes ends up > using the Cu from the XUL document's global? > > If that's the reason, please at least use GetIncumbentGlobal() here, which > handles various edge cases that GetScriptedCallerGlobal doesn't. Using the addon ID for the current compartment would probably work here, though I don't remember if anything has switched compartments by this point in the sandbox creation. fwiw, adblock does create its own sandboxes. > @@ +618,5 @@ > > MOZ_ASSERT(aObj); > > MOZ_ASSERT(!js::IsWrapper(aObj)); > > > > + if (IsSandbox(aObj)) > > + aObj = js::GetPrototypeNonProxy(aObj); > > This is a pretty big behavior change. Can you elaborate on the breakage we > get without this? It's possible that we may want to go this route, but so > far we haven't needed to (and have done localized fixes, like that in > nsGlobalWindow::CallerInnerWindow). If we _do_ do this, it should be a > separate patch, and I'll need to do an audit. I don't remember what breaks without this, it's been a while. Though it is nice to distinguish as little as possible between the addon scopes and the compartment they are sandboxed for. Bill, you might want to just remove this and see what happens.

The purpose of this patch is to make add-on ID mapping faster and also more easily accessible from C++. In JS, the patch uses a trie rather than an array to map paths to add-on IDs. With lots of add-ons, I think this could make lookups quite a bit faster. In C++, the patch adds a helper function to do the mapping. The helper function does some quick checks to see if the path is obviously not add on-related and then it calls into the add-on manager. I'm not sure who should review this. If it looks okay to you guys, I'll add a JS person to check over the few JS parts.

Brian, I have a couple more questions about the patch. First, do you think it would be possible to use a resolve hook on the sandbox global instead of the writeToGlobalPrototype flag? I looked at all the other hooks, and it seems like this one is the only one that has a chance to work. As far as I can tell, it's called in all the places where your patch hooks in. Second, do you remember the reasons for the two changes to jsobj.cpp? I can't figure out what's going on there. Thanks again.

See comment 37.

(In reply to Bill McCloskey (:billm) from comment #37) > Brian, I have a couple more questions about the patch. First, do you think > it would be possible to use a resolve hook on the sandbox global instead of > the writeToGlobalPrototype flag? I looked at all the other hooks, and it > seems like this one is the only one that has a chance to work. As far as I > can tell, it's called in all the places where your patch hooks in. What would this hook do? Normally we use resolve hooks for installing lazy properties on an object, like the various builtin stuff on a global object. The problem with this bug though is that we want to catch cases where the addon tries to install a new property on the window via a name style access on the sandbox global. For the resolve hook to help with that, it would I think need to install the property on the window, that property would need a custom setter or something that allowed it to be changed directly by assignments on the sandbox global (otherwise the property set would still just write to the global itself) and there would need to be some way to distinguish writes from reads in the resolve hook (which I don't think there is) so that properties aren't installed on the window which shadow ones in its own prototype. > Second, do you remember the reasons for the two changes to jsobj.cpp? I > can't figure out what's going on there. The first change is just an existing bug. On these property descriptors and such a null property op is equivalent to JS_PropertyStub and so forth but we don't really canonicalize these anywhere which makes comparison/testing error prone. The second change is something I ran into at some point, JS_CopyPropertyFrom crashes if you try to copy a non-existing property from one object to another. It would be better I think if it was just a no-op in this case.

(In reply to Brian Hackett (:bhackett) from comment #39) > What would this hook do? Normally we use resolve hooks for installing lazy > properties on an object, like the various builtin stuff on a global object. > The problem with this bug though is that we want to catch cases where the > addon tries to install a new property on the window via a name style access > on the sandbox global. For the resolve hook to help with that, it would I > think need to install the property on the window, that property would need a > custom setter or something that allowed it to be changed directly by > assignments on the sandbox global (otherwise the property set would still > just write to the global itself) and there would need to be some way to > distinguish writes from reads in the resolve hook (which I don't think there > is) so that properties aren't installed on the window which shadow ones in > its own prototype. Yes, you're right. I had thought that we could have the resolve hook return a property on the prototype, which NEW_RESOLVE hooks are allowed to do. However, it turns out that JSOP_DEFVAR completely ignores that and always installs the property on the global. But I wonder if we could use the addProperty hook to do something like what you're proposing. Whenever the sandbox class's addprop hook is called, it would change the setter for the property to something custom. It would also copy the property to the prototype. The setter for the property would also copy the changed value over to the prototype. That way the sandbox global and the real global would both have copies of all the properties set within the sandbox and they would stay in sync with each other.

Comment on attachment 8424237 [details] [diff] [review] addon-id-mapping v2 I think Irving might be the best reviewer here. Things I'm worried about: 1) will this cause us to load the full addon manager at each startup? That would be bad. 2) Is this intended only for file paths or also for chrome:// paths mapped by addons? I can't tell from the basic docs. >diff --git a/toolkit/mozapps/extensions/AddonManagerUtils.cpp b/toolkit/mozapps/extensions/AddonManagerUtils.cpp >+void >+mozilla::StartMappingURIsToAddons() typically you'd start this .cpp file with `using namespace mozilla` or include these in a `namespace mozilla {` block rather than fully-qualifying them in the defn. >+mozilla::MapURIToAddonID(nsIURI* aURI) Why is this short-cutting on gAddonManagerStarted? Is that a performance optimization and we'll pick up things later if/when the addon manager is actually started? >+ static NS_NAMED_LITERAL_CSTRING(kGRE, "resource://gre/"); >+ static NS_NAMED_LITERAL_CSTRING(kToolkit, "chrome://global/"); >+ static NS_NAMED_LITERAL_CSTRING(kBrowser, "chrome://browser/"); This bakes in some odd assumptions: 1) That chrome://global and chrome://browser don't have any overrides 2) That chrome://browser is special (this is not Firefox-specific code). Perhaps that's ok in this case? 3) It seems to be missing resource:/// resource://app/ which I'd expect to be important shortcuts I don't quite understand the impl either: is this only intended to work for restartless addons? If so that should be documented. >diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp >@@ -805,16 +806,18 @@ nsXREDirProvider::DoStartup() > // Init the Extension Manager > nsCOMPtr<nsIObserver> em = do_GetService("@mozilla.org/addons/integration;1"); > if (em) { > em->Observe(nullptr, "addons-startup", nullptr); > } else { > NS_WARNING("Failed to create Addons Manager."); > } > >+ mozilla::StartMappingURIsToAddons(); I'm pretty sure this codepath is not used by xpcshell tests. Are you ok with that? Should probably be documented.

Comment on attachment 8424237 [details] [diff] [review] addon-id-mapping v2 Thanks. It looks like I'll have to work on this some more. (In reply to Benjamin Smedberg [:bsmedberg] from comment #41) > Things I'm worried about: > 1) will this cause us to load the full addon manager at each startup? That > would be bad. I'll need to investigate this. > 2) Is this intended only for file paths or also for chrome:// paths mapped > by addons? I can't tell from the basic docs. It's meant to handle all URIs. However, the add-on manager already has code to canonicalize all sorts of URIs into file URIs. That happens before we get to the trie code, so that code only has to deal with files. > >diff --git a/toolkit/mozapps/extensions/AddonManagerUtils.cpp b/toolkit/mozapps/extensions/AddonManagerUtils.cpp > > Why is this short-cutting on gAddonManagerStarted? Is that a performance > optimization and we'll pick up things later if/when the addon manager is > actually started? I ran into some problems because the code was called too soon and we tried to load the add-on manager before we're allowed to do that. > >+ static NS_NAMED_LITERAL_CSTRING(kGRE, "resource://gre/"); > >+ static NS_NAMED_LITERAL_CSTRING(kToolkit, "chrome://global/"); > >+ static NS_NAMED_LITERAL_CSTRING(kBrowser, "chrome://browser/"); > > This bakes in some odd assumptions: > 1) That chrome://global and chrome://browser don't have any overrides > 2) That chrome://browser is special (this is not Firefox-specific code). > Perhaps that's ok in this case? > 3) It seems to be missing resource:/// resource://app/ which I'd expect to > be important shortcuts I wasn't aware of overrides. I guess I'll have to do more work for that. It's not necessary that the code catch all non-add on URIs. I just want it to handle the most common cases. But it shouldn't miss add-ons. > I don't quite understand the impl either: is this only intended to work for > restartless addons? If so that should be documented. No, it's supposed to work for everything. > >diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp > > >@@ -805,16 +806,18 @@ nsXREDirProvider::DoStartup() > > // Init the Extension Manager > > nsCOMPtr<nsIObserver> em = do_GetService("@mozilla.org/addons/integration;1"); > > if (em) { > > em->Observe(nullptr, "addons-startup", nullptr); > > } else { > > NS_WARNING("Failed to create Addons Manager."); > > } > > > >+ mozilla::StartMappingURIsToAddons(); > > I'm pretty sure this codepath is not used by xpcshell tests. Are you ok with > that? Should probably be documented. That's a problem. I'll try to fix it.

(In reply to Bill McCloskey (:billm) from comment #42) > Comment on attachment 8424237 [details] [diff] [review] > addon-id-mapping v2 > > Thanks. It looks like I'll have to work on this some more. > > (In reply to Benjamin Smedberg [:bsmedberg] from comment #41) > > Things I'm worried about: > > 1) will this cause us to load the full addon manager at each startup? That > > would be bad. > > I'll need to investigate this. It shouldn't, but definitely worth checking. > > 2) Is this intended only for file paths or also for chrome:// paths mapped > > by addons? I can't tell from the basic docs. > > It's meant to handle all URIs. However, the add-on manager already has code > to canonicalize all sorts of URIs into file URIs. That happens before we get > to the trie code, so that code only has to deal with files. Mossop recently did some work on the file mapping code; he's probably the best reviewer unless he's too busy. > > >diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp > > > > >@@ -805,16 +806,18 @@ nsXREDirProvider::DoStartup() > > > // Init the Extension Manager > > > nsCOMPtr<nsIObserver> em = do_GetService("@mozilla.org/addons/integration;1"); > > > if (em) { > > > em->Observe(nullptr, "addons-startup", nullptr); > > > } else { > > > NS_WARNING("Failed to create Addons Manager."); > > > } > > > > > >+ mozilla::StartMappingURIsToAddons(); > > > > I'm pretty sure this codepath is not used by xpcshell tests. Are you ok with > > that? Should probably be documented. > > That's a problem. I'll try to fix it. Most of our tests just load AddonManager.jsm directly, because they don't care about the XPCOM glue. This approach is OK with me if you're specifically testing APIs that the addon manager is exposing through XPCOM.

(In reply to Bill McCloskey (:billm) from comment #40) > Yes, you're right. I had thought that we could have the resolve hook return > a property on the prototype, which NEW_RESOLVE hooks are allowed to do. > However, it turns out that JSOP_DEFVAR completely ignores that and always > installs the property on the global. > > But I wonder if we could use the addProperty hook to do something like what > you're proposing. Whenever the sandbox class's addprop hook is called, it > would change the setter for the property to something custom. It would also > copy the property to the prototype. The setter for the property would also > copy the changed value over to the prototype. That way the sandbox global > and the real global would both have copies of all the properties set within > the sandbox and they would stay in sync with each other. This still doesn't seem ideal to me, for a few reasons. The sandbox's addon hook would not just be adding properties to the window with this setter, but modifying existing plain data properties to add the setter, which seems weird. And having properties on the window with this setter means that property writes in non-addon code will run slower and not be optimizable by the JITs.

I decided to split the add-on ID mapping out into a separate bug (bug 1017302). This patch implements the code for writeToGlobalPrototype. It uses the addProperty hook approach. I tried using a resolve hook to do this, but I just couldn't figure out a way to make it work without a lot of changes to the JS engine. If we eventually decide to remove the addProperty hook, I'll volunteer to remove this use. Hopefully, by then, we'll have some more proxy-like hooks that we can use instead. This patch also adds an add-on ID field for compartments. I probably could have split that apart, but it sort of fits with this patch and it's simple. I'll follow up with some xpcshell tests for this writeToGlobalPrototype feature in the next few days.

This patch changes the rest of the browser to pass the add-on ID argument into the sandbox using the normal options rather than the metadata. I'm guessing this is what you want, Bobby, based on your earlier review.

This patch introduces GetAddonScopeOrGlobal and uses it in a few places. It's disabled by a pref (dom.compartment_per_addon). This patch also changes mozJSComponentLoader to set the add-on ID correctly in the CompartmentOptions. That happens regardless of the pref, but I don't think there's really any danger in this part.

Forgot one small change to this patch.

Comment on attachment 8430414 [details] [diff] [review] change-addon-id-passing Review of attachment 8430414 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/Sandbox.cpp @@ +1979,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(IsSandbox(sandbox)); > + > + JSAddonId *id = JS_GetCompartmentAddonId(sandbox); If this is fallible, we need a null-check. If not, this should just be called JS::CompartmentAddonId or something. @@ +1980,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(IsSandbox(sandbox)); > + > + JSAddonId *id = JS_GetCompartmentAddonId(sandbox); > + JS::RootedValue idStr(cx, StringValue(JS_GetAddonIdString(id))); Something similar here.

Comment on attachment 8430416 [details] [diff] [review] mirroring v2 Review of attachment 8430416 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with comments addressed. ::: js/src/jsapi.h @@ +1673,5 @@ > extern JS_PUBLIC_API(JSString *) > JS_GetAddonIdString(JSAddonId *id); > > +extern JS_PUBLIC_API(JSAddonId *) > +JS_GetCompartmentAddonId(JSObject *obj); I think this should be JS::ObjectAddonId(). Alternatively, it should take a JSCompartment and be JS::CompartmentAddonId(). @@ +2666,5 @@ > bool getSingletonsAsTemplates() const { > return singletonsAsTemplates_; > }; > > + JSAddonId *addonId() const { return addonId_; } I think this should be addonIdOrNull. We should also add a comment indicating that null means "not associated with an addon", probably both in CompartmentOptions and JSCompartment. ::: js/xpconnect/src/Sandbox.cpp @@ +642,5 @@ > +#define XPCONNECT_SANDBOX_CLASS_METADATA_SLOT (XPCONNECT_GLOBAL_EXTRA_SLOT_OFFSET) > + > +// This slot is set to true or false depending on whether sandbox_addProperty > +// should mirror the property to the prototype. > +#define XPCONNECT_SANDBOX_CLASS_ADDPROP_SLOT (XPCONNECT_GLOBAL_EXTRA_SLOT_OFFSET + 1) I know that storing this kind of boolean in a slot is convention and all, but given that we're already storing writeToGlobalPrototype on the CompartmentPrivate, can we just store this there too and not mess with the JSClass flags? That seems less error-prone. @@ +680,5 @@ > return JS_ConvertStub(cx, obj, type, vp); > } > > +static bool > +sandbox_setProperty(JSContext *cx, JS::HandleObject obj, JS::HandleId id, let's call all of these writeToProto_fooProperty to make it clear that we don't use them for the general sandbox case. @@ +704,5 @@ > + > +struct AutoSkipPropertyMirroring > +{ > + AutoSkipPropertyMirroring(HandleObject obj) : obj(obj) { > + JS_SetReservedSlot(obj, XPCONNECT_SANDBOX_CLASS_ADDPROP_SLOT, BooleanValue(true)); Please assert that the value is a sandbox and that the slot value was previously false. @@ +715,5 @@ > + HandleObject obj; > +}; > + > +static bool > +sandbox_addProperty(JSContext *cx, HandleObject obj, HandleId id, MutableHandleValue vp) Can you add some comments explaining the scheme here? @@ +722,5 @@ > + > + // For some reason "undefined" is handled differently from other properties > + // by JS_EnumerateStandardClasses. So we handle it here. > + if (id == XPCJSRuntime::Get()->GetStringID(XPCJSRuntime::IDX_UNDEFINED)) > + return true; The above comment doesn't really help me understand why this is necessary. Why doesn't the JS_EnumerateStandardClasses call in CreateSandboxOption (when we've got an AutoSkipPropertyMirroring on the stack) handle this? @@ +727,5 @@ > + > + // Avoid recursively triggering sandbox_addProperty in the > + // JS_DefinePropertyById call below. > + Value recursing = JS_GetReservedSlot(obj, XPCONNECT_SANDBOX_CLASS_ADDPROP_SLOT); > + if (recursing.isBoolean() && recursing.toBoolean()) When will it not be a boolean? Can we just do: If (JS_GetReservedSlot(obj, SLOT).toBoolean()) return true? @@ +743,5 @@ > + if (!JS_CopyPropertyFrom(cx, id, unwrappedProto, obj)) > + return false; > + > + Rooted<JSPropertyDescriptor> pd(cx); > + if (!JS_GetPropertyDescriptorById(cx, obj, id, &pd)) Hm, I understand the desire to mirror the attributes of the prototype property here, but when does it actually matter? Also, this whole addProperty scheme doesn't work at all for JS-defined accessors, right? We're just assuming that people are unlikely to do that on |this| in global scope? This is basically a best-effort shim, right? @@ +764,5 @@ > > +static const JSClass SandboxWriteToProtoClass = { > + "Sandbox", > + XPCONNECT_GLOBAL_FLAGS_WITH_EXTRA_SLOTS(2), > + sandbox_addProperty, JS_DeletePropertyStub, JS_PropertyStub, JS_StrictPropertyStub, Please add a comment here indicating that billm promises to remove this addproperty usage when the time comes. :-) @@ +1173,5 @@ > + if (options.addonId) { > + addonId = JS_NewAddonId(cx, options.addonId); > + NS_ENSURE_TRUE(addonId, NS_ERROR_FAILURE); > + } > + if (JSObject *obj = JS::CurrentGlobalOrNull(cx)) { Shouldn't this be |else if|? @@ +1182,5 @@ > + compartmentOptions.setAddonId(addonId); > + > + const JSClass *clasp = &SandboxClass; > + if (options.writeToGlobalPrototype) > + clasp = &SandboxWriteToProtoClass; Nit: I'd just use the ternary operator. ::: js/xpconnect/src/xpcprivate.h @@ +3494,5 @@ > ~CompartmentPrivate(); > > bool wantXrays; > > + bool writeToGlobalPrototype; I'm really worried about exposing this to script, because I don't want addons to start using it. Please add a big comment here stating that this is for internal use only, that it may go away any time, that it should not be added to any web-facing documentation, and that it should not be used without consulting the XPConnect module owner.

I think this just fixes some brokenness in existing code. js::CheckDefineProperty is trying to check if we're changing the getter or setter. However, it fails to account for the fact that a default getter is passed in as JS_PropertyStub, while it's stored in the shape as null. So the comparison doesn't really work in those cases.

Comment on attachment 8430415 [details] [diff] [review] non-xbl Review of attachment 8430415 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with those things addressed. ::: js/xpconnect/src/XPCWrappedNativeScope.cpp @@ +310,5 @@ > +static bool > +CompartmentPerAddon() > +{ > + bool initialized = false; > + bool pref = false; I assume you meant these both to be |static bool|. ::: js/xpconnect/src/xpcpublic.h @@ +116,5 @@ > +inline JSObject * > +GetAddonScopeOrGlobal(JSContext *cx, JSObject *obj, JSAddonId *addonId) > +{ > + if (IsInAddonScope(obj)) > + return js::GetGlobalForObjectCrossCompartment(obj); So, my one concern with this pattern is that we might be in the scope of addon |foo|, and then try to get the scope for addon |bar|. This doesn't happen in the XBL case, because there are only 2 possibilities. Is this something we want to handle? My gut feeling is 'no', in which case I think we should assert in the branch that the passed-in addonId matches the one of obj's compartment. ::: modules/libpref/src/init/all.js @@ +137,5 @@ > > // Whether the UndoManager API is enabled > pref("dom.undo_manager.enabled", false); > > +// Whether to require add-on code to run in different compartments from browser Nit: I would remove the 'require' part and just say 'Whether to run'. Also, please make it clearer that this gives us a separate compartment per-(addon,global), and therefore may significantly increase the number of compartments in the system.

Here are some tests for the writeToGlobalPrototype code.

Comment on attachment 8431237 [details] [diff] [review] jsobj-changes Review of attachment 8431237 [details] [diff] [review]: ----------------------------------------------------------------- Sigh.

Adding JSAddonId to CompartmentOptions added a bunch of rooting hazards for CompartmentOptions. Since JSAddonId is an interned string that typically gets allocated at startup, I don't think there's much chance we'll want to move it. This tells the analysis not to worry about failure to root JSAddonId.

Comment on attachment 8440138 [details] [diff] [review] whitelist-addon-id-hazard Review of attachment 8440138 [details] [diff] [review]: ----------------------------------------------------------------- Looks right. Please also ensure you are asserting that AddonId is interned at runtime too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0265bd71a15 https://hg.mozilla.org/integration/mozilla-inbound/rev/85c26e838324 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b44740b12b1 https://hg.mozilla.org/integration/mozilla-inbound/rev/3289f6b88862 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2967398c62 https://hg.mozilla.org/integration/mozilla-inbound/rev/53bfc7d49029

https://hg.mozilla.org/mozilla-central/rev/a0265bd71a15 https://hg.mozilla.org/mozilla-central/rev/85c26e838324 https://hg.mozilla.org/mozilla-central/rev/7b44740b12b1 https://hg.mozilla.org/mozilla-central/rev/3289f6b88862 https://hg.mozilla.org/mozilla-central/rev/ec2967398c62 https://hg.mozilla.org/mozilla-central/rev/53bfc7d49029