1175770 - Experiment with new extension API

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

Attachment: patch

Hi Dave, I wanted to get something up here before Whistler so that we might have a chance to talk about it. Let me know if I can split this up in some way that would be helpful. I thought of splitting it by API, but I'm not sure if that would do any good.

Comment 1

[Nick Alexander :nalexander [he/him]]

I feel like this should be less browser/ specific.   In particular, mobile/android/ wants a lot of this API (as written), and will want extension points to remove and modify some pieces of this API.  A small amount of generalization will go a long way to providing an extensible multi-product API across the board.  That might be as simple as getting some boiler-plate and product-specific loading pints into toolkit/, or into a new, browser-product specific shared extension API location.  If we don't do this up front, I worry we'll build another browser.js-like situation, where sharing core browser functionality means copy-pasta everywhere.

I apologize for bringing some stop energy to a large and impressive patch -- this looks like a great start.

mfinkle, margaret: wanted to loop you in to this work.

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8623994 [details] [diff] [review]
patch

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

Lot's of stuff here and this isn't a full pass, I haven't looked at ExtensionAPI.jsm, ExtensionContent.jsm and ExtensionContentAPI.jsm yet. There is enough here that this is going to need a couple of passes, one for me to actually understand how everything fits together and another to actually find problems with that. I also didn't bother reviewing prepare.py, I think we should just support manifest.json natively. Most of this looks decent so far, lots of nit-picky stuff, I'd like to make sure we get naming consistent and self-explanatory up-front as that's been a problem with previous approaches. Of course we need a boat load of tests.

I agree we need a plan for other apps. I don't know what our add-ons plan is right now or how this fits in with it but we've constantly flip-flopped on mobile support for add-ons in the past, we need to make a firm decision there.

Up front I'd like to figure out integration with the add-on debugger. At the very least shared logging for stuff that can go wrong during extension execution, ideally making all the contexts show up in the debugger too.

Let's get together soon to talk about what we have here and what kind of a plan we want.

::: browser/components/browserextns/BrowserExtension.jsm
@@ +72,5 @@
> +// that runs in the chrome process. It's used for background pages
> +// (type="background"), popups (type="popup"), and any extension
> +// content loaded into browser tabs (type="tab").
> +//
> +// |window| is the DOM window the content runs in.

Let's call this contentWindow then

@@ +74,5 @@
> +// content loaded into browser tabs (type="tab").
> +//
> +// |window| is the DOM window the content runs in.
> +// |chromeWindow| is the XUL window associated with the content.
> +// It will be null for background pages.

parentWindow sounds more appropriate

@@ +76,5 @@
> +// |window| is the DOM window the content runs in.
> +// |chromeWindow| is the XUL window associated with the content.
> +// It will be null for background pages.
> +// |tab| is the XUL tab element associated with the content.
> +// It's only present for type="tab".

For supporting other apps it might be that the browser element is a better choice here.

