bug 1497555 - filter out same-document location changes in nsSecureBrowserUIImpl::OnLocationChange r=Ehsan
authorDana Keeler <dkeeler@mozilla.com>
Wed, 17 Oct 2018 21:38:24 +0000
changeset 490183 a87fc8375ec28730643ba38f36a253e0ab5942c8
parent 490182 2b7afed367792d6158d2f1ac18e614091cd9b6af
child 490184 34f52a304c5251541685810ab2fb2e0a508d7508
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersEhsan
bugs1497555
milestone64.0a1
bug 1497555 - filter out same-document location changes in nsSecureBrowserUIImpl::OnLocationChange r=Ehsan If nsSecureBrowserUIImpl::OnLocationChange receives a LOCATION_CHANGE_SAME_DOCUMENT notification, it doesn't need to (and in fact shouldn't) update its security state or notify downstream listeners. Differential Revision: https://phabricator.services.mozilla.com/D8900
browser/base/content/test/siteIdentity/browser.ini
browser/base/content/test/siteIdentity/browser_ignore_same_page_navigation.js
security/manager/ssl/nsSecureBrowserUIImpl.cpp
--- a/browser/base/content/test/siteIdentity/browser.ini
+++ b/browser/base/content/test/siteIdentity/browser.ini
@@ -104,8 +104,9 @@ 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_navigation_failures.js]
 [browser_secure_transport_insecure_scheme.js]
+[browser_ignore_same_page_navigation.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/siteIdentity/browser_ignore_same_page_navigation.js
@@ -0,0 +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/ */
+
+// Test that the nsISecureBrowserUI implementation doesn't send extraneous OnSecurityChange events
+// when it receives OnLocationChange events with the LOCATION_CHANGE_SAME_DOCUMENT flag set.
+
+add_task(async function() {
+  await BrowserTestUtils.withNewTab("about:blank", async (browser) => {
+    let onLocationChangeCount = 0;
+    let onSecurityChangeCount = 0;
+    let progressListener = {
+      onStateChange() {},
+      onLocationChange() {
+        onLocationChangeCount++;
+      },
+      onSecurityChange() {
+        onSecurityChangeCount++;
+      },
+      onProgressChange() {},
+      onStatusChange() {},
+
+      QueryInterface: ChromeUtils.generateQI([Ci.nsIWebProgressListener,
+                                              Ci.nsISupportsWeakReference]),
+    };
+    browser.addProgressListener(progressListener, Ci.nsIWebProgress.NOTIFY_ALL);
+
+    let uri = getRootDirectory(gTestPath).replace("chrome://mochitests/content",
+                                                  "https://example.com") + "dummy_page.html";
+    BrowserTestUtils.loadURI(browser, uri);
+    await BrowserTestUtils.browserLoaded(browser, false, uri);
+    is(onLocationChangeCount, 1, "should have 1 onLocationChange event");
+    is(onSecurityChangeCount, 1, "should have 1 onSecurityChange event");
+    await ContentTask.spawn(browser, null, async () => {
+      content.history.pushState({}, "", "https://example.com");
+    });
+    is(onLocationChangeCount, 2, "should have 2 onLocationChange events");
+    is(onSecurityChangeCount, 1, "should still have only 1 onSecurityChange event");
+  });
+});
--- a/security/manager/ssl/nsSecureBrowserUIImpl.cpp
+++ b/security/manager/ssl/nsSecureBrowserUIImpl.cpp
@@ -391,23 +391,26 @@ nsSecureBrowserUIImpl::OnLocationChange(
   // get OnSecurityChange events from this implementation. Instead, we check to
   // see if the web progress we were handed here is the same one as we were
   // initialized with.
   nsCOMPtr<nsIWebProgress> originalWebProgress = do_QueryReferent(mWebProgress);
   if (aWebProgress != originalWebProgress) {
     return NS_OK;
   }
 
-  // Clear any state that varies by location.
-  if (!(aFlags & LOCATION_CHANGE_SAME_DOCUMENT)) {
-    mOldState = 0;
-    mState = 0;
-    mTopLevelSecurityInfo = nullptr;
+  // If this is a same-document location change, we don't need to update our
+  // state or notify anyone.
+  if (aFlags & LOCATION_CHANGE_SAME_DOCUMENT) {
+    return NS_OK;
   }
 
+  mOldState = 0;
+  mState = 0;
+  mTopLevelSecurityInfo = nullptr;
+
   if (aFlags & LOCATION_CHANGE_ERROR_PAGE) {
     mState = STATE_IS_INSECURE;
     mTopLevelSecurityInfo = nullptr;
   } else {
     // NB: aRequest may be null. It may also not be QI-able to nsIChannel.
     nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
     if (channel) {
       MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,