Closed Bug 566478 Opened 10 years ago Closed 9 years ago

Invalid SSL cert error is not mobile-friendly

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: tchung, Assigned: mayhemer)

References

()

Details

(Whiteboard: rc1.1)

Attachments

(5 files, 2 obsolete files)

Attached image 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?
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.
For the favicon load, perhaps? Firefox had that bug: bug 453442
(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!
Assignee: nobody → mark.finkle
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
tracking-fennec: --- → 2.0b2+
Duplicate of this bug: 588437
Duplicate of this bug: 594460
Anyone have some websites where I can test a fix?
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
Depends on: 575950
bug 575950 blocks and fixes most of this
tracking-fennec: 2.0b2+ → 2.0-
OS: Mac OS X → All
Hardware: x86 → All
Summary: Scary looking Invalid SSL cert error on n900 → Invalid SSL cert error is not mobile-friendly
Version: 1.9.2 Branch → Trunk
is self assigned certificates a part of this?  bug 603361
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.
tracking-fennec: 2.0- → ?
tracking-fennec: ? → 2.0b2+
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
Assignee: mark.finkle → honzab.moz
Status: NEW → ASSIGNED
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.
Attached patch v1 - preview (obsolete) — Splinter Review
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
Attachment #486442 - Flags: review?(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
Attachment #486442 - Flags: review?(kaie) → review-
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.
Attachment #486619 - Flags: review?(21)
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
Attachment #486442 - Flags: review- → review+
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
Attachment #486619 - Flags: review?(21) → review+
I converted the "Browser" -> "this", but I left the "originalTarget" as is. We pulled it from Firefox code and it currently works.
pushed mobile-browser patch:
http://hg.mozilla.org/mobile-browser/rev/bec84542c46b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
(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.
Attached patch core v2 (obsolete) — Splinter Review
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?
"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);
(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.
Patch v2 shows the error dialog, not the error page in fennec.
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?
Attached patch core patch v3Splinter Review
I made a mistake in v2, here is (hopefully correctly) fixed v3.
Attachment #486655 - Attachment is obsolete: true
Patch v3 works for me in Fennec. I pushed to Try server
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.
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
(In reply to comment #34)
> Docshell is unreachable from HttpChannelChild.
I wanted to say "HttpChannelPARENT" of course.
Status: REOPENED → ASSIGNED
Attachment #486442 - Attachment is obsolete: true
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 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.
Attachment #486673 - Flags: review?(honzab.moz)
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?
(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 on attachment 486673 [details] [diff] [review]
core patch v3

This is functionally OK.
Attachment #486673 - Flags: review?(honzab.moz) → review+
core patch v3 pushed: http://hg.mozilla.org/mozilla-central/rev/5a356e146729
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101029 Namoroka/4.0b8pre Fennec/4.0b2pre
Status: RESOLVED → VERIFIED
Duplicate of this bug: 570003
Duplicate of this bug: 609262
You need to log in before you can comment on or make changes to this bug.