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

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: mossop, Assigned: johannh, Mentored)

Tracking

(Blocks: 3 bugs, {dev-doc-needed})

unspecified
mozilla48
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed, relnote-firefox 48+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

3 years ago
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.
This is going to be necessary for WebExtensions, where the bootstrap.js context we default to is nearly useless.
Blocks: 1161828

Updated

2 years ago
Blocks: 1226743

Updated

2 years ago
Duplicate of this bug: 1245718

Updated

2 years ago
Blocks: 1245718
Taking this, wish me luck.
Assignee: nobody → mail
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.
Mentor: kmaglione+bmo@mozilla.com
(Reporter)

Comment 6

2 years ago
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).
Mentor: kmaglione+bmo@mozilla.com → dtownsend@mozilla.com
Depends on: 1249689
(Reporter)

Comment 7

2 years ago
(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.
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.
Created attachment 8726161 [details] [diff] [review]
Allow addons to specify a custom global debug context
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?
Attachment #8726161 - Flags: feedback?(kmaglione+bmo)
Attachment #8726161 - Flags: feedback?(dtownsend)
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.
Attachment #8726161 - Flags: feedback?(kmaglione+bmo) → feedback+
Created attachment 8726961 [details] [diff] [review]
Allow addons to specify a custom global debug context
Attachment #8726161 - Attachment is obsolete: true
Attachment #8726161 - Flags: feedback?(dtownsend)
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?
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.
I'll provide a high level/integration test in bug 1261055.

Updated

a year ago
Duplicate of this bug: 1245718
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!
Created attachment 8737343 [details] [diff] [review]
Allow addons to specify a custom global debug context
Attachment #8726961 - Attachment is obsolete: true
Created attachment 8737351 [details] [diff] [review]
Allow addons to specify a custom global debug context
Attachment #8737343 - Attachment is obsolete: true
Attachment #8737351 - Flags: review?(dtownsend)
Attachment #8737351 - Flags: feedback?(poirot.alex)
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 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.
Attachment #8737351 - Flags: feedback?(poirot.alex) → feedback+
(Reporter)

Comment 22

a year ago
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.
Attachment #8737351 - Flags: review?(dtownsend)
(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?
Flags: needinfo?(dtownsend)
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.
(Reporter)

Comment 25

a year ago
(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.
Flags: needinfo?(dtownsend)
Ok, I understand! Let me do that...
Created attachment 8739852 [details] [diff] [review]
Allow addons to specify a custom global debug context
Attachment #8737351 - Attachment is obsolete: true
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.
Attachment #8739852 - Flags: review?(dtownsend)
(Reporter)

Comment 29

a year ago
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.
Attachment #8739852 - Flags: review?(dtownsend) → review+
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".
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20feb4b598ac
Keywords: checkin-needed

Comment 32

a year ago
https://hg.mozilla.org/integration/fx-team/rev/387bd412040f
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8665518&repo=fx-team
Flags: needinfo?(jhofmann)

Comment 34

a year ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/f98aad8ef492
Created attachment 8741335 [details] [diff] [review]
Allow addons to specify a custom global debug context
Attachment #8739852 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86e4d82d42fa
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! :)
Flags: needinfo?(jhofmann) → needinfo?(cbook)
(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!
Flags: needinfo?(cbook)

Comment 39

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

Comment 40

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a78ccb08a836
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1265447
Added to Fx48 Aurora release notes
relnote-firefox: --- → 48+
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.