Re-enable probe for certificate error pages
Categories
(Firefox :: Security, defect)
Tracking
()
People
(Reporter: RT, Assigned: johannh)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
3.34 KB,
text/plain
|
chutten
:
data-review+
|
Details |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
14.63 KB,
patch
|
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Re-enable probe created on bug 1484255 and then disabled on bug 1578239.
Reporter | ||
Comment 1•5 years ago
|
||
Hi Chris, can you please help review this request?
Assignee | ||
Comment 2•5 years ago
|
||
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•5 years ago
|
||
Reporter | ||
Comment 4•5 years ago
|
||
Thanks for the details, here is now the full form.
Updated•5 years ago
|
Comment 5•5 years ago
|
||
Reporter | ||
Comment 6•5 years ago
|
||
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 | ||
Comment 7•5 years ago
|
||
Updated•5 years ago
|
Comment 10•5 years ago
|
||
bugherder |
Assignee | ||
Comment 11•5 years ago
|
||
[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!
Assignee | ||
Comment 12•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 13•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 15•5 years ago
•
|
||
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•5 years ago
|
||
Comment 17•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Comment 18•5 years ago
|
||
This is also verified as fixed on 76.0 (20200427162639) under macOS 10.15, Ubuntu 18.04 x64 and Windows 10 x64.
Description
•