Honza Bambas (:mayhemer) Assignee
	Description • 14 years ago

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 ?)

	Honza Bambas (:mayhemer) Assignee
	Comment 1 • 14 years ago

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.

	Honza Bambas (:mayhemer) Assignee
	Comment 2 • 14 years ago

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

	Honza Bambas (:mayhemer) Assignee
	Comment 3 • 14 years ago

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).

	Honza Bambas (:mayhemer) Assignee
	Comment 4 • 14 years ago

(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

	Honza Bambas (:mayhemer) Assignee
	Comment 5 • 14 years ago

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.

	Jason Duell
	Comment 6 • 14 years ago

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)

	Honza Bambas (:mayhemer) Assignee
	Comment 7 • 14 years ago

Any explanations why canceled the review..?

	Jason Duell
	Comment 8 • 14 years ago

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 :)

	u367160
	Comment 9 • 14 years ago

seems this patch a little bit outdated and not working

	Honza Bambas (:mayhemer) Assignee
	Comment 10 • 14 years ago

(In reply to comment #9)
> not working

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

	u367160
	Comment 11 • 14 years ago

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.

	Honza Bambas (:mayhemer) Assignee
	Comment 12 • 14 years ago

Ekrem, you were using Fennec with this patch applied?

	u367160
	Comment 13 • 14 years ago

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

	Honza Bambas (:mayhemer) Assignee
	Comment 14 • 14 years ago

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.

	Pradyumna Singh
	Comment 15 • 14 years ago

(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.

	Honza Bambas (:mayhemer) Assignee
	Comment 16 • 14 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 17 • 14 years ago

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

	Jason Duell
	Comment 18 • 14 years ago

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

	Honza Bambas (:mayhemer) Assignee
	Comment 19 • 14 years ago

Yes, it is now a duplicate.