414581 - Autocomplete shows no results for some words, sometimes

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Ed Lee :Mardak]

Now that we returned to chunking meaning we search multiple times, this problem is back. E.g., type "w", see many results, then "wo", see results, then "wor" some results, "word", no results, "words", see results.

This was accidentally fixed with global frecency bug 394038 because the first chunk would always have enough results with bug 414257.

This problem can be somewhat avoided by de-focusing (select search bar) and refocusing the location bar and then hiting down to retrigger the history search.

However, this is on-par with how Beta 2 behaved because we were chunking by time instead of frecency.

My suspicion is the timer callback and thread-(un)-safety-ness of using the shared member variables for starting a new search as well as continuing a search with the timeout.

I don't have a reliable step to reproduce, but I've seen it occasionally when I don't use my own builds with adaptive search, etc. I've heard reports from friends that "sometimes it just stops searching when I type out words"

Comment 1

[Seth Spitzer]

> E.g., type "w", see many results, then "wo", see results, then "wor"
> some results, "word", no results, "words", see results.
>
> However, this is on-par with how Beta 2 behaved because we were chunking by
> time instead of frecency.

I've never seen that, pre-global frecency or post-global frecency.

Is it possible that one of the recent changes has caused this problem?

As far as i understand what's checked in, we are no longer re-using search results, and upon search we should be starting over at the first chunk.

perhaps something is wrong with "start over at the first chunk" code?

Comment 2

[Seth Spitzer]

> "sometimes it just stops searching when I type out words"

hmm, I wonder if that could be related to the timeout?

I've just updated to Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre, so I'll keep an eye out for any weirdness.

Comment 3

[Shawn Wilsher :sdwilsh]

I've seen it before and after global frecency.

Comment 4

[Seth Spitzer]

> I've seen it before and after global frecency.

thanks for the datapoint, shawn.  

can you elaborate on what you were seeing?

Comment 5

[Seth Spitzer]

zoinks, I just hit this, with the "words" example.

to clarify, I have this in my history:

http://johnbokma.com/firefox/keymarks-explained.html

with the title of "Firefox smart keywords explained: quick search from the address bar"

when I typed "words " I got this one hit, but when I added the "e" (to give me "words e" I got no results, but when I added the "x" (to give me "words ex") I got the result.

interestingly, I just revisited that page, so that probably moved it into the first frecency chunk and no I don't see the bug!

but I am able to reproduce with another site, not in my frecency chunk.

fwiw, I *do* remember seeing soemthing like this a while back, pre-global frecency, but I also remember fixing it.

let me review my notes and checkins.  but at least it is reproducable.

Comment 6

[Seth Spitzer]

> but I am able to reproduce with another site, not in my frecency chunk.

I meant:  but I am able to reproduce with another site, not in the first chunk.

Comment 7

[Seth Spitzer]

from the sidelines, here's a suggestion on how to start working on this:

when call AutoCompleteFullHistorySearch() and there is no match in the first chunk....

210     rv = AutoCompleteFullHistorySearch(&moreChunksToSearch);
211   }
212   NS_ENSURE_SUCCESS(rv, rv);

....moreChunksToSearch will be true, but we have not found a hit in the first chunk, so...

213  
214   // Determine the result of the search
215   PRUint32 count;
216   mCurrentResult->GetMatchCount(&count); 

...count is 0.

218   if (count > 0) {
219     mCurrentResult->SetSearchResult(moreChunksToSearch ?
220       nsIAutoCompleteResult::RESULT_SUCCESS_ONGOING :
221       nsIAutoCompleteResult::RESULT_SUCCESS);
222     mCurrentResult->SetDefaultIndex(0);
223   } else {

so we are in here.

224     mCurrentResult->SetSearchResult(moreChunksToSearch ?
225       nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING :
226       nsIAutoCompleteResult::RESULT_NOMATCH);
227     mCurrentResult->SetDefaultIndex(-1);
228   }

I havent't checked, but we should see if SetDefaultIndex(-1) stops the search or not (or if we aren't handing those result codes correctly, but I think we are.)

Comment 8

[Ed Lee :Mardak]

Attachment: v1

It seems like the timer code is safer than I thought.. or maybe it still mystifies me, but there can only be one timer going off at a time even?

This fixes the problem for me. Basically, it fails to continue searching if the first chunk returns nothing.

I got tipped off when I added a tag for the word that caused no results to show up. In my case it was "unif".

