438861 - Autocomplete does not pass back previous results to a second search

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Mark Banner (:standard8)]

Attachment: Testcase

For MailNews autocomplete migration to toolkit, we want to have multiple autocompletesearch items, i.e. multiple searches on one autocomplete.

The first two of these (at least) return their results to the listener/controller straight away when startSearch is called.

When we do a subsequent search (e.g. going from "S" to "St"), only the first search defined in the "autocompletesearch" attribute gets its previous result back. The second search just gets null.

I'm attaching a xpcshell test that fails due to this bug.

This is a real pain because we can't do efficient searching on subsequent changes.

Comment 1

[[e-mail]]

Attachment: XPFE fix

Comment 2

[Andrew Schultz]

Comment on attachment 324817 [details] [diff] [review]
XPFE fix

>@@ -649,7 +649,7 @@
>           {
>             this.mLastResults[aSessionName] = null;
>             if (firstReturn)
>-              this.clearResults(false);
>+              this.clearResultElements(false);

Hmm.  This seems to be the only place data is cleared out now.  Should this be doing something to null out the param like clearResultData?

>             this.searchFailed();
>             return;
>           } else if (aStatus == 

Feel free to remove else-after-return here.

>-            this.clearResults(false); // clear results, but don't repaint yet
>+            this.clearResultElements(false); // clear results, but don't repaint yet

Comment is better now than it was before.  :)

>           // if all searches are done and they all failed...

>-              this.clearResults(true); // clear data and repaint empty
>+              this.clearResultElements(true); // clear data and repaint empty

Comment isn't right anymore.  does this only not need to clear the data because there's no data to clear?  Is that true even if called from startLookup?  I'd think startLookup would want to clear any results for the session that failed.

It looks like clearResults is now only called by the desctructor.  Perhaps just inline clearResults to there.

Comment 3

[[e-mail]]

(In reply to comment #2)
>(From update of attachment 324817 [details] [diff] [review])
>>             this.mLastResults[aSessionName] = null;
>Should this be doing something to null out the param like clearResultData?
param is going away with the toolkit switch, no need to worry about it.

>>-              this.clearResults(true); // clear data and repaint empty
>>+              this.clearResultElements(true); // clear data and repaint empty
>Comment isn't right anymore.  does this only not need to clear the data because
>there's no data to clear?  Is that true even if called from startLookup?  I'd
>think startLookup would want to clear any results for the session that failed.
You're right, this change is probably incorrect.

Comment 4

[[e-mail]]

Attachment: [checked in] Addressed review comments

Comment 5

[[e-mail]]

Fix checked in.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 327390 [details] [diff] [review]
[checked in] Addressed review comments

>           {
>             this.mLastResults[aSessionName] = null;
>             if (firstReturn)
>-              this.clearResults(false);
>+              this.clearResultElements(false);
>             this.searchFailed();
>             return;
>-          } else if (aStatus == 
>-                     Components.interfaces.nsIAutoCompleteStatus.failureItems){
>+          if (aStatus == Components.interfaces.nsIAutoCompleteStatus.failureItems)
>             ++this.mFailureItems;
>-          }

You missed adding the } back in after the return statement. I've checked in a fixed as autocomplete is pretty much broken without that.

Comment 7

[Justin Wood (:Callek)]

Patch checked into mozilla-central in Bug 443837

Comment 8

[Mark Banner (:standard8)]

