Closed Bug 1198465 Opened 4 years ago Closed 4 years ago

non-deterministic "Find in this page" search with new characters dropped or appended to previous search due to delayed selection of search string

Categories

(Toolkit :: Find Toolbar, defect)

38 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: karlt, Assigned: quicksaver)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

The following steps make this bug easy to reproduce, but the behavior is
inconsistent during normal usage, most noticeable when the browser is busy.

1. Ensure E10S is not enabled.

2. Start load of https://html.spec.whatwg.org/

3. Perform the following steps while the page is loading:

   3a. Focus the content area, with a click.

   3b. Initiate "Find in this page" either by clicking Open Menu -> Find or
       Ctrl+F and immediately type one or more characters.

   3c. If the find bar is focused before completion of 3b, then

       3c1. If the page has completed loading, then repeat from 2.

       3c2. Otherwise repeat from 3a.

Expected results:
Find bar contains the characters typed in the latest step 3b, which characters
are not selected.

Actual results:

Find bar either contains characters both from a previous search and from the
latest step 3b, all selected, or only a trailing subset of the
characters typed, not selected.
bug 1198465 revert 230d46f387f2 for out-of-order of key events and select r?mikedeboer
Attachment #8655351 - Flags: review?(mdeboer)
A rebase of the backout was not quite trivial due to some other changes to the
code.

The changes in reverting findbar_window.xul highlight possible causes of the
bug: the synchronous focus of the input field (as
testNormalFindWithComposition was testing) and synchronous selection (as
testFindbarSelection was testing) were assumed for subsequent key events to
go to the right element and have the right effect.

A different solution is needed for bug 1055508.  If synchronous focus is not
practical with e10s, then an alternative solution is to block the input event
queue until the field is focused and selected, but this has not yet been
implemented AFAIK.  If synchronous selection fetch is not practical, then an
alternative solution is to consider the events that were received before the
selection reply, probably meaning that the selection is no longer inserted
when other input has already been received (in some ways similar to focus-stealing prevention).
That patch was put in Firefox 38, so it has been on release for three versions already. Personally I'd rather see it fixed than backed out.

I also tackled this issue in FindBar Tweak when I thought it was causing it. I think these are the points that *need* fixing so they're what I did there:
- ensure no prefill occurs after the user has input something [1];
- ensure subsequent keypresses while in content (before the findbar is focused) don't keep re-opening/re-focusing the findbar [2] (the original keypress come from the content process and they are sent through messages, so if the browser is busy enough and the user is fast enough, it could happen in theory at least);
- remove the synchronous focus line [3], this will need fixing of the tests though, which I don't believe can be easily avoided; I'm currently experimenting with this together with some ._startFindDeferred onResolvers (is this a word?) to see if they can better resolve the previous points without as much code.

[1] https://github.com/Quicksaver/FindBar-Tweak/commit/c7572f223bb5b01616ea26c9e3210c380fdf91c4
[2 lines 187-193] https://github.com/Quicksaver/FindBar-Tweak/commit/673101924fdc86108c9e48efe260858184e6172d
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#1020
Karl, thanks for the patch. It's an option to back this out, but I'd rather see Blake - or someone else - take a look at this issue like Luís is proposes in comment 5.
To clarify, the _didFind flag in [1] above is essentially the same as onCurrentSelection() no-op'ing if _startFindDeferred is null, with resolving and nulling _startFindDeferred in _find() as well. Already wrote this into FindBar Tweak [4] and didn't even need to remove the sync .focus() call in startFind() like I mentioned I was trying; everything seems ok so far.

