Closed Bug 862563 Opened 7 years ago Closed 5 years ago

Remove implicit acceptance requirement from data policy

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: mconnor, Assigned: gfritzsche)

References

Details

Attachments

(1 file, 27 obsolete files)

93.13 KB, patch
gps
: review+
Details | Diff | Splinter Review
This is not required and makes the code/UX more complex than necessary.
Just to be clear, if we remove the implicit acceptance, FHR upload could occur as soon as the notification is displayed (before the user has a chance to process it). I just figured that should be explicitly noted.
Per IRC conversation there are more changes than what the bug summary indicates. Please provide an explicit list of requirements. Then I will happily work on this.
Flags: needinfo?(mconnor)
Okay, detailed version:

* The notice is to inform users of a new behaviour of Firefox, in line with the principle of "no surprises"
* This notice should be shown immediately on first run of a Firefox version that includes FHR.
* FHR is on by default, and this notice should not be tied to the operation of that feature, and interactions with the notification should not be treated as user choices.
* The first FHR submission, to my knowledge, takes place after 24 hours, so if we're showing the notification immediately there's no risk of submission before the user can read the notification.  They are not required to interact with the notification in any way.

In IRC it was asked how this impacts Telemetry, so I'll make clear the requirements:

* On Nightly and Aurora, Telemetry is on by default.  Users can choose to opt out through the same preferences pane.  There is no requirement for explicit or implicit acceptance.
* On Beta and Release, Telemetry requires explicit opt-in, which users can do via the pref pane opened when the user clicks Learn More.
Flags: needinfo?(mconnor)
This removes notification tracking from policy.jsm and references in /services/datareporting. I /think/ I killed them all.

The remnants of policy.jsm center around server interaction. At this point, we could probably rename the file to scheduler.jsm and consider moving it to /services/healthreport/. We could also look into removing some of the APIs. But, I think those should be follow-up bugs and/or patches: I'd like to keep the removal patches self-contained to facilitate uplift. And, I'd like to uplift this patch because in some form because a) the code isn't necessary b) it just results in a whole bunch of unused user-set prefs being added to millions of profiles (which we could always clean up later if we want).
Attachment #738241 - Flags: review?(rnewman)
Comment on attachment 738241 [details] [diff] [review]
Part 1: Remove notification tracking from policy.jsm, v1

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

I think there are also a bunch of datareporting.policy prefs you can remove from datareporting-prefs.js.

When you've landed this lot, I'll try to find time to walk through and see if there're any other edges to chop off. Not until Android's done, though.

::: services/datareporting/policy.jsm
@@ +572,1 @@
>      // User has responded to data policy and data submission is enabled. Now

This comment needs to be changed, too.
Attachment #738241 - Flags: review?(rnewman) → review+
OS: Mac OS X → All
Hardware: x86 → All
Status: NEW → ASSIGNED
I need to refactor some tests. But, it's getting there.
Gavin will likely get r? of part 2 since he reviewed all the about:home stuff recently (which is very similar).
Blocks: 850709
this is just a refresh of the Part 1 patch submitted by :gps. It was cleaned up and made to work with the current version of mozilla-central, second part coming in shortly
Attachment #769172 - Flags: review?(rnewman)
Attachment #738241 - Attachment is obsolete: true
Assignee: gps → steven.mirea
This is a fix to the previous Part 2 pach submitted by :gps. The diff of the diffs is a single line modification in browser-data-submission-info-bar.js on line #54, a resources was loaded using a wrong extension

As this now shows a popup on the starting page of firefox it will most likely brake some tests so I'm also pushing it to try
Attachment #770288 - Flags: review?(gavin.sharp)
Attachment #738806 - Attachment is obsolete: true
Refreshed patch to not cause conflicts with the current mozilla-central
Attachment #774173 - Flags: review?(rnewman)
Attachment #774173 - Flags: review?(gps)
Attachment #769172 - Attachment is obsolete: true
Attachment #769172 - Flags: review?(rnewman)
Made the old version of the patch compliant to the current mozilla-central and modified a test to work with the modifications.
I also encountered weird activity with the test browser_datareporting_notification.js working locally but failing on try for the same architecture

https://tbpl.mozilla.org/?tree=Try&rev=79e961c5d2b9
Attachment #774176 - Flags: review?(gps)
Made the old version of the patch compliant to the current mozilla-central and modified a test to work with the modifications.
I also encountered weird activity with the test browser_datareporting_notification.js working locally but failing on try for the same architecture

https://tbpl.mozilla.org/?tree=Try&rev=79e961c5d2b9
Attachment #774177 - Flags: review?(gps)
Attachment #770288 - Attachment is obsolete: true
Attachment #770288 - Flags: review?(gavin.sharp)
Attachment #774176 - Attachment is obsolete: true
Attachment #774176 - Flags: review?(gps)
Comment on attachment 774173 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

I have one potential objection to this.

My assumption (correct me if I'm misremembering) is that we startPolling on startup. Without the implicit acceptance interval, that'll cause us to trigger an FHR upload immediately for a new profile or a user upgrade.

That has two consequences:

* Users don't get a soft interval in which to opt out
* We get a submission from every test profile.

Am I right in that assumption? If so, I'd suggest being a little more hesitant to upload (e.g., by preserving the firstRun logic, or computing the initial value of the next submission date specially if it's not present in prefs).

If I'm misremembering, then rock on.

::: services/datareporting/policy.jsm
@@ -223,5 @@
>   *  2. Try to submit as close to 24 hours apart as possible.
>   *  3. Do not submit too soon after application startup so as to not negatively
>   *     impact performance at startup.
> - *  4. Before first ever data submission, the user should be notified about
> - *     data collection practices.

I think this is still true.

@@ -225,5 @@
>   *     impact performance at startup.
> - *  4. Before first ever data submission, the user should be notified about
> - *     data collection practices.
> - *  5. User should have opportunity to react to this notification before
> - *     data submission.

I think this is kinda true, too -- we just assume that having thrown up a notification, and waiting some time before the first submission (which we do on Android, and I thought we still do on desktop -- nothing to submit otherwise!), counts as giving the user opportunity.
Attachment #774173 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #14)

> * Users don't get a soft interval in which to opt out
> * We get a submission from every test profile.

The first FHR submission is after 24 hours for a new profile and the first call to startPolling is after cca 60 seconds after application startup, which does not submit anything due to the 24h timeout
 
> If I'm misremembering, then rock on.

