962910 - Findbar result listeners sometimes missing

Status: RESOLVED FIXED

(SeaMonkey :: Find In Page, defect)

Description

[kevink9876543]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0 SeaMonkey/2.26a1 (Beta/Release)
Build ID: 20140122003001

Steps to reproduce:

SeaMonkey 2.26a1 nightly, new profile
1) preferences -> browser -> tabbed browsing: uncheck "Hide the tab bar when only one tab is open"
2) Close the open tab
3) Navigate to data:text/html,z<div>z</div>
4) search for "z" (without quotes) until end of page is passed
5) search for some text not in the page, e.g. "q"


Actual results:

At step 4, findbar did not show the "Reached top of page, continuing from bottom" or "Reached end of page, continuing from top" messages when search encounters either end of the page.
At step 5, the findbar does nothing.


Expected results:

When end of page is reached in search, the findbar should display its message saying so.
When search terms are not found, findbar text field should turn red and string "Phrase not found" should be displayed.

Comment 1

[Mike de Boer [:mikedeboer]]

Do you see any error messages in the Javascript Console? Or when you run SeaMonkey from the command line?

Comment 2

[kevink9876543]

Nothing related either place.

Comment 3

[kevink9876543]

Note: Also reproducible with SeaMonkey 2.24b1
No related error messages there either.

Comment 4

[kevink9876543]

Looks like this isn't a Toolkit bug after all.  Tweaked my fork of Adblock Plus a bit, and got the Filter Preferences findbar instance always reacting correctly without any hacks.  Moving to SeaMonkey.

Comment 5

[therube]

> When end of page is reached in search, the findbar should display its message saying so.

Confirmed.

> When search terms are not found, findbar text field should turn red and string
> "Phrase not found" should be displayed.

Oddly using your STR, it was turning red & it "Phrase not found" was displayed for me.
And I may or may not have gotten any sound along with the failure.
(Sometimes I do, sometimes I do not.)

This second part (two bugs, or related bugs, in one) may be a DUP of Bug 973573 - Red background for unmatched search strings not reliable.

Like Bug 973573, SeaMonkey 2.23 also correctly displays "Reached end of page, continued from top".

Comment 6

[kevink9876543]

It seems that when the findbar isn't reacting, its result listener is gone.  On SeaMonkey 2.25 beta 2, ran the following code in a modified version of the Workspace add-on for Firefox (set to chrome environment):

var l = window.gBrowser.finder._listeners;
for(let i in l) {
  Cu.reportError(i + ':' + l);
 }
Cu.reportError("done enumerating properties");

Result: Where the findbar reacts correctly, it prints:
Error: 0:[object XULElement]
Error: done enumerating properties

Where the findbar does not react, it prints only:
Error: done enumerating properties

Comment 7

[Jiri TRAVNICEK [:JITR]]

Some observations regarding STR:

    * The problem can be reproduced on all tabs except the first tab of each window. Tests show the first tab is determined when the window is opened: (1) If the first tab is moved, the working tab stays the same (this time not a first one anymore). (2) If the first tab is closed, none of the tabs in the window work. (3) If this tab is restored (using Ctrl+Shift+T), findbar works there again. (4) When the window is closed and then restored (using Ctrl+Shift+Y), the tab which is first (or the only tab if only 1 is open) is the tab where findbar works correctly.

    * The problem seems to be reproducible with arbitrary page.

    * The problem is reproducible with a fresh profile, no preferences needed to be modified.

    * Verified on latest trunk nigthlies I could find, both in Windows (2.29a1 build 20140609003001) as well as Linux (2.27a1 build 20140214003001).

    * I can reproduce all of the symptoms including results of the test suggested in comment #6 -- `window.gBrowser.finder._listeners' is array containing 1 `XULElement' whenever the findbar works and an empty array whenever it does not work. Comparison of object references (in Execute JS modified for SeaMonkey) shows that the `gBrowser' is same for all tabs while the `gBrowser.finder' is changing per tab.

Bug 973573 definitely seems like a part of this one to me.

Comment 8

[Jiri TRAVNICEK [:JITR]]

Testing with older (Linux x86) releases confirms this is a regression introduced in 2.24. The last working release was 2.23.

