Use BroadcastChannel to allow WebExtensions to communicate with Test Pilot by lmorchard · Pull Request #1022 · mozilla/testpilot

lmorchard · 2016-06-28T19:41:29Z

New lib/webextension-channels module to manage channels for every
enabled experiment
Each "channel" creates a background page in the WebExtension's origin
and listens for BroadcastChannel messages from the WebExtension
Example WebExtension that uses the BroadcastChannel

Fixes #433

groovecoder · 2016-06-28T19:44:10Z

I've got my stuff working with the old iframe postMessage branch for today's meeting/demo. But I'll look at switching to this tomorrow.

lmorchard · 2016-06-28T19:46:30Z

@groovecoder Yeah, I think this is what we're going to end up going with. It's way simpler, and doesn't require an iframe page - or even necessarily having a local Test Pilot server working.

groovecoder · 2016-06-28T19:48:38Z

+    nestedData: {
+      intData: 10,
+    },
+  });


Could use some elaboration on what the postMessage payload should contain. Just a JSON blob of data? Any required properties?

Same as the other mechanism - just a JS object with what you want included in the payload of the Telemetry ping. Test Pilot itself is agnostic on this, so it's more a concern between you & the metrics team to spec out.

That is to say, what you send here is the ... in the example ping shown in README-METRICS.md for the testpilottest ping - Test Pilot wraps that in the other envelope info and the full client environment data.

lmorchard · 2016-06-28T19:50:12Z

FWIW, this is largely based on @rpl's work experimenting with BroadcastChannel

groovecoder · 2016-06-29T18:34:42Z

I ran npm run package and tried to install the built .xpi file, but I got:

testpilot-addon:TypeError: can't convert undefined to object
Stack trace:
updateExperimentChannels@resource://gre/modules/commonjs/toolkit/loader.js -> resource://testpilot-addon/lib/webextension-channels.js:130:5
init@resource://gre/modules/commonjs/toolkit/loader.js -> resource://testpilot-addon/lib/webextension-channels.js:119:5
exports.main@resource://gre/modules/commonjs/toolkit/loader.js -> resource://testpilot-addon/index.js:579:3
run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/addon/runner.js:153:7
startup/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/addon/runner.js:87:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/addon/window.js:56:3
EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:652:5
AddonInterpositionService.prototype.interposeProperty/desc.value@resource://gre/components/multiprocessShims.js:160:52
@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/addon/window.js:54:1
evaluate@resource://gre/modules/commonjs/toolkit/loader.js:279:19
load@resource://gre/modules/commonjs/toolkit/loader.js:331:5
_require@resource://gre/modules/commonjs/toolkit/loader.js:653:16
require@resource://gre/modules/commonjs/toolkit/loader.js:607:12
startup/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/addon/runner.js:72:21
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
readURI/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/net/url.js:50:9
NetUtil_asyncFetch/<.onStopRequest@resource://gre/modules/NetUtil.jsm:128:17

Probably because I have/had no other installed add-ons when I installed this Test Pilot one?

groovecoder · 2016-06-29T18:45:24Z

Got the same error when I disabled and re-enabled this Test Pilot build while I had my add-on installed. 😞