@@ +80,5 @@
> +// It's only present for type="tab".
> +// |uri| is the URI of the content.
> +function ExtensionPage(ext, {type, window, chromeWindow, tab, uri})
> +{
> +  this.ext = ext;

Use "extension", it isn't too long and makes things clearer

@@ +139,5 @@
> +  // Map[URL prePath -> ext]. Determines which extension is
> +  // responsible for content under a particular URL prefix.
> +  baseURLs: new Map(),
> +
> +  init(ext) {

You never do anything with ext here, intentional?

@@ +169,5 @@
> +  injectInDocShell(docShell, ext, context) {
> +    this.docShells.set(docShell, {ext, context});
> +  },
> +
> +  injectFromBaseURL(baseURL, ext) {

Seems like you can just do this in the init method

@@ +173,5 @@
> +  injectFromBaseURL(baseURL, ext) {
> +    this.baseURLs.set(baseURL, ext);
> +  },
> +
> +  queryInterface: XPCOMUtils.generateQI([Ci.nsISupports, Ci.nsIObserver]),

Should be a capital Q, but this is probably unnecessary anyway, xpconnect will assume this is an nsIObserver because you pass it to addObserver

@@ +211,5 @@
> +      browser.removeEventListener("unload", listener);
> +      context.tab = null;
> +      context.close();
> +    };
> +    browser.addEventListener("unload", listener, true);

If I read this right if the extension's page navigates to a new URL we'll call context.close() which will close the tab. Expected?

@@ +220,5 @@
> +// as the associated popup.
> +function BrowserAction(options, ext)
> +{
> +  this.ext = ext;
> +  this.id = String(Math.random()).substr(2) + "-browser-action";

A 1 in a hundred chance of conflict doesn't sound great to me. Use a UUID or just a monotonically increasing counter.

@@ +280,5 @@
> +    if (title) {
> +      node.setAttribute("tooltiptext", title);
> +    } else {
> +      node.removeAttribute("tooltiptext");
> +    }

Does this set the title in the panel right?

@@ +290,5 @@
> +      node.removeAttribute("badge");
> +    }
> +
> +    function toHex(n) {
> +      return Math.floor(n/16).toString(16) + (n%16).toString(16);

Spaces around operators.

@@ +318,5 @@
> +        let panel = node.querySelector("panel");
> +        if (panel) {
> +          panel.remove();
> +        }
> +        node.removeAttribute("type");

I don't know if this causes XBL rebinding but we probably want to avoid it in case. Don't change the type attribute unnecessarily.

@@ +337,5 @@
> +        let browser = document.createElementNS(XUL_NS, "browser");
> +        browser.setAttribute("type", "content");
> +        browser.setAttribute("disableglobalhistory", "true");
> +        browser.setAttribute("width", "500");
> +        browser.setAttribute("height", "500");

iframe has a transparent attribute which might be useful. Any reason not to make this remote?

@@ +340,5 @@
> +        browser.setAttribute("width", "500");
> +        browser.setAttribute("height", "500");
> +        panel.appendChild(browser);
> +
> +        panel.addEventListener("load", () => {

Need to remove the event listener

@@ +394,5 @@
> +  // title, badge, etc. If it only changes a parameter for a single
> +  // tab, |tab| will be that tab. Otherwise it will be null.
> +  updateOnChange(tab) {
> +    if (tab) {
> +      this.updateWindow(tab.ownerDocument.defaultView);

You only need to do this if the tab is selected

@@ +443,5 @@
> +
> +  // tab is allowed to be null.
> +  getPopup(tab) {
> +    return this.popup.get(tab);
> +  },

Lots of identical methods here, might be able to simplify this.

@@ +476,5 @@
> +  },
> +};
> +
> +// Responsible for the background_page section of the manifest.
> +function BackgroundPage(options, ext)

Can this run in a child process?

@@ +531,5 @@
> +
> +  shutdown() {
> +    // Navigate away from the background page to invalidate any
> +    // setTimeouts or other callbacks.
> +    this.webNav.loadURI("about:blank", 0, null, null, null);

delete this.webNav, just to make sure we aren't leaving any cycles here

@@ +568,5 @@
> +    this.ext.alarms.delete(this);
> +    this.canceled = true;
> +  },
> +
> +  queryInterface: XPCOMUtils.generateQI([Ci.nsISupports, Ci.nsIObserver]),

Not needed

@@ +628,5 @@
> +    }
> +    this.ext.notifications.delete(this);
> +  },
> +
> +  queryInterface: XPCOMUtils.generateQI([Ci.nsISupports, Ci.nsIObserver]),

Again, no need for this.

@@ +637,5 @@
> +    }
> +
> +    if (this.ext.notificationCallback) {
> +      this.ext.notificationCallback(this);
> +    }

Should we call this.ext.notifications.delete(this) here?

@@ +646,5 @@
> +// comes directly from bootstrap.js when initializing.
> +function BrowserExtension(addonData)
> +{
> +  let domain = addonData.id;
> +  domain = domain.substring(1, domain.length - 1);

Huh?

@@ +652,5 @@
> +  // FIXME: Need to unregister this thing.
> +  let uri = addonData.resourceURI;
> +  let resourceHandler = Services.io.getProtocolHandler("resource");
> +  resourceHandler.QueryInterface(Ci.nsIResProtocolHandler);
> +  resourceHandler.setSubstitution(domain, uri);

Add-on IDs support characters that aren't valid in URIs (@ at least), you'll need to do some replacement first.

@@ +679,5 @@
> +
> +  this.hasShutdown = false;
> +  this.onShutdown = new Set();
> +
> +  GlobalManager.injectFromBaseURL(this.baseURI.prePath, this);

If we actually want to have separate construct and startup methods this should probably go into startup, or just see my note above about doing it from GlobalManager.init.

@@ +684,5 @@
> +}
> +
> +BrowserExtension.prototype = {
> +  get cloneScope() {
> +    return this.backgroundPage.window;

backgroundPage may be null

@@ +706,5 @@
> +
> +      if (!substitutions) {
> +        substitutions = [];
> +      }
> +      if (!(substitutions instanceof Array)) {

Use Array.isArray

@@ +722,5 @@
> +            return content;
> +          }
> +        }
> +      };
> +      return str.replace(/\$([A-Za-z0-9_@]+)\$/, replacer);

Not sure I'm parsing exactly what is going on here. Is this documented somewhere?

@@ +783,5 @@
> +           return;
> +         }
> +         let text = NetUtil.readInputStreamToString(inputStream, inputStream.available());
> +         resolve(JSON.parse(text));
> +       });

