Bug 606966 - Need an async history visit API exposed to JS

Part 22 - Dispatch errors for URIs that fail nsINavHistory::canAddURI
r=mak
a=blocking

toolkit/components/places/src/History.cpp

@@ -520,6 +520,58 @@ private:
   const nsresult mResult;
 };
 
+/**
+ * Checks to see if we can add aURI to history, and dispatches an error to
+ * aCallback (if provided) if we cannot.
+ *
+ * @param aURI
+ *        The URI to check.
+ * @param [optional] aGUID
+ *        The guid of the URI to check.  This is passed back to the callback.
+ * @param [optional] aPlaceId
+ *        The placeId of the URI to check.  This is passed back to the callback.
+ * @param [optional] aCallback
+ *        The callback to notify if the URI cannot be added to history.
+ * @return true if the URI can be added to history, false otherwise.
+ */
+bool
+CanAddURI(nsIURI* aURI,
+          const nsCString& aGUID = EmptyCString(),
+          PRInt64 aPlaceId = 0,
+          mozIVisitInfoCallback* aCallback = NULL)
+{
+  nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
+  NS_ENSURE_TRUE(navHistory, false);
+
+  PRBool canAdd;
+  nsresult rv = navHistory->CanAddURI(aURI, &canAdd);
+  if (NS_SUCCEEDED(rv) && canAdd) {
+    return true;
+  };
+
+  // We cannot add the URI.  Notify the callback, if we were given one.
+  if (aCallback) {
+    // NotifyCompletion does not hold a strong reference to the callback, so we
+    // have to manage it by AddRefing now and then releasing it after the event
+    // has run.
+    NS_ADDREF(aCallback);
+
+    VisitData place(aURI);
+    place.guid = aGUID;
+    place.placeId = aPlaceId;
+    nsCOMPtr<nsIRunnable> event =
+      new NotifyCompletion(aCallback, place, NS_ERROR_INVALID_ARG);
+    (void)NS_DispatchToMainThread(event);
+
+    // Also dispatch an event to release our reference to the callback after
+    // NotifyCompletion has run.
+    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
+    (void)NS_ProxyRelease(mainThread, aCallback, PR_TRUE);
+  }
+
+  return false;
+}
+
 /**
  * Adds a visit to the database.
  */
@@ -624,6 +676,13 @@ private:
       if (mPlaces[i].sessionId <= 0) {
         mPlaces[i].sessionId = navHistory->GetNewSessionID();
       }
+
+#ifdef DEBUG
+      nsCOMPtr<nsIURI> uri;
+      (void)NS_NewURI(getter_AddRefs(uri), mPlaces[i].spec);
+      NS_ASSERTION(CanAddURI(uri),
+                   "Passed a VisitData with a URI we cannot add to history!");
+#endif
     }
 
     // We AddRef on the main thread, and release it when we are destroyed.
@@ -1787,6 +1846,12 @@ History::UpdatePlaces(const jsval& aPlaceInfos,
       }
     }
 
+    // Make sure that any uri we are given can be added to history, and if not,
+    // skip it (CanAddURI will notify our callback for us).
+    if (uri && !CanAddURI(uri, guid, placeId, aCallback)) {
+      continue;
+    }
+
     // We must have at least one of uri, valid id, or guid.
     NS_ENSURE_ARG(uri || placeId > 0 || !guid.IsVoid());
 
@@ -1868,11 +1933,16 @@ History::UpdatePlaces(const jsval& aPlaceInfos,
     }
   }
 
-  mozIStorageConnection* dbConn = GetDBConn();
-  NS_ENSURE_STATE(dbConn);
+  // It is possible that all of the visits we were passed were dissallowed by
+  // CanAddURI, which isn't an error.  If we have no visits to add, however,
+  // we should not call InsertVisitedURIs::Start.
+  if (visitData.Length()) {
+    mozIStorageConnection* dbConn = GetDBConn();
+    NS_ENSURE_STATE(dbConn);
 
-  nsresult rv = InsertVisitedURIs::Start(dbConn, visitData, aCallback);
-  NS_ENSURE_SUCCESS(rv, rv);
+    nsresult rv = InsertVisitedURIs::Start(dbConn, visitData, aCallback);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
 
   return NS_OK;
 }

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

