(DO NOT MERGE) Issue #433: Dirty comm channel between main add-on and webextension by lmorchard · Pull Request #1008 · mozilla/testpilot

lmorchard · 2016-06-21T20:59:54Z

New /postmessage-proxy page served by Django
Main add-on loads /postmessage-proxy into a background page worker in
an iframe
Example webextension loads /postmessage-proxy into a background page
worker in an iframe
proxy.js on /postmessage-proxy listens for postMessage events:
- queueTelemetryPing to enqueue a telemetry ping to be sent by the
  main add-on
- fetchTelemetryPings to fetch all queued telemetry pings from the
  main add-on and submit them all
GIANT HACK: proxy.js maintains the queue as a JSON-serialized array in
a cookie, because that's the only shared persistent data for the
iframe I could get working. (LocalStorage fails with a security error)
add vendor gulpfile task to copy over vendor scripts
add vendor/js.cookie.js as a lazy way to manage cookies

Made a quick drawing of the messaging flow:

lmorchard · 2016-06-21T21:10:17Z

/cc @chuckharmston @meandavejustice @groovecoder - all you folks probably have an interest in this.

This is probably a terrible idea, because I came up with it while sleep-deprived and flying back from London. But, what this does is implement a message-passing channel between webextensions and our main bootstrap add-on.

The mechanism is a new page at /postmessage-proxy on the Test Pilot site. The webextension and the main add-on each load this page into an iframe inside an invisible document. That iframe accepts postMessage() messages.

The webextension sends a queueTelemetryPing message to the iframe. The main add-on periodically sends a fetchTelemetryPings message to the iframe. The iframe replies to fetchTelemetryPings with telemetryPings so the add-on can pick up telemetry payloads and send them with submitExternalPing

Here's the nasty hacky part: In the middle, the /postmessage-proxy page stores the telemetry pings queue as JSON serialized in a cookie. The special magic that makes everything work is that the cookie is shared between the separate instances of the page loaded in the main add-on and the webextension.

I wanted to use LocalStorage in the /postmessage-proxy page. But, whenever I try to get or set anything, I get a SecurityError in the console. Hopefully this has a fix, because the cookie method is not something I'm proud of.

Future expansion of this idea, if it can be polished up, can also include the messaging required for sending variant testing configuration around (for #477).

lmorchard · 2016-06-21T21:13:42Z

Oh, and also this commit has the webextension and main add-on both hard-coded to load /postmessage-proxy from local dev. That needs some work, too.

lmorchard · 2016-06-21T21:17:15Z

Oh yeah, and this results in real normal Telemetry pings, as opposed to the server-side solution in #952

groovecoder · 2016-06-22T00:33:01Z

What if we use IndexedDB (via localForage)? Less hacky?

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

What if we use IndexedDB (via localForage)? Less hacky?

Tried both the localStorage & IndexedDB drivers in localForage. Same error: SecurityError: The operation is insecure. I'm not quite sure if this is a legitimate security complaint, or an upstream Firefox bug. I imagine an iframe in a background page in a webextension or add-on is a fairly edgy case.

groovecoder · 2016-06-22T18:55:28Z

+      subject: EXPERIMENT_ID,
+      data: data
+    }
+  }, '*');


In the "real world" implementation, I assume we would tell devs to only post the message to targetOrigin of testpilot.firefox.com ?

Yeah, but until/unless I figure out a way to configure this for dev/stage/prod environment it's * for now

groovecoder · 2016-06-22T19:13:36Z

I'm still trying to get my local testpilot install to work, but then I'll be eager to try this out with my extension. I agree the cookie thing is a bit hacky, but it does seem to be the best we can do for now? Do you have a bug # or issue # for the security error thrown by localStorage and IndexedDB?

groovecoder · 2016-06-22T19:14:06Z

Would you want to do this instead of #952 ? Or in addition to it?

lmorchard · 2016-06-22T20:08:41Z

Would you want to do this instead of #952 ? Or in addition to it?

Instead of. This would give us normal full Telemetry pings for all experiment types.

lmorchard · 2016-06-22T20:09:22Z

Do you have a bug # or issue # for the security error thrown by localStorage and IndexedDB?

No, I have no idea yet whether it's a bug or working as intended. Still researching that / hoping someone else knows what's up.

groovecoder · 2016-06-22T22:15:00Z

FWIW I've added both methods of boilerplate/example code into my extension: https://github.com/groovecoder/blok/compare/disable-and-ask-why-4?expand=1#diff-d8c11c63dfd474a75f05fbbdba0889f8R27

If I can get testpilot running locally I'll try them both out.

lmorchard · 2016-06-23T17:55:32Z