Reopening - this is NOT fixed for toolkit (which this bug was originally filed for. The testcase attached should still be valid for toolkit builds (though I haven't checked it yet.

Comment 9

[Mark Banner (:standard8)]

Attachment: Toolkit fix

This is the fix for toolkit. It also fixes several other bugs:

- mMatchCounts was initially set to the length of the searchCount, not the capacity. I'm not sure why this hasn't really showed up at all, possibly because ClearResults() was clearing the list.
- When looking up the previous results index, a pointer to the previous results was used, this may not be the case, if the caller created a new set of results and copied the relevant ones from the previous, additionally, we know the index anyway.

Alongside those, the main part of this fix is to not clear out the stored results when we're just doing a follow-on search (and just zero the matches), and therefore this keeps the results available for the next search.

Includes the testcase that I did when I originally filed this bug.

Comment 10

[[e-mail]]

Comment on attachment 334328 [details] [diff] [review]
Toolkit fix

>+  PRUint32 length = 0;
>+  mResults->Count(&length);
>+  if (aSearchIndex >= (PRInt32)length) {
I don't think you can guarantee the order of the results.

>+      return;
>+    }
>+    else {
Nit: else after return

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> (From update of attachment 334328 [details] [diff] [review])
> >+  PRUint32 length = 0;
> >+  mResults->Count(&length);
> >+  if (aSearchIndex >= (PRInt32)length) {
> I don't think you can guarantee the order of the results.

Actually you can, see nsAutoCompleteController::OnSearchResult:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#668

Comment 12

[[e-mail]]

What I mean is, what happens if search 2 returns results before search 1 does?

Comment 13

[Neil Deakin]

Comment on attachment 334328 [details] [diff] [review]
Toolkit fix

nsAutoCompleteController::ProcessResult(
>   if (aResult)
>     aResult->GetMatchCount(&matchCount);
> 
>-  PRInt32 oldIndex = mResults->IndexOf(aResult);
>-  if (oldIndex == -1) {
>+  PRUint32 length = 0;
>+  mResults->Count(&length);
>+  if (aSearchIndex >= (PRInt32)length) {
>     // cache the result
>     mResults->AppendElement(aResult);
>     mMatchCounts.AppendElement(matchCount);
>   }
>   else {
>     // replace the cached result
>-    mResults->ReplaceElementAt(aResult, oldIndex);
>+    mResults->ReplaceElementAt(aResult, aSearchIndex);
>     oldMatchCount = mMatchCounts[aSearchIndex];
>-    mMatchCounts[oldIndex] = matchCount;
>+    mMatchCounts[aSearchIndex] = matchCount;
>   }

I don't understand this code much at all. Can you explain this part. What does mMatchCounts hold?

Is a result just one item of output (that is, there is one result per line in the autocomplete box), or multiple items?

Comment 14

[Mark Banner (:standard8)]

Attachment: Toolkit fix v2

Here's a much simpler fix that Neil Rashbrook suggested to me over irc.

mResults is an array of the results that were returned from the previous searches; each element in the array is an nsIAutoCompleteResult item returned from the search containing the individual results, so we'll get one item per search type used (e.g. history / addrbook / mydomain).

This patch caches the mResults array before calling StartSearch. This means when the first search comes back and forces all the results arrays to be cleared (which could be synchronous and occur before starting the search on the second search session), we'll still have a local cache of the results to pass the previous results correctly back to the searches that haven't been started yet.

Comment 15

[Neil Deakin]

Comment on attachment 334686 [details] [diff] [review]
Toolkit fix v2

Yes, this looks much better. Do we want to cleart the cached items once all searches have completed?

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> (From update of attachment 334686 [details] [diff] [review])
> Yes, this looks much better. Do we want to cleart the cached items once all
> searches have completed?
> 
Its a cache local to the function, therefore shouldn't the array clear these for us?

Comment 17

[Mark Banner (:standard8)]

Attachment: Toolkit fix v3

My apologies, I missed including the testcase last time, not sure if you wanted to look at it again or not.

Comment 18

[Mark Banner (:standard8)]

I checked this in as changeset id 379931a16cd7, I then backed out under id 20663933d4b0 due to what looked like a Lk/MH regression (all platforms, worst on Linux). However, Lk/MH didn't go back down again. So I think that the tinderbox regression was actually due to something else (although no other patches landed), so I'll try relanding in a couple of days.

Comment 19

[Mark Banner (:standard8)]

Checked in again as changeset 18409:166ebc402d37

No significant changes in Lk/MH so it looks like it was originally a tinderbox issue.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/04bd0b3e026d
fix xpfe autocomplete bustage from bug 438861 to be really in sync with cvs