test_form_autocomplete.html and test_form_autocomplete_with_list.html should pass on e10s

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

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

Updated

3 years ago
Depends on: 874429

Updated

3 years ago
Depends on: 1162330
Assignee

Comment 1

3 years ago
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.
Assignee

Comment 2

3 years ago
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

3 years ago
(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.
Assignee

Comment 4

3 years ago
I think it's hard to even just fix these 2 :)
Assignee

Updated

3 years ago
Blocks: 1162338

Comment 8

3 years ago
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.
Attachment #8727369 - Flags: review?(paolo.mozmail)

Updated

3 years ago
Attachment #8727369 - Flags: review?(paolo.mozmail)
Assignee

Comment 9

3 years ago
(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.
Assignee

Comment 10

3 years ago
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/
Assignee

Comment 11

3 years ago
it's incredible how a simple letter change can test more stuff :)
Assignee

Comment 12

3 years ago
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/

Updated

3 years ago
Attachment #8727369 - Flags: review?(paolo.mozmail)

Comment 13

3 years ago
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.

Updated

3 years ago
Attachment #8727369 - Flags: review?(paolo.mozmail)
Assignee

Comment 14

3 years ago
(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...
Assignee

Comment 15

3 years ago
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.
Assignee

Comment 16

3 years ago
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

3 years ago
(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

3 years ago
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!
Attachment #8727369 - Flags: review?(paolo.mozmail) → review+

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee0cb75bcbf9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee

Comment 21

3 years ago
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.
Attachment #8727369 - Flags: approval-mozilla-aurora?
I'd like this to stabilize in central for a bit before uplifting to Aurora.
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+
Attachment #8727369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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 ?
Flags: needinfo?(mak77)
Assignee

Comment 25

3 years ago
(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.
Flags: needinfo?(mak77) → needinfo?(cbook)
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.