Closed Bug 1148188 Opened 9 years ago Closed 9 years ago

Add a method for creating add-on specific APIs

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox40 --- fixed

People

(Reporter: mossop, Assigned: gkrizsanits)

References

Details

Attachments

(2 files, 4 obsolete files)

We want a way to create APIs that are aware of what add-on is calling them. We have the interposition service but it doesn't work so well for JS modules. A straightforward alternative would be to expose a function that JS can call to learn what add-on ID called it. Two example use-cases:

nsIBrowserSearchService.addEngine wants to know what add-on is installing a search engine. This would allow us to track what add-ons install what and uninstall search engines when the add-on that installed it is uninstalled. This is partly malware protection in nature so we can't just rely on the add-on being a good player and passing us its ID, we want to discover it automatically.

We want to add a simple method for creating a SDK module loader for any add-on that wants one. The most straightforward would be a JSM that exports a require function, when called the function takes care of creating a loader if one doesn't exist but it needs the add-on ID in order to keep the loaders scoped to the add-on.
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #1)
> I've made a third party sdk module to do this:
> 
> https://github.com/erikvold/addon-communication-jplib
> http://work.erikvold.com/jetpack/2014/01/06/addon-communication.html

I'm not sure how that helps, it relies on the API being defined by the SDK and so can get the add-on ID that way. It's also trivial for an add-on to spoof that ID.
I think this could be a good opportunity to clean up some stuff in the add-on interposition code. Here are some problems that stand in the way of this bug:

1. Only add-ons that have interposition enabled get AddonWrappers in XPConnect.
2. Add-ons that are declared as multiprocessCompatible don't have interposition enabled.
3. AddonWrapper doesn't do anything about function calls.

Perhaps we could do the following:

1. Use AddonWrapper for all add-ons, regardless of whether interposition is enabled for them.
2. Inside AddonWrapper, check if we should interpose each time a proxy hook is called. That shouldn't be too expensive.
3. Add call and construct hooks on AddonWrapper that keep a stack of add-on IDs (could be stored in a global somewhere). Then have a function on Components.utils that would return the add-on ID at the top of the stack.

