Bug 1341097 - Part 3: Don't dispatch oodles of titlechanged notifications for new history entries. r=mak, a=lizzard
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 27 Feb 2017 18:26:21 +0000
changeset 378914 c4ed29d7cddeb8129ec1c8ce7982f78d95cdf36e
parent 378913 15583c8ebe407a0de32a4c15c8aeea6554d8539c
child 378915 d7d8bd983a300b3fa9a15be16b698c62e48f4b2b
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, lizzard
bugs1341097
milestone53.0a2
Bug 1341097 - Part 3: Don't dispatch oodles of titlechanged notifications for new history entries. r=mak, a=lizzard MozReview-Commit-ID: 7jHOcCQ5ZBb
addon-sdk/source/lib/sdk/places/events.js
browser/components/newtab/PlacesProvider.jsm
browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
services/sync/tests/unit/test_history_store.js
toolkit/components/downloads/nsDownloadManager.cpp
toolkit/components/places/History.cpp
toolkit/components/places/nsINavHistoryService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistory.h
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/nsNavHistoryResult.h
toolkit/components/places/tests/browser/browser.ini
toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js
toolkit/components/places/tests/browser/empty_page.html
toolkit/components/places/tests/history/test_updatePlaces_sameUri_titleChanged.js
toolkit/components/places/tests/unit/test_async_history_api.js
toolkit/modules/NewTabUtils.jsm
--- a/addon-sdk/source/lib/sdk/places/events.js
+++ b/addon-sdk/source/lib/sdk/places/events.js
@@ -110,16 +110,30 @@ function formatValue (type, data) {
   if (type === 'type')
     return mapBookmarkItemType(data);
   if (type === 'url' && data)
     return data.spec;
   return data;
 }
 
 var historyObserver = createObserverInstance(HISTORY_EVENTS, HISTORY_ARGS);
