Closed Bug 1629826 Opened 5 years ago Closed 5 years ago

Re-enable probe for certificate error pages

Categories

(Firefox :: Security, defect)

Product:
Firefox
Component:
Security
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 77
Tracking Flags:
Tracking Status
firefox75 --- wontfix
firefox76 + verified
firefox77 + verified

People

(Reporter: RT, Assigned: johannh)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Full form
(deleted), text/plain
chutten
: data-review+
Details
Bug 1629826 - Re-enable event telemetry probes for certificate error pages. r=nhnt11
(deleted), text/x-phabricator-request
Details
Patch for beta
(deleted), patch
RyanVM
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file renewal_request.md (obsolete) (deleted) —

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

Hi Chris, can you please help review this request?

Flags: needinfo?(chutten)

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!

Flags: needinfo?(prathikshaprasadsuman)
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.
Flags: needinfo?(chutten)
Attachment #9140390 - Flags: data-review-
Blocks: 1630592
Attached file Full form (deleted) —

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

Attachment #9140390 - Attachment is obsolete: true
Attachment #9140991 - Flags: data-review?(chutten)
Attachment #9140991 - Attachment mime type: application/octet-stream → text/plain
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+
Attachment #9140991 - Flags: data-review?(chutten) → data-review+

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

Assignee: nobody → jhofmann
Status: NEW → ASSIGNED

Let's give it a shot!

Flags: needinfo?(prathikshaprasadsuman)
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f757556c4e2c Re-enable event telemetry probes for certificate error pages. r=nhnt11

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox77: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 77

[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!

status-firefox75: --- → wontfix
status-firefox76: --- → affected
tracking-firefox76: --- → ?
Flags: needinfo?(ryanvm)

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
Attachment #9143322 - Flags: approval-mozilla-release?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

status-firefox77: fixed → verified

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.

Flags: needinfo?(ryanvm) → needinfo?(jhofmann)
Attachment #9143322 - Flags: approval-mozilla-release?
Attached patch Patch for beta (deleted) — — Splinter Review

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
Flags: needinfo?(jhofmann) → needinfo?(ryanvm)
Attachment #9143652 - Flags: approval-mozilla-release?
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.
Flags: needinfo?(ryanvm)
Attachment #9143652 - Flags: approval-mozilla-release? → approval-mozilla-beta+

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

Status: RESOLVED → VERIFIED
status-firefox76: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: