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

RESOLVED WORKSFORME

Status

()

Firefox
Extension Compatibility
RESOLVED WORKSFORME
3 years ago
2 years ago

People

(Reporter: billm, Unassigned)

Tracking

(Blocks: 1 bug)

34 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [shims])

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?
Flags: needinfo?(dtownsend)
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?
Flags: needinfo?(dtownsend)
(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.
(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.
Yeah that looks ok
(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).
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)

Comment 7

3 years ago
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.
Blocks: 467520

Updated

3 years ago
Blocks: 1058542

Comment 8

3 years ago
(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.
Hmm, I'm surprised that works. Sandboxes are supposed to inherit their add-on ID from the caller if none is provided.
We're adding ShimWaiver in bug 1196975, so it might be better to switch to that once it's available.

Comment 11

3 years ago
(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.

Updated

2 years ago
Blocks: 1181835
See Also: → bug 1181835

Updated

2 years ago
No longer blocks: 1058542

Comment 12

2 years ago
triage team,  we likely need mossop for conversation - would there be a benefit to doing what is said in bug 1196975? limiting shim use?
Whiteboard: [shims]

Comment 13

2 years ago
We think this was fixed by bug 1196975.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.