Closed Bug 989598 Opened 11 years ago Closed 9 years ago

nsAutoCompleteController malfunction while handling multiple sources

Categories

(Toolkit :: Autocomplete, defect)

Product:
Toolkit
Component:
Autocomplete
Version:
24 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 720589

People

(Reporter: wuhaoshu, Unassigned)

References

Details

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release) Build ID: 20131205180928 Steps to reproduce: #1. Create a custom autocomplete search component, and register it as CONTRACT_ID = '@mozilla.org/autocomplete/search;1?name=speedy'; #2. Bind it as urlbar autocomplete search source ----CSS--------------- #urlbar { -moz-binding: url('chrome://whatever/content/Speedy.xbl#urlbar'); } ----XBL--------------- <binding id="urlbar" extends="chrome://browser/content/urlbarBindings.xml#urlbar"> <implementation> <method name="initSearchNames"> <body><![CDATA[ this.__proto__.__proto__.initSearchNames.call(this); if (this.mSearchNames.indexOf('speedy') < 0) { this.mSearchNames.push('speedy'); } ]]></body> </method> </implementation> </binding> ------------------- #3. Monitor the matchCount and output it whenever a new result comes. ----CSS--------------- #PopupAutoCompleteRichResult { -moz-binding: url('chrome://whatever/content/Speedy.xbl#PopupAutoCompleteRichResult'); } ----XBL--------------- <binding id="urlbar" extends="chrome://browser/content/urlbarBindings.xml#urlbar"> <implementation> <method name="invalidate"> <body><![CDATA[ console.error("matchCount: " + this.mInput.controller.matchCount); this.__proto__.__proto__.invalidate.call(this); ]]></body> </method> </implementation> </binding> #3. type something in the urlbar and let both the custom search component Speedy(@mozilla.org/autocomplete/search;1?name=speedy) and the native search component nsPlacesAutoComplete(@mozilla.org/autocomplete/search;1?name=history) return some entries as result, However,make sure Speedy return faster than nsPlacesAutoComplete. Actual results: Let use suppose the keywords "bugzilla" gets 5 hits in total, 4 entries from Speedy, and 1 entry from nsPlacesAutoComplete. The actual output is matchCount: 4 matchCount: 5 matchCount: 2 They are initiated by below correspondingly. Speedy's onSearchResult() nsPlacesAutoComplete's PAC_handleResult() ->PAC_notifyResults() nsPlacesAutoComplete's PAC_finishSearch() -> PAC_notifyResults() Expected results: The expected output is matchCount: 4 matchCount: 5 matchCount: 5 What happens here is the matchCount of Speedy is overwritten by mistake, when nsAutoCompleteController::ProcessResult() handles the 2nd call of nsPlacesAutoComplete.PAC_notifyResults(). The problem code is below, where mMatchCounts[aSearchIndex] should be mMatchCounts[resultIndex] in the else clause. int32_t resultIndex = mResults.IndexOf(aResult); if (resultIndex == -1) { // cache the result mResults.AppendObject(aResult); mMatchCounts.AppendElement(matchCount); resultIndex = mResults.Count() - 1; } else { oldMatchCount = mMatchCounts[aSearchIndex]; mMatchCounts[resultIndex] = matchCount; } Let us follow the behavior of nsAutoCompleteController::ProcessResult() as below. #1. Speedy return 2 entries. Since it is a new result, mResults.IndexOf(aResult) get no hit, and nsAutoCompleteController gives mResults[0] and mMatchCounts[0] to it. #2. 1st call of nsPlacesAutoComplete.PAC_notifyResults() is a new result as well, so it gets mResults[1] and mMatchCounts[1]. #3. when nsPlacesAutoComplete's search is done, PAC_finishSearch() calls PAC_notifyResults() again. The result is not a new entry anymore. And since urlbar's autocompletesearch="urline history ", "history" comes before "speedy", nsPlacesAutoComplete's SearchIndex is 0. While retrieving the oldMatchCount, aSearchIndex(0) is incorrectly used instead of resultIndex(1) and oldMatchCount gets 4 though it should be 1. This problem won't occur, if nsPlacesAutoComplete is the only search source. But whenever multiple sources are registered, and the custome search source returns faster then nsPlacesAutoComplete, it leads to error. Google "getImageAt" "autocomplete" "NS_ERROR_FAILURE", you will find bug reports back to 2009. I guess they are all caused by this bug. Though the code tells everything, if needed, I can upload a demo xpi. Please let me know. By the way, because of this bug, I have to do some tricky stuff in my new addon as a workaround and I hate it. I hope you guys will fix it soon and since I am an ESR user, please include the fix in the ESR release as well.
this is likely related to bug 720589, there's a reason mMatchCounts is handled like that though (but off-hand I don't recall it), so should be triple verified before changing it. It's definitely not a typo. the whole mMatchCounts handling is a bit fancy.
Status: UNCONFIRMED → NEW
Ever confirmed: true
btw, you may try my patch in bug 720589 and see if it improves things. Unfortunately I didn't look at that code recently, so it may be bitrotted, nor I ever had the time to handle review comments there.
You are right, my ticket is a duplicated one to #720589. I am not familar with Bugzilla, shall I mark it as duplicate? Forgive me, if my comments don't make sense. I am not a daily C++ programmer. As Gavin Sharp commented, your patch seems to have a possibility to break if a search actually does send a completely new result each time. To be exact, I think the source may have two choices to send new result: first, if it may reuse the exsiting nsIAutoCompleteResult object, and update the contents totally to include the previously found and newly found; or, if it may create a new nsIAutoCompleteResult object, and include only the delta parts(newly found). Your patch can't handle the latter case. As long as the source respects the rule, I think the proposed mod by me(use mMatchCounts[resultIndex] instead of mMatchCounts[aSearchIndex] to retrieve oldMatchCount) makes a lot of sense. Any thoughts? By the way, my custom autocomplete search component is a async one, though it just returns back faster than the native source.
you may be right, though I don't have the time to verify as of now. I'm setting a dependency cause regardless this bug may contain useful insight for fixing bug 720589
Blocks: 720589
let's assume duplicate now that bug 720589 is fixed and has a much better handling of multiple results. If there's still some issue please file it as a new bug.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.