Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric draft
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 26 Jan 2016 18:43:53 +0100
changeset 325843 9b7794cc6d81f8de6c43b303779835f06eca7271
parent 325842 54bf7536f7006f279889b0abf9ab7deeb532e63c
child 513517 7702a810fbdc67a4b9d0b7fc374a6ef7f34e0f1c
push id10058
push usermak77@bonardo.net
push dateTue, 26 Jan 2016 17:47:42 +0000
reviewersYoric
bugs1240013
milestone47.0a1
Bug 1240013 - Crash in nsNavBookmarks::OnVisit by setting long location.hash. r=Yoric
toolkit/components/places/History.cpp
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/nsNavHistoryResult.h
toolkit/components/places/tests/unit/test_null_interfaces.js
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -91,18 +91,21 @@ struct VisitData {
   , hidden(true)
   , typed(false)
   , transitionType(UINT32_MAX)
   , visitTime(0)
   , frecency(-1)
   , titleChanged(false)
   , shouldUpdateFrecency(true)
   {
-    (void)aURI->GetSpec(spec);
-    (void)GetReversedHostname(aURI, revHost);
+    MOZ_ASSERT(aURI);
+    if (aURI) {
+      (void)aURI->GetSpec(spec);
+      (void)GetReversedHostname(aURI, revHost);
+    }
     if (aReferrer) {
       (void)aReferrer->GetSpec(referrerSpec);
     }
     guid.SetIsVoid(true);
     title.SetIsVoid(true);
   }
 
   /**
@@ -642,17 +645,20 @@ public:
 
     nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
     if (!navHistory) {
       NS_WARNING("Trying to notify about a visit but cannot get the history service!");
       return NS_OK;
     }
 
     nsCOMPtr<nsIURI> uri;
-    (void)NS_NewURI(getter_AddRefs(uri), mPlace.spec);
+    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), mPlace.spec)));
+    if (!uri) {
+      return NS_ERROR_UNEXPECTED;
+    }
 
     // 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,
                                 mReferrer.visitId, mPlace.transitionType,
                                 mPlace.guid, mPlace.hidden);
     }
@@ -703,17 +709,21 @@ public:
 
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
 
     nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
     NS_ENSURE_TRUE(navHistory, NS_ERROR_OUT_OF_MEMORY);
     nsCOMPtr<nsIURI> uri;
-    (void)NS_NewURI(getter_AddRefs(uri), mSpec);
+    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), mSpec)));
+    if (!uri) {
+      return NS_ERROR_UNEXPECTED;
+    }
+
     navHistory->NotifyTitleChange(uri, mTitle, mGUID);
 
     return NS_OK;
   }
 private:
   const nsCString mSpec;
   const nsString mTitle;
   const nsCString mGUID;
@@ -737,23 +747,26 @@ public:
   {
     MOZ_ASSERT(aCallback, "Must pass a non-null callback!");
   }
 
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
 
+    bool hasValidURIs = true;
     nsCOMPtr<nsIURI> referrerURI;
     if (!mPlace.referrerSpec.IsEmpty()) {
-      (void)NS_NewURI(getter_AddRefs(referrerURI), mPlace.referrerSpec);
+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(referrerURI), mPlace.referrerSpec)));
+      hasValidURIs = !!referrerURI;
     }
 
     nsCOMPtr<nsIURI> uri;
-    (void)NS_NewURI(getter_AddRefs(uri), mPlace.spec);
+    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), mPlace.spec)));
+    hasValidURIs = hasValidURIs && !!uri;
 
     nsCOMPtr<mozIPlaceInfo> place;
     if (mIsSingleVisit) {
       nsCOMPtr<mozIVisitInfo> visit =
         new VisitInfo(mPlace.visitId, mPlace.visitTime, mPlace.transitionType,
                       referrerURI.forget());
       PlaceInfo::VisitsArray visits;
       (void)visits.AppendElement(visit);
@@ -766,20 +779,19 @@ public:
     }
     else {
       // Same as above.
       place =
         new PlaceInfo(mPlace.placeId, mPlace.guid, uri.forget(), mPlace.title,
                       -1);
     }
 
-    if (NS_SUCCEEDED(mResult)) {
+    if (NS_SUCCEEDED(mResult) && hasValidURIs) {
       (void)mCallback->HandleResult(place);
-    }
-    else {
+    } else {
       (void)mCallback->HandleError(mResult, place);
     }
 
     return NS_OK;
   }
 
 private:
   nsMainThreadPtrHandle<mozIVisitInfoCallback> mCallback;
@@ -991,17 +1003,17 @@ private:
     mPlaces.SwapElements(aPlaces);
     mReferrers.SetLength(mPlaces.Length());
 
     for (nsTArray<VisitData>::size_type i = 0; i < mPlaces.Length(); i++) {
       mReferrers[i].spec = mPlaces[i].referrerSpec;
 
 #ifdef DEBUG
       nsCOMPtr<nsIURI> uri;
-      (void)NS_NewURI(getter_AddRefs(uri), mPlaces[i].spec);
+      MOZ_ASSERT(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), mPlaces[i].spec)));
       NS_ASSERTION(CanAddURI(uri),
                    "Passed a VisitData with a URI we cannot add to history!");
 #endif
     }
   }
 
   /**
    * Inserts or updates the entry in moz_places for this visit, adds the visit,
@@ -1619,35 +1631,40 @@ public:
     // Wrap all notifications in a batch, so the view can handle changes in a
     // more performant way, by initiating a refresh after a limited number of
     // single changes.
     (void)navHistory->BeginUpdateBatch();
     for (auto iter = mPlaces.Iter(); !iter.Done(); iter.Next()) {
       PlaceHashKey* entry = iter.Get();
       const nsTArray<VisitData>& visits = entry->mVisits;
       nsCOMPtr<nsIURI> uri;
-      (void)NS_NewURI(getter_AddRefs(uri), visits[0].spec);
-      bool removingPage = visits.Length() == entry->VisitCount() &&
-                          !entry->IsBookmarked();
-
-      // FindRemovableVisits only sets the transition type on the VisitData
-      // objects it collects if the visits were filtered by transition type.
-      // RemoveVisitsFilter currently only supports filtering by transition
-      // type, so FindRemovableVisits will either find all visits, or all
-      // visits of a given type. Therefore, if transitionType is set on this
-      // visit, we pass the transition type to NotifyOnPageExpired which in
-      // turns passes it to OnDeleteVisits to indicate that all visits of a
-      // given type were removed.
-      uint32_t transition = visits[0].transitionType < UINT32_MAX
-                          ? visits[0].transitionType
-                          : 0;
-      navHistory->NotifyOnPageExpired(uri, visits[0].visitTime, removingPage,
-                                      visits[0].guid,
-                                      nsINavHistoryObserver::REASON_DELETED,
-                                      transition);
+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), visits[0].spec)));
+      // Notify an expiration only if we have a valid uri, otherwise
+      // the observer couldn't gather any useful data from the notification.
+      // This should be false only if there's a bug in the code preceding us.
+      if (uri) {
+        bool removingPage = visits.Length() == entry->VisitCount() &&
+                            !entry->IsBookmarked();
+
+        // FindRemovableVisits only sets the transition type on the VisitData
+        // objects it collects if the visits were filtered by transition type.
+        // RemoveVisitsFilter currently only supports filtering by transition
+        // type, so FindRemovableVisits will either find all visits, or all
+        // visits of a given type. Therefore, if transitionType is set on this
+        // visit, we pass the transition type to NotifyOnPageExpired which in
+        // turns passes it to OnDeleteVisits to indicate that all visits of a
+        // given type were removed.
+        uint32_t transition = visits[0].transitionType < UINT32_MAX
+                            ? visits[0].transitionType
+                            : 0;
+        navHistory->NotifyOnPageExpired(uri, visits[0].visitTime, removingPage,
+                                        visits[0].guid,
+                                        nsINavHistoryObserver::REASON_DELETED,
+                                        transition);
+      }
     }
     (void)navHistory->EndUpdateBatch();
 
     return NS_OK;
   }
 
 private:
   nsTHashtable<PlaceHashKey> mPlaces;
@@ -1904,17 +1921,17 @@ void
 StoreAndNotifyEmbedVisit(VisitData& aPlace,
                          mozIVisitInfoCallback* aCallback = nullptr)
 {
   MOZ_ASSERT(aPlace.transitionType == nsINavHistoryService::TRANSITION_EMBED,
              "Must only pass TRANSITION_EMBED visits to this!");
   MOZ_ASSERT(NS_IsMainThread(), "Must be called on the main thread!");
 
   nsCOMPtr<nsIURI> uri;
-  (void)NS_NewURI(getter_AddRefs(uri), aPlace.spec);
+  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), aPlace.spec)));
 
   nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
   if (!navHistory || !uri) {
     return;
   }
 
   navHistory->registerEmbedVisit(uri, aPlace.visitTime);
 
@@ -2402,34 +2419,35 @@ History::IsRecentlyVisitedURI(nsIURI* aU
 ////////////////////////////////////////////////////////////////////////////////
 //// IHistory
 
 NS_IMETHODIMP
 History::VisitURI(nsIURI* aURI,
                   nsIURI* aLastVisitedURI,
                   uint32_t aFlags)
 {
-  NS_PRECONDITION(aURI, "URI should not be NULL.");
+  NS_ENSURE_ARG(aURI);
+
   if (mShuttingDown) {
     return NS_OK;
   }
 
   if (XRE_IsContentProcess()) {
     URIParams uri;
     SerializeURI(aURI, uri);
 
     OptionalURIParams lastVisitedURI;
     SerializeURI(aLastVisitedURI, lastVisitedURI);
 
     mozilla::dom::ContentChild* cpc =
       mozilla::dom::ContentChild::GetSingleton();
     NS_ASSERTION(cpc, "Content Protocol is NULL!");
     (void)cpc->SendVisitURI(uri, lastVisitedURI, aFlags);
     return NS_OK;
-  } 
+  }
 
   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(aURI, &canAdd);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -2609,17 +2627,18 @@ History::UnregisterVisitedCallback(nsIUR
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 History::SetURITitle(nsIURI* aURI, const nsAString& aTitle)
 {
-  NS_PRECONDITION(aURI, "Must pass a non-null URI!");
+  NS_ENSURE_ARG(aURI);
+
   if (mShuttingDown) {
     return NS_OK;
   }
 
   if (XRE_IsContentProcess()) {
     URIParams uri;
     SerializeURI(aURI, uri);
 
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -621,16 +621,18 @@ nsNavBookmarks::RemoveItem(int64_t aItem
     if (bookmark.grandParentId != mTagsRoot) {
       nsNavHistory* history = nsNavHistory::GetHistoryService();
       NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
       rv = history->UpdateFrecency(bookmark.placeId);
       NS_ENSURE_SUCCESS(rv, rv);
     }
     // A broken url should not interrupt the removal process.
     (void)NS_NewURI(getter_AddRefs(uri), bookmark.url);
+    // We cannot assert since some automated tests are checking this path.
+    NS_WARN_IF_FALSE(uri, "Invalid URI in RemoveItem");
   }
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemRemoved(bookmark.id,
                                  bookmark.parentId,
                                  bookmark.position,
                                  bookmark.type,
@@ -1097,16 +1099,18 @@ nsNavBookmarks::RemoveFolderChildren(int
       if (child.grandParentId != mTagsRoot) {
         nsNavHistory* history = nsNavHistory::GetHistoryService();
         NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
         rv = history->UpdateFrecency(child.placeId);
         NS_ENSURE_SUCCESS(rv, rv);
       }
       // A broken url should not interrupt the removal process.
       (void)NS_NewURI(getter_AddRefs(uri), child.url);
+      // We cannot assert since some automated tests are checking this path.
+      NS_WARN_IF_FALSE(uri, "Invalid URI in RemoveFolderChildren");
     }
 
     NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                      nsINavBookmarkObserver,
                      OnItemRemoved(child.id,
                                    child.parentId,
                                    child.position,
                                    child.type,
@@ -2568,27 +2572,32 @@ nsNavBookmarks::GetObservers(uint32_t* _
 
   return NS_OK;
 }
 
 void
 nsNavBookmarks::NotifyItemVisited(const ItemVisitData& aData)
 {
   nsCOMPtr<nsIURI> uri;
-  (void)NS_NewURI(getter_AddRefs(uri), aData.bookmark.url);
-  NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
-                   nsINavBookmarkObserver,
-                   OnItemVisited(aData.bookmark.id,
-                                 aData.visitId,
-                                 aData.time,
-                                 aData.transitionType,
-                                 uri,
-                                 aData.bookmark.parentId,
-                                 aData.bookmark.guid,
-                                 aData.bookmark.parentGuid));
+  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), aData.bookmark.url)));
+  // Notify the visit only if we have a valid uri, otherwise the observer
+  // couldn't gather enough data from the notification.
+  // This should be false only if there's a bug in the code preceding us.
+  if (uri) {
+    NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
+                     nsINavBookmarkObserver,
+                     OnItemVisited(aData.bookmark.id,
+                                   aData.visitId,
+                                   aData.time,
+                                   aData.transitionType,
+                                   uri,
+                                   aData.bookmark.parentId,
+                                   aData.bookmark.guid,
+                                   aData.bookmark.parentGuid));
+  }
 }
 
 void
 nsNavBookmarks::NotifyItemChanged(const ItemChangeData& aData)
 {
   // A guid must always be defined.
   MOZ_ASSERT(!aData.bookmark.guid.IsEmpty());
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
@@ -2656,16 +2665,18 @@ nsNavBookmarks::OnEndUpdateBatch()
 
 
 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)
 {
+  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;
   visitData.time = aTime;
   visitData.transitionType = aTransitionType;
 
@@ -2676,16 +2687,18 @@ nsNavBookmarks::OnVisit(nsIURI* aURI, in
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnDeleteURI(nsIURI* aURI,
                             const nsACString& aGUID,
                             uint16_t aReason)
 {
+  NS_ENSURE_ARG(aURI);
+
 #ifdef DEBUG
   nsNavHistory* history = nsNavHistory::GetHistoryService();
   int64_t placeId;
   nsAutoCString placeGuid;
   MOZ_ASSERT(
     history && NS_SUCCEEDED(history->GetIdForPage(aURI, &placeId, placeGuid)) && !placeId,
     "OnDeleteURI was notified for a page that still exists?"
   );
@@ -2732,16 +2745,18 @@ nsNavBookmarks::OnManyFrecenciesChanged(
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnPageChanged(nsIURI* aURI,
                               uint32_t aChangedAttribute,
                               const nsAString& aNewValue,
                               const nsACString& aGUID)
 {
+  NS_ENSURE_ARG(aURI);
+
   nsresult rv;
   if (aChangedAttribute == nsINavHistoryObserver::ATTRIBUTE_FAVICON) {
     ItemChangeData changeData;
     rv = aURI->GetSpec(changeData.bookmark.url);
     NS_ENSURE_SUCCESS(rv, rv);
     changeData.property = NS_LITERAL_CSTRING("favicon");
     changeData.isAnnotation = false;
     changeData.newValue = NS_ConvertUTF16toUTF8(aNewValue);
@@ -2779,16 +2794,18 @@ nsNavBookmarks::OnPageChanged(nsIURI* aU
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnDeleteVisits(nsIURI* aURI, PRTime aVisitTime,
                                const nsACString& aGUID,
                                uint16_t aReason, uint32_t aTransitionType)
 {
+  NS_ENSURE_ARG(aURI);
+
   // Notify "cleartime" only if all visits to the page have been removed.
   if (!aVisitTime) {
     // If the page is bookmarked, notify observers for each associated bookmark.
     ItemChangeData changeData;
     nsresult rv = aURI->GetSpec(changeData.bookmark.url);
     NS_ENSURE_SUCCESS(rv, rv);
     changeData.property = NS_LITERAL_CSTRING("cleartime");
     changeData.isAnnotation = false;
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -561,18 +561,24 @@ public:
 
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(NS_IsMainThread(), "Must be called on the main thread");
     nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
     if (navHistory) {
       nsCOMPtr<nsIURI> uri;
       (void)NS_NewURI(getter_AddRefs(uri), mSpec);
-      navHistory->NotifyFrecencyChanged(uri, mNewFrecency, mGUID, mHidden,
-                                        mLastVisitDate);
+      // We cannot assert since some automated tests are checking this path.
+      NS_WARN_IF_FALSE(uri, "Invalid URI in FrecencyNotification");
+      // Notify a frecency change only if we have a valid uri, otherwise
+      // the observer couldn't gather any useful data from the notification.
+      if (uri) {
+        navHistory->NotifyFrecencyChanged(uri, mNewFrecency, mGUID, mHidden,
+                                          mLastVisitDate);
+      }
     }
     return NS_OK;
   }
 
 private:
   nsCString mSpec;
   int32_t mNewFrecency;
   nsCString mGUID;
@@ -2495,16 +2501,18 @@ nsNavHistory::RemovePages(nsIURI **aURIs
   NS_ENSURE_ARG(aURIs);
 
   nsresult rv;
   // build a list of place ids to delete
   nsCString deletePlaceIdsQueryString;
   for (uint32_t i = 0; i < aLength; i++) {
     int64_t placeId;
     nsAutoCString guid;
+    if (!aURIs[i])
+      continue;
     rv = GetIdForPage(aURIs[i], &placeId, guid);
     NS_ENSURE_SUCCESS(rv, rv);
     if (placeId != 0) {
       if (!deletePlaceIdsQueryString.IsEmpty())
         deletePlaceIdsQueryString.Append(',');
       deletePlaceIdsQueryString.AppendInt(placeId);
     }
   }
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -1074,27 +1074,29 @@ int32_t nsNavHistoryContainerResultNode:
   bool b_itemAnno = false;
 
   // Not used for item annos
   nsCOMPtr<nsIURI> a_uri, b_uri;
   if (a->mItemId != -1) {
     a_itemAnno = true;
   } else {
     nsAutoCString spec;
-    if (NS_SUCCEEDED(a->GetUri(spec)))
-      NS_NewURI(getter_AddRefs(a_uri), spec);
+    if (NS_SUCCEEDED(a->GetUri(spec))){
+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(a_uri), spec)));
+    }
     NS_ENSURE_TRUE(a_uri, 0);
   }
 
   if (b->mItemId != -1) {
     b_itemAnno = true;
   } else {
     nsAutoCString spec;
-    if (NS_SUCCEEDED(b->GetUri(spec)))
-      NS_NewURI(getter_AddRefs(b_uri), spec);
+    if (NS_SUCCEEDED(b->GetUri(spec))) {
+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(b_uri), spec)));
+    }
     NS_ENSURE_TRUE(b_uri, 0);
   }
 
   nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
   NS_ENSURE_TRUE(annosvc, 0);
 
   bool a_hasAnno, b_hasAnno;
   if (a_itemAnno) {
@@ -4406,16 +4408,19 @@ nsNavHistoryResult::OnItemAdded(int64_t 
                                 int32_t aIndex,
                                 uint16_t aItemType,
                                 nsIURI* aURI,
                                 const nsACString& aTitle,
                                 PRTime aDateAdded,
                                 const nsACString& aGUID,
                                 const nsACString& aParentGUID)
 {
+  NS_ENSURE_ARG(aItemType != nsINavBookmarksService::TYPE_BOOKMARK ||
+                aURI);
+
   ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(aParentId,
     OnItemAdded(aItemId, aParentId, aIndex, aItemType, aURI, aTitle, aDateAdded,
                 aGUID, aParentGUID)
   );
   ENUMERATE_HISTORY_OBSERVERS(
     OnItemAdded(aItemId, aParentId, aIndex, aItemType, aURI, aTitle, aDateAdded,
                 aGUID, aParentGUID)
   );
@@ -4431,16 +4436,19 @@ NS_IMETHODIMP
 nsNavHistoryResult::OnItemRemoved(int64_t aItemId,
                                   int64_t aParentId,
                                   int32_t aIndex,
                                   uint16_t aItemType,
                                   nsIURI* aURI,
                                   const nsACString& aGUID,
                                   const nsACString& aParentGUID)
 {
+  NS_ENSURE_ARG(aItemType != nsINavBookmarksService::TYPE_BOOKMARK ||
+                aURI);
+
   ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(aParentId,
       OnItemRemoved(aItemId, aParentId, aIndex, aItemType, aURI, aGUID,
                     aParentGUID));
   ENUMERATE_ALL_BOOKMARKS_OBSERVERS(
       OnItemRemoved(aItemId, aParentId, aIndex, aItemType, aURI, aGUID,
                     aParentGUID));
   ENUMERATE_HISTORY_OBSERVERS(
       OnItemRemoved(aItemId, aParentId, aIndex, aItemType, aURI, aGUID,
@@ -4506,16 +4514,18 @@ nsNavHistoryResult::OnItemVisited(int64_
                                   int64_t aVisitId,
                                   PRTime aVisitTime,
                                   uint32_t aTransitionType,
                                   nsIURI* aURI,
                                   int64_t aParentId,
                                   const nsACString& aGUID,
                                   const nsACString& aParentGUID)
 {
+  NS_ENSURE_ARG(aURI);
+
   ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(aParentId,
       OnItemVisited(aItemId, aVisitId, aVisitTime, aTransitionType, aURI,
                     aParentId, aGUID, aParentGUID));
   ENUMERATE_ALL_BOOKMARKS_OBSERVERS(
       OnItemVisited(aItemId, aVisitId, aVisitTime, aTransitionType, aURI,
                     aParentId, aGUID, aParentGUID));
   // Note: we do NOT call history observers in this case.  This notification is
   // the same as OnVisit, except that here we know the item is a bookmark.
@@ -4561,16 +4571,18 @@ nsNavHistoryResult::OnItemMoved(int64_t 
 
 
 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)
 {
+  NS_ENSURE_ARG(aURI);
+
   uint32_t added = 0;
 
   ENUMERATE_HISTORY_OBSERVERS(OnVisit(aURI, aVisitId, aTime, aSessionId,
                                       aReferringId, aTransitionType, aGUID,
                                       aHidden, &added));
 
   if (!mRootNode->mExpanded)
     return NS_OK;
@@ -4633,16 +4645,18 @@ nsNavHistoryResult::OnVisit(nsIURI* aURI
 }
 
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnTitleChanged(nsIURI* aURI,
                                    const nsAString& aPageTitle,
                                    const nsACString& aGUID)
 {
+  NS_ENSURE_ARG(aURI);
+
   ENUMERATE_HISTORY_OBSERVERS(OnTitleChanged(aURI, aPageTitle, aGUID));
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnFrecencyChanged(nsIURI* aURI,
                                       int32_t aNewFrecency,
@@ -4661,16 +4675,18 @@ nsNavHistoryResult::OnManyFrecenciesChan
 }
 
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnDeleteURI(nsIURI *aURI,
                                 const nsACString& aGUID,
                                 uint16_t aReason)
 {
+  NS_ENSURE_ARG(aURI);
+
   ENUMERATE_HISTORY_OBSERVERS(OnDeleteURI(aURI, aGUID, aReason));
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnClearHistory()
 {
@@ -4680,27 +4696,31 @@ nsNavHistoryResult::OnClearHistory()
 
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnPageChanged(nsIURI* aURI,
                                   uint32_t aChangedAttribute,
                                   const nsAString& aValue,
                                   const nsACString& aGUID)
 {
+  NS_ENSURE_ARG(aURI);
+
   ENUMERATE_HISTORY_OBSERVERS(OnPageChanged(aURI, aChangedAttribute, aValue, aGUID));
   return NS_OK;
 }
 
 
 /**
  * Don't do anything when visits expire.
  */
 NS_IMETHODIMP
 nsNavHistoryResult::OnDeleteVisits(nsIURI* aURI,
                                    PRTime aVisitTime,
                                    const nsACString& aGUID,
                                    uint16_t aReason,
                                    uint32_t aTransitionType)
 {
+  NS_ENSURE_ARG(aURI);
+
   ENUMERATE_HISTORY_OBSERVERS(OnDeleteVisits(aURI, aVisitTime, aGUID, aReason,
                                              aTransitionType));
   return NS_OK;
 }
--- a/toolkit/components/places/nsNavHistoryResult.h
+++ b/toolkit/components/places/nsNavHistoryResult.h
@@ -552,23 +552,16 @@ public:
   static int32_t SortComparison_FrecencyLess(nsNavHistoryResultNode* a,
                                              nsNavHistoryResultNode* b,
                                              void* closure);
   static int32_t SortComparison_FrecencyGreater(nsNavHistoryResultNode* a,
                                                 nsNavHistoryResultNode* b,
                                                 void* closure);
 
   // finding children: THESE DO NOT ADDREF
-  nsNavHistoryResultNode* FindChildURI(nsIURI* aURI, uint32_t* aNodeIndex)
-  {
-    nsAutoCString spec;
-    if (NS_FAILED(aURI->GetSpec(spec)))
-      return nullptr;
-    return FindChildURI(spec, aNodeIndex);
-  }
   nsNavHistoryResultNode* FindChildURI(const nsACString& aSpec,
                                        uint32_t* aNodeIndex);
   // returns the index of the given node, -1 if not found
   int32_t FindChild(nsNavHistoryResultNode* aNode)
     { return mChildren.IndexOf(aNode); }
 
   nsresult InsertChildAt(nsNavHistoryResultNode* aNode, int32_t aIndex);
   nsresult InsertSortedChild(nsNavHistoryResultNode* aNode,
--- a/toolkit/components/places/tests/unit/test_null_interfaces.js
+++ b/toolkit/components/places/tests/unit/test_null_interfaces.js
@@ -4,33 +4,41 @@
 
 /**
  * Test bug 489872 to make sure passing nulls to nsNavHistory doesn't crash.
  */
 
 // Make an array of services to test, each specifying a class id, interface
 // and an array of function names that don't throw when passed nulls
 var testServices = [
-  ["browser/nav-history-service;1", "nsINavHistoryService",
+  ["browser/nav-history-service;1",
+    ["nsINavHistoryService"],
     ["queryStringToQueries", "removePagesByTimeframe", "removePagesFromHost",
-     "removeVisitsByTimeframe", "getObservers"]],
-  ["browser/nav-bookmarks-service;1","nsINavBookmarksService",
-    ["createFolder", "getObservers"]],
-  ["browser/livemark-service;2","mozIAsyncLivemarks", ["reloadLivemarks"]],
-  ["browser/annotation-service;1","nsIAnnotationService", []],
-  ["browser/favicon-service;1","nsIFaviconService", []],
-  ["browser/tagging-service;1","nsITaggingService", []],
+     "removeVisitsByTimeframe", "getObservers"]
+  ],
+  ["browser/nav-bookmarks-service;1",
+    ["nsINavBookmarksService", "nsINavHistoryObserver", "nsIAnnotationObserver"],
+    ["createFolder", "getObservers", "onFrecencyChanged", "onTitleChanged",
+     "onPageAnnotationSet", "onPageAnnotationRemoved"]
+  ],
+  ["browser/livemark-service;2", ["mozIAsyncLivemarks"], ["reloadLivemarks"]],
+  ["browser/annotation-service;1", ["nsIAnnotationService"], []],
+  ["browser/favicon-service;1", ["nsIFaviconService"], []],
+  ["browser/tagging-service;1", ["nsITaggingService"], []],
 ];
 do_print(testServices.join("\n"));
 
 function run_test()
 {
-  for (let [cid, iface, nothrow] of testServices) {
-    do_print(`Running test with ${cid} ${iface} ${nothrow}`);
-    let s = Cc["@mozilla.org/" + cid].getService(Ci[iface]);
+  for (let [cid, ifaces, nothrow] of testServices) {
+    do_print(`Running test with ${cid} ${ifaces.join(", ")} ${nothrow}`);
+    let s = Cc["@mozilla.org/" + cid].getService(Ci.nsISupports);
+    for (let iface of ifaces) {
+      s.QueryInterface(Ci[iface]);
+    }
 
     let okName = function(name) {
       do_print(`Checking if function is okay to test: ${name}`);
       let func = s[name];
 
       let mesg = "";
       if (typeof func != "function")
         mesg = "Not a function!";
@@ -44,29 +52,29 @@ function run_test()
         return false;
       }
 
       return true;
     }
 
     do_print(`Generating an array of functions to test service: ${s}`);
     for (let n of Object.keys(s).filter(i => okName(i)).sort()) {
-      do_print(`\nTesting ${iface} function with null args: ${n}`);
+      do_print(`\nTesting ${ifaces.join(", ")} function with null args: ${n}`);
 
       let func = s[n];
       let num = func.length;
       do_print(`Generating array of nulls for #args: ${num}`);
       let args = Array(num).fill(null);
 
       let tryAgain = true;
       while (tryAgain == true) {
         try {
           do_print(`Calling with args: ${JSON.stringify(args)}`);
           func.apply(s, args);
-  
+
           do_print(`The function did not throw! Is it one of the nothrow? ${nothrow}`);
           Assert.notEqual(nothrow.indexOf(n), -1);
 
           do_print("Must have been an expected nothrow, so no need to try again");
           tryAgain = false;
         }
         catch(ex) {
           if (ex.result == Cr.NS_ERROR_ILLEGAL_VALUE) {