1477042 - Include private browsing in URL counts

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Mike Kaply [:mkaply]]

In bug 1271313, we added instrumentation for the counting of URIs/domains, but excluded private browsing.

Because of how we are using this data now (to determine active users), we should be counting private browsing as well.

Comment 1

[Mike Kaply [:mkaply]]

Francois, Marshall asked me to involve you as the data steward for this change.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Bug 1477042 - Include private browsing in URI counts.

Comment 3

[Phabricator Automation]

Comment on attachment 8993471 [details]
Bug 1477042 - Include private browsing in URI counts.

Florian Quèze [:florian] has approved the revision.

https://phabricator.services.mozilla.com/D2250

Comment 4

[François Marier [:francois]]

I see that Rebecca gave the original data review (https://bugzilla.mozilla.org/show_bug.cgi?id=1271313#c34) but I don't see a data review request form attached on bug 1271313. Would you mind filling out https://github.com/mozilla/data-review/blob/master/request.md and attaching it as a .txt file to this bug? It would be good to have a clear record of what we are collecting for this.

From what I can see in your patch, we would not be collecting Private Browsing data separately or labelling it in any way. It would go in the same bucket as all of the normal mode browsing. If that's accurate, please state so in the data review request form.

One thing that comes to mind though is people who are set to be in private browsing mode all of the time. Should we filter them out? My guess is that they don't expect that we're sending anything about their web browsing if they've set their browser to be permanently in Private Browsing.

Comment 5

[Mike Kaply [:mkaply]]

Attachment: Add a preferences to allow for a shield study.

This is a new patch that adds a preference so a shield study can be run on this.

Comment 6

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

Tracking for 62 since we may uplift once this lands.

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Bug 1477042 - Add a pref for counting URIs in private browsing mode.

Comment 8

[Phabricator Automation]

Comment on attachment 8995640 [details]
Bug 1477042 - Add a pref for counting URIs in private browsing mode.

Mike de Boer [:mikedeboer] has approved the revision.

https://phabricator.services.mozilla.com/D2479

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c03547ff0797
Add a pref for counting URIs in private browsing mode. r=mikedeboer

Comment 10

[Cosmin Sabou [:CosminS]]

Backed out for browser chrome failures on browser_address_edit.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=Linux%20x64%20asan%20Mochitests%20with%20e10s%20test-linux64-asan%2Fopt-mochitest-browser-chrome-e10s-7%20M-e10s(bc7)&fromchange=c03547ff07973aa3f3c6b46903d914a585f39f1b&tochange=6b0d60723f1ca9fd3c0cc811738b9e45a809c141&selectedJob=190608027

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190608027&repo=autoland&lineNumber=2102

Backout link: https://hg.mozilla.org/integration/autoland/rev/6b0d60723f1ca9fd3c0cc811738b9e45a809c141

[task 2018-07-27T20:47:58.661Z] 20:47:58     INFO - TEST-PASS | browser/components/payments/test/browser/browser_address_edit.js | Check country in response - 
[task 2018-07-27T20:47:58.662Z] 20:47:58     INFO - Console message: [JavaScript Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "resource://testing-common/content-task.js" line: 62}]
[task 2018-07-27T20:47:58.663Z] 20:47:58     INFO - Console message: [JavaScript Error: "NS_NOINTERFACE: " {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 56}]
[task 2018-07-27T20:47:58.664Z] 20:47:58     INFO - Buffered messages finished
[task 2018-07-27T20:47:58.665Z] 20:47:58     INFO - TEST-UNEXPECTED-FAIL | browser/components/payments/test/browser/browser_address_edit.js | [JavaScript Error: "NS_NOINTERFACE: " {file: "resource://gre/modules/PrivateBrowsingUtils.jsm" line: 56}] - 
[task 2018-07-27T20:47:58.666Z] 20:47:58     INFO - Stack trace:
[task 2018-07-27T20:47:58.667Z] 20:47:58     INFO - chrome://mochitests/content/browser/browser/components/payments/test/browser/head.js:onConsoleMessage:329
[task 2018-07-27T20:47:58.668Z] 20:47:58     INFO - chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:observe/<:377

Comment 11

[François Marier [:francois]]

