389593 - handle repeated calls to ProcessResult() [allow for incremental autocomplete search results]

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[(not reading, please use [e-mail] instead)]

handle repeated calls to ProcessResult() [allow for incremental autocomplete search results]

I found this while working on bug #389491, where I repeatedly call nsAutoCompleteController::OnSearchResult() which can call ProcessResults().
[looking at lxr, no one seems to do what I'm attempting to do.]

if you call OnSearchResult() only once, like you see in lxr, the current code works because mRowCount starts at 0, so oldRowCount is 0.

I'll attach a patch.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 2

[(not reading, please use [e-mail] instead)]

accepting for m8, as this blocks #389491 (which should land for m8)

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

this is the toolkit part of bug #389491 that doesn't have to do with places.

Comment 4

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

Comment on attachment 275665 [details] [diff] [review]
updated patch

>Index: toolkit/components/autocomplete/public/nsIAutoCompleteController.idl

>+   * Stops a asynchronous search.

nit: "an"

>Index: toolkit/components/autocomplete/src/nsAutoCompleteController.cpp

StopSearch() calls ClearSearchTimer(), so we should get rid of the calls to ClearSearchTimer() that happen before calls to StopSearch() (there are existing ones, but your patch adds one).

> nsAutoCompleteController::StopSearch()

>+  // stop all searches, even if mSearchStatus is not nsIAutoCompleteController::STATUS_SEARCHING
>+  // we might have an incremental search running (which as already returned results.

nit: "has", close parenthesis. I'm a bit worried about changing the behavior of StopSearch() to unconditionally stop all searches, because nsIAutocompleteSearch stopSearch() implementations might make assumptions about when they're called and could potentially cause bad side effects on the search. Given that the autocomplete APIs are poorly documented and not extensively tested beyond the relatively simple users in our tree, I'm not sure there's really much we can do to assuage my worry. I'm probably worrying too much :)

>@@ -1173,20 +1177,20 @@ nsAutoCompleteController::ProcessResult(

Seems like we invalidate the entire tree in ProcessResult(), ideally in the incremental search case we could invalidate only the rows that were added (which in most cases won't even be drawn because they're scrolled off the screen). I think the best way to do this would be to expose additional invalidate*() variants on nsIAutoCompletePopup, implement them in autocomplete.xml (which just forwards to the tree's boxobject), and then have the autocomplete controller invalidate only the parts that have changed. This might be tricky because the controller can't know how the two sets of results differ, because the history service nsIAutocompleteSearch impl just calls onSearchResult multiple times with a new autocomplete result each time, as far as I can tell, but something to look at in a followup bug, for sure (I think it would also benefit other autocomplete consumers, and probably fix bug 391889).

>   } else if (result == nsIAutoCompleteResult::RESULT_SUCCESS) {
>     // Increase the match count for all matches in this result
>     PRUint32 matchCount = 0;
>     aResult->GetMatchCount(&matchCount);
>-    mRowCount += matchCount;
>+    mRowCount = matchCount;
>     if (mTree)
>-      mTree->RowCountChanged(oldRowCount, matchCount);
>-
>+      mTree->RowCountChanged(oldRowCount,  mRowCount - oldRowCount);
>+    
>     // Try to autocomplete the default index for this search
>     CompleteDefaultIndex(aSearchIndex);
>   }

Looks to me like this change breaks support for multiple nsIAutoCompleteSearches (we have no in-tree consumers at the moment, that I know of, but I've been trying to make search suggest use it). mRowCount will be wrong once the second result is processed in that case, since it won't take into account the previously processed result. This is in some way incompatible with what you're doing - the controller expects each call to onSearchResult (and this ProcessResult) to receive a separate result object, but you're changing it so that each object replaces the previous one. You need to keep a list of already-returned searches (a list of indices into mSearches is probably sufficient, since ProcessResult is already passed an index) and adjust mRowCount appropriately based on whether this is a "new" result or simply an replacement for a previously processed result. r- because of this issue. I should really add some unit tests for this stuff...

>Index: toolkit/content/widgets/autocomplete.xml

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

I wonder whether we should just put a call to stopSearch in the popup's popuphiding handler instead of adding a handler to the input for contextmenu and mousedown.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 6

[(not reading, please use [e-mail] instead)]

1)

>+   * Stops a asynchronous search.
> nit: "an"

fixed, thanks

2)

> StopSearch() calls ClearSearchTimer(), so we should get rid of the calls to
> ClearSearchTimer() that happen before calls to StopSearch() (there are existing
> ones, but your patch adds one).

fixed, thanks.

3)

nit: "has", close parenthesis. 

fixed, thanks.

4)

> I'm a bit worried about changing the behavior of
> StopSearch() to unconditionally stop all searches

