926033 - Incorrect "Phrase not found" on new tab when find carried over from previous tab

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

1) Open browser, open any tab (I tested with about:home for simplicity, but this should happen for any tabs you want).
2) Open find bar, search for a phrase that doesn't exist, for example "newd"; find bar status will be "Phrase not found" as expected.
3) Open new tab (I just middle click the home button for another about:home tab).
4) Open find bar in the new tab; search query should be "newd" and find status should be reset as expected.
5) Toggle "highlight all" on; find status will be "Phrase not found" as expected.
6) Erase find query and type "i" (no quotes); find status will still be "Phrase not found" even though "i" exists in the page.

There will be no "current match" and no highlights will be placed either. You need to enter more characters to trigger a correct state.

I believe it's a regression from bug 666816: method gFindBar.onFindResult() is setting _findFailedString to browser.finder.searchString, but this value hasn't been set yet in the new tab because gFindBar.open() doesn't actually trigger a search, and the highlights function uses nsIFind and not nsITypeAheadFind, which doesn't update the finder.searchString value.

At this point, when _find() is called after typing something else, val.startsWith(this._findFailedString) evaluates to true for some reason... From my tests in my example, val = "i" and _findFailedString = "", which I found strange. But because of this, the find operation doesn't actually run that first time, so it wrongly keeps the "Phrase not found" status.

I tried changing that check in _find() from
> if (this._findFailedString == null ||
>   !val.startsWith(this._findFailedString))
to
> if (!this._findFailedString ||
>   !val.startsWith(this._findFailedString))
so that the case when _findFailedString == "" is bypassed (evaluates to true both when == null and when == ""); this way the _find() function proceeds correctly in the example I described, but I'm not sure if there's any other specific case where _findFialedString == "" and _find() really shouldn't perform the search. If there is such a case, this solution would probably break that.

Comment 1

[Tom S [:evilpie]]

Haven't looked at this yet, but a solution might be to set searchString in highlight.

Comment 2

[Luís Miguel [:quicksaver] (not reading bugmail)]

Hmm, actually, it already does in finder.highlight(), but instead of setting finder.searchString (which actually doesn't have a setter method) it's setting finder._searchString. I hadn't noticed this before. However, _searchString isn't being set or checked anywhere else.

Also, I believe nsITypeAheadFind's searchString, of which finder.searchString's getter returns the value, is read-only, so it can't be set from highlight or anywhere else (is that why _searchString exists, to somehow replace this?).

Comment 3

[Luís Miguel [:quicksaver] (not reading bugmail)]

It does seem like _searchString is used in RemoteFinder.jsm though, in replacement of nsITypeAheadFind's .searchString (since it doesn't use it).

Comment 4

[Tom S [:evilpie]]

I actually planned on using _searchString in Finder.jsm, but that probably introduced some regressions or something and was overlooked in the final push to get all tests passing.

Comment 5

[Tom S [:evilpie]]

Attachment: untested WIP

Comment 6

[Tom S [:evilpie]]

This fixes the testcase from comment #0. Looking at this patch, there might be some other issue. We trigger the onFindResult callback in highlight even when we remove the highlighting. That seems wrong.

Comment 7

[Luís Miguel [:quicksaver] (not reading bugmail)]

Functionally speaking, that should be fine, although admittedly irrelevant. (From the onFindResult handler) The _updateStats*() calls should be carried over from previous fastFind/findAgain/highlight() calls, as should the gFindBar._findFailedString value. And the _setFindCloseTimeout() is useless as there's no way to add or remove the highlights from within the quick find bar (not without my add-on anyway :) ).

Adding an if(aHighlight) check in Finder.highlight() before the _notify() calls could work, in theory at least.

Comment 8

[Tom S [:evilpie]]

I pushed a new patch to try, we will see how it goes.

Comment 9

[Tom S [:evilpie]]

Attachment: v1

This is looking good: https://tbpl.mozilla.org/?tree=Try&rev=da0c90bc6256. This is the last filed regression!

Comment 10

[Luís Miguel [:quicksaver] (not reading bugmail)]

I applied the patch locally through an add-on package, as I honestly have no idea how to compile firefox or use the test tools or whatever it is you're supposed to do with these patches to test them (never spent much effort to learn, admittedly).

It seems to work great. I can no longer reproduce the issue and I don't see any side-effects with highlights. So, looks great! Is it bad juju to congratulate you guys on the Finder.jsm module before the last patch actually lands? :)

