1629826 - Re-enable probe for certificate error pages

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[Romain Testard [:RT]]

Attachment: renewal_request.md

Re-enable probe created on bug 1484255 and then disabled on bug 1578239.

Comment 1

[Romain Testard [:RT]]

Hi Chris, can you please help review this request?

Comment 2

[Johann Hofmann [:johannh]]

Prathiksha, would you have time to pick this up? It's just restoring the code from https://phabricator.services.mozilla.com/D46115 but we need to ensure it's compatible with the latest refactorings.

Thanks!

Comment 3

[Chris H-C :chutten]

Comment on attachment 9140390 [details]
renewal_request.md

For future requests please set the `data-review` flag to `?` and specify the steward you'd like to assign this review to (from the list on [the wiki](https://wiki.mozilla.org/Firefox/Data_Collection)). That way the Data Review will enter our workflow in an expected manner.

Unfortunately this is `data-revew -`. For making an expiring probe permanent, please fill out the full form. This short form can only be used to renew probes for a further six months. (It has no provision for asking who will be responsible for the permanent collection over time).

Also, it is a best practice to ensure permanent collections are covered by regression tests to ensure later code changes do not interfere with the collection in a way that might go unnoticed. TelemetryTestUtils.jsm is a useful resource in writing such tests.

Comment 4

[Romain Testard [:RT]]

Attachment: Full form

Thanks for the details, here is now the full form.

Comment 5

[Chris H-C :chutten]

Comment on attachment 9140991 [details]
Full form

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file [Events.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Events.yaml) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Romain Testard is responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+

Comment 6

[Romain Testard [:RT]]

Hello, any chance this could be part of 76 so we can get early view of intermediate cert impact on cert error page displays?

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1629826 - Re-enable event telemetry probes for certificate error pages. r=nhnt11

Comment 8

[Johann Hofmann [:johannh]]

Let's give it a shot!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f757556c4e2c
Re-enable event telemetry probes for certificate error pages. r=nhnt11

Comment 10

[Narcis Beleuzu [:NarcisB]]

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

Comment 11

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]:
Ryan, this is the telemetry probe I mentioned on Friday. We would like to get it onto the 76 RC if that’s possible. I’ll do the uplift request in a moment.

Thank you!

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 9143322 [details]
Bug 1629826 - Re-enable event telemetry probes for certificate error pages. r=nhnt11

Beta/Release Uplift Approval Request

• User impact if declined: This probe should hopefully have no user impact whatsoever but rather help us a lot in determining whether we were able to fix a number of certificate errors (which we are assuming are a driver for user churn).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Visit a cert error page (see https://badssl.com) and interact with it. This should spawn a number of telemetry events in about:telemetry (for both loading and clicking on certain elements on the page).

Also, ideally none of the above mentioned pages should break in functionality.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is restoring an old patch that we had in release for a while. The code is well understood and not very complicated. It should not affect any functionality, just add telemetry. We have tests. Any breakage should be limited to certificate error pages (which is arguably not a great place to break).
• String changes made/needed: None

Comment 13

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 77.0a1 (2020.04.14) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 77.0a1 latest nightly on Win 8.1 x64, macOS 10.15 and Ubuntu 18.04 x64, visiting cert error page (https://badssl.com/) clicking on expired certificate or wrong.host certificate generate in about:telemetry a number of telemetry events (see attachement: ). I ran some smoke tests to see that it doesn't affect any functionality.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9143322 [details]
Bug 1629826 - Re-enable event telemetry probes for certificate error pages. r=nhnt11

This doesn't graft cleanly. Please attach a rebased patch and re-request approval.

Comment 15

[Johann Hofmann [:johannh]]

Attachment: Patch for beta

Beta/Release Uplift Approval Request

• User impact if declined: This probe should hopefully have no user impact whatsoever but rather help us a lot in determining whether we were able to fix a number of certificate errors (which we are assuming are a driver for user churn).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Visit a cert error page (see https://badssl.com) and interact with it. This should spawn a number of telemetry events in about:telemetry (for both loading and clicking on certain elements on the page).

Also, ideally none of the above mentioned pages should break in functionality.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is restoring an old patch that we had in release for a while. The code is well understood and not very complicated. It should not affect any functionality, just add telemetry. We have tests. Any breakage should be limited to certificate error pages (which is arguably not a great place to break).
• String changes made/needed: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9143652 [details] [diff] [review]
Patch for beta

Re-enables Telemetry to help us better understand what cert errors users are hitting. Approved for 76.0rc1.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ciprian Georgiu, Desktop QA]

This is also verified as fixed on 76.0 (20200427162639) under macOS 10.15, Ubuntu 18.04 x64 and Windows 10 x64.