stopSearch() did this before my patch, my patch just makes stopSearch() part of the nsIAutoCompleteController interface so that I could get to it from autocomplete.xml

Or did I misunderstand you?

I did fix some comments, such as:
 
-* Stop an asynchronous search that is in progress
+* Stop all searches that are in progress

and

-// Stop current search in case it's async.
+// Stop all searches in case they are async.

5)

> Seems like we invalidate the entire tree in ProcessResult()

I'll log a follow up bug on this, but I think there is another bug that this would also address.

(see https://bugzilla.mozilla.org/show_bug.cgi?id=389491#c32, which I'll also log as a spin off)

6)

>   } else if (result == nsIAutoCompleteResult::RESULT_SUCCESS) {
>     // Increase the match count for all matches in this result
>     PRUint32 matchCount = 0;
>     aResult->GetMatchCount(&matchCount);
>-    mRowCount += matchCount;
>+    mRowCount = matchCount;
>     if (mTree)
>-      mTree->RowCountChanged(oldRowCount, matchCount);
>-
>+      mTree->RowCountChanged(oldRowCount,  mRowCount - oldRowCount);
>+    
>     // Try to autocomplete the default index for this search
>     CompleteDefaultIndex(aSearchIndex);
>   }

> Looks to me like this change breaks support for multiple
> nsIAutoCompleteSearches (we have no in-tree consumers at the moment, that I
> know of, but I've been trying to make search suggest use it). mRowCount will be
> wrong once the second result is processed in that case, since it won't take
> into account the previously processed result. This is in some way incompatible
> with what you're doing - the controller expects each call to onSearchResult
> (and this ProcessResult) to receive a separate result object, but you're
> changing it so that each object replaces the previous one. 

> You need to keep a
> list of already-returned searches (a list of indices into mSearches is probably
> sufficient, since ProcessResult is already passed an index) and adjust
> mRowCount appropriately based on whether this is a "new" result or simply an
> replacement for a previously processed result. r- because of this issue. 

I see your point, but don't I have to pass a complete result set every time?  Otherwise, what will nsNavHistory::StartSearch() pass back as the previous result?

7)

> I should really add some unit tests for this stuff...

I'll log a spin off bug on that.

8)

> I wonder whether we should just put a call to stopSearch in the popup's
> popuphiding handler instead of adding a handler to the input for contextmenu
> and mousedown.

won't the popuphiding handler get called when we want the search to continue?

a) user types google
b) popup show
c) user adds ffff (to get googlefff), so popup is hid while that search happens.

Or am I minunderstanding what you are suggestng?

Comment 7

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 8

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

(In reply to comment #6)
> > I'm a bit worried about changing the behavior of
> > StopSearch() to unconditionally stop all searches
> 
> stopSearch() did this before my patch, my patch just makes stopSearch() part of
> the nsIAutoCompleteController interface so that I could get to it from
> autocomplete.xml

You exposed it, but you also modified it, removing the check for STATUS_SEARCHING. That change is what my comment was about.

> > You need to keep a
> > list of already-returned searches (a list of indices into mSearches is probably
> > sufficient, since ProcessResult is already passed an index) and adjust
> > mRowCount appropriately based on whether this is a "new" result or simply an
> > replacement for a previously processed result. r- because of this issue. 
> 
> I see your point, but don't I have to pass a complete result set every time? 
> Otherwise, what will nsNavHistory::StartSearch() pass back as the previous
> result?

I'm not sure what you mean. Looking at the patch for bug 389491, you're calling onSearchResult multiple times (from PerformAutoComplete()) with a different result each time, even though your nsIAutocompleteSearch has only received one StartSearch() call, because you want to return results incrementally. This confuses the controller which only expects one call to nsIAutoCompleteListener::onSearchResult for each  nsIAutoCompleteSearch::StartSearch() call. Given that the controller will now receive multiple "onSearchResults" for a given "StartSearch()", you need to fix it's rowcount logic to make up for it. You can't assume that each new result processed by ProcessResult replaces the previously processed results. That's all my comment was about, the controller will still pass the previous results for a given search to subsequent calls to it's StartSearch().

> > I wonder whether we should just put a call to stopSearch in the popup's
> > popuphiding handler instead of adding a handler to the input for contextmenu
> > and mousedown.
> 
> won't the popuphiding handler get called when we want the search to continue?

I'm not sure. I was just trying to put the call to stopSearch some place common instead of having to put it in handlers for all the events we think should stop the search. Maybe that's not feasible :)

Comment 9

[(not reading, please use [e-mail] instead)]

gavin, thanks for the clarification.  I understand your comment about nsAutoCompleteController::ProcessResult() now.

looking into it, I noticed another problem with my change that I need to address.

we use mSearchesOngoing to figure out when to call PostSearchCleanup(), but I'm calling ProcessResult() multiple times, so this won't work like it used to work.

