1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Marco Bonardo [:mak]]

Some work happened in bug 1162329, but the tests were not enabled cause they were still unstable

Comment 1

[Marco Bonardo [:mak]]

Btw, the reason the tests were not enabled is that the implementation is incomplete, the e10s version of formfill seems to only handle most common cases, but fails on a lot of edge case tests. There are a lot of tests here.
This will take some time cause it's very different from the underlying autocomplete implementation and I need to understand how the order of events changes across the 2 versions.

Comment 2

[Marco Bonardo [:mak]]

as a side note for anyone working on satchel tests (I guess it's Paolo mostly), there are 2 things broken:
1. when the controller detaches from an autocomplete field, it doesn't notify the e01s implementation, that keeps retaining old bogus data.
2. e10s implementation is not reusing previousResult as the old implementation

I have a patch that fixes these and passes test_form_autocomplete.html
I don't know if it passes the _with_list alternative nor if it's stable, but I wanted to point out these 2 bugs cause they may affect the other tests too.

Comment 3

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #2)
> I have a patch that fixes these and passes test_form_autocomplete.html
> I don't know if it passes the _with_list alternative nor if it's stable, but
> I wanted to point out these 2 bugs cause they may affect the other tests too.

Thanks for the info, I'll wait with bug 1162338 until this patch is ready. By the way, if you think it will be simpler to fix all the four tests in one go, feel free to take the other bug too.

Comment 4

[Marco Bonardo [:mak]]

I think it's hard to even just fix these 2 :)

Comment 5

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d09a93ea7f4

Comment 6

[Marco Bonardo [:mak]]

apart from infra failures, looks like this will do
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8f7f37a6a8&selectedJob=17707654

Comment 7

[Marco Bonardo [:mak]]

Attachment: MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

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

Comment 8

[:Paolo Amadini]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

https://reviewboard.mozilla.org/r/38463/#review35035

::: toolkit/components/satchel/nsFormAutoComplete.js:505
(Diff revision 1)
> +        previousSearchString: aPreviousResult && aPreviousResult.searchString.trim().toLowerCase(),

Reviewing this will take some time while I check the code locally. For the moment, I've tested that this works but only read the changes without following the flow precisely.

One thing I noticed is that we call "toLowerCase" here, and in another place we call "includes" between what appears to be this string and aUntrimmedSearchString. Is the latter already lowercased, or are we losing the previous result if there's an uppercase letter? Maybe this emulates previous behavior, or I'm reading this incorrectly?

Apparently all the tests in the touched test file use lowercase letters, this is why I suspect there could be a mismatch.

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #8)
> One thing I noticed is that we call "toLowerCase" here, and in another place
> we call "includes" between what appears to be this string and
> aUntrimmedSearchString. Is the latter already lowercased, or are we losing
> the previous result if there's an uppercase letter?

I'll take a look, sounds like a plausible mistake and could be missing specific test coverage.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

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

Comment 11

[Marco Bonardo [:mak]]

it's incredible how a simple letter change can test more stuff :)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

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

Comment 13

[:Paolo Amadini]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

https://reviewboard.mozilla.org/r/38463/#review35355

::: toolkit/components/satchel/test/test_form_autocomplete_with_list.html:383
(Diff revision 3)
> -      expectPopup();
> +      waitForMenuChange(0);
> -      restoreForm();
> -      doKey("down");
>        break;
>  
> -    case 204:
> -      // Change the textContent to update the value attribute.
> +    case 205:
> +      restoreForm();
> -      doKey("down");

I reviewed everything except for the test case changes. I have two questions while I'm looking at the tests in more detail.

First, is the comment removal intentional?

Second, does the removal of doKey("down") indicate a change in behavior when opening the popup, was it just unnecessary in the original test to begin with, or something else?

::: toolkit/content/browser-child.js:570
(Diff revision 3)
> +    element.addEventListener("FormFillDisconnect", function disconnect() {
> +      element.removeEventListener("FormFillDisconnect", disconnect);
> +      sendAsyncMessage("FormAutoComplete:Disconnect");
> +    });

I guess this event won't happen and the message won't be forwarded to the parent if we just remove the input element from the DOM. It's unclear to me if this can have any impact, though.

Comment 14

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #13)
> First, is the comment removal intentional?

no, it slipped through.

> Second, does the removal of doKey("down") indicate a change in behavior when
> opening the popup, was it just unnecessary in the original test to begin
> with, or something else?

The problem is that timing changes from non-e10s to e10s, cause in the latter case there is async message passing, so something that before we were expecting to happen immediately, can happen later. For example when something changes in the datalist contents, autocomplete is not updated synchronously.
This specific "down" imo was pointless, we were restoring the input contents and pressing "down" to open the popup and move to the next test, then pressing "down" again (to select the first entry? for what?).
I changed the test to also check the removed entry has been removed (we were not checking that before), plus properly wait for its removal, that is now asynchronous.
Even if I add back "down", it won't do anything useful.

> ::: toolkit/content/browser-child.js:570
> (Diff revision 3)
> > +    element.addEventListener("FormFillDisconnect", function disconnect() {
> > +      element.removeEventListener("FormFillDisconnect", disconnect);
> > +      sendAsyncMessage("FormAutoComplete:Disconnect");
> > +    });
> 
> I guess this event won't happen and the message won't be forwarded to the
> parent if we just remove the input element from the DOM. It's unclear to me
> if this can have any impact, though.

I actually tried to send an ipc message out to the messagemanager, I also arrived up to have the right messagemanager in the cpp controller... but then sending a message requires a principal and a js context. The code becomes too hairy at that point. So i had to stick to a custom event.
Do you have in mind any case where code could remove the input while the user is interacting with it?
Eventually we could listen on the documentElement... or if you have more robust ideas, I'm listening...

Comment 15

[Marco Bonardo [:mak]]

I was thinking I may try to add a new XPCOM interface and from cpp try to QI the popup and invoke it. I will try and see if it works.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

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

Comment 17

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #14)
> then pressing "down" again (to select the first entry? for what?).

That's what I suspected. Thanks for fixing that!

Comment 18

[:Paolo Amadini]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

https://reviewboard.mozilla.org/r/38463/#review35831

Looks simpler and more robust. r+ if all tests pass on try!

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ee0cb75bcbf9

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: form history implementation is incomplete
[Describe test coverage new/current, TreeHerder]: unit tests
[Risks and why]: the made changes should not cause scary behaviors of form history, test coverage is decent
[String/UUID change made/needed]: added one method to nsIFormAutoComplete, should not cause compatibility troubles.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like this to stabilize in central for a bit before uplifting to Aurora.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8727369 [details]
MozReview Request: Bug 1252074 - test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s. r=paolo

Been in Nightly for a few days, needed to improve e10s story, Aurora47+

Comment 24

[Carsten Book [:Tomcat]]

uplift to aurora resulted in :

remote: added 9 changesets with 31 changes to 28 files
remote: 
remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIFormAutoComplete in changeset 9ad3bb0265df
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIFormAutoComplete
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.
remote: *************************************************************

can you take a look ?

Comment 25

[Marco Bonardo [:mak]]

(In reply to Carsten Book [:Tomcat] from comment #24)
> remote: *************************** ERROR ***************************
> remote: Push rejected because the following IDL interfaces were
> remote: modified without changing the UUID:
> remote:   - nsIFormAutoComplete in changeset 9ad3bb0265df
> remote: 
> remote: To update the UUID for all of the above interfaces and their
> remote: descendants, run:
> remote:   ./mach update-uuids nsIFormAutoComplete
> remote: 
> remote: If you intentionally want to keep the current UUID, include
> remote: 'IGNORE IDL' in the commit message.
> remote: *************************************************************

Something is wrong with this hook, it is no more needed to bump the uuid of idl files, so I don't know why we still have a hook checking that, likely someone forgot to remove it.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09df85bf34d9