1220781 - "Advanced" button on certificate errors doesn't work on cert errors in frames or badStsCert - @hidden vs. style.visibility = "hidden"

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Matthew N. [:MattN]]

I received sec_error_unknown_issuer on a site in a frame and the "Advanced" button wasn't functional.

The advanced section had @hidden=true but the button was only toggling `style.visibility` on the contents.

I suspect this is a regression from bug 1207107 as it touched related code (visibility/hidden).

Comment 1

[Brian Grinstead [:bgrins]]

I can reproduce on https://subdomain.preloaded-hsts.badssl.com/

Comment 2

[Brian Grinstead [:bgrins]]

According to a comment in aboutCertError this is attempting to disallow an override: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutcerterror/aboutCertError.xhtml#94-96.

Instead of hiding the advancedPanel I think we should be hiding the exceptions button.  This will more closely match the behavior before the UI change.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1220781 - Hide the exception button, not advanced section for badStsCert or cert errors in frames;r=MattN

Bug 1220781 - Hide the exception button, not advanced section for badStsCert or cert errors in frames;r=MattN

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8682119 [details]
MozReview Request: Bug 1220781 - Hide the exception button, not advanced section for badStsCert or cert errors in frames;r=MattN

https://reviewboard.mozilla.org/r/23971/#review21399

I don't have test pages handy to test this but it seems plausible that this fixes my issue.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] from comment #4)
> Comment on attachment 8682119 [details]
> MozReview Request: Bug 1220781 - Hide the exception button, not advanced
> section for badStsCert or cert errors in frames;r=MattN
> 
> https://reviewboard.mozilla.org/r/23971/#review21399
> 
> I don't have test pages handy to test this but it seems plausible that this
> fixes my issue.

Thanks for the review.  This case is also triggered on any cert error page in a frame, so this should reproduce it also: data:text/html,<iframe src='https://expired.badssl.com'></iframe>

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=205a3e68000f

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7ec97368b2d6

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ec97368b2d6

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7ec97368b2d6

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8682119 [details]
MozReview Request: Bug 1220781 - Hide the exception button, not advanced section for badStsCert or cert errors in frames;r=MattN

Approval Request Comment
[Feature/regressing bug #]: 1207107
[User impact if declined]: User won't be able to view advanced details for non-overridable cert errors
[Describe test coverage new/current, TreeHerder]: New test case
[Risks and why]: Changing the selector of an element that's getting hidden in JS on the cert error page and that element will always exist (from the HTML), so it's not a very big risk.
[String/UUID change made/needed]:

Comment 10

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 45.0a1 (2015-11-04) Win 7

Comment 11

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

Comment on attachment 8682119 [details]
MozReview Request: Bug 1220781 - Hide the exception button, not advanced section for badStsCert or cert errors in frames;r=MattN

The fix has been verified on Nightly, let's uplift to Aurora44.

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c3eed72d9e20

Comment 14

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 44.0a2 (2015-11-11) OS X 10.11