@@ -302,6 +302,55 @@ function test_add_visit_invalid_transitionType_throws()
   run_next_test();
 }
 
+function test_non_addable_uri_errors()
+{
+  // Array of protocols that nsINavHistoryService::canAddURI returns false for.
+  const URLS = [
+    "about:config",
+    "imap://cyrus.andrew.cmu.edu/archive.imap",
+    "news://new.mozilla.org/mozilla.dev.apps.firefox",
+    "mailbox:Inbox",
+    "moz-anno:favicon:http://mozilla.org/made-up-favicon",
+    "view-source:http://mozilla.org",
+    "chrome://browser/content/browser.xul",
+    "resource://gre-resources/hiddenWindow.html",
+    "data:,Hello%2C%20World!",
+    "wyciwyg:/0/http://mozilla.org",
+    "javascript:alert('hello wolrd!');",
+  ];
+  let places = [];
+  URLS.forEach(function(url) {
+    try {
+      let place = {
+        uri: NetUtil.newURI(url),
+        title: "test for " + url,
+        visits: [
+          new VisitInfo(),
+        ],
+      };
+      places.push(place);
+    }
+    catch (e if e.result === Cr.NS_ERROR_MALFORMED_URI) {
+      // NetUtil.newURI() can throw if e.g. our app knows about imap://
+      // but the account is not set up and so the URL is invalid for us.
+      // Note this in the log but ignore as it's not the subject of this test.
+      do_log_info("Could not construct URI for '" + url + "'; ignoring");
+    }
+  });
+
+  let callbackCount = 0;
+  gHistory.updatePlaces(places, function(aResultCode, aPlaceInfo) {
+    do_log_info("Checking '" + aPlaceInfo.uri.spec + "'");
+    do_check_eq(aResultCode, Cr.NS_ERROR_INVALID_ARG);
+    do_check_false(gGlobalHistory.isVisited(aPlaceInfo.uri));
+
+    // If we have had all of our callbacks, continue running tests.
+    if (++callbackCount == places.length) {
+      run_next_test();
+    }
+  });
+}
+
 function test_add_visit()
 {
   const VISIT_TIME = Date.now() * 1000;
@@ -639,6 +688,7 @@ let gTests = [
   test_add_visit_no_date_throws,
   test_add_visit_no_transitionType_throws,
   test_add_visit_invalid_transitionType_throws,
+  test_non_addable_uri_errors,
   test_add_visit,
   test_properties_saved,
   test_guid_saved,

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

@@ -82,19 +82,20 @@ function run_test() {
   // check that certain schemes never show up as visited
   // even if we attempt to add them to history
   // see CanAddURI() in nsNavHistory.cpp
-  var urlsToIgnore = [
+  const URLS = [
     "about:config",
-    "data:,Hello%2C%20World!",
     "imap://cyrus.andrew.cmu.edu/archive.imap",
-    "news://news.mozilla.org/mozilla.dev.apps.firefox",
-    "moz-anno:favicon:http://www.mozilla.org/2005/made-up-favicon/84-1321",
+    "news://new.mozilla.org/mozilla.dev.apps.firefox",
+    "mailbox:Inbox",
+    "moz-anno:favicon:http://mozilla.org/made-up-favicon",
+    "view-source:http://mozilla.org",
     "chrome://browser/content/browser.xul",
-    "view-source:http://www.google.com/",
-    "javascript:alert('hello world!');",
     "resource://gre-resources/hiddenWindow.html",
+    "data:,Hello%2C%20World!",
+    "wyciwyg:/0/http://mozilla.org",
+    "javascript:alert('hello wolrd!');",
   ];
-
-  for each (var currentURL in urlsToIgnore) {
+  URLS.forEach(function(currentURL) {
     try {
       var cantAddUri = uri(currentURL);
     }
@@ -102,11 +103,11 @@ function run_test() {
       // nsIIOService.newURI() can throw if e.g. our app knows about imap://
       // but the account is not set up and so the URL is invalid for us.
       // Note this in the log but ignore as it's not the subject of this test.
-      print("Exception thrown for '" + currentURL + "', ignored.");
+      do_log_info("Could not construct URI for '" + currentURL + "'; ignoring");
     }
     if (cantAddUri) {
       add_uri_to_history(cantAddUri, false);
       do_check_false(gh.isVisited(cantAddUri));
     }
-  }
+  });
 }