Seems like a common piece of code, pull it out into a shared function

@@ +818,5 @@
> +    let firstTry = locale;
> +    let secondTry = locale.substr(0, locale.indexOf("-"));
> +
> +    return this.readLocaleFile(firstTry).catch(() => this.readLocaleFile(secondTry)).catch(() => {});
> +  },

See the two functions here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#517

@@ +829,5 @@
> +
> +    if ("browser_action" in manifest) {
> +      this.browserAction = new BrowserAction(manifest.browser_action, this);
> +      this.browserAction.build();
> +    }

I think it's going to make sense to split the APIs themselves into another file and probably have some simple way to map an item in the manifest to an API class so we don't have to custom write code like this for every API.

@@ +861,5 @@
> +      this.localeMessages = messages;
> +      try {
> +        this.runManifest(manifest);
> +      } catch (e) {
> +        dump(`EXCEPTION: ${e} ${e.fileName} ${e.lineNumber}\n`);

The extension manager logs exceptions from startup calls so this might be unnecessary.

@@ +896,5 @@
> +    }
> +
> +    for (let notification of this.notifications) {
> +      notification.clear();
> +    }

All these should just add listeners for close

::: browser/components/browserextns/ExtensionCommon.jsm
@@ +26,5 @@
> +}
> +
> +// Run a function, cloning arguments into context.cloneScope, and
> +// report exceptions.
> +function runSafe(context, f, ...args)

I might expect that this also copies the function into the cloneScope. Maybe it should or maybe it should document that it doesn't.

@@ +37,5 @@
> +  return runSafeWithoutClone(f, ...args);
> +}
> +
> +// A simple list class that supports |remove(elt)| and |has(elt)|.
> +function RemovableList()

This just looks like Set. Is this a Chrome API thing?

@@ +81,5 @@
> +//   return () => {
> +//     // Return a way to unregister the listener.
> +//     SomehowUnregisterListener(listener);
> +//   };
> +// }).api()

Oof. Can we just use something like the event emitter pattern (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/event-emitter.js has one that could be moved somewhere shared)

