# HG changeset patch # User Marco Bonardo # Date 1511459047 -3600 # Node ID c070c95edef7a728efc5ee9d852988762038bfed # Parent 5230e12b2ccd1181da356e71e5995a79f204ad9c Bug 1419825 - Callers of insertVisitedURIs may overwrite the history title passing a null title. r=kitcambridge MozReview-Commit-ID: EPU4mv8rn7h diff --git a/toolkit/components/places/History.cpp b/toolkit/components/places/History.cpp --- a/toolkit/components/places/History.cpp +++ b/toolkit/components/places/History.cpp @@ -1041,17 +1041,19 @@ public: lastFetchedPlace = &mPlaces.ElementAt(i); lastFetchedVisitCount = lastFetchedPlace->visitCount; } else { // Copy over the data from the already known place. place.placeId = lastFetchedPlace->placeId; place.guid = lastFetchedPlace->guid; place.lastVisitId = lastFetchedPlace->visitId; place.lastVisitTime = lastFetchedPlace->visitTime; - place.titleChanged = !lastFetchedPlace->title.Equals(place.title); + if (!place.title.IsVoid()) { + place.titleChanged = !lastFetchedPlace->title.Equals(place.title); + } place.frecency = lastFetchedPlace->frecency; // Add one visit for the previous loop. place.visitCount = ++lastFetchedVisitCount; } // If any transition is typed, ensure the page is marked as typed. if (typed != lastFetchedPlace->typed) { place.typed = true; @@ -2272,37 +2274,51 @@ History::InsertPlace(VisitData& aPlace, nsresult History::UpdatePlace(const VisitData& aPlace) { MOZ_ASSERT(!NS_IsMainThread(), "must be called off of the main thread!"); MOZ_ASSERT(aPlace.placeId > 0, "must have a valid place id!"); MOZ_ASSERT(!aPlace.guid.IsVoid(), "must have a guid!"); - nsCOMPtr stmt = GetStatement( + nsCOMPtr stmt; + bool titleIsVoid = aPlace.title.IsVoid(); + if (titleIsVoid) { + // Don't change the title. + stmt = GetStatement( + "UPDATE moz_places " + "SET hidden = :hidden, " + "typed = :typed, " + "guid = :guid " + "WHERE id = :page_id " + ); + } else { + stmt = GetStatement( "UPDATE moz_places " "SET title = :title, " "hidden = :hidden, " "typed = :typed, " "guid = :guid " "WHERE id = :page_id " ); + } NS_ENSURE_STATE(stmt); mozStorageStatementScoper scoper(stmt); nsresult rv; - // Empty strings should clear the title, just like nsNavHistory::SetPageTitle. - if (aPlace.title.IsEmpty()) { - rv = stmt->BindNullByName(NS_LITERAL_CSTRING("title")); + if (!titleIsVoid) { + // An empty string clears the title. + if (aPlace.title.IsEmpty()) { + rv = stmt->BindNullByName(NS_LITERAL_CSTRING("title")); + } else { + rv = stmt->BindStringByName(NS_LITERAL_CSTRING("title"), + StringHead(aPlace.title, TITLE_LENGTH_MAX)); + } + NS_ENSURE_SUCCESS(rv, rv); } - else { - rv = stmt->BindStringByName(NS_LITERAL_CSTRING("title"), - StringHead(aPlace.title, TITLE_LENGTH_MAX)); - } - NS_ENSURE_SUCCESS(rv, rv); rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"), aPlace.typed); NS_ENSURE_SUCCESS(rv, rv); rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), aPlace.hidden); NS_ENSURE_SUCCESS(rv, rv); rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("guid"), aPlace.guid); NS_ENSURE_SUCCESS(rv, rv); rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), aPlace.placeId); @@ -2384,18 +2400,18 @@ History::FetchPageInfo(VisitData& _place // it to anything. As a result, ignore the fact that we may have changed the // title (because we don't want to, that would be empty), and set the title // to what is currently stored in the datbase. if (_place.title.IsVoid()) { _place.title = title; } // Otherwise, just indicate if the title has changed. else { - _place.titleChanged = !(_place.title.Equals(title) || - (_place.title.IsEmpty() && title.IsVoid())); + _place.titleChanged = !(_place.title.Equals(title)) && + !(_place.title.IsEmpty() && title.IsVoid()); } int32_t hidden; rv = stmt->GetInt32(3, &hidden); NS_ENSURE_SUCCESS(rv, rv); _place.hidden = !!hidden; int32_t typed; @@ -3007,17 +3023,22 @@ History::UpdatePlaces(JS::Handle visit(aCtx); rv = GetJSObjectFromArray(aCtx, visits, j, &visit); NS_ENSURE_SUCCESS(rv, rv); VisitData& data = *visitData.AppendElement(VisitData(uri)); - data.title = title; + if (!title.IsEmpty()) { + data.title = title; + } else if (!title.IsVoid()) { + // Setting data.title to an empty string wouldn't make it non-void. + data.title.SetIsVoid(false); + } data.guid = guid; // We must have a date and a transaction type! rv = GetIntFromJSObject(aCtx, visit, "visitDate", &data.visitTime); NS_ENSURE_SUCCESS(rv, rv); // visitDate should be in microseconds. It's easy to do the wrong thing // and pass milliseconds to updatePlaces, so we lazily check for that. // While it's not easily distinguishable, since both are integers, we can diff --git a/toolkit/components/places/History.jsm b/toolkit/components/places/History.jsm --- a/toolkit/components/places/History.jsm +++ b/toolkit/components/places/History.jsm @@ -1308,16 +1308,17 @@ var remove = async function(db, {guids, * Defaults to an empty object so that this method can be used * to simply convert an updateInfo object into a PageInfo object. * * @return (PageInfo) * A PageInfo object populated with data from updateInfo. */ function mergeUpdateInfoIntoPageInfo(updateInfo, pageInfo = {}) { pageInfo.guid = updateInfo.guid; + pageInfo.title = updateInfo.title; if (!pageInfo.url) { pageInfo.url = new URL(updateInfo.uri.spec); pageInfo.title = updateInfo.title; pageInfo.visits = updateInfo.visits.map(visit => { return { date: PlacesUtils.toDate(visit.visitDate), transition: visit.transitionType, referrer: (visit.referrerURI) ? new URL(visit.referrerURI.spec) : null diff --git a/toolkit/components/places/PlacesSyncUtils.jsm b/toolkit/components/places/PlacesSyncUtils.jsm --- a/toolkit/components/places/PlacesSyncUtils.jsm +++ b/toolkit/components/places/PlacesSyncUtils.jsm @@ -213,17 +213,17 @@ const HistorySyncUtils = PlacesSyncUtils * Fetch information about a guid (url, title and frecency) * * @param guid * @returns {Object} Object with three members: url, title and frecency of the given guid */ async fetchURLInfoForGuid(guid) { let db = await PlacesUtils.promiseDBConnection(); let rows = await db.executeCached(` - SELECT url, title, frecency + SELECT url, IFNULL(title, "") AS title, frecency FROM moz_places WHERE guid = :guid`, { guid } ); if (rows.length === 0) { return null; } return { diff --git a/toolkit/components/places/tests/PlacesTestUtils.jsm b/toolkit/components/places/tests/PlacesTestUtils.jsm --- a/toolkit/components/places/tests/PlacesTestUtils.jsm +++ b/toolkit/components/places/tests/PlacesTestUtils.jsm @@ -46,17 +46,18 @@ var PlacesTestUtils = Object.freeze({ places.push(placeInfo) } else { throw new Error("Unsupported type passed to addVisits"); } // Create a PageInfo for each entry. for (let place of places) { let info = {url: place.uri}; - info.title = (typeof place.title === "string") ? place.title : "test visit for " + info.url.spec ; + let spec = place.uri instanceof Ci.nsIURI ? place.uri.spec : new URL(place.uri).href; + info.title = "title" in place ? place.title : "test visit for " + spec ; if (typeof place.referrer == "string") { place.referrer = NetUtil.newURI(place.referrer); } else if (place.referrer && place.referrer instanceof URL) { place.referrer = NetUtil.newURI(place.referrer.href); } let visitDate = place.visitDate; if (visitDate) { if (visitDate.constructor.name != "Date") { diff --git a/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js --- a/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js +++ b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js @@ -1,140 +1,127 @@ -const REDIRECT_URI = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_thrice.sjs"); -const INTERMEDIATE_URI_1 = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_twice_perma.sjs"); -const INTERMEDIATE_URI_2 = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_once.sjs"); -const TARGET_URI = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/final.html"); +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +const ROOT_URI = "http://mochi.test:8888/tests/toolkit/components/places/tests/browser/"; +const REDIRECT_URI = Services.io.newURI(ROOT_URI + "redirect_thrice.sjs"); +const INTERMEDIATE_URI_1 = Services.io.newURI(ROOT_URI + "redirect_twice_perma.sjs"); +const INTERMEDIATE_URI_2 = Services.io.newURI(ROOT_URI + "redirect_once.sjs"); +const TARGET_URI = Services.io.newURI(ROOT_URI + "final.html"); const REDIRECT_SOURCE_VISIT_BONUS = Services.prefs.getIntPref("places.frecency.redirectSourceVisitBonus"); -// const LINK_VISIT_BONUS = -// Services.prefs.getIntPref("places.frecency.linkVisitBonus"); -// const TYPED_VISIT_BONUS = -// Services.prefs.getIntPref("places.frecency.typedVisitBonus"); const PERM_REDIRECT_VISIT_BONUS = Services.prefs.getIntPref("places.frecency.permRedirectVisitBonus"); // Ensure that decay frecency doesn't kick in during tests (as a result // of idle-daily). Services.prefs.setCharPref("places.frecency.decayRate", "1.0"); registerCleanupFunction(async function() { Services.prefs.clearUserPref("places.frecency.decayRate"); - await PlacesTestUtils.clearHistory(); + await PlacesUtils.history.clear(); }); -function promiseVisitedURIObserver(redirectURI, targetURI, expectedTargetFrecency) { - // Create and add history observer. - return new Promise(resolve => { - let historyObserver = { - _redirectNotified: false, - onVisit(aURI, aVisitID, aTime, aSessionID, aReferringID, - aTransitionType) { - info("Received onVisit: " + aURI.spec); - - if (aURI.equals(redirectURI)) { - this._redirectNotified = true; - // Wait for the target page notification. - return; - } - - if (!aURI.equals(targetURI)) { - return; - } - - PlacesUtils.history.removeObserver(historyObserver); - - ok(this._redirectNotified, "The redirect should have been notified"); - - resolve(); - }, - onBeginUpdateBatch() {}, - onEndUpdateBatch() {}, - onTitleChanged() {}, - onDeleteURI() {}, - onClearHistory() {}, - onPageChanged() {}, - onDeleteVisits() {}, - QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]) - }; - PlacesUtils.history.addObserver(historyObserver); - }); +async function check_uri(uri, frecency, hidden) { + is(await PlacesTestUtils.fieldInDB(uri, "frecency"), frecency, + "Frecency of the page is the expected one"); + is(await PlacesTestUtils.fieldInDB(uri, "hidden"), hidden, + "Hidden value of the page is the expected one"); } -async function testURIFields(url, expectedFrecency, expectedHidden) { - let frecency = await promiseFieldForUrl(url, "frecency"); - is(frecency, expectedFrecency, - `Frecency of the page is the expected one (${url.spec})`); - - let hidden = await promiseFieldForUrl(url, "hidden"); - is(hidden, expectedHidden, `The redirecting page should be hidden (${url.spec})`); -} - -let expectedRedirectSourceFrecency = 0; -let expectedTargetFrecency = 0; +let redirectSourceFrecency = 0; +let redirectTargetFrecency = 0; add_task(async function test_multiple_redirect() { // Used to verify the redirect bonus overrides the typed bonus. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); - expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; + redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't // currently track redirects across multiple redirects, we fallback to the // PERM_REDIRECT_VISIT_BONUS. - expectedTargetFrecency += PERM_REDIRECT_VISIT_BONUS; + redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS; + let redirectNotified = false; - let visitedURIPromise = promiseVisitedURIObserver(REDIRECT_URI, TARGET_URI, expectedTargetFrecency); - - let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - await Promise.all([visitedURIPromise, newTabPromise]); + let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => { + info("Received onVisit: " + uri.spec); + if (uri.equals(REDIRECT_URI)) { + redirectNotified = true; + } + return uri.equals(TARGET_URI); + }, "history"); - await testURIFields(REDIRECT_URI, expectedRedirectSourceFrecency, 1); - await testURIFields(INTERMEDIATE_URI_1, expectedRedirectSourceFrecency, 1); - await testURIFields(INTERMEDIATE_URI_2, expectedRedirectSourceFrecency, 1); - await testURIFields(TARGET_URI, expectedTargetFrecency, 0); + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); + info("Waiting for onVisit"); + await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - gBrowser.removeCurrentTab(); + await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); + await check_uri(INTERMEDIATE_URI_1, redirectSourceFrecency, 1); + await check_uri(INTERMEDIATE_URI_2, redirectSourceFrecency, 1); + await check_uri(TARGET_URI, redirectTargetFrecency, 0); + + await BrowserTestUtils.removeTab(tab); }); add_task(async function redirect_check_second_typed_visit() { // A second visit with a typed url. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); - expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; + redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't // currently track redirects across multiple redirects, we fallback to the // PERM_REDIRECT_VISIT_BONUS. - expectedTargetFrecency += PERM_REDIRECT_VISIT_BONUS; + redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS; + let redirectNotified = false; - let visitedURIPromise = promiseVisitedURIObserver(REDIRECT_URI, TARGET_URI, expectedTargetFrecency); - - let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - await Promise.all([visitedURIPromise, newTabPromise]); + let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => { + info("Received onVisit: " + uri.spec); + if (uri.equals(REDIRECT_URI)) { + redirectNotified = true; + } + return uri.equals(TARGET_URI); + }, "history"); - await testURIFields(REDIRECT_URI, expectedRedirectSourceFrecency, 1); - await testURIFields(INTERMEDIATE_URI_1, expectedRedirectSourceFrecency, 1); - await testURIFields(INTERMEDIATE_URI_2, expectedRedirectSourceFrecency, 1); - await testURIFields(TARGET_URI, expectedTargetFrecency, 0); + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); + info("Waiting for onVisit"); + await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - gBrowser.removeCurrentTab(); + await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); + await check_uri(INTERMEDIATE_URI_1, redirectSourceFrecency, 1); + await check_uri(INTERMEDIATE_URI_2, redirectSourceFrecency, 1); + await check_uri(TARGET_URI, redirectTargetFrecency, 0); + + await BrowserTestUtils.removeTab(tab); }); add_task(async function redirect_check_subsequent_link_visit() { // Another visit, but this time as a visited url. - expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; - // TODO Bug 487813 - This should be LINK_VISIT_BONUS, however as we don't + redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; + // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't // currently track redirects across multiple redirects, we fallback to the // PERM_REDIRECT_VISIT_BONUS. - expectedTargetFrecency += PERM_REDIRECT_VISIT_BONUS; + redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS; + let redirectNotified = false; - let visitedURIPromise = promiseVisitedURIObserver(REDIRECT_URI, TARGET_URI, expectedTargetFrecency); - - let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - await Promise.all([visitedURIPromise, newTabPromise]); + let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => { + info("Received onVisit: " + uri.spec); + if (uri.equals(REDIRECT_URI)) { + redirectNotified = true; + } + return uri.equals(TARGET_URI); + }, "history"); - await testURIFields(REDIRECT_URI, expectedRedirectSourceFrecency, 1); - await testURIFields(INTERMEDIATE_URI_1, expectedRedirectSourceFrecency, 1); - await testURIFields(INTERMEDIATE_URI_2, expectedRedirectSourceFrecency, 1); - await testURIFields(TARGET_URI, expectedTargetFrecency, 0); + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); + info("Waiting for onVisit"); + await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - gBrowser.removeCurrentTab(); + await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); + await check_uri(INTERMEDIATE_URI_1, redirectSourceFrecency, 1); + await check_uri(INTERMEDIATE_URI_2, redirectSourceFrecency, 1); + await check_uri(TARGET_URI, redirectTargetFrecency, 0); + + await BrowserTestUtils.removeTab(tab); }); diff --git a/toolkit/components/places/tests/browser/browser_redirect.js b/toolkit/components/places/tests/browser/browser_redirect.js --- a/toolkit/components/places/tests/browser/browser_redirect.js +++ b/toolkit/components/places/tests/browser/browser_redirect.js @@ -1,122 +1,111 @@ -/* 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/. */ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ -const REDIRECT_URI = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect.sjs"); -const TARGET_URI = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect-target.html"); +const ROOT_URI = "http://mochi.test:8888/tests/toolkit/components/places/tests/browser/"; +const REDIRECT_URI = Services.io.newURI(ROOT_URI + "redirect.sjs"); +const TARGET_URI = Services.io.newURI(ROOT_URI + "redirect-target.html"); const REDIRECT_SOURCE_VISIT_BONUS = Services.prefs.getIntPref("places.frecency.redirectSourceVisitBonus"); const LINK_VISIT_BONUS = Services.prefs.getIntPref("places.frecency.linkVisitBonus"); const TYPED_VISIT_BONUS = Services.prefs.getIntPref("places.frecency.typedVisitBonus"); // Ensure that decay frecency doesn't kick in during tests (as a result // of idle-daily). Services.prefs.setCharPref("places.frecency.decayRate", "1.0"); registerCleanupFunction(async function() { Services.prefs.clearUserPref("places.frecency.decayRate"); - await PlacesTestUtils.clearHistory(); + await PlacesUtils.history.clear(); }); -function promiseVisitedWithFrecency(expectedRedirectFrecency, expectedTargetFrecency) { - // Create and add history observer. - return new Promise(resolve => { - let historyObserver = { - _redirectNotified: false, - onVisit(aURI, aVisitID, aTime, aSessionID, aReferringID, - aTransitionType) { - info("Received onVisit: " + aURI.spec); - - if (aURI.equals(REDIRECT_URI)) { - this._redirectNotified = true; - // Wait for the target page notification. - return; - } - - PlacesUtils.history.removeObserver(historyObserver); - - ok(this._redirectNotified, "The redirect should have been notified"); - - fieldForUrl(REDIRECT_URI, "frecency", function(aFrecency) { - is(aFrecency, expectedRedirectFrecency, - "Frecency of the redirecting page is the expected one"); - - fieldForUrl(REDIRECT_URI, "hidden", function(aHidden) { - is(aHidden, 1, "The redirecting page should be hidden"); +let redirectSourceFrecency = 0; +let redirectTargetFrecency = 0; - fieldForUrl(TARGET_URI, "frecency", function(aFrecency2) { - is(aFrecency2, expectedTargetFrecency, - "Frecency of the target page is the expected one"); - - fieldForUrl(TARGET_URI, "hidden", function(aHidden2) { - is(aHidden2, 0, "The target page should not be hidden"); - resolve(); - }); - }); - }); - }); - }, - onBeginUpdateBatch() {}, - onEndUpdateBatch() {}, - onTitleChanged() {}, - onDeleteURI() {}, - onClearHistory() {}, - onPageChanged() {}, - onDeleteVisits() {}, - QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]) - }; - PlacesUtils.history.addObserver(historyObserver); - }); +async function check_uri(uri, frecency, hidden) { + is(await PlacesTestUtils.fieldInDB(uri, "frecency"), frecency, + "Frecency of the page is the expected one"); + is(await PlacesTestUtils.fieldInDB(uri, "hidden"), hidden, + "Hidden value of the page is the expected one"); } -let expectedRedirectSourceFrecency = 0; -let expectedTypedVisitBonus = 0; - add_task(async function redirect_check_new_typed_visit() { // Used to verify the redirect bonus overrides the typed bonus. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); - expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; - expectedTypedVisitBonus += TYPED_VISIT_BONUS; + redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; + redirectTargetFrecency += TYPED_VISIT_BONUS; + let redirectNotified = false; - let visitedPromise = promiseVisitedWithFrecency(expectedRedirectSourceFrecency, - expectedTypedVisitBonus); + let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => { + info("Received onVisit for: " + uri.spec); + if (uri.equals(REDIRECT_URI)) { + redirectNotified = true; + } + return uri.equals(TARGET_URI); + }, "history"); - let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - await Promise.all([visitedPromise, newTabPromise]); + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); + info("Waiting for onVisit"); + await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - gBrowser.removeCurrentTab(); + await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); + await check_uri(TARGET_URI, redirectTargetFrecency, 0); + + await BrowserTestUtils.removeTab(tab); }); add_task(async function redirect_check_second_typed_visit() { // A second visit with a typed url. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); - expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; - expectedTypedVisitBonus += TYPED_VISIT_BONUS; + redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; + redirectTargetFrecency += TYPED_VISIT_BONUS; + let redirectNotified = false; - let visitedPromise = promiseVisitedWithFrecency(expectedRedirectSourceFrecency, - expectedTypedVisitBonus); + let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => { + info("Received onVisit: " + uri.spec); + if (uri.equals(REDIRECT_URI)) { + redirectNotified = true; + } + return uri.equals(TARGET_URI); + }, "history"); - let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - await Promise.all([visitedPromise, newTabPromise]); + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); + info("Waiting for onVisit"); + await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - gBrowser.removeCurrentTab(); + await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); + await check_uri(TARGET_URI, redirectTargetFrecency, 0); + + await BrowserTestUtils.removeTab(tab); }); add_task(async function redirect_check_subsequent_link_visit() { // Another visit, but this time as a visited url. - expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; - expectedTypedVisitBonus += LINK_VISIT_BONUS; - - let visitedPromise = promiseVisitedWithFrecency(expectedRedirectSourceFrecency, - expectedTypedVisitBonus); + redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; + redirectTargetFrecency += LINK_VISIT_BONUS; + let redirectNotified = false; - let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - await Promise.all([visitedPromise, newTabPromise]); + let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => { + info("Received onVisit: " + uri.spec); + if (uri.equals(REDIRECT_URI)) { + redirectNotified = true; + } + return uri.equals(TARGET_URI); + }, "history"); - gBrowser.removeCurrentTab(); + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); + info("Waiting for onVisit"); + await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); + + await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); + await check_uri(TARGET_URI, redirectTargetFrecency, 0); + + await BrowserTestUtils.removeTab(tab); }); diff --git a/toolkit/components/places/tests/history/test_insert_null_title.js b/toolkit/components/places/tests/history/test_insert_null_title.js new file mode 100644 --- /dev/null +++ b/toolkit/components/places/tests/history/test_insert_null_title.js @@ -0,0 +1,78 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +// Tests that passing a null title to history insert or update doesn't overwrite +// an existing title, while an empty string does. + +"use strict"; + +async function fetchTitle(url) { + let entry; + await TestUtils.waitForCondition( + async () => { + entry = await PlacesUtils.history.fetch(url); + return !!entry; + }, + "fetch title for entry"); + return entry.title; +} + +add_task(async function() { + const url = "http://mozilla.com"; + let title = "Mozilla"; + + do_print("Insert a visit with a title"); + let result = await PlacesUtils.history.insert({ + url, + title, + visits: [ + { transition: PlacesUtils.history.TRANSITIONS.LINK } + ] + }); + Assert.equal(title, result.title, "title should be stored"); + Assert.equal(title, await fetchTitle(url), "title should be stored"); + + // This is shared by the next tests. + let promiseTitleChange = PlacesTestUtils.waitForNotification("onTitleChanged", + () => notified = true, "history"); + + do_print("Insert a visit with a null title, should not clear the previous title"); + let notified = false; + result = await PlacesUtils.history.insert({ + url, + title: null, + visits: [ + { transition: PlacesUtils.history.TRANSITIONS.LINK } + ] + }); + Assert.equal(title, result.title, "title should be unchanged"); + Assert.equal(title, await fetchTitle(url), "title should be unchanged"); + await Promise.race([promiseTitleChange, new Promise(r => do_timeout(1000, r))]); + Assert.ok(!notified, "A title change should not be notified"); + + do_print("Insert a visit without specifying a title, should not clear the previous title"); + notified = false; + result = await PlacesUtils.history.insert({ + url, + visits: [ + { transition: PlacesUtils.history.TRANSITIONS.LINK } + ] + }); + Assert.equal(title, result.title, "title should be unchanged"); + Assert.equal(title, await fetchTitle(url), "title should be unchanged"); + await Promise.race([promiseTitleChange, new Promise(r => do_timeout(1000, r))]); + Assert.ok(!notified, "A title change should not be notified"); + + do_print("Insert a visit with an empty title, should clear the previous title"); + result = await PlacesUtils.history.insert({ + url, + title: "", + visits: [ + { transition: PlacesUtils.history.TRANSITIONS.LINK } + ] + }); + do_print("Waiting for the title change notification"); + await promiseTitleChange; + Assert.equal("", result.title, "title should be empty"); + Assert.equal("", await fetchTitle(url), "title should be empty"); +}); diff --git a/toolkit/components/places/tests/history/xpcshell.ini b/toolkit/components/places/tests/history/xpcshell.ini --- a/toolkit/components/places/tests/history/xpcshell.ini +++ b/toolkit/components/places/tests/history/xpcshell.ini @@ -1,15 +1,16 @@ [DEFAULT] head = head_history.js [test_async_history_api.js] [test_fetch.js] [test_hasVisits.js] [test_insert.js] +[test_insert_null_title.js] [test_insertMany.js] [test_remove.js] [test_removeMany.js] [test_removeVisits.js] [test_removeByFilter.js] [test_removeVisitsByFilter.js] [test_sameUri_titleChanged.js] [test_update.js] diff --git a/toolkit/components/places/tests/unit/test_sync_utils.js b/toolkit/components/places/tests/unit/test_sync_utils.js --- a/toolkit/components/places/tests/unit/test_sync_utils.js +++ b/toolkit/components/places/tests/unit/test_sync_utils.js @@ -303,29 +303,30 @@ add_task(async function test_fetchGuidFo // Remove the visits added during this test. await PlacesTestUtils.clearHistory(); }); add_task(async function test_fetchURLInfoForGuid() { // Add some visits of the following URLs. specifying the title. let visits = [{ uri: "https://www.mozilla.org/en-US/", title: "mozilla" }, { uri: "http://getfirefox.com/", title: "firefox" }, - { uri: "http://getthunderbird.com/", title: "thunderbird" }]; + { uri: "http://getthunderbird.com/", title: "thunderbird" }, + { uri: "http://quantum.mozilla.com/", title: null}]; for (let visit of visits) { await PlacesTestUtils.addVisits(visit); } for (let visit of visits) { let guid = await PlacesSyncUtils.history.fetchGuidForURL(visit.uri); let info = await PlacesSyncUtils.history.fetchURLInfoForGuid(guid); // Compare the info returned by fetchURLInfoForGuid, // URL and title should match while frecency must be different than -1. equal(info.url, visit.uri, "The url provided should be the same as the url retrieved."); - equal(info.title, visit.title, "The title provided should be the same as the title retrieved."); + equal(info.title, visit.title || "", "The title provided should be the same as the title retrieved."); notEqual(info.frecency, -1, "The frecency of the visit should be different than -1."); } // Create a "fake" GUID and check that the result of fetchURLInfoForGuid is null. let guid = makeGuid(); let info = await PlacesSyncUtils.history.fetchURLInfoForGuid(guid); equal(info, null, "The information object of a non-existent guid should be null.");