1305194 - Find highlights too quickly while I'm typing

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Justin Dolske [:Dolske]]

The performance of find-in-page is too fast! ;)

I've noticed a few times that while typing a search term, it's highlighting stuff in the page before I've finished typing. E.G. I just noticed it highlighting the letter "i" (which was all over the page!), while I was typing a term beginning with that letter.

At best this is a little distracting, at worst it's slowing things down searching for intermediate strings that are just going to be discarded for a future search. (Although the only slowdown I've seen is that it the previously highlighted short string may linger for a fraction of a second. So again more distraction than "slowness").

The usual fix for this kind of thing is to simply add a timeout, so that input is ignored a few milliseconds until the user hasn't typed more. (And perhaps with a timeout that's a bit longer for the first few characters?) Do we do anything like this already?

Comment 1

[Mike de Boer [:mikedeboer]]

Yes we do this already - can you play around with the `findbar.iteratorTimeout` pref a bit? You do need to restart your browser for it to take effect.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: WIP

Ah, cool, so maybe something simple like this?

Aside: I'm not sure the (existing) clearTimeout() is quite right... Seems like this will cause the pending promise (for the setTimeout) to get stuck and fail to ever complete. (Is that a leak?) This should probably be storing the actual promise, and rejecting it instead of clearing the timeout?

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8795534 [details] [diff] [review]
WIP

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

::: toolkit/modules/FinderIterator.jsm
@@ +426,5 @@
> +      let searchTerm = this._currentParams.word;
> +      if (searchTerm.length == 1)
> +        timeout *= 4;
> +      else if (searchTerm.length == 2)
> +        timeout *= 2;

I hadn't thought about this type of granularity, but I like it!!
r=me for this part.

@@ +429,5 @@
> +      else if (searchTerm.length == 2)
> +        timeout *= 2;
> +dump("Searching for <" + searchTerm + "> with timeout " + timeout + "\n");
> +
> +      yield new Promise(resolve => this._timer = setTimeout(resolve, timeout));

You're right, I was trying to be smart here. However, this is causing a very specific type of leak: JS call-stack explosion. This is not the type of leak we're used to (doesn't directly impact memory or CPU usage), but might be a cause for odd bugs after months of intensive findbar usage.
The simple fix would be:

```js
if (this._timer)
  clearTimeout(this._timer);
if (this._runningFindResolver)
  this._runningFindResolver();

// Yer-code.

yield new Promise(resolve => {
  this._runningFindResolver = resolve;
  this._timer = setTimeout(resolve, timeout)
});
this._timer = this._runningFindResolver = null;
```

And the if-statement below will do the rest of the work for us, because the spawnId will be different for the previous find.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

Single and double character find operations cause a big hit on nsFind, because
it usually yields many more occurrences than other queries. But most importantly,
it needs to keep iterating each text node much longer than other queries, because
there is much less escapes/ loop exits happening.
This also fixes the FinderIterator infinite-depth call-stack due to unresolved
promises that remain active during the lifetime of the FinderIterator. Unwinding
the call-stack properly should save us bytes from the heap.

Review commit: https://reviewboard.mozilla.org/r/83564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83564/

Comment 5

[Mike de Boer [:mikedeboer]]

Justin, I hope you don't mind me stealing.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

https://reviewboard.mozilla.org/r/83564/#review82872

::: toolkit/content/tests/chrome/findbar_window.xul:562
(Diff revision 1)
> +        if (test.text.length == 1)
> +          timeout = ((timeout - 20) * 4) + 20;
> +        else if (test.text.length == 2)
> +          timeout = ((timeout - 20) * 2) + 20;

What is the point of subtracting 20 from the timeout before multiplying by 4 and then adding 20 back?

This will now be 60ms less than the iterator_timeout, whereas before this patch the timeout was equal to iterator_timeout.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

https://reviewboard.mozilla.org/r/83564/#review82872

> What is the point of subtracting 20 from the timeout before multiplying by 4 and then adding 20 back?
> 
> This will now be 60ms less than the iterator_timeout, whereas before this patch the timeout was equal to iterator_timeout.

Because 20ms is added at the top of the file too. I can also remove that additional 20ms, because I don't think we need it...

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83564/diff/1-2/

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

https://reviewboard.mozilla.org/r/83564/#review83470

Okay, I see now. So with this change the iterator timeout that is used within the test matches that of the pref? I think we should still add 20ms to the iterator timeout, just do it after we have gone through the if-elseif lines.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83564/diff/2-3/

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83564/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7882c91ca20
wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operations there are on a page. r=jaws

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for leaking docshells, e.g. in browser_Finder_hidden_textarea.js and browser_findbar.js. Had backed out the wrong changeset (for bug 1279652) before, but relanded that.

https://hg.mozilla.org/integration/autoland/rev/380b5366b20f4ad1cb21ee8252486017e0f66ad2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a7882c91ca201fa4ed52f486b406c096032cc4f3
Logs with leaks:
https://treeherder.mozilla.org/logviewer.html#?job_id=4866208&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=4866156&repo=autoland

Comment 14

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90a605da4049

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83564/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d428a03f0f4b
wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operations there are on a page. r=jaws

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d428a03f0f4b

Comment 18

[Simona Badau, Desktop QA]

Attachment: bug1305194.mp4

I was trying to verify this issue and indeed the delay that was desired can be noticed and it kind of reduces the flickering.

But, on the following searches that same delay makes the previous search be displayed again. Please see the screen cast. For e.g, if I search for "i", I clear the Find bar and after that I search for "a", "i" is highlighted for a short period of time just before "a" being highlighted. 

Is this the result you expected with the landing of this patch?

Comment 19

[Mike de Boer [:mikedeboer]]

Yes. Thanks for verifying! And apologies for the super late reply...

Comment 20

[Simona Badau, Desktop QA]

Based on Comment 19, setting the status-firefox54: to verified. 
Since the Find Toolbar feature is enabled by default only on the latest Nightly 54.0a1, setting the status-firefox52 and status-firefox53 to disabled.