From 5842ef98601b777ef683b24d49e1fd70faf5ac59 Mon Sep 17 00:00:00 2001 From: "sspitzer@mozilla.org" Date: Mon, 1 Oct 2007 10:36:18 -0700 Subject: [PATCH] fix for bug #394508: queries that use maxResults can return incorrect results due to post query filtering. initial patch=Colin Walters r=dietrich, a=mconnor --- .../components/places/src/nsNavHistory.cpp | 99 +++++++++++-------- toolkit/components/places/src/nsNavHistory.h | 3 +- 2 files changed, 61 insertions(+), 41 deletions(-) diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp index f4cf4c6b9d6..2fef08f938d 100644 --- a/toolkit/components/places/src/nsNavHistory.cpp +++ b/toolkit/components/places/src/nsNavHistory.cpp @@ -1558,7 +1558,7 @@ nsNavHistory::EvaluateQueryForNode(const nsCOMArray& aQueries nsCOMArray queries; queries.AppendObject(query); nsCOMArray 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& aQueries, nsNavHi return PR_TRUE; } +static +PRBool NeedToFilterResultSet(const nsCOMArray& 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& aQueries, nsNavHistoryQueryOptions *aOptions, @@ -2238,8 +2264,7 @@ nsNavHistory::ConstructQueryString(const nsCOMArray& 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 +2359,10 @@ nsNavHistory::ConstructQueryString(const nsCOMArray& aQueries NS_NOTREACHED("Invalid sorting mode"); } - // limit clause if there are 'maxResults' - if (aOptions->MaxResults() > 0) { + // determine whether we can push maxResults constraints + // into the queries as LIMIT, or if we need to do result count clamping later + // using FilterResultSet() + if (!NeedToFilterResultSet(aQueries, aOptions) && aOptions->MaxResults() > 0) { queryString += NS_LITERAL_CSTRING(" LIMIT "); queryString.AppendInt(aOptions->MaxResults()); queryString.AppendLiteral(" "); @@ -2389,56 +2416,39 @@ nsNavHistory::GetQueryResults(nsNavHistoryQueryResultNode *aResultNode, // bind parameters PRInt32 numParameters = 0; - - // 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 - PRBool resultAsList = PR_TRUE; - 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 ++) { + 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 { + // optimize the case where we just use the results as is + // and we don't need to do any post-query filtering + if (NeedToFilterResultSet(aQueries, aOptions)) { // generate the toplevel results nsCOMArray 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 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; } @@ -4127,16 +4137,22 @@ nsNavHistory::URIHasTag(nsIURI* aURI, const nsAString& aTag) // nsNavHistory::FilterResultSet // -// This does some post-query-execution filtering: -// - searching on title & url -// - parent folder (recursively) -// - excludeQueries +// This does some post-query-execution filtering: +// - 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& aSet, nsCOMArray* aFiltered, - const nsCOMArray& aQueries) + const nsCOMArray& aQueries, + nsNavHistoryQueryOptions *aOptions) { nsresult rv; @@ -4260,6 +4276,9 @@ nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aQueryNode, } if (appendNode) aFiltered->AppendObject(aSet[nodeIndex]); + + if (aOptions->MaxResults() > 0 && aFiltered->Count() >= aOptions->MaxResults()) + break; } // de-allocate the matrixes diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h index 650b0c31802..d6f4fbdc6bc 100644 --- a/toolkit/components/places/src/nsNavHistory.h +++ b/toolkit/components/places/src/nsNavHistory.h @@ -516,7 +516,8 @@ protected: nsresult FilterResultSet(nsNavHistoryQueryResultNode *aParentNode, const nsCOMArray& aSet, nsCOMArray* aFiltered, - const nsCOMArray& aQueries); + const nsCOMArray& aQueries, + nsNavHistoryQueryOptions* aOptions); // observers nsMaybeWeakPtrArray mObservers;