378079 - AutoComplete returns invalid rows when more than one AutoCompleteSearch is used

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Matt Crocker]

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

Comment 1

[Matt Crocker]

Attachment: Fix + Comments

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 3

[Matt Crocker]

Uhoh, what are the right autocomplete APIs?  I'm integrating the browser searchbar binding into Songbird at the moment.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 5

[Matt Crocker]

Ah, that makes sense.  Thanks.

Who should I hassle for a review?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

See http://www.mozilla.org/projects/toolkit/review.html. I'd be glad to review it.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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 .

Comment 8

[Nickolay_Ponomarev]

If this is ready for check-in, please mark it as [checkin needed] in the status whiteboard and someone will get it in.

Comment 9

[Matt Crocker]

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?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Either works, as long as you can test that the fix doesn't regress. Do whatever you find easiest :)

Comment 11

[Matt Crocker]

Attachment: Fix + Unit Test

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 13

[Matt Crocker]

Thanks, that would be great.

Comment 14

[Shawn Wilsher :sdwilsh]

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