e10s fix test_bug_511615.html and test_bug_787624.html

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Autocomplete
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mrbkap, Assigned: Paolo)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(e10s+, firefox48 fixed)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
These two tests both need direct access to the autocomplete popup to work. They probably need to proxy those tests to the parent or something like that.
(Assignee)

Updated

2 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED

Updated

2 years ago
Blocks: 1216897
Iteration: --- → 47.3 - Mar 7
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [fxprivacy]
(Assignee)

Updated

2 years ago
Depends on: 1162330
(Assignee)

Updated

2 years ago
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → NEW
Depends on: 1252074

Updated

2 years ago
Iteration: 47.3 - Mar 7 → ---
Priority: P1 → --

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
(Assignee)

Comment 1

2 years ago
I'll start with a patch for test_bug_787624.html since it turns out that most of these tests were unrelated, probably just copied from test_bug_511615.html. They are still checked there.

Also, the direction="rtl" changes were unrelated and the test works as well without them. I've backed out the patch in bug 787624 and verified that the test fails.

I'll work on the other test next. I'll fix both tests in this bug, both because they're related and because we're using the number of bugs for release calculations and it makes sense to have a single item.
(Assignee)

Comment 2

2 years ago
Created attachment 8729570 [details]
MozReview Request: Bug 1162338 - Part 1 - Fix test_bug_787624.html to work in e10s. r=mak

Review commit: https://reviewboard.mozilla.org/r/39507/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39507/
Attachment #8729570 - Flags: review?(mak77)

Updated

2 years ago
Priority: P2 → P1
Comment on attachment 8729570 [details]
MozReview Request: Bug 1162338 - Part 1 - Fix test_bug_787624.html to work in e10s. r=mak

https://reviewboard.mozilla.org/r/39507/#review36279

finally some sanity!
Attachment #8729570 - Flags: review?(mak77) → review+
(Assignee)

Updated

2 years ago
Depends on: 1256297
(Assignee)

Comment 4

2 years ago
Created attachment 8730190 [details]
MozReview Request: Bug 1162338 - Part 2 - Fix test_bug_511615.html to work in e10s. r=mak

Review commit: https://reviewboard.mozilla.org/r/39743/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39743/
Attachment #8730190 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Attachment #8729570 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Created attachment 8730191 [details]
MozReview Request: Bug 1162338 - Fail tests.

Review commit: https://reviewboard.mozilla.org/r/39745/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39745/
(Assignee)

Updated

2 years ago
Attachment #8730190 - Attachment is obsolete: true
Attachment #8730190 - Flags: review?(mak77)
(Assignee)

Comment 6

2 years ago
Created attachment 8730192 [details]
MozReview Request: Bug 1162338 - Part 1 - Fix test_bug_787624.html to work in e10s. r=mak

Review commit: https://reviewboard.mozilla.org/r/39747/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39747/
Attachment #8730192 - Flags: review?(mak77)
(Assignee)

Comment 7

2 years ago
Created attachment 8730194 [details]
MozReview Request: Bug 1162338 - Part 2 - Fix test_bug_511615.html to work in e10s. r=mak

Review commit: https://reviewboard.mozilla.org/r/39749/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39749/
Attachment #8730194 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Attachment #8730192 - Flags: review?(mak77) → review+
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/39749/#review36387

::: toolkit/components/satchel/test/test_bug_511615.html:141
(Diff revision 1)
> -  }
> -  else {
> -    ok(false, "Autocomplete popup not expected" + testNum);
> -  }
> +    () => doKeyUnprivileged("down"),
> +    () => doKeyUnprivileged("page_down"),
> +    () => doKeyUnprivileged("return"),
> +    () => doKeyUnprivileged("v"),
> +    () => doKeyUnprivileged(" "),
> +    () => doKeyUnprivileged("back_space"),

Marco, if you think that some of these key presses (or in similar code blocks below) might be redundant, please tell me which ones I could remove, because each one of them delays the test a little bit.

In general the security issue tested by this code is less likely to re-appear since window listeners must now explicitly specify if they would like to get untrusted events. This can be seen in the last changeset where I changed the code to fail the tests.
(Assignee)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f2942590d6
Comment on attachment 8730192 [details]
MozReview Request: Bug 1162338 - Part 1 - Fix test_bug_787624.html to work in e10s. r=mak

