advanced debugging information not showing up properly on about:certerror pages

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: keeler, Assigned: johannh)

Tracking

({regression})

50 Branch
mozilla51
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50+ fixed, firefox51+ fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments)

STR: 1. visit a page with a certificate error (e.g. https://wrong.host.badssl.com/ )
2. Click "Advanced"
3. Click the error code ("SSL_ERROR_BAD_CERT_DOMAIN" in this case)

Expected results: advanced debugging information such as what certificate chain the server sent should show up.

Actual results: the information appears to show up briefly, but then something happens and it disappears again.

Comment 1

2 years ago
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a878df4314faa5a620aca4b04fbae422f68392d3&tochange=f4ed38f18dbc97c69c3147c0685f71b6edc4aa96

Regressed by: f4ed38f18dbc	Gijs Kruitbosch — Bug 1280584 - implement cloneWithNewRef and thereby make hash/ref links use a simple unified codepath in the IO service, r=valentin
Blocks: 1280584
status-firefox49: --- → unaffected
status-firefox50: --- → affected
Flags: needinfo?(valentin.gosu)
Keywords: regressionwindow-wanted

Comment 2

2 years ago
[Tracking Requested - why for this release]:
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
Component: Security → Networking
Product: Firefox → Core
Version: Trunk → 50 Branch
status-firefox48: --- → unaffected
Created attachment 8777498 [details]
Screen shot from Mac Nightly

Adding a screen shot from mac nightly, where the UI can look messed up as well.
Tracking 50/51+ for this regression, both from a functionality and visual perspective.
tracking-firefox50: ? → +
tracking-firefox51: ? → +
Gijs would know if there's a way to fix the about:certerror code, or if we need to backout Bug 1280584.
Assignee: nobody → gijskruitbosch+bugs
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active]

Comment 6

2 years ago
about:certerror should be fixed. It's possible about:neterror has similar issues (I haven't checked).

It's using a "relative" hash link (#technicalInformation) with a click handler, and the click handler doesn't call event.preventDefault() (because it never needed to, because navigation failed before bug 1280584). Then there's all the magic of the docshell's window.location and the browser's URL bar showing e.g. wrong.host.badssl.com, but the document's URI is actually about:certerror?<query params>, so then we end up navigating to about:certerror?<query params>#technicalInformation, which refreshes the page (which hash changes of course normally wouldn't do), which is why you can't see the information ("something happens" in comment #0).

I just got back from 2 weeks of PTO, so a fix may take a few days to materialize while I plod through backlog. The trivial fix is probably to make the click handler call preventDefault(), the better fix is to not set .href in this case or to set it to the correct URI, but the former might need styling changes and the latter might need to pass the magic URI from the browser content script into the page or something to that effect.
(Assignee)

Comment 7

2 years ago
I can look into this, let's give Gijs some time to fix up his backlog :)
Assignee: gijskruitbosch+bugs → jhofmann
Status: NEW → ASSIGNED
(Assignee)

Comment 8

2 years ago
So in general I wonder why we need that hash link. If we would fix up the URL to be correct then we'd end up with something like https://wrong.host.badssl.com/#technicalInformation, which only makes sense if we think about the page as permanently misconfigured. I doubt anyone will want to refer to https://facebook.com/#technicalInformation should Facebook ever decide to misconfigure their TLS.

I also found that setting the hash link prevents the nice animation that is actually scrolling down the page.

For that reason I'll simply remove the hash reference altogether.
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
This patch removes the hash.

Another small thing I changed because it was in the same block: This also gets rid of the confusing closing behavior of the error code link, where you would scroll up to reach the link but then on clicking the link the first time it would just do nothing because it closed the technical info box for no good reason. Now it consistently leads you to the technical info. If you compare it with stable you'll find it's much nicer.

Comment 11

2 years ago
mozreview-review
Comment on attachment 8780068 [details]
Bug 1290927 - Remove hash link from technical info on certerror pages.

https://reviewboard.mozilla.org/r/70884/#review68308

::: browser/base/content/aboutNetError.xhtml:422
(Diff revision 1)
>          if (gIsCertError) {
>            // Initialize the error code link embedded in the error message to
>            // display debug information about the cert error.
>            var errorCode = document.getElementById("errorCode");
>            if (errorCode) {
> -            errorCode.href = "#technicalInformation";
> +            errorCode.href = "";

I'd rather we didn't set .href at all and inserted more specific CSS to make this whitespace: nowrap, assuming the code still works in that case (note in particular that the link should still be keyboard-focusable, which I'm not sure off-hand it will be). :-)
Attachment #8780068 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
I'm not sure the old link was keyboard focusable, but now it should behave exactly like before.
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
So after looking into this a bit longer I think using javascript:void(0) is the best way to go. I'm not a big fan of it either but it solves all of our problems. It doesn't create an ugly hash link, it will enable the correct style and it no-ops on click.

Comment 16

2 years ago
mozreview-review
Comment on attachment 8780068 [details]
Bug 1290927 - Remove hash link from technical info on certerror pages.

https://reviewboard.mozilla.org/r/70884/#review68506

wfm, let's land this and we can worry about perfection later. This will also need aurora uplift.
Attachment #8780068 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 17

2 years ago
Bram, before we land and uplift this, I just wanted to check that the small change we did to the CertError page works for you. 

We removed the ability to close the technical information panel using the link. If you try it on current stable it's just really confusing, since you have to scroll up anyway to be able to toggle the link. A link should also not really be toggle-able.
Flags: needinfo?(bram)

Comment 18

2 years ago
Hi Johann, how does the user close the technical information panel? As long as there’s a clear way to do this, I’m good with the push.
Flags: needinfo?(bram)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
Ok we need to uplift this so I'll remove the technical information link fix.

I'll file a bug for it since this behavior is definitely not great. We can discuss there.
(Assignee)

Comment 22

2 years ago
Comment on attachment 8780068 [details]
Bug 1290927 - Remove hash link from technical info on certerror pages.

Approval Request Comment
[Feature/regressing bug #]: Bug 1280584
[User impact if declined]: Users can not check advanced technical information on certificate error pages
[Describe test coverage new/current, TreeHerder]: The certerror pages are thoroughly tested, this particular case was hard to catch because it requires real user input to trigger.
[Risks and why]: Potential risks from this patch are limited to the frontend of the certerror page. It's a very simple patch.
[String/UUID change made/needed]: None
Attachment #8780068 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/1262d39de13e4e782fe855e3ff7f0a179101a162
Bug 1290927 - Remove hash link from technical info on certerror pages. r=Gijs
(Assignee)

Updated

2 years ago
Blocks: 1295571

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1262d39de13e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8780068 [details]
Bug 1290927 - Remove hash link from technical info on certerror pages.

Recent regression in Fx50, let's uplift to Aurora.
Attachment #8780068 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf25a87f8966
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.