+// Hack alert: we sometimes need to send extra title-changed notifications
+// ourselves for backwards compat. Replace the original onVisit callback to
+// add on that logic:
+historyObserver.realOnVisit = historyObserver.onVisit;
+historyObserver.onVisit = function(url, visitId, time, sessionId,
+                                   referringId, transitionType, guid, hidden,
+                                   visitCount, typed, lastKnownTitle) {
+  // If this is the first visit we're adding, fire title-changed
+  // in case anyone cares.
+  if (visitCount == 1) {
+    historyObserver.onTitleChanged(url, lastKnownTitle);
+  }
+  this.realOnVisit(url, visitId, time, sessionId, referringId, transitionType);
+};
 historyService.addObserver(historyObserver, false);
 
 var bookmarkObserver = createObserverInstance(BOOKMARK_EVENTS, BOOKMARK_ARGS);
 bookmarkService.addObserver(bookmarkObserver, false);
 
 when(() => {
   historyService.removeObserver(historyObserver);
   bookmarkService.removeObserver(bookmarkObserver);
--- a/browser/components/newtab/PlacesProvider.jsm
+++ b/browser/components/newtab/PlacesProvider.jsm
@@ -47,29 +47,56 @@ Links.prototype = {
     return HISTORY_RESULTS_LIMIT;
   },
 
   /**
    * A set of functions called by @mozilla.org/browser/nav-historyservice
    * All history events are emitted from this object.
    */
   historyObserver: {
+    _batchProcessingDepth: 0,
+    _batchCalledFrecencyChanged: false,
+
+    /**
+     * Called by the history service.
+     */
+    onBeginUpdateBatch() {
+      this._batchProcessingDepth += 1;
+    },
+
+    onEndUpdateBatch() {
+      this._batchProcessingDepth -= 1;
+      if (this._batchProcessingDepth == 0 && this._batchCalledFrecencyChanged) {
+        this.onManyFrecenciesChanged();
+        this._batchCalledFrecencyChanged = false;
+      }
+    },
+
     onDeleteURI: function historyObserver_onDeleteURI(aURI) {
       // let observers remove sensetive data associated with deleted visit
       gLinks.emit("deleteURI", {
         url: aURI.spec,
       });
     },
 
     onClearHistory: function historyObserver_onClearHistory() {
       gLinks.emit("clearHistory");
     },
 
     onFrecencyChanged: function historyObserver_onFrecencyChanged(aURI,
                            aNewFrecency, aGUID, aHidden, aLastVisitDate) { // jshint ignore:line
+
+      // If something is doing a batch update of history entries we don't want
+      // to do lots of work for each record. So we just track the fact we need
+      // to call onManyFrecenciesChanged() once the batch is complete.
+      if (this._batchProcessingDepth > 0) {
+        this._batchCalledFrecencyChanged = true;
+        return;
+      }
+
       // The implementation of the query in getLinks excludes hidden and
       // unvisited pages, so it's important to exclude them here, too.
       if (!aHidden && aLastVisitDate &&
           NewTabUtils.linkChecker.checkLoadURI(aURI.spec)) {
         gLinks.emit("linkChanged", {
           url: aURI.spec,
           frecency: aNewFrecency,
           lastVisitDate: aLastVisitDate,
@@ -79,16 +106,24 @@ Links.prototype = {
     },
 
     onManyFrecenciesChanged: function historyObserver_onManyFrecenciesChanged() {
       // Called when frecencies are invalidated and also when clearHistory is called
       // See toolkit/components/places/tests/unit/test_frecency_observers.js
       gLinks.emit("manyLinksChanged");
     },
 
+    onVisit(aURI, aVisitId, aTime, aSessionId, aReferrerVisitId, aTransitionType,
+            aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle) {
+      // For new visits, if we're not batch processing, notify for a title update
+      if (!this._batchProcessingDepth && aVisitCount == 1 && aLastKnownTitle) {
+        this.onTitleChanged(aURI, aTitle, aGuid);
+      }
+    },
+
     onTitleChanged: function historyObserver_onTitleChanged(aURI, aNewTitle) {
       if (NewTabUtils.linkChecker.checkLoadURI(aURI.spec)) {
         gLinks.emit("linkChanged", {
           url: aURI.spec,
           title: aNewTitle
         });
       }
     },
--- a/browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
+++ b/browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
@@ -140,25 +140,24 @@ add_task(function* test_Links_getLinks_D
 add_task(function* test_Links_onLinkChanged() {
   let provider = PlacesProvider.links;
 
   let url = "https://example.com/onFrecencyChanged1";
   let linkChangedMsgCount = 0;
 
   let linkChangedPromise = new Promise(resolve => {
     let handler = (_, link) => { // jshint ignore:line
-      /* There are 3 linkChanged events:
+      /* There are 2 linkChanged events:
        * 1. visit insertion (-1 frecency by default)
        * 2. frecency score update (after transition type calculation etc)
-       * 3. title change
        */
       if (link.url === url) {
         equal(link.url, url, `expected url on linkChanged event`);
         linkChangedMsgCount += 1;
-        if (linkChangedMsgCount === 3) {
+        if (linkChangedMsgCount === 2) {
           ok(true, `all linkChanged events captured`);
           provider.off("linkChanged", this);
           resolve();
         }
       }
     };
     provider.on("linkChanged", handler);
   });
--- a/services/sync/tests/unit/test_history_store.js
+++ b/services/sync/tests/unit/test_history_store.js
@@ -28,26 +28,27 @@ function queryPlaces(uri, options) {
 function queryHistoryVisits(uri) {
   let options = PlacesUtils.history.getNewQueryOptions();
   options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY;
   options.resultType = Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT;
   options.sortingMode = Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_ASCENDING;
   return queryPlaces(uri, options);
 }
 
-function onNextTitleChanged(callback) {
+function onNextVisit(callback) {
   PlacesUtils.history.addObserver({
     onBeginUpdateBatch: function onBeginUpdateBatch() {},
     onEndUpdateBatch: function onEndUpdateBatch() {},
     onPageChanged: function onPageChanged() {},
     onTitleChanged: function onTitleChanged() {
+    },
+    onVisit: function onVisit() {
       PlacesUtils.history.removeObserver(this);
       Utils.nextTick(callback);
     },
-    onVisit: function onVisit() {},
     onDeleteVisits: function onDeleteVisits() {},
     onPageExpired: function onPageExpired() {},
     onDeleteURI: function onDeleteURI() {},
     onClearHistory: function onClearHistory() {},
     QueryInterface: XPCOMUtils.generateQI([
       Ci.nsINavHistoryObserver,
       Ci.nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS,
       Ci.nsISupportsWeakReference
@@ -120,17 +121,17 @@ add_test(function test_store() {
     do_check_eq(record.title, "Get Firefox!");
     do_check_eq(record.visits.length, 1);
     do_check_eq(record.visits[0].date, TIMESTAMP1);
     do_check_eq(record.visits[0].type, Ci.nsINavHistoryService.TRANSITION_LINK);
 
     _("Let's modify the record and have the store update the database.");
     let secondvisit = {date: TIMESTAMP2,
                        type: Ci.nsINavHistoryService.TRANSITION_TYPED};
-    onNextTitleChanged(ensureThrows(function() {
+    onNextVisit(ensureThrows(function() {
       let queryres = queryHistoryVisits(fxuri);
       do_check_eq(queryres.length, 2);
       do_check_eq(queryres[0].time, TIMESTAMP1);
       do_check_eq(queryres[0].title, "Hol Dir Firefox!");
       do_check_eq(queryres[1].time, TIMESTAMP2);
       do_check_eq(queryres[1].title, "Hol Dir Firefox!");
       run_next_test();
     }));
@@ -142,17 +143,17 @@ add_test(function test_store() {
     ]);
   }
 });
 
 add_test(function test_store_create() {
   _("Create a brand new record through the store.");
   tbguid = Utils.makeGUID();
   tburi = Utils.makeURI("http://getthunderbird.com");
-  onNextTitleChanged(ensureThrows(function() {
+  onNextVisit(ensureThrows(function() {
     do_check_attribute_count(store.getAllIDs(), 2);
     let queryres = queryHistoryVisits(tburi);
     do_check_eq(queryres.length, 1);
     do_check_eq(queryres[0].time, TIMESTAMP3);
     do_check_eq(queryres[0].title, "The bird is the word!");
     run_next_test();
   }));
   applyEnsureNoFailures([
--- a/toolkit/components/downloads/nsDownloadManager.cpp
+++ b/toolkit/components/downloads/nsDownloadManager.cpp
@@ -2327,17 +2327,18 @@ nsDownloadManager::OnEndUpdateBatch()
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDownloadManager::OnVisit(nsIURI *aURI, int64_t aVisitID, PRTime aTime,
                            int64_t aSessionID, int64_t aReferringID,
                            uint32_t aTransitionType, const nsACString& aGUID,
-                           bool aHidden, uint32_t aVisitCount, uint32_t aTyped)
+                           bool aHidden, uint32_t aVisitCount, uint32_t aTyped,
+                           const nsAString& aLastKnowntTitle)
 {
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDownloadManager::OnTitleChanged(nsIURI *aURI,
                                   const nsAString &aPageTitle,
                                   const nsACString &aGUID)
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -653,17 +653,18 @@ public:
 
     // Notify the visit.  Note that TRANSITION_EMBED visits are never added
     // to the database, thus cannot be queried and we don't notify them.
     if (mPlace.transitionType != nsINavHistoryService::TRANSITION_EMBED) {
       navHistory->NotifyOnVisit(uri, mPlace.visitId, mPlace.visitTime,
                                 mPlace.referrerVisitId, mPlace.transitionType,
                                 mPlace.guid, mPlace.hidden,
                                 mPlace.visitCount + 1, // Add current visit.
-                                static_cast<uint32_t>(mPlace.typed));
+                                static_cast<uint32_t>(mPlace.typed),
+                                mPlace.title);
     }
 
     nsCOMPtr<nsIObserverService> obsService =
       mozilla::services::GetObserverService();
     if (obsService) {
       DebugOnly<nsresult> rv =
         obsService->NotifyObservers(uri, URI_VISIT_SAVED, nullptr);
       NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Could not notify observers");
@@ -1040,17 +1041,17 @@ public:
       // visit/place, so update the count:
       mSuccessfulUpdatedCount++;
 
       nsCOMPtr<nsIRunnable> event = new NotifyVisitObservers(place);
       rv = NS_DispatchToMainThread(event);
       NS_ENSURE_SUCCESS(rv, rv);
 
       // Notify about title change if needed.
-      if ((!known && !place.title.IsVoid()) || place.titleChanged) {
+      if (place.titleChanged) {
         event = new NotifyTitleObservers(place.spec, place.title, place.guid);
         rv = NS_DispatchToMainThread(event);
         NS_ENSURE_SUCCESS(rv, rv);
       }
     }
 
     nsresult rv = transaction.Commit();
     NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/nsINavHistoryService.idl
+++ b/toolkit/components/places/nsINavHistoryService.idl
@@ -666,27 +666,31 @@ interface nsINavHistoryObserver : nsISup
    * @param aHidden
    *        Whether the visited page is marked as hidden.
    * @param aVisitCount
    *        Number of visits (included this one) for this URI.
    * @param aTyped
    *        Whether the URI has been typed or not.
    *        TODO (Bug 1271801): This will become a count, rather than a boolean.
    *        For future compatibility, always compare it with "> 0".
+   * @param aLastKnownTitle
+   *        The last known title of the page. Might not be from the current visit,
+   *        and might be null if it is not known.
    */
   void onVisit(in nsIURI aURI,
                in long long aVisitId,
                in PRTime aTime,
                in long long aSessionId,
                in long long aReferrerVisitId,
                in unsigned long aTransitionType,
                in ACString aGuid,
                in boolean aHidden,
                in unsigned long aVisitCount,
-               in unsigned long aTyped);
+               in unsigned long aTyped,
+               in AString aLastKnownTitle);
 
   /**
    * Called whenever either the "real" title or the custom title of the page
    * changed. BOTH TITLES ARE ALWAYS INCLUDED in this notification, even though
    * only one will change at a time. Often, consumers will want to display the
    * user title if it is available, and fall back to the page title (the one
    * specified in the <title> tag of the page).
    *
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -3216,17 +3216,18 @@ nsNavBookmarks::OnEndUpdateBatch()
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,
                         int64_t aSessionID, int64_t aReferringID,
                         uint32_t aTransitionType, const nsACString& aGUID,
-                        bool aHidden, uint32_t aVisitCount, uint32_t aTyped)
+                        bool aHidden, uint32_t aVisitCount, uint32_t aTyped,
+                        const nsAString& aLastKnownTitle)
 {
   NS_ENSURE_ARG(aURI);
 
   // If the page is bookmarked, notify observers for each associated bookmark.
   ItemVisitData visitData;
   nsresult rv = aURI->GetSpec(visitData.bookmark.url);
   NS_ENSURE_SUCCESS(rv, rv);
   visitData.visitId = aVisitId;
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -484,40 +484,42 @@ nsNavHistory::LoadPrefs()
   FRECENCY_PREF(mDefaultWeight,            PREF_FREC_DEFAULT_BUCKET_WEIGHT);
 
 #undef FRECENCY_PREF
 }
 
 
 void
 nsNavHistory::NotifyOnVisit(nsIURI* aURI,
-                          int64_t aVisitId,
-                          PRTime aTime,
-                          int64_t aReferrerVisitId,
-                          int32_t aTransitionType,
-                          const nsACString& aGuid,
-                          bool aHidden,
-                          uint32_t aVisitCount,
-                          uint32_t aTyped)
+                            int64_t aVisitId,
+                            PRTime aTime,
+                            int64_t aReferrerVisitId,
+                            int32_t aTransitionType,
+                            const nsACString& aGuid,
+                            bool aHidden,
+                            uint32_t aVisitCount,
+                            uint32_t aTyped,
+                            const nsAString& aLastKnownTitle)
 {
   MOZ_ASSERT(!aGuid.IsEmpty());
+  MOZ_ASSERT(aVisitCount, "Should have at least 1 visit when notifying");
   // If there's no history, this visit will surely add a day.  If the visit is
   // added before or after the last cached day, the day count may have changed.
   // Otherwise adding multiple visits in the same day should not invalidate
   // the cache.
   if (mDaysOfHistory == 0) {
     mDaysOfHistory = 1;
   } else if (aTime > mLastCachedEndOfDay || aTime < mLastCachedStartOfDay) {
     mDaysOfHistory = -1;
   }
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavHistoryObserver,
                    OnVisit(aURI, aVisitId, aTime, 0, aReferrerVisitId,
-                           aTransitionType, aGuid, aHidden, aVisitCount, aTyped));
+                           aTransitionType, aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle));
 }
 
 void
 nsNavHistory::NotifyTitleChange(nsIURI* aURI,
                                 const nsString& aTitle,
                                 const nsACString& aGUID)
 {
   MOZ_ASSERT(!aGUID.IsEmpty());
--- a/toolkit/components/places/nsNavHistory.h
+++ b/toolkit/components/places/nsNavHistory.h
@@ -439,17 +439,18 @@ public:
   void NotifyOnVisit(nsIURI* aURI,
                      int64_t aVisitId,
                      PRTime aTime,
                      int64_t aReferrerVisitId,
                      int32_t aTransitionType,
                      const nsACString& aGuid,
                      bool aHidden,
                      uint32_t aVisitCount,
-                     uint32_t aTyped);
+                     uint32_t aTyped,
+                     const nsAString& aLastKnownTitle);
 
   /**
    * Fires onTitleChanged event to nsINavHistoryService observers
    */
   void NotifyTitleChange(nsIURI* aURI,
                          const nsString& title,
                          const nsACString& aGUID);
 
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -4651,31 +4651,42 @@ nsNavHistoryResult::OnItemMoved(int64_t 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,
                             int64_t aSessionId, int64_t aReferringId,
                             uint32_t aTransitionType, const nsACString& aGUID,
-                            bool aHidden, uint32_t aVisitCount, uint32_t aTyped)
+                            bool aHidden, uint32_t aVisitCount, uint32_t aTyped,
+                            const nsAString& aLastKnownTitle)
 {
   NS_ENSURE_ARG(aURI);
 
   // Embed visits are never shown in our views.
   if (aTransitionType == nsINavHistoryService::TRANSITION_EMBED) {
     return NS_OK;
   }
 
   uint32_t added = 0;
 
   ENUMERATE_HISTORY_OBSERVERS(OnVisit(aURI, aVisitId, aTime, aSessionId,
                                       aReferringId, aTransitionType, aGUID,
                                       aHidden, &added));
 
+  // When we add visits through UpdatePlaces, we don't bother telling
+  // the world that the title 'changed' from nothing to the first title
+  // we ever see for a history entry. Our consumers here might still
+  // care, though, so we have to tell them - but only for the first
+  // visit we add. For subsequent changes, updateplaces will dispatch
+  // ontitlechanged notifications as normal.
+  if (!aLastKnownTitle.IsVoid() && aVisitCount == 1) {
+    ENUMERATE_HISTORY_OBSERVERS(OnTitleChanged(aURI, aLastKnownTitle, aGUID));
+  }
+
   if (!mRootNode->mExpanded)
     return NS_OK;
 
   // If this visit is accepted by an overlapped container, and not all
   // overlapped containers are visible, we should still call Refresh if the
   // visit falls into any of them.
   bool todayIsMissing = false;
   uint32_t resultType = mRootNode->mOptions->ResultType();
--- a/toolkit/components/places/nsNavHistoryResult.h
+++ b/toolkit/components/places/nsNavHistoryResult.h
@@ -89,17 +89,19 @@ private:
                      bool aHidden, uint32_t* aAdded);
 
 // The external version is used by results.
 #define NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL(...)                 \
   NS_DECL_BOOKMARK_HISTORY_OBSERVER_BASE(__VA_ARGS__)                   \
   NS_IMETHOD OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,      \
                      int64_t aSessionId, int64_t aReferringId,          \
                      uint32_t aTransitionType, const nsACString& aGUID, \
-                     bool aHidden, uint32_t aVisitCount, uint32_t aTyped) __VA_ARGS__;
+                     bool aHidden, uint32_t aVisitCount,                \
+                     uint32_t aTyped, const nsAString& aLastKnownTitle) \
+                     __VA_ARGS__;
 
 // nsNavHistoryResult
 //
 //    nsNavHistory creates this object and fills in mChildren (by getting
 //    it through GetTopLevel()). Then FilledAllResults() is called to finish
 //    object initialization.
 
 #define NS_NAVHISTORYRESULT_IID \
--- a/toolkit/components/places/tests/browser/browser.ini
+++ b/toolkit/components/places/tests/browser/browser.ini
@@ -13,15 +13,18 @@ support-files =
 [browser_bug680727.js]
 [browser_colorAnalyzer.js]
 [browser_double_redirect.js]
 [browser_favicon_privatebrowsing_perwindowpb.js]
 [browser_favicon_setAndFetchFaviconForPage.js]
 [browser_favicon_setAndFetchFaviconForPage_failures.js]
 [browser_history_post.js]
 [browser_notfound.js]
+[browser_onvisit_title_null_for_navigation.js]
+support-files =
+  empty_page.html
 [browser_redirect.js]
 [browser_multi_redirect_frecency.js]
 [browser_settitle.js]
 [browser_visited_notfound.js]
 [browser_visituri.js]
 [browser_visituri_nohistory.js]
 [browser_visituri_privatebrowsing_perwindowpb.js]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js
@@ -0,0 +1,28 @@
+const TEST_PATH = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");
+
+add_task(function* checkTitleNotificationForNavigation() {
+  const EXPECTED_URL = Services.io.newURI(TEST_PATH + "empty_page.html");
+  let promiseTitleChanged = new Promise(resolve => {
+    let obs = {
+      onVisit(aURI, aVisitId, aTime, aSessionId, aReferrerVisitId, aTransitionType,
+              aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle) {
+        info("onVisit: " + aURI.spec);
+        if (aURI.equals(EXPECTED_URL)) {
+          Assert.equal(aLastKnownTitle, null, "Should not have a title");
+        }
+      },
+
+      onTitleChanged(aURI, aTitle, aGuid) {
+        if (aURI.equals(EXPECTED_URL)) {
+          is(aTitle, "I am an empty page", "Should have correct title in titlechanged notification");
+          PlacesUtils.history.removeObserver(obs);
+          resolve();
+        }
+      },
+    };
+    PlacesUtils.history.addObserver(obs, false);
+  });
+  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, EXPECTED_URL.spec);
+  yield promiseTitleChanged;
+  yield BrowserTestUtils.removeTab(tab);
+});
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/browser/empty_page.html
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>I am an empty page</title>
+  </head>
+  <body>Empty</body>
+</html>
--- a/toolkit/components/places/tests/history/test_updatePlaces_sameUri_titleChanged.js
+++ b/toolkit/components/places/tests/history/test_updatePlaces_sameUri_titleChanged.js
@@ -5,17 +5,17 @@ add_task(function* test() {
   let uri = "http://test.com/";
 
   let promiseTitleChangedNotifications = new Promise(resolve => {
     let historyObserver = {
       _count: 0,
       __proto__: NavHistoryObserver.prototype,
       onTitleChanged(aURI, aTitle, aGUID) {
         Assert.equal(aURI.spec, uri, "Should notify the proper url");
-        if (++this._count == 2) {
+        if (++this._count == 1) {
           PlacesUtils.history.removeObserver(historyObserver);
           resolve();
         }
       }
     };
     PlacesUtils.history.addObserver(historyObserver, false);
   });
 
--- a/toolkit/components/places/tests/unit/test_async_history_api.js
+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
@@ -93,24 +93,27 @@ function VisitObserver(aURI,
 VisitObserver.prototype = {
   __proto__: NavHistoryObserver.prototype,
   onVisit(aURI,
                     aVisitId,
                     aTime,
                     aSessionId,
                     aReferringId,
                     aTransitionType,
-                    aGUID) {
-    do_print("onVisit(" + aURI.spec + ", " + aVisitId + ", " + aTime +
-             ", " + aSessionId + ", " + aReferringId + ", " +
-             aTransitionType + ", " + aGUID + ")");
+                    aGUID,
+                    aHidden,
+                    aVisitCount,
+                    aTyped,
+                    aLastKnownTitle) {
+    let args = [...arguments].slice(1);
+    do_print("onVisit(" + aURI.spec + args.join(", ") + ")");
     if (!this.uri.equals(aURI) || this.guid != aGUID) {
       return;
     }
-    this.callback(aTime, aTransitionType);
+    this.callback(aTime, aTransitionType, aLastKnownTitle);
   },
 };
 
 /**
  * Tests that a title was set properly in the database.
  *
  * @param aURI
  *        The uri to check.
@@ -946,47 +949,55 @@ add_task(function* test_title_change_not
     });
 
   PlacesUtils.history.addObserver(silentObserver, false);
   let placesResult = yield promiseUpdatePlaces(place);
   if (placesResult.errors.length > 0) {
     do_throw("Unexpected error.");
   }
 
-  // The second case to test is that we get the notification when we add
+  // The second case to test is that we don't get the notification when we add
   // it for the first time.  The first case will fail before our callback if it
   // is busted, so we can do this now.
   place.uri = NetUtil.newURI(place.uri.spec + "/new-visit-with-title");
   place.title = "title 1";
-  function promiseTitleChangedObserver(aPlace) {
-    return new Promise((resolve, reject) => {
-      let callbackCount = 0;
-      let observer = new TitleChangedObserver(aPlace.uri, aPlace.title, function() {
-        switch (++callbackCount) {
-          case 1:
-            // The third case to test is to make sure we get a notification when
-            // we change an existing place.
-            observer.expectedTitle = place.title = "title 2";
-            place.visits = [new VisitInfo()];
-            PlacesUtils.asyncHistory.updatePlaces(place);
-            break;
-          case 2:
-            PlacesUtils.history.removeObserver(silentObserver);
-            PlacesUtils.history.removeObserver(observer);
-            resolve();
-            break;
-        }
-      });
+  let expectedNotification = false;
+  let titleChangeObserver;
+  let titleChangePromise = new Promise((resolve, reject) => {
+    titleChangeObserver = new TitleChangedObserver(place.uri, place.title, function() {
+      Assert.ok(expectedNotification, "Should not get notified for " + place.uri.spec + " with title " + place.title);
+      if (expectedNotification) {
+        PlacesUtils.history.removeObserver(silentObserver);
+        PlacesUtils.history.removeObserver(titleChangeObserver);
+        resolve();
+      }
+    });
+    PlacesUtils.history.addObserver(titleChangeObserver, false);
+  });
 
-      PlacesUtils.history.addObserver(observer, false);
-      PlacesUtils.asyncHistory.updatePlaces(aPlace);
-    });
-  }
+  let visitPromise = new Promise(resolve => {
+    PlacesUtils.history.addObserver({
+      onVisit(uri) {
+        Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI.");
+        PlacesUtils.history.removeObserver(this);
+        resolve();
+      }
+    }, false);
+  });
+  PlacesUtils.asyncHistory.updatePlaces(place);
+  yield visitPromise;
 
-  yield promiseTitleChangedObserver(place);
+  // The third case to test is to make sure we get a notification when
+  // we change an existing place.
+  expectedNotification = true;
+  titleChangeObserver.expectedTitle = place.title = "title 2";
+  place.visits = [new VisitInfo()];
+  PlacesUtils.asyncHistory.updatePlaces(place);
+
+  yield titleChangePromise;
   yield PlacesTestUtils.promiseAsyncUpdates();
 });
 
 add_task(function* test_visit_notifies() {
   // There are two observers we need to see for each visit.  One is an
   // nsINavHistoryObserver and the other is the uri-visit-saved observer topic.
   let place = {
     guid: "abcdefghijkl",
@@ -1230,8 +1241,80 @@ add_task(function* test_ignore_results_a
   Assert.equal(placesResult.errors.length, 0,
                "Should have seen 0 errors because we disabled reporting.");
   Assert.equal(placesResult.results.length, 0,
                "Should have seen 0 results because we disabled reporting.");
   Assert.equal(placesResult.resultCount, 1,
                "Should know that we updated 1 item from the completion callback.");
   yield PlacesTestUtils.promiseAsyncUpdates();
 });
+
+add_task(function* test_title_on_initial_visit() {
+  let place = {
+    uri: NetUtil.newURI(TEST_DOMAIN + "test_visit_title"),
+    title: "My title",
+    visits: [
+      new VisitInfo(),
+    ],
+    guid: "mnopqrstuvwx",
+  };
+  let visitPromise = new Promise(resolve => {
+    let visitObserver = new VisitObserver(place.uri, place.guid,
+                                          function(aVisitDate,
+                                                   aTransitionType,
+                                                   aLastKnownTitle) {
+      do_check_eq(place.title, aLastKnownTitle);
+
+      PlacesUtils.history.removeObserver(visitObserver);
+      resolve();
+    });
+    PlacesUtils.history.addObserver(visitObserver, false);
+  });
+  yield promiseUpdatePlaces(place);
+  yield visitPromise;
+
+  // Now check an empty title doesn't get reported as null
+  place = {
+    uri: NetUtil.newURI(TEST_DOMAIN + "test_visit_title"),
+    title: "",
+    visits: [
+      new VisitInfo(),
+    ],
+    guid: "fghijklmnopq",
+  };
+  visitPromise = new Promise(resolve => {
+    let visitObserver = new VisitObserver(place.uri, place.guid,
+                                          function(aVisitDate,
+                                                   aTransitionType,
+                                                   aLastKnownTitle) {
+      do_check_eq(place.title, aLastKnownTitle);
+
+      PlacesUtils.history.removeObserver(visitObserver);
+      resolve();
+    });
+    PlacesUtils.history.addObserver(visitObserver, false);
+  });
+  yield promiseUpdatePlaces(place);
+  yield visitPromise;
+
+  // and that a missing title correctly gets reported as null.
+  place = {
+    uri: NetUtil.newURI(TEST_DOMAIN + "test_visit_title"),
+    visits: [
+      new VisitInfo(),
+    ],
+    guid: "fghijklmnopq",
+  };
+  visitPromise = new Promise(resolve => {
+    let visitObserver = new VisitObserver(place.uri, place.guid,
+                                          function(aVisitDate,
+                                                   aTransitionType,
+                                                   aLastKnownTitle) {
+      do_check_eq(null, aLastKnownTitle);
+
+      PlacesUtils.history.removeObserver(visitObserver);
+      resolve();
+    });
+    PlacesUtils.history.addObserver(visitObserver, false);
+  });
+  yield promiseUpdatePlaces(place);
+  yield visitPromise;
+});
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -729,16 +729,24 @@ var PlacesProvider = {
   onEndUpdateBatch() {
     this._batchProcessingDepth -= 1;
     if (this._batchProcessingDepth == 0 && this._batchCalledFrecencyChanged) {
       this.onManyFrecenciesChanged();
       this._batchCalledFrecencyChanged = false;
     }
   },
 
+  onVisit(aURI, aVisitId, aTime, aSessionId, aReferrerVisitId, aTransitionType,
+          aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle) {
+    // For new visits, if we're not batch processing, notify for a title // update
+    if (!this._batchProcessingDepth && aVisitCount == 1 && aLastKnownTitle) {
+      this.onTitleChanged(aURI, aLastKnownTitle, aGuid);
+    }
+  },
+
   onDeleteURI: function PlacesProvider_onDeleteURI(aURI, aGUID, aReason) {
     // let observers remove sensetive data associated with deleted visit
     this._callObservers("onDeleteURI", {
       url: aURI.spec,
     });
   },
 
   onClearHistory() {