660156 - Race condition between sync and async autocomplete searches

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Michael Ventnor]

Well, OK, not *really* a race condition, but I couldn't think of a better concise way to describe it.

Say you have an async autocomplete search (say, the places history DB) that is put into mResults at index 0. Say you have a second search, a sync one (say, the inline URL filler), put into mResults at index 1.

Both results are fired, and the sync one by its very nature always returns first. What happens in ProcessResult is interesting, upon getting the first result, we clear the mResults array (recall the sync search was index 1) and add back that search to the cleared mResults.

Oops, now our sync result is at index 0! The subsequent code which checks for index 1 fails.

Still with me? The best part is, it's a one-liner fix. I'll attach a patch when I write a test.

Comment 1

[Michael Ventnor]

Attachment: Patch

Comment 2

[Michael Ventnor]

Attachment: Patch 2

Actually, while that patch works, it isn't quite correct with the intentions of variables.

This patch does the same thing, but uses different variables and renames parameters to make it clearer and more correct.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 535621 [details] [diff] [review]
Patch 2

Edward has a lot of experience with this, so I'm going to have him do a first pass here.

Comment 4

[Ed Lee :Mardak]

We might want to take a step back and figure out what's the expected behavior that we want when there's 2+ autocomplete search engines.

The existing code will append the results as OnSearchResult is called independent of the original order of the autocompletesearch attribute. This shows up in the UI as whichever results come back first will appear first.

The bug here happens when an earlier but slower search gives results, it will try completing to the later but faster results -- these results are visually first in the list.

The fix here makes the controller complete to the result that was just OnSearchResulted, but only if the faster result did not complete anything to begin with as the faster result completing will cause text to be selected and cancel the completing logic.

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

I also suggested one possible way to do this in https://bugzilla.mozilla.org/show_bug.cgi?id=659445#c8 — not sure if it's relevant to this particular issue, but wanted to make sure you see it. :)

Comment 6

[Michael Ventnor]

