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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: keeler, Assigned: johannh)

Tracking

({regression})

50 Branch
mozilla51
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.
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
Flags: needinfo?(valentin.gosu)
[Tracking Requested - why for this release]:
Component: Security → Networking
Product: Firefox → Core
Version: Trunk → 50 Branch
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.
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]
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.
I can look into this, let's give Gijs some time to fix up his backlog :)
Assignee: gijskruitbosch+bugs → jhofmann
Status: NEW → ASSIGNED
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.
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 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)
I'm not sure the old link was keyboard focusable, but now it should behave exactly like before.
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 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+
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)
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)
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 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?
https://hg.mozilla.org/integration/fx-team/rev/1262d39de13e4e782fe855e3ff7f0a179101a162
Bug 1290927 - Remove hash link from technical info on certerror pages. r=Gijs
Blocks: 1295571
https://hg.mozilla.org/mozilla-central/rev/1262d39de13e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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+
You need to log in before you can comment on or make changes to this bug.