Testing with trunk nightlies (again Linux x86) shows this has been last working in the 2.23a1 build 20130913003001. The next 2 nightlies (20130914003001 and 20130915003001) are completely busted in this regard (findbar is not even showing, but some symptoms of this problem can already be seen -- the "not found" notification is showing in the status bar and also works in the first tab only). The following nighly 2.23a1 build 20130916003001 has this fixed (most probably thanks to landing of bug 916477) and behavior already matches that observable till nowadays.

There seem to be no tinderbox builds (especially that old), so the regression window is unfortunately rather large:

    * working:
          http://hg.mozilla.org/mozilla-central/rev/b9029b1de410
          http://hg.mozilla.org/comm-central/rev/a3d827851227

    * completely busted:
          http://hg.mozilla.org/mozilla-central/rev/53d5e43e23cc
          http://hg.mozilla.org/comm-central/rev/0b7a62366012

    * partially fixed:
          http://hg.mozilla.org/mozilla-central/rev/dc909122bcf5
          http://hg.mozilla.org/comm-central/rev/55c9e25ba55a

As a rather uneducated guess, I'd suspect this to be some remaining bustage caused by bug 666816, which the bug 916477 missed.

Comment 9

[[e-mail]]

That's mostly correct, but it's also partly to do with Firefox's switch to one fast find per window, while we were still using one fast find per tab. (We might have to switch too; I'm not sure whether we can adequately fake out one finder per tab.)

Comment 10

[[e-mail]]

There would be a very nice way of fixing this, if only bug 324188 had used _fastFind instead of fastFind, thus avoiding trying to look for fastFind on the tabbrowser. Sigh...

Comment 11

[[e-mail]]

Attachment: Proposed patch

In order to be able to switch listeners between finders, I had to create a dummy finder object whose job it is to track the listeners and pass everything else through to the current finder. I would have liked to use the tabbrowser itself as the dummy object but unfortunately it already has a fastFind property and we can't replace it as it breaks browser.xml code we don't even need.

Comment 12

[Philip Chee]

Comment on attachment 8581358 [details] [diff] [review]
Proposed patch

Nits:
What happened to getActiveSelectionText() ?

> +              this.finder.mListeners.forEach(l => this.mCurrentBrowser.finder.removeResultListener(l));
> +            this.finder.mListeners.forEach(l => this.mCurrentBrowser.finder.addResultListener(l));

> +          mListeners: [],

Can we use Set() here? Set has a forEach() plus you can add() and delete() without having to check for the existence first.

> +          fastFind: function(aSearchString, aLinksOnly, aDrawOutline) {
> +            this.finder.fastFind(aSearchString, aLinksOnly, aDrawOutline);
> +          },

This code looks fragile. Can we use ES6 Proxies to forward stuff that we don't care about?

Comment 13

[[e-mail]]

(In reply to Philip Chee from comment #12)
> What happened to getActiveSelectionText() ?
Only used internally or by RemoteFinder.jsm

> > +          mListeners: [],
> Can we use Set() here? Set has a forEach() plus you can add() and delete()
> without having to check for the existence first.
Good point, I'll look into that.

> > +          fastFind: function(aSearchString, aLinksOnly, aDrawOutline) {
> > +            this.finder.fastFind(aSearchString, aLinksOnly, aDrawOutline);
> > +          },
> Can we use ES6 Proxies to forward stuff that we don't care about?
I'll have to read up on them.

Comment 14

[[e-mail]]

Attachment: Updated for review comments

I changed the [] to a Set but the Proxy change would have been complicated.

Comment 15

[[e-mail]]

Pushed comm-central changeset b341352f42b9.

Comment 16

[[e-mail]]

Comment on attachment 8584038 [details] [diff] [review]
Updated for review comments

[Approval Request Comment]
Regression caused by (bug #): 666816
User impact if declined: Findbar doesn't update correctly
String changes made by this patch: None

Comment 17

[[e-mail]]

http://hg.mozilla.org/releases/comm-aurora/rev/aa75723b3cff
http://hg.mozilla.org/releases/comm-beta/rev/0bf9ce4891ac

I messed up the uplifts for bug 1146168 though.

Comment 18

[Philip Chee]

Comment on attachment 8584038 [details] [diff] [review]
Updated for review comments

In case we need 2.33.2
[Approval Request Comment]
Regression caused by (bug #): Bug 666816
User impact if declined: Findbar doesn't update correctly
String changes made by this patch: None

Comment 19

[Philip Chee]

Comment on attachment 8584038 [details] [diff] [review]
Updated for review comments

> In case we need 2.33.2
I guess not