bug 1493427 - use the docShell to call OnSecurityChange in nsSecureBrowserUIImpl::OnLocationChange r=Gijs
authorDana Keeler <dkeeler@mozilla.com>
Tue, 02 Oct 2018 20:26:40 +0000
changeset 495025 84b4b9b7586a12ecf26f1d8eeee5a7bdeed9468b
parent 495024 0f5faf5435a815f27817a2944e5d0e83dea0c685
child 495026 f2737c4b2abebdc89bd3eb464ef1906df6234eb8
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1493427
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1493427 - use the docShell to call OnSecurityChange in nsSecureBrowserUIImpl::OnLocationChange r=Gijs When navigating to an about: page that doesn't exist (e.g. "about:somethingthatdoesnotexist"), the docShell will call nsSecureBrowserUIImpl::OnLocationChange with a request that is null. Consequently, we can't use that to QueryInterface to a nsISecurityEventSink to call OnSecurityChange. The previous implementation would use the prior request's nsISecurityEventSink, which was a bug but luckily this produced the correct behavior. Since the original docShell the nsSecureBrowserUIImpl was initialized with is what needs to be notified, we can just QueryInterface that to an nsISecurityEventSink and call OnSecurityChange directly instead. Differential Revision: https://phabricator.services.mozilla.com/D6951
browser/base/content/test/siteIdentity/browser.ini
browser/base/content/test/siteIdentity/browser_navigation_failures.js
browser/base/content/test/siteIdentity/browser_tls_handshake_failure.js
security/manager/ssl/nsSecureBrowserUIImpl.cpp
--- a/browser/base/content/test/siteIdentity/browser.ini
+++ b/browser/base/content/test/siteIdentity/browser.ini
@@ -103,9 +103,9 @@ support-files =
 [browser_no_mcb_for_onions.js]
 tags = mcb
 support-files =
   test_no_mcb_for_onions.html
 [browser_check_identity_state.js]
 [browser_iframe_navigation.js]
 support-files =
   iframe_navigation.html
-[browser_tls_handshake_failure.js]
+[browser_navigation_failures.js]
rename from browser/base/content/test/siteIdentity/browser_tls_handshake_failure.js
rename to browser/base/content/test/siteIdentity/browser_navigation_failures.js
--- a/browser/base/content/test/siteIdentity/browser_tls_handshake_failure.js
+++ b/browser/base/content/test/siteIdentity/browser_navigation_failures.js
@@ -1,25 +1,41 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
-// Tests that the site identity indicator is properly updated for connections
-// where the TLS handshake fails.
-// See bug 1492424.
+// Tests that the site identity indicator is properly updated for navigations
+// that fail for various reasons. In particular, we currently test TLS handshake
+// failures and about: pages that don't actually exist.
+// See bug 1492424 and bug 1493427.
 
