570003 - [e10s] Make the certificate error page appear

Status: RESOLVED DUPLICATE of bug 566478

(Core :: DOM: Navigation, defect)

Description

[Honza Bambas (:mayhemer)]

While I was trying to make the security state notifications work in e10s and wanted to display "Certificate error" page, I realized there is no connection between the docshell running on the chrome process, that is allowed to display chrome pages, and the docshell running on the content process having all (or at least most of) load status information to call nsDocShell::DisplayLoadError.

Is there any bug or plan to make this work again?  Do we even want to do this or, is there some other way to display error pages in e10s?

If not, I will start thinking of how to re-establish this mechanism.

(Maybe we should start thinking of some mirroring of loadgroups and docshell between chrome and content ?)

Comment 1

[Honza Bambas (:mayhemer)]

Changing scope of this bug, problem has been found in a different place (bug 570616).  I have previously set breakpoint to a wrong place leading me to persuasion there is no way to display load error pages.

This bug is about making "External Reporting" of certificate errors work again, bug 536301 comment 6.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

HttpChannelParent now provides the security UI by GI on its TabParent.

Comment 3

[Honza Bambas (:mayhemer)]

Note: we will probably need this patch, pressing cancel in the dialog cause crashes in the sec info serialization code (bug in the binary stream).

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to comment #3)
> Note: we will probably need this patch, pressing cancel in the dialog cause
> crashes in the sec info serialization code (bug in the binary stream).

Sorry for spam!  I forgot this hunk [1] from bug 570616 fixes that crash, so this is NOT NEEDED to avoid any crashes.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=462039&action=diff#a/security/manager/ssl/src/nsSSLStatus.cpp_sec1

Comment 5

[Honza Bambas (:mayhemer)]

The cert error page buttons are implemented in browser.js, which is AFAIK not updated to work under multiprocess Firefox.  After it is fixed, I will re-check the buttons work or not (I expect this could 'just work').

Is there any bug for this?  Would be great to add a dependency.

Bottom line: postpone this bug.

Comment 6

[Jason Duell]

Comment on attachment 450755 [details] [diff] [review]
v1

