1324049 - Replace data choices infobar with privacy policy in a background tab on first run for the feb 2017 funnelcake

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Verdi [:verdi] Best to slack me]

Currently, 60 seconds into the first run of Firefox we open a bottom infobar that says, "Firefox automatically sends some data to Mozilla so that we can improve your experience. [Choose What I Share]". For our Feb 2017 funnelcake we want to prevent that infobar from showing on first run (or at any other time) and instead open a second tab (the first being the wow moment + account sign-in page) in the background that loads https://www.mozilla.org/privacy/firefox/

Comment 1

[Dão Gottwald [:dao]]

Said page doesn't tell the user how to control this stuff, although I assume that information is theoretically still accessible somewhere behind the "Learn more" links. In other words, we'll lose the obvious "Choose what I want to share" option. Is this part of the plan and if so, why?

Comment 2

[Verdi [:verdi] Best to slack me]

(In reply to Dão Gottwald [:dao] from comment #1)
> Said page doesn't tell the user how to control this stuff, although I assume
> that information is theoretically still accessible somewhere behind the
> "Learn more" links. In other words, we'll lose the obvious "Choose what I
> want to share" option. Is this part of the plan and if so, why?

Yes this is part of the plan (approved by legal for this test). The primary purpose of the bar is to obtain general, implied consent for the data collection specified in the notice which is accomplished by it's display. So in this scenario, the display of the second tab would accomplish that.  And yes, when you click "learn more" the page expands and the various sections link to detailed support instructions for changing your settings. 

In addition to this change, the legal team is looking to make changes to the page to make it easier to understand and use. We also want to experiment with additions to the UITour api (not for our Feb funnelcake) so that this page could direct the user to the Firefox options.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

For the funnelcake build, we'll need to set datareporting.policy.firstRunURL to "https://www.mozilla.org/privacy/firefox/".

Comment 4

[Georg Fritzsche [:gfritzsche]]

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

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

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +421,5 @@
> +      Preferences.set(PREF_FIRST_RUN, false);
> +    }
> +
> +    const firstRunPolicyURL =
> +      isFirstRun ? Preferences.get(PREF_FIRST_RUN_URL, "") : "";

Can we move that whole logic from here on into a separate function `_showFirstRunURL()` or so?

