Closed Bug 926033 Opened 8 years ago Closed 7 years ago

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


(Toolkit :: Find Toolbar, defect)

27 Branch
Not set



Tracking Status
firefox27 --- verified
firefox28 --- verified


(Reporter: quicksaver, Assigned: evilpie)




(2 files, 2 obsolete files)

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 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))
> 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.
Haven't looked at this yet, but a solution might be to set searchString in highlight.
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?).
It does seem like _searchString is used in RemoteFinder.jsm though, in replacement of nsITypeAheadFind's .searchString (since it doesn't use it).
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.
Component: Untriaged → Find Toolbar
Product: Firefox → Toolkit
Assignee: nobody → evilpies
Ever confirmed: true
Attached patch untested WIP (obsolete) — Splinter Review
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.
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.
I pushed a new patch to try, we will see how it goes.
Attached patch v1Splinter Review
This is looking good: This is the last filed regression!
Attachment #826286 - Attachment is obsolete: true
Attachment #828166 - Flags: review?(mdeboer)
Attachment #828166 - Flags: feedback?(quicksaver)
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 on attachment 828166 [details] [diff] [review]

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

Comment 10.
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.
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.
You're very welcome, I'm glad I could help.
Attachment #828166 - Flags: feedback?(quicksaver) → feedback+
Attached patch Patch 2: regression test. (obsolete) — Splinter Review
Attachment #829233 - Flags: review?(evilpies)
Attachment #828166 - Flags: review?(mdeboer)
Attachment #828166 - Flags: review+
Attachment #828166 - Flags: feedback+
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 }
Attachment #829233 - Flags: review?(evilpies) → review+
Carrying over r=evilpie
Attachment #829233 - Attachment is obsolete: true
Attachment #829368 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
I confirm I can no longer reproduce this in the latest Nightly.

I'd suggest uplifting the patch to Aurora as well.
Comment on attachment 828166 [details] [diff] [review]

[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
Attachment #828166 - Flags: approval-mozilla-aurora?
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.
Attachment #829368 - Flags: approval-mozilla-aurora?
Attachment #828166 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!!
Attachment #829368 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Checkin-needed for m-a. Thank you!
Keywords: checkin-needed
Luis, can you please confirm this is resolved for you now in Firefox 27 and 28?
Flags: needinfo?(quicksaver)
Yes, this works perfectly for me.
Flags: needinfo?(quicksaver)
(In reply to Luís Miguel [:Quicksaver] from comment #27)
> Yes, this works perfectly for me.

You need to log in before you can comment on or make changes to this bug.