fix for bug #394508: queries that use maxResults can return incorrect results due to post query filtering. initial patch=Colin Walters <walters@verbum.org> r=dietrich, a=mconnor

toolkit/components/places/src/nsNavHistory.cpp

@@ -1558,7 +1558,7 @@ nsNavHistory::EvaluateQueryForNode(const nsCOMArray<nsNavHistoryQuery>& aQueries
       nsCOMArray<nsNavHistoryQuery> queries;
       queries.AppendObject(query);
       nsCOMArray<nsNavHistoryResultNode> filteredSet;
-      nsresult rv = FilterResultSet(nsnull, inputSet, &filteredSet, queries);
+      nsresult rv = FilterResultSet(nsnull, inputSet, &filteredSet, queries, aOptions);
       if (NS_FAILED(rv))
         continue;
       if (! filteredSet.Count())
@@ -2144,6 +2144,32 @@ PRBool IsHistoryMenuQuery(const nsCOMArray<nsNavHistoryQuery>& aQueries, nsNavHi
   return PR_TRUE;
 }
 
+static
+PRBool NeedToFilterResultSet(const nsCOMArray<nsNavHistoryQuery>& aQueries, 
+                             nsNavHistoryQueryOptions *aOptions)
+{
+  // optimize the case where we just want a list with no grouping: this
+  // directly fills in the results and we avoid a copy of the whole list
+  PRUint32 groupCount;
+  const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
+
+  if (groupCount != 0 || aOptions->ExcludeQueries())
+    return PR_TRUE;
+
+  PRInt32 i;
+  for (i = 0; i < aQueries.Count(); i ++) {
+    if (aQueries[i]->Folders().Length() != 0) {
+      return PR_TRUE;
+    } else {
+      PRBool hasSearchTerms;
+      nsresult rv = aQueries[i]->GetHasSearchTerms(&hasSearchTerms);
+      if (NS_FAILED(rv) || hasSearchTerms)
+        return PR_TRUE;
+    }
+  }
+  return PR_FALSE;
+}
+
 nsresult
 nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries,
                                    nsNavHistoryQueryOptions *aOptions, 
@@ -2155,6 +2181,11 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
     return NS_ERROR_INVALID_ARG;
   }
 
+  // determine whether we can push maxResults constraints
+  // into the queries as LIMIT, or if we need to do result count clamping later
+  // using FilterResultSet()
+  PRBool canLimitInSQL = !NeedToFilterResultSet(aQueries, aOptions);
+
   // for the very special query for the history menu 
   // we generate a super-optimized SQL query
   if (IsHistoryMenuQuery(aQueries, aOptions)) {
@@ -2169,8 +2200,11 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
       "(h.id IN (SELECT DISTINCT h.id FROM moz_historyvisits, "
       " moz_places h WHERE place_id = "
       " h.id AND hidden <> 1 AND visit_type <> 4 AND visit_type <> 0 "
-      " ORDER BY visit_date DESC LIMIT ");
+      " ORDER BY visit_date DESC");
+    if (canLimitInSQL) {
+      queryString += NS_LITERAL_CSTRING(" LIMIT ");
       queryString.AppendInt(aOptions->MaxResults());
+    }
     queryString += NS_LITERAL_CSTRING(")) GROUP BY h.id ORDER BY 6 DESC"); // v.visit_date
     return NS_OK;
   }
@@ -2238,8 +2272,7 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
       "LEFT OUTER JOIN moz_historyvisits v ON b.fk = v.place_id "
       "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
       groupBy = NS_LITERAL_CSTRING(" GROUP BY b.id");
-    }
+    } else {
-    else {
       // XXX: implement support for nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED 
       return NS_ERROR_NOT_IMPLEMENTED;
     }
@@ -2334,8 +2367,8 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
       NS_NOTREACHED("Invalid sorting mode");
   }
 