Comment 9

[Ed Lee :Mardak]

This is also(In reply to comment #8)
> I got tipped off when I added a tag for the word that caused no results to show
> up. In my case it was "unif".
This is also why my adaptive learning avoided this problem because previous inputs are put at the front of the list so it was more likely to have a result.

Comment 10

[Ed Lee :Mardak]

Comment on attachment 300039 [details] [diff] [review]
v1

Any insights into why this fixes the problem? I haven't looked into what those other methods do. (I'm in class :p)

Comment 11

[Ed Lee :Mardak]

Attachment: screenshot of v1

He he he...... One unintended "feature" of this current patch is that we don't clear the results if we don't find something. This means it's easier for a user to "back track" to a previous search term that did match something. So if you typo a domain but it did happen to match the site you wanted before you hit the typo, you can still hit down and select it.

Do we want this "feature" (it's not a bug! ;) .. :D )

Comment 12

[Seth Spitzer]

obviously faaborg/beltzner/mconnors vote counts more than me, but I don't think we want this "feature".

I am also having doubts about the correctness of this fix.

> Any insights into why this fixes the problem? 

Well, by not calling mCurrentResult->SetListener(), this block never "happens"

     mCurrentResult->SetSearchResult(moreChunksToSearch ?
       nsIAutoCompleteResult::RESULT_NOMATCH_ONGOING :
       nsIAutoCompleteResult::RESULT_NOMATCH);
     mCurrentResult->SetDefaultIndex(-1);

I haven't debugged, but can you (or dietrich) see if "mCurrentResult->SetDefaultIndex(-1);" has the side effect of stopping the search?  

I think that would explain this problem.

Comment 13

[Seth Spitzer]

Comment on attachment 300039 [details] [diff] [review]
v1

I don't think this is right, but if you have new evidence, please see re-review.

Comment 14

[Ed Lee :Mardak]

(In reply to comment #12)
> I haven't debugged, but can you (or dietrich) see if
> "mCurrentResult->SetDefaultIndex(-1);" has the side effect of stopping the
> search?  
I already tried commenting out set index to -1 before I attached the patch.

The program flow goes through the else where it sets NOMATCH_ONGOING and index to -1. Skipping those doesn't solve the problem. Always going into the if (count > 0) condition with if (1) doesn't help.

StartAutoCompleteTimer is correctly called and not DoneSearch.

Comment 15

[Seth Spitzer]

bummer that my guess about the problem is not helping.  thanks for looking into it.

> StartAutoCompleteTimer is correctly called and not DoneSearch.

So the timer keeps firing and we keep searching?  That's a good sign!

Is something else closing the popup, or preventing us from opening it?

I feel that we're getting close.  Standing by for a quick r= if you (or someone else) solves this riddle.

Sorry that I'm not able to debug myself at the moment.

Thanks again for doing all this post-global-frecency cleanup, bug fixing, etc.  I really appreciate it.

Comment 16

[Ed Lee :Mardak]

(In reply to comment #15)
> > StartAutoCompleteTimer is correctly called and not DoneSearch.
> So the timer keeps firing and we keep searching?  That's a good sign!
Sorry. I forgot to mention that it only fires once. It calls StartAutoCompleteTimer, but it never comes back. Perhaps something else is canceling it. I'll step through nsAutoCompleteController.cpp's ProcessResult.

Comment 17

[Ed Lee :Mardak]

Long story short.. we have 0 search results, so the popup is closed which stops the search.

nsNavHistory::PerformAutoComplete
  -> mCurrentListener->OnSearchResult(this, mCurrentResult)

nsAutoCompleteController::OnSearchResult
  -> ProcessResult(i, aResult);

nsAutoCompleteController::ProcessResult
  -> if (mRowCount) else ClosePopup()

nsAutoCompleteController::ClosePopup
  -> mInput->SetPopupOpen(PR_FALSE)

autocomplete.xml::nsIAutoCompleteInput::property name="popupOpen"
  -> if (val) else else this.closePopup()

... does that call.. this?
autocomplete.xml::nsIAutoCompletePopup::method name="closePopup"
  -> this.hidePopup()

... ?? then somewhere in javascript land.. here?
autocomplete.xml::nsIAutoCompletePopup::handler event="popuphiding"
  -> controller.stopSearch()

nsAutoCompleteController::StopSearch
  -> search->StopSearch()

nsNavHistory::StopSearch
  -> mAutoCompleteTimer->Cancel()

Would it be right to not call stopSearch here? Added from bug 389593.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/content/widgets/autocomplete.xml&rev=1.110&mark=848-849#844
Or check controller.searchStatus != nsIAutoCompleteController::STATUS_SEARCHING
But I suppose the point of calling stopSearch is to stop a search that's currently searching..

Comment 18

[Ed Lee :Mardak]

Attachment: v2

Don't call stopSearch when the popup is hidden.

There's still many ways for searches to stop from the controller:
SetInput, HandleText, HandleEscape, HandleStartComposition, mProcessResult && EnterAfterSearch

Comment 19

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

Hmm, perhaps this was all my fault! See bug 389593 comment 4 where I suggested adding this. Seth noted in bug 389593 comment 6 that that probably wouldn't work very well, but it seems to have survived in that patch (see also bug 389593 comment 6).

If we take this patch we should perhaps restore the handlers quoted in the last part of bug 389593 comment 4.

Comment 20

[Ed Lee :Mardak]

Attachment: v2.1

All calls to ClosePopup are now dominated by a call to StopSearch except

1) ProcessResult when it finds no results yet (fixing this bug)
2) PostSearchCleanup which is after search is done
3) EnterMatch which checks STATUS_SEARCHING and sets mEnterAfterSearch which calls StopSearch from ProcessResult