Ni Bobby about the design and Gabor to see if you're interested in working on this after bug 1133761 is done.
Flags: needinfo?(gkrizsanits)
Flags: needinfo?(bobbyholley)
(In reply to Bill McCloskey (:billm) from comment #3)
> Ni Bobby about the design and Gabor to see if you're interested in working
> on this after bug 1133761 is done.

Yes, I'm totally interested. Although the model seems a bit scary to me, there are so many confusing things that can go on with the call stack. I wish we didn't have to rely on them. Is this version eval/evalInSandbox/exportFunction proof? Do add-ons have access to other scopes they can create a Function object in and initiate the call by them (using them as listener callbacks for example so they are not in the call stack)?

In short I think we should design a version that is focusing on the question of who has the reference to nsIBrowserSearchService.addEngine rather than who is the caller. What the wrapper layer could do for us is flagging wrappers once they went through an add-on. So if my add-on gets a reference to something, and passing it to a 3rd compartment, the wrapper on the other side will know that he got it from the add-on. And this could be transitive.

Anyway, I leave it to Bobby, I'm just trying to understand the problem space right now. Is nsIBrowserSearchService implemented in JS (seems like to me, so we have a double wrapped JS object, correct? do we create a native wrapper for them in each scope or we just use CCW in this case?) Can its method called by content if it's exposed to content (I think no, there would be a COW that blocks calls)? Do add-ons have access to system privileged scope that do not belong to them (I think yes)?
Flags: needinfo?(gkrizsanits)
(In reply to Bill McCloskey (:billm) from comment #3)
> I think this could be a good opportunity to clean up some stuff in the
> add-on interposition code. Here are some problems that stand in the way of
> this bug:
> 
> 1. Only add-ons that have interposition enabled get AddonWrappers in
> XPConnect.
> 2. Add-ons that are declared as multiprocessCompatible don't have
> interposition enabled.

When I was looking into interposition for doing this I was considering just creating a main interposition that all add-ons get that then delegates to the multiprocessShims when the add-on isn't marked e10s-safe. I was also considering making it possible to register these kinds of APIs with it in some way which would change their invocation or something.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> (In reply to Bill McCloskey (:billm) from comment #3)
> Anyway, I leave it to Bobby, I'm just trying to understand the problem space
> right now. Is nsIBrowserSearchService implemented in JS (seems like to me,
> so we have a double wrapped JS object, correct? do we create a native
> wrapper for them in each scope or we just use CCW in this case?) Can its
> method called by content if it's exposed to content (I think no, there would
> be a COW that blocks calls)? Do add-ons have access to system privileged
> scope that do not belong to them (I think yes)?

To be clear, the current interposition service does work for nsIBrowserSearchService.addEngine, but it doesn't work for my second example. I'm not against using two different methods for the different cases but it would be nice to have something consistent.

Also, it's not inconceivable that we might find other XPCOM APIs that want this but are C++ implemented.
I'm fine with using AddonWrapper all the time if Bill thinks the performance is acceptable. Hooking call/construct seems like a good idea in any case.

I'm not wild about stack introspection proposed in comment 0 or the taint tracking proposed in comment 4. Any such system is going to be incredibly difficult to get right, and I'm concerned that we don't have a clear picture of the semantics we really want. I don't want to expose observable behavior that isn't founded in crisp and unambiguous principles - doing so create a maintenance nightmare for years to come (think jsd).

I also don't think we can or should try to "beat" malicious addons. If we can catch most of them with interposition that's great - but if these things are running with system principal, we can't stop them from doing one of a million clever things to sidestep restrictions that we try to place. Let's use the tools that we have (i.e. interposition) to do what we can, but not add new tools just for that.

I don't understand why use case #2 from comment 0 can't be solved with interpositions. Can't we just create an interposition that passes our addon id the JSM functions we want to pass it to?
Flags: needinfo?(bobbyholley)
Here's what we want to do in case 2:

== Require.jsm ==
this.EXPORT_SYMBOLS=["require"]
function require() { ... get addon ID somehow ... }

== addon.js ==
Cu.import("resource://gre/modules/Require.jsm");
require("jetpack-thing");

Interposition allows us to interpose on property accesses on wrappers whose target is outside the addon. This example doesn't include any such property accesses. We could add support for interposition on calls I guess. But then we have to run a lot more JS code for every call, and it's not really needed.

I'd be fine with a solution where Require.jsm would somehow tag the function as needing the addon ID. Then we could observe that when we wrap it. However, I'm not sure that will work for the search case since it goes through a wrapped native and wrapped JS.

I agree that this doesn't need to be completely unbreakable. We just need to find a simple, clean way to do it.
(In reply to Bill McCloskey (:billm) from comment #7)
> Interposition allows us to interpose on property accesses on wrappers whose
> target is outside the addon. This example doesn't include any such property
> accesses. We could add support for interposition on calls I guess. But then
> we have to run a lot more JS code for every call, and it's not really needed.

If we interpose on calls to Require.jsm, why is this all that much more than is needed? Or is the issue that we need the ID in functions that don't live in Require.jsm?
 
> I'd be fine with a solution where Require.jsm would somehow tag the function
> as needing the addon ID. Then we could observe that when we wrap it.
> However, I'm not sure that will work for the search case since it goes
> through a wrapped native and wrapped JS.

Couldn't we just add an optional addon ID param to the method  that's interested in it, and use an interposition to make sure it gets passed accurately?
 
> I agree that this doesn't need to be completely unbreakable. We just need to
> find a simple, clean way to do it.

Agreed. And I would rather do that with our existing machinery (or small extensions to it) if at all possible. The XPConnect wrapping code is super complicated, and I don't want to bake in special cases and custom machinery for stuff like this unless we're really sure that it's a general problem we need to solve.
(In reply to Bobby Holley (:bholley) from comment #8)
> (In reply to Bill McCloskey (:billm) from comment #7)
> > Interposition allows us to interpose on property accesses on wrappers whose
> > target is outside the addon. This example doesn't include any such property
> > accesses. We could add support for interposition on calls I guess. But then
> > we have to run a lot more JS code for every call, and it's not really needed.
> 
> If we interpose on calls to Require.jsm, why is this all that much more than
> is needed? Or is the issue that we need the ID in functions that don't live
> in Require.jsm?

Well, our interposition code doesn't differentiate between different target modules. So we would be interposing on all calls, which sounds expensive.

Also, the other problem I mentioned above is that interposition is disabled for multiprocessCompatible addons. And we want the stuff here to work for all addons.
(In reply to Bill McCloskey (:billm) from comment #9) 
> Well, our interposition code doesn't differentiate between different target
> modules.

That's something I would be in favor of fixing.

> Also, the other problem I mentioned above is that interposition is disabled
> for multiprocessCompatible addons. And we want the stuff here to work for
> all addons.

Right, and I mentioned my support for fixing that in comment 6.
I think this bug can be broken into three pieces:

1. One piece would create a separate kind of interposition for add-ons that are multiprocessCompatible. Right now we install the shims in two places, [1] and [2]. Then we look them up at [3]. I think we should change the code at [3] so that we use a "default" interposition service if none is provided (and if the add-on ID is non-null and interposition is enabled). We would have to do a do_GetService call at [3] and create a new contract ID, like "default-addon-shims;1".

2. Then we need to add a call hook to nsIAddonInterposition. Right now we only have the "interpose" hook, which is called on property access [4]. Perhaps we could change "interpose" to "interposeProperty" and create "interposeCall". Then we would write call and construct hooks in AddonWrapper [5]. Those hooks would call interposeCall. (AddonWrapper only handles the non-XPCWrappedNative case, but I don't think we should worry about a call hook for WrappedNatives since we don't have any use for it.)

3. Then we need to optimize all this stuff so that it can ride the trains. However, it's probably better to leave that for bug 1116854. I'm still not entirely sure how we should design that yet.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4302
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#558
[3] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNativeScope.cpp#132
[4] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/idl/nsIAddonInterposition.idl#24
[5] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AddonWrapper.h#24
When should this be done? Sounds like a bit of a work, I would finish up my m6 bugs first (unless I'm blocked with them and have some free time which happens...). If that sounds OK, I would like to work on this.
tracking-e10s: --- → ?
Assignee: nobody → gkrizsanits
(In reply to Bill McCloskey (:billm) from comment #11)
> 
> 2. Then we need to add a call hook to nsIAddonInterposition. Right now we
> only have the "interpose" hook, which is called on property access [4].
> Perhaps we could change "interpose" to "interposeProperty" and create
> "interposeCall". Then we would write call and construct hooks in
> AddonWrapper [5]. Those hooks would call interposeCall. (AddonWrapper only
> handles the non-XPCWrappedNative case, but I don't think we should worry
> about a call hook for WrappedNatives since we don't have any use for it.)
> 

After thinking some more about 3. I'm not sure if this is the right way to do
this. How about instead of the interposeCall on the shim, one could register a function
object to interpose on. Cu.registerInterposeCall(functionObj, interposeCall).

Then we would store these interpose call objects on XPCWrappedNativeScope in
a weakmap. And in addonwrapper in the call and construct hooks, we could
look up the wrapper in the map and do a js call on the registered interposeCall
object. So we would only do the interposition if really needed.

Then interposeCall would look something like this: 
interposeCall(addonId, this, target, args) and based on its return value we
could decide whether the engine needs to go on with the original call or not.
(Like if it returns an object with a result property then we're done and just
use that for the return value, if it returns a primitive instead we go ahead
and do the original call and ignore the return value of the interposeCall in
the C++ code) 

About the XPCWrappedNative case, I think the first example in Comment 1 just
gives an example use case... So we might want to figure out something there too.
But since each native knows its XPCWrappedNativeScope and its own wrapper, it
could just do the lookup and do the same thing what addonwrapper does. (Not
sure where that code goes, but I think I could figure out something or ask
Bobby)

OR I can keep the general interposeCall on the shim and then the registerInterposeCall
would take only one argument. (and the weakMap had the same value as the key or something)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> After thinking some more about 3. I'm not sure if this is the right way to do
> this. How about instead of the interposeCall on the shim, one could register
> a function
> object to interpose on. Cu.registerInterposeCall(functionObj, interposeCall).
> 
> Then we would store these interpose call objects on XPCWrappedNativeScope in
> a weakmap. And in addonwrapper in the call and construct hooks, we could
> look up the wrapper in the map and do a js call on the registered
> interposeCall
> object. So we would only do the interposition if really needed.
> 
> Then interposeCall would look something like this: 
> interposeCall(addonId, this, target, args) and based on its return value we
> could decide whether the engine needs to go on with the original call or not.
> (Like if it returns an object with a result property then we're done and just
> use that for the return value, if it returns a primitive instead we go ahead
> and do the original call and ignore the return value of the interposeCall in
> the C++ code) 

That seems like a reasonable way to go.

> About the XPCWrappedNative case, I think the first example in Comment 1 just
> gives an example use case... So we might want to figure out something there
> too.

Do you mean the nsIBrowserSearchService.addEngine case? That can already be handled by interposition. When the addon gets the "addEngine" property on an nsIBrowserSearchService object, we can replace it with a version that passes in the add-on ID.

> But since each native knows its XPCWrappedNativeScope and its own wrapper, it
> could just do the lookup and do the same thing what addonwrapper does. (Not
> sure where that code goes, but I think I could figure out something or ask
> Bobby)

Bobby might correct me, but I think the problem is that there isn't any definitive JS version of an XPCOM function. We create them on demand here:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNativeJSOps.cpp#379
For most XPCWNs, there will be one per scope (the exception, I think, is when a WN is forced by PreCreate hooks to live in a particular scope). So even if we registerInterposeCall in one scope, it won't apply to any other scopes. (For the AddonWrapper case, we can probably Unwrap the function that gets passed in to get the definitive identity for it. There isn't really a way to do that for the WN case as far as I know.)

However, the XPCWrappedNative case is special because every function is a property of some object. That's just how XPCOM works. So we can always use the property hooks to do interposition instead of a call hook.

> OR I can keep the general interposeCall on the shim and then the
> registerInterposeCall
> would take only one argument. (and the weakMap had the same value as the key
> or something)

That would probably be fine too. But it's not related to whether we do the XPCWrappedNative case, right?
I run into a few issues while trying to implement the fast version. So the idea of using a WeakMap probably won't fly. We register a target function we want to do interpose on. In the interposition call we want in many cases call the original target manually (among doing other stuff as well), and then we want to prevent it doing another interposition and end up in an inifinit loop (we could fix this in js as well in the interposeCall function but that would be a very ugly API). My idea to fix it was to just remove the target function object from the map and then re-add it once the interposition call is done. But there is no delete method exposed on the WeakMapPtr class, and I think with a reason...

Other issue is that I'm not sure it's good to register a function object and then hoping that the consumer won't forget to keep a reference of the said object alive (otherwise it might get GCed and removed from the weak map)

So the question is, if not weakmap, then what can we use in xpconnect? I've seen things like:
    typedef js::HashSet<JSObject*,
                        js::PointerHasher<JSObject*, 3>,
                        js::SystemAllocPolicy> DOMExpandoSet;

But how is that safe? Is it safe to just use a <JSObject*, JSObject*>? Can't the GC/CC just move things around? Or can/should we flag these objects somehow before using their naked pointers in the hashmap?
Flags: needinfo?(bobbyholley)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> I run into a few issues while trying to implement the fast version. So the
> idea of using a WeakMap probably won't fly. We register a target function we
> want to do interpose on. In the interposition call we want in many cases
> call the original target manually (among doing other stuff as well), and
> then we want to prevent it doing another interposition and end up in an
> inifinit loop (we could fix this in js as well in the interposeCall function
> but that would be a very ugly API). My idea to fix it was to just remove the
> target function object from the map and then re-add it once the
> interposition call is done. But there is no delete method exposed on the
> WeakMapPtr class, and I think with a reason...

I think what we should do here is to have the interposition callback take a reference to the clean function, which we would annotate with some sort of interposition waiver. I'd like to avoid scoped tricks like removing the function from the map, since that causes reentrancy issues.
 
> Other issue is that I'm not sure it's good to register a function object and
> then hoping that the consumer won't forget to keep a reference of the said
> object alive (otherwise it might get GCed and removed from the weak map)

Yeah it seems like we want to pin the interposed function alive as long as the interposition is active - a strong map seems like the way to do this.
 
> So the question is, if not weakmap, then what can we use in xpconnect? I've
> seen things like:
>     typedef js::HashSet<JSObject*,
>                         js::PointerHasher<JSObject*, 3>,
>                         js::SystemAllocPolicy> DOMExpandoSet;
> 


> But how is that safe? Is it safe to just use a <JSObject*, JSObject*>? Can't
> the GC/CC just move things around? Or can/should we flag these objects
> somehow before using their naked pointers in the hashmap?

We manually trace them in XPCWrappedNativeScope::TraceWrappedNativesInAllScopes.
Flags: needinfo?(bobbyholley)
Blocks: 1047687
(In reply to Bobby Holley (:bholley) from comment #16)
> I think what we should do here is to have the interposition callback take a
> reference to the clean function, which we would annotate with some sort of
> interposition waiver. I'd like to avoid scoped tricks like removing the
> function from the map, since that causes reentrancy issues.

I'm not too wild about inventing another waiver wrapper. It's just black
magic from JS side, with all sort of issues with object identity. What reentrancy
issues do you have in mind that you think we cannot deal with exactly?

> Yeah it seems like we want to pin the interposed function alive as long as
> the interposition is active - a strong map seems like the way to do this.

I'm going to try to use JSObject2JSObjectMap then, I'm not too familiar with
these maps, I wish JS API would offer a simple way to use JSObject maps, is
it just me or is this really a huge fault in the JS API we should file a bug
about? (we will need some sort of JSObject map for other things as well in the
DOM probably, for example see Bug 1103933)

> We manually trace them in
> XPCWrappedNativeScope::TraceWrappedNativesInAllScopes.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> (In reply to Bobby Holley (:bholley) from comment #16)
> > I think what we should do here is to have the interposition callback take a
> > reference to the clean function, which we would annotate with some sort of
> > interposition waiver. I'd like to avoid scoped tricks like removing the
> > function from the map, since that causes reentrancy issues.
> 
> I'm not too wild about inventing another waiver wrapper. It's just black
> magic from JS side, with all sort of issues with object identity.

Whether or not we implement it with a waiver wrapper, we still need a separate identity so that the interposition can properly invoke the original method.

> What reentrancy issues do you have in mind that you think we cannot deal with exactly?

We interpose, the interposition calls the original method, the original method fires a sync event, we end up back in chrome, we make an unrelated call to the same method, and we need to interpose a second time. This is why we can't just temporarily remove the entry from the map.

> > Yeah it seems like we want to pin the interposed function alive as long as
> > the interposition is active - a strong map seems like the way to do this.
> 
> I'm going to try to use JSObject2JSObjectMap then, I'm not too familiar with
> these maps, I wish JS API would offer a simple way to use JSObject maps, is
> it just me or is this really a huge fault in the JS API we should file a bug
> about? (we will need some sort of JSObject map for other things as well in
> the
> DOM probably, for example see Bug 1103933)

you mean window.Map? We could certainly expose a MapPtr, akin to WeakMapPtr (which I implemented). But if you're already in XPConnect, JSObject2JSObjectMap is probably fine.
(In reply to Bobby Holley (:bholley) from comment #18)
> We interpose, the interposition calls the original method, the original
> method fires a sync event, we end up back in chrome, we make an unrelated
> call to the same method, and we need to interpose a second time. This is why
> we can't just temporarily remove the entry from the map.

Thanks this is a case I didn't take into account. I'm going to implement a waiver
wrapper tomorrow. But since there are quite some agreement before I can land this
API and since the time is very tight. I'm filing the patches as they are for feedback,
and will file one more patch that replaces AutoEnterInterposeCall with the waiver.

> you mean window.Map? We could certainly expose a MapPtr, akin to WeakMapPtr
> (which I implemented). But if you're already in XPConnect,
> JSObject2JSObjectMap is probably fine.

Something like that yes. But also WeakMapPtr is quite weird with the inability to
explicitly remove elements. My real issue is however that consumers of the jsapi
like DOM/xpconnect needs these maps, and by taking a look at JSObject2JSObjectMap
implementation, there is no simple way to use them. I would prefer something on
jsapi or at least jsfriendapi level. But this is off topic, I just use JSObject2JSObjectMap.
Attached patch part1: defaultShims (obsolete) — Splinter Review
I'm not sure how should we skip the interpose calls for the default shim...
Attachment #8600907 - Flags: feedback?(wmccloskey)
Attached patch part2: interposeCall (obsolete) — Splinter Review
This patch is only the interposition call on the shim itself separated from the registration.
Attachment #8600912 - Flags: feedback?(wmccloskey)
Attachment #8600912 - Flags: feedback?(bobbyholley)
Attached patch part3: SetAddonCallInterposition (obsolete) — Splinter Review
Attachment #8600913 - Flags: feedback?(wmccloskey)
Attachment #8600913 - Flags: feedback?(bobbyholley)
Attachment #8600912 - Attachment description: Bug 1148188 - part2: interposeCall → part2: interposeCall
Attached patch example usage (obsolete) — Splinter Review
Not sure what test should we add for this API yet... anyway, a quick example probably helps to get the idea what I had in mind.

The reason I don't do the call in C++ is that re-wrapping all the args is quite ugly code, and having an interposition call on nsIAddonInterpostion allows a more flexible API. (I'm not quite sure what else we will want to use this hook for and it's easier to adjust things on the JS side)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> Something like that yes. But also WeakMapPtr is quite weird with the
> inability to
> explicitly remove elements.

That could be added I think - I just didn't need it when I implemented WeakMapPtr.
Comment on attachment 8600907 [details] [diff] [review]
part1: defaultShims

Review of attachment 8600907 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. One thing to keep in mind is that the default shim is only used for add-ons. So I don't think there's any need to worry about reentrancy. We certainly don't expect shims to be implemented by add-ons.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +137,5 @@
>              mInterposition = p->value();
>          }
> +        // We also want multiprocessCompatible add-ons to have a default interposition.
> +        if (!mInterposition && addonId && isSystem) {
> +          bool interpositionEnabled = !mozilla::Preferences::GetBool(

Why are you negating this?

::: toolkit/components/addoncompat/addoncompat.manifest
@@ +1,4 @@
>  component {1363d5f0-d95e-11e3-9c1a-0800200c9a66} multiprocessShims.js
>  contract @mozilla.org/addons/multiprocess-shims;1 {1363d5f0-d95e-11e3-9c1a-0800200c9a66}
> +component {50bc93ce-602a-4bef-bf3a-61fc749c4caf} defaultShims.js
> +contract @mozilla.org/addons/multiprocess-shims;1 {1363d5f0-d95e-11e3-9c1a-0800200c9a66}

This line isn't right. It should look like this:
contract @mozilla.org/addons/default-addon-shims;1 {50bc93ce-602a-4bef-bf3a-61fc749c4caf}

::: toolkit/components/addoncompat/defaultShims.js
@@ +23,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAddonInterposition, Ci.nsISupportsWeakReference]),
> +
> +  interpose: function(addon, target, iid, prop) {
> +    // We probably should never call this...
> +    return true;

Maybe null is a better result here.
Attachment #8600907 - Flags: feedback?(wmccloskey) → feedback+
Comment on attachment 8600912 [details] [diff] [review]
part2: interposeCall

Review of attachment 8600912 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/idl/nsIAddonInterposition.idl
@@ +55,5 @@
> +     * it is ignored and the engine goes forth and calls the target.
> +     */
> +    jsval interposeCall(in jsval addonId,
> +                        in jsval target,
> +                        in jsval interposeCall,

Would this be better called |originalFunc|?

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +96,5 @@
> +        return true;
> +
> +    // If interposeCall returns an object it means we don't have to call the original
> +    // intercepted function, and the returned object stores the return value as the
> +    // result property.

Hm, why all of this? Shouldn't we just require the interposition to invoke the original function itself if that's what it wants to do?

I'd really rather avoid introducing magic object layouts wherever possible.
Attachment #8600912 - Flags: feedback?(bobbyholley)
Comment on attachment 8600913 [details] [diff] [review]
part3: SetAddonCallInterposition

Review of attachment 8600913 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCComponents.cpp
@@ +3490,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsXPCComponents_Utils::SetAddonCallInterposition(HandleValue scope,
> +                                                 HandleValue target,

What does |scope| mean here exactly? Is there ever a situation where it's not same-compartment with target? If so we don't need it.

Also, shouldn't this all be keyed off of addon ID like the rest of the interposition stuff?

@@ +3491,5 @@
>  
>  NS_IMETHODIMP
> +nsXPCComponents_Utils::SetAddonCallInterposition(HandleValue scope,
> +                                                 HandleValue target,
> +                                                 HandleValue interposeCall,

Instead of |target| and |interposeCall| I'd call them |originalFunction| and |replacementFunction|.

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +76,5 @@
>      nsCOMPtr<nsIAddonInterposition> interp = scope->GetInterposition();
>  
> +    RootedObject unwrappedTarget(cx, UncheckedUnwrap(target));
> +    XPCWrappedNativeScope* targetScope = ObjectScope(unwrappedTarget);
> +    RootedObject callInterposition(cx, 

whitespace

@@ +82,5 @@
> +
> +    if (!callInterposition)
> +        return true;
> +
> +    AutoEnterInterposeCall ai(cx, unwrappedTarget, targetScope);

I think the naming should be |AutoInterposeCall aic(...);|.
Attachment #8600913 - Flags: feedback?(bobbyholley)
Comment on attachment 8600912 [details] [diff] [review]
part2: interposeCall

Review of attachment 8600912 [details] [diff] [review]:
-----------------------------------------------------------------

Just to be very clear: we don't need to worry about waivers at all in this bug since the interposition code will never run in an addon scope, so it will never have interposition applied to it. Sorry I didn't clear that up earlier; I didn't understand what the waiver conversation was about until I looked at the code.

The basics of this patch look fine though. I think it just needs a little simplification.

::: js/xpconnect/idl/nsIAddonInterposition.idl
@@ +39,5 @@
> +                            in jsval target,
> +                            in nsIIDPtr iface,
> +                            in jsval prop);
> +    /**
> +     * Intercept calls on functions from non-addon scopes.

Correction: we're intercepting calls from add-ons scopes into non-addon scopes.

@@ +42,5 @@
> +    /**
> +     * Intercept calls on functions from non-addon scopes.
> +     *
> +     * @param addonId The ID of the add-on accessing the property.
> +     * @param target The fuinction object being intercepted.

typo: function

@@ +55,5 @@
> +     * it is ignored and the engine goes forth and calls the target.
> +     */
> +    jsval interposeCall(in jsval addonId,
> +                        in jsval target,
> +                        in jsval interposeCall,

I still don't quite understand what this param is for, but I think we can drop it.

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +91,5 @@
> +        xpc::Throw(cx, rv);
> +        return false;
> +    }
> +
> +    if (!returnVal.isObject())

I agree with Bobby about not doing the special stuff below. Let's just wrap returnVal and return it. And remove the |done| stuff.
Attachment #8600912 - Flags: feedback?(wmccloskey) → feedback+
Comment on attachment 8600913 [details] [diff] [review]
part3: SetAddonCallInterposition

Review of attachment 8600913 [details] [diff] [review]:
-----------------------------------------------------------------

The interface here is a bit weird. To do the interposition, we call a method on the interposition service. It can decide what sort of interposition to do by looking at the target that's passed in. However, we also have this map. The keys of the map tell us which functions we actually want to interpose on, which is a nice optimization. But the map's values don't really seem necessary. The interposition service could use them to decide what interposition to do, but it doesn't need them because it could just look at the target.

I think it would be simpler if we got rid of the map and just had a bit on each XPCWrappedNativeScope to decide whether to use call interpositions on that scope. So the pseudocode for interposing would look like this:
  // Assume target is the function you're trying to call
  if (target's xpc wn scope does not have interposition bit set) return and call function as normal;
  call InterposeCall(target, thisVal, arguments) and return result;
That way we don't have the complexity of a map to maintain. The API would just be something like:
  Cu.requestCallInterposition(<some object in the compartment>);
and then we would set the bit on the object's xpc wn scope.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3490,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsXPCComponents_Utils::SetAddonCallInterposition(HandleValue scope,
> +                                                 HandleValue target,

I don't think we need to worry about keying off add-on ID. We expect this to be needed for all add-ons.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +23,5 @@
>  using namespace JS;
>  
>  /***************************************************************************/
>  
> +AutoEnterInterposeCall::AutoEnterInterposeCall(JSContext* cx, JS::HandleObject target, XPCWrappedNativeScope* scope)

I don't think you need this class at all. Because we're only going from add-ons to non-addons, there shouldn't be any re-entrancy issues.
Attachment #8600913 - Flags: feedback?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #25)
> Comment on attachment 8600907 [details] [diff] [review]
> part1: defaultShims
> Why are you negating this?

> This line isn't right. It should look like this:
> contract @mozilla.org/addons/default-addon-shims;1
> {50bc93ce-602a-4bef-bf3a-61fc749c4caf}
> 

Sorry for these dumb mistakes, I knew I should have spent a bit more time on re-reading these patches, but did not want to delay them yet another day... and thanks for catching these! :)
(In reply to Bobby Holley (:bholley) from comment #26)
> Comment on attachment 8600912 [details] [diff] [review]
> part2: interposeCall
> 
> Hm, why all of this? Shouldn't we just require the interposition to invoke
> the original function itself if that's what it wants to do?
> 
> I'd really rather avoid introducing magic object layouts wherever possible.

You're right, I think I overgeneralized this one.

(In reply to Bill McCloskey (:billm) from comment #28)
> Just to be very clear: we don't need to worry about waivers at all in this
> bug since the interposition code will never run in an addon scope, so it
> will never have interposition applied to it. Sorry I didn't clear that up
> earlier; I didn't understand what the waiver conversation was about until I
> looked at the code.

I was not sure what are all the planned use cases for this call hook in the
future, but probably we should just focus on a simple version that does
what we need NOW. I will just ditch that part.

> > +    jsval interposeCall(in jsval addonId,
> > +                        in jsval target,
> > +                        in jsval interposeCall,
> 
> I still don't quite understand what this param is for, but I think we can
> drop it.

If you look at require.jsm, require is already juggling with optional arguments,
and we want to throw in yet another one (addonId) I felt like it's nicer if someone
wants a function to be intercepted, he can register a replacer function as well.

So instead of require, let's say requireForAddons will be called that can handle
the addonId part and then call require. (in the example I filed the interposeCall)
is the anonymous function I pass in in require.jsm.
(In reply to Bobby Holley (:bholley) from comment #27)
> Comment on attachment 8600913 [details] [diff] [review]
> part3: SetAddonCallInterposition
> What does |scope| mean here exactly? Is there ever a situation where it's
> not same-compartment with target? If so we don't need it.

Eh... it's only a reminder from a previous version, I should have removed that.

> 
> Also, shouldn't this all be keyed off of addon ID like the rest of the
> interposition stuff?

Not really, we only needed that part because for those cases we didn't want
to do interpostion for all addons like we want here.

(In reply to Bill McCloskey (:billm) from comment #29)
> Comment on attachment 8600913 [details] [diff] [review]
> part3: SetAddonCallInterposition
> 
> Review of attachment 8600913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The interface here is a bit weird. To do the interposition, we call a method
> on the interposition service. It can decide what sort of interposition to do
> by looking at the target that's passed in. However, we also have this map.
> The keys of the map tell us which functions we actually want to interpose
> on, which is a nice optimization. But the map's values don't really seem
> necessary. The interposition service could use them to decide what
> interposition to do, but it doesn't need them because it could just look at
> the target.
> 
> I think it would be simpler if we got rid of the map and just had a bit on
> each XPCWrappedNativeScope to decide whether to use call interpositions on
> that scope. So the pseudocode for interposing would look like this:
>   // Assume target is the function you're trying to call
>   if (target's xpc wn scope does not have interposition bit set) return and
> call function as normal;
>   call InterposeCall(target, thisVal, arguments) and return result;
> That way we don't have the complexity of a map to maintain. The API would
> just be something like:
>   Cu.requestCallInterposition(<some object in the compartment>);
> and then we would set the bit on the object's xpc wn scope.

I could totally get rid of the value part of the map, and use a set instead, but
we don't really have a set... So it was kind of free to add the value part of
that map, for the little convince I think I gained there (see my previous comment).

Now about getting rid of the whole map idea... I feel a bit concerned. I really don't
want to invent a bunch of new interpose calls we don't need since for each of these calls
we pay significant overhead (creating an array for the args for example, and a bunch of
extra calls through xpconnect layers...) These might seem like a pre-mature optimization
but here is what I'm trying to avoid. With the current version the add-on calls require
it gets intercepted and we're done. With the XPCWrappedNativeScope flagging version, in case
the module loader decides to share the XPCWrappedNativeScope among all the modules, we're going
to intercept every call that goes between the addon and object from the SDK modules. If we want
traditional addons to use SDK modules for some tasks, this can be a huge drawback... 
What do you think?

> > +AutoEnterInterposeCall::AutoEnterInterposeCall(JSContext* cx, JS::HandleObject target, XPCWrappedNativeScope* scope)
> 
> I don't think you need this class at all. Because we're only going from
> add-ons to non-addons, there shouldn't be any re-entrancy issues.

Hmm... You are actually right, the calls I was worried about come from a different scopes in
every cases I can think of and we care about...
Again, I think I tried to do an over generalization here, but now that I went over our main use
case, I realized something else. Let's say we intercept the require that comes from the add-on
scope, and the module it loads does some requires too. We will not intercept those! It's not a
huge issue, since we can just store that addonID in that first require, just something we should
keep in mind. On the other hand, do we ever want to intercept anything else then? Maybe we should
just store that one object (require) we ever want to intercept, instead of the map and then just walk away.
> Now about getting rid of the whole map idea... I feel a bit concerned. I really
> don't want to invent a bunch of new interpose calls we don't need since for each
> of these calls we pay significant overhead (creating an array for the args for
> example, and a bunch of extra calls through xpconnect layers...)

I'm not sure what you mean here. We'd still just have interposeCall and interposeProperty. And we'd only pay the cost of creating the args array if the flag on the scope is true, which should be very rare.

> These might seem
> like a pre-mature optimization but here is what I'm trying to avoid. With the
> current version the add-on calls require it gets intercepted and we're done. With
> the XPCWrappedNativeScope flagging version, in case the module loader decides to
> share the XPCWrappedNativeScope among all the modules, we're going to intercept
> every call that goes between the addon and object from the SDK modules. If we
> want traditional addons to use SDK modules for some tasks, this can be a huge
> drawback... 

I agree that there could be a problem if we only want to interpose on one function from some big module. But that's not the case here: require.js only exposes one function. And each SDK module gets its own XPCWrappedNativeScope. That's not likely to change in the future. We've invested too much in compartment-per-global. If it did change, we could revisit this bug.

So I'd like to go with the flag approach. It's very simple and we don't have to worry about weird GC bugs involving the maps.
(In reply to Bill McCloskey (:billm) from comment #33)
> interposeProperty. And we'd only pay the cost of creating the args array if
> the flag on the scope is true, which should be very rare.

If my concern is valid then it is not that rare...

> exposes one function. And each SDK module gets its own
> XPCWrappedNativeScope. That's not likely to change in the future. We've
> invested too much in compartment-per-global. If it did change, we could
> revisit this bug.

Have you checked this with someone? I just want to be sure we double check this
because while I was in the SDK team there were quite some work being done to
achieve that modules share the same global (that does not go against the one
compartment per global approach). The idea was to freeze all the standard
classes, and give each module it's own scope object (never the real
global of the shared compartment)

You can specify the target object for module loader: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/idl/mozIJSSubScriptLoader.idl#30
Which will use the given object as target: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSSubScriptLoader.cpp#283
And for the addon loader you seem to have an option for this: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#223

I'm not entirely sure, just trying to judge from the code... also, require.jsm can be totally unrelated from this, I'm not a big expert in our js source base and how all these things are tied together. So if you are positive that we should not worry about this I will trust you. But if for some reason we end up loading all the modules into the same compartment as the require.jsm because of these shared sandboxes, that would mean that the flag is either set for all modules or none, and that's the reason I invented the whole map idea instead...

> 
> So I'd like to go with the flag approach. It's very simple and we don't have
> to worry about weird GC bugs involving the maps.

If I'm wrong I totally see your point and we should go with the flag. I'm not
looking forward to hunt down GC bugs either.
Attachment #8602065 - Flags: review?(wmccloskey)
I merged part 2 and 3 because I never really wanted to land those patches separately and since the simplified version seem to fit just fine into one patch. Bobby, I flag only Bill for review this time, since the xpconnect parts are a lot simpler now, but feel free to take a look and hit the alarm if you don't agree with some parts!

I went with the flag instead of the map version, since the modules will be in a different scope than the require function we want to do interposition on.
Attachment #8600907 - Attachment is obsolete: true
Attachment #8600912 - Attachment is obsolete: true
Attachment #8600913 - Attachment is obsolete: true
Attachment #8600917 - Attachment is obsolete: true
Attachment #8602067 - Flags: review?(wmccloskey)
Comment on attachment 8602067 [details] [diff] [review]
part2: interposeCall. v2

Review of attachment 8602067 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Gabor! This looks great.

::: js/src/jsfriendapi.cpp
@@ +1265,5 @@
> +js::ConvertArgsToArray(JSContext* cx, const CallArgs& args)
> +{
> +    RootedObject argsArray(cx, NewDenseCopiedArray(cx, args.length(), args.array()));
> +    return argsArray;
> +}

Need a newline after the brace.

::: js/xpconnect/idl/xpccomponents.idl
@@ +689,5 @@
>      [implicit_jscontext]
>      void setAddonInterposition(in ACString addonId, in nsIAddonInterposition interposition);
>  
> +    [implicit_jscontext]
> +    void setAddonCallInterposition(in jsval target);

Maybe add a comment here explaining that this enables interposeCall for any calls into the compartment of |target| (not just for target itself).

::: js/xpconnect/src/XPCComponents.cpp
@@ +3494,5 @@
> +                                                 JSContext* cx)
> +{
> +    NS_ENSURE_TRUE(target.isObject(), NS_ERROR_INVALID_ARG);
> +    RootedObject targetObj(cx, &target.toObject());
> +    XPCWrappedNativeScope* xpcScope = ObjectScope(targetObj);

I'd rather we unwrap targetObj here so that we can register stuff from outside its compartment.

::: js/xpconnect/tests/unit/test_interposition.js
@@ +145,5 @@
> +  let moduleScope = Cu.Sandbox(this);
> +  moduleScope.ADDONID = ADDONID;
> +  moduleScope.do_check_eq = do_check_eq;
> +  function funToIntercept(addonId) {
> +    do_check_eq(addonId, ADDONID);

Could you set a flag here to signal that the function was actually called, and then check that the flag is true after the moduleFunction() call?

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +66,5 @@
>      return true;
>  }
>  
> +bool
> +InterposeCall(JSContext* cx, JS::HandleObject target, const JS::CallArgs& args, bool& done)

Could you pass done as a bool*? I like to be able to see &done at the callsite to know it's an outparam.

@@ +89,5 @@
> +    JSAddonId* addonId = AddonIdOfObject(target);
> +    RootedValue addonIdValue(cx, StringValue(StringOfAddonId(addonId)));
> +    RootedValue targetValue(cx, ObjectValue(*target));
> +    RootedValue thisValue(cx, args.thisv());
> +    RootedObject argsArray(cx, ConvertArgsToArray(cx, args));

We should probably null check this result.

@@ +90,5 @@
> +    RootedValue addonIdValue(cx, StringValue(StringOfAddonId(addonId)));
> +    RootedValue targetValue(cx, ObjectValue(*target));
> +    RootedValue thisValue(cx, args.thisv());
> +    RootedObject argsArray(cx, ConvertArgsToArray(cx, args));
> +    RootedValue argsVal(cx);

Just initialize with ObjectValue(*argsArray)?
Attachment #8602067 - Flags: review?(wmccloskey) → review+
Comment on attachment 8602065 [details] [diff] [review]
part1: defaultShims. v2

Review of attachment 8602065 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/addoncompat/defaultShims.js
@@ +22,5 @@
> +  classID: Components.ID("{50bc93ce-602a-4bef-bf3a-61fc749c4caf}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAddonInterposition, Ci.nsISupportsWeakReference]),
> +
> +  interpose: function(addon, target, iid, prop) {
> +    // We probably should never call this...

Let's get rid of the comment. I think we'll probably use this for the search service interposition that Mossop wants to do.
Attachment #8602065 - Flags: review?(wmccloskey) → review+
You need to log in before you can comment on or make changes to this bug.