@@ +432,5 @@
> +      } catch (e) {}
> +
> +      if (win) {
> +        win.openUILinkIn(firstRunPolicyURL, "tab", { inBackground: true });
> +        this._userNotified();

At this stage we just initiated the load, we didn't actually notify the user yet.
I don't think we should set this yet (and thus start uploading).

::: toolkit/components/telemetry/datareporting-prefs.js
@@ +8,5 @@
>  pref("datareporting.policy.dataSubmissionPolicyBypassNotification", false);
>  pref("datareporting.policy.currentPolicyVersion", 2);
>  pref("datareporting.policy.minimumPolicyVersion", 1);
>  pref("datareporting.policy.minimumPolicyVersion.channel-beta", 2);
> +pref("datareporting.policy.firstRunURL", "");

Please document this in toolkit/components/telemetry/docs/internals/preferences.rst.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > +        win.openUILinkIn(firstRunPolicyURL, "tab", { inBackground: true });
> > +        this._userNotified();
> 
> At this stage we just initiated the load, we didn't actually notify the user
> yet.
> I don't think we should set this yet (and thus start uploading).

When do you suggest we should do this instead?

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v2

Now calling _userNotified when the tab has been selected, and also trying to take into account failed load attempts. Not sure if this is what you had in mind.

Comment 7

[Georg Fritzsche [:gfritzsche]]

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

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

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +414,5 @@
> +   * Try to open the privacy policy in a background tab instead of showing the infobar.
> +   */
> +  _openFirstRunPage() {
> +    const firstRunPolicyURL = Preferences.get(PREF_FIRST_RUN_URL, "");
> +    if (!firstRunPolicyURL) {

Nit: Preferences.get(..., null)?
That makes the check less surprising.

@@ +448,5 @@
> +      tab.removeEventListener("TabSelect", onTabSelect);
> +    };
> +
> +    win.addEventListener("unload", removeListeners);
> +    tab.addEventListener("TabSelect", onTabSelect);

Do we want to consider the user only notified when the page is viewed?
Or already when the page is loaded in the background and *could be viewed*?

::: toolkit/components/telemetry/docs/internals/preferences.rst
@@ +58,5 @@
>    This preference is not present until the first run. After, its value is set to false. This is used to show the infobar with a more aggressive timeout if it wasn't shown yet.
>  
> +``datareporting.policy.firstRunURL``
> +
> +  If set, a browser tab will be opened on first run instead of the infobar.

"... a browser tab showing the policy ...".

Comment 8

[Georg Fritzsche [:gfritzsche]]

@mverdi:
> Do we want to consider the user only notified when the page is viewed?
> Or already when the page is loaded in the background and *could be viewed*?

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> > Do we want to consider the user only notified when the page is viewed?
> > Or already when the page is loaded in the background and *could be viewed*?

The problem with the first option is that, if a user does not view the page during the first session, we can't send any data yet.
This goes against our goals of decreasing data latency.

We should go with the second option if possible.

Comment 10

[Verdi [:verdi] Best to slack me]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> > > Do we want to consider the user only notified when the page is viewed?
> > > Or already when the page is loaded in the background and *could be viewed*?
> 
> The problem with the first option is that, if a user does not view the page
> during the first session, we can't send any data yet.
> This goes against our goals of decreasing data latency.
> 
> We should go with the second option if possible.

In our discussions on this, just having the tab available severs as the user being notified. In tests, I've not seen anyone interact with the infobar but I do see them interact with second tabs. I *think* this might result in more people seeing the privacy notice.

Comment 11

[Georg Fritzsche [:gfritzsche]]

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

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

Please change this to trigger _userNotified() when the tab loaded successfully.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Verdi [:verdi] from comment #10)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> > (In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> > > > Do we want to consider the user only notified when the page is viewed?
> > > > Or already when the page is loaded in the background and *could be viewed*?
> > 
> > The problem with the first option is that, if a user does not view the page
> > during the first session, we can't send any data yet.
> > This goes against our goals of decreasing data latency.
> > 
> > We should go with the second option if possible.
> 
> In our discussions on this, just having the tab available severs as the user
> being notified. In tests, I've not seen anyone interact with the infobar but
> I do see them interact with second tabs. I *think* this might result in more
> people seeing the privacy notice.

Has legal explicitly approved this as well? The infobar can be read without interacting with it. The page cannot be read without selecting the tab.

Comment 13

[Verdi [:verdi] Best to slack me]

(In reply to Dão Gottwald [:dao] from comment #12)
> 
> Has legal explicitly approved this as well? The infobar can be read without
> interacting with it. The page cannot be read without selecting the tab.

Yes. We have approval for this test. This is an interaction that I developed with them over the last few months (meeting notes here, LDAP req. https://docs.google.com/document/d/1s0t1IJU9ZIfkYbsEyBeoO9zqCI5Jmyl16RvtH5LPI3c/edit ).

Comment 14

[Dão Gottwald [:dao]]

(In reply to Verdi [:verdi] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > 
> > Has legal explicitly approved this as well? The infobar can be read without
> > interacting with it. The page cannot be read without selecting the tab.
> 
> Yes. We have approval for this test. This is an interaction that I developed
> with them over the last few months (meeting notes here, LDAP req.
> https://docs.google.com/document/d/
> 1s0t1IJU9ZIfkYbsEyBeoO9zqCI5Jmyl16RvtH5LPI3c/edit ).

I know that the test got overall approval, but my question is whether the relevant stakeholder were aware of this specific detail: that we'd start data submission after loading the privacy policy page in a background tab regardless of whether the user would actually see that page.

I tried to search for some keywords in the meeting notes but couldn't find anything relevant.

Comment 15

[Verdi [:verdi] Best to slack me]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Verdi [:verdi] from comment #13)
> > (In reply to Dão Gottwald [:dao] from comment #12)
> > > 
> > > Has legal explicitly approved this as well? The infobar can be read without
> > > interacting with it. The page cannot be read without selecting the tab.
> > 
> > Yes. We have approval for this test. This is an interaction that I developed
> > with them over the last few months (meeting notes here, LDAP req.
> > https://docs.google.com/document/d/
> > 1s0t1IJU9ZIfkYbsEyBeoO9zqCI5Jmyl16RvtH5LPI3c/edit ).
> 
> I know that the test got overall approval, but my question is whether the
> relevant stakeholder were aware of this specific detail: that we'd start
> data submission after loading the privacy policy page in a background tab
> regardless of whether the user would actually see that page.
> 
Yes this is how it's designed to work. We were under the impression that nothing was sent in the first session and that that was a thing that needed to be fixed. So, taking that into account, the plan that we developed is that we can send info back in the first session as long as the second tab is visible. In fact, the "Privacy Notice" link on the download page is enough to enable this for most cases. The reason we're including the tab is because you can't be sure that the person who downloaded the installer is the person opening Firefox for the first time.

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Verdi [:verdi] from comment #15)
> Yes this is how it's designed to work. We were under the impression that
> nothing was sent in the first session and that that was a thing that needed
> to be fixed.

Sending data back in the first session is already possible now.
We show the data choices infobar after 60 seconds.
We treat the showing of the bar as implied consent for the "opt-out" part of the Telemetry data and start sending from then on.

Comment 17

[Verdi [:verdi] Best to slack me]

(In reply to Georg Fritzsche [:gfritzsche] [away Dec 24 - Jan 2] from comment #16)
> (In reply to Verdi [:verdi] from comment #15)
> > Yes this is how it's designed to work. We were under the impression that
> > nothing was sent in the first session and that that was a thing that needed
> > to be fixed.
> 
> Sending data back in the first session is already possible now.
> We show the data choices infobar after 60 seconds.
> We treat the showing of the bar as implied consent for the "opt-out" part of
> the Telemetry data and start sending from then on.

Great! This was definitely designed so that we have implied consent for the opt-out data.

Comment 18

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8821621 [details] [diff] [review]
patch v3

r?gijs in case he can review this today since Georg is already gone.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8821621 [details] [diff] [review]
patch v3

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

r=me, but see below. I think a test will also help with uplift approvals (plus give some peace of mind that this works correctly and continues to work correctly, given its potential legal implications).

One other thought: I suspect we will want URL variables/substitutions in the link, for version/locale etc.? At least locale would be beneficial...

::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ +430,5 @@
> +      return false;
> +    }
> +
> +    // We'll consider the user notified once the privacy policy has been loaded
> +    // in a background tab even if that tab hasn't been selected.

Unfortunately this will 'succeed' even if, say, the user is not connected to the network and we show a network error page (at least, tested with a non-existing domain on my Windows machine on beta, and onStateChange does fire with STATE_STOP and STATE_IS_NETWORK). That's hard to check for because the browser's currentURI will still be the linked URI instead of something like about:neterror. You could check the URI associated with the contentPrincipal? Maybe just check for !principal.origin.startsWith("about:") or something, to avoid relying on the target URL being something specific...

Could we write a browser test that verified calling this function works (and shim/replace _userNotified and ensure it is called only once, ie that the listeners are successfully removed) ?

Comment 21

[Dão Gottwald [:dao]]

Attachment: patch v4

(In reply to :Gijs (gone until 3 jan) from comment #20)
> One other thought: I suspect we will want URL variables/substitutions in the
> link, for version/locale etc.? At least locale would be beneficial...

Done, although it's not needed for the funnelcake.

> ::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
> @@ +430,5 @@
> > +      return false;
> > +    }
> > +
> > +    // We'll consider the user notified once the privacy policy has been loaded
> > +    // in a background tab even if that tab hasn't been selected.
> 
> Unfortunately this will 'succeed' even if, say, the user is not connected to
> the network and we show a network error page (at least, tested with a
> non-existing domain on my Windows machine on beta, and onStateChange does
> fire with STATE_STOP and STATE_IS_NETWORK). That's hard to check for because
> the browser's currentURI will still be the linked URI instead of something
> like about:neterror. You could check the URI associated with the
> contentPrincipal? Maybe just check for
> !principal.origin.startsWith("about:") or something, to avoid relying on the
> target URL being something specific...

browser.documentURI works, but there seems to be a race condition causing documentURI to be about:blank instead of an error page URI, so I checked explicitly for about:blank as well. I don't think we want to check just for "about:", since the privacy policy page could be an about: page.

> Could we write a browser test that verified calling this function works (and
> shim/replace _userNotified and ensure it is called only once, ie that the
> listeners are successfully removed) ?

TelemetryReportingPolicyImpl isn't accessible outside of the jsm.

Comment 22

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #21)
> > Could we write a browser test that verified calling this function works (and
> > shim/replace _userNotified and ensure it is called only once, ie that the
> > listeners are successfully removed) ?
> 
> TelemetryReportingPolicyImpl isn't accessible outside of the jsm.

Not even via a backstagepass (ie rv of Cu.import)?

Comment 23

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #21)
> Created attachment 8821771 [details] [diff] [review]
> patch v4
> 
> (In reply to :Gijs (gone until 3 jan) from comment #20)
> > One other thought: I suspect we will want URL variables/substitutions in the
> > link, for version/locale etc.? At least locale would be beneficial...
> 
> Done, although it's not needed for the funnelcake.
> 
> > ::: toolkit/components/telemetry/TelemetryReportingPolicy.jsm
> > @@ +430,5 @@
> > > +      return false;
> > > +    }
> > > +
> > > +    // We'll consider the user notified once the privacy policy has been loaded
> > > +    // in a background tab even if that tab hasn't been selected.
> > 
> > Unfortunately this will 'succeed' even if, say, the user is not connected to
> > the network and we show a network error page (at least, tested with a
> > non-existing domain on my Windows machine on beta, and onStateChange does
> > fire with STATE_STOP and STATE_IS_NETWORK). That's hard to check for because
> > the browser's currentURI will still be the linked URI instead of something
> > like about:neterror. You could check the URI associated with the
> > contentPrincipal? Maybe just check for
> > !principal.origin.startsWith("about:") or something, to avoid relying on the
> > target URL being something specific...
> 
> browser.documentURI works, but there seems to be a race condition causing
> documentURI to be about:blank instead of an error page URI, so I checked
> explicitly for about:blank as well. I don't think we want to check just for
> "about:", since the privacy policy page could be an about: page.

Oh, right, so the 'network stop' thing could come from the initial about:blank load... that might actually help with bug 1292960 and friends.

Comment 24

[Dão Gottwald [:dao]]

(In reply to :Gijs (gone until 3 jan) from comment #22)
> (In reply to Dão Gottwald [:dao] from comment #21)
> > > Could we write a browser test that verified calling this function works (and
> > > shim/replace _userNotified and ensure it is called only once, ie that the
> > > listeners are successfully removed) ?
> > 
> > TelemetryReportingPolicyImpl isn't accessible outside of the jsm.
> 
> Not even via a backstagepass (ie rv of Cu.import)?

I don't know, never tried that. If I remember correctly, the jsm locks some objects, but maybe not this one. In any case I don't like the idea of messing with private code like that. Could have all kinds of weird side effects if this method calls something else than _userNotified.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bbad04c9255
Implement pref for opening the privacy policy in a background tab on first run instead of showing the data choices infobar. r=gijs

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bbad04c9255

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8821771 [details] [diff] [review]
patch v4

Approval Request Comment
[Feature/Bug causing the regression]: bug 1322718 comment 1

[User impact if declined]: we'd show the info bar

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: can't be verified in Nightly since the new behavior is disabled by default

[Needs manual test from QE? If yes, steps to reproduce]: I assume QA will be part of the funnelcake process. This change will need to be part of that.

[List of other uplifts needed for the feature/fix]: 

[Is the change risky?]: yes, although the risk would be limited to the funnelcake build

[Why is the change risky/not risky?]: If something goes unexpectedly wrong here, it's possible that we wouldn't get the telemetry data that we need. However the code has reasonable checks and safeguards and should still open the info bar on second run if something went wrong with opening the tab.

[String changes made/needed]:

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8821771 [details] [diff] [review]
patch v4

Shows privacy policy for funnelcake users. (opt-out rather than opt-in, as I understand it.)

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01a5cd136989

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/03358dc42d00

Comment 31

[Andrei Vaida [:avaida]]

Setting the flag for verification, description available in Comment 0.

Dao, if you think there's anything in particular that manual QA should focus on, outside of what's self-explanatory in Comment 0, please let us know.

Comment 32

[Dão Gottwald [:dao]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #31)
> Setting the flag for verification, description available in Comment 0.
> 
> Dao, if you think there's anything in particular that manual QA should focus
> on, outside of what's self-explanatory in Comment 0, please let us know.

It would be good to verify that FHR upload is enabled after the tab loaded. I believe you can do that with about:telemetry. It would also be good to verify behavior when the tab failed to load.

Comment 33

[u554753]

Acknowledged the additional info in Comment 32, and will look for that in the test cases.