Just out of curiosity, in Finder.fastFind(), why are you getting the searchString value from _fastFind.searchString and not using aSearchString directly? Is it to be coherent with the code in Finder.findAgain(), or could there be some case where the two values would not be the same?

Comment 11

[Luís Miguel [:quicksaver] (not reading bugmail)]

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

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

Comment 10.

Comment 12

[Luís Miguel [:quicksaver] (not reading bugmail)]

Ok, I'm sorry, but for the life for me I can't figure out how to clear that feedback flag. I won't spam this thread any further with this.

Comment 13

[Tom S [:evilpie]]

Honestly I have no good reason to use _fastFind.searchString instead of aSearchString. Looking at the code of nsTypeAheadFind it shouldn't make a difference either. 

Thank you Luís, without your help we would have never found most of these bugs. You also spotted other issue in the code. Your were very very valuable for this effort.

Comment 14

[Luís Miguel [:quicksaver] (not reading bugmail)]

You're very welcome, I'm glad I could help.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: regression test.

Comment 16

[Tom S [:evilpie]]

Comment on attachment 829233 [details] [diff] [review]
Patch 2: regression test.

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

Wow. Amazing thank you for writing a test. :) :) I almost feel a little bit bad about complaining.

::: toolkit/content/tests/browser/browser_findbar.js
@@ +11,5 @@
> +  }
> +});
> +
> +let alpha = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
> +function getRandomString(len) {

This seems like overkill, just use a string like "--- THIS SHOULD NEVER MATCH ---"

@@ +46,5 @@
> +    finish();
> +  });
> +}
> +
> +let gTabs = [];

I would prefer if you moved this before the first use.

@@ +72,5 @@
> +
> +  let findbar = gBrowser.getFindBar();
> +  findbar.startFind(findbar.FIND_NORMAL);
> +  let highlightElement = findbar.getElement("highlight");
> +  if ((highlightElement.checked && !highlightOn) ||

Isn't this just highlightElement.checked != highlightOn ?

@@ +79,5 @@
> +  }
> +  executeSoon(() => {
> +    findbar._findField.value = searchText;
> +    findbar._find();
> +    executeSoon(() => deferred.resolve());

This is not really going to work if we don't get the result in the next tick. (This can't happen right now, because finding isn't really asynchronous yet). You should do something like the other tests:
//
findbar.browser.finder.addResultListener({ onResult ... deferred.resolve }

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: regression test.

Carrying over r=evilpie

Comment 18

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/ea1ab9c401cd
https://hg.mozilla.org/integration/fx-team/rev/1f2b79e09b65

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea1ab9c401cd
https://hg.mozilla.org/mozilla-central/rev/1f2b79e09b65

Comment 20

[Luís Miguel [:quicksaver] (not reading bugmail)]

I confirm I can no longer reproduce this in the latest Nightly.

I'd suggest uplifting the patch to Aurora as well.

Comment 21

[Mike de Boer [:mikedeboer]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 666816
User impact if declined: State mismatch of the findbar between tabs, which leads to confusing/ wrong behavior.
Testing completed (on m-c, etc.): baked on m-c for 3 days
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 829368 [details] [diff] [review]
Patch 2.1: regression test.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 666816
User impact if declined: regression test that goes with attachment 828166 [details] [diff] [review].
Testing completed (on m-c, etc.): baked on m-c for 3 days
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a

Not sure what the policy is for uplifting tests too, so I included it for approval just in case.

Comment 23

[bhavana bajaj [:bajaj]]

Comment on attachment 829368 [details] [diff] [review]
Patch 2.1: regression test.

Approving the low risk patch to fix a recent fallout.Thanks for the tests!!

Comment 24

[Mike de Boer [:mikedeboer]]

Checkin-needed for m-a. Thank you!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb81b44bf0d8
https://hg.mozilla.org/releases/mozilla-aurora/rev/51d2d4266321

Comment 26

[u279076]

Luis, can you please confirm this is resolved for you now in Firefox 27 and 28?

Comment 27

[Luís Miguel [:quicksaver] (not reading bugmail)]

Yes, this works perfectly for me.

Comment 28

[u279076]

(In reply to Luís Miguel [:Quicksaver] from comment #27)
> Yes, this works perfectly for me.

Thanks!