Comment 21

[Ed Lee :Mardak]

Attachment: v2.1

Meh. qrefresh didn't pick up this other change.

Comment 22

[Seth Spitzer]

Comment on attachment 300125 [details] [diff] [review]
v2.1

the intent of the fix feels right to me.

but it sounds like gavin (from comment #19) is all over this review, so I'm going to stay out of the way and let him review.

please double check that we do stop the search if the user hits escape or clicks away, closing the popup.  

can you check that?

Comment 23

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 300044 [details]
screenshot of v1

I personally like this (we still need a message when there are 0 initial results), but reassigning to beltzner to make sure he is onboard with the idea.

Comment 24

[Ed Lee :Mardak]

Adding the two handlers doesn't help stop the search if you click away from the location bar.. e.g., click the page. The focus is no longer in the bar, so pressing esc doesn't help after either.

<handler event="contextmenu" phase="capturing" action="this.mController.stopSearch(); this.closePopup();"/>
<handler event="mousedown" phase="capturing" action="this.mController.stopSearch();"/>

The search does stop if you move the cursor left/right, hit esc, hit enter, type stuff, erase stuff. Just not clicking away. I'm not quite sure what you mean by "closing the popup" -- clicking the drop down menu? which opens the popup?

Comment 25

[Seth Spitzer]

> I'm not quite sure what you mean by "closing the popup"

To clarify, I meant: do we stop searching if the user:

a)  hits the escape key
b)  switches focus to another element (like the google search bar, with ctrl k)

if we are continuing to search in the background in either case, we should fix that.

Comment 26

[Seth Spitzer]

I am probably over stepping my bounds by doing p1 / m11 / beta 3, but I think we really want this fix (once reviewed by gavin.)

dietrich / edward / gavin, do you agree?

Comment 27

[Ed Lee :Mardak]

The current fix will get rid of the problem, but it introduces a new one if you use the mouse to click the page when it's still searching. The user can still cancel the search by pressing esc, selecting a result, moving the cursor with the keyboard.

Comment 28

[Ed Lee :Mardak]

Attachment: v3

Don't close the popup if we're going to get more results soon!

Comment 29

[Seth Spitzer]

> Don't close the popup if we're going to get more results soon!

nice, thanks edward (and gavin).

I take it this fix, unlike the previous one, does not introduce the new problem listed in comment #27?

Comment 30

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

(In reply to comment #29)
> I take it this fix, unlike the previous one, does not introduce the new problem
> listed in comment #27?

Right. From #places:
<Mardak> looks good for me
<Mardak> popup goes away when moving cursor to the sides, clicking the page, hitting esc, hitting enter, changing text

Comment 31

[Mike Connor [:mconnor]]

Comment on attachment 300291 [details] [diff] [review]
v3

please land ASAP, a=mconnor on behalf of drivers

Comment 32

[Ed Lee :Mardak]

Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp,v  <--  nsAutoCompleteController.cpp
new revision: 1.76; previous revision: 1.75
done

Comment 34

[Chris Blore]

VERIFIED FIXED on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre