Bug 737846 - Ensure favicons service doesn't add unwanted pages to history.
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 27 Mar 2012 15:28:14 +0200
changeset 92226 033512dd3678ea54fb3d08dff4124d752b3412bd
parent 92225 27898323401e207f30f0b13dd9e0f1132d07f76d
child 92227 26dbed3d9a8704059683d95bc6f2ee586b7bab27
push idunknown
push userunknown
push dateunknown
bugs737846
milestone14.0a1
Bug 737846 - Ensure favicons service doesn't add unwanted pages to history. r=dietrich
toolkit/components/places/AsyncFaviconHelpers.cpp
toolkit/components/places/mozIAsyncFavicons.idl
toolkit/components/places/nsFaviconService.cpp
toolkit/components/places/nsIFaviconService.idl
toolkit/components/places/tests/chrome/Makefile.in
toolkit/components/places/tests/chrome/history_post.sjs
toolkit/components/places/tests/chrome/test_favicon_annotations.xul
toolkit/components/places/tests/chrome/test_history_post.xul
toolkit/components/places/tests/favicons/test_expireAllFavicons.js
toolkit/components/places/tests/favicons/test_favicons.js
toolkit/components/places/tests/favicons/test_favicons_conversions.js
toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js
toolkit/components/places/tests/head_common.js
toolkit/components/places/tests/inline/head_autocomplete.js
toolkit/components/places/tests/inline/test_autocomplete_functional.js
toolkit/components/places/tests/inline/test_typed.js
--- a/toolkit/components/places/AsyncFaviconHelpers.cpp
+++ b/toolkit/components/places/AsyncFaviconHelpers.cpp
@@ -789,47 +789,25 @@ AsyncAssociateIconToPage::Run()
     // Get the new icon id.  Do this regardless mIcon.id, since other code
     // could have added a entry before us.  Indeed we interrupted the thread
     // after the previous call to FetchIconInfo.
     mIcon.status = (mIcon.status & ~(ICON_STATUS_CACHED)) | ICON_STATUS_SAVED;
     rv = FetchIconInfo(mDB, mIcon);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
-  // If the page does not have an id, try to insert a new one.
+  // If the page does not have an id, don't try to insert a new one, cause we
+  // don't know where the page comes from.  Not doing so we may end adding
+  // a page that otherwise we'd explicitly ignore, like a POST or an error page.
   if (mPage.id == 0) {
-    nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
-      "INSERT INTO moz_places (url, rev_host, hidden, favicon_id, frecency, guid) "
-      "VALUES (:page_url, :rev_host, 1, :favicon_id, 0, GENERATE_GUID()) "
-    );
-    NS_ENSURE_STATE(stmt);
-    mozStorageStatementScoper scoper(stmt);
-    rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mPage.spec);
-    NS_ENSURE_SUCCESS(rv, rv);
-    // The rev_host can be null.
-    if (mPage.revHost.IsEmpty()) {
-      rv = stmt->BindNullByName(NS_LITERAL_CSTRING("rev_host"));
-    }
-    else {
-      rv = stmt->BindStringByName(NS_LITERAL_CSTRING("rev_host"), mPage.revHost);
-    }
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("favicon_id"), mIcon.id);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = stmt->Execute();
-    NS_ENSURE_SUCCESS(rv, rv);
+    return NS_OK;
+  }
 
-    // Get the new id and GUID.
-    rv = FetchPageInfo(mDB, mPage);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    mIcon.status |= ICON_STATUS_ASSOCIATED;
-  }
   // Otherwise just associate the icon to the page, if needed.
-  else if (mPage.iconId != mIcon.id) {
+  if (mPage.iconId != mIcon.id) {
     nsCOMPtr<mozIStorageStatement> stmt;
     if (mPage.id) {
       stmt = mDB->GetStatement(
         "UPDATE moz_places SET favicon_id = :icon_id WHERE id = :page_id"
       );
       NS_ENSURE_STATE(stmt);
       rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPage.id);
       NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/mozIAsyncFavicons.idl
+++ b/toolkit/components/places/mozIAsyncFavicons.idl
@@ -55,18 +55,18 @@ interface mozIAsyncFavicons : nsISupport
    * attempts to fetch and save the icon data by loading the favicon URI
    * through an async network request.
    *
    * If the icon data already exists, we won't try to reload the icon unless
    * aForceReload is true.  Similarly, if the icon is in the failed favicon
    * cache we won't do anything unless aForceReload is true, in which case
    * we'll try to reload the favicon.
    *
-   * This function will only save favicons for "good" URIs, as defined by what
-   * gets added to history or is a bookmark.  For "bad" URIs, this function
+   * This function will only save favicons for pages that are already stored in
+   * the database, like visited pages or bookmarks.  For any other URIs, it
    * will succeed but do nothing.  This function will also ignore the error
    * page favicon URI (see FAVICON_ERRORPAGE_URL below).
    *
    * Icons that fail to load will automatically be added to the failed favicon
    * cache, and this function will not save favicons for non-bookmarked URIs
    * when history is disabled.
    *
    * @note This function is identical to
--- a/toolkit/components/places/nsFaviconService.cpp
+++ b/toolkit/components/places/nsFaviconService.cpp
@@ -315,18 +315,21 @@ nsFaviconService::SetFaviconUrlForPage(n
       NS_ASSERTION(hasResult, "hasResult is false but the call succeeded?");
       iconId = getInfoStmt->AsInt64(0);
     }
   }
 
   // Now, link our icon entry with the page.
   PRInt64 pageId;
   nsCAutoString guid;
-  rv = history->GetOrCreateIdForPage(aPageURI, &pageId, guid);
+  rv = history->GetIdForPage(aPageURI, &pageId, guid);
   NS_ENSURE_SUCCESS(rv, rv);
+  if (!pageId) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
 
   nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
     "UPDATE moz_places SET favicon_id = :icon_id WHERE id = :page_id"
   );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
 
   rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), pageId);
--- a/toolkit/components/places/nsIFaviconService.idl
+++ b/toolkit/components/places/nsIFaviconService.idl
@@ -45,29 +45,30 @@ interface nsIFaviconDataCallback;
 [scriptable, uuid(2cf188f4-3c96-4bca-b668-36b25aaf7c1d)]
 interface nsIFaviconService : nsISupports
 {
   /**
    * Declares that a given page uses a favicon with the given URI.
    *
    * Will create an entry linking the favicon URI to the page, regardless
    * of whether we have data for that icon.  You can populate it later with
-   * SetFaviconData.  However, remember that any favicons not associated with a
-   * visited web page, a bookmark, or a "place:" URI, will be removed during
-   * expiration runs.
+   * SetFaviconData.  However, remember that favicons must only be associated
+   * with a visited web page, a bookmark, or a "place:" URI.  Trying to
+   * associate the icon to any other page will throw.
    *
    * This will send out history pageChanged notification if the new favicon has
    * any data and it's different from the old associated favicon.  This means
    * that you should try to set data before calling this method if you have it,
    * otherwise it won't fire any notifications.
    *
    * @param aPageURI
    *        URI of the page whose favicon is being set.
    * @param aFaviconURI
    *        URI of the favicon to associate with the page.
+   * @throws NS_ERROR_NOT_AVAILABLE if aPageURI doesn't exist in the database.
    */
   void setFaviconUrlForPage(in nsIURI aPageURI,
                             in nsIURI aFaviconURI);
 
   /**
    * Same as SetFaviconUrlForPage except that this also attempts to fetch and
    * save the icon data by loading the favicon URI through an async network
    * request.
--- a/toolkit/components/places/tests/chrome/Makefile.in
+++ b/toolkit/components/places/tests/chrome/Makefile.in
@@ -56,17 +56,18 @@ include $(topsrcdir)/config/rules.mk
 _CHROME_FILES	= \
 		test_371798.xul \
 		test_342484.xul \
 		test_341972a.xul \
 		test_341972b.xul \
 		test_favicon_annotations.xul \
 		test_303567.xul \
 		test_381357.xul \
+		test_history_post.xul \
+		history_post.sjs \
 		test_reloadLivemarks.xul \
 		$(NULL)
 
 libs:: $(_HTTP_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
 
 libs:: $(_CHROME_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
-
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/chrome/history_post.sjs
@@ -0,0 +1,6 @@
+function handleRequest(request, response)
+{
+  response.setStatusLine("1.0", 200, "OK");
+  response.setHeader("Content-Type", "text/plain; charset=utf-8", false);
+  response.write("Ciao");
+}
--- a/toolkit/components/places/tests/chrome/test_favicon_annotations.xul
+++ b/toolkit/components/places/tests/chrome/test_favicon_annotations.xul
@@ -130,42 +130,63 @@ function loadNextTest()
   }
 
   let img = document.getElementById("favicon");
   img.setAttribute("src", nextURI());
 }
 
 function test()
 {
+  SimpleTest.waitForExplicitFinish();
   let db = Cc["@mozilla.org/browser/nav-history-service;1"].
            getService(Ci.nsPIPlacesDatabase).
            DBConnection;
 
   // Empty any old favicons
   db.executeSimpleSQL("DELETE FROM moz_favicons");
 
   let ios = Cc["@mozilla.org/network/io-service;1"].
             getService(Ci.nsIIOService);
   let uri = function(aSpec) {
     return ios.newURI(aSpec, null, null);
   };
 
-  // Set the favicon data.  Note that the "moz-anno:" protocol requires the
-  // favicon to be stored in the database, but the replaceFaviconDataFromDataURL
-  // function will not save the favicon unless it is associated with a page.
-  // Thus, we must associate the icon with a page explicitly in order for it to
-  // be visible through the protocol.
-  fs.replaceFaviconDataFromDataURL(uri(testURIs[1]), expectedURIs[1],
-                                   (Date.now() + 60 * 60 * 24 * 1000) * 1000);
-  fs.setAndFetchFaviconForPage(uri("http://example.com/favicon_annotations"),
-                               uri(testURIs[1]), true);
+  let pageURI = uri("http://example.com/favicon_annotations");
+  let history = Cc["@mozilla.org/browser/history;1"]
+                  .getService(Ci.mozIAsyncHistory);
+  history.updatePlaces(
+    {
+      uri: pageURI,
+      visits: [{ transitionType: Ci.nsINavHistoryService.TRANSITION_TYPED,
+                 visitDate: Date.now() * 1000
+              }],
+    },
+    {
+      handleError: function UP_handleError() {
+        ok(false, "Unexpected error in adding visit.");
+      },
+      handleResult: function () {},
+      handleCompletion: function UP_handleCompletion() {
+        // Set the favicon data.  Note that the "moz-anno:" protocol requires
+        // the favicon to be stored in the database, but the
+        // replaceFaviconDataFromDataURL function will not save the favicon
+        // unless it is associated with a page.  Thus, we must associate the
+        // icon with a page explicitly in order for it to be visible through
+        // the protocol.
+        fs.replaceFaviconDataFromDataURL(uri(testURIs[1]), expectedURIs[1],
+                                         (Date.now() + 60 * 60 * 24 * 1000) * 1000);
+        fs.setAndFetchFaviconForPage(pageURI, uri(testURIs[1]), true);
 
-  // And start our test process.
-  loadNextTest();
-  SimpleTest.waitForExplicitFinish();
+        // And start our test process.
+        loadNextTest();
+      }
+    }
+  });
+
+
 }
 
   ]]>
   </script>
 
   <body xmlns="http://www.w3.org/1999/xhtml">
     <img id="favicon" onload="loadEventHandler();"/>
     <p id="display"></p>
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/chrome/test_history_post.xul
@@ -0,0 +1,67 @@
+<?xml version="1.0"?>
+
+<!-- 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/.  -->
+
+<window title="Test post pages are not added to history"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+        onload="test();">
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js"/>
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"/>
+
+  <script type="application/javascript">
+  <![CDATA[
+
+    const Cc = Components.classes;
+    const Ci = Components.interfaces;
+
+    Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
+    Components.utils.import("resource://gre/modules/NetUtil.jsm");
+
+    const SJS_URI = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/chrome/history_post.sjs");
+
+    function test()
+    {
+      SimpleTest.waitForExplicitFinish();
+      let submit = document.getElementById("submit");
+      submit.click();
+      let iframe = document.getElementById("post_iframe");
+      iframe.addEventListener("load", function onLoad() {
+        iframe.removeEventListener("load", onLoad);
+        let history = Cc["@mozilla.org/browser/history;1"]
+                        .getService(Ci.mozIAsyncHistory);
+        history.isURIVisited(SJS_URI, function (aURI, aIsVisited) {
+          ok(!aIsVisited, "The POST page should not be added to history");
+
+          let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
+                                  .DBConnection;
+          let stmt = db.createStatement(
+            "SELECT 1 FROM moz_places WHERE url = :page_url"
+          );
+          stmt.params.page_url = SJS_URI.spec;
+          ok(!stmt.executeStep(), "The page should not be in the database");
+          stmt.finalize();
+          SimpleTest.finish();
+        });
+      });
+    }
+
+  ]]>
+  </script>
+
+  <body xmlns="http://www.w3.org/1999/xhtml">
+    <iframe name="post_iframe" id="post_iframe"/>
+    <form method="post" action="http://mochi.test:8888/tests/toolkit/components/places/tests/chrome/history_post.sjs" target="post_iframe">
+      <input type="submit" id="submit"/>
+    </form>
+    <p id="display"></p>
+    <div id="content" style="display:none;"></div>
+    <pre id="test"></pre>
+  </body>
+</window>
--- a/toolkit/components/places/tests/favicons/test_expireAllFavicons.js
+++ b/toolkit/components/places/tests/favicons/test_expireAllFavicons.js
@@ -3,20 +3,16 @@
 
 /**
  * This file tests that favicons are correctly expired by expireAllFavicons.
  */
 
 ////////////////////////////////////////////////////////////////////////////////
 /// Globals
 
-XPCOMUtils.defineLazyServiceGetter(this, "gHistory",
-                                   "@mozilla.org/browser/history;1",
-                                   "mozIAsyncHistory");
-
 const TEST_PAGE_URI = NetUtil.newURI("http://example.com/");
 const BOOKMARKED_PAGE_URI = NetUtil.newURI("http://example.com/bookmarked");
 
 ////////////////////////////////////////////////////////////////////////////////
 /// Tests
 
 function run_test() {
   run_next_test();
@@ -31,36 +27,25 @@ add_test(function test_expireAllFavicons
     checkFaviconMissingForPage(TEST_PAGE_URI, function () {
       checkFaviconMissingForPage(BOOKMARKED_PAGE_URI, function () {
         run_next_test();
       });
     });
   }, PlacesUtils.TOPIC_FAVICONS_EXPIRED, false);
 
   // Add a visited page.
-  gHistory.updatePlaces({
-    uri: TEST_PAGE_URI,
-    visits: [{
-      transitionType: Ci.nsINavHistoryService.TRANSITION_TYPED,
-      visitDate: Date.now() * 1000
-    }]
-  }, {
-    handleError: function EAF_handleError(aResultCode, aPlaceInfo) {
-      do_throw("Unexpected error: " + aResultCode);
-    },
-    handleResult: function EAF_handleResult(aPlaceInfo) {
-      PlacesUtils.favicons.setAndFetchFaviconForPage(TEST_PAGE_URI,
-                                                     SMALLPNG_DATA_URI, true);
+  addVisits({ uri: TEST_PAGE_URI, transition: TRANSITION_TYPED }, function () {
+    PlacesUtils.favicons.setAndFetchFaviconForPage(TEST_PAGE_URI,
+                                                   SMALLPNG_DATA_URI, true);
 
-      // Add a page with a bookmark.
-      PlacesUtils.bookmarks.insertBookmark(
-                            PlacesUtils.toolbarFolderId, BOOKMARKED_PAGE_URI,
-                            PlacesUtils.bookmarks.DEFAULT_INDEX,
-                            "Test bookmark");
-      PlacesUtils.favicons.setAndFetchFaviconForPage(
-        BOOKMARKED_PAGE_URI, SMALLPNG_DATA_URI, true,
-        function EAF_onFaviconDataAvailable() {
-          // Start expiration only after data has been saved in the database.
-          PlacesUtils.favicons.expireAllFavicons();
-        });
-    }
+    // Add a page with a bookmark.
+    PlacesUtils.bookmarks.insertBookmark(
+                          PlacesUtils.toolbarFolderId, BOOKMARKED_PAGE_URI,
+                          PlacesUtils.bookmarks.DEFAULT_INDEX,
+                          "Test bookmark");
+    PlacesUtils.favicons.setAndFetchFaviconForPage(
+      BOOKMARKED_PAGE_URI, SMALLPNG_DATA_URI, true,
+      function EAF_onFaviconDataAvailable() {
+        // Start expiration only after data has been saved in the database.
+        PlacesUtils.favicons.expireAllFavicons();
+      });
   });
 });
--- a/toolkit/components/places/tests/favicons/test_favicons.js
+++ b/toolkit/components/places/tests/favicons/test_favicons.js
@@ -299,12 +299,21 @@ add_test(function test_insert_asynchrono
           new SingleGUIDCallback(function (again) {
             do_check_eq(guid, again);
             run_next_test();
           }));
       });
   }));
 });
 
