1367076 - Replace data choices infobar with privacy policy in a background tab on first run

Status: VERIFIED FIXED

(Firefox :: General, enhancement)

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

This breaks reftests, crashtests and marionette:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94e8c6859aac99aa65e84c063b09b546c17e2ac5

Not sure where the prefs for these test suites are set. According to <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#Need_to_set_preferences_for_test-suites>, modifying testing/mozbase/mozprofile/mozprofile/profile.py should do the trick, but this didn't seem to work here.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

This fixes reftests and crashtests, marionette remains broken:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a37a1920787d0cb48f0bb97e330f3b8ed7099cd6

Comment 3

[Dão Gottwald [:dao]]

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

Henrik, where do I need to set datareporting.policy.dataSubmissionPolicyBypassNotification=true for marionette to pick this up in automation? testing/mozbase/mozprofile/mozprofile/profile.py as documented on MDN doesn't seem to work.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There are currently two places because we are in a transition:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/server.js (Recommended prefs)
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py

For the latter it depends on if his pref is only for firefox or gecko in general. So pick the correct class.

Comment 5

[Dão Gottwald [:dao]]

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

(In reply to Henrik Skupin (:whimboo) from comment #4)
> There are currently two places because we are in a transition:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/server.js
> (Recommended prefs)
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/
> marionette_driver/geckoinstance.py
> 
> For the latter it depends on if his pref is only for firefox or gecko in
> general. So pick the correct class.

Thanks. The pref lives in toolkit so "gecko" probably makes more sense.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> There are currently two places because we are in a transition:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/server.js
> (Recommended prefs)
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/
> marionette_driver/geckoinstance.py
> 
> For the latter it depends on if his pref is only for firefox or gecko in
> general. So pick the correct class.

Wait, both of these files were already setting datareporting.policy.dataSubmissionPolicyBypassNotification=true. That pref appears to work as expected for reftests and mochitests but not for marionette (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=a37a1920787d0cb48f0bb97e330f3b8ed7099cd6). Any other clue what could be going wrong here?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The test which is failing here, resets the profile and restarts Firefox. So all set preferences are getting cleared. Gijs, can most likely help here given that he initially wrote this test, and could say what to do with it. 

Keep in mind that this is not a usual Marionette unit test, but a browser test which is folded into the Mn test suite, due to a missing subtest feature yet. The test is located here:

https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/marionette/test_refresh_firefox.py

Comment 8

[:Gijs (he/him)]

There isn't an easy way to fix this that I can see.

Perhaps the most straightforward option I can see would be to change the "marionette." hardcoded prefix for MOZ_MARIONETTE_PREF_STATE_ACROSS_RESTARTS, and to then include the relevant pref in one of the ones marionette sets ASAP in the process' lifetime, here: https://dxr.mozilla.org/mozilla-central/rev/f81bcc23d37d7bec48f08b19a9327e93c54d37b5/browser/components/migration/tests/marionette/test_refresh_firefox.py#378-388
However, I don't know if that will come up to any limits on the length of environment variables on the different platforms on which we run the test.

A more correct fix would be avoiding reshowing the notification bar or the page if the user had already seen it and/or it was disabled on the old profile. That would have to be fixed in FirefoxProfileMigrator.js, but we still don't have a general way of migrating prefs and so fixing that is non-trivial, I think.

Another hacky thing you could do is avoiding the notification bar/page codepath when session restore has restored something (which is the case here, and pretty much guarantees the user has already seen it anyway, or will have it restored in a new tab), or changing the test to simply close/remove the tab you're adding.

Comment 9

[Nicole Yee (:nicoleyee)]

Hey Dao and Gijs,

Thanks for helping the onboarding team with these bugs. Dao, what do you think is the best way to proceed with this fix?

We planned on landing this in 55, and ideally, we would start the QA testing for this bug starting tomorrow.

Comment 10

[Dão Gottwald [:dao]]

Attachment: Bug 1367076 - Set datareporting.policy.firstRunURL by default to replace data choices infobar with privacy policy in a background tab on first run.

Review commit: https://reviewboard.mozilla.org/r/144500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144500/

Comment 11

[:Gijs (he/him)]

Comment on attachment 8872958 [details]
Bug 1367076 - Set datareporting.policy.firstRunURL by default to replace data choices infobar with privacy policy in a background tab on first run.

https://reviewboard.mozilla.org/r/144500/#review148408

Comment 12

[Nicole Yee (:nicoleyee)]

Hey Dao,

Can you commit these changes to m-c? Code Freeze is tomorrow, June 5th. If you don't have commit access, RyanVM can you please help us do this?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffd92f10ba94
Set datareporting.policy.firstRunURL by default to replace data choices infobar with privacy policy in a background tab on first run. r=Gijs

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ffd92f10ba94

Comment 15

[Dão Gottwald [:dao]]

(In reply to Nicole Yee (:nicoleyee) from comment #12)
> Hey Dao,
> 
> Can you commit these changes to m-c? Code Freeze is tomorrow, June 5th. If
> you don't have commit access, RyanVM can you please help us do this?

Monday was a bank holiday in Germany, and I was still waiting for gfritzsche's review. I'm leaving the request in place in case he has something to add.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8872958 [details]
Bug 1367076 - Set datareporting.policy.firstRunURL by default to replace data choices infobar with privacy policy in a background tab on first run.

https://reviewboard.mozilla.org/r/144500/#review150074

This looks good.

While the Telemetry change seems trivial, this is critical to our Telemetry reporting.
Are you planning to have a QA pass here or do we need to?

Comment 17

[Dão Gottwald [:dao]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> While the Telemetry change seems trivial, this is critical to our Telemetry
> reporting.
> Are you planning to have a QA pass here or do we need to?

Forwarding this question to Peter.

Comment 18

[Peter Dolanjski [:pdol]]

Nicole, can you follow up with Soft vision to ensure QA coverage?

Comment 19

[Justin [:JW_SoftvisionQA]]

I can verify that https://www.mozilla.org/privacy/firefox/ loads as a background tab instead of the infobar "Firefox automatically sends some data to Mozilla so that we can improve your experience. [Choose What I Share]".

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8872958 [details]
Bug 1367076 - Set datareporting.policy.firstRunURL by default to replace data choices infobar with privacy policy in a background tab on first run.

https://reviewboard.mozilla.org/r/144500/#review179472

::: testing/mozbase/mozprofile/mozprofile/profile.py:357
(Diff revision 1)
>          # Don't warn when exiting the browser
>          'browser.warnOnQuit': False,
>          # Don't send Firefox health reports to the production server
>          'datareporting.healthreport.documentServerURI': 'http://%(server)s/healthreport/',
> +        # Skip data reporting policy notifications
> +        'datareporting.policy.dataSubmissionPolicyBypassNotification': True,

Dao, FYI this is not the place to add preferences anymore. I don't know how many test harnesses still rely on those, but for at least Marionette we moved them all out into geckoinstance.py, and marionette.js.

We should finally strip out all those prefs from mozprofile.