+const kSecureURI = getRootDirectory(gTestPath).replace("chrome://mochitests/content",
+                                                       "https://example.com") + "dummy_page.html";
 add_task(async function() {
-  let rootURI = getRootDirectory(gTestPath).replace("chrome://mochitests/content",
-                                                    "https://example.com");
-  await BrowserTestUtils.withNewTab(rootURI + "dummy_page.html", async (browser) => {
+  await BrowserTestUtils.withNewTab(kSecureURI, async (browser) => {
     let identityMode = window.document.getElementById("identity-box").className;
     is(identityMode, "verifiedDomain", "identity should be secure before");
 
     const TLS_HANDSHAKE_FAILURE_URI = "https://ssl3.example.com/";
     // Try to connect to a server where the TLS handshake will fail.
     BrowserTestUtils.loadURI(browser, TLS_HANDSHAKE_FAILURE_URI);
     await BrowserTestUtils.browserLoaded(browser, false, TLS_HANDSHAKE_FAILURE_URI, true);
 
     let newIdentityMode = window.document.getElementById("identity-box").className;
     is(newIdentityMode, "unknownIdentity", "identity should be unknown (not secure) after");
   });
 });
+
+add_task(async function() {
+  await BrowserTestUtils.withNewTab(kSecureURI, async (browser) => {
+    let identityMode = window.document.getElementById("identity-box").className;
+    is(identityMode, "verifiedDomain", "identity should be secure before");
+
+    const BAD_ABOUT_PAGE_URI = "about:somethingthatdoesnotexist";
+    // Try to load an about: page that doesn't exist
+    BrowserTestUtils.loadURI(browser, BAD_ABOUT_PAGE_URI);
+    await BrowserTestUtils.browserLoaded(browser, false, BAD_ABOUT_PAGE_URI, true);
+
+    let newIdentityMode = window.document.getElementById("identity-box").className;
+    is(newIdentityMode, "unknownIdentity", "identity should be unknown (not secure) after");
+  });
+});
--- a/security/manager/ssl/nsSecureBrowserUIImpl.cpp
+++ b/security/manager/ssl/nsSecureBrowserUIImpl.cpp
@@ -283,17 +283,18 @@ nsSecureBrowserUIImpl::UpdateStateAndSec
 // window or whatever corresponds to an <iframe mozbrowser> element). In some
 // cases, we also receive it from nsIWebProgress instances that are children of
 // that nsIWebProgress. We ignore notifications from children because they don't
 // change the top-level state (if children load mixed or tracking content, the
 // docShell will know and will tell us in GetState when we call
 // CheckForBlockedContent).
 // When we receive a notification from the top-level nsIWebProgress, we extract
 // any relevant security information and set our state accordingly. We then call
-// OnSecurityChange to notify any downstream listeners of the security state.
+// OnSecurityChange on the docShell corresponding to the nsIWebProgress we were
+// initialized with to notify any downstream listeners of the security state.
 NS_IMETHODIMP
 nsSecureBrowserUIImpl::OnLocationChange(nsIWebProgress* aWebProgress,
                                         nsIRequest* aRequest,
                                         nsIURI* aLocation,
                                         uint32_t aFlags)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -326,34 +327,36 @@ nsSecureBrowserUIImpl::OnLocationChange(
   nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
   if (channel) {
     MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,
             ("  we have a channel %p", channel.get()));
     nsresult rv = UpdateStateAndSecurityInfo(channel, aLocation);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
+  }
 
-    nsCOMPtr<nsISecurityEventSink> eventSink;
-    NS_QueryNotificationCallbacks(channel, eventSink);
-    if (NS_WARN_IF(!eventSink)) {
-      return NS_ERROR_INVALID_ARG;
+  mozilla::dom::ContentBlockingLog* contentBlockingLog = nullptr;
+  nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocShell);
+  if (docShell) {
+    nsIDocument* doc = docShell->GetDocument();
+    if (doc) {
+      contentBlockingLog = doc->GetContentBlockingLog();
     }
-    mozilla::dom::ContentBlockingLog* contentBlockingLog = nullptr;
-    nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocShell);
-    if (docShell) {
-      nsIDocument* doc = docShell->GetDocument();
-      if (doc) {
-        contentBlockingLog = doc->GetContentBlockingLog();
-      }
-    }
+  }
+
+  nsCOMPtr<nsISecurityEventSink> eventSink = do_QueryInterface(docShell);
+  if (eventSink) {
     MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,
-            ("  calling OnSecurityChange %p %x\n", aRequest, mState));
+            ("  calling OnSecurityChange %p %x", aRequest, mState));
     Unused << eventSink->OnSecurityChange(aRequest, mOldState, mState,
                                           contentBlockingLog);
+  } else {
+    MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,
+            ("  no docShell or couldn't QI it to nsISecurityEventSink?"));
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsSecureBrowserUIImpl::OnStateChange(nsIWebProgress*,
                                      nsIRequest*,