Race condition between sync and async autocomplete searches

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Autocomplete
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Michael Ventnor, Assigned: Michael Ventnor)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 566489, 659445
(Assignee)

Comment 1

7 years ago
Created attachment 535574 [details] [diff] [review]
Patch
Assignee: nobody → ventnor.bugzilla
Attachment #535574 - Flags: review?(sdwilsh)
(Assignee)

Comment 2

7 years ago
Created attachment 535621 [details] [diff] [review]
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.
Attachment #535574 - Attachment is obsolete: true
Attachment #535574 - Flags: review?(sdwilsh)
Attachment #535621 - Flags: review?(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.
Attachment #535621 - Flags: feedback?(edilee)

Comment 4

7 years ago
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.
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. :)
(Assignee)

Comment 6

7 years ago
(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 on attachment 535621 [details] [diff] [review]
Patch 2

Clearing this request until we get a better UX spec here.
Attachment #535621 - Flags: review?(sdwilsh)

Comment 8

7 years ago
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

7 years ago
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.)
(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 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...
(Assignee)

Comment 12

7 years ago
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.
Attachment #535621 - Flags: feedback?(edilee) → feedback?(gavin.sharp)
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)
Attachment #535621 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 14

7 years ago
Created attachment 541616 [details] [diff] [review]
Patch 3

This gives us the xpfe behaviour, and removes the nonsense line of code.
Attachment #535621 - Attachment is obsolete: true
Attachment #541616 - Flags: review?(sdwilsh)
(Assignee)

Updated

7 years ago
Attachment #541616 - Flags: review?(sdwilsh) → review?(mak77)
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
Attachment #541616 - Flags: review?(mak77) → review-
(Assignee)

Comment 16

7 years ago
Created attachment 546734 [details] [diff] [review]
Patch 4
Attachment #541616 - Attachment is obsolete: true
Attachment #546734 - Flags: review?(mak77)
Comment on attachment 546734 [details] [diff] [review]
Patch 4

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

The changes look good
Attachment #546734 - Flags: review?(mak77) → review+
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.
(Assignee)

Comment 19

6 years ago
Created attachment 575328 [details] [diff] [review]
Updated to tip
Attachment #546734 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f23efef7f616
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.