#750 - Add about:studies #801

andymikulski · 2017-06-08T17:47:32Z

Fixes #750

Adds about:studies protocol handler
Registers about:studies after Shield client has started
Adds placeholder page

This needs some tests, but the meat of the issue is here and ready for review.

andymikulski · 2017-06-08T19:03:07Z

TC failure seems to be unrelated - tests pass locally.

mythmon

This looks good. I dug into some of the Firefox JS that this references, and I think you've got a good foundation here, which is the point of the issue here.

I think going with the protocol handler is definitely the idea here. I looked at some of the other about pages (which I was able to find after reading this PR), and there really isn't a centralized place we could add to.

I have a couple small changes I think we could make, and I think this could use some tests. If nothing else, simply loading the about:studies page and making an assertion about it's content would be a good test.

mythmon · 2017-06-08T21:28:11Z

recipe-client-addon/bootstrap.js


 this.install = function() {};

 this.startup = async function() {
  await ShieldRecipeClient.startup();
+  AboutStudiesProtocol.register();


We shouldn't be modifying bootstrap.js if we can avoid it. We should treat ShieldRecipeClient as the entrypoint for anything in the add-on. This is because it is very hard to control when bootstrap runs in tests, and putting everything in another file makes that a lot easier.

mythmon · 2017-06-08T21:28:53Z

recipe-client-addon/bootstrap.js

 };

 this.shutdown = function(data, reason) {
+  AboutStudiesProtocol.unregister();
  ShieldRecipeClient.shutdown(reason);

  // Unload add-on modules. We don't do this in ShieldRecipeClient so that


On the other hand, we do have to add AboutStudiesProtocol.jsm and any other modules we add to this list of modules to unload.

mythmon · 2017-06-08T21:32:51Z

recipe-client-addon/lib/AboutStudiesProtocol.jsm

+  contractID: "@mozilla.org/network/protocol/about;1?what=studies",
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIStudiesProtocolHandler]),
+
+  newChannel: (aURI) => {


The a prefix stands for "argument", and is generally going out of style in Firefox code. This is a nit, but can we not use it?

mythmon · 2017-06-08T21:36:34Z

recipe-client-addon/lib/AboutStudiesProtocol.jsm

+ * `AboutStudies.xml` file.
+ */
+function StudiesProtocolHandler() {}
+StudiesProtocolHandler.prototype = {


Can this be either a class or a plain object? What's the benefit of using the prototype way of doing things?

I wanted to keep the uri, classID, etc tied to the ProtocolHandler class, so I just defined everything in one go. Using es6 classes we have to do something like:

class Whatever { ... } Whatever.prototype = { thing: 'woo' }

Alternatively, we could define the uri etc in the class constructor, however I suspect the Cm.registerFactory call looks up the handler info from the prototype. I will investigate and see what we can do.

I pulled out the data that was being stored in the prototype into its own separate object - turns out we dont need those attributes on the class itself, just need them while registering the protocol handler.

mythmon · 2017-06-08T21:37:19Z

recipe-client-addon/lib/AboutStudies.xml

@@ -0,0 +1,12 @@
+<?xml version="1.0"?>
+<!DOCTYPE html>


Why is this file an xml file? Can we make it AboutStudies.xml?

mythmon · 2017-06-08T21:58:27Z

recipe-client-addon/lib/AboutStudiesProtocol.jsm

+        Services.scriptSecurityManager.getSystemPrincipal(),
+        null,
+        Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
+        Ci.nsIContentPolicy.TYPE_OTHER


We probably shouldn't use TYPE_OTHER here. For one, the docs say we should avoid it. I think TYPE_DOCUMENT would be more appropriate.

I think you missed this one.

You're right, sorry! I've corrected this.

mythmon · 2017-06-08T22:05:20Z

Oh, I also don't think this needs to land on a feature branch. Since it doesn't change any existing features, I think it is safe to land this on master, even with the filler content.

mythmon

Looks good, except for one thing you missed (the channel type). Still needs test before we can merge. Thanks for the changes.

andymikulski · 2017-06-13T19:45:18Z

@mythmon @gijsk This is ready for review. Let me know if the tests need to be more in-depth, currently I just have it testing that navigating to about:studies works, and that the page contains the content we expect.

mythmon · 2017-06-13T19:55:23Z

The taskcluster failure is #806, which has been fixing master. Can you rebase on master to pick that up fix?

mythmon · 2017-06-13T20:33:04Z

I have no idea what the CircleCI errors are. Webpack seems grumpy? I bet we didn't pin some dependency and something updated.

Osmose · 2017-06-13T20:41:59Z

I have no idea what the CircleCI errors are. Webpack seems grumpy? I bet we didn't pin some dependency and something updated.

replaceWithSourceString is a Babel thing, I believe. May have something to do with one of our Babel dependencies.

mythmon · 2017-06-13T20:42:33Z

May have something to do with one of our Babel dependencies.

One of the unpinned deps that @andymikulski just fixed was babili, so that makes a lot of sense.

gijsk · 2017-06-13T21:51:10Z

I am sad about this. Do we need to do this?

Some general points - I don't have time to do an in-depth review right now, but from a quick look some points need addressing anyway, so...

AboutPocket.jsm should allow you to have a better example and get rid of the boilerplate. I don't know where the register/unregister/protocol factory code came from, but Cm and various other aliases/XPCOMUtils should make this a lot less work. Maybe it's just the this.instance checking - XPCOM should be taking care of this for you.
URI flags don't "not do anything", they control the security restrictions and abilities (e.g. can/must they load in the child process or no) of the page... they're quite important, generally speaking. I don't know if about's protocol handler here has sane defaults by now, and how they apply to things that don't use the builtin protocol redirector, but there we are.
please verify that the page has a null principal or similar non-privileged principal in the test
enforce this in newChannel (see aboutpocket.jsm)
use the loadInfo parameter to create the channel (again, see aboutpocket.jsm)
this is a new about: page and ideally it should be forced to load in the child process and not the parent (to take advantage of sandboxing). I don't know how easy it is to lazily load this module in the child process (to make the URI available there), but ideally we should try. It will need the relevant URI flags and I guess maybe we should make newChannel throw if called in the parent, I'm not sure.

mythmon · 2017-06-13T21:57:57Z

I am sad about this. Do we need to do this?

What in particular are you sad about? We definitely need to add some sort of user visible page. I don't think it strictly needs to be an about: page, but that seemed like the best choice. Do you have another suggestion?

gijsk · 2017-06-13T22:00:23Z

To elaborate on my sadness: we have a tremendous amount of about: pages that were once happy aspirations of projects, with 0 UI exposure, that we have to carry (or only recently stopped carrying) around because of inertia. That list contains, off the top of my head:

about:performance
about:healthreport
about:permissions (RIP)
about:profiles
really, about:pocket-*
about:logo
about:providerdirectory
about:sync-tabs (going away soon, hopefully, in favour of things that have UI...)
about:socialerror
about:credits / about:license (I don't know why we have 2)
about:cache but also about:cache-entry and about:networking (why do we have 3 separate things?)

I just sometimes get the feeling people think their project doesn't matter unless it has its own about: page. I don't think that makes sense. I think we should think carefully before adding more. Is this the best way of exposing this thing (whatever it is)? Can we just make a button somewhere generate a srcdoc or data URI?

It's not really clear to me what needs to end up living on this page. But from reading the original issue, maybe this should just be a part of about:addons ?

MattGrimes · 2017-06-13T22:15:40Z

@gijsk The connection to about:addons is a technical relationship since we are using add-ons as a delivery mechanism. It doesn't really make sense from a user perspective. We don't want users to be confused about what they have installed themselves and what studies they are actively participating in. I've had quite a few conversations with some of the product, legal, and UX folks about this already. We'd like a unified place where users can go to see if they are currently participating in a study and view any studies they may have been a part of in the past. I don't really care if it's an about page, but that does seem (to me at least) a strong candidate based on prior art.

gijsk · 2017-06-13T22:20:10Z

@MattGrimes OK, how does this new page get exposed? How do users find it?

Osmose · 2017-06-13T22:20:45Z

But from reading the original issue, maybe this should just be a part of about:addons ?

The UI for about:addons is strongly tied in with the registration of new AddonTypes; I looked into this originally, and feedback from @rhelmer and others was "Don't try to add new stuff to about:addons if at all possible".

mythmon · 2017-06-13T22:21:39Z

As for the background of what this page is for, it is supposed to show a list of studies the user is participating in, both opt-out and opt-in, past and present. It's a legal requirement for doing opt-out studies. I definitely agree that having no UI exposure and letting this languish would be a problem, but hopefully we can avoid that. I hope that having the code isolated in the shield-recipe-client add-on will help with the languishing part: at least it is self contained.

gijsk · 2017-06-13T22:28:14Z

As for the background of what this page is for, it is supposed to show a list of studies the user is participating in, both opt-out and opt-in, past and present. It's a legal requirement for doing opt-out studies.

Is there supposed to be UI for the user to remove/disable studies (or opt in/out of all studies, or something) ?

Osmose · 2017-06-13T22:29:28Z

@MattGrimes OK, how does this new page get exposed? How do users find it?

Studies will link to the page occasionally with links in their UIs saying things like "Why am I seeing this?" to help explain that they're in a feature experiment.
A SUMO page will exist that will probably link to the page
The privacy policy may also link to the page? I'm not sure about this one.

I wonder if a resource://studies/index.html URL would work? It's user-facing-ish, but maybe that's fine?

Osmose · 2017-06-13T22:31:58Z

Is there supposed to be UI for the user to remove/disable studies (or opt in/out of all studies, or something) ?

Not in this PR, but eventually yes, the page will be used for viewing in-progress studies, and giving users the option to disable them, as well as see studies they previously participated in.

gijsk · 2017-06-13T22:35:35Z

@MattGrimes OK, how does this new page get exposed? How do users find it?

Studies will link to the page occasionally with links in their UIs saying things like "Why am I seeing this?" to help explain that they're in a feature experiment.

A SUMO page will exist that will probably link to the page

The privacy policy may also link to the page? I'm not sure about this one.

Generally speaking, Firefox doesn't let web pages (like SUMO/privacy policy) link to about: pages, as that's a security risk. about:blank is the obvious exception. We definitely shouldn't let web pages link to about: pages that can do anything vaguely privilege-related. It may or may not be possible to have exceptions for select mozilla-controlled properties x page combinations, but that would need code changes in Firefox proper so it would be useful to have a clear idea about what the needs are (also because this might make it more attractive to put the list in, say, about:support).

I need to go sleep now, but this stuff (how do people get there, what does the page do) all ties into security warranties/hole-punches we will need for this thing, whatever it ends up being, so fleshing this out more before pushing ahead with this PR seems useful - even if I don't know that this PR is the best place to have the discussion.

mythmon · 2017-06-13T23:25:14Z

Regarding the CI issues: This looks like it is affecting all PRs, not just this one. I filed #812 for that problem. Lets remove the alleged fix from this branch, and try and get just a fix for just it landed ASAP.

andymikulski · 2017-06-20T22:30:28Z

Closing until some specs are landed on.

andymikulski assigned mythmon Jun 8, 2017

andymikulski requested a review from mythmon June 8, 2017 17:47

mythmon suggested changes Jun 8, 2017

View reviewed changes

mythmon assigned andymikulski and unassigned mythmon Jun 8, 2017

andymikulski changed the base branch from about-studies to master June 12, 2017 17:15

mythmon suggested changes Jun 12, 2017

View reviewed changes

andymikulski force-pushed the add-about-studies branch from 208896e to 623ebed Compare June 12, 2017 18:35

andymikulski requested a review from gijsk June 13, 2017 19:42

andymikulski assigned mythmon and gijsk and unassigned andymikulski Jun 13, 2017

andymikulski force-pushed the add-about-studies branch from ed1d697 to bd2e0d6 Compare June 13, 2017 19:43

Andy Mikulski added 3 commits June 13, 2017 14:00

Add about:studies protocol handler

6af5469

Code review

53c8aa6

Add about:studies tests

ee5cae9

andymikulski force-pushed the add-about-studies branch from bd2e0d6 to ee5cae9 Compare June 13, 2017 20:02

mythmon approved these changes Jun 13, 2017

View reviewed changes

Pin npm package version numbers

7da24d0

andymikulski assigned andymikulski and unassigned mythmon and gijsk Jun 19, 2017

andymikulski closed this Jun 20, 2017

andymikulski deleted the add-about-studies branch September 11, 2017 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#750 - Add about:studies #801

#750 - Add about:studies #801

andymikulski commented Jun 8, 2017 •

edited

andymikulski commented Jun 8, 2017

mythmon left a comment •

edited

mythmon Jun 8, 2017

mythmon Jun 8, 2017

mythmon Jun 8, 2017

mythmon Jun 8, 2017

andymikulski Jun 12, 2017

andymikulski Jun 12, 2017

mythmon Jun 8, 2017

mythmon Jun 8, 2017

mythmon Jun 12, 2017

andymikulski Jun 12, 2017

mythmon commented Jun 8, 2017

mythmon left a comment

andymikulski commented Jun 13, 2017

mythmon commented Jun 13, 2017

mythmon commented Jun 13, 2017

Osmose commented Jun 13, 2017

mythmon commented Jun 13, 2017

gijsk commented Jun 13, 2017

mythmon commented Jun 13, 2017

gijsk commented Jun 13, 2017

MattGrimes commented Jun 13, 2017

gijsk commented Jun 13, 2017

Osmose commented Jun 13, 2017

mythmon commented Jun 13, 2017

gijsk commented Jun 13, 2017

Osmose commented Jun 13, 2017 •

edited

Osmose commented Jun 13, 2017

gijsk commented Jun 13, 2017

mythmon commented Jun 13, 2017

andymikulski commented Jun 20, 2017

#750 - Add about:studies #801

#750 - Add about:studies #801

Conversation

andymikulski commented Jun 8, 2017 • edited

andymikulski commented Jun 8, 2017

mythmon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mythmon commented Jun 8, 2017

mythmon left a comment

Choose a reason for hiding this comment

andymikulski commented Jun 13, 2017

mythmon commented Jun 13, 2017

mythmon commented Jun 13, 2017

Osmose commented Jun 13, 2017

mythmon commented Jun 13, 2017

gijsk commented Jun 13, 2017

mythmon commented Jun 13, 2017

gijsk commented Jun 13, 2017

MattGrimes commented Jun 13, 2017

gijsk commented Jun 13, 2017

Osmose commented Jun 13, 2017

mythmon commented Jun 13, 2017

gijsk commented Jun 13, 2017

Osmose commented Jun 13, 2017 • edited

Osmose commented Jun 13, 2017

gijsk commented Jun 13, 2017

mythmon commented Jun 13, 2017

andymikulski commented Jun 20, 2017

andymikulski commented Jun 8, 2017 •

edited

mythmon left a comment •

edited

Osmose commented Jun 13, 2017 •

edited