Merge about:neterror and about:certerror

VERIFIED FIXED in Firefox 48

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: nhnt11)

Tracking

unspecified
Firefox 48
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox48 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(4 attachments, 9 obsolete attachments)

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
(Reporter)

Description

3 years ago
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]

Updated

2 years ago
Whiteboard: [fxprivacy] → [fxprivacy] [triage]

Updated

2 years ago
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P3 → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
(Assignee)

Comment 2

2 years ago
Created attachment 8728740 [details] [diff] [review]
Patch

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
(Assignee)

Comment 3

2 years ago
Created attachment 8730621 [details] [diff] [review]
Merge UI code

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)
(Assignee)

Comment 4

2 years ago
Created attachment 8730622 [details] [diff] [review]
Deduplicate message passing code
Attachment #8730622 - Flags: review?(ttaubert)
(Assignee)

Comment 5

2 years ago
Created attachment 8730624 [details] [diff] [review]
Update tests
Attachment #8730624 - Flags: review?(ttaubert)
(Reporter)

Comment 6

2 years ago
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)
(Reporter)

Updated

2 years ago
Attachment #8730622 - Flags: review?(ttaubert) → review?(past)
(Reporter)

Updated

2 years ago
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 8

2 years ago
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 9

2 years ago
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 10

2 years ago
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+

Comment 11

2 years ago
(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.

Comment 12

2 years ago
(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.

Comment 13

2 years ago
(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.

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
Created attachment 8732730 [details] [diff] [review]
Merge UI code v2

Addressed review comments. String changes moved to a separate patch, coming up.
Attachment #8730621 - Attachment is obsolete: true
Attachment #8732730 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 16

2 years ago
Created attachment 8732731 [details] [diff] [review]
Update strings
Attachment #8732731 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 17

2 years ago
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 18

2 years ago
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 19

2 years ago
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+
(Assignee)

Comment 20

2 years ago
> @@ +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).

Comment 21

2 years ago
(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+

Comment 23

2 years ago
(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 :-)
(Assignee)

Comment 25

2 years ago
Created attachment 8738345 [details] [diff] [review]
Merge UI code v3

Kept forgetting to attach this, sorry for the ridiculous delay.
Attachment #8732730 - Attachment is obsolete: true
Attachment #8738345 - Flags: review+
(Assignee)

Comment 26

2 years ago
Created attachment 8738346 [details] [diff] [review]
Update strings v2
Attachment #8738346 - Flags: review+
(Assignee)

Comment 27

2 years ago
Created attachment 8738370 [details] [diff] [review]
Merge UI code v4

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)
(Assignee)

Comment 28

2 years ago
Created attachment 8738371 [details] [diff] [review]
Deduplicate message passing code v1.1

Fix typo: event -> evt
Attachment #8730622 - Attachment is obsolete: true
Attachment #8738371 - Flags: review+
(Assignee)

Comment 29

2 years ago
Created attachment 8738377 [details] [diff] [review]
Update tests v2

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)
(Assignee)

Comment 30

2 years ago
Created attachment 8738379 [details] [diff] [review]
Update strings v2.1

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 31

2 years ago
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 32

2 years ago
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: 1242886
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)
(Assignee)

Comment 34

2 years ago
Created attachment 8738802 [details] [diff] [review]
Merge UI code v4.1

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+
(Assignee)

Comment 35

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/1839dee776400ef389063c87707ae40596ec77dc
Bug 1240594 - Merge about:neterror and about:certerror UI code. r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/11e583eaf726a4065a65642cdfb5583b65d6065f
Bug 1240594 - Remove aboutCertError.dtd and move strings still in use to neterror.dtd. r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/9bebd608252d7620b1e4977363cec3bc2c37ec36
Bug 1240594 - Deduplicate certerror and neterror message passing code. r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/6c75ca57566bae6b9586fd843e90616f919c666f
Bug 1240594 - Update tests that touch about:certerror. r=Gijs

Comment 36

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1839dee77640
https://hg.mozilla.org/mozilla-central/rev/11e583eaf726
https://hg.mozilla.org/mozilla-central/rev/9bebd608252d
https://hg.mozilla.org/mozilla-central/rev/6c75ca57566b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

2 years ago
Iteration: --- → 48.3 - Apr 18
Priority: P2 → P1
QA Contact: paul.silaghi

Updated

2 years ago
Depends on: 1263174
I'm not sure how to test this. Could please provide some additional info?
Flags: needinfo?(nhnt11)

Comment 38

2 years ago
(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
status-firefox48: fixed → verified
Flags: needinfo?(paul.silaghi)

Updated

2 years ago
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)

Comment 41

2 years ago
(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)

Comment 42

2 years ago
(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.

Updated

2 years ago
Flags: needinfo?(hskupin)

Comment 43

2 years ago
(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.

Comment 44

2 years ago
(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.

Updated

2 years ago
Blocks: 1270807
You need to log in before you can comment on or make changes to this bug.