+add_test(function test_setFaviconURLForPage_nonexistingPage_throws() {
+  try {
+    PlacesUtils.favicons.setFaviconUrlForPage(
+      NetUtil.newURI("http://nonexisting.moz.org"), icons[0].uri);
+    do_throw("Setting an icon for a nonexisting page should throw")
+  } catch (ex if ex.result == Cr.NS_ERROR_NOT_AVAILABLE) {}
+  run_next_test();
+});
+
 /*
  * TODO: need a test for async write that modifies a GUID for a favicon.
  * This will come later, when there's an API that actually does that!
  */
--- a/toolkit/components/places/tests/favicons/test_favicons_conversions.js
+++ b/toolkit/components/places/tests/favicons/test_favicons_conversions.js
@@ -29,36 +29,38 @@ let isWindows = ("@mozilla.org/windows-r
  *        Windows and should not be checked on that platform.
  * @param aCallback
  *        This function is called after the check finished.
  */
 function checkFaviconDataConversion(aFileName, aFileMimeType, aFileLength,
                                     aExpectConversion, aVaryOnWindows,
                                     aCallback) {
   let pageURI = NetUtil.newURI("http://places.test/page/" + aFileName);
-  let faviconURI = NetUtil.newURI("http://places.test/icon/" + aFileName);
-  let fileData = readFileOfLength(aFileName, aFileLength);
+  addVisits({ uri: pageURI, transition: TRANSITION_TYPED }, function () {
+    let faviconURI = NetUtil.newURI("http://places.test/icon/" + aFileName);
+    let fileData = readFileOfLength(aFileName, aFileLength);
 
-  PlacesUtils.favicons.replaceFaviconData(faviconURI, fileData, fileData.length,
-                                          aFileMimeType);
-  PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, faviconURI, true,
-    function CFDC_verify(aURI, aDataLen, aData, aMimeType) {
-      if (!aExpectConversion) {
-        do_check_true(compareArrays(aData, fileData));
-        do_check_eq(aMimeType, aFileMimeType);
-      } else {
-        if (!aVaryOnWindows || !isWindows) {
-          let expectedFile = do_get_file("expected-" + aFileName + ".png");
-          do_check_true(compareArrays(aData, readFileData(expectedFile)));
+    PlacesUtils.favicons.replaceFaviconData(faviconURI, fileData, fileData.length,
+                                            aFileMimeType);
+    PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, faviconURI, true,
+      function CFDC_verify(aURI, aDataLen, aData, aMimeType) {
+        if (!aExpectConversion) {
+          do_check_true(compareArrays(aData, fileData));
+          do_check_eq(aMimeType, aFileMimeType);
+        } else {
+          if (!aVaryOnWindows || !isWindows) {
+            let expectedFile = do_get_file("expected-" + aFileName + ".png");
+            do_check_true(compareArrays(aData, readFileData(expectedFile)));
+          }
+          do_check_eq(aMimeType, "image/png");
         }
-        do_check_eq(aMimeType, "image/png");
-      }
 
-      aCallback();
-    });
+        aCallback();
+      });
+  });
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 /// Tests
 
 function run_test() {
   run_next_test();
 }
--- a/toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
+++ b/toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
@@ -22,22 +22,23 @@ function run_test()
   run_next_test();
 }
 
 add_test(function test_normal()
 {
   let pageURI = NetUtil.newURI("http://example.com/normal");
   waitForFaviconChanged(pageURI, FAVICON_URI,
                         function test_normal_callback() {
-    do_check_true(isUrlHidden(pageURI));
-    do_check_eq(frecencyForUrl(pageURI), 0);
     checkFaviconDataForPage(pageURI, FAVICON_MIMETYPE, FAVICON_DATA,
                             run_next_test);
   });
-  PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, FAVICON_URI, true);
+
+  addVisits({ uri: pageURI, transition: TRANSITION_TYPED}, function () {
+    PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, FAVICON_URI, true);
+  });
 });
 
 add_test(function test_aboutURI_bookmarked()
 {
   let pageURI = NetUtil.newURI("about:testAboutURI_bookmarked");
   waitForFaviconChanged(pageURI, FAVICON_URI,
                         function test_aboutURI_bookmarked_callback() {
     checkFaviconDataForPage(pageURI, FAVICON_MIMETYPE, FAVICON_DATA,
--- a/toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js
+++ b/toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage_failures.js
@@ -86,58 +86,79 @@ add_test(function test_aboutURI()
 
 add_test(function test_privateBrowsing_nonBookmarkedURI()
 {
   if (!("@mozilla.org/privatebrowsing;1" in Cc)) {
     run_next_test();
     return;
   }
 
-  let pb = Cc["@mozilla.org/privatebrowsing;1"]
-           .getService(Ci.nsIPrivateBrowsingService);
-  Services.prefs.setBoolPref("browser.privatebrowsing.keep_current_session",
-                             true);
-  pb.privateBrowsingEnabled = true;
+  let pageURI = NetUtil.newURI("http://example.com/privateBrowsing");
+  addVisits({ uri: pageURI, transitionType: TRANSITION_TYPED }, function () {
+    let pb = Cc["@mozilla.org/privatebrowsing;1"]
+             .getService(Ci.nsIPrivateBrowsingService);
+    Services.prefs.setBoolPref("browser.privatebrowsing.keep_current_session",
+                               true);
+    pb.privateBrowsingEnabled = true;
 
-  PlacesUtils.favicons.setAndFetchFaviconForPage(
-                       NetUtil.newURI("http://example.com/privateBrowsing"),
-                       FAVICON_URI, true);
+    PlacesUtils.favicons.setAndFetchFaviconForPage(
+                         pageURI,
+                         FAVICON_URI, true);
 
-  // The setAndFetchFaviconForPage function calls CanAddURI synchronously,
-  // thus we can exit Private Browsing Mode immediately.
-  pb.privateBrowsingEnabled = false;
-  Services.prefs.clearUserPref("browser.privatebrowsing.keep_current_session");
-  run_next_test();
+    // The setAndFetchFaviconForPage function calls CanAddURI synchronously,
+    // thus we can exit Private Browsing Mode immediately.
+    pb.privateBrowsingEnabled = false;
+    Services.prefs.clearUserPref("browser.privatebrowsing.keep_current_session");
+    run_next_test();
+  });
 });
 
 add_test(function test_disabledHistory()
 {
-  Services.prefs.setBoolPref("places.history.enabled", false);
-
-  PlacesUtils.favicons.setAndFetchFaviconForPage(
-                       NetUtil.newURI("http://example.com/disabledHistory"),
-                       FAVICON_URI, true);
+  let pageURI = NetUtil.newURI("http://example.com/disabledHistory");
+  addVisits({ uri: pageURI, transition: TRANSITION_TYPED }, function () {
+    Services.prefs.setBoolPref("places.history.enabled", false);
 
-  // The setAndFetchFaviconForPage function calls CanAddURI synchronously, thus
-  // we can set the preference back to true immediately . We don't clear the
-  // preference because not all products enable Places by default.
-  Services.prefs.setBoolPref("places.history.enabled", true);
-  run_next_test();
+    PlacesUtils.favicons.setAndFetchFaviconForPage(
+                         pageURI,
+                         FAVICON_URI, true);
+
+    // The setAndFetchFaviconForPage function calls CanAddURI synchronously, thus
+    // we can set the preference back to true immediately . We don't clear the
+    // preference because not all products enable Places by default.
+    Services.prefs.setBoolPref("places.history.enabled", true);
+    run_next_test();
+  });
 });
 
 add_test(function test_errorIcon()
 {
+  let pageURI = NetUtil.newURI("http://example.com/errorIcon");
+  let places = [{ uri: pageURI, transition: TRANSITION_TYPED }];
+  addVisits({ uri: pageURI, transition: TRANSITION_TYPED }, function () {
+    PlacesUtils.favicons.setAndFetchFaviconForPage(
+                         pageURI,
+                         FAVICON_ERRORPAGE_URI, true);
+
+    run_next_test();
+  });
+});
+
+add_test(function test_nonexistingPage()
+{
   PlacesUtils.favicons.setAndFetchFaviconForPage(
-                       NetUtil.newURI("http://example.com/errorIcon"),
-                       FAVICON_ERRORPAGE_URI, true);
+                       NetUtil.newURI("http://example.com/nonexistingPage"),
+                       FAVICON_URI, true);
 
   run_next_test();
 });
 
 add_test(function test_finalVerification()
 {
   // This is the only test that should cause the waitForFaviconChanged callback
   // we set up earlier to be invoked.  In turn, the callback will invoke
   // run_next_test() causing the tests to finish.
-  PlacesUtils.favicons.setAndFetchFaviconForPage(
-                       LAST_PAGE_URI,
-                       LAST_FAVICON_URI, true);
+  addVisits({ uri: LAST_PAGE_URI, transition: TRANSITION_TYPED }, function () {
+    PlacesUtils.favicons.setAndFetchFaviconForPage(
+                         LAST_PAGE_URI,
+                         LAST_FAVICON_URI, true);
+  });
 });
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -865,8 +865,66 @@ NavHistoryResultObserver.prototype = {
   nodeTagsChanged: function () {},
   nodeTitleChanged: function () {},
   nodeURIChanged: function () {},
   sortingChanged: function () {},
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsINavHistoryResultObserver,
   ])
 };
+
+/**
+ * Asynchronously adds visits to a page, invoking a callback function when done.
+ *
+ * @param aPlaceInfo
+ *        Can be an nsIURI, in such a case a single LINK visit will be added.
+ *        Otherwise can be an object describing the visit to add, or an array
+ *        of these objects:
+ *          { uri: nsIURI of the page,
+ *            transition: one of the TRANSITION_* from nsINavHistoryService,
+ *            [optional] title: title of the page,
+ *            [optional] visitDate: visit date in microseconds from the epoch
+ *          }
+ * @param [optional] aCallback
+ *        Function to be invoked on completion.
+ * @param [optional] aStack
+ *        The stack frame used to report errors.
+ */
+function addVisits(aPlaceInfo, aCallback, aStack)
+{
+  let stack = aStack || Components.stack.caller;
+  let places = [];
+  if (aPlaceInfo instanceof Ci.nsIURI) {
+    places.push({ uri: aPlaceInfo });
+  }
+  else if (Array.isArray(aPlaceInfo)) {
+    places = places.concat(aPlaceInfo);
+  } else {
+    places.push(aPlaceInfo)
+  }
+
+  // Create mozIVisitInfo for each entry.
+  let now = Date.now();
+  for (let i = 0; i < places.length; i++) {
+    if (!places[i].title) {
+      places[i].title = "test visit for " + places[i].uri.spec;
+    }
+    places[i].visits = [{
+      transitionType: places[i].transition === undefined ? TRANSITION_LINK
+                                                         : places[i].transition,
+      visitDate: places[i].visitDate || (now++) * 1000
+    }];
+  }
+
+  PlacesUtils.asyncHistory.updatePlaces(
+    places,
+    {
+      handleError: function AAV_handleError() {
+        do_throw("Unexpected error in adding visit.", stack);
+      },
+      handleResult: function () {},
+      handleCompletion: function UP_handleCompletion() {
+        if (aCallback)
+          aCallback();
+      }
+    }
+  );
+}
--- a/toolkit/components/places/tests/inline/head_autocomplete.js
+++ b/toolkit/components/places/tests/inline/head_autocomplete.js
@@ -188,31 +188,8 @@ function addBookmark(aBookmarkObj) {
                           .insertBookmark(parentId,
                                           NetUtil.newURI(aBookmarkObj.url),
                                           PlacesUtils.bookmarks.DEFAULT_INDEX,
                                           "A bookmark");
   if (aBookmarkObj.keyword) {
     PlacesUtils.bookmarks.setKeywordForBookmark(itemId, aBookmarkObj.keyword);
   }
 }
-
-function VisitInfo(aTransitionType, aVisitTime)
-{
-  this.transitionType =
-    aTransitionType === undefined ? TRANSITION_LINK : aTransitionType;
-  this.visitDate = aVisitTime || Date.now() * 1000;
-}
-
-function addVisits(aUrls)
-{
-  let places = [];
-  aUrls.forEach(function(url) {
-    places.push({
-                  uri: url.url,
-                  title: "test for " + url.url,
-                  visits: [
-                    new VisitInfo(url.transition),
-                  ],
-    });
-  });
-
-  gHistory.updatePlaces(places);
-}
--- a/toolkit/components/places/tests/inline/test_autocomplete_functional.js
+++ b/toolkit/components/places/tests/inline/test_autocomplete_functional.js
@@ -5,144 +5,101 @@
 // Functional tests for inline autocomplete
 
 add_autocomplete_test([
   "Add urls, check for correct order",
   "vis",
   "visit2.mozilla.org/",
   function ()
   {
-    let urls = [{ url: NetUtil.newURI("http://visit1.mozilla.org"),
-                  transition: undefined,
-                },
-                { url: NetUtil.newURI("http://visit2.mozilla.org"),
-                  transition: TRANSITION_TYPED,
-                }];
-    addVisits(urls);
+    let places = [{ uri: NetUtil.newURI("http://visit1.mozilla.org") },
+                  { uri: NetUtil.newURI("http://visit2.mozilla.org"),
+                    transition: TRANSITION_TYPED }];
+    addVisits(places);
   }
 ]);
 
 add_autocomplete_test([
   "Add urls, make sure www and http are ignored",
   "visit1",
   "visit1.mozilla.org/",
   function ()
   {
-
-    let urls = [{ url: NetUtil.newURI("http://www.visit1.mozilla.org"),
-                  transition: undefined,
-                },
-               ];
-    addVisits(urls);
+    addVisits(NetUtil.newURI("http://www.visit1.mozilla.org"));
   }
 ]);
 
 add_autocomplete_test([
   "Autocompleting after an existing host completes to the url",
   "visit3.mozilla.org/",
   "visit3.mozilla.org/",
   function ()
   {
-
-    let urls = [{ url: NetUtil.newURI("http://www.visit3.mozilla.org"),
-                  transition: undefined,
-                },
-               ];
-    addVisits(urls);
+    addVisits(NetUtil.newURI("http://www.visit3.mozilla.org"));
   }
 ]);
 
 add_autocomplete_test([
   "Searching for www.me should yield www.me.mozilla.org/",
   "www.me",
   "www.me.mozilla.org/",
   function ()
   {
-
-    let urls = [{ url: NetUtil.newURI("http://www.me.mozilla.org"),
-                  transition: undefined,
-                },
-               ];
-    addVisits(urls);
+    addVisits(NetUtil.newURI("http://www.me.mozilla.org"));
   }
 ]);
 
 add_autocomplete_test([
   "With a bookmark and history, the query result should be the bookmark",
   "bookmark",
   "bookmark1.mozilla.org/",
   function ()
   {
-
-    let urls = [{ url: NetUtil.newURI("http://bookmark1.mozilla.org/foo"),
-                  transition: undefined,
-                },
-               ];
     addBookmark({ url: "http://bookmark1.mozilla.org/", });
-    addVisits(urls);
+    addVisits(NetUtil.newURI("http://bookmark1.mozilla.org/foo"));
   }
 ]);
 
 add_autocomplete_test([
   "Check to make sure we get the proper results with full paths",
   "smokey",
   "smokey.mozilla.org/",
   function ()
   {
 
-    let urls = [{ url: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=delicious"),
-                  transition: undefined,
-                },
-                { url: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=smokey"),
-                  transition: undefined,
-                },
-               ];
-    addVisits(urls);
+    let places = [{ uri: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=delicious") },
+                  { uri: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=smokey") }];
+    addVisits(places);
   }
 ]);
 
 add_autocomplete_test([
   "Check to make sure we autocomplete to the following '/'",
   "smokey.mozilla.org/fo",
   "smokey.mozilla.org/foo/",
   function ()
   {
 
-    let urls = [{ url: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=delicious"),
-                  transition: undefined,
-                },
-                { url: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=smokey"),
-                  transition: undefined,
-                },
-               ];
-    addVisits(urls);
+    let places = [{ uri: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=delicious") },
+                  { uri: NetUtil.newURI("http://smokey.mozilla.org/foo/bar/baz?bacon=smokey") }];
+    addVisits(places);
   }
 ]);
 
 add_autocomplete_test([
   "Check to make sure we autocomplete after ?",
   "smokey.mozilla.org/foo?",
   "smokey.mozilla.org/foo?bacon=delicious",
   function ()
   {
-
-    let urls = [{ url: NetUtil.newURI("http://smokey.mozilla.org/foo?bacon=delicious"),
-                  transition: undefined,
-                },
-               ];
-    addVisits(urls);
+    addVisits(NetUtil.newURI("http://smokey.mozilla.org/foo?bacon=delicious"));
   }
 ]);
 
 add_autocomplete_test([
   "Check to make sure we autocomplete after #",
   "smokey.mozilla.org/foo?bacon=delicious#bar",
   "smokey.mozilla.org/foo?bacon=delicious#bar",
   function ()
   {
-
-    let urls = [{ url: NetUtil.newURI("http://smokey.mozilla.org/foo?bacon=delicious#bar"),
-                  transition: undefined,
-                },
-               ];
-    addVisits(urls);
+    addVisits(NetUtil.newURI("http://smokey.mozilla.org/foo?bacon=delicious#bar"));
   }
 ]);
--- a/toolkit/components/places/tests/inline/test_typed.js
+++ b/toolkit/components/places/tests/inline/test_typed.js
@@ -6,75 +6,65 @@
 // ensure autocomplete is able to dinamically switch behavior.
 
 add_autocomplete_test([
   "Searching for domain should autoFill it",
   "moz",
   "mozilla.org/",
   function () {
     Services.prefs.setBoolPref("browser.urlbar.autoFill.typed", false);
-    addVisits([ { url: NetUtil.newURI("http://mozilla.org/link/")
-                , transition: TRANSITION_LINK }
-              ]);
+    addVisits(NetUtil.newURI("http://mozilla.org/link/"));
   }
 ]);
 
 add_autocomplete_test([
   "Searching for url should autoFill it",
   "mozilla.org/li",
   "mozilla.org/link/",
   function () {
     Services.prefs.setBoolPref("browser.urlbar.autoFill.typed", false);
-    addVisits([ { url: NetUtil.newURI("http://mozilla.org/link/")
-                , transition: TRANSITION_LINK }
-              ]);
+    addVisits(NetUtil.newURI("http://mozilla.org/link/"));
   }
 ]);
 
 // Now do searches with typed behavior forced to true.
 
 add_autocomplete_test([
   "Searching for non-typed domain should not autoFill it",
   "moz",
   "moz",
   function () {
     Services.prefs.setBoolPref("browser.urlbar.autoFill.typed", true);
-    addVisits([ { url: NetUtil.newURI("http://mozilla.org/link/")
-                , transition: TRANSITION_LINK }
-              ]);
+    addVisits(NetUtil.newURI("http://mozilla.org/link/"));
   }
 ]);
 
 add_autocomplete_test([
   "Searching for typed domain should autoFill it",
   "moz",
   "mozilla.org/",
   function () {
     Services.prefs.setBoolPref("browser.urlbar.autoFill.typed", true);
-    addVisits([ { url: NetUtil.newURI("http://mozilla.org/typed/")
-                , transition: TRANSITION_TYPED }
-              ]);
+    addVisits({ uri: NetUtil.newURI("http://mozilla.org/typed/"),
+                transition: TRANSITION_TYPED });
   }
 ]);
 
 add_autocomplete_test([
   "Searching for non-typed url should not autoFill it",
   "mozilla.org/li",
   "mozilla.org/li",
   function () {
     Services.prefs.setBoolPref("browser.urlbar.autoFill.typed", true);
-    addVisits([ { url: NetUtil.newURI("http://mozilla.org/link/")
-                , transition: TRANSITION_LINK }
-              ]);
+    addVisits(NetUtil.newURI("http://mozilla.org/link/"));
   }
 ]);
 
 add_autocomplete_test([
   "Searching for typed url should autoFill it",
   "mozilla.org/li",
   "mozilla.org/link/",
   function () {
     Services.prefs.setBoolPref("browser.urlbar.autoFill.typed", true);
-    addVisits([ { url: NetUtil.newURI("http://mozilla.org/link/")
-                , transition: TRANSITION_TYPED }
-              ]);
+    addVisits({ uri: NetUtil.newURI("http://mozilla.org/link/"),
+                transition: TRANSITION_TYPED });
   }
 ]);