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 500291 a87fc8375ec28730643ba38f36a253e0ab5942c8
parent 500290 2b7afed367792d6158d2f1ac18e614091cd9b6af
child 500292 34f52a304c5251541685810ab2fb2e0a508d7508
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersEhsan
bugs1497555
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 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,