Comment 10

[(not reading, please use [e-mail] instead)]

chatted with Gavin, and to address the mSearchesOngoing, I'm going to add two status codes (RESULT_NOMATCH_ONGOING and RESULT_SUCCESS_ONGOING)

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 12

[(not reading, please use [e-mail] instead)]

1)

> You exposed it, but you also modified it, removing the check for
> STATUS_SEARCHING. That change is what my comment was about.

to address this comment, I made this change:

+  /* 
+   * Stop all asynchronous searches (no matter what the searchStatus)
+   */
+  void stopSearch();

2)

> You can't assume that each new result
> processed by ProcessResult replaces the previously processed results.

to address this comment, here's what I did:

When nsAutoCompleteController::ProcessResult() is called, we used to always append it to the results.  now, I first check to see if it is there already.  if so, we replace instead of append.  and, when adjusting row count, I use that knowledge (if the result is cached) to indicate we are replacing it, so we should adjust row count accordingly.

3)  RESULT_NOMATCH_ONGOING and RESULT_SUCCESS_ONGOING

Per our discussion, I've added these searchResult codes, and used them.

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 277178 [details] [diff] [review]
updated patch

clearing review request, need to fix a problem with this patch.

Comment 14

[(not reading, please use [e-mail] instead)]

the problem with the current patch is that I've broken the handling of enter.

with the current patch, our search status is STATUS_SEARCHING until the ongoing search fully completes.

but because our status is STATUS_SEARCHING, there is existing code that waits until the search completes to handle enter, and for a large history, we need to stop the search.

working on a fix.

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: updated patch (fix idl comment)

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: updated patch to address "enter sometimes isn't handled" issue

Comment 18

[(not reading, please use [e-mail] instead)]

Attachment: updated patch (includes missing idl change)

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: patch to pass gavin's unit test

Comment 20

[(not reading, please use [e-mail] instead)]

Attachment: gavin's unit test

Comment 21

[(not reading, please use [e-mail] instead)]

fixed.

Checking in toolkit/components/autocomplete/public/nsIAutoCompleteController.idl
;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteControlle
r.idl,v  <--  nsIAutoCompleteController.idl
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteResult.id
l,v  <--  nsIAutoCompleteResult.idl
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteSearch.idl;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteSearch.id
l,v  <--  nsIAutoCompleteSearch.idl
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cp
p,v  <--  nsAutoCompleteController.cpp
new revision: 1.62; previous revision: 1.61
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.h;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.h,
v  <--  nsAutoCompleteController.h
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.81; previous revision: 1.80
done

Comment 22

[(not reading, please use [e-mail] instead)]

landed gavin's unit test:

Checking in test_autocomplete_multiple.js;
/cvsroot/mozilla/toolkit/components/autocomplete/tests/unit/test_autocomplete_mu
ltiple.js,v  <--  test_autocomplete_multiple.js
initial revision: 1.1
done

Comment 23

[Weston Carloss]

Bonsai had the wrong bug number so I accidentally left this in 389503, but this is the correct bug number.



This appears to be causing a crash for me.  Using 200708211545 build from here
http://hourly-archive.localgho.st/, I have no crash.  Using the 200708211656
build from the same location, I get a crash when the suggestions start
appearing from the search bar.  The only checkin during this interval is: 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1187736300&maxdate=1187740559

I get the crash in normal mode, and in safe mode.  However, I do not crash with
a new profile.  

Does anyone want me to upload part of my profile so that we can see what might
be causing this?

Comment 24

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

(In reply to comment #23)
> This appears to be causing a crash for me.

Weston, could you file a new bug on the crash? It's much easier to deal with new issues in separate bugs. Please CC Seth and I.

Comment 25

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

I filed bug 393140 to fix the commit message.

Comment 26

[(not reading, please use [e-mail] instead)]

weston, can you try again with tomorrow's nightly?

If you still crash, would you be willing to send me your places.sqlite file?

Comment 27

[(not reading, please use [e-mail] instead)]

weston, you might have been hitting bug #393191.  

is browser.formfill.enable false in your profile?

can you try again with a build with that fix for that bug?

Comment 28

[Weston Carloss]

(In reply to comment #27)
> weston, you might have been hitting bug #393191.  
> 
> is browser.formfill.enable false in your profile?
> 
> can you try again with a build with that fix for that bug?
> 

Yes, browser.formfill.enable is set to false.

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082216 Minefield/3.0a8pre ID:2007082216, the crashes have gone away.  It seems like 393191 was the problem.

Thanks, Seth.

Comment 29

[(not reading, please use [e-mail] instead)]

>> Seems like we invalidate the entire tree in ProcessResult()
> I'll log a follow up bug on this, but I think there is another bug that this
> would also address.

spun that issue off to bug #393902