566478 - Invalid SSL cert error is not mobile-friendly

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tony Chung [:tchung]]

Attachment: SSL error

When using MozillaGuest and your network times out, websites will throw a scary SSL cert error all across the screen.   I know this is likely by design since i need to re-login to authenticate the network, but i'm wondering if we can display a friendlier message.   

This can be a common use case in a coffee shop wifi, and your wifi account times out.

See screenshot

Repro:
1) Log into public wifi network that requires a timed interval guest account (eg. MozillaGuest)
2) surf around until it times out
3) revisit your webpages, and notice the scary SSL error message

Expected:
- if ssl error, perhaps something more native to fennec and friendlier looking?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm not sure why the dialog is being displayed. The error page should be displayed in web content. The screenshot looks like an error page is being displayed.

We can override nsISSLCertErrorDialog if we need to keep the dialog  from being displayed.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

For the favicon load, perhaps? Firefox had that bug: bug 453442

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)
> For the favicon load, perhaps? Firefox had that bug: bug 453442

Hmm, Fennec has had that bug (and fixed it) 3 times. Maybe this is number 4!

Comment 4

[ahoza]

I see the same problem, only that is displayed in web content, not as a dialog for Android build: Mozilla/5.0 (Android; Linux armv7l;en-US;rv:2.0b3pre) Gecko/20100722 Namoroka / 4.0b3pre Fennec/2.0a1pre
Device: HTC Desire A8181

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Anyone have some websites where I can test a fix?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

This seems to be happen for one of two reasons (or both):
* SecureBrowserUI is broken for e10s pages
* NSS doesn't load in e10s pages.

We seem to use "external error reporting" based on the SecureBrowserUI:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp#415

I also see an assertion about NSS not loading in the child process too.

Lastly, https://mozilla.org correctly shows the error page, not dialog, if I
set browser.tabs.remote=false

depends on bug 575950

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 575950 blocks and fixes most of this

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

is self assigned certificates a part of this?  bug 603361

Comment 11

[Matt Brubeck (:mbrubeck)]

Re-nominating for blocking-fennec.  This was not fixed by bug 575950.  The SSL error dialog is unusable on mobile, and because it opens in a new window it breaks keyboard focus on Android.

Comment 12

[Honza Bambas (:mayhemer)]

Bug 575950 never had to fix this bug.

We have to take and adjust code from bug 570003 and probably do more work in the mobile chrome to make this work again.

-> me

Comment 13

[Andreea Pod]

Device: Motorola Droid 2
BuildID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre) Gecko/20101027
Firefox/4.0b8pre Fennec /4.0b2pre

This error appears for me when visiting https://mozilla.org, https://overstock.com. After dismiss it by clicking cancel or back system button it's impossible to write in URL bar.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v1 - preview

This is a preview (but might be the final!) of the fix for m-c to make the error page appear again.

- based on the patch for bug 570003
- fixed failure of getting "@mozilla.org/nss_errors_service;1" on the child process at [1]: implementation of nsINSSErrorsService moved from nsNSSComponent to a new class mozilla::psm::NSSErrorsService as its impl is not dependent on NSS
- "@mozilla.org/nss_errors_service;1" now joint with NSSErrorsService

Left to do:
Mobile chrome needs to be updated.  The buttons on the page don't work, needs to be remoted.

I can work in this later, if anyone start working on it, please drop a note!

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3794

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 486442 [details] [diff] [review]
v1 - preview

I didn't do an exact comparison of the code you moved to the new files, I assume you didn't make any changes besides the class names. If you want me to do a thorough review of that part, please tell me.

What about the contributors section for the new files? Since it's essentially moved code, I think you should keep the original contributor of that code.

if you fix that, r=kaie

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for mobile-browser

This patch is needed for Fennec to handle the 3 buttons that appear on the, now functional, error page. We basically need to move some code into the content process and send messages back to chrome.

Comment 17

