Bug 652379 - place:folder=-1 returns a non-empty result.

r=dietrich

toolkit/components/places/nsNavHistory.cpp

@@ -2861,22 +2861,25 @@ nsNavHistory::ExecuteQueries(nsINavHistoryQuery** aQueries, PRUint32 aQueryCount
     queries.AppendObject(query);
   }
 
-  nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
-  NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
-  // root node
+  // Create the root node.
   nsRefPtr<nsNavHistoryContainerResultNode> rootNode;
   PRInt64 folderId = GetSimpleBookmarksQueryFolder(queries, options);
   if (folderId) {
-    // In the simple case where we're just querying children of a single bookmark
-    // folder, we can more efficiently generate results.
+    // In the simple case where we're just querying children of a single
+    // bookmark folder, we can more efficiently generate results.
+    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
+    NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
     nsRefPtr<nsNavHistoryResultNode> tempRootNode;
     rv = bookmarks->ResultNodeForContainer(folderId, options,
                                            getter_AddRefs(tempRootNode));
-    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
-                     "Generating a generic empty node for a broken query!");
     if (NS_SUCCEEDED(rv)) {
       rootNode = tempRootNode->GetAsContainer();
     }
+    else {
+      NS_WARNING("Generating a generic empty node for a broken query!");
+      // This is a perf hack to generate an empty query that skips filtering.
+      options->SetExcludeItems(PR_TRUE);
+    }
   }
 
   if (!rootNode) {
@@ -6147,9 +6150,10 @@ nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aQueryNode,
       if (includeFolders[queryIndex]->Length() != 0 &&
           resultType != nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS) {
         // Filter out the node if its parent is in the excludeFolders
-        // cache.
-        if (excludeFolders[queryIndex]->Contains(parentId))
+        // cache or it has no parent.
+        if (excludeFolders[queryIndex]->Contains(parentId) || parentId == -1) {
           continue;
+        }
 
         if (!includeFolders[queryIndex]->Contains(parentId)) {
           // If parent is not found in current includeFolders cache, we check
@@ -6533,6 +6537,8 @@ nsNavHistory::QueryRowToResult(PRInt64 itemId, const nsACString& aURI,
     // whole result.  Instead make a generic empty query node.
     *aNode = new nsNavHistoryQueryResultNode(aTitle, aFavicon, aURI);
     (*aNode)->mItemId = itemId;
+    // This is a perf hack to generate an empty query that skips filtering.
+    (*aNode)->GetAsQuery()->Options()->SetExcludeItems(PR_TRUE);
     NS_ADDREF(*aNode);
   }
 
@@ -6916,7 +6922,7 @@ GetSimpleBookmarksQueryFolder(const nsCOMArray<nsNavHistoryQuery>& aQueries,
 
   // Don't care about onlyBookmarked flag, since specifying a bookmark
   // folder is inferring onlyBookmarked.
-  NS_ASSERTION(query->Folders()[0] > 0, "bad folder id");
+
   return query->Folders()[0];
 }
 

toolkit/components/places/nsNavHistoryResult.cpp

@@ -2341,11 +2341,12 @@ nsNavHistoryQueryResultNode::CanExpand()
   if (IsContainersQuery())
     return PR_TRUE;
 
-  // If we are child of an ExcludeItems parent or root, we should not expand.
+  // If ExcludeItems is set on the root or on the node itself, don't expand.
   if ((mResult && mResult->mRootNode->mOptions->ExcludeItems()) ||
-      (mParent && mParent->mOptions->ExcludeItems()))
+      Options()->ExcludeItems())
     return PR_FALSE;
 
+  // Check the ancestor container.
   nsNavHistoryQueryOptions* options = GetGeneratingOptions();
   if (options) {
     if (options->ExcludeItems())
@@ -2353,8 +2354,10 @@ nsNavHistoryQueryResultNode::CanExpand()
     if (options->ExpandQueries())
       return PR_TRUE;
   }
+
   if (mResult && mResult->mRootNode == this)
     return PR_TRUE;
+
   return PR_FALSE;
 }
 

toolkit/components/places/nsNavHistoryResult.h

@@ -716,9 +716,9 @@ class nsNavHistoryQueryResultNode : public nsNavHistoryContainerResultNode,
                                     public nsINavHistoryQueryResultNode
 {
 public:
-  nsNavHistoryQueryResultNode(const nsACString& aQueryURI,
-                              const nsACString& aTitle,
-                              const nsACString& aIconURI);
+  nsNavHistoryQueryResultNode(const nsACString& aTitle,
+                              const nsACString& aIconURI,
+                              const nsACString& aQueryURI);
   nsNavHistoryQueryResultNode(const nsACString& aTitle,
                               const nsACString& aIconURI,
                               const nsCOMArray<nsNavHistoryQuery>& aQueries,

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

@@ -6,22 +6,34 @@ function run_test() {
     PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("http://1.moz.org/"),
     PlacesUtils.bookmarks.DEFAULT_INDEX, "Bookmark 1"
   );
-  let id = PlacesUtils.bookmarks.insertBookmark(
+  let id1 = PlacesUtils.bookmarks.insertBookmark(
     PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("place:folder=1234"),
-    PlacesUtils.bookmarks.DEFAULT_INDEX, "Shortcut"
+    PlacesUtils.bookmarks.DEFAULT_INDEX, "Shortcut 1"
+  );
+  let id2 = PlacesUtils.bookmarks.insertBookmark(
+    PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("place:folder=-1"),
+    PlacesUtils.bookmarks.DEFAULT_INDEX, "Shortcut 2"
   );
   PlacesUtils.bookmarks.insertBookmark(
     PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("http://2.moz.org/"),
     PlacesUtils.bookmarks.DEFAULT_INDEX, "Bookmark 2"
   );
 
+  // Add also a simple visit.
+  PlacesUtils.history.addVisit(
+    NetUtil.newURI("http://3.moz.org/"), Date.now() * 1000, null,
+    PlacesUtils.history.TRANSITION_TYPED, false, 0
+  );
+
   // Query containing a broken folder shortcuts among results.
   let query = PlacesUtils.history.getNewQuery();
   query.setFolders([PlacesUtils.unfiledBookmarksFolderId], 1);
   let options = PlacesUtils.history.getNewQueryOptions();
   let root = PlacesUtils.history.executeQuery(query, options).root;
   root.containerOpen = true;
-  do_check_eq(root.childCount, 3);
+
+  do_check_eq(root.childCount, 4);
+
   let shortcut = root.getChild(1);
   do_check_eq(shortcut.uri, "place:folder=1234");
   PlacesUtils.asContainer(shortcut);
@@ -29,8 +41,19 @@ function run_test() {
   do_check_eq(shortcut.childCount, 0);
   shortcut.containerOpen = false;
   // Remove the broken shortcut while the containing result is open.
-  PlacesUtils.bookmarks.removeItem(id);
+  PlacesUtils.bookmarks.removeItem(id1);
+  do_check_eq(root.childCount, 3);
+
+  shortcut = root.getChild(1);
+  do_check_eq(shortcut.uri, "place:folder=-1");
+  PlacesUtils.asContainer(shortcut);
+  shortcut.containerOpen = true;
+  do_check_eq(shortcut.childCount, 0);
+  shortcut.containerOpen = false;
+  // Remove the broken shortcut while the containing result is open.
+  PlacesUtils.bookmarks.removeItem(id2);
   do_check_eq(root.childCount, 2);
+
   root.containerOpen = false;
 
   // Broken folder shortcut as root node.
@@ -41,4 +64,13 @@ function run_test() {
   root.containerOpen = true;
   do_check_eq(root.childCount, 0);
   root.containerOpen = false;
+
+  // Broken folder shortcut as root node with folder=-1.
+  query = PlacesUtils.history.getNewQuery();
+  query.setFolders([-1], 1);
+  options = PlacesUtils.history.getNewQueryOptions();
+  root = PlacesUtils.history.executeQuery(query, options).root;
+  root.containerOpen = true;
+  do_check_eq(root.childCount, 0);
+  root.containerOpen = false;
 }