1167802 - Provide a way to opt out of e10s shims even when add-on is not e10s-compatible

Status: RESOLVED WORKSFORME

(Firefox :: Extension Compatibility, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

There appear to be places in Jetpack where we perform an operation that triggers the shims, but we don't want shims. Ideally we would disable the shims for the entire add-on, but we may still need them in some places (deprecated APIs for example).

Some examples:
* hotkeys module registers key events on chrome window that may not need to listen for content key events
* document-element-inserted observer on panels in the parent process

We can actually do this without any platform changes at all I think. We just need to create a JSM that accesses properties for add-ons. Then you could do something like this:

ShimWaiver.getProperty(Services.obs, "addObserver")(this, "document-element-inserted", false);

One thing I worry about is that this sort of thing will be hard to use in Jetpack. Registering an event listener or observer seems to go through several levels of indirection before it finally does the registration. It would be unfortunate if we had to pass some sort of useShims flag through each of those calls. Dave, do you have any thoughts on this?

Comment 1

[Dave Townsend [:mossop]]

This is basically bug 1132767 and I came to about the same conclusion. At the time I had hoped we would quickly get enough of the SDK e10s compliant to be able to just disable the shims entirely.

Maybe we're over-thinking this. What if the SDK code just registered for observer notifications with the topic "noshim-...." and the shim just strip the front off and register with the normal observer service rather than doing shimming in those cases?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Dave Townsend [:mossop] from comment #1)
> Maybe we're over-thinking this. What if the SDK code just registered for
> observer notifications with the topic "noshim-...." and the shim just strip
> the front off and register with the normal observer service rather than
> doing shimming in those cases?

Yeah, I guess that would work. We'd need to strip those off in the default shim too, which would slow down normal code paths. But maybe that's the path of least resistance.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Dave Townsend [:mossop] from comment #1)
> Maybe we're over-thinking this. What if the SDK code just registered for
> observer notifications with the topic "noshim-...." and the shim just strip
> the front off and register with the normal observer service rather than
> doing shimming in those cases?

Actually, the easiest way is probably to strip off the "noshim-" part in the SDK right before the call to addObserver/addEventListener. Then we could do the ShimWaiver trick in comment 0 to actually do the registration.

The ShimWaiver module would just need to look like this:

let ShimWaiver = {
  getProperty(obj, prop) {
    return obj[prop];
  }
};

We could stick it in toolkit/components/addoncompat/ShimWaiver.jsm or something.

Comment 4

[Dave Townsend [:mossop]]

Yeah that looks ok

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Bill McCloskey (:billm) from comment #3)
> (In reply to Dave Townsend [:mossop] from comment #1)
> > Maybe we're over-thinking this. What if the SDK code just registered for
> > observer notifications with the topic "noshim-...." and the shim just strip
> > the front off and register with the normal observer service rather than
> > doing shimming in those cases?
> 
> Actually, the easiest way is probably to strip off the "noshim-" part in the
> SDK right before the call to addObserver/addEventListener. Then we could do
> the ShimWaiver trick in comment 0 to actually do the registration.
> 
> The ShimWaiver module would just need to look like this:
> 
> let ShimWaiver = {
>   getProperty(obj, prop) {
>     return obj[prop];
>   }
> };
> 
> We could stick it in toolkit/components/addoncompat/ShimWaiver.jsm or
> something.

I'm not quite sure how this would work, or what it would do for us for the hotkeys for example (bug 1168597).

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Let's assume for now that hotkeys doesn't need the shims. I don't think it does, but I could be wrong. The first thing to do is to check.

The basic idea is to figure out which events shouldn't use the shims and then make sure we don't use shims for those events. Th first part can be accomplished by giving the events a special name in Jetpack, like "noshims-keydown". The second part can be accomplished by registering the listeners for those events in a non-addon scope. Since non-addons don't get shims, that ensures we won't get shims for those events.

Here's a rough plan for hotkeys:

* Change the keyboard observer here:
  https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/keyboard/observer.js#44
so that it passes modified event names, like "noshims-keydown", "noshims-keyup", "noshims-keypress".

* Change the event code here:
  https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/dom/events.js#47
to check if the type begins with "noshims-". If it does, then we want to avoid using the shims for the event. Right now the shims are triggered when we get the "addEventListener" property on any event target in an add-on. To avoid that from happening, we can ask some non-addon code to fetch the addEventListener property for us. Using the ShimWaiver module I described above, that would look like this:
  ShimWaiver.getProperty(element, "addEventListener")(type, listener, capture);
where type has already had the "noshims-" part stripped off.

* Write a little ShimWaiver JSM with the code in comment 3. It could go in toolkit/components/addoncompat. Its only purpose is to run code in a non-addon scope so that the shims don't get used.

Comment 7

[Wladimir Palant]

SDK isn't the only place where this is needed. I'm attempting prevent to the about: protocol handler registered by Adblock Plus from being shimmmed - so far without success. It's overriding the handler I've registered in the child process, and cannot be turned off it seems. Note that Adblock Plus in general is still far from being able to work without any shims whatsoever.

Comment 8

[Wladimir Palant]

(In reply to Bill McCloskey (:billm) from comment #3)
> The ShimWaiver module would just need to look like this:
> 
> let ShimWaiver = {
>   getProperty(obj, prop) {
>     return obj[prop];
>   }
> };

I managed to implement this functionality without any not yet existent modules:

function getPropertyWithoutCompatShims: function(obj, prop)
{
  let sandbox = Cu.Sandbox(systemPrincipal);
  sandbox.obj = obj;
  sandbox.prop = prop;
  return Cu.evalInSandbox("obj[prop]", sandbox);
}

Using this to retrieve Cm.registerFactory solves the issue for me.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hmm, I'm surprised that works. Sandboxes are supposed to inherit their add-on ID from the caller if none is provided.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We're adding ShimWaiver in bug 1196975, so it might be better to switch to that once it's available.

Comment 11

[Wladimir Palant]

(In reply to Bill McCloskey (:billm) from comment #9)
> Hmm, I'm surprised that works. Sandboxes are supposed to inherit their
> add-on ID from the caller if none is provided.

Yes, I was surprised as well - I actually expected that I would have to pass in a bogus add-on ID. But for some reason it does work.

Comment 12

[:shell escalante]

triage team,  we likely need mossop for conversation - would there be a benefit to doing what is said in bug 1196975? limiting shim use?

Comment 13

[Andy McKay]

We think this was fixed by bug 1196975.