I also tried adding a breakpoint at Metrics.onExperimentPing({ but it was never reached.

My new code is: https://github.com/mozilla/blok/compare/telemetry-ping-6?expand=1

lmorchard · 2016-06-29T20:12:55Z

Ugh, yeah, I have a general bad habit of not trying things in the blank slate state with a fresh profile. Just wrapped that code in a check whether the list of installed experiments is even defined yet, hopefully that improves things.

groovecoder · 2016-06-30T14:53:28Z

I don't get that error anymore, but I get a different error (twice) when loading the Test Pilot add-on now:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/PopupNotifications.jsm :: PopupNotifications_refreshPanel/< :: line 667"  data: no]
PopupNotifications_refreshPanel/<()
PopupNotifications.jsm:667
forEach()
self-hosted
PopupNotifications_refreshPanel()
PopupNotifications.jsm:616
PopupNotifications_showPanel()
PopupNotifications.jsm:728
PopupNotifications_update()
PopupNotifications.jsm:844
PopupNotifications_show()
PopupNotifications.jsm:394
gXPInstallObserver.showInstallConfirmation()
browser-addons.js:206
gXPInstallObserver.observe/showNotification()
browser-addons.js:383
gXPInstallObserver.observe()
browser-addons.js:399
notifyObservers()
amWebInstallListener.js:51
Installer.prototype.checkAllDownloaded()
amWebInstallListener.js:171
Installer()
amWebInstallListener.js:81
extWebInstallListener.prototype.onWebInstallRequested()
amWebInstallListener.js:335
buildNextInstall()
extensions.js:1293
gViewController.commands.cmd_installFromFile.doCommand/buildNextInstall/<()
extensions.js:1303
safeCall()
AddonManager.jsm:179
AddonManagerInternal.getInstallForFile/<.nextObject/<()
AddonManager.jsm:1955
this.XPIProvider.getInstallForFile/<()
XPIProvider.jsm:3892
makeSafe/<()
XPIProvider.jsm:2001
AddonInstall.prototype.initLocalInstall/</<()
XPIProvider.jsm:5317
makeSafe/<()
XPIProviderUtils.js:168
completeAddon()
XPIProviderUtils.js:157
this.AddonRepository.getCachedAddonByID<()
AddonRepository.jsm:578
next()
self-hosted
TaskImpl_run()
Task.jsm:319
TaskImpl()
Task.jsm:280
createAsyncFunction/asyncFunction()
Task.jsm:254
getRepositoryAddon()
XPIProviderUtils.js:159
this.XPIDatabase.getAddon/<()
XPIProviderUtils.js:1119
Handler.prototype.process()
Promise-backend.js:937
this.PromiseWalker.walkerLoop()
Promise-backend.js:816
bound walkerLoop()
self-hosted
bound bound walkerLoop()

I tried adding console.log statements ...

   // Rebuild channels for all known experiments
   updateExperimentChannels() {
     channels = {};
+    console.log('store: ' + store);
+    console.log('store.installedAddons: ' + store.installedAddons);
     if (store.installedAddons) {
       Object.keys(store.installedAddons).forEach(id => {
         const channel = new WebExtensionChannel(id);

But I don't see this messages in the browser toolbox console when I install the addon.xpi When should I expect the updateExperimentChannels "method" to be called?

lmorchard · 2016-06-30T20:43:15Z

Hmm, that error is confusing - doesn't mention any Test Pilot code anywhere, and I can't repro on a fresh Firefox profile. But it does look like it happens during the doorhanger to warn of an add-on installation, maybe?

As for updateExperimentChannels, it should get called at startup and whenever an experiment is enabled / disabled.

lmorchard · 2016-06-30T20:48:11Z

I tossed those console.log statements into my local copy. I see the messages on startup and on experiment enable - both in a fresh profile and in my daily dev browser.

lmorchard · 2016-06-30T20:54:33Z

@groovecoder all that said, your WebExtension code looks correct and a WebExtension shouldn't be able to cause these errors... so there's still probably something weird happening on the Test Pilot side 😞

groovecoder · 2016-07-01T18:46:05Z

I didn't get the error in a fresh profile.

Do I need to do something with my add-on code to make sure it's registered as a testpilot experiment? So I can make sure it's causing updateExperimentChannels to fire?

lmorchard · 2016-07-01T19:19:19Z

Do I need to do something with my add-on code to make sure it's registered as a testpilot experiment?

Oh, right! Yeah, you do: In your local testpilot.dev instance - which you do need, now that I'm thinking this through - you have to add your webextension as an experiment. And then, enable it from the local site. The add-on only cycles through known & installed experiments to set up BroadcastChannels, and it fetches that list via the API from the site.

lmorchard · 2016-07-01T19:22:14Z

This is also getting me thinking about coming up with a way to make this process simpler - because you shouldn't have to have a full local instance of the Test Pilot site just to develop a WebExtension.

Will probably take me some time, but we might be able to come up with just a stub JSON file to feed the Test Pilot add-on in the future.

Edit: #1029

groovecoder · 2016-07-05T13:48:32Z

Are there docs to add & enable my web extension as an experiment in testpilot.dev ?

lmorchard · 2016-07-05T14:08:34Z

No docs. You go to /admin/, login as admin with password admin, and add an Experiment record with an add-on id that matches your add-on

jaredhirsch · 2016-07-05T21:13:17Z

+  const docShell = addonChromeWebNav.QueryInterface(Ci.nsIInterfaceRequestor) // eslint-disable-line new-cap
+        .getInterface(Ci.nsIDocShell);
+  docShell.createAboutBlankContentViewer(principal);
+  const window = docShell.contentViewer.DOMDocument.defaultView;


@lmorchard Is this (lines 28-38) the best way to get a DOM window pointer for the given moz-extension URL? I guess I'm wondering if you found this snippet somewhere, or if it was suggested by someone on the FF team.

I asked about safely getting a DOM Window in #fx-team but got no response. I'm wondering if there might be a simpler or safer way--usually Gecko has N ways to do something, but only 1-3 ways that are considered good ways. I'll look further into it.

@6a68 This is mostly not my code, it comes from experiments with BroadcastChannel over here. Honestly, I didn't really know you could get a DOM window pointer for another origin from add-on code :)

Honestly, I didn't really know you could get a DOM window pointer for another origin from add-on code :)

@lmorchard Ah yeah, once you require('chrome') you effectively have rootly powers, and a lot of weird stuff becomes possible ^_^

After looking at this snippet for a while, and reading through https://bugzil.la/1186732, I'm much less scared of it. But it might be nice to document it for the sake of our future selves. Here's an attempt, take whatever seems useful, or skip it if it seems to make things more confusing overall:

function createChannelForAddonId(name, addonId) { // The BroadcastChannel API allows messaging between different windows that // share the same origin. Bug 1186732 extended this to WebExtensions (which // may not have an origin) by adding a special URL that loads an about:blank // page at the (generalized) "origin" of the extension. // // Load that about:blank page, and use its `window` to get a BroadcastChannel // that allows two-way communication between the main Test Pilot extension and // individual experiment extensions. // Note: the `targetExtensionUUID` is different for each copy of Firefox, // so that malicious websites can't guess the special URL associated with // an extension. const targetExtensionUUID = getExtensionUUID(addonId); // Create the special about:blank URL for the extension. const baseURI = Services.io .newURI(`moz-extension://${targetExtensionUUID}/_blank.html`, null, null); // Create a principal (security context) for the generalized origin given // by the extension's special URL and its `addonId`. const principal = Services.scriptSecurityManager .createCodebasePrincipal(baseURI, { addonId }); // Create a hidden window and open the special about:blank page for the // extension. const addonChromeWebNav = Services.appShell.createWindowlessBrowser(true); const docShell = addonChromeWebNav.QueryInterface(Ci.nsIInterfaceRequestor) // eslint-disable-line new-cap .getInterface(Ci.nsIDocShell); docShell.createAboutBlankContentViewer(principal); const window = docShell.contentViewer.DOMDocument.defaultView; // Finally, get the BroadcastChannel associated with the extension. const addonBroadcastChannel = new window.BroadcastChannel(name); // Callers need to keep the pointer to the window, otherwise the window's // BroadcastChannel will get garbage collected. return { addonChromeWebNav, addonBroadcastChannel }; }

Think I'll just plonk that on in - verbose commenting of this magic is a great addition, thanks!

jaredhirsch · 2016-07-05T21:20:35Z

@lmorchard I'm still trying to get docker sorted, but I read through the code and added some comments. I'll verify that stuff works tomorrow.

lmorchard · 2016-07-06T02:27:50Z

@6a68 I didn't write most of this stuff dealing with BroadcastChannel, so I mostly wanted to see if it works for our purposes. I don't really have my head totally wrapped around what all it does, so some patches / advice are welcome - because you probably know way more than I do :)

groovecoder · 2016-07-06T14:57:34Z

+    this.pingListeners.add(callback);
+
+    if (this.pingListeners.size >= 0) {
+      this.addonBroadcastChannel.addEventListener('message', this);


Confused by this ...

When the WebExtensionChannel has pingListener, the code here adds this as the message event listener. Does something automatically send/forward/bubble the message event to this.handleEvent, which actually calls notifyPing?

@groovecoder handleEvent is indeed a less-known, but standard, DOM API that I've seen used in Gecko code quite a bit:

https://developer.mozilla.org/en-US/docs/Web/API/EventListener

https://dxr.mozilla.org/mozilla-central/search?q=handleEvent&redirect=false

lmorchard · 2016-07-06T16:11:00Z

Updated to close BroadcastChannel while disposing the WebExtensionChannel

lmorchard · 2016-07-06T16:35:08Z

Could have sworn that this all worked when I tried it locally. But, looking at the event handling, I'm not sure how it did... hmm. Updated with a new commit to add/remove event handler with a bound function. Haven't tried this locally yet because meetings, but pushing my changes to the PR so other folks can look.

groovecoder · 2016-07-06T19:05:16Z

YES! 🎉 this.handleEventBound makes it work! When I click a disable reason button in my web extension, it sends a message to the broadcast channel, and test pilot calls Metrics.onExperimentPing !! Thanks for all the time on this, I think we have it working finally!

lmorchard · 2016-07-06T20:04:12Z

I think the last open issue might be what @6a68 raised about creating the DOM window pointer in the WebExtension's origin. If that's more a nit than a gotcha, we can merge this for next push

Good thing about this approach is we should be able to extend it to support @chuckharmston's new variant testing framework in WebExtensions, next.

jaredhirsch · 2016-07-06T21:17:17Z

+    this.addonChromeWebNav = addonChromeWebNav;
+    this.addonBroadcastChannel = addonBroadcastChannel;
+
+    this.handleEventBound = ev => this.handleEvent(ev);


This seems weird to me. I'll look into it this evening.

I could probably just do this.handleEvent.bind(this), but thought this looked clearer. I think the add/remove event methods need a consistent bound function ref to deal with. Though I did think handleEvent and passing this was enough.

I agree, there's probably something fishy here: the BroadcastChannel implementation of add/removeEventListener just wraps the default implementation. That said, it does work and it's readable, so I think we're fine to just leave it and move on.

…Pilot - New lib/webextension-channels module to manage channels for every enabled experiment - Each "channel" creates a background page in the WebExtension's origin and listens for BroadcastChannel messages from the WebExtension - Example WebExtension that uses the BroadcastChannel Fixes mozilla#433

jaredhirsch · 2016-07-07T05:06:53Z

@lmorchard I think this is good to land.

I ran out of time doing server setup and wasn't able to get the addons communicating, but it sounds like you and @groovecoder both saw it working, so it's probably fine. The channel creation snippet seems sane, and the rest of the code LGTM. 🍻

groovecoder · 2016-07-07T13:26:59Z

lmorchard · 2016-07-07T16:48:07Z

Seems like we've beaten this enough to merge it, so I'll go ahead & do that... now

lmorchard added the status: in progress label Jun 28, 2016

This was referenced Jun 28, 2016

(DO NOT MERGE) Issue #433: Dirty comm channel between main add-on and webextension #1008

Closed

add metrics docs internetarchive/wayback-machine-firefox#10

Merged

lmorchard force-pushed the 433-webextension-telemetry-2 branch from 0f38165 to 506f838 Compare June 28, 2016 19:45

groovecoder reviewed Jun 28, 2016
View reviewed changes

lmorchard force-pushed the 433-webextension-telemetry-2 branch from 506f838 to a66c3b9 Compare June 29, 2016 18:26

lmorchard force-pushed the 433-webextension-telemetry-2 branch from a66c3b9 to bb6f723 Compare June 29, 2016 20:11

jaredhirsch self-assigned this Jul 5, 2016

jaredhirsch reviewed Jul 5, 2016
View reviewed changes

groovecoder reviewed Jul 6, 2016
View reviewed changes

lmorchard force-pushed the 433-webextension-telemetry-2 branch 3 times, most recently from bb6f723 to 0c1b4b7 Compare July 6, 2016 16:10

lmorchard force-pushed the 433-webextension-telemetry-2 branch from 0c1b4b7 to 156c9bc Compare July 6, 2016 16:33

jaredhirsch reviewed Jul 6, 2016
View reviewed changes

lmorchard force-pushed the 433-webextension-telemetry-2 branch from 156c9bc to 7738ed4 Compare July 6, 2016 21:23

jaredhirsch removed their assignment Jul 7, 2016

lmorchard merged commit 9dc1621 into mozilla:master Jul 7, 2016

lmorchard removed the status: in progress label Jul 7, 2016

groovecoder mentioned this pull request Jul 13, 2016

Test Pilot & Web Extensions messages: prefValueRequest and addonListRequest #1068

Closed

groovecoder mentioned this pull request Oct 18, 2016

Shield Studies Utils as Embedded Web Extension mozilla/shield-studies-addon-utils#45

Closed

Conversation

lmorchard commented Jun 28, 2016

Uh oh!

groovecoder commented Jun 28, 2016

Uh oh!

lmorchard commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmorchard Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmorchard commented Jun 28, 2016

Uh oh!

groovecoder commented Jun 29, 2016

Uh oh!

groovecoder commented Jun 29, 2016

Uh oh!

lmorchard commented Jun 29, 2016

Uh oh!

groovecoder commented Jun 30, 2016

Uh oh!

lmorchard commented Jun 30, 2016

Uh oh!

lmorchard commented Jun 30, 2016

Uh oh!

lmorchard commented Jun 30, 2016

Uh oh!

groovecoder commented Jul 1, 2016

Uh oh!

lmorchard commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmorchard commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

groovecoder commented Jul 5, 2016

Uh oh!

lmorchard commented Jul 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmorchard Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhirsch commented Jul 5, 2016

Uh oh!

lmorchard commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmorchard commented Jul 6, 2016

Uh oh!

lmorchard commented Jul 6, 2016

Uh oh!

groovecoder commented Jul 6, 2016

Uh oh!

lmorchard commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredhirsch commented Jul 7, 2016

Uh oh!

groovecoder commented Jul 7, 2016

Uh oh!

lmorchard Jun 28, 2016 •

edited

Loading

lmorchard commented Jul 1, 2016 •

edited

Loading

lmorchard commented Jul 1, 2016 •

edited

Loading

lmorchard Jul 6, 2016 •

edited

Loading

lmorchard commented Jul 6, 2016 •

edited

Loading