># HG changeset patch
># Parent d206936bf7e991caef14ae64755540ddc5bf3a94
>
>diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp
>--- a/netwerk/protocol/http/HttpChannelParent.cpp
>+++ b/netwerk/protocol/http/HttpChannelParent.cpp
>@@ -43,20 +43,21 @@
> #include "nsHttpChannel.h"
> #include "nsHttpHandler.h"
> #include "nsNetUtil.h"
> #include "nsISupportsPriority.h"
> #include "nsIAuthPromptProvider.h"
> #include "nsIDocShellTreeItem.h"
> #include "nsIBadCertListener2.h"
> #include "nsSerializationHelper.h"
> #include "nsISerializable.h"
> #include "nsIAssociatedContentSecurity.h"
>+#include "nsISecureBrowserUI.h"
> 
> namespace mozilla {
> namespace net {
> 
> // C++ file contents
> HttpChannelParent::HttpChannelParent(PIFrameEmbeddingParent* iframeEmbedding)
> : mIPCClosed(false)
> {
>   // Ensure gHttpHandler is initialized: we need the atom table up and running.
>   nsIHttpProtocolHandler* handler;
>@@ -305,20 +306,27 @@ HttpChannelParent::GetInterface(const ns
>       aIID.Equals(NS_GET_IID(nsIApplicationCacheContainer)) ||
>       aIID.Equals(NS_GET_IID(nsIProgressEventSink)) ||
>       // FIXME:  bug 561830: when fixed, we shouldn't be asked for this interface
>       aIID.Equals(NS_GET_IID(nsIDocShellTreeItem)) ||
>       // Let this return NS_ERROR_NO_INTERFACE: it's OK to not provide it.
>       aIID.Equals(NS_GET_IID(nsIBadCertListener2))) 
>   {
>     return QueryInterface(aIID, result);
>   } 
> 
>+  if (aIID.Equals(NS_GET_IID(nsISecureBrowserUI))) {
>+    if (!mTabParent)
>+      return NS_NOINTERFACE;
>+
>+    return mTabParent->QueryInterface(aIID, result);
>+  }
>+
>   // Interface we haven't dealt with yet. Make sure we know by dying.
>   // - use "grep -ri [uuid] ROOT_SRC_DIR" with the uuid from the printf to
>   //   find the offending interface.
>   // - FIXME: make non-fatal before we ship
>   printf("*&*&*& HttpChannelParent::GetInterface: uuid=%s not impl'd yet! "
>          "File a bug!\n", 
>          aIID.ToString());
>   DROP_DEAD();
> }
> 
>diff --git a/security/manager/ssl/src/nsNSSIOLayer.cpp b/security/manager/ssl/src/nsNSSIOLayer.cpp
>--- a/security/manager/ssl/src/nsNSSIOLayer.cpp
>+++ b/security/manager/ssl/src/nsNSSIOLayer.cpp
>@@ -371,68 +371,75 @@ nsNSSSocketInfo::EnsureDocShellDependent
> 
>   // Are we running within a context that wants external SSL error reporting?
>   // We'll look at the presence of a security UI object inside docshell.
>   // If the docshell wants the lock icon, you'll get the ssl error pages, too.
>   // This is helpful to distinguish from all other contexts, like mail windows,
>   // or any other SSL connections running in the background.
>   // We must query it now and remember, because fatal SSL errors will come 
>   // with a socket close, and the socket transport might detach the callbacks 
>   // instance prior to our error reporting.
> 
>-  nsCOMPtr<nsIDocShell> docshell;
>-
>-  nsCOMPtr<nsIDocShellTreeItem> item(do_GetInterface(proxiedCallbacks));
>-  if (item)
>+  nsISecureBrowserUI* secureUI = nsnull;
>+#ifdef MOZ_IPC
>+  CallGetInterface(proxiedCallbacks.get(), &secureUI);
>+#endif
>+
>+  if (!secureUI) {
>+    nsCOMPtr<nsIDocShell> docshell;
>+
>+    nsCOMPtr<nsIDocShellTreeItem> item(do_GetInterface(proxiedCallbacks));
>+    if (item)
>+    {
>+      nsCOMPtr<nsIDocShellTreeItem> proxiedItem;
>+      nsCOMPtr<nsIDocShellTreeItem> rootItem;
>+      NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>+                           NS_GET_IID(nsIDocShellTreeItem),
>+                           item.get(),
>+                           NS_PROXY_SYNC,
>+                           getter_AddRefs(proxiedItem));
>+
>+      proxiedItem->GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
>+      docshell = do_QueryInterface(rootItem);
>+      NS_ASSERTION(docshell, "rootItem do_QI is null");
>+    }
>+
>+    if (docshell)
>+    {
>+      nsCOMPtr<nsIDocShell> proxiedDocShell;
>+      NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>+                           NS_GET_IID(nsIDocShell),
>+                           docshell.get(),
>+                           NS_PROXY_SYNC,
>+                           getter_AddRefs(proxiedDocShell));
>+      if (proxiedDocShell)
>+        proxiedDocShell->GetSecurityUI(&secureUI);
>+    }
>+  }
>+
>+  if (secureUI)
>   {
>-    nsCOMPtr<nsIDocShellTreeItem> proxiedItem;
>-    nsCOMPtr<nsIDocShellTreeItem> rootItem;
>-    NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>-                         NS_GET_IID(nsIDocShellTreeItem),
>-                         item.get(),
>-                         NS_PROXY_SYNC,
>-                         getter_AddRefs(proxiedItem));
>-
>-    proxiedItem->GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
>-    docshell = do_QueryInterface(rootItem);
>-    NS_ASSERTION(docshell, "rootItem do_QI is null");
>-  }
>-
>-  if (docshell)
>-  {
>-    nsCOMPtr<nsIDocShell> proxiedDocShell;
>-    NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>-                         NS_GET_IID(nsIDocShell),
>-                         docshell.get(),
>-                         NS_PROXY_SYNC,
>-                         getter_AddRefs(proxiedDocShell));
>-    nsISecureBrowserUI* secureUI = nsnull;
>-    if (proxiedDocShell)
>-      proxiedDocShell->GetSecurityUI(&secureUI);
>-    if (secureUI)
>-    {
>-      nsCOMPtr<nsIThread> mainThread(do_GetMainThread());
>-      NS_ProxyRelease(mainThread, secureUI, PR_FALSE);
>-      mExternalErrorReporting = PR_TRUE;
>-
>-      // If this socket is associated to a docshell, let's try to remember
>-      // the currently used cert. If this socket gets a notification from NSS
>-      // having the same raw socket, we can keep the PSM wrapper object
>-      // and all the data it has cached (like verification results).
>-      nsCOMPtr<nsISSLStatusProvider> statprov = do_QueryInterface(secureUI);
>-      if (statprov) {
>-        nsCOMPtr<nsISupports> isup_stat;
>-        statprov->GetSSLStatus(getter_AddRefs(isup_stat));
>-        if (isup_stat) {
>-          nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(isup_stat);
>-          if (sslstat) {
>-            sslstat->GetServerCert(getter_AddRefs(mPreviousCert));
>-          }
>+    nsCOMPtr<nsIThread> mainThread(do_GetMainThread());
>+    NS_ProxyRelease(mainThread, secureUI, PR_FALSE);
>+    mExternalErrorReporting = PR_TRUE;
>+
>+    // If this socket is associated to a docshell, let's try to remember
>+    // the currently used cert. If this socket gets a notification from NSS
>+    // having the same raw socket, we can keep the PSM wrapper object
>+    // and all the data it has cached (like verification results).
>+    nsCOMPtr<nsISSLStatusProvider> statprov = do_QueryInterface(secureUI);
>+    if (statprov) {
>+      nsCOMPtr<nsISupports> isup_stat;
>+      statprov->GetSSLStatus(getter_AddRefs(isup_stat));
>+      if (isup_stat) {
>+        nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(isup_stat);
>+        if (sslstat) {
>+          sslstat->GetServerCert(getter_AddRefs(mPreviousCert));
>         }
>       }
>     }
>   }
> 
>   return NS_OK;
> }
> 
> nsresult
> nsNSSSocketInfo::GetExternalErrorReporting(PRBool* state)

Comment 7

[Honza Bambas (:mayhemer)]

Any explanations why canceled the review..?

Comment 8

[Jason Duell]

Comment on attachment 450755 [details] [diff] [review]
v1

> Bottom line: postpone this bug.

Honza:  sorry, I saw the above comment and thought you would be coming up with a new patch. 

Also pardon my total bugzilla fail (posting the entire patch as comment 6 for no reason :)

Comment 9

[u367160]

seems this patch a little bit outdated and not working

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to comment #9)
> not working

What exactly means not working?  How are you testing it, what you expect, what it does?

Comment 11

[u367160]

sorry for not have been precise, I set my system date to one year old and try to open a secure page, expecting certificate error page displayed with add exception button. The actual result was pop up "Secure connection failed" dialogue with view certificate and cancel button.

Comment 12

[Honza Bambas (:mayhemer)]

Ekrem, you were using Fennec with this patch applied?

Comment 13

[u367160]

basically yes, just moved the changes for netwerk/protocol/http/HttpChannelParent.cpp to netwerk/protocol/http/HttpChannelParentListener.cpp in the same function because it was not applying

Comment 14

[Honza Bambas (:mayhemer)]

Fennec currently has some problems with the security UI and ssl state.  See also bug 575950.  It might be related.  We probably cannot fix this bug w/o that one.

Comment 15

[Pradyumna Singh]

(In reply to comment #13)
> basically yes, just moved the changes for
> netwerk/protocol/http/HttpChannelParent.cpp to
> netwerk/protocol/http/HttpChannelParentListener.cpp in the same function
> because it was not applying

After making these changes to the patch, and applying patch to latest upstream code,the https page having the untrusted certificate could not render for me.

Comment 16

[Honza Bambas (:mayhemer)]

Fennec has a different way of showing a certificate error - the cert error dialog.  This is no longer a valid bug.

Comment 17

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

Turns out, this might be exactly what Fennec needs to fix bug 566478

Comment 18

[Jason Duell]

bug 566478 has been fixed: can we re-CLOSE this?

Comment 19

[Honza Bambas (:mayhemer)]

Yes, it is now a duplicate.