Closed Bug 1240594 Opened 8 years ago Closed 8 years ago

Merge about:neterror and about:certerror

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- verified

People

(Reporter: ttaubert, Assigned: nhnt11)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(4 files, 9 obsolete files)

10.19 KB, patch
nhnt11
: review+
Details | Diff | Splinter Review
4.84 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
8.39 KB, patch
nhnt11
: review+
Details | Diff | Splinter Review
41.42 KB, patch
nhnt11
: review+
Details | Diff | Splinter Review
We should merge those pages, they are way too similar to be separate.
FYI: If "security.alternate_certificate_error_page" is removed from firefox.js, cert errors will also use about:neterror.
Assignee: nobody → nhnt11
Priority: -- → P3
Whiteboard: [fxprivacy]
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P3 → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Attached patch Patch (obsolete) — Splinter Review
OK, this has taken me a while but after overcoming my paranoia of breaking things by merging these two files that seem to have been forked back in 2008, I finally have a patch.

One thing I haven't done in this patch is actually get rid of about:certerror. There are a couple of hard-coded references to the page in places other than browser/ ([1] [2] and [3]). [3] likely doesn't need to change, but changing [1] and [2] would likely break mobile/b2g (and other consumers).

Instead, I've simply pointed about:certerror to aboutNetError.xhtml in AboutRedirector.cpp. I'm not sure if this is the best approach and am open to suggestions.

This patch works and as far as I can tell, achieves at least parity with the current UIs. A couple of tests have been updated, and I'll do a try push to make sure nothing else breaks.

It's also worth mentioning that I've attempted to clean up unused CSS and stuff whenever I encountered it. I'm not requesting review yet because I still feel like more cleanup can possibly be done. I'll do another self-review and set a flag after that, but I really wanted to get this up so I'm attaching it anyway. I'll also try and split it into smaller patches before requesting r?.

[1] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/docshell/base/nsDocShell.cpp#4944
[2] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/dom/browser-element/BrowserElementChildPreload.js#1990
[3] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/accessible/base/nsCoreUtils.cpp#442
Attached patch Merge UI code (obsolete) — Splinter Review
Try push caught a test fail, fixed in this patch.

Some notes to help with the review:

- I moved the aboutCertError-specific init code to its own function. It's called from the main initPage() function with an early return for the certerror case.
- I added the few certerror-specific UI elements into the markup, rather than try to reuse existing elements with hacky JS to move stuff around (specifically, I didn't merge the weak-crypto and bad-cert advanced panels).
- I took the opportunity to de-duplicate the message passing code for cert/neterrors in content.js (in a separate patch, attached after this).
- The rest of the patch is moving the certerror specific CSS and strings into neterror code.

I also mentioned some stuff in comment 2, please have a look at that too.
Attachment #8728740 - Attachment is obsolete: true
Attachment #8730621 - Flags: review?(ttaubert)
Attached patch Deduplicate message passing code (obsolete) — Splinter Review
Attachment #8730622 - Flags: review?(ttaubert)
Attached patch Update tests (obsolete) — Splinter Review
Attachment #8730624 - Flags: review?(ttaubert)
Comment on attachment 8730621 [details] [diff] [review]
Merge UI code

Panos, can you please find a good reviewer for this? I don't do a lot HTML/CSS/JS these days anymore and never knew a lot about these error pages.
Attachment #8730621 - Flags: review?(ttaubert) → review?(past)
Attachment #8730622 - Flags: review?(ttaubert) → review?(past)
Attachment #8730624 - Flags: review?(ttaubert) → review?(past)
Comment on attachment 8730621 [details] [diff] [review]
Merge UI code

Gijs is already doing the review for bug 1220481, so he is the ideal candidate for the job.
Attachment #8730621 - Flags: review?(past) → review?(gijskruitbosch+bugs)
Attachment #8730622 - Flags: review?(past) → review?(gijskruitbosch+bugs)
Attachment #8730624 - Flags: review?(past) → review?(gijskruitbosch+bugs)
Comment on attachment 8730621 [details] [diff] [review]
Merge UI code

Review of attachment 8730621 [details] [diff] [review]:
-----------------------------------------------------------------

This was much easier to review than I thought it would be. Thank you so much for doing this, and for splitting up the patches. This isn't quite there yet, but AFAICT it's very very close.

::: browser/base/content/aboutNetError.xhtml
@@ +106,5 @@
>          node.style.display = toggle[node.style.display];
> +        // Toggling the advanced panel must ensure that the debugging
> +        // information panel is hidden as well, since it's opened by the
> +        // error code link in the advanced panel.
> +        if (node.id == "badCertAdvancedPanel") {

I think it'd be neater if we did this when callers call this with the cert panel, instead of in here.

@@ +123,5 @@
>          document.getElementById("buttonContainer").style.display = "flex";
>          document.getElementById("advancedButton").style.display = "block";
>          document.getElementById("errorTryAgain").style.display = "none";
>  
> +        var isCertError = getErrorCode() == "nssBadCert";

Heh, reading through the code throughout the system here, argh for how messy the logic here is.

- when there's a security error, docshell assigns nssBadCert in some cases (sslv3 and some other stuff gets a separate error code) and consults a pref (security.alternate_certificate_error_page) to use an alternative error page ("certerror" is the value in desktop, mobile and b2g).
- 2 lines later, docshell literally checks for the "certerror" value, even though that's a pref, in order to generate telemetry...
- mobile checks for "about:certerror?e=nssBadCert", so don't anyone dare change the order in which docshell appends the URL arguments;
- we use "nssFailure2" for some other certificate issues, and we also have ssl v3 things and some other thingymajigs.

But really, there's no reason why nssBadCert is only and/or always corresponding with about:certerror. That's just what docshell happens to do right now...

... but also, aboutNetError.xhtml checks for nssBadCert already, though if my reading of the code is right that case currently NEVER HAPPENS. Sigh. So that's just dead code that we've been lugging around for a while. Except now we're making it non-dead, just to be confusing. :-)

Seems to me that we should treat everything in the:

  } else if (NS_ERROR_GET_MODULE(aError) == NS_ERROR_MODULE_SECURITY) {

block in docshell the same here, but we can delay making this sane-er in favour of keeping parity for now. Let's try to remember to file a followup bug.

@@ +166,5 @@
> +        showAdvancedButton(true);
> +
> +        var cssClass = getCSSClass();
> +        if (cssClass == "expertBadCert") {
> +          toggleDisplay('badCertAdvancedPanel');

Nit: we like our double quotes.

@@ +201,5 @@
> +          document.getElementById("badStsCertExplanation").removeAttribute("hidden");
> +        }
> +
> +
> +        var tech = document.getElementById("technicalContentText");

This is new, so can we name it something that indicates it's specific to cert errors?

@@ +202,5 @@
> +        }
> +
> +
> +        var tech = document.getElementById("technicalContentText");
> +        if (tech)

Why are we nullchecking this?

@@ -211,5 @@
>            favicon.setAttribute("href", "chrome://global/skin/icons/" + className + "_favicon.png");
>            faviconParent.appendChild(favicon);
>          }
> -        if (className == "expertBadCert") {
> -          showSecuritySection();

We're killing all the callsites, but you seem to be leaving this function in? Please remove the function also. :-)

@@ +405,5 @@
> +          if (errorCode) {
> +            errorCode.href = "#technicalInformation";
> +            errorCode.addEventListener("click", () => {
> +              var div = document.getElementById("certificateErrorDebugInformation");
> +              if (div.style.display == "block") {

Seems like we could make toggleDisplay() return the new state and use that to call scrollIntoView() when we're showing the element?

@@ +409,5 @@
> +              if (div.style.display == "block") {
> +                div.style.display = "none";
> +              } else {
> +                div.style.display = "block";
> +                div.scrollIntoView(true);

While we're here, can we use the beautiful new extra options object parameter to make this smooth? ( https://developer.mozilla.org/en/docs/Web/API/Element/scrollIntoView )

@@ -471,5 @@
> -        <!-- Override section - For ssl errors only.  Removed on init for other
> -             error types.  -->
> -        <div id="securityOverrideDiv">
> -          <a id="securityOverrideLink" href="javascript:showSecuritySection();" >&securityOverride.linkText;</a>
> -          <div id="securityOverrideContent" style="display: none;">&securityOverride.warningContent;</div>

If we don't use these strings anymore now, we should probably remove them?

::: browser/locales/en-US/chrome/overrides/netError.dtd
@@ +138,5 @@
>    <li>Please contact the website owners to inform them of this problem.</li>
>  </ul>
>  ">
>  
> +<!ENTITY nssBadCert.title "Your connection is not secure">

After reading through your comments, I'm not sure why these strings are changing as part of this patch. Can you clarify why this is happening and/or split it off into its own patch?

It looks like you copied this from certerror.introPara - maybe we can just copy that string with the same identifier here, and stick it in a hidden element on the HTML page that we hide when the error page isn't about:certerror ?

More generally, we shouldn't change strings without changing their identifier, because otherwise other locales won't know the string has changed and won't update.

::: browser/themes/shared/aboutNetError.css
@@ +123,5 @@
>    display: none;
>  }
>  
> +div#weakCryptoAdvancedPanel,
> +div#badCertAdvancedPanel {

Neither of these need the tag name in front of them.

@@ +133,5 @@
>     * makes the overall div look uneven */
>    padding: 0 12px 12px 12px;
>    box-shadow: 0 0 4px #ddd;
>    font-size: 0.9em;
> +  margin-top: 24px;

Nit: You nixed the comment here, can you put it back please? :-)
Attachment #8730621 - Flags: review?(gijskruitbosch+bugs) → feedback+
Comment on attachment 8730622 [details] [diff] [review]
Deduplicate message passing code

Review of attachment 8730622 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

r=me if there's a green trypush for this (plus everything else) at the end.
Attachment #8730622 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8730624 [details] [diff] [review]
Update tests

Review of attachment 8730624 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8730624 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8730621 [details] [diff] [review]
> Merge UI code
> 
> Review of attachment 8730621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This was much easier to review than I thought it would be. Thank you so much
> for doing this, and for splitting up the patches. This isn't quite there
> yet, but AFAICT it's very very close.
> 
> ::: browser/base/content/aboutNetError.xhtml
> @@ +106,5 @@
> >          node.style.display = toggle[node.style.display];
> > +        // Toggling the advanced panel must ensure that the debugging
> > +        // information panel is hidden as well, since it's opened by the
> > +        // error code link in the advanced panel.
> > +        if (node.id == "badCertAdvancedPanel") {
> 
> I think it'd be neater if we did this when callers call this with the cert
> panel, instead of in here.

Thinking about this more, we could create a separate utility function, too - it just feels wrong to have element-specific code in such a generically-named "toggle some element" function, if that makes sense.
(In reply to :Gijs Kruitbosch from comment #8)
> ... but also, aboutNetError.xhtml checks for nssBadCert already, though if
> my reading of the code is right that case currently NEVER HAPPENS. Sigh. So
> that's just dead code that we've been lugging around for a while. Except now
> we're making it non-dead, just to be confusing. :-)

Well, at least it might fix Bug 1002523 in the process.
(In reply to :Cykesiopka from comment #12)
> (In reply to :Gijs Kruitbosch from comment #8)
> > ... but also, aboutNetError.xhtml checks for nssBadCert already, though if
> > my reading of the code is right that case currently NEVER HAPPENS. Sigh. So
> > that's just dead code that we've been lugging around for a while. Except now
> > we're making it non-dead, just to be confusing. :-)
> 
> Well, at least it might fix Bug 1002523 in the process.

The code is just removed in this patch. It sounds like we should treat nssFailure2 the same way as nssBadCert, and that might fix this "properly", but in principle that could be a followup bug, as that isn't strictly required for unifying these two files.
(In reply to :Gijs Kruitbosch from comment #13)
> The code is just removed in this patch. It sounds like we should treat
> nssFailure2 the same way as nssBadCert, and that might fix this "properly",
> but in principle that could be a followup bug, as that isn't strictly
> required for unifying these two files.

... and this is why I should actually re-read stuff and stare at the attached patches before I make a comment. Anyways, yes, no need for anything that isn't about unifying these files to be done in this bug.
Attached patch Merge UI code v2 (obsolete) — Splinter Review
Addressed review comments. String changes moved to a separate patch, coming up.
Attachment #8730621 - Attachment is obsolete: true
Attachment #8732730 - Flags: review?(gijskruitbosch+bugs)
Attached patch Update strings (obsolete) — Splinter Review
Attachment #8732731 - Flags: review?(gijskruitbosch+bugs)
FWIW I didn't copy over the certerror string identifiers because I felt it'd be more "complete" to get rid of any references hinting that certerror had its own html markup and DTD. I do understand the l10n practicality of keeping the identifiers though so I've done that in this patch.
Comment on attachment 8732730 [details] [diff] [review]
Merge UI code v2

Review of attachment 8732730 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below fixed (note that 1 of these is the same as last time, and 1 is somewhere where you've replaced things in one place but not another - did you test this version of the patch?)

::: browser/base/content/aboutNetError.xhtml
@@ +116,5 @@
>          document.getElementById("buttonContainer").style.display = "flex";
>          document.getElementById("advancedButton").style.display = "block";
>          document.getElementById("errorTryAgain").style.display = "none";
>  
> +        var isCertError = getErrorCode() == "nssBadCert";

We do this comparison a number of times. Can we just create a global for this, like gIsCertError, that we set once and reuse? That will also make it easier to incorporate other changes to that error code if/when necessary.

@@ +205,5 @@
> +        if (cssClass == "badStsCert") {
> +          document.getElementById("badStsCertExplanation").removeAttribute("hidden");
> +        }
> +
> +        document.getElementById("badCertTechnicalInfo").textContent = getDescription();

You renamed this here but the CSS still uses the old selector.

::: browser/themes/shared/aboutNetError.css
@@ +133,5 @@
>     * makes the overall div look uneven */
>    padding: 0 12px 12px 12px;
>    box-shadow: 0 0 4px #ddd;
>    font-size: 0.9em;
> +  margin-top: 24px;

This is still missing the comment that was here originally. ("Align with the try again button")
Attachment #8732730 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8732731 [details] [diff] [review]
Update strings

Review of attachment 8732731 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/overrides/netError.dtd
@@ +138,5 @@
>    <li>Please contact the website owners to inform them of this problem.</li>
>  </ul>
>  ">
>  
> +<!ENTITY certerror.longpagetitle1 "Your connection is not secure">

Hm, well, I see your point about the names of the strings, but because they're in a different file I actually don't know what would be best. Francesco can probably help.

flod, these strings are essentially moving file. Should we use new identifiers, or can we keep the same ones (ie is either option better for localizers) ?

Note that either way you should fold/roll this patch into the "merge UI" one when landing because one won't run without the other (missing dtd references).
Attachment #8732731 - Flags: review?(gijskruitbosch+bugs)
Attachment #8732731 - Flags: review?(francesco.lodolo)
Attachment #8732731 - Flags: review+
> @@ +205,5 @@
> > +        if (cssClass == "badStsCert") {
> > +          document.getElementById("badStsCertExplanation").removeAttribute("hidden");
> > +        }
> > +
> > +        document.getElementById("badCertTechnicalInfo").textContent = getDescription();
> 
> You renamed this here but the CSS still uses the old selector.

That's terrible, sorry. I made this change at the end and forgot about it, and didn't test before uploading tonight. 

> ::: browser/themes/shared/aboutNetError.css
> @@ +133,5 @@
> >     * makes the overall div look uneven */
> >    padding: 0 12px 12px 12px;
> >    box-shadow: 0 0 4px #ddd;
> >    font-size: 0.9em;
> > +  margin-top: 24px;
> 
> This is still missing the comment that was here originally. ("Align with the
> try again button")

Ah, meant to reply to this. This line, while identical to the one with that comment, is actually serving a different purpose - to add a bit of space between the button container and the advanced panel. I can add a comment to that effect, but it's not really aligning with the try again button. Also by the way, note that the rule was for "#weakCryptoAdvanced" - a non existent element (even before this patch).
(In reply to Nihanth Subramanya [:nhnt11] from comment #20)
> > ::: browser/themes/shared/aboutNetError.css
> > @@ +133,5 @@
> > >     * makes the overall div look uneven */
> > >    padding: 0 12px 12px 12px;
> > >    box-shadow: 0 0 4px #ddd;
> > >    font-size: 0.9em;
> > > +  margin-top: 24px;
> > 
> > This is still missing the comment that was here originally. ("Align with the
> > try again button")
> 
> Ah, meant to reply to this. This line, while identical to the one with that
> comment, is actually serving a different purpose - to add a bit of space
> between the button container and the advanced panel. I can add a comment to
> that effect, but it's not really aligning with the try again button.

Oh, right. No, never mind then.

> Also by
> the way, note that the rule was for "#weakCryptoAdvanced" - a non existent
> element (even before this patch).

/me cries

All the better this is getting some cleanup. There's other things we should do at some point, like make this code use URLSearchParams and whatnot, but that can wait.
Comment on attachment 8732731 [details] [diff] [review]
Update strings

Review of attachment 8732731 [details] [diff] [review]:
-----------------------------------------------------------------

When a string moves to another file, it's basically a brand new string, so keeping the existing identifier is OK (it's only important to change the ID when the string changes within the same file).

Having said that, I personally prefer not having "indexed" identifiers in these cases (certerror.pagetitle instead of certerror.pagetitle1), since there's no need to invalidate an existing string within the file. But I'm fine with the current patch (with the comment fixed).

::: browser/locales/en-US/chrome/overrides/netError.dtd
@@ +139,5 @@
>  </ul>
>  ">
>  
> +<!ENTITY certerror.longpagetitle1 "Your connection is not secure">
> +<!-- Localization note (nssBadCert.longDesc2) - The text content of the span tag

nssBadCert.longDesc2 should be certerror.introPara.

@@ +143,5 @@
> +<!-- Localization note (nssBadCert.longDesc2) - The text content of the span tag
> +will be replaced at runtime with the name of the server to which the user
> +was trying to connect. -->
> +<!ENTITY certerror.introPara "The owner of <span class='hostname'/> has configured
> +their website improperly.  To protect your information from being stolen,

I know this is an old string present in other files, but I wonder if we should check copy in a separate bug (owner (singular)…their (plural)?).
Attachment #8732731 - Flags: review?(francesco.lodolo) → review+
(In reply to Francesco Lodolo [:flod] from comment #22)
> @@ +143,5 @@
> > +<!-- Localization note (nssBadCert.longDesc2) - The text content of the span tag
> > +will be replaced at runtime with the name of the server to which the user
> > +was trying to connect. -->
> > +<!ENTITY certerror.introPara "The owner of <span class='hostname'/> has configured
> > +their website improperly.  To protect your information from being stolen,
> 
> I know this is an old string present in other files, but I wonder if we
> should check copy in a separate bug (owner (singular)…their (plural)?).

"Their" in this context is a gender-neutral possessive (because we don't know if the owner should be "his" or "her" or "its" if it's a company or whatever). It is correct English, see https://en.wikipedia.org/wiki/Singular_they
(In reply to :Gijs Kruitbosch from comment #23)
> "Their" in this context is a gender-neutral possessive (because we don't
> know if the owner should be "his" or "her" or "its" if it's a company or
> whatever). It is correct English, see
> https://en.wikipedia.org/wiki/Singular_they

Thanks, learned something completely new :-)
Attached patch Merge UI code v3 (obsolete) — Splinter Review
Kept forgetting to attach this, sorry for the ridiculous delay.
Attachment #8732730 - Attachment is obsolete: true
Attachment #8738345 - Flags: review+
Attached patch Update strings v2 (obsolete) — Splinter Review
Attachment #8738346 - Flags: review+
Attached patch Merge UI code v4 (obsolete) — Splinter Review
Something made me want to look at this patch again before landing, and it turns out I didn't quite get the content events to work correctly. This fixes that. It's straightforward but I'd like you to take a look to be sure (BMO interdiff between v4 and v3 should suffice).

Meanwhile I've done yet another try push, to be sure-er.
Attachment #8732731 - Attachment is obsolete: true
Attachment #8738345 - Attachment is obsolete: true
Attachment #8738370 - Flags: review?(gijskruitbosch+bugs)
Fix typo: event -> evt
Attachment #8730622 - Attachment is obsolete: true
Attachment #8738371 - Flags: review+
Attached patch Update tests v2Splinter Review
Jared, this updates the whitelist in browser_misused_characters_in_strings.js to reflect that certerror.introPara now lives in netError.dtd.

Should be enough for you to look at the interdiff between "Update tests v2" and "Update tests".
Attachment #8730624 - Attachment is obsolete: true
Attachment #8738377 - Flags: review?(jaws)
I needed to keep the certerror.introPara string all in one line (as I explained in bug 1259859 comment 34) in order to make browser_misused_characters_in_strings.js pass.

Technically this seems to be due to a bug in the test itself - and I have reported that in the bug as I said above - but in the interest of moving this forward I'm making the change here. The original string in aboutCertError.dtd didn't have newlines anyway.
Attachment #8738346 - Attachment is obsolete: true
Attachment #8738379 - Flags: review+
Comment on attachment 8738370 [details] [diff] [review]
Merge UI code v4

Review of attachment 8738370 [details] [diff] [review]:
-----------------------------------------------------------------

Interdiff looks fabulous besides the teeny tiny nit below. Thanks!

::: browser/base/content/aboutNetError.xhtml
@@ +219,5 @@
>        {
>          var err = getErrorCode();
> +        if (err == "nssBadCert") {
> +          gIsCertError = true;
> +        }

Nit: gIsCertError = err == "nssBadCert";

if statements that only assign a boolean are almost always unnecessary. :-)
Attachment #8738370 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8738377 [details] [diff] [review]
Update tests v2

Review of attachment 8738377 [details] [diff] [review]:
-----------------------------------------------------------------

r=me while I'm here anyway.
Attachment #8738377 - Flags: review?(jaws) → review+
Blocks: 712612
Would you be able to fix the test? The test is currently doing a regex on the DTD file to get the entities, but that regex isn't working across lines. See http://hg.mozilla.org/mozilla-central/annotate/68c0b7d6f16c/browser/base/content/test/general/browser_misused_characters_in_strings.js#l137
Flags: needinfo?(nhnt11)
Addressed review comment.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33)
> Would you be able to fix the test? The test is currently doing a regex on
> the DTD file to get the entities, but that regex isn't working across lines.
> See
> http://hg.mozilla.org/mozilla-central/annotate/68c0b7d6f16c/browser/base/
> content/test/general/browser_misused_characters_in_strings.js#l137

Filed bug 1262648.
Attachment #8738370 - Attachment is obsolete: true
Flags: needinfo?(nhnt11)
Attachment #8738802 - Flags: review+
Iteration: --- → 48.3 - Apr 18
Priority: P2 → P1
QA Contact: paul.silaghi
Depends on: 1263174
I'm not sure how to test this. Could please provide some additional info?
Flags: needinfo?(nhnt11)
(In reply to Paul Silaghi, QA [:pauly] from comment #37)
> I'm not sure how to test this. Could please provide some additional info?

This should be a change with no user-visible effects. It would be good if QA could do exploratory testing around different network and SSL error pages (https://badssl.com/ might be helpful for the SSL ones) to check for any regressions. Does that help?
Flags: needinfo?(nhnt11) → needinfo?(paul.silaghi)
Yes, thanks.
Did some exploratory testing on 48.0a1 (2016-04-11) Win 7.
Everything looks fine, except bug 1263174.
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Depends on: 1269253
Before this patch landed we have shown the domain under the advanced section for which the certificate was valid. Now we do not have this information anymore. Here an example: https://ssl-unknownissuer.mozqa.com. Is that an expected change?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Henrik Skupin (:whimboo) from comment #40)
> Before this patch landed we have shown the domain under the advanced section
> for which the certificate was valid. Now we do not have this information
> anymore. Here an example: https://ssl-unknownissuer.mozqa.com. Is that an
> expected change?

That string comes from network code, not from the error page itself, and I therefore suspect the change here was caused by a separate/orthogonal commit/bugfix (because I can see the difference in the string as passed in the document's URI (evaluate document.documentURI in the web console when the error page is loaded) when comparing 47 and 49). Did you actually run mozregression on this? Either way, not expected for this bug, please file a separate bug and doublecheck the regression range.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(hskupin)
(In reply to :Gijs Kruitbosch from comment #41)
> (In reply to Henrik Skupin (:whimboo) from comment #40)
> > Before this patch landed we have shown the domain under the advanced section
> > for which the certificate was valid. Now we do not have this information
> > anymore. Here an example: https://ssl-unknownissuer.mozqa.com. Is that an
> > expected change?
> 
> That string comes from network code, not from the error page itself, and I
> therefore suspect the change here was caused by a separate/orthogonal
> commit/bugfix (because I can see the difference in the string as passed in
> the document's URI (evaluate document.documentURI in the web console when
> the error page is loaded) when comparing 47 and 49). Did you actually run
> mozregression on this? Either way, not expected for this bug, please file a
> separate bug and doublecheck the regression range.

This looks like Bug 1261936. In particular, note that https://ssl-unknownissuer.mozqa.com uses a cert that only has a Common Name and no Subject Alternative Name entries. For certs like these, this is exactly the behaviour that is expected.
Flags: needinfo?(hskupin)
(In reply to Carsten Book [:Tomcat] from comment #36)
> https://hg.mozilla.org/mozilla-central/rev/11e583eaf726
> 
> +<!ENTITY certerror.whatShouldIDo.badStsCertExplanation "This site uses HTTP
> +Strict Transport Security (HSTS) to specify that &brandShortName; only connect
> +to it securely.

http://mxr.mozilla.org/mozilla-aurora/source/browser/locales/en-US/chrome/overrides/netError.dtd#195

It looks like there is a "can" or "may" missing here.
(In reply to Ton from comment #43)
> (In reply to Carsten Book [:Tomcat] from comment #36)
> > https://hg.mozilla.org/mozilla-central/rev/11e583eaf726
> > 
> > +<!ENTITY certerror.whatShouldIDo.badStsCertExplanation "This site uses HTTP
> > +Strict Transport Security (HSTS) to specify that &brandShortName; only connect
> > +to it securely.
> 
> http://mxr.mozilla.org/mozilla-aurora/source/browser/locales/en-US/chrome/
> overrides/netError.dtd#195
> 
> It looks like there is a "can" or "may" missing here.

Good spot. This was already the case for the previous string. I'll file a new bug to deal with it.
Blocks: 1270807
You need to log in before you can comment on or make changes to this bug.