From 6016550e26386c6d5323aeed9a8c0e02b836d068 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Tue, 6 Aug 2019 19:09:28 +0000 Subject: [PATCH] Bug 1569146 - a page redirecting to itself should appear in history. r=Standard8 The history anti-flooding system prevents repeated loads of the same url from flooding history, generating bogus frecency values. In some cases though, like a page redirecting to itself, it may be overzealous. A redirect source is always marked as hidden, and if the page redirects to itself the second visit will be skipped, because repeated. We always want to haveat least one unhidden visit in that case, thus we can skip the anti-flooding check if the page will be unhidden. Differential Revision: https://phabricator.services.mozilla.com/D40426 --HG-- extra : moz-landing-system : lando --- toolkit/components/places/History.cpp | 38 ++++++++------ toolkit/components/places/History.h | 10 ++-- .../places/tests/browser/browser.ini | 50 +++++++++++++++++++ .../tests/browser/browser_redirect_self.js | 46 +++++++++++++++++ .../places/tests/browser/redirect_self.sjs | 27 ++++++++++ toolkit/components/places/tests/moz.build | 6 ++- 6 files changed, 155 insertions(+), 22 deletions(-) create mode 100644 toolkit/components/places/tests/browser/browser_redirect_self.js create mode 100644 toolkit/components/places/tests/browser/redirect_self.sjs diff --git a/toolkit/components/places/History.cpp b/toolkit/components/places/History.cpp index bf184d6ed16f..51af113afcc9 100644 --- a/toolkit/components/places/History.cpp +++ b/toolkit/components/places/History.cpp @@ -529,7 +529,7 @@ class NotifyManyVisitsObservers : public Runnable { } if (aNow - aPlace.visitTime < RECENTLY_VISITED_URIS_MAX_AGE) { - mHistory->AppendToRecentlyVisitedURIs(aURI); + mHistory->AppendToRecentlyVisitedURIs(aURI, aPlace.hidden); } mHistory->NotifyVisited(aURI); @@ -1978,7 +1978,7 @@ void History::Shutdown() { } } -void History::AppendToRecentlyVisitedURIs(nsIURI* aURI) { +void History::AppendToRecentlyVisitedURIs(nsIURI* aURI, bool aHidden) { // Add a new entry, if necessary. RecentURIKey* entry = mRecentlyVisitedURIs.GetEntry(aURI); if (!entry) { @@ -1986,6 +1986,7 @@ void History::AppendToRecentlyVisitedURIs(nsIURI* aURI) { } if (entry) { entry->time = PR_Now(); + entry->hidden = aHidden; } // Remove entries older than RECENTLY_VISITED_URIS_MAX_AGE. @@ -1997,13 +1998,6 @@ void History::AppendToRecentlyVisitedURIs(nsIURI* aURI) { } } -inline bool History::IsRecentlyVisitedURI(nsIURI* aURI) { - RecentURIKey* entry = mRecentlyVisitedURIs.GetEntry(aURI); - // Check if the entry exists and is younger than - // RECENTLY_VISITED_URIS_MAX_AGE. - return entry && (PR_Now() - entry->time) < RECENTLY_VISITED_URIS_MAX_AGE; -} - //////////////////////////////////////////////////////////////////////////////// //// IHistory @@ -2042,16 +2036,10 @@ History::VisitURI(nsIWidget* aWidget, nsIURI* aURI, nsIURI* aLastVisitedURI, return NS_OK; } - // Do not save a reloaded uri if we have visited the same URI recently. bool reload = false; if (aLastVisitedURI) { rv = aURI->Equals(aLastVisitedURI, &reload); NS_ENSURE_SUCCESS(rv, rv); - if (reload && IsRecentlyVisitedURI(aURI)) { - // Regardless we must update the stored visit time. - AppendToRecentlyVisitedURIs(aURI); - return NS_OK; - } } nsTArray placeArray(1); @@ -2110,6 +2098,26 @@ History::VisitURI(nsIWidget* aWidget, nsIURI* aURI, nsIURI* aLastVisitedURI, place.shouldUpdateFrecency = false; } + // Do not save a reloaded uri if we have visited the same URI recently. + if (reload) { + RecentURIKey* entry = mRecentlyVisitedURIs.GetEntry(aURI); + // Check if the entry exists and is younger than + // RECENTLY_VISITED_URIS_MAX_AGE. + if (entry && (PR_Now() - entry->time) < RECENTLY_VISITED_URIS_MAX_AGE) { + bool wasHidden = entry->hidden; + // Regardless of whether we store the visit or not, we must update the + // stored visit time. + AppendToRecentlyVisitedURIs(aURI, place.hidden); + // We always want to store an unhidden visit, if the previous visits were + // hidden, because otherwise the page may not appear in the history UI. + // This can happen for example at a page redirecting to itself. + if (!wasHidden || place.hidden) { + // We can skip this visit. + return NS_OK; + } + } + } + // EMBED visits are session-persistent and should not go through the database. // They exist only to keep track of isVisited status during the session. if (place.transitionType == nsINavHistoryService::TRANSITION_EMBED) { diff --git a/toolkit/components/places/History.h b/toolkit/components/places/History.h index 0e19755d2613..01bf25bb3d48 100644 --- a/toolkit/components/places/History.h +++ b/toolkit/components/places/History.h @@ -138,8 +138,10 @@ class History final : public IHistory, /** * Helper function to append a new URI to mRecentlyVisitedURIs. See * mRecentlyVisitedURIs. + * @param {nsIURI} aURI The URI to append + * @param {bool} aHidden The hidden status of the visit being appended. */ - void AppendToRecentlyVisitedURIs(nsIURI* aURI); + void AppendToRecentlyVisitedURIs(nsIURI* aURI, bool aHidden); void NotifyVisitedParent(const nsTArray& aURIs); @@ -230,13 +232,9 @@ class History final : public IHistory, MOZ_ASSERT_UNREACHABLE("Do not call me!"); } MOZ_INIT_OUTSIDE_CTOR PRTime time; + MOZ_INIT_OUTSIDE_CTOR bool hidden; }; nsTHashtable mRecentlyVisitedURIs; - /** - * Whether aURI has been visited "recently". - * See RECENTLY_VISITED_URIS_MAX_AGE. - */ - bool IsRecentlyVisitedURI(nsIURI* aURI); }; } // namespace places diff --git a/toolkit/components/places/tests/browser/browser.ini b/toolkit/components/places/tests/browser/browser.ini index c5a21fcd5ca2..63e247264c6a 100644 --- a/toolkit/components/places/tests/browser/browser.ini +++ b/toolkit/components/places/tests/browser/browser.ini @@ -3,22 +3,72 @@ support-files = head.js [browser_bug399606.js] +support-files = + 399606-history.go-0.html + 399606-httprefresh.html + 399606-location.reload.html + 399606-location.replace.html + 399606-window.location.html + 399606-window.location.href.html [browser_bug461710.js] +support-files = + 461710_iframe.html + 461710_link_page-2.html + 461710_link_page-3.html + 461710_link_page.html + 461710_visited_page.html [browser_bug646422.js] [browser_bug680727.js] skip-if = fission || verify [browser_double_redirect.js] +support-files = + begin.html + final.html + redirect_once.sjs + redirect_twice.sjs [browser_favicon_privatebrowsing_perwindowpb.js] [browser_history_post.js] +support-files = + history_post.html + history_post.sjs [browser_notfound.js] [browser_onvisit_title_null_for_navigation.js] skip-if = verify support-files = empty_page.html [browser_redirect.js] +support-files = + redirect.sjs + redirect-target.html +[browser_redirect_self.js] +support-files = + redirect_self.sjs [browser_multi_redirect_frecency.js] +support-files = + final.html + redirect_once.sjs + redirect_thrice.sjs + redirect_twice.sjs + redirect_twice_perma.sjs [browser_settitle.js] +support-files = + title1.html + title2.html [browser_visited_notfound.js] [browser_visituri.js] +support-files = + begin.html + final.html + redirect_once.sjs + redirect_twice.sjs [browser_visituri_nohistory.js] +support-files = + begin.html + final.html + favicon-normal16.png + favicon-normal32.png [browser_visituri_privatebrowsing_perwindowpb.js] +support-files = + begin.html + favicon.html + final.html diff --git a/toolkit/components/places/tests/browser/browser_redirect_self.js b/toolkit/components/places/tests/browser/browser_redirect_self.js new file mode 100644 index 000000000000..86e54dde14ee --- /dev/null +++ b/toolkit/components/places/tests/browser/browser_redirect_self.js @@ -0,0 +1,46 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/* + * Tests a page that redirects to itself. On the initial visit the page should + * be marked as hidden, but then the second visit should unhide it. + * This ensures that that the history anti-flooding system doesn't skip the + * second visit. + */ + +add_task(async function() { + await PlacesUtils.history.clear(); + const url = + "http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_self.sjs"; + let visitCount = 0; + function onVisitsListener(events) { + visitCount++; + Assert.equal(events.length, 1, "Right number of visits notified"); + Assert.equal(events[0].url, url, "Got a visit for the expected url"); + if (visitCount == 1) { + Assert.ok(events[0].hidden, "The visit should be hidden"); + } else { + Assert.ok(!events[0].hidden, "The visit should not be hidden"); + } + } + PlacesObservers.addListener(["page-visited"], onVisitsListener); + registerCleanupFunction(async function() { + PlacesObservers.removeListener(["page-visited"], onVisitsListener); + await PlacesUtils.history.clear(); + }); + await BrowserTestUtils.withNewTab( + { + gBrowser, + url, + }, + async browser => { + await TestUtils.waitForCondition(() => visitCount == 2); + // Check that the visit is not hidden in the database. + Assert.ok( + !(await PlacesTestUtils.fieldInDB(url, "hidden")), + "The url should not be hidden in the database" + ); + } + ); +}); diff --git a/toolkit/components/places/tests/browser/redirect_self.sjs b/toolkit/components/places/tests/browser/redirect_self.sjs new file mode 100644 index 000000000000..953afe5f26c8 --- /dev/null +++ b/toolkit/components/places/tests/browser/redirect_self.sjs @@ -0,0 +1,27 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +// Script that redirects to itself once. + +function handleRequest(request, response) { + if ( + request.hasHeader("Cookie") && + request.getHeader("Cookie").includes("redirect-self") + ) { + response.setStatusLine("1.0", 200, "OK"); + // Expire the cookie. + response.setHeader( + "Set-Cookie", + "redirect-self=true; expires=Thu, 01 Jan 1970 00:00:00 GMT", + true + ); + response.setHeader("Content-Type", "text/plain; charset=utf-8", false); + response.write("OK"); + } else { + response.setStatusLine(request.httpVersion, 302, "Moved Temporarily"); + response.setHeader("Set-Cookie", "redirect-self=true", true); + response.setHeader("Location", "redirect_self.sjs"); + response.write("Moved Temporarily"); + } +} diff --git a/toolkit/components/places/tests/moz.build b/toolkit/components/places/tests/moz.build index 69858e3fd7e3..c4e28a7667f5 100644 --- a/toolkit/components/places/tests/moz.build +++ b/toolkit/components/places/tests/moz.build @@ -24,7 +24,10 @@ XPCSHELL_TESTS_MANIFESTS += [ 'unit/xpcshell.ini', ] -BROWSER_CHROME_MANIFESTS += ['browser/browser.ini'] +BROWSER_CHROME_MANIFESTS += [ + 'browser/browser.ini' +] + MOCHITEST_CHROME_MANIFESTS += [ 'chrome/chrome.ini', ] @@ -55,6 +58,7 @@ TEST_HARNESS_FILES.testing.mochitest.tests.toolkit.components.places.tests.brows 'browser/redirect-target.html', 'browser/redirect.sjs', 'browser/redirect_once.sjs', + 'browser/redirect_self.sjs', 'browser/redirect_thrice.sjs', 'browser/redirect_twice.sjs', 'browser/redirect_twice_perma.sjs',