FWIW, I also just tried setting the postMessage origin to http://testpilot.dev:8000 rather than * - and now I get this error:

Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided
(‘http://testpilot.dev:8000’) does not match the recipient window’s origin 
(‘http://testpilot.dev:8000’).

😕 The hell? I feel like I've strayed into a minefield of corner cases here.

groovecoder · 2016-06-23T18:50:23Z

                  {'user_id': request.user.email if not request.user.is_anonymous() else ''})
+
+
+@xframe_options_exempt


Any chance we could use parent-src CSP around this to protect it from click-jacking? (IIRC ALLOW-FROM doesn't allow multiple origin values?)

groovecoder · 2016-06-23T20:34:27Z

Wow, so this actually works in my web extension. 🎉

Except I couldn't put the <iframe> directly into my pageAction popup 😞 I had to:

postMessage from my pageAction popup to my background page
postMessage from my background page to the testpilot postmessage-proxy <iframe>

Still, I:

Disabled Test Pilot
Clicked the postMessage-firing button in my pageAction pup-up
Loaded up http://testpilot.dev:8000/postmessage-proxy
Saw my data in the telemetryPings cookie
Re-enabled Test Pilot
Waited a few seconds
Refreshed the postmessage-proxy page
Saw that Test Pilot had process my telemetry pings

I see addon/dev-prefs.json has "toolkit.telemetry.server": "http://127.0.0.1" ... any chance I can point that at a staging/sandbox telemetry IP to see this actually hit the telemetry pipeline?

Oh, and here's a fun video reward for making such an amazingly-creative solution:

https://www.youtube.com/watch?v=qybUFnY7Y8w

lmorchard · 2016-06-23T20:58:20Z

I see addon/dev-prefs.json has "toolkit.telemetry.server": "http://127.0.0.1" ... any chance I can point that at a staging/sandbox telemetry IP to see this actually hit the telemetry pipeline?

There's no sandbox for Telemetry as far as I know. The 127.0.0.1 server is intended to be an instance of vdjeric/gzipServer, but it could be any HTTP server to see the POST requests. There's an about:config setting to turn on telemetry console messages when requests go out, too, but I can't remember what it is :/

lmorchard · 2016-06-23T21:01:43Z

Except I couldn't put the <iframe> directly into my pageAction popup

Hmm, is that not a spot where chrome.runtime.sendMessage() could work? Not sure because I haven't actually gotten that far with building a webextension myself, though.

groovecoder · 2016-06-23T21:04:55Z

Probably could use that too. Not sure if it gets me anything over postMessage right now. But my add-on is growing large enough that some actual architecture and design is in order. I'll keep sendMessage in mind for that.

lmorchard · 2016-06-24T21:41:37Z

Not seeing any other feedback or ideas on this... @chuckharmston? @meandavejustice?

Might just see what I can do to clean up the last loose ends and merge this Monday if I don't get any more eyes on it before end of day. Mainly:

~~Find a simpler / smaller cookie library?~~
Point the proxy frames at testpilot.firefox.com - or find some way to configure them based on main addon environment selection (might only be possible for the main addon itself)
~~Tests? (not sure how feasible without an integration test solution)~~

Just going to go with the cookie hack until/unless a better working solution reveals itself.

groovecoder · 2016-06-27T12:38:39Z

Not sure if it's any better, but ...

For another add-on we've coded it such that it uses whichever site was last visited in the browser. I.e., when the browser goes to testpilot.firefox.com, a content script configures the add-on to use testpilot.firefox.com, when the user goes to testpilot.dev:8000, the content script configures the add-on to use testpilot.dev:8000.

But that just shifts the issue - still hard to know which site you're using when doing development. Hmm ... It also means each web extension experiment would have to implement a similar mechanism, so that's double-bad.

groovecoder · 2016-06-27T12:39:54Z

So yeah - 👍 to hard-code testpilot.firefox.com into the add-ons and the extension experiments by default. And make devs go in and change the code to use other environments.

…nsion - New /postmessage-proxy page served by Django - Main add-on loads /postmessage-proxy into a background page worker in an iframe - Example webextension loads /postmessage-proxy into a background page worker in an iframe - proxy.js on /postmessage-proxy listens for postMessage events: - queueTelemetryPing to enqueue a telemetry ping to be sent by the main add-on - fetchTelemetryPings to fetch all queued telemetry pings from the main add-on and submit them all - GIANT HACK: proxy.js maintains the queue as a JSON-serialized array in a cookie, because that's the only shared persistent data for the iframe I could get working. (LocalStorage fails with a security error) - add vendor gulpfile task to copy over vendor scripts - add vendor/js.cookie.js as a lazy way to manage cookies

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

lmorchard force-pushed the 433-webextension-telemetry branch from 8848ef4 to 8556899 Compare June 21, 2016 21:01

groovecoder reviewed Jun 22, 2016
View reviewed changes

groovecoder reviewed Jun 23, 2016
View reviewed changes

lmorchard mentioned this pull request Jun 24, 2016

Add /api/metrics/testpilottest POST resource #952

Closed

lmorchard force-pushed the 433-webextension-telemetry branch from 8556899 to d3f7306 Compare June 24, 2016 21:44

clouserw mentioned this pull request Jun 28, 2016

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

Merged

lmorchard force-pushed the 433-webextension-telemetry branch from d3f7306 to 88a05a7 Compare June 28, 2016 17:20

		{'user_id': request.user.email if not request.user.is_anonymous() else ''})


		@xframe_options_exempt

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

lmorchard commented Jun 21, 2016

Uh oh!

lmorchard commented Jun 21, 2016

Uh oh!

groovecoder commented Jun 22, 2016

Uh oh!

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

Uh oh!

Uh oh!

groovecoder Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

lmorchard Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

groovecoder commented Jun 22, 2016

Uh oh!

groovecoder commented Jun 22, 2016

Uh oh!

lmorchard commented Jun 22, 2016

Uh oh!

lmorchard commented Jun 22, 2016

Uh oh!

groovecoder commented Jun 22, 2016

Uh oh!

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

Uh oh!

Uh oh!

groovecoder Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

groovecoder commented Jun 23, 2016

Uh oh!

lmorchard commented Jun 23, 2016

Uh oh!

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

Uh oh!

Uh oh!

groovecoder commented Jun 23, 2016

Uh oh!

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

Uh oh!

Uh oh!

groovecoder commented Jun 27, 2016

Uh oh!

groovecoder commented Jun 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lmorchard commented Jun 21, 2016 •

edited

Loading

lmorchard commented Jun 21, 2016 •

edited

Loading

lmorchard commented Jun 22, 2016 •

edited

Loading

lmorchard commented Jun 23, 2016 •

edited

Loading

lmorchard commented Jun 23, 2016 •

edited

Loading

lmorchard commented Jun 24, 2016 •

edited

Loading