-  // limit clause if there are 'maxResults'
+  // limit clause if there are 'maxResults' and can use limit in our SQL
-  if (aOptions->MaxResults() > 0) {
+  if (canLimitInSQL && aOptions->MaxResults() > 0) {
     queryString += NS_LITERAL_CSTRING(" LIMIT ");
     queryString.AppendInt(aOptions->MaxResults());
     queryString.AppendLiteral(" ");
@@ -2390,54 +2423,29 @@ nsNavHistory::GetQueryResults(nsNavHistoryQueryResultNode *aResultNode,
   // bind parameters
   PRInt32 numParameters = 0;
 
-  // optimize the case where we just want a list with no grouping: this
+  // optimize the case where we just use the results as is
-  // directly fills in the results and we avoid a copy of the whole list
+  // and we don't need to do any post-query filtering
-  PRBool resultAsList = PR_TRUE;
+  if (NeedToFilterResultSet(aQueries, aOptions)) {
-  PRUint32 groupCount;
-  const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
-
-  if (groupCount != 0 || aOptions->ExcludeQueries()) {
-    resultAsList = PR_FALSE;
-  }
-
-  PRInt32 i;
-  for (i = 0; i < aQueries.Count(); i ++) {
-    PRInt32 clauseParameters = 0;
-    rv = BindQueryClauseParameters(statement, numParameters,
-                                   aQueries[i], aOptions, &clauseParameters);
-    NS_ENSURE_SUCCESS(rv, rv);
-    numParameters += clauseParameters;
-    if (resultAsList) {
-      if (aQueries[i]->Folders().Length() != 0) {
-        resultAsList = PR_FALSE;
-      } else {
-        PRBool hasSearchTerms;
-        rv = aQueries[i]->GetHasSearchTerms(&hasSearchTerms);
-        if (hasSearchTerms)
-          resultAsList = PR_FALSE;
-        NS_ENSURE_SUCCESS(rv, rv);
-      }
-    }
-  }
-
-  if (resultAsList) {
-    rv = ResultsAsList(statement, aOptions, aResults);
-    NS_ENSURE_SUCCESS(rv, rv);
-  } else {
     // generate the toplevel results
     nsCOMArray<nsNavHistoryResultNode> toplevel;
     rv = ResultsAsList(statement, aOptions, &toplevel);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    PRUint32 groupCount;
+    const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
+
     if (groupCount == 0) {
-      FilterResultSet(aResultNode, toplevel, aResults, aQueries);
+      FilterResultSet(aResultNode, toplevel, aResults, aQueries, aOptions);
     } else {
       nsCOMArray<nsNavHistoryResultNode> filteredResults;
-      FilterResultSet(aResultNode, toplevel, &filteredResults, aQueries);
+      FilterResultSet(aResultNode, toplevel, &filteredResults, aQueries, aOptions);
       rv = RecursiveGroup(aResultNode, filteredResults, groupings, groupCount,
                           aResults);
       NS_ENSURE_SUCCESS(rv, rv);
     }
+  } else {
+    rv = ResultsAsList(statement, aOptions, aResults);
+    NS_ENSURE_SUCCESS(rv, rv);
   } 
 
   return NS_OK;
@@ -4131,12 +4139,18 @@ nsNavHistory::URIHasTag(nsIURI* aURI, const nsAString& aTag)
 //   - searching on title & url
 //   - parent folder (recursively)
 //   - excludeQueries
+//   - tags
+//   - limit count
+//
+// Note:  changes to filtering in FilterResultSet() 
+// may require changes to NeedToFilterResultSet()
 
 nsresult
 nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aQueryNode,
                               const nsCOMArray<nsNavHistoryResultNode>& aSet,
                               nsCOMArray<nsNavHistoryResultNode>* aFiltered,
-                              const nsCOMArray<nsNavHistoryQuery>& aQueries)
+                              const nsCOMArray<nsNavHistoryQuery>& aQueries,
+                              nsNavHistoryQueryOptions *aOptions)
 {
   nsresult rv;
 
@@ -4260,6 +4274,9 @@ nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aQueryNode,
     }
     if (appendNode)
       aFiltered->AppendObject(aSet[nodeIndex]);
+      
+    if (aOptions->MaxResults() > 0 && aFiltered->Count() >= aOptions->MaxResults())
+      break;
   }
 
   // de-allocate the matrixes

toolkit/components/places/src/nsNavHistory.h

@@ -516,7 +516,8 @@ protected:
   nsresult FilterResultSet(nsNavHistoryQueryResultNode *aParentNode,
                            const nsCOMArray<nsNavHistoryResultNode>& aSet,
                            nsCOMArray<nsNavHistoryResultNode>* aFiltered,
-                           const nsCOMArray<nsNavHistoryQuery>& aQueries);
+                           const nsCOMArray<nsNavHistoryQuery>& aQueries,
+                           nsNavHistoryQueryOptions* aOptions);
 
   // observers
   nsMaybeWeakPtrArray<nsINavHistoryObserver> mObservers;