360572 - deleting a previous search entry does not work if a search suggestion is shown

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[bugs-mozilla]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0

The deletion of previous search entries with shift+del doesn't work if search suggest shows some options. Using shift+del deletes the current entry, but the next time the entry is back again.

Reproducible: Always

Steps to Reproduce:
1. Clear your search history.
2. Search for "test" using the search bar.
3. Enter "test" again in the search bar, select the previous entry "test" from the drop-down and hit shift+del in order to delete it from the search history.
4. The entry "test" is apparently deleted.
5. Enter "test" again in the search bar, notice that "test" is still shown in the drop-down box.

Actual Results:  
The search entry "test" ist not deleted.

Expected Results:  
The search entry should be deleted after the first time I hit shift+delete.

I don't know whether this bug should be in the component "Form Manager" instead. 

Using a search engine with no auto-suggest feature, deleting old search entries works without problems.

Workaround: Klick in the search bar and don't enter any text, use the cursor (down) to see the previous searches and delete them with shift+delete.

Comment 1

[JCorliss]

Using "delete" which is supposed to be acceptable for fx 2.0 achieves same results.

Comment 2

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

I've confirmed this issue locally. The problem is that search suggest "wraps" form history's nsIAutocompleteResult when suggestions are returned, but that wrapper's removeEntryAt() function (which is called when you press delete in the search bar) doesn't forward the call to the form history result for removal from the DB (it only deletes the entry from its array). We could fix this by calling removeEntryAt on the wrapped form history result, except that we would need to hold a reference to that result indefinitely, which means that we'd need to make sure not to regress bug 346933.

Comment 3

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

(In reply to comment #2)
> except that we would need to hold a reference to that result indefinitely,
> which means that we'd need to make sure not to regress bug 346933.

Ah, except that we don't use the mork form history service anymore (on the trunk), so we can fix this easily there. The branch still uses mork for form history, though, so we'll still need to be careful if we want to fix this there.

Comment 4

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

Attachment: branch patch

This can land on the trunk for baking, and fixes the bug there too, but I'd like to do some extra cleanup on the trunk.

This avoids the problem described in bug 346933 in two ways:
1) The reference to the form history result is made from the suggest nsIAutoCompleteResult wrapper, and not from the search suggest component (this seems much less problematic in general, since the two objects have similar lifetimes)
2) The autocomplete controller (nsAutoCompleteController.cpp) manages it's reference to the suggest nsIAutoCompleteResult carefully - references to it are removed from HandleEscape() and Rollup(), which means that the reference to the form history result (via the suggest result) can disappear as soon as it isn't needed, and not linger until shutdown like before.

Comment 5

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

Comment on attachment 246675 [details] [diff] [review]
branch patch

Hmm, this causes a weird bug I need to look into.

Comment 6

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

Attachment: branch patch

Ok, so I found the weird bug. The previous patch removed the nulling out of this._formHistoryResult in onReadyStateChange, and the suggest timeout timer was still running, so _reset() wouldn't null it out. This meant that when the timer expired, we were sending a second onSearchResults notification with a non-null result to our nsIAutoCompleteObserver. This wasn't an issue before, because this._formHistoryResult would be nulled out as soon as we stopped using it, so the second notification would be ignored (null result). I fixed the core issue by making onResultsReady null out this._listener, which has the effect of making the timer callback (notify) a no-op (once we've sent a result to our listener, we don't ever need to send one again). I also changed the history-only branch of our onSearchResult to call _reset instead of just nulling out this._formHistoryResult, since there's also no need to do anything after that's been called.

Comment 7

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

What I really want to request is wanted-1.8.0.x, but apparently I can't do that.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 246679 [details] [diff] [review]
branch patch

looks good

branch eval: relatively low-risk, should land early in 1.8.1.2 cycle

Comment 9

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

mozilla/browser/components/search/nsSearchSuggestions.js 	1.13

Comment 10

[Daniel Veditz [:dveditz]]

moving blocking nom to wanted per comment 7 and 8

Comment 11

[Jay Patel [:jay]]

Comment on attachment 246679 [details] [diff] [review]
branch patch

Approved for 1.8 branch, a=jay for drivers.

Gavin:  Do we need this fix on the 1.8.0 branch also?

Comment 12

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

(In reply to comment #11)
> Gavin:  Do we need this fix on the 1.8.0 branch also?

No, search suggest is a Firefox 2 feature and this bug was in that code.

Comment 13

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

mozilla/browser/components/search/nsSearchSuggestions.js 	1.1.2.11

Comment 14

[bugs-mozilla]

I've tried with a current trunk-build - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070111 Minefield/3.0a2pre - and it works as expected now, so I guess this bug can be marked VERIFIED? Or should I test it with a 1.8-branch build?

Comment 15

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

(In reply to comment #14)
> I've tried with a current trunk-build - Mozilla/5.0 (Windows; U; Windows NT
> 5.1; en-US; rv:1.9a2pre) Gecko/20070111 Minefield/3.0a2pre - and it works as
> expected now, so I guess this bug can be marked VERIFIED?

Yep. Thanks for verifying!

> Or should I test it with a 1.8-branch build?

That certainly wouldn't hurt. If you do so, we can change the fixed1.8.1.2 keyword to verified1.8.1.2.

Comment 16

[bugs-mozilla]

According to comment 12 Firefox 1.5 builds are not effected so I downloaded Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070111 BonEcho/2.0.0.2pre from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.8/ (I assume this is the 1.8.x branch) and it works here too.