Bug 672681 - Make nsIDownloadHistory::addDownload asynchronous. r=mak

toolkit/components/places/History.cpp

@@ -45,6 +45,7 @@
 #include "History.h"
 #include "nsNavHistory.h"
 #include "nsNavBookmarks.h"
+#include "nsAnnotationService.h"
 #include "Helpers.h"
 #include "PlaceInfo.h"
 #include "VisitInfo.h"
@@ -79,6 +80,11 @@ namespace places {
 // Observer event fired after a visit has been registered in the DB.
 #define URI_VISIT_SAVED "uri-visit-saved"
 
+#define DESTINATIONFILEURI_ANNO \
+        NS_LITERAL_CSTRING("downloads/destinationFileURI")
+#define DESTINATIONFILENAME_ANNO \
+        NS_LITERAL_CSTRING("downloads/destinationFileName")
+
 ////////////////////////////////////////////////////////////////////////////////
 //// VisitData
 
@@ -1255,6 +1261,109 @@ private:
   nsRefPtr<History> mHistory;
 };
 
+/**
+ * Adds download-specific annotations to a download page.
+ */
+class SetDownloadAnnotations : public mozIVisitInfoCallback
+{
+public:
+  NS_DECL_ISUPPORTS
+
+  SetDownloadAnnotations(nsIURI* aDestination)
+  : mDestination(aDestination)
+  , mHistory(History::GetService())
+  {
+    MOZ_ASSERT(mDestination);
+    MOZ_ASSERT(NS_IsMainThread());
+  }
+
+  NS_IMETHOD HandleError(nsresult aResultCode, mozIPlaceInfo *aPlaceInfo)
+  {
+    // Just don't add the annotations in case the visit isn't added.
+    return NS_OK;
+  }
+
+  NS_IMETHOD HandleResult(mozIPlaceInfo *aPlaceInfo)
+  {
+    // Exit silently if the download destination is not a local file.
+    nsCOMPtr<nsIFileURL> destinationFileURL = do_QueryInterface(mDestination);
+    if (!destinationFileURL) {
+      return NS_OK;
+    }
+
+    nsCOMPtr<nsIURI> source;
+    nsresult rv = aPlaceInfo->GetUri(getter_AddRefs(source));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsCOMPtr<nsIFile> destinationFile;
+    rv = destinationFileURL->GetFile(getter_AddRefs(destinationFile));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsAutoString destinationFileName;
+    rv = destinationFile->GetLeafName(destinationFileName);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsCAutoString destinationURISpec;
+    rv = destinationFileURL->GetSpec(destinationURISpec);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // Use annotations for storing the additional download metadata.
+    nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
+    NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
+
+    rv = annosvc->SetPageAnnotationString(
+      source,
+      DESTINATIONFILEURI_ANNO,
+      NS_ConvertUTF8toUTF16(destinationURISpec),
+      0,
+      nsIAnnotationService::EXPIRE_WITH_HISTORY
+    );
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv = annosvc->SetPageAnnotationString(
+      source,
+      DESTINATIONFILENAME_ANNO,
+      destinationFileName,
+      0,
+      nsIAnnotationService::EXPIRE_WITH_HISTORY
+    );
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsAutoString title;
+    rv = aPlaceInfo->GetTitle(title);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // In case we are downloading a file that does not correspond to a web
+    // page for which the title is present, we populate the otherwise empty
+    // history title with the name of the destination file, to allow it to be
+    // visible and searchable in history results.
+    if (title.IsEmpty()) {
+      rv = mHistory->SetURITitle(source, destinationFileName);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    return NS_OK;
+  }
+
+  NS_IMETHOD HandleCompletion()
+  {
+    return NS_OK;
+  }
+
+private:
+  nsCOMPtr<nsIURI> mDestination;
+
+  /**
+   * Strong reference to the History object because we do not want it to
+   * disappear out from under us.
+   */
+  nsRefPtr<History> mHistory;
+};
+NS_IMPL_ISUPPORTS1(
+  SetDownloadAnnotations,
+  mozIVisitInfoCallback
+)
+
 /**
  * Stores an embed visit, and notifies observers.
  *
@@ -1905,6 +2014,65 @@ History::SetURITitle(nsIURI* aURI, const nsAString& aTitle)
   return NS_OK;
 }
 
+////////////////////////////////////////////////////////////////////////////////
+//// nsIDownloadHistory
+
+NS_IMETHODIMP
+History::AddDownload(nsIURI* aSource, nsIURI* aReferrer,
+                     PRTime aStartTime, nsIURI* aDestination)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  NS_ENSURE_ARG(aSource);
+
+  if (mShuttingDown) {
+    return NS_OK;
+  }
+
+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
+    NS_ERROR("Cannot add downloads to history from content process!");
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
+  nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
+  NS_ENSURE_TRUE(navHistory, NS_ERROR_OUT_OF_MEMORY);
+
+  // Silently return if URI is something we shouldn't add to DB.
+  bool canAdd;
+  nsresult rv = navHistory->CanAddURI(aSource, &canAdd);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!canAdd) {
+    return NS_OK;
+  }
+
+  nsTArray<VisitData> placeArray(1);
+  NS_ENSURE_TRUE(placeArray.AppendElement(VisitData(aSource, aReferrer)),
+                 NS_ERROR_OUT_OF_MEMORY);
+  VisitData& place = placeArray.ElementAt(0);
+  NS_ENSURE_FALSE(place.spec.IsEmpty(), NS_ERROR_INVALID_ARG);
+
+  place.visitTime = aStartTime;
+  place.SetTransitionType(nsINavHistoryService::TRANSITION_DOWNLOAD);
+
+  mozIStorageConnection* dbConn = GetDBConn();
+  NS_ENSURE_STATE(dbConn);
+
+  nsCOMPtr<mozIVisitInfoCallback> callback = aDestination
+                                  ? new SetDownloadAnnotations(aDestination)
+                                  : nsnull;
+
+  rv = InsertVisitedURIs::Start(dbConn, placeArray, callback);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // Finally, notify that we've been visited.
+  nsCOMPtr<nsIObserverService> obsService =
+    mozilla::services::GetObserverService();
+  if (obsService) {
+    obsService->NotifyObservers(aSource, NS_LINK_VISITED_EVENT_TOPIC, nsnull);
+  }
+
+  return NS_OK;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 //// mozIAsyncHistory
 
@@ -2101,9 +2269,10 @@ History::Observe(nsISupports* aSubject, const char* aTopic,
 ////////////////////////////////////////////////////////////////////////////////
 //// nsISupports
 
-NS_IMPL_THREADSAFE_ISUPPORTS3(
+NS_IMPL_THREADSAFE_ISUPPORTS4(
   History
 , IHistory
+, nsIDownloadHistory
 , mozIAsyncHistory
 , nsIObserver
 )

toolkit/components/places/History.h

@@ -42,6 +42,7 @@
 
 #include "mozilla/IHistory.h"
 #include "mozIAsyncHistory.h"
+#include "nsIDownloadHistory.h"
 #include "Database.h"
 
 #include "mozilla/dom/Link.h"
@@ -62,12 +63,14 @@ struct VisitData;
   {0x0937a705, 0x91a6, 0x417a, {0x82, 0x92, 0xb2, 0x2e, 0xb1, 0x0d, 0xa8, 0x6c}}
 
 class History : public IHistory
+              , public nsIDownloadHistory
               , public mozIAsyncHistory
               , public nsIObserver
 {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_IHISTORY
+  NS_DECL_NSIDOWNLOADHISTORY
   NS_DECL_MOZIASYNCHISTORY
   NS_DECL_NSIOBSERVER
 

toolkit/components/places/nsNavHistory.cpp

@@ -151,14 +151,6 @@ static const PRInt64 USECS_PER_DAY = LL_INIT(20, 500654080);
 // character-set annotation
 #define CHARSET_ANNO NS_LITERAL_CSTRING("URIProperties/characterSet")
 
-// Download destination file URI annotation
-#define DESTINATIONFILEURI_ANNO \
-  NS_LITERAL_CSTRING("downloads/destinationFileURI")
-
-// Download destination file name annotation
-#define DESTINATIONFILENAME_ANNO \
-  NS_LITERAL_CSTRING("downloads/destinationFileName")
-
 // These macros are used when splitting history by date.
 // These are the day containers and catch-all final container.
 #define HISTORY_ADDITIONAL_DATE_CONT_NUM 3
@@ -219,7 +211,6 @@ NS_IMPL_CLASSINFO(nsNavHistory, NULL, nsIClassInfo::SINGLETON,
 NS_INTERFACE_MAP_BEGIN(nsNavHistory)
   NS_INTERFACE_MAP_ENTRY(nsINavHistoryService)
   NS_INTERFACE_MAP_ENTRY(nsIGlobalHistory2)
-  NS_INTERFACE_MAP_ENTRY(nsIDownloadHistory)
   NS_INTERFACE_MAP_ENTRY(nsIBrowserHistory)
   NS_INTERFACE_MAP_ENTRY(nsIObserver)
   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
@@ -231,11 +222,10 @@ NS_INTERFACE_MAP_BEGIN(nsNavHistory)
 NS_INTERFACE_MAP_END
 
 // We don't care about flattening everything
-NS_IMPL_CI_INTERFACE_GETTER4(
+NS_IMPL_CI_INTERFACE_GETTER3(
   nsNavHistory
 , nsINavHistoryService
 , nsIGlobalHistory2
-, nsIDownloadHistory
 , nsIBrowserHistory
 )
 
@@ -3783,80 +3773,6 @@ nsNavHistory::OnEndVacuum(bool aSucceeded)
 }
 
 
-////////////////////////////////////////////////////////////////////////////////
-//// nsIDownloadHistory
-
-NS_IMETHODIMP
-nsNavHistory::AddDownload(nsIURI* aSource, nsIURI* aReferrer,
-                          PRTime aStartTime, nsIURI* aDestination)
-{
-  NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
-  NS_ENSURE_ARG(aSource);
-
-  // don't add when history is disabled and silently fail
-  if (IsHistoryDisabled())
-    return NS_OK;
-
-  PRInt64 visitID;
-  nsresult rv = AddVisit(aSource, aStartTime, aReferrer, TRANSITION_DOWNLOAD,
-                         false, 0, &visitID);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (!aDestination) {
-    return NS_OK;
-  }
-
-  // Exit silently if the download destination is not a local file.
-  nsCOMPtr<nsIFileURL> destinationFileURL = do_QueryInterface(aDestination);
-  if (!destinationFileURL) {
-    return NS_OK;
-  }
-
-  nsCOMPtr<nsIFile> destinationFile;
-  rv = destinationFileURL->GetFile(getter_AddRefs(destinationFile));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  nsAutoString destinationFileName;
-  rv = destinationFile->GetLeafName(destinationFileName);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  nsCAutoString destinationURISpec;
-  rv = destinationFileURL->GetSpec(destinationURISpec);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // Use annotations for storing the additional download metadata.
-  nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
-  NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
-
-  (void)annosvc->SetPageAnnotationString(
-    aSource,
-    DESTINATIONFILEURI_ANNO,
-    NS_ConvertUTF8toUTF16(destinationURISpec),
-    0,
-    nsIAnnotationService::EXPIRE_WITH_HISTORY
-  );
-
-  (void)annosvc->SetPageAnnotationString(
-    aSource,
-    DESTINATIONFILENAME_ANNO,
-    destinationFileName,
-    0,
-    nsIAnnotationService::EXPIRE_WITH_HISTORY
-  );
-
-  // In case we are downloading a file that does not correspond to a web
-  // page for which the title is present, we populate the otherwise empty
-  // history title with the name of the destination file, to allow it to be
-  // visible and searchable in history results.
-  nsAutoString title;
-  if (NS_SUCCEEDED(GetPageTitle(aSource, title)) && title.IsEmpty()) {
-    (void)SetPageTitle(aSource, destinationFileName);
-  }
-
-  return NS_OK;
-}
-
-
 ////////////////////////////////////////////////////////////////////////////////
 //// nsPIPlacesDatabase
 

toolkit/components/places/nsNavHistory.h

@@ -47,7 +47,6 @@
 #include "nsPIPlacesHistoryListenersNotifier.h"
 #include "nsIBrowserHistory.h"
 #include "nsIGlobalHistory.h"
-#include "nsIDownloadHistory.h"
 #include "nsINavBookmarksService.h"
 #include "nsIPrivateBrowsingService.h"
 #include "nsIFaviconService.h"
@@ -109,7 +108,6 @@ class nsNavHistory : public nsSupportsWeakReference
                    , public nsINavHistoryService
                    , public nsIObserver
                    , public nsIBrowserHistory
-                   , public nsIDownloadHistory
                    , public nsPIPlacesDatabase
                    , public nsPIPlacesHistoryListenersNotifier
                    , public mozIStorageVacuumParticipant
@@ -122,7 +120,6 @@ public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSINAVHISTORYSERVICE
   NS_DECL_NSIGLOBALHISTORY2
-  NS_DECL_NSIDOWNLOADHISTORY
   NS_DECL_NSIBROWSERHISTORY
   NS_DECL_NSIOBSERVER
   NS_DECL_NSPIPLACESDATABASE

toolkit/components/places/nsPlacesModule.cpp

@@ -64,7 +64,6 @@ const mozilla::Module::CIDEntry kPlacesCIDs[] = {
 const mozilla::Module::ContractIDEntry kPlacesContracts[] = {
   { NS_NAVHISTORYSERVICE_CONTRACTID, &kNS_NAVHISTORYSERVICE_CID },
   { NS_GLOBALHISTORY2_CONTRACTID, &kNS_NAVHISTORYSERVICE_CID },
-  { NS_DOWNLOADHISTORY_CONTRACTID, &kNS_NAVHISTORYSERVICE_CID },
   { NS_ANNOTATIONSERVICE_CONTRACTID, &kNS_ANNOTATIONSERVICE_CID },
   { NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "moz-anno", &kNS_ANNOPROTOCOLHANDLER_CID },
   { NS_NAVBOOKMARKSSERVICE_CONTRACTID, &kNS_NAVBOOKMARKSSERVICE_CID },
@@ -74,6 +73,7 @@ const mozilla::Module::ContractIDEntry kPlacesContracts[] = {
   { NS_IHISTORY_CONTRACTID, &kNS_ANDROIDHISTORY_CID },
 #else
   { NS_IHISTORY_CONTRACTID, &kNS_HISTORYSERVICE_CID },
+  { NS_DOWNLOADHISTORY_CONTRACTID, &kNS_HISTORYSERVICE_CID },
 #endif
   { NS_PLACESIMPORTEXPORTSERVICE_CONTRACTID, &kNS_PLACESIMPORTEXPORTSERVICE_CID },
   { NULL }

toolkit/components/places/tests/head_common.js

@@ -736,3 +736,24 @@ function do_compare_arrays(a1, a2, sorted)
            a2.filter(function (e) a1.indexOf(e) == -1).length == 0;
   }
 }
+
+/**
+ * Generic nsINavHistoryObserver that doesn't implement anything, but provides
+ * dummy methods to prevent errors about an object not having a certain method.
+ */
+function NavHistoryObserver() {}
+
+NavHistoryObserver.prototype = {
+  onBeginUpdateBatch: function () {},
+  onEndUpdateBatch: function () {},
+  onVisit: function () {},
+  onTitleChanged: function () {},
+  onBeforeDeleteURI: function () {},
+  onDeleteURI: function () {},
+  onClearHistory: function () {},
+  onPageChanged: function () {},
+  onDeleteVisits: function () {},
+  QueryInterface: XPCOMUtils.generateQI([
+    Ci.nsINavHistoryObserver,
+  ])
+};

toolkit/components/places/tests/unit/test_async_history_api.js

@@ -40,29 +40,6 @@ function VisitInfo(aTransitionType,
   this.visitDate = aVisitTime || Date.now() * 1000;
 }
 
-/**
- * Generic nsINavHistoryObserver that doesn't implement anything, but provides
- * dummy methods to prevent errors about an object not having a certain method.
- */
-function NavHistoryObserver()
-{
-}
-NavHistoryObserver.prototype =
-{
-  onBeginUpdateBatch: function() { },
-  onEndUpdateBatch: function() { },
-  onVisit: function() { },
-  onTitleChanged: function() { },
-  onBeforeDeleteURI: function() { },
-  onDeleteURI: function() { },
-  onClearHistory: function() { },
-  onPageChanged: function() { },
-  onDeleteVisits: function() { },
-  QueryInterface: XPCOMUtils.generateQI([
-    Ci.nsINavHistoryObserver,
-  ]),
-};
-
 /**
  * Listens for a title change notification, and calls aCallback when it gets it.
  *

toolkit/components/places/tests/unit/test_download_history.js

@@ -8,30 +8,34 @@
 ////////////////////////////////////////////////////////////////////////////////
 /// Globals
 
-const downloadHistory = Cc["@mozilla.org/browser/download-history;1"]
+XPCOMUtils.defineLazyServiceGetter(this, "gDownloadHistory",
-                        .getService(Ci.nsIDownloadHistory);
+                                   "@mozilla.org/browser/download-history;1",
+                                   "nsIDownloadHistory");
 
-const TEST_URI = NetUtil.newURI("http://google.com/");
+XPCOMUtils.defineLazyServiceGetter(this, "gHistory",
-const REFERRER_URI = NetUtil.newURI("http://yahoo.com");
+                                   "@mozilla.org/browser/history;1",
+                                   "mozIAsyncHistory");
 
-const NS_LINK_VISITED_EVENT_TOPIC = "link-visited";
+const DOWNLOAD_URI = NetUtil.newURI("http://www.example.com/");
-const ENABLE_HISTORY_PREF = "places.history.enabled";
+const REFERRER_URI = NetUtil.newURI("http://www.example.org/");
-const PB_KEEP_SESSION_PREF = "browser.privatebrowsing.keep_current_session";
+const PRIVATE_URI = NetUtil.newURI("http://www.example.net/");
 
 /**
- * Sets a flag when the link visited notification is received.
+ * Waits for the first visit notification to be received.
+ *
+ * @param aCallback
+ *        This function is called with the same arguments of onVisit.
  */
-const visitedObserver = {
+function waitForOnVisit(aCallback) {
-  topicReceived: false,
+  let historyObserver = {
-  observe: function VO_observe(aSubject, aTopic, aData)
+    __proto__: NavHistoryObserver.prototype,
-  {
+    onVisit: function HO_onVisit() {
-    this.topicReceived = true;
+      PlacesUtils.history.removeObserver(this);
-  }
+      aCallback.apply(null, arguments);
-};
+    }
-Services.obs.addObserver(visitedObserver, NS_LINK_VISITED_EVENT_TOPIC, false);
+  };
-do_register_cleanup(function() {
+  PlacesUtils.history.addObserver(historyObserver, false);
-  Services.obs.removeObserver(visitedObserver, NS_LINK_VISITED_EVENT_TOPIC);
+}
-});
 
 /**
  * Checks to see that a URI is in the database.
@@ -59,15 +63,6 @@ function uri_in_db(aURI, aExpected)
   root.containerOpen = false;
 }
 
-/**
- * Cleanup function called by each individual test if necessary.
- */
-function cleanup_and_run_next_test()
-{
-  visitedObserver.topicReceived = false;
-  waitForClearHistory(run_next_test);
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 /// Tests
 
@@ -79,72 +74,113 @@ function run_test()
 add_test(function test_dh_is_from_places()
 {
   // Test that this nsIDownloadHistory is the one places implements.
-  do_check_true(downloadHistory instanceof Ci.nsINavHistoryService);
+  do_check_true(gDownloadHistory instanceof Ci.mozIAsyncHistory);
 
-  run_next_test();
+  waitForClearHistory(run_next_test);
 });
 
 add_test(function test_dh_addDownload()
 {
-  // Sanity checks.
+  waitForOnVisit(function DHAD_onVisit(aURI) {
-  uri_in_db(TEST_URI, false);
+    do_check_true(aURI.equals(DOWNLOAD_URI));
-  uri_in_db(REFERRER_URI, false);
 
-  downloadHistory.addDownload(TEST_URI, REFERRER_URI, Date.now() * 1000);
+    // Verify that the URI is already available in results at this time.
+    uri_in_db(DOWNLOAD_URI, true);
 
-  do_check_true(visitedObserver.topicReceived);
+    waitForClearHistory(run_next_test);
-  uri_in_db(TEST_URI, true);
+  });
-  uri_in_db(REFERRER_URI, true);
 
-  cleanup_and_run_next_test();
+  gDownloadHistory.addDownload(DOWNLOAD_URI, null, Date.now() * 1000);
 });
 
-add_test(function test_dh_privateBrowsing()
+add_test(function test_dh_addDownload_referrer()
 {
-  // Sanity checks.
+  waitForOnVisit(function DHAD_prepareReferrer(aURI, aVisitID) {
-  uri_in_db(TEST_URI, false);
+    do_check_true(aURI.equals(REFERRER_URI));
-  uri_in_db(REFERRER_URI, false);
+    let referrerVisitId = aVisitID;
 
-  var pb = null;
+    waitForOnVisit(function DHAD_onVisit(aURI, aVisitID, aTime, aSessionID,
-  try {
+                                              aReferringID) {
-    // PrivateBrowsing component is not always available to Places implementers.
+      do_check_true(aURI.equals(DOWNLOAD_URI));
-     pb = Cc["@mozilla.org/privatebrowsing;1"].
+      do_check_eq(aReferringID, referrerVisitId);
-          getService(Ci.nsIPrivateBrowsingService);
+
-  } catch (ex) {
+      // Verify that the URI is already available in results at this time.
-    // Skip this test.
+      uri_in_db(DOWNLOAD_URI, true);
+
+      waitForClearHistory(run_next_test);
+    });
+
+    gDownloadHistory.addDownload(DOWNLOAD_URI, REFERRER_URI, Date.now() * 1000);
+  });
+
+  // Note that we don't pass the optional callback argument here because we must
+  // ensure that we receive the onVisit notification before we call addDownload.
+  gHistory.updatePlaces({
+    uri: REFERRER_URI,
+    visits: [{
+      transitionType: Ci.nsINavHistoryService.TRANSITION_TYPED,
+      visitDate: Date.now() * 1000
+    }]
+  });
+});
+
+add_test(function test_dh_addDownload_privateBrowsing()
+{
+  if (!("@mozilla.org/privatebrowsing;1" in Cc)) {
+    todo(false, "PB service is not available, bail out");
     run_next_test();
     return;
   }
-  Services.prefs.setBoolPref(PB_KEEP_SESSION_PREF, true);
+
+  waitForOnVisit(function DHAD_onVisit(aURI) {
+    // We should only receive the notification for the non-private URI.  This
+    // test is based on the assumption that visit notifications are received in
+    // the same order of the addDownload calls, which is currently true because
+    // database access is serialized on the same worker thread.
+    do_check_true(aURI.equals(DOWNLOAD_URI));
+
+    uri_in_db(DOWNLOAD_URI, true);
+    uri_in_db(PRIVATE_URI, false);
+
+    waitForClearHistory(run_next_test);
+  });
+
+  let pb = Cc["@mozilla.org/privatebrowsing;1"]
+           .getService(Ci.nsIPrivateBrowsingService);
+  Services.prefs.setBoolPref("browser.privatebrowsing.keep_current_session",
+                             true);
   pb.privateBrowsingEnabled = true;
+  gDownloadHistory.addDownload(PRIVATE_URI, REFERRER_URI, Date.now() * 1000);
 
-  downloadHistory.addDownload(TEST_URI, REFERRER_URI, Date.now() * 1000);
+  // The addDownload functions calls CanAddURI synchronously, thus we can exit
-
+  // Private Browsing Mode immediately.
-  do_check_false(visitedObserver.topicReceived);
-  uri_in_db(TEST_URI, false);
-  uri_in_db(REFERRER_URI, false);
-
   pb.privateBrowsingEnabled = false;
-  cleanup_and_run_next_test();
+  Services.prefs.clearUserPref("browser.privatebrowsing.keep_current_session");
+  gDownloadHistory.addDownload(DOWNLOAD_URI, REFERRER_URI, Date.now() * 1000);
 });
 
-add_test(function test_dh_disabledHistory()
+add_test(function test_dh_addDownload_disabledHistory()
 {
-  // Sanity checks.
+  waitForOnVisit(function DHAD_onVisit(aURI) {
-  uri_in_db(TEST_URI, false);
+    // We should only receive the notification for the non-private URI.  This
-  uri_in_db(REFERRER_URI, false);
+    // test is based on the assumption that visit notifications are received in
+    // the same order of the addDownload calls, which is currently true because
+    // database access is serialized on the same worker thread.
+    do_check_true(aURI.equals(DOWNLOAD_URI));
 
-  // Disable history.
+    uri_in_db(DOWNLOAD_URI, true);
-  Services.prefs.setBoolPref(ENABLE_HISTORY_PREF, false);
+    uri_in_db(PRIVATE_URI, false);
 
-  downloadHistory.addDownload(TEST_URI, REFERRER_URI, Date.now() * 1000);
+    waitForClearHistory(run_next_test);
+  });
 
-  do_check_false(visitedObserver.topicReceived);
+  Services.prefs.setBoolPref("places.history.enabled", false);
-  uri_in_db(TEST_URI, false);
+  gDownloadHistory.addDownload(PRIVATE_URI, REFERRER_URI, Date.now() * 1000);
-  uri_in_db(REFERRER_URI, false);
 
-  Services.prefs.setBoolPref(ENABLE_HISTORY_PREF, true);
+  // The addDownload functions calls CanAddURI synchronously, thus we can reset
-  cleanup_and_run_next_test();
+  // the preference immediately.
+  Services.prefs.clearUserPref("places.history.enabled");
+  gDownloadHistory.addDownload(DOWNLOAD_URI, REFERRER_URI, Date.now() * 1000);
 });
 
 /**
@@ -170,7 +206,7 @@ add_test(function test_dh_details()
       PlacesUtils.annotations.removeObserver(annoObserver);
       PlacesUtils.history.removeObserver(historyObserver);
 
-      cleanup_and_run_next_test();
+      waitForClearHistory(run_next_test);
     }
   };
 
@@ -214,17 +250,18 @@ add_test(function test_dh_details()
     onDeleteURI: function() {},
     onClearHistory: function() {},
     onPageChanged: function() {},
-    onDeleteVisits: function() {}   
+    onDeleteVisits: function() {}
   };
 
   PlacesUtils.annotations.addObserver(annoObserver, false);
   PlacesUtils.history.addObserver(historyObserver, false);
 
   // Both null values and remote URIs should not cause errors.
-  downloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000);
+  gDownloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000);
-  downloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000, null);
+  gDownloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000, null);
-  downloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000, REMOTE_URI);
+  gDownloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000, REMOTE_URI);
 
   // Valid local file URIs should cause the download details to be saved.
-  downloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000, destFileUri);
+  gDownloadHistory.addDownload(SOURCE_URI, null, Date.now() * 1000,
+                               destFileUri);
 });