720792 - Implement a better solution to start selected searches without a timeout

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Marco Bonardo [:mak]]

The current solution is hackish and breaks Seamonkey (in the sense it brings the timeout from 50ms to 100ms).
We should figure out a better and more compatible solution.

Comment 1

[Marco Bonardo [:mak]]

Attachment: backout current hack

This back outs the current hack, so we can restart from where we were.

Comment 2

[Marco Bonardo [:mak]]

Attachment: wip

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I've tried to be conservative in the patch, so that if there aren't immediate searches the code should follow the exact path it was before. Otherwise it will begin the search first.

I don't know what could I specifically test here, we already have various search tests, and also one for sync+async searches that was added in bug 660156. So, let me know if you think I may add a specific test for some part of this, at first glance the existing coverage is looking acceptable.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 591267 [details] [diff] [review]
backout current hack

well, this will also need a review, though it's just undoing change.

Comment 5

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

Comment on attachment 591267 [details] [diff] [review]
backout current hack

I guess we don't really need to undo the nsAutoCompleteController.cpp change, right? That one seems to make sense regardless of the other changes. Maybe it's safer to just back it all out though.

Comment 6

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

Comment on attachment 591431 [details] [diff] [review]
patch v1.0

>diff --git a/toolkit/components/autocomplete/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/nsAutoCompleteController.cpp

>+nsAutoCompleteController::BeforeStartSearch()