\m/
(In reply to Richard Newman [:rnewman] from comment #14)
> My assumption (correct me if I'm misremembering) is that we startPolling on
> startup. Without the implicit acceptance interval, that'll cause us to
> trigger an FHR upload immediately for a new profile or a user upgrade.
> 
> That has two consequences:
> 
> * Users don't get a soft interval in which to opt out
> * We get a submission from every test profile.

On first run, we schedule first submission for +24h. Assuming the notification is presented immediately on first run, the user should see the notice and will have a day before anything happens.

> ::: services/datareporting/policy.jsm
> @@ -223,5 @@
> > - *  4. Before first ever data submission, the user should be notified about
> > - *     data collection practices.
> 
> I think this is still true.

Read comment #3.
 
> @@ -225,5 @@
> >   *     impact performance at startup.
> > - *  4. Before first ever data submission, the user should be notified about
> > - *     data collection practices.
> > - *  5. User should have opportunity to react to this notification before
> > - *     data submission.
> 
> I think this is kinda true, too -- we just assume that having thrown up a
> notification, and waiting some time before the first submission (which we do
> on Android, and I thought we still do on desktop -- nothing to submit
> otherwise!), counts as giving the user opportunity.

Comment #3 :)

That being said, I do think there is room for us to be considerate. I believe the policy should at least verify the notification has been presented before uploading. If it doesn't and some future bug accidentally causes the notification to not be displayed, that would potentially protect us from being in legal hot water. The same scenario applies for all Gecko application (not Firefox). If they don't implement a notification bar, do we want the product automatically submitting data?

Not checking the notification completely seems like a rather risky thing to do.
(In reply to Gregory Szorc [:gps] from comment #16)

> On first run, we schedule first submission for +24h. Assuming the
> notification is presented immediately on first run, the user should see the
> notice and will have a day before anything happens.

Great.

> > I think this is still true.
> 
> Read comment #3.

My point was that it's still true -- we notify on first run so that we notify before any data collection/submission -- so we should keep the point in the policy explanation!

> Comment #3 :)

And same response from me -- I'm just saying that if it's still true, and still influenced the implementation choices, then the comment doesn't need to be deleted.

> That being said, I do think there is room for us to be considerate. I
> believe the policy should at least verify the notification has been
> presented before uploading.

I think that's a reasonable thing to assert, yes. And simple to implement.
Comment on attachment 774173 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

OK, this patch is very difficult to review without looking at part 2 because part 2 re-adds some of the important bits removed by this patch, notably the logic for showing the notification.

Anyway, Richard and I both feel the policy should enforce a basic "has the notification been presented" check before upload can occur. This requires restoring some of the deleted code and having the notification if "the notification has been displayed" somehow make it back to policy.jsm. I recommend having DataReportingService.js handle an observer notification and call into this._policy.
Attachment #774173 - Flags: review?(gps)
Attachment #774173 - Flags: review+
Comment on attachment 774177 [details] [diff] [review]
Part 2: Display data reporting notification on first run;

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

I'm cancelling review because this relies on changes being made in part 1 resulting from earlier reviews.

::: browser/components/nsBrowserGlue.js
@@ +162,5 @@
>      Weave.Service.scheduler.delayedAutoConnect(delay);
>    },
>  #endif
>  
> +  // nsIObserver implementation

I appreciate you removing whitespace, but you didn't touch this file in a meaningful way, so you should probably revert these changes (or perform them with a separate commit).
Attachment #774177 - Flags: review?(gps)
Now DataReporting has a new listener for when the notification is displayed and only starts to poll once that event happens
Attachment #775838 - Flags: review?(gps)
Attachment #774177 - Attachment is obsolete: true
Because we decided to enforce checking that the user has been notified before any data submission can occur, a bunch of code had to be added back in order to facilitate this.
Attachment #777390 - Flags: review?(gps)
Attachment #774173 - Attachment is obsolete: true
Comment on attachment 777390 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

On the right track, but there are numerous important issues that need addressed.

::: services/datareporting/DataReportingService.js
@@ +85,5 @@
>      this.healthReporter.requestDataUpload(request);
>    },
>  
>    onNotifyDataPolicy: function (request) {
> +    Observers.notify("datareporting:notify-data-policy:show", request);

I'm not sure why you changed the same. The functionality seems the same to me!

@@ +281,5 @@
> +      dataReportingVersion = Services.prefs.getIntPref("datareporting.policy.prompedVersion");
> +    } catch (ex) {}
> +
> +    if (dataReportingVersion < 1) {
> +      this.policy.ensureUserNotified();

Now that we restored ensureUserNotified in this version of the patch, I think all this version checking should be in policy.jsm.

::: services/datareporting/policy.jsm
@@ +52,2 @@
>    this.policy = policy;
> +  this.promise = promise;

It is actually a deferred, not a promise. Promises have .then() on them. A deferred has .resolve() and .reject().

@@ +59,3 @@
>     */
> +  onUserNotifyComplete: function () {
> +    this.promise.resolve();

Why did you remove the return? I thought we had tests relying on this. Maybe you got rid of them?

@@ -225,5 @@
>   *     impact performance at startup.
>   *  4. Before first ever data submission, the user should be notified about
> - *     data collection practices.
> - *  5. User should have opportunity to react to this notification before
> - *     data submission.

I think #5 from the old list should be preserved. IMO that would be acting in the best interests of users. That being said, if we prompt on first application run and there is no upload scheduled for +24h, then that counts, IMO. But, we should be clear about the expectation here.

@@ -248,5 @@
> - *   * onNotifyDataPolicy(request) - Called when the policy is requesting the
> - *     user to be notified that data submission will occur. The function
> - *     receives a `NotifyPolicyRequest` instance. The callee should call one or
> - *     more of the functions on that instance when specific events occur. See
> - *     the documentation for that type for more.

You removed onNotifyDataPolicy() from this list yet still use it below!

@@ -407,5 @@
> -   * per-deployment default pref.
> -   */
> -  get dataSubmissionPolicyBypassAcceptance() {
> -    return this._prefs.get("dataSubmissionPolicyBypassAcceptance", false);
> -  },

We /may/ want this either for making tests easier to write and/or for enterprise deployments wishing to consent on behalf of their users. That being said, I'm inclined to keep it out and re-add it if it's needed.

@@ -419,5 @@
> -  get dataSubmissionPolicyNotifiedDate() {
> -    return CommonUtils.getDatePref(this._prefs,
> -                                   "dataSubmissionPolicyNotifiedTime", 0,
> -                                   this._log, OLDEST_ALLOWED_YEAR);
> -  },

Why did you remove this getter but leave the setter? Surely this is needed for testing!

@@ -500,5 @@
> -  },
> -
> -  set dataSubmissionPolicyAcceptedVersion(value) {
> -    this._prefs.set("dataSubmissionPolicyAcceptedVersion", value);
> -  },

This can't be removed: we need to record which version of the policy people have seen so we can reprompt if the terms of the policy have changed. You can consider changing the variable name, but then you'll need to write code that migrates old preferences to the new style.

@@ -510,5 @@
> -   * submission can occur.
> -   *
> -   * @return DataReportingPolicy.STATE_NOTIFY_* constant.
> -   */
> -  get notifyState() {

Why did you remove this? This was quite handy to have IMO.

@@ +605,5 @@
>        return;
>      }
>  
> +    if (!this.ensureUserNotified()) {
> +      this._log.warn("The user has not been yet notified about data upload.");

"The user has not been notified about the data submission policy. Not attempting upload."

@@ +631,3 @@
>     */
> +  ensureUserNotified: function () {
> +    if (this._dataSubmissionPolicyNotifiedDate) {

We also need to check the version here. Although, since we only have 1 version, I suppose we can leave it out. If the old code didn't do it, no need to do it here yet.

::: services/datareporting/tests/xpcshell/test_policy.js
@@ -66,5 @@
> -  do_check_eq(policy.firstRunDate.getTime(), nowT);
> -
> -  policy.dataSubmissionPolicyNotifiedDate= now;
> -  do_check_eq(policyPrefs.get("dataSubmissionPolicyNotifiedTime"), nowT);
> -  do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(), nowT);

You shouldn't have removed these 3 lines since the pref is still there!

@@ -85,5 @@
> -  do_check_false(policyPrefs.get("dataSubmissionPolicyAccepted", true));
> -  do_check_false(policy.dataSubmissionPolicyAccepted);
> -
> -  policy.dataSubmissionPolicyAcceptedVersion = 2;
> -  do_check_eq(policyPrefs.get("dataSubmissionPolicyAcceptedVersion"), 2);

You'll need to restore this test as well.

@@ +100,5 @@
>  
>    run_next_test();
>  });
>  
> +add_test(function test_submission_kill_switch() {

I don't believe this test is named properly.

@@ -143,5 @@
> -
> -  run_next_test();
> -});
> -
> -add_task(function test_initial_submission_notification() {

This test seems mostly still valid and should be restored (modulo removed functionality).

@@ -173,5 @@
> -              policy._dataSubmissionPolicyNotifiedDate.getTime());
> -  do_check_eq(policy.notifyState, policy.STATE_NOTIFY_WAIT);
> -});
> -
> -add_test(function test_bypass_acceptance() {

This should be restored if the bypass acceptance preference is restored.

@@ +130,5 @@
>    policy.healthReportUploadEnabled = false;
>    policy.checkStateAndTrigger();
>    do_check_eq(listener.requestDataUploadCount, 0);
>    policy.healthReportUploadEnabled = true;
> +  policy.dataSubmissionPolicyNotifiedDate = new Date();

Before, we had a nice high-level API to record this (recordUserAcceptance). I suggest restoring such an API (recordPolicyDisplayed?). This will make the code much easier to read. And, it centralizes all the logic for storing what state that entails. e.g. the date, version number, ...

::: services/healthreport/healthreporter.jsm
@@ -1301,5 @@
>      // backoff. We should arguably not do this. However, reporting
>      // startup errors is important. And, they should not occur with much
>      // frequency in the wild. So, it shouldn't be too big of a deal.
> -    if (!inShutdown &&
> -        this._policy.ensureNotifyResponse(new Date()) &&

This is wrong. It will result in us submitting data if the user hasn't agreed to the policy. I'm pretty sure that would violate a law! If we didn't have a test for this, we definitely need one for this exact reason!

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ -751,5 @@
>      yield shutdownServer(server);
>    }
>  });
>  
> -add_task(function test_policy_accept_reject() {

This test should be adapted, not removed.
Attachment #777390 - Flags: review?(gps) → feedback+
Comment on attachment 775838 [details] [diff] [review]
Part 2: Display data reporting notification on first run;

Cancelling until part 1 is settled down.
Attachment #775838 - Flags: review?(gps)
Implemented the long list of abovementioned comments
Attachment #779965 - Flags: review?(gps)
Attachment #777390 - Attachment is obsolete: true
Comment on attachment 779965 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

This is getting very close to r+ from me. Just a few issues remain. Much better than the last patch!

We should probably delete the removed preferences so they don't pollute the preferences store for all of time.

I would just look for user-defined preferences in the policy.jsm constructor and delete them if they exist.

This has the side-effect that if a user downgrades she will be reprompted about the data policy. However, downgrades should be rare and the penalty is a minor UX inconvenience, so I think we can live with it.

::: services/datareporting/DataReportingService.js
@@ +8,5 @@
>  
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://gre/modules/Services.jsm");

I don't think you need to add Services.jsm.

::: services/datareporting/datareporting-prefs.js
@@ -7,5 @@
> -pref("datareporting.policy.dataSubmissionPolicyBypassAcceptance", false);
> -pref("datareporting.policy.dataSubmissionPolicyNotifiedTime", "0");
> -pref("datareporting.policy.dataSubmissionPolicyResponseType", "");
> -pref("datareporting.policy.dataSubmissionPolicyResponseTime", "0");
> -pref("datareporting.policy.firstRunTime", "0");

Can you restore this file with the still-used preferences? I like to have prefs defined so people know they exist.

::: services/datareporting/policy.jsm
@@ +52,5 @@
>   *        (DataReportingPolicy) The policy instance this request came from.
>   * @param deferred
>   *        (deferred) The promise that will be fulfilled when display occurs.
>   */
> +let NotifyPolicyRequest = function (policy, deferred) {

You can just say "function NotifyPolicyRequest(policy, deferred) {".

You don't need the variable assignment unless you are mutating the function (e.g. .bind(this)) or you wish to reference the function by another name.

@@ +179,5 @@
>   *  2. Try to submit as close to 24 hours apart as possible.
>   *  3. Do not submit too soon after application startup so as to not negatively
>   *     impact performance at startup.
>   *  4. Before first ever data submission, the user should be notified about
> + *     data collection practices

Nit: Add period.

@@ +202,5 @@
>   *
> + *   * onRequestRemoteDelete(request) - Called when the policy is requesting
> + *     deletion of remotely stored data. The function is passed a
> + *     `DataSubmissionRequest`. The listener should call one of the special
> + *     resolving functions on that instance (just like `onRequestDataUpload`).

Nit: Don't move this text block if it doesn't change.

@@ +355,5 @@
>     */
>    get notifyState() {
> +    return  this._dataSubmissionPolicyNotifiedDate !== null &&
> +            this._dataSubmissionPolicyNotifiedDate.getTime() > 0 &&
> +            this.dataSubmissionPolicyNotifiedVersion == POLICY_VERSION;

We should rename this to notificationCurrent since the API changed. Also, use >= for the version compare. Presumably if a user has accepted a future version of the policy then all is well. Be sure there is test coverage for this!

@@ +642,5 @@
>      }
>  
> +    if (!this.ensureUserNotified()) {
> +      this._log.warn("The user has not been notified about the data submission" +
> +                      " policy. Not attempting upload.");

Nit: We typically put the space at the end of a string rather than the beginning.

@@ +673,5 @@
>  
> +    let deferred = Promise.defer();
> +    deferred.promise.then(function onSuccess() {
> +      this.dataSubmissionPolicyNotifiedDate = this.now();
> +      this.dataSubmissionPolicyNotifiedVersion = POLICY_VERSION;

I think the logic for "record data policy notification" should live in a standalone function. This will make testing easier and will make it more clear how things work.

::: services/datareporting/tests/xpcshell/test_policy.js
@@ +128,3 @@
>    do_check_eq(listener.notifyUserCount, 0);
>  
> +  // Currently, the notification is displayed immediately upon load

Nit: Always use periods.

@@ +133,5 @@
> +  do_check_eq(listener.requestDataUploadCount, 0);
> +
> +  yield ensureUserNotifiedAndTrigger(policy);
> +
> +  do_check_true(policy._dataSubmissionPolicyNotifiedDate !== null);

do_check_neq(..., null)

@@ -228,5 @@
> -  do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(), 0);
> -  do_check_eq(policy.notifyState, policy.STATE_NOTIFY_UNNOTIFIED);
> -});
> -
> -add_task(function test_notification_accepted() {

It seems like the explicit test_notification_accepted and test_notification_rejected tests should be retained in some form. We definitely want a standalone test that ensures a failure to notify is handled properly.

@@ +167,5 @@
>    policy.healthReportUploadEnabled = false;
>    policy.checkStateAndTrigger();
>    do_check_eq(listener.requestDataUploadCount, 0);
>    policy.healthReportUploadEnabled = true;
> +  yield ensureUserNotifiedAndTrigger(policy);

This should be at the top of the test, otherwise you aren't testing what the test says it is testing!

::: services/healthreport/healthreporter.jsm
@@ +1222,5 @@
>    /**
>     * Whether this instance will upload data to a server.
>     */
>    get willUploadData() {
> +    return (this._policy._dataSubmissionPolicyNotifiedDate !== null) &&

This should just call into the helper API in policy, not a low-level implementation detail.

@@ +1301,5 @@
>      // startup errors is important. And, they should not occur with much
>      // frequency in the wild. So, it shouldn't be too big of a deal.
>      if (!inShutdown &&
> +          this._policy.healthReportUploadEnabled &&
> +          this._policy.ensureUserNotified()) {

I'm debating whether we should call ensureUserNotified() or whether we should just check the notification flag. Think about this, write a comment, and we'll hash it out during the next review.
Attachment #779965 - Flags: review?(gps) → feedback+
Implemented the above mentioned comments

> >      // startup errors is important. And, they should not occur with much
> >      // frequency in the wild. So, it shouldn't be too big of a deal.
> >      if (!inShutdown &&
> > +          this._policy.healthReportUploadEnabled &&
> > +          this._policy.ensureUserNotified()) {
> 
> I'm debating whether we should call ensureUserNotified() or whether we should just check the notification flag. 
> Think about this, write a comment, and we'll hash it out during the next review.

The error data submission will not occur as the first call to ensureUserNotified() returns false and proceeds to async-ly notify the user. The current workflow is first create HealthReporter and later notify (but upload after 24h). Currently it would not make much difference as we don't have a fallback if HealthReporter fails to initialize, but in an ideal circumstance where there would be any subsequent tries to re-instantiate HealthReporter upon failure, this would actually cause a data submission from the 2nd try onwards. 
All in all, given that this issue can only happen once per installation in the rare case that healthreport fails to initialize before the user has ever been notified, I don't see any reason why we should change it - also, if pressumably the fatal init to HealthReporter is followed by, lets say, a crash, we would actually submit the info after the browser restarts.
Attachment #782896 - Flags: review?(gps)
Attachment #779965 - Attachment is obsolete: true
Comment on attachment 782896 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

Every patch version is getting better. The major remaining hurdle is dealing with application upgrades. We want to delete old, legacy preferences on upgrade. And, we want to convert old preferences to the new ones. e.g. accept time to notified time. If we fail to do the latter, we'll renotify all our users for no good reason. We don't like any notifications, so failing to carry the settings forward in not acceptable. We'll definitely need tests for these scenarios!

It's getting close.

::: services/datareporting/policy.jsm
@@ -281,5 @@
>  
> -  // If we've never run before, record the current time.
> -  if (!this.firstRunDate.getTime()) {
> -    this.firstRunDate = this.now();
> -  }

Can you keep this in? There were some whisperings a few months back regarding wanting FHR to report its activation date (something beyond the 180 days of history we currently retain). This preference would allow us to backfill that data. If we delete it, we'll lose that data for new users.

I don't like saying we should retain this because of a non-yet-manifested requirement. But, I have a strong hunch Metrics will ask for it at some time. The cost of keeping it is low, so...

@@ +321,5 @@
>    },
>  
>    set dataSubmissionPolicyNotifiedDate(value) {
>      this._log.debug("Setting user notified date: " + value);
> +    this._dataSubmissionPolicyNotifiedDate = value;

Why do you keep an internal shadow value? The old shadow value was to prevent recording until implicit acceptance had passed. Since that's gone and we don't record state until the promise is resolved, I don't believe we need this.

@@ +347,4 @@
>    },
>  
> +  set dataSubmissionPolicyNotifiedVersion(value) {
> +    this._prefs.set("dataSubmissionPolicyNotifiedVersion", value);

You changed the name of this preference. What happens to existing users that have already accepted the old policy? I'm going to assert we don't want to reprompt over 100 millions users needlessly.

@@ +693,3 @@
>  
> +  _recordDataPolicyNotification: function (date, version) {
> +    this.dataSubmissionPolicyNotifiedDate = date;

Please log here!

::: services/datareporting/tests/xpcshell/test_policy.js
@@ -49,5 @@
>  
>    let tomorrow = Date.now() + 24 * 60 * 60 * 1000;
>    do_check_true(tomorrow - policy.nextDataSubmissionDate.getTime() < 1000);
>  
> -  do_check_eq(policy.notifyState, policy.STATE_NOTIFY_UNNOTIFIED);

Instead of replacing, replace with a check for policy.notificationCurrent.

@@ -66,5 @@
> -  do_check_eq(policy.firstRunDate.getTime(), nowT);
> -
> -  policy.dataSubmissionPolicyNotifiedDate= now;
> -  do_check_eq(policyPrefs.get("dataSubmissionPolicyNotifiedTime"), nowT);
> -  do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(), nowT);

We still have this pref. Why did you remove the test?

@@ +128,4 @@
>  
> +  let reset_working = () => {
> +    // Initialize the policy with any non zero time. 42 minutes were chosen randomly.
> +    policy._dataSubmissionPolicyNotifiedDate = new Date(42 * 60 * 1000);

This date is in 1970!

@@ +136,2 @@
>  
> +  is_false('The initial state should be unnotified.');

I don't think you need this is_true, is_false stuff. Just call do_check_*.

@@ +183,2 @@
>  
> +  // Currently, the notification is displayed immediately upon load.

I think a more appropriate comment is that uploads will trigger notification as needed.

@@ -213,5 @@
> -  do_check_eq(policy.dataSubmissionPolicyResponseDate.getTime(), policy.now().getTime());
> -  do_check_eq(policy.dataSubmissionPolicyResponseType, "accepted-implicit-time-elapsed");
> -});
> -
> -add_task(function test_notification_rejected() {

We should still have a test that rejects the notification promise, just to be thorough.

::: services/healthreport/healthreporter.jsm
@@ +1222,5 @@
>    /**
>     * Whether this instance will upload data to a server.
>     */
>    get willUploadData() {
> +    return (this._policy._dataSubmissionPolicyNotifiedDate !== null) &&

Shouldn't this query policy.notificationCurrent? Since this doesn't check the policy version and all the tests presumably pass, this points to a lapse in test coverage!

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +672,5 @@
>      reporter._providerManager.registerProvider(new DummyProvider());
>  
>      let policy = reporter._policy;
>  
>      defineNow(policy, policy._futureDate(-24 * 60 * 68 * 1000));

68. That's wrong.

Do we even need to go back 24h now that we removed the 24h delay?

@@ +744,3 @@
>      do_check_false(reporter.willUploadData);
>  
> +    yield ensureUserNotified(reporter);

Does this whole test function do anything any more?
Attachment #782896 - Flags: review?(gps) → feedback+
Updated with the previos mentioned comments

> @@ +744,3 @@
> >      do_check_false(reporter.willUploadData);
> >  
> > +    yield ensureUserNotified(reporter);
>
> Does this whole test function do anything any more?

As it so happens, this test actually failed when I removed the shadow value from dataSubmissionPolicyNotifiedDate due to outdated code. This is supposed to be healthreporter's test to check if the policy is working as it should - basically policy acceptance so I say it is ok to keep it for now
Attachment #785983 - Flags: review?(gps)
Attachment #782896 - Attachment is obsolete: true
Refreshed this patch to be compatible with the latest Part 1
Attachment #785993 - Flags: review?(gps)
Comment on attachment 785983 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

Every patch gets closer to r+. However, this patch still fails to address migrating old preference values to the new world. We *must* migrate old preference values when users upgrade to this code. This patch will not get r+ until this is done.

::: services/datareporting/policy.jsm
@@ +313,5 @@
>      "onRequestRemoteDelete",
>      "onNotifyDataPolicy",
>    ],
>  
> +

Nit: Lose the extra newline.

::: services/datareporting/tests/xpcshell/test_policy.js
@@ +61,5 @@
>      onNotifyDataPolicy: function() {},
>    };
>  
>    let policy = new DataReportingPolicy(policyPrefs, hrPrefs, listener);
> +  do_check_true(Date.now() - policy.firstRunDate.getTime() < 1024);

Why did you increase this?

@@ -66,5 @@
>    do_check_eq(policy.firstRunDate.getTime(), nowT);
>  
> -  policy.dataSubmissionPolicyNotifiedDate= now;
> -  do_check_eq(policyPrefs.get("dataSubmissionPolicyNotifiedTime"), nowT);
> -  do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(), nowT);

This didn't go away, so don't remove the test.

@@ +136,5 @@
> +  let reset_working = () => {
> +    // Initialize the policy with any non zero time
> +    policy.dataSubmissionPolicyNotifiedDate = new Date();
> +    policy.dataSubmissionPolicyAcceptedVersion = DATAREPORTING_POLICY_VERSION;
> +  };

I would call policy._recordDataPolicyNotification instead.

@@ +143,3 @@
>  
> +  policy.dataSubmissionPolicyAcceptedVersion = DATAREPORTING_POLICY_VERSION;
> +  do_check_false(policy.notificationCurrent, 'The default state of the date should have a time of 0 and it should therefore fail');

Can you also check the == 0 part?

@@ +169,5 @@
> +
> +  yield ensureUserNotifiedAndTrigger(policy);
> +
> +  do_check_neq(policy.dataSubmissionPolicyNotifiedDate, null);
> +  do_check_true(policy.dataSubmissionPolicyNotifiedDate.getTime() > 0);

Please add checks for the expected old/default values before the checkStateAndTrigger().

@@ +171,5 @@
> +
> +  do_check_neq(policy.dataSubmissionPolicyNotifiedDate, null);
> +  do_check_true(policy.dataSubmissionPolicyNotifiedDate.getTime() > 0);
> +  do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(),
> +              policy.dataSubmissionPolicyNotifiedDate.getTime());

What's the point of this test?

@@ +191,4 @@
>    do_check_true(policy.dataSubmissionPolicyNotifiedDate.getTime() > 0);
>    do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(),
> +              policy.dataSubmissionPolicyNotifiedDate.getTime());
> +  do_check_true(policy.notificationCurrent);

How is this test function different from the 1 above?

@@ -246,5 @@
> -  do_check_true(policy.dataSubmissionPolicyAccepted);
> -  do_check_eq(policy.dataSubmissionPolicyResponseDate.getTime(), now.getTime());
> -});
> -
> -add_task(function test_notification_rejected() {

So we no longer have test coverage for a rejected promise when the notification request fails?

@@ +245,3 @@
>    let [policy, policyPrefs, hrPrefs, listener] = getPolicy("data_submission_no_data");
>  
>    policy.dataSubmissionPolicyResponseDate = new Date(Date.now() - 24 * 60 * 60 * 1000);

Didn't we remove this pref?

@@ -405,5 @@
>    let [policy, policyPrefs, hrPrefs, listener] = getPolicy("submission_far_future_scheduling");
>  
> -  let now = new Date(Date.now() - 24 * 60 * 60 * 1000);
> -  defineNow(policy, now);
> -  policy.recordUserAcceptance();

You fundamentally changed this test by removing policy acceptance! Now, the listener.requestDataUploadCount is 0 because the policy hasn't bee accepted, not because of the scheduled upload date!

@@ +336,5 @@
>  
>    let nextDate = policy._futureDate(3 * 24 * 60 * 60 * 1000 - 1);
>    policy.nextDataSubmissionDate = nextDate;
>    policy.checkStateAndTrigger();
>    do_check_eq(listener.requestDataUploadCount, 0);

We should verify the policy is current here, just to be sure we didn't upload because of that.

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ -733,5 @@
>      do_check_true(reporter.willUploadData);
> -
> -    policy.recordUserRejection();
> -    do_check_false(policy.dataSubmissionPolicyAccepted);
> -    do_check_false(reporter.willUploadData);

Another place where we regressed our coverage of failed notification. These tests are important: if we regress behavior here, we may be in violation of privacy laws!
Attachment #785983 - Flags: review?(gps) → feedback+
Comment on attachment 785993 [details] [diff] [review]
Part 2: Display data reporting notification on first run;

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

::: browser/base/content/browser.js
@@ +1354,5 @@
>      TabsInTitlebar.uninit();
>  
> ++#ifdef MOZ_DATA_REPORTING
> ++    gDataNotificationInfoBar.init();
> ++#endif

You moved this to onUnload?!

::: browser/base/content/test/browser_datareporting_notification.js
@@ -77,5 @@
>        waitForNotificationClose(notification.currentNotification, function onClose() {
> -        is(policy.notifyState, policy.STATE_NOTIFY_COMPLETE, "Closing info bar completes user notification.");
> -        ok(policy.dataSubmissionPolicyAccepted, "Data submission policy accepted.");
> -        is(policy.dataSubmissionPolicyResponseType, "accepted-info-bar-dismissed",
> -           "Reason for acceptance was info bar dismissal.");

You still set some prefs in the info bar handler. We should verify those!
Attachment #785993 - Flags: review?(gps)
> > -  do_check_true(policy.dataSubmissionPolicyAccepted);
> > -  do_check_eq(policy.dataSubmissionPolicyResponseDate.getTime(), now.getTime());
> > -});
> > -
> > -add_task(function test_notification_rejected() {
> 
> So we no longer have test coverage for a rejected promise when the notification request fails?

That function name was a duplicate. There is a second (now unique only) test called test_notification_rejected
Attachment #790437 - Flags: review?(gps)
Attachment #785983 - Attachment is obsolete: true
Blocks: 831404
Comment on attachment 790437 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

This is *very* close to an r+. So very close, in fact, I'm asking rnewman to start looking at it.

Will get r+ from me once the prefs are just right.

::: services/datareporting/policy.jsm
@@ +718,5 @@
> +    let prefs = this._prefs;
> +    let notifiedTime = this.dataSubmissionPolicyNotifiedDate;
> +    let oldVersion = prefs.get("dataSubmissionPolicyAcceptedVersion", 0);
> +
> +    // We slightly changed the function of dataSubmissionPolicyNotifiedDate but

s/function/meaning

@@ +719,5 @@
> +    let notifiedTime = this.dataSubmissionPolicyNotifiedDate;
> +    let oldVersion = prefs.get("dataSubmissionPolicyAcceptedVersion", 0);
> +
> +    // We slightly changed the function of dataSubmissionPolicyNotifiedDate but
> +    //  we kept the same name so we only have to migrate the side effects of the pref.

Nit: Whitespace

@@ +728,1 @@
>      }

Please delete the old preferences after migration. This will make downgrading slightly annoying since it will result in a new notification. But, downgrades *are* annoying and these things can be expected.

::: services/datareporting/tests/xpcshell/test_policy.js
@@ +137,5 @@
>  
> +add_task(function test_migratePrefs () {
> +  let [policy, policyPrefs, hrPrefs, listener] = getPolicy("migratePrefs");
> +  let notifiedTime = CommonUtils.getDatePref(prefs, "dataSubmissionPolicyNotifiedTime",
> +                                                0, this._log, OLDEST_ALLOWED_YEAR);

Nit: Align 0 with the "p" in "prefs"

@@ +142,5 @@
>  
> +  policy.dataSubmissionPolicyAcceptedVersion = 0;
> +  policy.dataSubmissionPolicyNotifiedDate = new Date(Date.now());
> +  policy._migratePrefs();
> +  do_check_eq(policy.dataSubmissionPolicyAcceptedVersion, DATAREPORTING_POLICY_VERSION);

So, if the user hadn't accepted the policy under the old version of the client, upon upgrading/migrating they will have accepted it. That seems... not right.

I'd feel much better if the migration code only moved preferences forward if both the notified time and version prefs were set.

What do you think? (Also, rnewman may have a different opinion here.)

@@ +147,4 @@
>  
> +  policy.dataSubmissionPolicyAcceptedVersion = DATAREPORTING_POLICY_VERSION + 1;
> +  policy._migratePrefs();
> +  do_check_eq(policy.dataSubmissionPolicyAcceptedVersion, DATAREPORTING_POLICY_VERSION + 1);

I'd feel much better if you used a new policy object and prefs branch for this 2nd test.
Attachment #790437 - Flags: review?(rnewman)
Attachment #790437 - Flags: review?(gps)
Attachment #790437 - Flags: feedback+
Comment on attachment 790437 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

::: services/datareporting/policy.jsm
@@ +26,5 @@
>  Cu.import("resource://services-common/utils.js");
>  
> +// The current policy version number. If the version number stored in the prefs
> +// is smaller than this, data upload will be disabled until the user is re-notified
> +// about the policy changes.

Not really; data upload will be disabled until the user's acceptance of the new policy version is recorded.

@@ +373,2 @@
>     */
> +  get notificationCurrent() {

I'd prefer a better name, like "userNotifiedOfCurrentPolicy".

@@ +723,5 @@
> +    //  we kept the same name so we only have to migrate the side effects of the pref.
> +    // Since previous versions checked to see if the user was notified, we can
> +    // just go and record notification if that event has happened.
> +    if (notifiedTime && notifiedTime.getYear() >= OLDEST_ALLOWED_YEAR) {
> +      this.dataSubmissionPolicyAcceptedVersion = Math.max(DATAREPORTING_POLICY_VERSION, oldVersion);

Easy, tiger. You're going to record the user's acceptance of the current policy just because they've seen an old one?

::: services/datareporting/tests/xpcshell/test_policy.js
@@ +142,5 @@
>  
> +  policy.dataSubmissionPolicyAcceptedVersion = 0;
> +  policy.dataSubmissionPolicyNotifiedDate = new Date(Date.now());
> +  policy._migratePrefs();
> +  do_check_eq(policy.dataSubmissionPolicyAcceptedVersion, DATAREPORTING_POLICY_VERSION);

To echo an earlier comment: you definitely shouldn't fast-forward versions. If we consider notification to be acceptance then I think it's OK to reinterpret old prefs accordingly.

::: services/healthreport/healthreporter.jsm
@@ +1301,5 @@
>      // startup errors is important. And, they should not occur with much
>      // frequency in the wild. So, it shouldn't be too big of a deal.
>      if (!inShutdown &&
> +          this._policy.healthReportUploadEnabled &&
> +          this._policy.ensureUserNotified()) {

Indenting.
Attachment #790437 - Flags: review?(rnewman) → feedback+
> >  Cu.import("resource://services-common/utils.js");
> >  
> > +// The current policy version number. If the version number stored in the prefs
> > +// is smaller than this, data upload will be disabled until the user is re-notified
> > +// about the policy changes.

> Not really; data upload will be disabled until the user's acceptance of the new policy version is recorded.

@rnewman: "user acceptance" is defined as the display of the data-reporting notification for the current version. Therefore if there is a smaller version stored in the prefs, data upload will be disable until a re-notification occurs. Or did I get your comment wrong?
Attachment #794897 - Flags: review?(rnewman)
Attachment #794897 - Flags: review?(gps)
Attachment #790437 - Attachment is obsolete: true
(In reply to Stefan Mirea [:smirea] from comment #35)

> @rnewman: "user acceptance" is defined as the display of the data-reporting
> notification for the current version. Therefore if there is a smaller
> version stored in the prefs, data upload will be disable until a
> re-notification occurs. Or did I get your comment wrong?

Just niceties of semantics.

IMO we care about 'acceptance'. This bug is really "remove delayed implicit acceptance" -- all we're doing is conflating display with acceptance, which allows for some simplification.

And so IMO we're disabling upload until they 'accept' the new policy version, which will occur when they've seen the notification, and the comment should reflect that intent. Comments should introduce clarity, even when what we're doing is smudged!

Sidenote: there is a pain gap here. Say you install a new version of Firefox, and you happen to do so when an FHR upload is overdue.

We've updated our policy, so we bump the version. This code sees that, spits out the notification, then the timer code runs and decides to do an upload. The assumption that we won't upload until 24h have passed (Comment 3) is only true for first run -- this poor user will have their FHR data uploaded before a human eye could even read the notification.

Generally speaking we're just ignoring that (unlikely?) pain.
Some clarification: there should be no definition of "acceptance" here, in code/comments/UI.  We do notify users, not because it is required legally, or because we're looking to imply acceptance, but because that's how we roll as Mozilla in terms of transparency.  No acceptance is required for FHR to function by default, in the same way that update, blocklist, and other features all Just Work.

Having a mechanism to provide update about what we're doing in a user-transparent way is not something that should ever be linked to submission of data or functioning of the user feature. Make sense?
Comment on attachment 794897 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

Thanks for clarifying, Mike!
Attachment #794897 - Flags: review?(rnewman) → review+
Comment on attachment 794897 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking;

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

Yay! Where's the review on part 2?
Attachment #794897 - Flags: review?(gps) → review+
Implemented tests for preference from gDataNotificationInfoBar. Also set an attribute gDataNotificationInfoBar._prefBranch so it is not hard-coded in the method
Attachment #796834 - Flags: review?(gps)
Attachment #775838 - Attachment is obsolete: true
Attachment #785993 - Attachment is obsolete: true
Comment on attachment 796834 [details] [diff] [review]
Part 2: Display data reporting notification on first run;

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

Patch is on the right track. Just needs some refinement.

::: browser/base/content/browser-data-submission-info-bar.js
@@ +77,5 @@
>        accessKey: gNavigatorBundle.getString("dataReportingNotification.button.accessKey"),
>        popup: null,
>        callback: function () {
> +        this._setDatePref("responseTime", new Date());
> +        this._prefs.set("responseType", "learn-more");

Throughout this file you should be calling methods on the policy rather than setting prefs directly. A goal of the policy API is to abstract the storage of state from consumers. Writing directly into prefs works against this goal.

::: browser/base/content/browser.js
@@ +937,5 @@
>      retrieveToolbarIconsizesFromTheme();
>  
> +#ifdef MOZ_DATA_REPORTING
> +    gDataNotificationInfoBar.init();
> +#endif

Please remind me why this was moved again? This /could/ incur a performance hit since it is initialized earlier. It would be desirable to avoid that, if possible.

::: browser/base/content/test/browser_datareporting_notification.js
@@ +85,3 @@
>          is(notification.allNotifications.length, 0, "No notifications remain.");
> +        is(policy._prefs.get("promptedVersion", 0), 1, "Version pref set.");
> +        ok(ns.CommonUtils.getDatePref(policy._prefs, "notifyTime", -1).getTime() > -1, "notifyTime pref set.");

Isn't there an API for this on the policy object? There should be.

@@ -146,5 @@
> -        is(policy.notifyState, policy.STATE_NOTIFY_COMPLETE,
> -           "Closing info bar with multiple windows completes notification.");
> -        ok(policy.dataSubmissionPolicyAccepted, "Data submission policy accepted.");
> -        is(policy.dataSubmissionPolicyResponseType, "accepted-info-bar-button-pressed",
> -           "Policy records reason for acceptance was button press.");

Why did you remove this test coverage?

::: browser/components/preferences/in-content/tests/browser_healthreport.js
@@ -28,5 @@
>    runPaneTest(testBasic);
>  }
>  
>  function testBasic(win, doc, policy) {
> -  is(policy.dataSubmissionPolicyAccepted, false, "Data submission policy not accepted.");

This is an important precondition to test!

::: browser/components/preferences/tests/browser_healthreport.js
@@ -42,5 @@
>  
>  function testBasic(win, policy) {
>    let doc = win.document;
>  
> -  is(policy.dataSubmissionPolicyAccepted, false, "Data submission policy not accepted.");

Ditto.

::: services/healthreport/healthreporter.jsm
@@ -647,4 @@
>      if (this._initialized) {
>        return CommonUtils.laterTickResolvingPromise(this);
>      }
> -

Why make these changes?
Attachment #796834 - Flags: review?(gps) → feedback+
Implemented changes
Attachment #800403 - Flags: review?(gps)
Attachment #796834 - Attachment is obsolete: true
Comment on attachment 800403 [details] [diff] [review]
Part 2: Display data reporting notification on first run;

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

This looks good. Do you have a try result showing it passes? At this point, we should probably hold off landing until after the tree merge on Monday. Give this more time to bake so bugs can be flushed out without going through uplift.

::: browser/base/content/test/browser_datareporting_notification.js
@@ +91,5 @@
>    }, true);
>  
>    policy = sendNotifyRequest("single_window_notified");
> +  is(policy.dataSubmissionPolicyAcceptedVersion, 0, "No version should be set on init.");
> +  is(policy.dataSubmissionPolicyNotifiedDate.getTime(), 0, "No date should be set on init.");

Probably want a userNotifiedOfCurrentPolicy == false check somewhere.
Attachment #800403 - Flags: review?(gps) → review+
Given that this bug has been going back and forth for ages and that it finally got 2 r+, it pains me to do this, but alas it has to be done:

The code worked, but some browser chrome mochi-tests were designed less than ideally and they failed on tbpl (couldn't reproduce locally which made this even more fun).
Mainly:
 - browser/base/content/test/browser_datareporting_notification.js
 - browser/components/preferences/tests/browser_healthreport.js
as seen here: https://tbpl.mozilla.org/?tree=Try&rev=7dd73c516993

This was happening for some reasons:
 Main reason (amongst other small ones): shared resources between tests so other tests ended up displaying the data policy notification and changing the policy preferences. Due to the way gDataNotificationInfoBar works (singleton) once the notification was displayed (and not closed) it would not display again so events in the above mentioned tests that dependend on notifications firing an AlertActive event on creation just hang.

Given that you cannot count on the fact that other tests will clean up after themselves, I changed these tests to make sure their resources are set to default - clear notifications, reset policy prefs and attach a new policy to the DataReportingService as well as adding a promise in policy.jsm to track when the user has been successfully notified.

These fixed them here: https://tbpl.mozilla.org/?tree=Try&rev=a68deff56baa

I merged the added patch to Part 2 and re-ran the whole suite here, just to be 100% sure: https://tbpl.mozilla.org/?tree=Try&rev=41d84f98d8a5
Attachment #806964 - Flags: review?(gps)
Attachment #800403 - Attachment is obsolete: true
Some unrelated tests started failing locally because policy was trying to export DATAREPORTING_POLICY_VERSION using the const keyword which apparently does not work anymore. Minor fix was mad by replacing the definition with Object.defineProperty(this, ...).

Other than then, see above comment for more changes
Attachment #807266 - Flags: review?(gps)
Attachment #806964 - Attachment is obsolete: true
Attachment #806964 - Flags: review?(gps)
Comment on attachment 807266 [details] [diff] [review]
Part 2: Display data reporting notification on first run; r=gps

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

Reading your patch comments and the code, it appears that what's happening is the info bar is being displayed during some other test (as part of the overall test suite) and this is interfering with the data submission info bar tests. Is this an accurate statement?

If so, this is a regression from the current behavior and can't be checked in.

The existing behavior (before the patches in this bug) is for the info bar to be suppressed during test runs. This is accomplished by the presence of the datareporting.policy.dataSubmissionPolicyBypassAcceptance preference in testing/profiles/prefs_general.js and layout/tools/reftest/remotereftest.py.

We *must* suppress the info bar during tests because some tests won't like it if the info bar appears while they are running. This will almost certainly cause intermittent oranges and possibly hard failures if the right conditions are ever met.

Essentially, you need to prevent the info bar from being displayed during tests until the tests that actual test its functionality are called.

Another issue with this patch is that browser_healthreport.js *appears* to now rely on the info bar being displayed. This isn't a good assumption. If you run the tests in random order or in isolation, this test may fail.

::: browser/base/content/test/browser_datareporting_notification.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +function closeAllNotifications () {

You could probably make this a helper function in some shared file. Not sure where that would be, however. So, meh.
Attachment #807266 - Flags: review?(gps)
Re-implemented our old friend dataReportingPolicyBypassAcceptance pref and set it to true in the head.js for all mochitest browser tests. Now tests that wish to specifically test it need to explicitly enable it and (hopefully) disable it back in the cleanup function.

Green tpbl: https://tbpl.mozilla.org/?tree=Try&rev=635d64263f45
Attachment #807552 - Flags: review?(gps)
Attachment #807266 - Attachment is obsolete: true
Comment on attachment 807552 [details] [diff] [review]
Part 2: Display data reporting notification on first run;  * *

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

This is heading in the right direction. There a few major issues preventing this from attaining r+. It is *very* close, however.

Both preference versions of browser_healthreport.js should be in sync. It's weird (probably wrong) to see different changes in the two files.

::: browser/base/content/test/general/browser_datareporting_notification.js
@@ +236,5 @@
> +
> +  let policyPrefs = new ns.Preferences("datareporting.policy.");
> +  let policy = new ns.DataReportingPolicy(policyPrefs, service._prefs, service);
> +  service.policy = policy;
> +  setDataSubmissionPolicyBypassAcceptance(true);

That's a lot of code! I'd just reset the prefs branch or the individual pref.

::: browser/base/content/test/general/head.js
@@ +17,5 @@
> +                                     .getService(Components.interfaces.nsISupports)
> +                                     .wrappedJSObject
> +                                     .policy;
> +  policy.dataSubmissionPolicyBypassAcceptance = true;
> +}

Search for "PolicyBypassAcceptance" in the tree and you'll find references in layout/tools/reftest/remotereftest.py and testing/profiles/prefs_general.js. Believe it or not, it's preferred to use these files instead of modifying head.js.

::: browser/components/preferences/tests/browser_healthreport.js
@@ +17,5 @@
> +    // the policy in which case resetPreferences() won't do much good.
> +    policy._prefs.reset("dataSubmissionEnabled");
> +    policy._prefs.reset("firstRunTime");
> +    policy._prefs.reset("dataSubmissionPolicyNotifiedTime");
> +    policy._prefs.reset("dataSubmissionPolicyAcceptedVersion");

Can we reset the entire branch? That seems like the best thing to do. I'm not sure why the existing tests don't do this. Probably my laziness :/

::: services/datareporting/datareporting-prefs.js
@@ +5,5 @@
>  pref("datareporting.policy.dataSubmissionEnabled", true);
>  pref("datareporting.policy.firstRunTime", "0");
>  pref("datareporting.policy.dataSubmissionPolicyNotifiedTime", "0");
> +pref("datareporting.policy.dataSubmissionPolicyAcceptedVersion", 0);
> +pref("datareporting.policy.dataSubmissionPolicyBypassAcceptance", true);
\ No newline at end of file

Default value of true?! That is very wrong.

::: services/datareporting/policy.jsm
@@ +34,5 @@
> +  enumerable: false,
> +  configurable: false,
> +  writable: false,
> +  value: 1
> +});

A pattern I've seen is to assign to this.SYMBOL using regular assignment and then having a loop over the symbols you want to constify and apply Object.defineProperty to make them not writable. A bit hacky. But it gets the job done, especially if you have multiple exported constants from a file. Probably not worth the complication here though.

@@ +350,5 @@
>                              value, OLDEST_ALLOWED_YEAR);
>    },
>  
> +  get dataSubmissionPolicyBypassAcceptance() {
> +    return this._prefs.get("dataSubmissionPolicyBypassAcceptance", false);

We should call this "BypassNotification" since we don't want to use the term "acceptance" in this code. And, that more accurately describes what the pref does now anyway.

This was always a hidden pref, so I'm not too worried about backwards compatibility. Adding it to pref migration would be nice, however.

@@ +711,5 @@
> +      this._userNotifyPromise = null;
> +    }.bind(this), function onError(error) {
> +      this._log.warn("Data policy notification presentation failed: " +
> +                     CommonUtils.exceptionStr(error));
> +    }.bind(this));

Fat arrow functions!
Attachment #807552 - Flags: review?(gps) → feedback+
> Fat arrow functions!
Apparently () => {}.bind(this) is an invalid format and needs to be wrapped in () that's why I tend to use the classical function () {}, easier to read imo, but changed anyways

> Default value of true?! That is very wrong.
Forgot to change it from prefs, nice catch!
Attachment #808838 - Flags: review?(gps)
Attachment #807552 - Attachment is obsolete: true
Comment on attachment 808838 [details] [diff] [review]
Part 2: Display data reporting notification on first run;

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

Aside from nits, this looks good to me. Please fix nits, submit to rnewman for review, and carry my r+ forward.

::: browser/components/preferences/tests/browser_healthreport.js
@@ +17,5 @@
> +    // the policy in which case resetPreferences() won't do much good.
> +    policy._prefs.reset("dataSubmissionEnabled");
> +    policy._prefs.reset("firstRunTime");
> +    policy._prefs.reset("dataSubmissionPolicyNotifiedTime");
> +    policy._prefs.reset("dataSubmissionPolicyAcceptedVersion");

Why don't you reset the entire branch?

@@ +75,5 @@
> +  let service = Components.classes["@mozilla.org/datareporting/service;1"]
> +                                  .getService(Components.interfaces.nsISupports)
> +                                  .wrappedJSObject;
> +  service.policy._prefs.resetBranch("datareporting.policy.");
> +  service.policy.dataSubmissionPolicyBypassNotification = true;

Just reset the pref instead of assigning a value. Since the prefs file defines true as the value, reset should do the right thing (I think).

::: services/datareporting/policy.jsm
@@ +710,5 @@
> +      this._recordDataPolicyNotification(this.now(), DATAREPORTING_POLICY_VERSION);
> +      this._userNotifyPromise = null;
> +    }).bind(this), ((error) => {
> +      this._log.warn("Data policy notification presentation failed: " +
> +                     CommonUtils.exceptionStr(error));

Since we don't unset this._userNotifyPromise, we'll never prompt again.

While you can make that argument that a failure will likely result in more failures, you can also make the argument that this may fire too early in app startup, causing it to fail, and that subsequent attempts will work.

The more robust solution is to unset userNotifyPromise in the case of failure. Do it. No test needed.

::: services/datareporting/tests/xpcshell/test_policy.js
@@ -138,5 @@
>  add_task(function test_migratePrefs () {
>    let [policy, policyPrefs, hrPrefs, listener] = getPolicy("migratePrefs");
>    let outdated_prefs = {
>      dataSubmissionPolicyAccepted: true,
> -    dataSubmissionPolicyBypassAcceptance: true,

I don't believe you need to remove this since you should be deleting the pref.

@@ +197,5 @@
>    let [policy, policyPrefs, hrPrefs, listener] = getPolicy("notification_accept_displayed");
>  
>    do_check_eq(listener.requestDataUploadCount, 0);
>    do_check_eq(listener.notifyUserCount, 0);
> +  do_check_eq(policy._userNotifyPromise, null, "No initial notification attempt.");

do_check_null()

@@ +211,5 @@
>    do_check_true(policy.dataSubmissionPolicyNotifiedDate.getTime() > 0);
>    do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(),
>                policy.dataSubmissionPolicyNotifiedDate.getTime());
>    do_check_true(policy.userNotifiedOfCurrentPolicy);
> +  do_check_eq(policy._userNotifyPromise, null);

do_check_null()

::: testing/profiles/prefs_general.js
@@ +106,5 @@
>  user_pref("network.activity.blipIntervalMilliseconds", 250);
>  
> +// We do not wish to display datareporting policy notifications as it might
> +// cause other tests to fail. Tests that wish to test the notification functionallity
> +// should explicitly disdable this pref.

Fix spelling.
Attachment #808838 - Flags: review?(gps) → review+
Anthony: In the course of working on this bug, I realized we likely don't have manual test coverage of the display of FHR's data policy notification info bar in the browser. The display of this bar is important because of legal implications and I would like to have it added to the series of tests manually performed during Firefox releases. Just to be explicit, we do have automatic test coverage. But this is important and I think manual redundancy is warranted.

What's the process for adding a manual testing procedure?
Flags: needinfo?(anthony.s.hughes)
(In reply to Gregory Szorc [:gps] from comment #52)
> we do have automatic test coverage. But this is important and I think manual redundancy is warranted.

TL;DR
If there is high risk of regression here we can do some exploratory testing once it lands. Any long-term coverage should be handled by automation in my opinion.

I'm not sure I agree that it is warranted. Adding a "dumb" manual spotcheck every release is not an effective use of our time. If the automation is deficient we should look at improving the manual tests which already exist or look at adding a Mozmill automated test. If there is a specific concern you'd like to be addressed we can certainly do some exploratory testing to shake out any regressions once it lands. I'm pretty sensitive about my limited team being used as "automation"; one of the biggest lessons we've learned with rapid release is that we need to be a lot smarter than that.

I anticipate this answer is not going to be satisfactory for you so please do send me an email so we can work it out without adding noise to this bug. Once we have a clear path forward I can come back to this bug to communicate that plan.
Flags: needinfo?(anthony.s.hughes)
I know you said reply in email, but I'll make this small.

I'm all for automated tests! It's just this bug has legal impact more than others so I was thinking something additional might be warranted.

For some reason, I forgot we had Marionette and Mozmill tests. We should probably write one of those tests.
That's fair.

So what I propose we do is once this if fixed, QA will test it across all fixed branches. Once verified fixed we will flag for an automated test. QA will continue to spotcheck this manually in all future releases until the Mozmill/Marionette test is in place.

Does this sound reasonable?
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #55)
> That's fair.
> 
> So what I propose we do is once this if fixed, QA will test it across all
> fixed branches. Once verified fixed we will flag for an automated test. QA
> will continue to spotcheck this manually in all future releases until the
> Mozmill/Marionette test is in place.
> 
> Does this sound reasonable?

Yes.
Thanks Gregory, I will await landing of the fix.
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #57)
> Thanks Gregory, I will await landing of the fix.

By the way, as a point of process please set the verifyme keyword once this lands so it gets added to our prioritized verification queue and set the in-moztrap? flag to me as a reminder to write the Moztrap test.

Thanks
Fixing NITs, carying gps's r+, asking :rnewman for final green light
Attachment #812136 - Flags: review?(rnewman)
Attachment #808838 - Attachment is obsolete: true
Comment on attachment 812136 [details] [diff] [review]
Part 2: Display data reporting notification on first run;

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

::: browser/base/content/browser-data-submission-info-bar.js
@@ +49,5 @@
>  
> +    // We delay this because we don't want to incur the memory hit until
> +    // the functionality is required.
> +    let ns = {};
> +    Cu.import("resource://services-common/utils.js", ns);

I don't get it. You don't use `ns` hereafter, so unless you're trying to achieve a side-effect of that module being evaluated, this line is completely useless -- all of the utils symbols will be installed into `ns`.

Am I missing why you're doing this?

@@ +102,5 @@
> +        try {
> +          this._displayDataPolicyInfoBar();
> +        } catch (ex) {
> +          request.onUserNotifyFailed(ex);
> +          break;

Just return instead of breaking.

::: browser/base/content/test/general/browser_datareporting_notification.js
@@ +226,5 @@
> +                                  .getService(Components.interfaces.nsISupports)
> +                                  .wrappedJSObject;
> +  service.policy._prefs.resetBranch("datareporting.policy.");
> +
> +  closeAllNotifications();

… returns a promise, which you're not blocking on or returning. Sounds like a recipe for problems.

::: browser/base/content/test/general/head.js
@@ +14,5 @@
> +  if (!notificationBox || !notificationBox.currentNotification) {
> +    return Promise.resolve();
> +  }
> +
> +  let deferred = ns.Promise.defer();

Did you mean to load something into the empty object?
> … returns a promise, which you're not blocking on or returning. Sounds like a recipe for problems.

I could not find any reference on how/if the mochitests suite waits for the cleanup function to finish if it returns a promise or how to explicitly request it to finish, but I guess returning a promise is the best chance there is...
Attachment #812209 - Flags: review?(rnewman)
Attachment #812136 - Attachment is obsolete: true
Attachment #812136 - Flags: review?(rnewman)
Attachment #812209 - Attachment description: Part 2: Display data reporting notification on first run (implemented Comment 60); → Part 2: Display data reporting notification on first run v13 (implemented Comment 60);
Comment on attachment 812209 [details] [diff] [review]
Part 2: Display data reporting notification on first run v13 (implemented Comment 60);

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

Re waiting for cleanup: you can:

* implement cleanup inside each test
* implement a 'cleanup' test
* spin the event loop during cleanup
* extend testing/mochitest/browser-test.js to support asynchronous cleanup.

If these tests are *expected* to succeed in such a way that the cleanup isn't usually necessary, then let's forge ahead.

If that's not the case, then let's do the cleanup inside each test and wait for it to be done before continuing, and file a bug to support asynchronous cleanup.

r+ if one of those avenues is taken.
Attachment #812209 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #62)
> * spin the event loop during cleanup

nooooooooooooooooooooooooooooooooooooooooo

> * extend testing/mochitest/browser-test.js to support asynchronous cleanup.

What does asynchronous cleanup mean? Why can't your test avoid calling finish() until "clean up" is complete?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #63)
> (In reply to Richard Newman [:rnewman] from comment #62)
> > * spin the event loop during cleanup
> 
> nooooooooooooooooooooooooooooooooooooooooo

Oh, I concur -- listed for the sake of completeness.

> > * extend testing/mochitest/browser-test.js to support asynchronous cleanup.
> 
> What does asynchronous cleanup mean?

We have a promise which, when resolved, indicates that some cleanup work has been completed. We would like a way to make the test runner wait for that promise to be resolved.

> Why can't your test avoid calling
> finish() until "clean up" is complete?

This test file is using the mochitest registerCleanupFunction mechanism, which doesn't offer something like waitForExplicitFinish -- only tests do. The two realistic avenues I suggested in comment 62 are:

* Don't use mochitest's cleanup mechanism
* Extend mochitest's cleanup mechanism to not force cleanup to be synchronous (e.g., support waitForExplicitFinish in cleanup, as well as tests).
OK, thanks for explaining. Either of those options sound OK, though just not using the cleanup mechanism and rolling your own sounds most expedient.
Blocks: 925587
Assignee: steven.mirea → nobody
Flags: firefox-backlog+
Flagging this as a diamond bug. Richard, would you be willing to help a contributor carry this over the line?
Status: ASSIGNED → NEW
Flags: needinfo?(rnewman)
Whiteboard: [diamond]
Flags: needinfo?(rnewman)
Whiteboard: [diamond] → [diamond][mentor=rnewman][lang=js]
Whiteboard: [diamond][mentor=rnewman][lang=js] → [diamond][mentor=rnewman][lang=js] p=0 [qa?]
(In reply to Richard Newman [:rnewman] from comment #64)
> > > * extend testing/mochitest/browser-test.js to support asynchronous cleanup.
> > 
> > What does asynchronous cleanup mean?
> 
> We have a promise which, when resolved, indicates that some cleanup work has
> been completed. We would like a way to make the test runner wait for that
> promise to be resolved.

That sounds like we could just replicate what bug 988844 did for xpcshell tests. Yoric, does that sound right?
Flags: needinfo?(dteller)
Probably. Note, however, that the implementation of promise-compliant cleanup for xpcshell tests actually spins the event loop behind the scenes.
Flags: needinfo?(dteller)
Assignee: nobody → georg.fritzsche
Added to Iteration 32.3.  Georg, can you provide a point estimate and your recommendation if this bug should be marked as [qa-] or [qa+] for QA verification.
Status: NEW → ASSIGNED
Flags: needinfo?(georg.fritzsche)
Whiteboard: [diamond][mentor=rnewman][lang=js] p=0 [qa?] → [diamond][mentor=rnewman][lang=js] p=0 s=it-32c-31a-30b.3 [qa?]
Assignee: georg.fritzsche → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(georg.fritzsche)
Whiteboard: [diamond][mentor=rnewman][lang=js] p=0 s=it-32c-31a-30b.3 [qa?] → [diamond][mentor=rnewman][lang=js] p=0 [qa?]
Whiteboard: [diamond][mentor=rnewman][lang=js] p=0 [qa?] → [diamond][mentor=rnewman][lang=js] p=13 [qa?]
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Whiteboard: [diamond][mentor=rnewman][lang=js] p=13 [qa?] → p=13 s=33.1 [qa?]
Whiteboard: p=13 s=33.1 [qa?] → p=13 s=33.1 [qa+]
(In reply to Georg Fritzsche [:gfritzsche] from comment #67)
> > We have a promise which, when resolved, indicates that some cleanup work has
> > been completed. We would like a way to make the test runner wait for that
> > promise to be resolved.
> 
> That sounds like we could just replicate what bug 988844 did for xpcshell
> tests. Yoric, does that sound right?

... and while checking into doing that, it turns out we already support async cleanup (returning promises from cleanup functions) via bug 1000512. \o/
rebased
Attachment #794897 - Attachment is obsolete: true
rebased
Attachment #812209 - Attachment is obsolete: true
Attachment #8440679 - Flags: review+
Attachment #8440680 - Flags: review+
Comment on attachment 8440679 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking

Richard, i realize that it's been a while since the last previous patch updates here.
While was good to go review-wise, can you take a quick look if we might have conflicting changes etc. come up in the mean-time?
Attachment #8440679 - Flags: feedback?(rnewman)
Attachment #8440680 - Flags: feedback?(rnewman)
Comment on attachment 8440679 [details] [diff] [review]
Part 1: Remove strong requirement of policy tracking

Holding off for test-failures.
Attachment #8440679 - Flags: feedback?(rnewman)
Attachment #8440680 - Flags: feedback?(rnewman)
Iteration: --- → 33.2
Points: --- → 13
Whiteboard: p=13 s=33.1 [qa+] → [qa+]
Attached patch Folded and fixed up (obsolete) — Splinter Review
Figured all the tests issue out finally.
I folded the patches together because it would have been much harder to figure out the cleanup & test fixups otherwise.

Review requests for both gps and rnewman depending on who has time to take a look.

Note that there is one test change i'm unsure about, pointing it out inline in a sec.
Attachment #8440679 - Attachment is obsolete: true
Attachment #8440680 - Attachment is obsolete: true
Attachment #8447612 - Flags: review?(rnewman)
Attachment #8447612 - Flags: review?(gps)
Comment on attachment 8447612 [details] [diff] [review]
Folded and fixed up

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

::: services/datareporting/tests/xpcshell/test_policy.js
@@ +677,5 @@
>    // Increase the minimum policy version and check if we're notified.
> +
> +  createPolicy(true, currentPolicyVersion, ++minimumPolicyVersion);
> +  do_check_true(policyPrefs.has("dataSubmissionPolicyAcceptedVersion"));
> +  yield triggerPolicyCheckAndEnsureNotified(false);

I'm not entirely sure about the minimum policy handling here.
I'd expect bumping so that the minimumPolicyVersion is still lesser-or-equal to the last notified version not to trigger new notifications - sensible?
Attachment #8447612 - Flags: review?(rnewman)
Iteration: 33.2 → 33.3
QA Whiteboard: [qa+]
Whiteboard: [qa+]
Blocks: 1037120
Comment on attachment 8447612 [details] [diff] [review]
Folded and fixed up

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

Oy. This review took me over a day.

I stopped writing comments midway through. Since you are in Africa and since I have a special relationship with this bug (I really despise this bug and want it to go away), and since there was nothing major in my comments, I just started implementing my nits and small fixes as a fixup patch. I'll fold this in and land this patch for you after I have Try confirmation it's working.

::: browser/base/content/browser-data-submission-info-bar.js
@@ -90,5 @@
>        }.bind(this)
>      );
> -
> -    // Tell the notification request we have displayed the notification.
> -    request.onUserNotifyComplete();

I'm not a fan of moving this. We want onUserNotifyComplete() to be called only if a notification is displayed. I don't like putting extra code between the call sites.

::: browser/components/preferences/tests/browser_healthreport.js
@@ +89,5 @@
> +  let service = Cc["@mozilla.org/datareporting/service;1"]
> +                  .getService(Ci.nsISupports)
> +                  .wrappedJSObject;
> +  service.policy._prefs.resetBranch("datareporting.policy.");
> +  service.policy.dataSubmissionPolicyBypassNotification = true;

This file reminds me that I had a tentative patch to make preference resets automatic during mochitest tests. See bug 995463.

::: services/datareporting/DataReportingService.js
@@ +174,5 @@
>              //
>              // The instance installs its own shutdown observers. So, we just
>              // fire and forget: it will clean itself up.
>              let reporter = this.healthReporter;
> +            this.policy.ensureUserNotified();

I'm hesitant to say this because it's inviting scope bloat and I want to see this land, but I suspect someone from UX is going to have issue with this. IMO, we should defer that discussion to a follow-up (this patch is hard enough as is) and uplift the fix, if necessary.

::: services/datareporting/datareporting-prefs.js
@@ +7,2 @@
>  pref("datareporting.policy.dataSubmissionPolicyNotifiedTime", "0");
> +pref("datareporting.policy.dataSubmissionPolicyAcceptedVersion", "0");

This should be an int. Only the time prefs are strings because they overflow 32-bit integers.

::: services/datareporting/policy.jsm
@@ +746,5 @@
> +    }).bind(this), ((error) => {
> +      this._log.warn("Data policy notification presentation failed: " +
> +                     CommonUtils.exceptionStr(error));
> +      this._userNotifyPromise = null;
> +    }).bind(this));

Can we make onSuccess a fat arrow function?

@@ +770,5 @@
> +    this.dataSubmissionPolicyAcceptedVersion = version;
> +  },
> +
> +  _migratePrefs: function () {
> +    // Current prefs are mostly the same than the old ones, except for some deprecated ones.

Nit: Bad English.

::: services/datareporting/tests/xpcshell/test_policy.js
@@ +44,5 @@
> + * data upload and afterwards does a call to policy.checkStateAndTrigger()
> + * @param  {Policy} policy
> + * @return {Promise}
> + */
> +function ensureUserNotifiedAndTrigger (policy) {

Nit: no space between function name and args.

@@ +45,5 @@
> + * @param  {Policy} policy
> + * @return {Promise}
> + */
> +function ensureUserNotifiedAndTrigger (policy) {
> +  return Task.spawn(function ensureUserNotifiedAndTrigger () {

Nit: function* for generators now. The non-* syntax is apparently non-standard and going to go away some day.

@@ +82,5 @@
>  
> +  let now = new Date();
> +  policy.dataSubmissionPolicyNotifiedDate = now;
> +  do_check_eq(policyPrefs.get("dataSubmissionPolicyNotifiedTime"), now.getTime());
> +  do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(), now.getTime());

This test function is testing the policy constructor, not policy mutation. Please move this block elsewhere.

@@ +88,4 @@
>    let tomorrow = Date.now() + 24 * 60 * 60 * 1000;
>    do_check_true(tomorrow - policy.nextDataSubmissionDate.getTime() < 1000);
>  
> +  do_check_false(policy.userNotifiedOfCurrentPolicy);

This line is fine.

But we need an explicit check for the default prefs value for accepted version.

@@ -82,5 @@
>    do_check_eq(policy.firstRunDate.getTime(), nowT);
>  
> -  policy.dataSubmissionPolicyNotifiedDate= now;
> -  do_check_eq(policyPrefs.get("dataSubmissionPolicyNotifiedTime"), nowT);
> -  do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(), nowT);

Not sure why this block was moved. Meh.

@@ +212,5 @@
> +  do_check_eq(listener.requestDataUploadCount, 0);
> +
> +  yield ensureUserNotifiedAndTrigger(policy);
> +
> +  do_check_neq(policy.dataSubmissionPolicyNotifiedDate, null);

Won't this always be true since the default value is Date(0)?

@@ +217,3 @@
>    do_check_true(policy.dataSubmissionPolicyNotifiedDate.getTime() > 0);
>    do_check_eq(policy.dataSubmissionPolicyNotifiedDate.getTime(),
> +              policy.dataSubmissionPolicyNotifiedDate.getTime());

Unless Splinter is lying to me, you are comparing the same value, no?

@@ +677,5 @@
>    // Increase the minimum policy version and check if we're notified.
> +
> +  createPolicy(true, currentPolicyVersion, ++minimumPolicyVersion);
> +  do_check_true(policyPrefs.has("dataSubmissionPolicyAcceptedVersion"));
> +  yield triggerPolicyCheckAndEnsureNotified(false);

Yeah, this code is very suspicious. As I mentioned in the bug that introduced it, I wouldn't have granted r+. I'm probably going to file a follow-up after this lands to clean it up.
Attachment #8447612 - Flags: review?(gps) → review+
Gah. These existed in Try but I thought they were normal oranges.

I'll look into this.
Flags: needinfo?(georg.fritzsche)
gfritzsche: I made a number of small changes to the patch before landing. When you hack on this, your best bet is to `hg export 8673477f5fc2 | hg import -` and start working from that.
Iteration: 33.3 → 34.1
QA Contact: kamiljoz
Iteration: 34.1 → 34.2
Syntax fix-up after confusing all-over breakage: https://tbpl.mozilla.org/?tree=Try&rev=975b7fddc23c
Finally narrowed the pref issues down (DumpJSStack() to the rescue), next try run:
https://tbpl.mozilla.org/?tree=Try&rev=1ef54eda0598
gps, trivial change, but i'm not entirely about this.
Pending try results i will just land with this unless you have a better idea though.
Attachment #8471856 - Flags: review?(gps)
Comment on attachment 8471856 [details] [diff] [review]
Restore bypass pref when resetting pref branch

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

Ugh.

So, this workaround is sane. But I should have never granted r+ on the earlier patch. Resetting the whole branch is the wrong thing to do. Because your patch touches that line, it gets r-.

With branch resetting, policy version updates would wipe out:

* firstRunDate
* dataSubmissionPolicyNotifiedDate
* dataSubmissionPolicyBypassNotification
* dataSubmissionEnabled
* currentPolicyVersion
* minimumPolicyVersion
* dataSubmissionPolicyAcceptedVersion

That's like using a chainsaw for surgery.

So we wipe out acceptance of the previous policy version (dataSubmissionPolicyAcceptedVersion) in the process of accepting a new one?! That's not right at all. We also throw away dataSubmissionEnabled. That could wipe away user choice.

I'm not sure why we need to reset prefs at all: the notification should be derived from the difference between the desired policy version and the last accepted policy version. Preference resetting should not be involved at all.

I should never have granted r+.

The proper solution involves updating ensureUserNotified() to check policy version alignment.
Attachment #8471856 - Flags: review?(gps) → review-
Comment on attachment 8447612 [details] [diff] [review]
Folded and fixed up

Changing to r- (see my last comment).
Attachment #8447612 - Flags: review+ → review-
So, this has been around for a few patch revisions in this bug, so i never looked at it closer.
Looking at this again at a sane time today, i don't see what we even have the pref branch reset for.

Next try run:
https://tbpl.mozilla.org/?tree=Try&rev=780c2a3aed58
This just drops the pref branch reset.
Attachment #8447612 - Attachment is obsolete: true
Attachment #8471856 - Attachment is obsolete: true
Attachment #8472332 - Flags: review?(gps)
Comment on attachment 8472332 [details] [diff] [review]
Dropped pref branch reset

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

The interdiff looks sane. I've applied this locally and verified basic behavior. Let's hope this sticks this time...
Attachment #8472332 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/2d8fba7b3e14
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Iteration: 34.2 → 34.3
QA Whiteboard: [qa+]
Flags: qe-verify+
Blocks: 1055854
Depends on: 1060038
Depends on: 1060460
Update:

Apparently there might be an issue with the telemetry notification appearing several times despite being dismissed the first time around. I'm investigating and trying to reproduce the issue and will add updates via bug # 1055854.
Can someone please summarize EXACTLY what changed here with regards to prefs?

The diff here is unimaginable.

What prefs can be set to completely prevent the display of the telemetry/health report ask (again)?
If you want to disable the notification completely (essentially setting it to "auto accept", set datareporting.policy.dataSubmissionPolicyBypassNotification to true.
(In reply to Gregory Szorc [:gps] from comment #98)
> If you want to disable the notification completely (essentially setting it
> to "auto accept", set
> datareporting.policy.dataSubmissionPolicyBypassNotification to true.

Well, most enterprises are turning off the other prefs for telemetry and health reporting. So what they want is "everything off and please don't ask ever"
datareporting.policy.dataSubmissionEnabled is the master kill switch for submitting data to Mozilla.
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.