[Kai Engert [:KaiE:]]

Comment on attachment 486442 [details] [diff] [review]
v1 - preview

changing to r+, given that my change request is minor, but please fix before checkin. thanks

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Pushed with contributor changes:
http://hg.mozilla.org/mozilla-central/rev/c3886efa7e57

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 486619 [details] [diff] [review]
patch for mobile-browser

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+  _handleCertException: function _handleCertException(aMessage) {
>+
>+      Browser.loadURI(url);

I think you can use "this" since handleCertException already lives in Browser

>   /**
>    * Compute the sidebar percentage visibility.
>    *
>    * @param [optional] dx
>    * @param [optional] dy an offset distance at which to perform the visibility
>@@ -1122,16 +1103,19 @@ var Browser = {
>         break;
> 
>       case "scroll":
>         if (browser == this.selectedBrowser) {
>           Browser.hideTitlebar();
>           Browser.hideSidebars();
>         }
>         break;
>+      case "Browser:CertException":
>+        Browser._handleCertException(aMessage);
>+        break;
>     }

Same here I think, I'm not sure why there is all those "Browser"?

>diff --git a/chrome/content/content.js b/chrome/content/content.js
>+    
>+        let ot = aEvent.originalTarget;

originalTarget or just target?



r+ with nits adressed

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

I converted the "Browser" -> "this", but I left the "originalTarget" as is. We pulled it from Firefox code and it currently works.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed mobile-browser patch:
http://hg.mozilla.org/mobile-browser/rev/bec84542c46b

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

I had to back the platform patch out. Tests were timing out because the cert dialog was being displayed:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288281887.1288282961.23739.gz#err0

Check out the screenshot in the log. I can attach it too.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot of test failure

Comment 24

[Kai Engert [:KaiE:]]

By default, the core code assumes it's not a browser, and there is no handler that can display messages in a web page. Because of this, the default is to show the errors as dialogs.

If you get dialogs, that means the code fails to detect the ability to display web pages.

I guess you need to come up with a way to detect that reliably in an e10s environment. Search for "ExternalErrorReporting" in nsNSSIOLayer.cpp

Comment 25

[Matt Brubeck (:mbrubeck)]

(In reply to comment #11)
> Re-nominating for blocking-fennec.  This was not fixed by bug 575950.  The SSL
> error dialog is unusable on mobile, and because it opens in a new window it
> breaks keyboard focus on Android.

The keyboard problem is fixed by bug 591307.

Comment 26

[Kai Engert [:KaiE:]]

Attachment: core v2

I should have reviewed v1 more carefully.

The biggest problem in patch v1 is:
You declare the variable secureUI twice, and the later block never sees the value.

I'm now also worried that the semantics around if(secureUI) and if(docshell) have changed.


I have applied the v1 patch locally, and using desktop firefox, I was able to reproduce that I get the incorrect error dialog.

Mark, could you try this patch with firefox mobile?

Comment 27

[Kai Engert [:KaiE:]]

"whitespace-ignore" version of the changes to nsNSSIOLayer.cpp in patch v2:

@@ -379,6 +379,11 @@ nsNSSSocketInfo::EnsureDocShellDependent
   // with a socket close, and the socket transport might detach the callbacks 
   // instance prior to our error reporting.
 
+  nsISecureBrowserUI* secureUI = nsnull;
+#ifdef MOZ_IPC
+  CallGetInterface(proxiedCallbacks.get(), &secureUI);
+#endif
+
   nsCOMPtr<nsIDocShell> docshell;
 
   nsCOMPtr<nsIDocShellTreeItem> item(do_GetInterface(proxiedCallbacks));
@@ -397,7 +402,7 @@ nsNSSSocketInfo::EnsureDocShellDependent
     NS_ASSERTION(docshell, "rootItem do_QI is null");
   }
 
-  if (docshell)
+  if (docshell && !secureUI)
   {
     nsCOMPtr<nsIDocShell> proxiedDocShell;
     NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
@@ -405,10 +410,11 @@ nsNSSSocketInfo::EnsureDocShellDependent
                          docshell.get(),
                          NS_PROXY_SYNC,
                          getter_AddRefs(proxiedDocShell));
-    nsISecureBrowserUI* secureUI = nsnull;
     if (proxiedDocShell)
       proxiedDocShell->GetSecurityUI(&secureUI);
-    if (secureUI)
+  }
+
+  if (docshell && secureUI)
     {
       nsCOMPtr<nsIThread> mainThread(do_GetMainThread());
       NS_ProxyRelease(mainThread, secureUI, PR_FALSE);

Comment 28

[Kai Engert [:KaiE:]]

(In reply to comment #26)
> I have applied the v1 patch locally, and using desktop firefox, I was able to
> reproduce that I get the incorrect error dialog.

... and patch v2 does not have this regression in my local testing.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Patch v2 shows the error dialog, not the error page in fennec.

Comment 30

[Honza Bambas (:mayhemer)]

Kai, thanks for catching the wrong merge, cl.exe didn't want about that.  

Mark, I suspect we don't have docshell, could you check it in a debugger?

Comment 31

[Kai Engert [:KaiE:]]

Attachment: core patch v3

I made a mistake in v2, here is (hopefully correctly) fixed v3.

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Patch v3 works for me in Fennec. I pushed to Try server

Comment 33

[Kai Engert [:KaiE:]]

Attachment: Make it easier to review patch v3

This shows whitespace-ignoring versions of the portion that is different in v1 vs v3.

Given that I can't review my own patch, I'm providing this, and suggest that any other experienced reviewer or super-reviewer could r+ this, no need for another PSM peer reviewer IMHO.

Comment 34

[Honza Bambas (:mayhemer)]

For nsIDocShellTreeItem we return NO_INTERFACE [1].  Docshell is unreachable from HttpChannelChild.

I understand we need the condition for docshell being present, since that is
responsible for the actual external error reporting.  So, it was actually
correct in the version 2 of the patch.  Have to think...

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParentListener.cpp#175

Comment 35

[Honza Bambas (:mayhemer)]

(In reply to comment #34)
> Docshell is unreachable from HttpChannelChild.
I wanted to say "HttpChannelPARENT" of course.

Comment 36

[Kai Engert [:KaiE:]]

Honza, that final block, where I added a check for docshell, doesn't access the docshell variable. Neither the old code, nor the new code, it was my mistake to add that in v2.

Comment 37

[Kai Engert [:KaiE:]]

Comment on attachment 486673 [details] [diff] [review]
core patch v3

Honza, I'm glad you're around. Mark seems to be under pressure to get this released :)

If you can r+ this, would be great. Maybe the other attachment makes it easy for you.

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 486673 [details] [diff] [review]
core patch v3

Kai, when we've got the secureUI, we don't need to Qi/Gi for docshell any more, right?  If so, then my patch with just properly removed second secureUI declaration would the same work as this patch, or not?

Comment 39

[Kai Engert [:KaiE:]]

(In reply to comment #38)
> 
> Kai, when we've got the secureUI, we don't need to Qi/Gi for docshell any more,
> right?  If so, then my patch with just properly removed second secureUI
> declaration would the same work as this patch, or not?

You're probably right, but if you would like to make further patches, we'll have to do another round of testing and try server builds.

It doesn't hurt to query for docshell. If the tryserver build confirms that this patch works, I propose we take this patch, and we can make the performance improvement for the next release.

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 486673 [details] [diff] [review]
core patch v3

This is functionally OK.

Comment 41

[Matt Brubeck (:mbrubeck)]

core patch v3 pushed: http://hg.mozilla.org/mozilla-central/rev/5a356e146729

Comment 42

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101029 Namoroka/4.0b8pre Fennec/4.0b2pre