>+  mResultCache.Clear();
>+  if (!mResultCache.AppendObjects(mResults)) {

This seems to "leak" the result objects (hold references to them until the next search begins, or until shutdown). The original purpose of this code seems to be to keep a reference to them across calls to startSearch (since the search can call back into nsAutoCompleteController::OnSearchResult synchronously, and that might somehow end up clearing the cache, or something like that? Such a mess). Assuming that's true, can't you just keep the cache local to a given nsAutoCompleteController::StartSearch invocation?

>+nsAutoCompleteController::AfterStartSearch()
>+{
>+  mSearchesFailed = 0;
>+  mResultCache.Clear();
>+  if (mSearchesFailed == mSearches.Count())

This looks buggy - I imagine you want to reset mSearchesFailed after checking its value.

> nsAutoCompleteController::StartSearchTimer()

This could probably use a re-name - StartSearches()?

Overall this looks OK, but I'm sad that it makes stuff even more complicated. The urge to burn this all to the ground and re-design it is growing as I read more and more of these patches... It seems unlikely that this could be fixed incrementally given the current APIs.

Comment 7

[Marco Bonardo [:mak]]

So, I'm going to push the backout and disable the feature by default till we get part 2 done, so we are ok with Seamonkey and the merge in the meanwhile.

Later I'll look again at the code and answer you questions.

(In reply to Gavin Sharp (use [e-mail] for email) from comment #5)
> Comment on attachment 591267 [details] [diff] [review]
> backout current hack
> 
> I guess we don't really need to undo the nsAutoCompleteController.cpp
> change, right? That one seems to make sense regardless of the other changes.
> Maybe it's safer to just back it all out though.

Yes, I can try to reinsert this concept in part 2, even if that may require to dupe some code, though in part 1 I prefer just backing out the changes.

Comment 8

[Marco Bonardo [:mak]]

part1: backout delay changes
https://hg.mozilla.org/mozilla-central/rev/0b19bd7d123b
Disable autoFill by default till we land part 2
https://hg.mozilla.org/mozilla-central/rev/63757aa2e629

Comment 9

[Marco Bonardo [:mak]]

and disable the b-c test that doesn't wait for a delay
https://hg.mozilla.org/mozilla-central/rev/1f3c7fa158aa

Comment 10

[Marco Bonardo [:mak]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #6)

> >+  mResultCache.Clear();
> >+  if (!mResultCache.AppendObjects(mResults)) {
> 
> This seems to "leak" the result objects (hold references to them until the
> next search begins, or until shutdown).

mResultCache is Clear()ed in AfterStartSearch() (for clarity I'm renaming these to BeforeSearches() and AfterSearches()), that is fired as soon as all the searches have been started, so I don't think it may leak till shutdown.

> The original purpose of this code
> seems to be to keep a reference to them across calls to startSearch (since
> the search can call back into nsAutoCompleteController::OnSearchResult
> synchronously, and that might somehow end up clearing the cache, or
> something like that? Such a mess). Assuming that's true, can't you just keep
> the cache local to a given nsAutoCompleteController::StartSearch invocation?

The resultCache code has been added in bug 438861, to support search fields with multiple searches. Looks like on results reuse a search may clear mResults before the next ones can use their previous result. Based on this, I think may be possible that the synchronous queries clear results to the asynchronous queries, so I don't think we can restrict the cache to a single StartSearch() call.
Though I'm not sure what exactly causes the call to ClearResults, I have some suspect on the backspace handling, that is really bogus, as I discovered in bug 719888.

Comment 11

[Marco Bonardo [:mak]]

Ugh, looks like what clears the results is processResult, but it's doing it correctly, so it's like that bug fixed the controller for a bug in the search.
The query should pass either RESULT_SUCCESS_ONGOING or RESULT_NOMATCH_ONGOING to keep the result alive and use it incrementally but it doesn't, so processResult first decrements mSearchesOngoing  and then
  if (mSearchesOngoing == 0) {
    // If this is the last search to return, cleanup.
    PostSearchCleanup();
  }
clears mResults.
Then the search tries to reuse the previous result after this point.
At least this is what test_previousResult.js is doing, not surely ensuring something works.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.1

This fixes the previous ccmments, but mResultsCache one, cause I don't think it leaks, being cleared as soon as all the searches have been started.

What really happens is that, on StartSearch, mFirstSearchResult is set to true, and the first StartSearch command clears mResults seeing it true. Then the next searches are unable to pass the previous result, thus the need for the cache to feed them.  For this reason it can't be made local to a specific startSearch.

My first thought was to remove the need to clear mResults on the first startSearch, by using insertObjectAt in mResults rather than AppendObject. Though this way it may be possible to leak some result for a longer time, if some StartSearch call doesn't bring to actually adding a result for whatever error.
So, also to reduce the number of changes, I decided to keep it as it was.

Comment 13

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

Comment on attachment 594947 [details] [diff] [review]
patch v1.1

>diff --git a/toolkit/components/autocomplete/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/nsAutoCompleteController.cpp

>@@ -156,40 +158,50 @@ nsAutoCompleteController::SetInput(nsIAu

>+      if (searchDesc && searchDesc->GetSearchType(&searchType) &&

I think you want an NS_SUCCEDED here?

Wouldn't this bug have caused mImmediateSearchesCount to never be > 0, and cause the immediate searches to never be called? Did you just attach the wrong patch?

>+nsAutoCompleteController::StartSearches()

>+    if (timeout == 0)

>+    if (mSearches.Count() == immediateSearchesCount) {

These seem like equivalent checks, given the logic above. Can you merge them?

>+      // All the searches haveen started, just finish.

"have been"

This looks good otherwise, but I'm worried about the missing NS_SUCCEEDED. How is the test coverage for this stuff?

Comment 14

[Marco Bonardo [:mak]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #13)
> I think you want an NS_SUCCEDED here?
> 
> Wouldn't this bug have caused mImmediateSearchesCount to never be > 0, and
> cause the immediate searches to never be called? Did you just attach the
> wrong patch?

No, I was just dumb. actually this causes us to consider all searches delayed (it's the fallback value) so they don't start immediately. That means I need a new test to ensure immediate searches start immediately! will make it.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Address comments and adds 3 dedicated tests for immediate searches and timeout 0.

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91ca0cc06f46

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/91ca0cc06f46

Comment 18

[Steffen Wilberg]

(In reply to Marco Bonardo [:mak] from comment #8)
> Disable autoFill by default till we land part 2
> https://hg.mozilla.org/mozilla-central/rev/63757aa2e629
So what's the plan for reenabling autoFill?

Comment 19

[Marco Bonardo [:mak]]

(In reply to Steffen Wilberg from comment #18)
> So what's the plan for reenabling autoFill?

we just need the review in bug 720659 for proper IME support.