https://reviewboard.mozilla.org/r/39747/#review36619
Attachment #8730192 - Flags: review+
Comment on attachment 8730194 [details]
MozReview Request: Bug 1162338 - Part 2 - Fix test_bug_511615.html to work in e10s. r=mak

https://reviewboard.mozilla.org/r/39749/#review36611

::: toolkit/components/satchel/test/test_bug_511615.html:40
(Diff revision 1)
> -  var formID = input.parentNode.id;
> -  is(input.value, expectedValue, "Checking " + formID + " input");
> -}
> + * Indicates the time to wait before checking that the state of the autocomplete
> + * popup, including whether it is open, has not changed in response to events.
> + *
> + * Manual testing on a fast machine revealed that 80ms was still unreliable,
> + * while 100ms detected a simulated failure reliably. Unfortunately, this means
> + * that to take into account slower machines we should use a value like 300ms.

this states 300, but then the actual used value is 200... Maybe here we should just state "a larger value".

::: toolkit/components/satchel/test/test_bug_511615.html:45
(Diff revision 1)
> + * that to take into account slower machines we should use a value like 300ms.
> + *
> + * Note that if a machine takes more than this time to show the popup, this
> + * would not cause a failure, conversely the machine would not be able to detect
> + * whether the test should have failed. In other words, this use of timeouts is
> + * never expected to cause intermittent failures with test automation.

This comment is unclear here, I'd move it later, where you added the Promise.race then it would be clear why it won't cause intermittent failures.

::: toolkit/components/satchel/test/test_bug_511615.html:189
(Diff revision 1)
> +    yield checkSelectedIndexAfterResponseTime(0);
> +    is(input.value, "", "The selected item should not have been used.");
> +  }
>  
> -SimpleTest.waitForExplicitFinish();
> +  // Close the popup.
> +  doKey("escape");

nit: a input.blur() should also do

::: toolkit/components/satchel/test/test_popup_direction.html:14
(Diff revision 1)
> +</head>
> +<body>
> +Test for Popup Direction
> +<p id="display"></p>
> +
> +<!-- we presumably can't hide the content for this test. -->

nit: I'm not sure what's the scope of this comment in all the tests here...

::: toolkit/components/satchel/test/test_popup_direction.html:34
(Diff revision 1)
> +function waitForNextPopup() {
> +  return new Promise(resolve => { resolvePopupShownListener = resolve; });
> +}
> +
> +add_task(function* test_popup_direction() {
> +  var input = $_(1, "field1");

nit: let

::: toolkit/components/satchel/test/test_popup_direction.html:54
(Diff revision 1)
> +
> +    let popupState = yield new Promise(resolve => getPopupState(resolve));
> +    is(popupState.direction, direction, "Direction should match.");
> +
> +    // Close the popup.
> +    doKey("escape");

ditto
Attachment #8730194 - Flags: review?(mak77) → review+
(Assignee)

Comment 12

2 years ago
(In reply to Marco Bonardo [::mak] from comment #11)
> ::: toolkit/components/satchel/test/test_bug_511615.html:45
> (Diff revision 1)
> > + * that to take into account slower machines we should use a value like 300ms.
> > + *
> > + * Note that if a machine takes more than this time to show the popup, this
> > + * would not cause a failure, conversely the machine would not be able to detect
> > + * whether the test should have failed. In other words, this use of timeouts is
> > + * never expected to cause intermittent failures with test automation.
> 
> This comment is unclear here, I'd move it later, where you added the
> Promise.race then it would be clear why it won't cause intermittent failures.

I put the comment in that place because if an unrelated intermittent failure occurs, it's likely the first thing someone would look into is the "requestFlakyTimeout" call, maybe trying to adjust the timeout above it, so putting the explanation there might save some time by explaining that the choice of that value couldn't be responsible for intermittent failures (as far as I can tell!).
(Assignee)

Comment 13

2 years ago
(In reply to Marco Bonardo [::mak] from comment #11)
> > +<!-- we presumably can't hide the content for this test. -->
> 
> nit: I'm not sure what's the scope of this comment in all the tests here...

That's because the mochitest template specifies "display: none" for the "div" element.

http://mxr.mozilla.org/mozilla-central/search?string=%3Cdiv+id%3D%22content%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/58fab9169d7b
https://hg.mozilla.org/integration/fx-team/rev/a3c038ea446c

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58fab9169d7b
https://hg.mozilla.org/mozilla-central/rev/a3c038ea446c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.