1005193 - Allow add-ons to tell the add-on manager what their "main" global is

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

The console and scratchpad added in bug 993029 both run in the context of bootstrap.js. This is fine for some add-ons but not others (like SDK add-ons) where a different global would make more sense. It would be useful if add-ons could tell the add-on manager what global to use.

Comment 1

[Christopher Newhouse]

This would be great to have for debugging SDK Addons.  The ability to interact with the "background script" in Google Chrome Extensions during development via console is an invaluable tool.  This would help SDK Addon make development on Firefox much easier.

Comment 2

[Kris Maglione [:kmag]]

This is going to be necessary for WebExtensions, where the bootstrap.js context we default to is nearly useless.

Comment 4

[Johann Hofmann [:johannh]]

Taking this, wish me luck.

Comment 5

[Kris Maglione [:kmag]]

Good luck.

I'm completely unqualified to mentor this, but I'll try anyway. Let me know if you have any questions. If I can't answer them, I'll find someone who can.

Comment 6

[Dave Townsend [:mossop]]

I think we're going to need to finish bug 1249689 before we can implement this. I'm happy to mentor as far as I can (once we get into devtools territory we might need another).

Comment 7

[Dave Townsend [:mossop]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> Taking this, wish me luck.

A rough idea that I had here was to add an API using the instanceID added in bug 1249689 to allow the add-on to safely change its scope in the devtools to something new.

Comment 8

[Johann Hofmann [:johannh]]

Cool, thanks! I'll have to read into the code first anyway, so waiting for bug 1249689 should be fine. Please dump any ideas/pointers here, I'll definitely also have questions once I had time to look into it more closely.

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Allow addons to specify a custom global debug context

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8726161 [details] [diff] [review]
Allow addons to specify a custom global debug context

With a bit of trial and error I came to this solution. I like it because it's short and doesn't require changes in devtools. We're basically adding a new "context" field to the activeAddons map that can be modified by the respective AddonInternal. If no context was set it just defaults to the old bootstrap.js sandbox scope.

As you can see this is just WIP to check if I took the right approach or if there's something terribly wrong with it. At least on my machine it works pretty neatly for WebExtensions.

Mossop, what do you think?

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8726161 [details] [diff] [review]
Allow addons to specify a custom global debug context

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

This looks pretty good to me, aside from a few nits. It could use some tests, though.

::: toolkit/components/extensions/ext-backgroundPage.js
@@ +115,5 @@
>  extensions.on("manifest_background", (type, directive, extension, manifest) => {
>    let bgPage = new BackgroundPage(manifest.background, extension);
>    bgPage.build();
>    backgroundPagesMap.set(extension, bgPage);
> +  AddonManager.getAddonByInstanceID(extension.addonData.instanceID)

We should probably do this from BackgroundPage.build()

@@ +123,5 @@
>  extensions.on("shutdown", (type, extension) => {
>    if (backgroundPagesMap.has(extension)) {
>      backgroundPagesMap.get(extension).shutdown();
>      backgroundPagesMap.delete(extension);
> +    AddonManager.getAddonByInstanceID(extension.addonData.instanceID)

We should probably do this from BackgroundPage.shutdown()

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +4181,5 @@
>        return;
>  
>      for (let [id, val] of this.activeAddons) {
>        aConnection.setAddonOptions(
> +        id, { global: val.context || val.bootstrapScope });

It seems like it might be better to just set the initial value to the bootstrap scope.

@@ +4470,5 @@
>      this.persistBootstrappedAddons();
>      this.addAddonsToCrashReporter();
>  
>      this.activeAddons.set(aId, {
> +      context: null,

This should probably be called `debugContext` to match the Addon property.

@@ +7175,5 @@
>      return (addon._installLocation.name == KEY_APP_SYSTEM_DEFAULTS ||
>              addon._installLocation.name == KEY_APP_SYSTEM_ADDONS);
>    },
>  
> +  set debugContext(context) {

I think maybe `debugGlobal` would make the purpose clearer. Also, we shouldn't have a setter without a matching getter.

Can you add a doc comment, too?

@@ +7176,5 @@
>              addon._installLocation.name == KEY_APP_SYSTEM_ADDONS);
>    },
>  
> +  set debugContext(context) {
> +    XPIProvider.activeAddons.get(this.id).context = context;

We should check that the ID actually exists in `activeAddons`, so we don't throw an unexpected error if the add-on isn't running.

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Allow addons to specify a custom global debug context

Comment 13

[Johann Hofmann [:johannh]]

Thanks, I updated my patch. What's missing now are tests, and to be honest I'm not sure how to approach testing for this. I could add some assertion to the existing Mochitests for ext-background.js, but I don't know what/how exactly to check. Ideas?

Comment 14

[Alexandre Poirot [:ochameau]]

It would be great to have a very high level test in about:debugging.
It would check that everything works from end to end. But we don't have any test close to that.
And unfortunately, it is a challending test to write given that the toolbox lives in another process for addons. I'll try to write one, but I'm not sure it will be reasonably simple enough to be worth it.

It would be much easier to write a test like this one:
  http://mxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg_addon-console.js
But against a WebExtension Addon, with a background page (instead of addon4.xpi).
This test checks that the console receives the console.log message from bootstrap.js,
you could do something similar, but with the background page.
You may also execute arbitrary JS for additional assertions like this:
 let response = yield webConsole.evaluateJS("executeSomeBackgroundPageCodeWhichReturns42()");
 ok(response.result, 42);

Please ping me if you have any question/isssue.

Comment 15

[Alexandre Poirot [:ochameau]]

I'll provide a high level/integration test in bug 1261055.

Comment 17

[Johann Hofmann [:johannh]]

Hey, thanks for your help! I left this unresolved between vacations but I took a stab at testing it during the work week in Berlin and Luca(:rpl) suggested a similar solution but we ran into some issues, mostly about the mock of the DevTools Actor only initializing the parts required for testing debugging, not the web console. We thought it wasn't worth going this route and I went for a more unit-testy approach afterwards. I can upload it tomorrow and maybe you or :mag can take a look at the issues I've been having there. :)

A high level test would be great, agreed!

Comment 18

[Johann Hofmann [:johannh]]

Attachment: Allow addons to specify a custom global debug context

Comment 19

[Johann Hofmann [:johannh]]

Attachment: Allow addons to specify a custom global debug context

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8737351 [details] [diff] [review]
Allow addons to specify a custom global debug context

This is really quite a small test now, but it's also not a lot of functionality that's changing really. The test basically makes sure that everything goes right on extension side and doesn't care if the devtools console actually works correctly, I guess that's what we have Bug 1261055 for :)

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8737351 [details] [diff] [review]
Allow addons to specify a custom global debug context

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

::: toolkit/components/extensions/test/mochitest/test_ext_background_debug_global.html
@@ +47,5 @@
> +          is("test!", context.testThing, "global context is the background script context");
> +          resolve();
> +        }
> +      }
> +    });

I would not have expect such test, but it makes sense when reading your patch.
It is somewhat low level as it plugs in into the guts of AddonManager and devtools.
I'm wondering if it should live in toolkit/mozapps?

Please at least put a comment saying that it asserts XPIProvider.jsm code and not really devtools.
(connectionchange is being listen by XPIProvider)
Which isn't obvious. I first looked into devtools code and found nothing listening for this event.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8737351 [details] [diff] [review]
Allow addons to specify a custom global debug context

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

I don't want to expose debugGlobal on the public API like this, it means add-ons can potentially get easy access to another add-on's code. I think we want getAddonByInstanceID to return a more privileged AddonWrapper, it should be pretty easy to do something like this:

function PrivateWrapper(aAddon) {
  AddonWrapper.call(this, aAddon);
}

PrivateWrapper.prototype = Object.create(AddonWrapper.prototype);
Object.assign(PrivateWrapper.prototype, {
  // private functionality here
}

And you'll probably need a map to cache those for re-use.

Comment 23

[Johann Hofmann [:johannh]]

(In reply to Dave Townsend [:mossop] from comment #22)
> Comment on attachment 8737351 [details] [diff] [review]
> Allow addons to specify a custom global debug context
> 
> Review of attachment 8737351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't want to expose debugGlobal on the public API like this, it means
> add-ons can potentially get easy access to another add-on's code. I think we
> want getAddonByInstanceID to return a more privileged AddonWrapper, it
> should be pretty easy to do something like this:
> 
> function PrivateWrapper(aAddon) {
>   AddonWrapper.call(this, aAddon);
> }
> 
> PrivateWrapper.prototype = Object.create(AddonWrapper.prototype);
> Object.assign(PrivateWrapper.prototype, {
>   // private functionality here
> }
> 
> And you'll probably need a map to cache those for re-use.

Wow, yeah that sounds like a pretty glaring security issue. Good catch! Can we guarantee that getAddonByInstanceID can not be called from other add-ons with a different (guessed?) ID though?

Comment 24

[Johann Hofmann [:johannh]]

One thing that just came to my mind: we will not have any security issues if we remove the getter (no one's using it right now anyway) and just keep the setter (or just make it a function setDebugGlobal in that case). I'd personally think that's the better solution.

Comment 25

[Dave Townsend [:mossop]]

(In reply to Johann Hofmann [:johannh] from comment #23)
> (In reply to Dave Townsend [:mossop] from comment #22)
> > Comment on attachment 8737351 [details] [diff] [review]
> > Allow addons to specify a custom global debug context
> > 
> > Review of attachment 8737351 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't want to expose debugGlobal on the public API like this, it means
> > add-ons can potentially get easy access to another add-on's code. I think we
> > want getAddonByInstanceID to return a more privileged AddonWrapper, it
> > should be pretty easy to do something like this:
> > 
> > function PrivateWrapper(aAddon) {
> >   AddonWrapper.call(this, aAddon);
> > }
> > 
> > PrivateWrapper.prototype = Object.create(AddonWrapper.prototype);
> > Object.assign(PrivateWrapper.prototype, {
> >   // private functionality here
> > }
> > 
> > And you'll probably need a map to cache those for re-use.
> 
> Wow, yeah that sounds like a pretty glaring security issue. Good catch! Can
> we guarantee that getAddonByInstanceID can not be called from other add-ons
> with a different (guessed?) ID though?

We're passing the instanceID not the ID. The instanceID is a unique object that only the add-on has access to (until it passes it somewhere else of course) so that's about as much safety as we need.

(In reply to Johann Hofmann [:johannh] from comment #24)
> One thing that just came to my mind: we will not have any security issues if
> we remove the getter (no one's using it right now anyway) and just keep the
> setter (or just make it a function setDebugGlobal in that case). I'd
> personally think that's the better solution.

It would still allow another add-on to override an add-ons debug global. That's probably not an issue however there are other things that we've been talking about exposing in a private API so that the platform can get more access to add-on info safely so I'd much rather do the basic work to enable that here than retrofitting that later.

Comment 26

[Johann Hofmann [:johannh]]

Ok, I understand! Let me do that...

Comment 27

[Johann Hofmann [:johannh]]

Attachment: Allow addons to specify a custom global debug context

Comment 28

[Johann Hofmann [:johannh]]

Comment on attachment 8739852 [details] [diff] [review]
Allow addons to specify a custom global debug context

Added the PrivateWrapper to XPIProvider.jsm and added a comment based on :ochameaus feedback.

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 8739852 [details] [diff] [review]
Allow addons to specify a custom global debug context

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +7339,5 @@
> +    let activeAddon;
> +    if (activeAddon = XPIProvider.activeAddons.get(this.id)) {
> +      activeAddon.debugGlobal = global;
> +    }
> +  }

Presumably this only works before the debugger is opened. We might want a follow-up to notify the debugger of the change or something.

Comment 30

[Johann Hofmann [:johannh]]

Thanks for the review!

(In reply to Dave Townsend [:mossop] from comment #29)
> 
> Presumably this only works before the debugger is opened. We might want a
> follow-up to notify the debugger of the change or something.

You're probably right but if I'm not mistaken you can only open the debugger window if the addon was installed already, so it's a pretty hypothetical problem, no? This could only be useful if the addon wants to change its context "at runtime".

Comment 31

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20feb4b598ac

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/387bd412040f

Comment 33

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8665518&repo=fx-team

Comment 34

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/f98aad8ef492

Comment 35

[Johann Hofmann [:johannh]]

Attachment: Allow addons to specify a custom global debug context

Comment 36

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86e4d82d42fa

Comment 37

[Johann Hofmann [:johannh]]

This should be good now. I came to the realization that android doesn't have devtools and so this test would obviously fail. I disabled on Android and gave it another try spin, including Android.

Thanks! :)

Comment 38

[Carsten Book [:Tomcat]]

(In reply to Johann Hofmann [:johannh] from comment #37)
> This should be good now. I came to the realization that android doesn't have
> devtools and so this test would obviously fail. I disabled on Android and
> gave it another try spin, including Android.
> 
> Thanks! :)

thanks! pushed now!

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a78ccb08a836

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a78ccb08a836

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx48 Aurora release notes