(In reply to Cosmin Sabou [:CosminS] from comment #10)
> Backed out for browser chrome failures on browser_address_edit.

Also, the data review hasn't been done yet. See my questions in comment 4.

Comment 12

[Mike Kaply [:mkaply]]

> Also, the data review hasn't been done yet. See my questions in comment 4.

This is a setup for a shield study, not the actual change.

As far as the other error goes, dumb cut and paste error that (unfortunately) passed local testing. New patch coming.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c4e3c2df1f2
Add a pref for counting URIs in private browsing mode. r=mikedeboer

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4c4e3c2df1f2

Comment 15

[Andreea Cupsa [:acupsa], Experiments QA]

We have finished testing the PBM URL counts change from bug 1477042. 

QA’s recommendation: YELLOW - SHIP IT CONDITIONALLY

Reasoning:
There were no issues found during testing, but from QA perspective we are unsure what our desired behavior regarding the users that are forcing PBM through about:config is (question also asked in comment 4).
Users that are aware of this pref and actively use it, would probably expect no data collection at all.

Testing Summary:
We verified that the counting of URIs/domains works while in Private Browsing Mode in the following ways:
- Browsing normally.
- Navigating to a website after opening Private Browsing Window from the Firefox menu or using the keyboard shortcut.
- Opening a website in a New Tab and New Private Window from the context menu.
- Opening a website in a New Tab and New Private Window using keyboard navigation, CTRL/CMD, SHIFT + mouse actions or mouse middle click.
- Browsing while also being in Safe Mode.
- While being in Forced/Permanent Private Mode (set from Settings > Privacy & Security > History > Never remember history / Use custom settings for history  > Always use private browsing mode. 
-  By forcing PBM trough “browser.privatebrowsing.autostart” pref change to “true” value in “about:config” page.
- While ad blockers are installed.
- After a browser refresh, restart or update.


Tested Firefox versions:
- Firefox Release 63.0a1

Tested Platforms:
- Windows 10 x64;
- Mac 10.13.3;
- Arch Linux x64;

Comment 16

[Andreea Cupsa [:acupsa], Experiments QA]

> Tested Firefox versions:
> - Firefox Release 63.0a1

I meant to write Firefox Nightly 63.0a1

Comment 17

[Mike Kaply [:mkaply]]

> There were no issues found during testing, but from QA perspective we are unsure what our desired behavior regarding the users that are forcing PBM through about:config is (question also asked in comment 4).

I believe it should still collect.

Private browsing is about not collecting browser history or tracking information (and we are very specific about that on the private browsing page).

If we are only tacking occasional private browsing users, I would assert that this measurement will have no effect because the users would still be considered active because of their other activity.

This additional count will primarily only affect users that are always private browsing.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mike, can you please nominate the patch for uplift to Beta62? Thanks! I'll try to get this landed in b13 that we plan to gtb in an hour.

Comment 19

[Mike Kaply [:mkaply]]

Comment on attachment 8995640 [details]
Bug 1477042 - Add a pref for counting URIs in private browsing mode.

Approval Request Comment
[Feature/Bug causing the regression]: Enable shield study
[User impact if declined]: None
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds a pref. Existing codepath not changed unless pref is set.
[String changes made/needed]:

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8995640 [details]
Bug 1477042 - Add a pref for counting URIs in private browsing mode.

Fix was verified in Nightly63, this change is needed to run a study on release, Beta62+

Comment 21

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

https://hg.mozilla.org/releases/mozilla-beta/rev/dd92dec96711

Comment 22

[Andreea Cupsa [:acupsa], Experiments QA]

We retested the PBM URL counts change with the scenarios from comment 15 on Firefox Beta 62.0b13, on the following platforms: Windows 10 x64; Mac 10.13.3; Arch Linux x64. 

No issues were found during testing, therefore the QA’s recommendation is: GREEN - SHIP IT

Comment 23

[Mike Kaply [:mkaply]]

Comment on attachment 8995640 [details]
Bug 1477042 - Add a pref for counting URIs in private browsing mode.

Approval Request Comment
[Feature/Bug causing the regression]: Add pref for shield study
[User impact if declined]: None
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds a pref, doesn't change existing behavior.
[String changes made/needed]:

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8995640 [details]
Bug 1477042 - Add a pref for counting URIs in private browsing mode.

Needed for an upcoming study on release. Approved for 61.0.2.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d22d9ccbec14

Comment 26

[Marius Coman [:mcoman], Ecosystem QA]

We retested the PBM URL counts change with the scenarios from comment 15 on the latest Firefox RC (61.0.2), on the following platforms: Windows 10 x64; Mac 10.13.3; Ubuntu 16.04 x64. 

No issues were found during testing, therefore the QA’s recommendation is: GREEN - SHIP IT