(In reply to comment #4)
> We might want to take a step back and figure out what's the expected
> behavior that we want when there's 2+ autocomplete search engines.
> 
> The existing code will append the results as OnSearchResult is called
> independent of the original order of the autocompletesearch attribute. This
> shows up in the UI as whichever results come back first will appear first.
> 
> The bug here happens when an earlier but slower search gives results, it
> will try completing to the later but faster results -- these results are
> visually first in the list.
> 
> The fix here makes the controller complete to the result that was just
> OnSearchResulted, but only if the faster result did not complete anything to
> begin with as the faster result completing will cause text to be selected
> and cancel the completing logic.

Without this patch, if a search other than the first specified one is the one to complete, then it won't complete at all. That behaviour is clearly wrong.

What should be the case is that only one of the searches should be able to complete by having the defaultIndex set to > -1. If not, then that is a bug by the caller, I cannot think of a way that we can sanely and consistently deal with it. When I implement the actual autocomplete, I will make sure this is the case.

Do you have a different idea, Edward?

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 535621 [details] [diff] [review]
Patch 2

Clearing this request until we get a better UX spec here.

Comment 8

[[e-mail]]

Comment on attachment 535621 [details] [diff] [review]
Patch 2

>     oldMatchCount = mMatchCounts[aSearchIndex];
>-    mMatchCounts[oldIndex] = matchCount;
>+    mMatchCounts[resultIndex] = matchCount;
It's a good thing that mMatchCounts isn't used...

Comment 9

[[e-mail]]

FYI, XPFE autocomplete prefills using the first returned result that provides a default index. (If no result provides a default index, then the first result of the first search in the list that provides a success result is used. I'm not claiming that any of this makes sense.)

Comment 10

[[e-mail]]

(In reply to comment #8)
>(From update of attachment 535621 [details] [diff] [review])
>>     oldMatchCount = mMatchCounts[aSearchIndex];
>>-    mMatchCounts[oldIndex] = matchCount;
>>+    mMatchCounts[resultIndex] = matchCount;
>It's a good thing that mMatchCounts isn't used...
...although it could in theory be used by RowIndexToSearch.

Comment 11

[[e-mail]]

Comment on attachment 535621 [details] [diff] [review]
Patch 2

>-  PRInt32 oldIndex = mResults.IndexOf(aResult);
>-  if (oldIndex == -1) {
>     // cache the result
>     mResults.AppendObject(aResult);
>     mMatchCounts.AppendElement(matchCount);
>   }
>   else {
>     // replace the cached result
>-    mResults.ReplaceObjectAt(aResult, oldIndex);
While I'm here, this code makes no sense either, since the object at oldIndex is of course the one we just found the index of above...

Comment 12

[Michael Ventnor]

Comment on attachment 535621 [details] [diff] [review]
Patch 2

(In reply to comment #9)
> FYI, XPFE autocomplete prefills using the first returned result that
> provides a default index. (If no result provides a default index, then the
> first result of the first search in the list that provides a success result
> is used. I'm not claiming that any of this makes sense.)

That's actually precisely what I want to do in toolkit (except for the part in parentheses, we don't complete anything in that case).

Yes, there does seem to be some code that could probably be removed...

Over IRC, Mardak reckons that gavin will give better feedback, so changing the flag to him.

Comment 13

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

Comment on attachment 535621 [details] [diff] [review]
Patch 2

Following xpfe's approach and removing the bogus code seems like the best way forward (I suppose that means feedback- on this specific patch? I dunno)

I noticed a few nits in the test: lack of "this." in AutoCompleteInput.prototype.selectTextRange, missing a .bind(this) in AutoCompleteSearch.prototype.startSearch (I don't see how that method can work as-is)

Comment 14

[Michael Ventnor]

Attachment: Patch 3

This gives us the xpfe behaviour, and removes the nonsense line of code.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 541616 [details] [diff] [review]
Patch 3

Review of attachment 541616 [details] [diff] [review]:
-----------------------------------------------------------------

As usual, nits are not needed for a positive review, but welcome if you fix'em.
The main issue here is that the test seems bogus.

Regarding interaction, I think once all related bugs have addressed review comments, even if patches may still have a pending final review pass, they should land on UX, to gather feedback asap from both usual UX testers and limi, in order to figure out if all the issues with the original approach are properly handled and the order of results seems consistent. We all may probably want to play with it sooner than later.

::: toolkit/components/autocomplete/tests/unit/test_660156.js
@@ +7,5 @@
> +function AutoCompleteSearch(aName, aResult, aIsSync) {
> +  this.name = aName;
> +  this._result = aResult;
> +  this._sync = aIsSync;
> +}

I'd prefer if you'd create 2 inherited objects, AutoCompleteAsyncSearch and AutoCompleteSyncSearch:
- aIsSync is confusing me, maybe because I saw too much Sync code (mostly it may make harder to parse mxr searches)
- the code paths are completely splitted (if sync ... else ...)
- their usage below would be self documenting, rather than relying on an anonymous bool argument

@@ +9,5 @@
> +  this._result = aResult;
> +  this._sync = aIsSync;
> +}
> +AutoCompleteSearch.prototype = Object.create(AutoCompleteSearchBase.prototype);
> +AutoCompleteSearch.prototype.constructor = AutoCompleteSearch;

nit: As I said in the other review, I suppose this seems unneeded if you don't override it in the base object

@@ +18,5 @@
> +{
> +  if (this._sync) {
> +    this._returnResults(aListener);
> +  } else {
> +    setTimeout(500, this._returnResults, aListener);

I don't think this works at all, it seems running on the wrong scope (did you want to this._returnResults.bind(this)?) and the setTimeout arguments are inverted... This means you are not actually testing the right interaction. you should probably change the test to ensure the async query runs and returns some correct results.

@@ +28,5 @@
> +  var result = this._result;
> +
> +  result.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS;
> +  aListener.onSearchResult(this, result);
> +};

nit: the bracing style is inconsistent across methods

@@ +58,5 @@
> +                             false);
> +  // Sync search
> +  var search2 = new AutoCompleteSearch("search2",
> +                             new AutoCompleteResult(results, true),
> +                             true);

nit: indentation is wrong
nit: you may want to rename search1 and search2 to syncSearch and asyncSearch

Comment 16

[Michael Ventnor]

Attachment: Patch 4

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 546734 [details] [diff] [review]
Patch 4

Review of attachment 546734 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look good

Comment 18

[Marco Bonardo [:mak]]

may I ask to please not land separate parts of this feature till all dependent bugs are in a good shape. I'd prefer if all the parts land at the same time.

Comment 19

[Michael Ventnor]

Attachment: Updated to tip

Comment 20

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f23efef7f616

Comment 21

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/f23efef7f616