Closed Bug 378079 Opened 17 years ago Closed 17 years ago

AutoComplete returns invalid rows when more than one AutoCompleteSearch is used

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: matt, Assigned: matt)

References

Details

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070418 Songbird/0.3pre (00000000000000)

The AutoCompleteController returns incorrect cell text in cases where more than one nsIAutoCompleteSearch is used and RESULT_NOMATCH is returned.


Scenario:

Pretend that...
* two nsIAutoCompleteSearch sources have been registered
* a search has been completed
* the first search returned RESULT_NOMATCH
* the second search returned RESULT_SUCCESS with two results


The popup tree is being built, and this is the first call to RowIndexToSearch:

http://lxr.mozilla.org/seamonkey/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#1373

1373 nsAutoCompleteController::RowIndexToSearch(PRInt32 aRowIndex, PRInt32 *aSearchIndex, PRInt32 *aItemIndex)
1374 {
1375   *aSearchIndex = -1;
1376   *aItemIndex = -1;
1377   
1378   PRUint32 count;
1379   mSearches->Count(&count);
1380   PRUint32 index = 0;
1381   for (PRUint32 i = 0; i < count; ++i) {
1382     nsCOMPtr<nsIAutoCompleteResult> result;
1383     mResults->GetElementAt(i, getter_AddRefs(result));
1384     if (!result)
1385       continue;
1386       
1387     PRUint16 searchResult;
1388     result->GetSearchResult(&searchResult);
1389     
1390     PRUint32 rowCount = 1;
1391     if (searchResult == nsIAutoCompleteResult::RESULT_SUCCESS) {
1392       result->GetMatchCount(&rowCount);
1393     }
1394     
1395     if (index + rowCount-1 >= (PRUint32) aRowIndex) {
1396       *aSearchIndex = i;
1397       *aItemIndex = aRowIndex - index;
1398       return NS_OK;
1399     }
1400 
1401     index += rowCount;
1402   }
1403 
1404   return NS_OK;
1405 }


Since this is the first row, aRowIndex=0

On line 1390 rowCount is initialized to 1, but is never updated by result->GetMatchCount since the first search did not succeed.

Therefore in the if statement on 1395...

(index + rowCount-1 >= (PRUint32) aRowIndex) 
== ( 0 + 1 - 1 >= 0 ) 
== true

...causing an incorrect aSearchIndex and aItemIndex to be returned.


The visual result is a popup with two rows, the first row blank, and the second row showing the first result of the second search.


A fix is to initialize rowCount = 0, and then check rowCount != 0 in the if statement on 1395 (required as these are unsigned ints). 


I'll post a patch shortly.

Reproducible: Always
Attached patch Fix + CommentsSplinter Review
Thanks for looking into this! I've been meaning to fix it ever since I tried to make search suggest use the right autocomplete APIs instead of the hack it currently uses.
Status: UNCONFIRMED → NEW
Component: Search → Autocomplete
Ever confirmed: true
Product: Firefox → Toolkit
QA Contact: search → autocomplete
Uhoh, what are the right autocomplete APIs?  I'm integrating the browser searchbar binding into Songbird at the moment.
Sorry, I was unclear. It's using the right APIs, but it's not using them properly. I meant "use the multiple search feature", instead of the current "use a single nsIAutoCompleteSearch (the one in nsSearchSuggestions.js) that forwards to another (the one for form history) to get additional results". nsSearchSuggestions.js has all the gory details.
Ah, that makes sense.  Thanks.

Who should I hassle for a review?
Attachment #262167 - Flags: review?(gavin.sharp)
Comment on attachment 262167 [details] [diff] [review]
Fix + Comments

Sorry for the delay in reviewing here.

I thought there were other problems with this code when it adds rows to failed searches for a status message, or something like that, but I can't seem to pinpoint it right now, and I assume you guys would have probably run into it if you're using the multiple search feature, so maybe I'm just remembering wrong.

This code needs a unit test - could you write one? You should be able to base it on an existing test, like http://lxr.mozilla.org/seamonkey/source/toolkit/components/autocomplete/tests/unit/test_330578.js .
Attachment #262167 - Flags: review?(gavin.sharp) → review+
If this is ready for check-in, please mark it as [checkin needed] in the status whiteboard and someone will get it in.
Assignee: nobody → matt
Version: unspecified → Trunk
Thanks for the review.  Sure, I don't mind writing a test.

Testing the AutoCompleteController requires an AutoCompleteInput and AutoCompletePopup.  Should I stub these out and test with xpcshell, or use mochitest and an actual autocomplete textbox element?
Either works, as long as you can test that the fix doesn't regress. Do whatever you find easiest :)
Attached patch Fix + Unit TestSplinter Review
Attachment #269290 - Flags: review?(gavin.sharp)
Comment on attachment 269290 [details] [diff] [review]
Fix + Unit Test

This is great work, thanks a lot for getting this done Matt! I can land this if you'd like.
Attachment #269290 - Flags: review?(gavin.sharp) → review+
Whiteboard: [checkin needed]
Thanks, that would be great.
Keywords: checkin-needed
Whiteboard: [checkin needed]
Checking in toolkit/components/autocomplete/tests/unit/test_378079.js;
initial revision: 1.1
Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp;
new revision: 1.59; previous revision: 1.58
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta1
You need to log in before you can comment on or make changes to this bug.