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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

[Alice0775 White]

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

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

[Marcia Knous [:marcia]]

Attachment: Screen shot from Mac Nightly

Adding a screen shot from mac nightly, where the UI can look messed up as well.

Comment 4

[Marcia Knous [:marcia]]

Tracking 50/51+ for this regression, both from a functionality and visual perspective.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Gijs would know if there's a way to fix the about:certerror code, or if we need to backout Bug 1280584.

Comment 6

[:Gijs (he/him)]

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.

Comment 7

[Johann Hofmann [:johannh]]

I can look into this, let's give Gijs some time to fix up his backlog :)

Comment 8

[Johann Hofmann [:johannh]]

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 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1290927 - Remove hash link from technical info on certerror pages.

Review commit: https://reviewboard.mozilla.org/r/70884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70884/

Comment 10

[Johann Hofmann [:johannh]]

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

[:Gijs (he/him)]

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). :-)

Comment 12

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70884/diff/1-2/

Comment 13

[Johann Hofmann [:johannh]]

I'm not sure the old link was keyboard focusable, but now it should behave exactly like before.

Comment 14

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70884/diff/2-3/

Comment 15

[Johann Hofmann [:johannh]]

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

[:Gijs (he/him)]

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.

Comment 17

[Johann Hofmann [:johannh]]

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.

Comment 18

[Bram Pitoyo [:bram]]

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.

Comment 19

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70884/diff/3-4/

Comment 20

[Johann Hofmann [:johannh]]

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.

Comment 21

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe5366025e46

Comment 22

[Johann Hofmann [:johannh]]

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

Comment 23

[Johann Hofmann [:johannh]]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1262d39de13e

Comment 25

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

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf25a87f8966