1463748 - Fork and pref-off the new error pages

Status: RESOLVED FIXED

(Firefox :: Security, enhancement, P3)

Comment 1

[Trisha Gupta [:trisha]]

Attachment: bug1463748.patch

I have uploaded a work-in-progress patch for feedback. Can you please see if the pref name is okay? Also, do you think I'm going in the right direction with this? Thanks.

Comment 2

[Johann Hofmann [:johannh]]

Comment on attachment 8982256 [details] [diff] [review]
bug1463748.patch

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

This is going in a great direction! Please assign the URL as I mentioned below and then you can copy aboutNetError.xhtml and change something on the page to check whether it's working...

::: browser/components/about/AboutRedirector.cpp
@@ +149,5 @@
>  
> +  static bool sNCEPEnabledCacheInited = false;
> +  if (!sNCEPEnabledCacheInited) {
> +    Preferences::AddBoolVarCache(&AboutRedirector::sNewCertErrorPageEnabled,
> +                                 "browser.securitynewcerterrorpage.enabled");

maybe browser.security.newcerterrorpage.enabled?

@@ +170,5 @@
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
> +
> +      if (path.EqualsLiteral("certerror")||
> +          (sNewCertErrorPageEnabled && path.EqualsLiteral("certerror"))) {

this is the right condition, but you have one path.EqualsLiteral("certerror") too much in there, it can just be

if (sNewCertErrorPageEnabled && path.EqualsLiteral("certerror"))

@@ +175,5 @@
> +        nsCOMPtr<nsINetUtil> netUtil =
> +          do_GetService("chrome://browser/content/aboutNetError-new.xhtml", &rv);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        rv = netUtil->escapeURL(url);
> +        NS_ENSURE_SUCCESS(rv, rv);

do_GetService is a getter for an XPCOM service (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/XPCOM_Interfaces#XPCOM_Services), so you can't assign the new URL like that. An easier way to do it would be:

url.AssignASCII("chrome://browser/content/aboutNetError-new.xhtml");

Comment 3

[Trisha Gupta [:trisha]]

Attachment: bug1463748.patch

Comment 4

[Trisha Gupta [:trisha]]

Attachment: bug1463748.patch

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8984095 [details] [diff] [review]
bug1463748.patch

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

This looks great, just a few nits. Can you please run 

hg copy browser/base/content/aboutNetError.xhtml browser/base/content/aboutNetError-new.xhtml --after

to make sure hg knows that we've just copied this file (so that blame gets preserved). Afterwards you'll probably have to do hg commit --amend again.

Thank you!

::: browser/app/profile/firefox.js
@@ +984,4 @@
>  // Name of alternate about: page for certificate errors (when undefined, defaults to about:neterror)
>  pref("security.alternate_certificate_error_page", "certerror");
>  
> +//Indicates if new certificate error page (enabled) or not

nit: please add a space behind the //

@@ +984,5 @@
>  // Name of alternate about: page for certificate errors (when undefined, defaults to about:neterror)
>  pref("security.alternate_certificate_error_page", "certerror");
>  
> +//Indicates if new certificate error page (enabled) or not
> +pref("browser.security.newcerterrorpage.enabled", true);

Should we default this to false for now?

::: browser/components/about/AboutRedirector.cpp
@@ +173,5 @@
> +
> +      if (sNewCertErrorPageEnabled && path.EqualsLiteral("certerror")) {
> +        url.AssignASCII("chrome://browser/content/aboutNetError-new.xhtml");
> +      }
> +        

please get rid of this whitespace

Comment 6

[Trisha Gupta [:trisha]]

Attachment: bug1463748.patch

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8984194 [details] [diff] [review]
bug1463748.patch

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

Thanks for correcting the nits, but are you sure that you did hg copy on the aboutNetError-new.xhtml file? Did that succeed? You can also just hg remove it and run

hg copy browser/base/content/aboutNetError.xhtml browser/base/content/aboutNetError-new.xhtml

and commit --amend again. Your patch should then indicate that you've made a copy of the file.

Can you try that? Thanks!

Comment 8

[Trisha Gupta [:trisha]]

Attachment: Bug 1463748 - Fork and pref-off the new error pages

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

Comment 9

[Trisha Gupta [:trisha]]

Comment on attachment 8985098 [details]
Bug 1463748 - Fork and pref-off the new error pages

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

Comment 10

[Trisha Gupta [:trisha]]

Comment on attachment 8985098 [details]
Bug 1463748 - Fork and pref-off the new error pages

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

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8985098 [details]
Bug 1463748 - Fork and pref-off the new error pages

https://reviewboard.mozilla.org/r/250806/#review257236

This looks great, thank you!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac73efd7dea3
Fork and pref-off the new error pages r=johannh

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ac73efd7dea3