@@ +105,5 @@
> +  addListener(callback) {
> +    if (!this.registered) {
> +      this.context.callOnClose(this);
> +
> +      let fireFunc = (...args) => this.fire(...args);

this.fire.bind(this)? I'm assuming that creates a smaller stack when debugging things.

@@ +182,5 @@
> +      let obj = Cu.createObjectIn(dest, {defineAs: prop});
> +      injectAPI(value, obj);
> +    } else {
> +      dest[prop] = value;
> +    }

This doesn't support getters/setters, not sure if that is a problem or not.

@@ +203,5 @@
> +// script.  On the parent side: one broker total, covering both the
> +// global MM and the ppmm. Message must be tagged with a recipient,
> +// which is an object with properties. Listeners can filter for
> +// messages that have a certain value for a particular property in the
> +// recipient.

Make it clear that if the message doesn't define the property then it matches the filter.

@@ +257,5 @@
> +        }
> +      }
> +
> +      if (pass) {
> +        listeners.push(listener);

Might as well just call the listener here

::: browser/components/browserextns/ExtensionStorage.jsm
@@ +39,5 @@
> +  },
> +
> +  read(extensionId) {
> +    if (this.cache.has(extensionId)) {
> +      return Promise.resolve(this.cache.get(extensionId));

This still allows for double-read if we call while the previous call is still finishing. Instead stick the promise into a cache and just return that.

@@ +53,5 @@
> +
> +      let extData = JSON.parse(decoder.decode(array));
> +      this.cache.set(extensionId, extData);
> +      return extData;
> +    }).catch(() => {

Should log an error here (I would love it if we could log to an extension specific console and tie this all together with the add-on debugger)

@@ +76,5 @@
> +    let array = encoder.encode(JSON.stringify(extData));
> +    let path = this.getStorageFile(extensionId);
> +    OS.File.makeDir(this.getExtensionDir(extensionId), {ignoreExisting: true, from: profileDir});
> +    let promise = OS.File.writeAtomic(path, array);
> +    return promise;

Probably needs an AsyncShutdown blocker

@@ +86,5 @@
> +    }
> +    return this.cache.get(extensionId);
> +  },
> +
> +  set(extensionId, items, value) {

What is the value argument for here?

@@ +89,5 @@
> +
> +  set(extensionId, items, value) {
> +    return new Promise(resolve => {
> +      this.read(extensionId).then(extData => {
> +        extData = this.checkCachedData(extensionId, extData);

Not sure this is necessary

@@ +139,5 @@
> +        let result = {};
> +        if (keys === null) {
> +          for (let prop in extData) {
> +            result[prop] = extData[prop];
> +          }

Object.assign(result, extData)

::: browser/components/browserextns/FrameManager.jsm
@@ +56,5 @@
> +    if (parentWindowId == windowId) {
> +      return -1;
> +    }
> +    return this.getId(parentWindowId);
> +  },

Totally confused by what these functions are doing. Needs docs.

::: browser/modules/E10SUtils.jsm
@@ +28,5 @@
> +  chromeProcessResourceURLs: new Set(),
> +
> +  loadResourceURIInChrome(host) {
> +    this.chromeProcessResourceURLs.add(host);
> +  },

Need a way to remove to properly support restartlessness

::: browser/themes/shared/browser.inc
@@ +13,5 @@
>  %define inAnyPanel :-moz-any(:not([cui-areatype="toolbar"]), [overflowedItem=true])
> +
> +.browser-action-panel > .panel-arrowcontainer > .panel-arrowcontent {
> +  padding: 0;
> +}

This file is only for defines, these rules should be put in the per-platform files

Comment 3

[[:fabrice] Fabrice Desré]

That's great!
We also want to use the same add-on APIs on b2g so we'll need to make that shareable among products. I'll sync up with Bill to see where I can help.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #2)
> I haven't looked at
> ExtensionAPI.jsm, ExtensionContent.jsm and ExtensionContentAPI.jsm yet.

Actually I talked about this patch with Bill last week, and it seems like a lot of these stuff relies on APIs I've designed/worked on so I'm going to take a look at them as well. Which is not a reason for you not to continue reviewing them too, just wanted to give you a heads-up about it.

Comment 5

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8623994 [details] [diff] [review]
patch

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

Still lot to read and think, but I thought I share my thoughts so far.

::: browser/components/browserextns/ExtensionAPI.jsm
@@ +1364,5 @@
> +    let chromeObj = Cu.createObjectIn(scope, {defineAs: "browser"});
> +    injectAPI(api(ext, context), chromeObj);
> +
> +    let XHR = function() {
> +      return Cu.createCustomXMLHttpRequest(ext.cloneScope, context.uri, true);

I don't think I understand the logic here based on what I've seen in the other related bug. This will change right? Or is there any reason why not to use the default XHR of the scope that has the principal with that special OriginAttribute?

::: browser/components/browserextns/ExtensionCommon.jsm
@@ +182,5 @@
> +      let obj = Cu.createObjectIn(dest, {defineAs: prop});
> +      injectAPI(value, obj);
> +    } else {
> +      dest[prop] = value;
> +    }

I think we could support getters/setters too with exportFunction if we really have to. I was thinking about if it can be an issue that we get xray vision on the arguments because of exportFunction... But I think we can fix them if we find any, so I'm fine with this.

::: browser/components/browserextns/ExtensionContent.jsm
@@ +111,5 @@
> +        Services.scriptloader.loadSubScript(url, sandbox);
> +      }
> +
> +      if (this.options.jsCode) {
> +        Cu.evalInSandbox(this.options.jsCode, sandbox);

I think it would be helpful to set the filename and lineno options here.

@@ +138,5 @@
> +                 .QueryInterface(Ci.nsIInterfaceRequestor);
> +  let mm = ir.getInterface(Ci.nsIContentFrameMessageManager);
> +  this.messageManager = mm;
> +
> +  this.sandbox = Cu.Sandbox([window], {sandboxPrototype: window, wantXrays: true});

wantXrays is true by default, and as we talked about this earlier we should check the principal of the window

Also, there was this annoying issue that various libraries that add-on developers tend to use seem to assume that window===this, and there was a hack for it in the SDK... (Bug 1021580) but I hope we can get away from that, I wonder how does that work in Chromium.

In general I wonder what are the differences between google's isolated world and our xray vision, I'm planning to look into it some time. Have you made some research on it? Things like window.onload from content script...

Another thing is if we use [window] here, what is the plan for XHR from content script? I guess we could require a special XHR ctor from the sandbox here and flag this principal too like the one on the parent side for the bg page?

Comment 6

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #1)
> I feel like this should be less browser/ specific.   In particular,
> mobile/android/ wants a lot of this API (as written), and will want
> extension points to remove and modify some pieces of this API.  A small
> amount of generalization will go a long way to providing an extensible
> multi-product API across the board.  That might be as simple as getting some
> boiler-plate and product-specific loading pints into toolkit/, or into a
> new, browser-product specific shared extension API location.  If we don't do
> this up front, I worry we'll build another browser.js-like situation, where
> sharing core browser functionality means copy-pasta everywhere.
> 
> I apologize for bringing some stop energy to a large and impressive patch --
> this looks like a great start.
> 
> mfinkle, margaret: wanted to loop you in to this work.

Bill and I chatted about this work earlier this week. I think we should try to keep the API implementation generic where it makes sense, such as the APIs that modify page content, but most APIs that interact with UI will need to have separate mobile/desktop implementations. I would think that most scripts that run in the content process could be shared.

I don't think we need to block landing an initial implementation on figuring all this out, but it would be good to have a directory structure that makes it clear what bits are shared vs. platform-specific.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8623994 [details] [diff] [review]
patch

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

::: browser/components/browserextns/ExtensionAPI.jsm
@@ +69,5 @@
> +    return -1;
> +  },
> +
> +  getTab(tabId) {
> +    // FIXME: Speed this up without leaking memory somehow.

You could use objects instead of numbers for the id, which would let you use these objects as keys in a WeakMap.

Comment 8

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

Attachment: patch v2

Here's a new version of the patch. I'll respond directly to some of the review comments in my next comment. I think I covered everything.

The main change was to move most of the code to toolkit and to break the API implementation into multiple files, which are loaded by the subscript loader. I also took advantage of all the work Bobby has done for moz-extension:// URIs, cross-origin XHRs from extensions, and web_accessible_content.

Dave, I'll be out tomorrow. Can we talk about this on Wednesday?

(Also, canceling Mark's needinfo. I already talked to Margaret. We're going to try to make this work on Android, but it's not a top priority.)

Comment 9

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

(In reply to Dave Townsend [:mossop] from comment #2)
> I also didn't bother reviewing prepare.py,
> I think we should just support manifest.json natively.

I agree. However, that will be a good deal more work involving the add-on manager, which I don't know well. I really want to get something landed soon so other people can start contributing. It's very difficult for multiple people to work on this bug if nothing is checked in.

> Most of this looks
> decent so far, lots of nit-picky stuff, I'd like to make sure we get naming
> consistent and self-explanatory up-front as that's been a problem with
> previous approaches. Of course we need a boat load of tests.

I've tried to make the naming more consistent. Also working on tests.

> Up front I'd like to figure out integration with the add-on debugger. At the
> very least shared logging for stuff that can go wrong during extension
> execution, ideally making all the contexts show up in the debugger too.

I have no idea how to do this. Maybe you could point me to some code? I had expected that add-on debugging would just work, but the only script it shows is "bootstrap.js". I'm also not sure what console I'm getting.

> For supporting other apps it might be that the browser element is a better
> choice here.

I think other apps are going to need a completely different implementation here, so I'm not sure it matters. It shouldn't be too hard to switch to browser though.

> iframe has a transparent attribute which might be useful. Any reason not to
> make this remote?

Making it remote means that everything has to be remote since all the various extension pages can get references to each other's windows. That's going to be a good bit more work, but it's definitely on my list.

> @@ +861,5 @@
> > +      this.localeMessages = messages;
> > +      try {
> > +        this.runManifest(manifest);
> > +      } catch (e) {
> > +        dump(`EXCEPTION: ${e} ${e.fileName} ${e.lineNumber}\n`);
> 
> The extension manager logs exceptions from startup calls so this might be
> unnecessary.

I think I ran into a problem where the log went to the console, which wouldn't display for some reason because of the exception.

> @@ +37,5 @@
> > +  return runSafeWithoutClone(f, ...args);
> > +}
> > +
> > +// A simple list class that supports |remove(elt)| and |has(elt)|.
> > +function RemovableList()
> 
> This just looks like Set. Is this a Chrome API thing?

I was not aware that Set is ordered. Fixed.

> @@ +81,5 @@
> > +//   return () => {
> > +//     // Return a way to unregister the listener.
> > +//     SomehowUnregisterListener(listener);
> > +//   };
> > +// }).api()
> 
> Oof. Can we just use something like the event emitter pattern
> (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/event-
> emitter.js has one that could be moved somewhere shared)

Well, Chrome uses addListener/removeListener, so the API is kind of fixed. In terms of the implementation, EventManager does a lot of stuff that's not in the code you referenced:
- It takes care of unregistering stuff when the extension is unloaded.
- It consolidates event registration. If there are three calls to addListener in the same extension, EventManager takes care of calling the registration function only once. That's kind of nice in a couple places.

> @@ +182,5 @@
> > +      let obj = Cu.createObjectIn(dest, {defineAs: prop});
> > +      injectAPI(value, obj);
> > +    } else {
> > +      dest[prop] = value;
> > +    }
> 
> This doesn't support getters/setters, not sure if that is a problem or not.

I don't think it's a problem. If we need them, we can add support.

> @@ +257,5 @@
> > +        }
> > +      }
> > +
> > +      if (pass) {
> > +        listeners.push(listener);
> 
> Might as well just call the listener here

I think that can break if the listener adds or removes listeners. I could be wrong though.

Comment 10

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

> > Up front I'd like to figure out integration with the add-on debugger. At the
> > very least shared logging for stuff that can go wrong during extension
> > execution, ideally making all the contexts show up in the debugger too.

> I have no idea how to do this. Maybe you could point me to some code? I had expected that
> add-on debugging would just work, but the only script it shows is "bootstrap.js". I'm also
> not sure what console I'm getting.

Actually, I took a closer look and I think it should be easy to fix this for code running in the chrome process. I don't know how to do it for content scripts, though. We don't currently have any debugger UI (that I know of) that talks to actors in two different processes. That's why we have the extremely awkward Browser Toolbox/Browser Content Toolbox distinction.

Alexandre, how hard do you think it would be to make it so that the "Debug" button for an add-on talks to add-on code in the chrome process and content scripts in content processes?

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Bill McCloskey (:billm) from comment #10)
> > > Up front I'd like to figure out integration with the add-on debugger. At the
> > > very least shared logging for stuff that can go wrong during extension
> > > execution, ideally making all the contexts show up in the debugger too.
> 
> > I have no idea how to do this. Maybe you could point me to some code? I had expected that
> > add-on debugging would just work, but the only script it shows is "bootstrap.js". I'm also
> > not sure what console I'm getting.
> 
> Actually, I took a closer look and I think it should be easy to fix this for
> code running in the chrome process. I don't know how to do it for content
> scripts, though. We don't currently have any debugger UI (that I know of)
> that talks to actors in two different processes. That's why we have the
> extremely awkward Browser Toolbox/Browser Content Toolbox distinction.
> 
> Alexandre, how hard do you think it would be to make it so that the "Debug"
> button for an add-on talks to add-on code in the chrome process and content
> scripts in content processes?

Hard.

The toolbox is designed around client/target abstraction which connects to one specific process/context. I already hacked around these abstractions in order to support iframe-switching so that you can switch the whole toolbox context from top-level document to any other document (iframe, or another top-level window) but that all works in the same process. Supporting contexts in another process is even more complex as we are spawing another "DebuggerServer" in each process, with new client/target.

But given that today, for existing frame script or SDK content script, you already have to use the Browser Content Toolbox... I'm tempted to say that it isn't really related to your new work, but more a general Devtools-codebase limitation.

If that's really important, we should make this a priority in devtools group as I'm expecting it to be a quite significant work.

Otherwise, for chrome scripts to appear in existing "Chrome/Parent-process addon toolbox", I think you just need to use Sandboxes for addon scripts and flag them with addonID metadata, like this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4516

Comment 12

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

Attachment: ext.zip

Here's a very simple extension that should even work in b2g.

Comment 13

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

Attachment: initial tests

This is an initial set of tests just for Fabrice to look at.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8639607 [details] [diff] [review]
patch v2

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

Reviewed the main parts but not the ext-* scripts. Most of this is pretty clean stuff and I don't see a lot wrong with it.

The main concern I have is the wealth of event mechanisms we're using here. Management has event-emitter style events, other code uses callOnClose and then there is the EventManager and SingletonEventManager. The latter I find particularly confusing, I can grok it after I've looked at it for a few minutes but I'm pretty sure that unless I was looking constantly I'd forget pretty quickly. Consistency is important for developing maintainable code so I'd like to think about whether we can use a consistent style throughout. It should be pretty easy to convert the callOnClose to use a similar setup to Management (note we have a well-tested event-emitter in tree: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/event-emitter.js). Not sure about the other stuff and my brain is done for the day so I'll think more on it tomorrow.

::: browser/base/content/browser.js
@@ +972,5 @@
>      BrowserOnClick.init();
>      DevEdition.init();
>      AboutPrivateBrowsingListener.init();
>      TrackingProtection.init();
> +    ExtensionManagement.initWindow();

I don't see the point in this. In almost all cases we init on startup and uninit on shutdown, why do we need explicit calls? Just init as soon as ExtensionManagement is loaded and probably don't even need to bother to uninit on shutdown.

::: toolkit/components/extensions/Extension.jsm
@@ +442,5 @@
> +  readLocaleMessages() {
> +    let uri = Services.io.newURI("_locales", null, this.baseURI);
> +    if (!(uri instanceof Ci.nsIFileURL)) {
> +      return {};
> +    }

So this doesn't work for packed extensions?

@@ +511,5 @@
> +  },
> +
> +  startup() {
> +    GlobalManager.init(this);
> +    Management.emit("startup", this);

At this point there isn't any information about the extension available. Would it be better to call this after the manifest has been read?

@@ +551,5 @@
> +    ExtensionManagement.shutdownExtension(this.uuid);
> +
> +    let handler = Services.io.getProtocolHandler("moz-extension");
> +    handler.QueryInterface(Ci.nsISubstitutingProtocolHandler);
> +    handler.setSubstitution(this.uuid, null);

ExtensionManagement.shutdownExtension does this already

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +141,5 @@
> +      if (this.options.cssCode) {
> +        let url = "data:text/css;charset=utf-8," + encodeURIComponent(this.options.cssCode);
> +        runSafe(winUtils.loadSheetUsingURIString, url, winUtils.AUTHOR_SHEET);
> +      }
> +    }

I don't think these stylesheets will be loaded when an extension is started after the window has already loaded.

@@ +152,5 @@
> +      }
> +
> +      if (this.options.jsCode) {
> +        Cu.evalInSandbox(this.options.jsCode, sandbox);
> +      }

I remember hitting some weird cases with bootstrap.js where code in the sandbox didn't necessarily run with the latest JS features, we ended up needing this hack to make it work: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4548. Check that extension code can use features like "let" etc.

@@ +255,5 @@
> +      this.windows.delete(window);
> +
> +      this.trigger("document_start", window);
> +      window.addEventListener("DOMContentLoaded", this, true);
> +      window.addEventListener("load", this, true);

Do these also give us events for inner-frames? If so we'll need to filter those out in handleEvent

@@ +419,5 @@
> +  init(global) {
> +    if (this.globalCount == 0) {
> +      ExtensionManager.init();
> +      DocumentManager.init();
> +    }

Do we ever start a process without a frame? Given that the only way this module is loaded is by the first frame in a process might as well just init these immediately.

@@ +438,5 @@
> +  uninit(global) {
> +    this.globalCount--;
> +    if (this.globalCount == 0) {
> +      ExtensionManager.uninit();
> +      DocumentManager.uninit();

Don't we close the process when the last frame goes away? Do we need to do this uninit work in that case?

::: toolkit/components/extensions/ExtensionManagement.jsm
@@ +88,5 @@
> +      this.topWindowIds.add(data.windowId);
> +      break;
> +
> +    case "Extension:RemoveTopWindowID":
> +      this.topWindowIds.delete(data.windowId);

How much trouble do we get into if the child process crashes and we don't receive this message?

@@ +155,5 @@
> +    this.aps.setAddonLoadURICallback(extension.id, null);
> +
> +    let handler = Services.io.getProtocolHandler("moz-extension");
> +    handler.QueryInterface(Ci.nsISubstitutingProtocolHandler);
> +    handler.setSubstitution(this.uuid, null);

s/this.uuid/uuid/

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +318,5 @@
> +
> +    case "Extension:Connect":
> +      this.runListeners("connect", target, data);
> +      break;
> +    }

You could just use a single message here and put type in the data. Would make it easier to add new messages in the future.

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 8639607 [details] [diff] [review]
patch v2

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

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +395,5 @@
> +
> +  receiveMessage({name, data}) {
> +    let extension;
> +    switch (name) {
> +    case "Extensions:Startup":

should be Extension:Startup

@@ +401,5 @@
> +      this.extensions.set(data.id, extension);
> +      DocumentManager.startupExtension(data.id);
> +      break;
> +
> +    case "Extensions:Shutdown":

should be Extension:Shutdown

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 8639607 [details] [diff] [review]
patch v2

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

::: browser/components/extensions/ext-browserAction.js
@@ +23,5 @@
> +// as the associated popup.
> +function BrowserAction(options, extension)
> +{
> +  this.extension = extension;
> +  this.id = (nextActionId++) + "-browser-action";

Can we use the add-on ID here instead? It might when debugging the UI to know where a button came from.

@@ +62,5 @@
> +        node.addEventListener("command", event => {
> +          if (node.getAttribute("type") != "panel") {
> +            let window = event.originalTarget.ownerDocument.defaultView;
> +            let custom = new window.CustomEvent("ExtensionClick",
> +                                                {detail: {extension: this.extension}, bubbles: true});

Why use DOM events for this rather than just having the listening code below register on the object directly somehow?

@@ +103,5 @@
> +                                        'class', 'toolbarbutton-badge');
> +    if (badgeNode) {
> +      let color = this.badgeBackgroundColor.get(tab);
> +      if (color) {
> +        color = "#" + toHex(color[0]) + toHex(color[1]) + toHex(color[2]);

Could just do rgb(r, g, b) instead of converting to hex.

@@ +123,5 @@
> +      if (panel) {
> +        panel.remove();
> +      }
> +
> +      let popupURL = this.extension.baseURI.resolve(popup);

You don't need this if popup is null

@@ +314,5 @@
> +            r = color.substr(1, 2);
> +            g = color.substr(3, 2);
> +            b = color.substr(5, 2);
> +          }
> +          color = [parseInt(r, 16), parseInt(g, 16), parseInt(b, 16)];

Here you convert a valid CSS color to rgb values, then the code that uses it converts it back to a CSS color. Why not just pass through?

::: browser/components/extensions/ext-tabs.js
@@ +118,5 @@
> +            }
> +          } else if (event.type == "TabPinned") {
> +            changeInfo.pinned = true;
> +            needed = true;
> +          } else if (event.true == "TabUnpinned") {

s/event.true/event.type/

@@ +142,5 @@
> +                status = "complete";
> +              }
> +            } else if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
> +                       statusCode == Cr.NS_BINDING_ABORTED) {
> +              status = "complete";

I think this will flag complete when an inner load is cancelled for some reason.

@@ +157,5 @@
> +            let gBrowser = browser.ownerDocument.defaultView.gBrowser;
> +            let tab = gBrowser.getTabForBrowser(browser);
> +            let tabId = TabManager.getId(tab);
> +            let changeInfo = {url: locationURI.spec};
> +            fire(tabId, sanitize(extension, changeInfo), TabManager.convert(extension, tab));

Should we still fire this if the changeInfo is now empty because it has been sanitized away?

@@ +206,5 @@
> +        if (!createProperties) {
> +          createProperties = {};
> +        }
> +
> +        let url = createProperties.url || "about:newtab";

The default new tab can be overridden so presumably we should use whatever it is meant to be.

@@ +253,5 @@
> +        }
> +      },
> +
> +      remove: function(tabs, callback) {
> +        if (!(tabs instanceof Array)) {

Use Array.isArray

::: browser/components/extensions/ext-utils.js
@@ +217,5 @@
> +      return;
> +    }
> +
> +    for (let listener of this._openListeners) {
> +      listener(event.target.defaultView);

Just use window here

::: browser/components/extensions/ext-windows.js
@@ +76,5 @@
> +
> +        let args;
> +        if ("url" in createData) {
> +          args = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray);
> +          if (createData.url instanceof Array) {

Use Array.isArray

@@ +97,5 @@
> +          }
> +        }
> +
> +        let window = Services.ww.openWindow(null, "chrome://browser/content/browser.xul", "_blank",
> +                                            "chrome,dialog=no,all" + extraFeatures, args);

Does this wait for the load event before returning, should we wait for that before calling the callback?

@@ +129,5 @@
> +
> +      remove: function(windowId, callback) {
> +        let window = WindowManager.getWindow(windowId);
> +        window.close();
> +        runSafe(extension, callback);

In the SDK code we hit many problems with jumping back to the extension before the window was truly closed, the windows API would still return the closing window for example. Can we wait for the domwindowclosed event before calling the callback to make sure we don't hit that.

::: toolkit/components/extensions/ext-notifications.js
@@ +75,5 @@
> +
> +extensions.registerPrivilegedAPI("notifications", (extension, context) => {
> +  return {
> +    notifications: {
> +      _nextId: 1,

This is unused

::: toolkit/components/extensions/ext-runtime.js
@@ +29,5 @@
> +        } else {
> +          [extensionId, message, options, responseCallback] = args;
> +        }
> +        let recipient = {extensionId: extensionId ? extensionId : extension.id};
> +        return context.messenger.sendMessage(Services.cpmm, message, recipient, responseCallback);

Isn't this running in the main process, in which case wouldn't we use Services.ppmm?

::: toolkit/components/extensions/ext-webNavigation.js
@@ +21,5 @@
> +      if (!data.browser) {
> +        return;
> +      }
> +
> +      let tabId = TabManager.getBrowserId(data.browser);

This assumes we're running in Firefox. Anything we can do when TabManager isn't around or are we just assuming Fennec will define it?

::: toolkit/components/extensions/ext-webRequest.js
@@ +20,5 @@
> +      if (!data.browser) {
> +        return;
> +      }
> +
> +      let tabId = TabManager.getBrowserId(data.browser);

Ditto

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 8639607 [details] [diff] [review]
patch v2

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

I don't think I'm going to spot more problems at this point, aside from the obvious bugs I've noted we should land this and take the other bits as follow-ups

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3821b236f9

Comment 19

[[:fabrice] Fabrice Desré]

\o/

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4e3821b236f9 since one of the 2 changes seems to have caused:

for causing test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12405582&repo=mozilla-inbound

Comment 21

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/976ba676a659

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0364858aed56

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0364858aed56