Bug 1419825 - Callers of insertVisitedURIs may overwrite the history title passing a null title. r=kit draft
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 23 Nov 2017 18:44:07 +0100
changeset 708335 9cb20b76ecf7dd73976a445b158ae225e8000a1d
parent 707249 f2cf6d1473808039be5ecd8727cc3791d5d7d2d4
child 743170 3aa6027db1f56e507f8e718e04fe64cf1b1e530b
push id92362
push usermak77@bonardo.net
push dateWed, 06 Dec 2017 15:56:53 +0000
reviewerskit
bugs1419825
milestone59.0a1
Bug 1419825 - Callers of insertVisitedURIs may overwrite the history title passing a null title. r=kit MozReview-Commit-ID: EPU4mv8rn7h
toolkit/components/places/History.cpp
toolkit/components/places/History.jsm
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/tests/PlacesTestUtils.jsm
toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
toolkit/components/places/tests/browser/browser_redirect.js
toolkit/components/places/tests/history/test_insert_null_title.js
toolkit/components/places/tests/history/xpcshell.ini
toolkit/components/places/tests/unit/test_sync_utils.js
--- 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;
@@ -2270,37 +2272,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<mozIStorageStatement> stmt = GetStatement(
+  nsCOMPtr<mozIStorageStatement> 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);
@@ -2382,18 +2398,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;
@@ -2993,17 +3009,22 @@ History::UpdatePlaces(JS::Handle<JS::Val
     // Check each visit, and build our array of VisitData objects.
     visitData.SetCapacity(visitData.Length() + visitsLength);
     for (uint32_t j = 0; j < visitsLength; j++) {
       JS::Rooted<JSObject*> 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
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -1310,16 +1310,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
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -215,17 +215,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 {
--- a/toolkit/components/places/tests/PlacesTestUtils.jsm
+++ b/toolkit/components/places/tests/PlacesTestUtils.jsm
@@ -49,17 +49,18 @@ this.PlacesTestUtils = Object.freeze({
     } else {
       throw new Error("Unsupported type passed to addVisits");
     }
 
     // Create a PageInfo for each entry.
     let lastStoredVisit;
     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 = Services.io.newURI(place.referrer);
       } else if (place.referrer && place.referrer instanceof URL) {
         place.referrer = Services.io.newURI(place.referrer.href);
       }
       let visitDate = place.visitDate;
       if (visitDate) {
         if (visitDate.constructor.name != "Date") {
--- 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);
 });
--- 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);
 });
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");
+});
--- 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]
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -301,29 +301,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.");