(Important point, can't believe I forgot to mention this before...) The add-on also synchronously selects the find query every time the findbar opens [5], to avoid jumbled up queries when a previous value existed and the user typed before onCurrentSelection had a chance to select the contents of the input field, regardless of whether it prefills with any text selection or not, and with my patch in case it no-ops as well.

FYI I took a quick look at the referenced tests and I don't believe any of this should break them, although I could be wrong as TBH I didn't investigate them very extensively.

[4] https://github.com/Quicksaver/FindBar-Tweak/commit/d458201a46d8c9c5184bcd5a6dc5db4027e11b3c
[5] https://github.com/Quicksaver/FindBar-Tweak/blob/master/resource/modules/perTab.jsm#L6-L14
(In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> A different solution is needed for bug 1055508.  If synchronous focus is not

What is synchronous focus?

> practical with e10s, then an alternative solution is to block the input event
> queue until the field is focused and selected, but this has not yet been
> implemented AFAIK.  If synchronous selection fetch is not practical, then an

The parent process should basically never block on the child. We cheat in a few cases now but the eventual goal is to drive those cases to zero, so synchronous selection fetch isn't practical.

> alternative solution is to consider the events that were received before the
> selection reply, probably meaning that the selection is no longer inserted
> when other input has already been received (in some ways similar to
> focus-stealing prevention).

I believe my original patch was trying to avoid this problem (see bug 1055508, comment 11). Unless I'm missing something, it seems like this should be pretty easy to fix, I don't see why the entire approach needs to be scrapped.
Flags: needinfo?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #8)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> > A different solution is needed for bug 1055508.  If synchronous focus is not
> 
> What is synchronous focus?

FWIW I think it's the _findField.focus() in startFind() I also referred to in comment 5 and comment 7, as it's the only focus done sync left behind after bug 1055508; as in the findbar is focused "immediately" when commanded/opened without waiting for the prefill action by finder.getInitialSelection() from content.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #8)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> > A different solution is needed for bug 1055508.  If synchronous focus is not
> 
> What is synchronous focus?

By "synchronous focus" I'm referring to the property that if one event triggers a focus change, then that focus change will be in effect by the time the next event is processed, so that it is dispatched to the intended element.

I'm not actually clear on whether this property is still present or not.  I don't know whether focus problems are involved here or whether it is just the selection fetch.  I raised the possibility of a problem with focus because the tests that were testing for this seemed to have been changed so that they don't test for this.

With e10s enabled, key events are being lost.  Perhaps a focus problem is the cause of that, but this bug is about in-process find in page.

I can imagine the selection fetch needing to be async, but if the behavior of the find depends on the response to the selection fetch, then the find needs to wait for the reply before it acts.  Usually though the behavior does not depend on the reply, because the first key event will usually (depending on what behavior is bound to the key) delete the selection, in which cases it is not necessary to wait for the reply.  I assume we don't lose out too badly from waiting for content because we are searching in content anyway, which I assume requires waiting for content.

FWIW STR here don't require an active selection (but the parent process may not know whether an active selection is present).

> I believe my original patch was trying to avoid this problem (see bug
> 1055508, comment 11). Unless I'm missing something, it seems like this
> should be pretty easy to fix, I don't see why the entire approach needs to
> be scrapped.

Sounds good, thanks.  Perhaps I was jumping to conclusions based on the test changes apparently removing the tests for this.  I don't know enough about the code changes to know how it is designed to work now.
(In reply to Karl Tomlinson (ni?:karlt) from comment #10)
> With e10s enabled, key events are being lost.

FWIW I think this is what I meant with the second "ensure" point in my comment 5; my proposed changes also fix this.

> Usually though the behavior
> does not depend on the reply, because the first key event will usually
> (depending on what behavior is bound to the key) delete the selection, in
> which cases it is not necessary to wait for the reply.

I think this is always true. Any key pressed that doesn't actually change/delete the prefilled value from a text selection as you mentioned, shouldn't be relevant here, unless I'm missing some specific case, which probably should also be handled specifically.
If this is not going to be fixed by other means before the uplift, then can we take the back-out for now?

There doesn't seem to be any need to prolong this bug another six weeks.
(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> If this is not going to be fixed by other means before the uplift, then can
> we take the back-out for now?
> 
> There doesn't seem to be any need to prolong this bug another six weeks.

I don't understand, unless this is backed out from beta right now, which I strongly doubt will happen, does it really matter? By the time either a fix or a backout reaches release, it's entirely possible that so will have e10s (well it will have reached at least beta).

Backing out means potentially rolling out part of the find in page mechanism that apparently didn't work very well in e10s either. Bug for bug, I'd rather "keep" this one TBH. :)
(In reply to Luís Miguel [:Quicksaver] from comment #13)
> I don't understand, unless this is backed out from beta right now, which I
> strongly doubt will happen, does it really matter? By the time either a fix
> or a backout reaches release, it's entirely possible that so will have e10s
> (well it will have reached at least beta).

It does matter because release is not using e10s and the state of find in page with e10s enabled leads me to believe that e10s is not ready for beta.

> Backing out means potentially rolling out part of the find in page mechanism
> that apparently didn't work very well in e10s either. Bug for bug, I'd
> rather "keep" this one TBH. :)

Backing out only means removing that feature from e10s, which is not ready for release anyway.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #8)
> I believe my original patch was trying to avoid this problem (see bug
> 1055508, comment 11). Unless I'm missing something, it seems like this
> should be pretty easy to fix, I don't see why the entire approach needs to
> be scrapped.

Sounds like it... in the meantime, we'll have to deal with this issue at some point. Blake, are you willing to spend a few cycles on this or Luís, would you like to create a patch out of the ideas and code snippets you mentioned in this bug?

Thanks, both, for your time!
Flags: needinfo?(quicksaver)
Flags: needinfo?(mrbkap)
Attachment #8655351 - Flags: review?(mdeboer)
I'll attach a potential patch for this tomorrow.
Flags: needinfo?(mrbkap)
(Sorry for the late response, I've been installing the build tools on another computer and didn't want to say anything until I knew if it would work there, so that you wouldn't wait on me needlessly.)

I just finished installing and building everything and I think I've already fixed this bug locally (with tests even!). So if you're willing to wait a couple more hours for me to figure out how to make actual patches of the things I'll be happy to share; unless Blake is already on his way. :)
Flags: needinfo?(quicksaver)
Changes in patch:
- find query is selected as soon as it is focused by startFind(), to avoid showing a previous query jumbled with newly typed characters;
- when opened/commanded, findbar won't be prefilled with a current text selection if a find operation runs inbetween, as happens when the query is changed by typing in the findbar.

The tests should cover both these cases. I couldn't figure out how to split the tests into their own separate patch, sorry about that. I guess unless/until someone is willing to mentor me for the rest of the way, this is as far as I can go. :)
Comment on attachment 8663050 [details] [diff] [review]
proposed patch plus tests for bug 1198465

Thanks a lot for writing this up! Passing the review on to Mike.
Attachment #8663050 - Flags: review?(mdeboer)
Assignee: nobody → quicksaver
Mike, review ping
Flags: needinfo?(mdeboer)
Ah, thanks for the ping - it dropped off my radar for some reason... on it!
Flags: needinfo?(mdeboer)
Comment on attachment 8663050 [details] [diff] [review]
proposed patch plus tests for bug 1198465

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

I think this is really close Luis! I have a couple of comments that I'd like to ask you to address... please ping me if anything's not clear!

Thanks and I'm looking forward to the next version.

::: browser/base/content/test/general/browser_bug1198465.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/

Can you move this test to 'toolkit/content/tests/browser/' ?

@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

I think we use this header for CC-blocks:

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

@@ +21,5 @@
> +    "findbar is now focused");
> +
> +  EventUtils.sendChar("z", window);
> +  is(findBar._findField.value, "z", "z erases xy");
> +  

nit: superfluous space.

@@ +24,5 @@
> +  is(findBar._findField.value, "z", "z erases xy");
> +  
> +  findBar._findField.value = "";
> +  ok(!findBar._findField.value, "erase findbar after first test");
> +  

nit: superfluous space.

@@ +34,5 @@
> +  is(findBar._findField.mInputField,
> +    document.activeElement,
> +    "findbar is now focused");
> +
> +  EventUtils.sendChar("a", window);

We now have `yield BrowserTestUtils.sendChar(key, browser);`! With it, this test will work in e10s mode as well.

::: toolkit/content/widgets/findbar.xml
@@ +861,5 @@
>                !val.startsWith(this._findFailedString))
>            {
> +            // Getting here means the user commanded a find op. Make sure any
> +            // initial prefilling is ignored if it hasn't happened yet.
> +            if(this._startFindDeferred) {

nit: missing space between 'if' and '('.

@@ +1024,5 @@
>            if (this.prefillWithSelection && userWantsPrefill) {
>              // NB: We have to focus this._findField here so tests that send
>              // key events can open and close the find bar synchronously.
>              this._findField.focus();
> +            

nit: whitespace.

@@ +1030,5 @@
> +            // only happen in this.onCurrentSelection and, because it is async,
> +            // there's a chance keypresses could come inbetween, leading to
> +            // jumbled up queries.
> +            this._findField.select();
> +            

nit: whitespace.

@@ +1186,5 @@
>          <parameter name="aIsInitialSelection" />
>          <body><![CDATA[
> +          // Ignore the prefill if the user has already typed in the findbar,
> +          // it would have been overwritten anyway. See bug 1198465.
> +          if(aIsInitialSelection && !this._startFindDeferred)

nit: missing space between 'if' and '('.

@@ +1205,5 @@
>              this._enableFindButtons(!!this._findField.value);
>              this._findField.select();
>              this._findField.focus();
> +
> +            this._startFindDeferred.resolve();

Why would you want to remove this conditional?
Attachment #8663050 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #22)
> Comment on attachment 8663050 [details] [diff] [review]
> proposed patch plus tests for bug 1198465
> 
> Review of attachment 8663050 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the comments. I'll address everything in the next patch, it has to wait until the middle of the week though, I hope that's ok (I'll still try to get on it sooner if I can).

> @@ +1205,5 @@
> >              this._enableFindButtons(!!this._findField.value);
> >              this._findField.select();
> >              this._findField.focus();
> > +
> > +            this._startFindDeferred.resolve();
> 
> Why would you want to remove this conditional?

_startFindDeferred has to exist at that point (if aIsInitialSelection ), otherwise execution would never reach there, as the method returns at the top and no-ops. Unless I missed something?
(In reply to Mike de Boer [:mikedeboer] from comment #22)
> @@ +34,5 @@
> > +  is(findBar._findField.mInputField,
> > +    document.activeElement,
> > +    "findbar is now focused");
> > +
> > +  EventUtils.sendChar("a", window);
> 
> We now have `yield BrowserTestUtils.sendChar(key, browser);`! With it, this
> test will work in e10s mode as well.

The test types directly into the findbar which is focused and is outside the browser element, so sending a character to the browser is irrelevant; in fact I think it shouldn't happen at all whether it's in e10s or not. Am I wrong?

The test is failing in e10s because it should use `yield BrowserTestUtils.openNewForegroundTab(gBrowser)` at the top though, I'll make sure to do that in the next version of the test.
Flags: needinfo?(mdeboer)
BTW the patch does work in e10s as well, it just has no practical results because of the bug with key events being lost there (comment 10) taking precedence over this bug. I thought it best to fix that on a separate bug.

Karl, did you ever file a bug for that? I can't find it. I'd be glad to work on it after this bug, I already have an (untested) idea of how to fix it.
(In reply to Luís Miguel [:quicksaver] from comment #25)
> BTW the patch does work in e10s as well, it just has no practical results
> because of the bug with key events being lost there (comment 10) taking
> precedence over this bug. I thought it best to fix that on a separate bug.
> 
> Karl, did you ever file a bug for that?

I haven't filed it, thanks.
Addressed all nits and moved the test into toolkit and fixed it in e10s. I tried using `yield BrowserTestUtils.sendChar` but as I expected it doesn't work. I added a note about that in the test for future reference.
Attachment #8663050 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Attachment #8676767 - Flags: review?(mdeboer)
(In reply to Luís Miguel [:quicksaver] from comment #24)
> The test types directly into the findbar which is focused and is outside the
> browser element, so sending a character to the browser is irrelevant; in
> fact I think it shouldn't happen at all whether it's in e10s or not. Am I
> wrong?

I keep forgetting that this function is about sending an event to the window. Argh. :-P
Status: NEW → ASSIGNED
Comment on attachment 8676767 [details] [diff] [review]
Ensure any typed value overrides any previous search query and the initial prefill

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

Okidoke, this looks great! You want me to push this one to try so we can request checkin?

::: toolkit/content/tests/browser/browser_bug1198465.js
@@ +14,5 @@
> +  //    findbar; the findbar isn't part of the browser content.
> +  //  - we need to _not_ wait for _startFindDeferred to be resolved; yielding
> +  //    a synthesized keypress on the browser implicitely happens after the
> +  //    browser has dispatched its return message with the prefill value for
> +  //    the findbar, which essentially nulls these tests.

Thanks for adding this comment, but I think you can omit it - it was a something I was confused about :)
Attachment #8676767 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #29)
> Okidoke, this looks great! You want me to push this one to try so we can
> request checkin?

Absolutely, I'm sure this is something for which I don't have privileges.

> Thanks for adding this comment, but I think you can omit it - it was a
> something I was confused about :)

Should I upload a new patch without it for the push to try? FWIW personally I'm all for as many comments as possible everywhere; the same way you were confused about it, someone else later reading it might be as well. But you obviously would know best about what to do here. :)
Oooh, prefillWithSelection is false by default in OSX, so _startFindDeferred is immediately resolved, that's where the test is failing. With that disabled, this bug doesn't happen, so neither the patch or the test need to cover that.

What's the correct procedure in this case, enable the preference before the test and revert it at the end? (Is there any special test-method to do this? I don't think I've ever seen a test that does something similar.)
(In reply to Luís Miguel [:quicksaver] from comment #32)
> Oooh, prefillWithSelection is false by default in OSX, so _startFindDeferred
> is immediately resolved, that's where the test is failing. With that
> disabled, this bug doesn't happen, so neither the patch or the test need to
> cover that.

Correct!

> What's the correct procedure in this case, enable the preference before the
> test and revert it at the end? (Is there any special test-method to do this?
> I don't think I've ever seen a test that does something similar.)

Well, the correct procedure would be to read the pref value at the start of the test and bail out (`return;`) early when its value is `false`. Then it will skip this test entirely on OSX, but we needn't worry about that.
(In reply to Mike de Boer [:mikedeboer] from comment #33)
> Well, the correct procedure would be to read the pref value at the start of
> the test and bail out (`return;`) early when its value is `false`. Then it
> will skip this test entirely on OSX, but we needn't worry about that.

Isn't it best to read the pref, set it to true, ensure the test still runs on OSX, then reset the pref back to the original value? I'm sure this bug also happens in OSX if the user sets that pref to true (haven't tested TBH).

BTW in the meantime, found SpecialPowers to do this if needed. :)
Hmm, ok, you're probably right. The way to implement that in the test would be:

```
// Put this before the 'add_task':
var kPrefName = "accessibility.typeaheadfind.prefillwithselection";

Services.prefs.setBoolPref(kPrefName, true);

registerCleanupFunction(function() {
  Services.prefs.clearUserPref(kPrefName);
});
```
Fix test for OSX as discussed. Second try?
Attachment #8676767 - Attachment is obsolete: true
Attachment #8677395 - Flags: review?(mdeboer)
Comment on attachment 8677395 [details] [diff] [review]
Ensure any typed value overrides any previous search query and the initial prefill

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

LGTM!
Attachment #8677395 - Flags: review?(mdeboer) → review+
Failures and bustage seem unrelated to this?
(In reply to Mike de Boer [:mikedeboer] from comment #37)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=075c4af94930

One of the OS X 10.6 debug Mochitest Mochitest M(2) failures demonstrated at
https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b84cc63b2aa9
The OS X 10.6 debug Mochitest Mochitest Browser Chrome M(bc7) failures demonstrated at https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=92d4493c6d58

Don't know what to make of
> 05:21:49 INFO - c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\memory\jemalloc\src\include\jemalloc\internal\../jemalloc.h(353) : fatal error C1070: mismatched #if/#endif pair in file 'c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\memory\jemalloc\src\include\jemalloc\jemalloc.h' 
but this patch would not trigger that.
Keywords: checkin-needed
so both patches (the one attached and the reviewboard patch needto land here ?
Flags: needinfo?(quicksaver)
Comment on attachment 8655351 [details]
MozReview Request: bug 1198465 revert 230d46f387f2 for out-of-order of key events and select r?mikedeboer

Only the patch attached here, I obsoleted the reviewboard patch to avoid further confusion.
Attachment #8655351 - Attachment is obsolete: true
Flags: needinfo?(quicksaver)
https://hg.mozilla.org/mozilla-central/rev/a9c1d48ff133
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.