Closed Bug 1319237 Opened 8 years ago Closed 7 years ago

Make setWindowSize and setWindowPosition synchronous

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 wontfix, firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

(Keywords: pi-marionette-server, pi-marionette-spec)

Attachments

(14 files, 1 obsolete file)

Bug 1319237 - Correct window size- and position test;
58 bytes, text/x-review-board-request
automatedtester
: review+
jgraham
: review+
Details
Bug 1319237 - Calculate correct window position;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1319237 - Assign GeckoDriver#getWindowSize return value in one statement;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1319237 - Allow pytests to be skipped imperatively at runtime;
58 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1319237 - Make session fixture module scoped;
58 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1319237 - Generalise wait condition utility;
58 bytes, text/x-review-board-request
automatedtester
: review+
impossibus
: review+
Details
Bug 1319237 - Make GeckoDriver#setWindowPosition synchronous;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1319237 - Make GeckoDriver#setWindowSize synchronous;
58 bytes, text/x-review-board-request
automatedtester
: review+
impossibus
: review+
Details
Bug 1319237 - Donate window position test to Mozilla;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1319237 - Improve window position test assertions;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1319237 - Avoid reposition waiting if position is unchanged;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1319237 - Enable WebDriver context test;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1319237 - Disable WebDriver navigation to file: protocol test;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1319237 - Fix calls to session.find;
59 bytes, text/x-review-board-request
impossibus
: review+
Details 
The GeckoDriver#setWindowSize and GeckoDriver#setWindowPosition commands are not synchronous in that they don’t wait for the size/position to change before returning.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8812935 [details]
Bug 1319237 - Correct window size- and position test;

https://reviewboard.mozilla.org/r/94464/#review94884

::: testing/web-platform/tests/webdriver/contexts.py
(Diff revision 1)
>      # so we should see the results immediately
>  
>      session.window.size = (400, 500)
>      assert session.window.size == (400, 500)
>  
> -    session.window.size = (500, 600)

I think this is expected to work; why was it removed?
Comment on attachment 8812939 [details]
Bug 1319237 - Allow pytests to be skipped imperatively at runtime;

https://reviewboard.mozilla.org/r/94472/#review94888

::: testing/web-platform/harness/wptrunner/executors/pytestrunner/runner.py:92
(Diff revision 1)
>          if report.when != "call":
>              message = "%s error" % report.when
>          self.record(report.nodeid, "ERROR", message, report.longrepr)
>  
>      def record_skip(self, report):
> -        self.record(report.nodeid, "ERROR",
> +        self.record(report.nodeid, "PASS")

I guess this is strictly more expressive, but we have to be careful not to allow people to use it except for optional features.
Attachment #8812939 - Flags: review?(james) → review+
Comment on attachment 8812940 [details]
Bug 1319237 - Make session fixture module scoped;

https://reviewboard.mozilla.org/r/94474/#review94890
Attachment #8812940 - Flags: review?(james) → review+
Comment on attachment 8812943 [details]
Bug 1319237 - Make GeckoDriver#setWindowSize synchronous;

https://reviewboard.mozilla.org/r/94480/#review94908
Attachment #8812943 - Flags: review?(dburns) → review+
Comment on attachment 8812942 [details]
Bug 1319237 - Make GeckoDriver#setWindowPosition synchronous;

https://reviewboard.mozilla.org/r/94478/#review94910
Attachment #8812942 - Flags: review?(dburns) → review+
Comment on attachment 8812941 [details]
Bug 1319237 - Generalise wait condition utility;

https://reviewboard.mozilla.org/r/94476/#review94914
Attachment #8812941 - Flags: review?(dburns) → review+
Comment on attachment 8812938 [details]
Bug 1319237 - Assign GeckoDriver#getWindowSize return value in one statement;

https://reviewboard.mozilla.org/r/94470/#review94916
Attachment #8812938 - Flags: review?(dburns) → review+
Comment on attachment 8812937 [details]
Bug 1319237 - Mark yielding functions as generators;

https://reviewboard.mozilla.org/r/94468/#review94918
Attachment #8812937 - Flags: review?(dburns) → review+
Comment on attachment 8812936 [details]
Bug 1319237 - Calculate correct window position;

https://reviewboard.mozilla.org/r/94466/#review94920
Attachment #8812936 - Flags: review?(dburns) → review+
Comment on attachment 8812935 [details]
Bug 1319237 - Correct window size- and position test;

https://reviewboard.mozilla.org/r/94464/#review94884

> I think this is expected to work; why was it removed?

Oops.
Comment on attachment 8812935 [details]
Bug 1319237 - Correct window size- and position test;

https://reviewboard.mozilla.org/r/94464/#review95264
Attachment #8812935 - Flags: review?(dburns) → review+
Comment on attachment 8812935 [details]
Bug 1319237 - Correct window size- and position test;

https://reviewboard.mozilla.org/r/94464/#review95270
Attachment #8812935 - Flags: review?(james) → review+
What is this waiting for?
Flags: needinfo?(ato)
(In reply to David Burns :automatedtester from comment #49)
> What is this waiting for?

Unexplained test failures.
Flags: needinfo?(ato)
I think I found the source of the test failures.  element.find was returning [] to wait.until when failing to find the elements, which in JS evaluates to true.  Changing this to false fixes the problem.
Attachment #8812937 - Attachment is obsolete: true
Blocks: 1328298
Comment on attachment 8812943 [details]
Bug 1319237 - Make GeckoDriver#setWindowSize synchronous;

https://reviewboard.mozilla.org/r/94480/#review110332

::: testing/marionette/driver.js:2453
(Diff revision 9)
>  
>    let {width, height} = cmd.parameters;
> +
>    let win = this.getCurrentWindow();
> +  yield new Promise(resolve => {
> +    win.addEventListener("resize", resolve, {once: true});

As discussed with Andreas on IRC this can cause a hang of Marionette with a final force kill of Firefox if the event is never raised.

I proposed to implement a timed event handler which could reject the promise if the event hasn't been fired after a specified amount of time.

I'm going to file a new bug for it.
Comment on attachment 8812941 [details]
Bug 1319237 - Generalise wait condition utility;

https://reviewboard.mozilla.org/r/94476/#review110740

::: testing/marionette/test_wait.js:11
(Diff revision 10)
> +
> +Cu.import("chrome://marionette/content/wait.js");
> +
> +add_task(function* test_until_types() {
> +  for (let typ of [true, false, "foo", 42, [], {}]) {
> +    equal(typ, yield wait.until(resolve => resolve(typ)));

I think you want `strictEqual` since you're checking `[]`.

::: testing/marionette/wait.js:46
(Diff revision 10)
> + *         reject([]);
> + *       }
> + *     });
> + *
> + * @param {function(resolve: function(?), reject: function(?)): ?} func
> + *     Function to run on the main thread.

s/on/off/
Attachment #8812941 - Flags: review?(mjzffr) → review+
Comment on attachment 8832886 [details]
Bug 1319237 - Improve window position test assertions;

https://reviewboard.mozilla.org/r/109132/#review110768

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_position.py:12
(Diff revision 2)
> -        position = self.marionette.get_window_position()
> -        self.assertTrue(isinstance(position["x"], int))
> -        self.assertTrue(isinstance(position["y"], int))
> +        pos = self.marionette.get_window_position()
> +        self.assertIsInstance(pos["x"], int)
> +        self.assertIsInstance(pos["y"], int)

I prefer to avoid abbreviations, but ok.
Attachment #8832886 - Flags: review?(mjzffr) → review+
Comment on attachment 8832887 [details]
Bug 1319237 - Avoid reposition waiting if position is unchanged;

https://reviewboard.mozilla.org/r/109134/#review110770
Attachment #8832887 - Flags: review?(mjzffr) → review+
Comment on attachment 8833333 [details]
Bug 1319237 - Disable WebDriver navigation to file: protocol test;

https://reviewboard.mozilla.org/r/109592/#review110778
Attachment #8833333 - Flags: review?(mjzffr) → review+
Comment on attachment 8832885 [details]
Bug 1319237 - Donate window position test to Mozilla;

https://reviewboard.mozilla.org/r/109130/#review111026
Attachment #8832885 - Flags: review?(dburns) → review+
Comment on attachment 8812941 [details]
Bug 1319237 - Generalise wait condition utility;

https://reviewboard.mozilla.org/r/94476/#review110740

> I think you want `strictEqual` since you're checking `[]`.

Well spotted.
Comment on attachment 8832886 [details]
Bug 1319237 - Improve window position test assertions;

https://reviewboard.mozilla.org/r/109132/#review110768

> I prefer to avoid abbreviations, but ok.

Fixed.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5f206ed675b
Correct window size- and position test; r=automatedtester,jgraham
https://hg.mozilla.org/integration/autoland/rev/feaf6ba3bd9a
Calculate correct window position; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/778721edd8b9
Assign GeckoDriver#getWindowSize return value in one statement; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/af71a9376bef
Allow pytests to be skipped imperatively at runtime; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/a2096fa29cd7
Make session fixture module scoped; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/76c8aafbd933
Generalise wait condition utility; r=automatedtester,maja_zf
https://hg.mozilla.org/integration/autoland/rev/f651a9e54a57
Make GeckoDriver#setWindowPosition synchronous; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/8df8cda56942
Make GeckoDriver#setWindowSize synchronous; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f2bdf078f39a
Donate window position test to Mozilla; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f8449c471b8f
Improve window position test assertions; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/4dc796bbcfaa
Avoid reposition waiting if position is unchanged; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/1f7f75c57c59
Enable WebDriver context test; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/9746878e5fc7
Disable WebDriver navigation to file: protocol test; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/c85849b790c5
Fix calls to session.find; r=maja_zf
This also created some intermittents in Wd, apparently.
Comment on attachment 8812943 [details]
Bug 1319237 - Make GeckoDriver#setWindowSize synchronous;

https://reviewboard.mozilla.org/r/94480/#review113008

::: testing/marionette/driver.js:2423
(Diff revision 14)
>  
>  /**
>   * Set the size of the browser window currently in focus.
>   *
> - * Not supported on B2G. The supplied width and height values refer to
> - * the window outerWidth and outerHeight values, which include scroll
> + * The supplied width and height values refer to the window outerWidth
> + * and outerHeight values, which include scrollbars.

"which include scrollbars" is misleading because innerWidth|Height also include scrollbars. The distinction is that outerWidth also includes borders, chrome, etc.
Attachment #8812943 - Flags: review?(mjzffr) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c3e1a41a6a4
Correct window size- and position test; r=automatedtester,jgraham
https://hg.mozilla.org/integration/autoland/rev/5c289841a713
Calculate correct window position; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f94feb5c6ce6
Assign GeckoDriver#getWindowSize return value in one statement; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/c5976f923a9e
Allow pytests to be skipped imperatively at runtime; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/fd248cd2735a
Make session fixture module scoped; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/c3e0436c49ed
Generalise wait condition utility; r=automatedtester,maja_zf
https://hg.mozilla.org/integration/autoland/rev/a26205ad227c
Make GeckoDriver#setWindowPosition synchronous; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/b37ea97fa14d
Make GeckoDriver#setWindowSize synchronous; r=automatedtester,maja_zf
https://hg.mozilla.org/integration/autoland/rev/60859dc1fde4
Donate window position test to Mozilla; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/eb9c22a9b707
Improve window position test assertions; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/c460127ccb63
Avoid reposition waiting if position is unchanged; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/c9e7feb55b9a
Enable WebDriver context test; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/5cfe97b88163
Disable WebDriver navigation to file: protocol test; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/5a01a4abbd7e
Fix calls to session.find; r=maja_zf
I have now addressed the intermittent try failures on the Wd job.  I found that the DOM resize event is, despite its promises, not synchronous.  When it says it fires after the document view has been resized, it is most definitely lying.  Because resize events can fire at a high rate, DOM modifications are not guaranteed to have propagated by the time it fires.  This means, amongst other things, that outerWidth/outerHeight may not have been written to the DOM by the time resize has fired.

Even more interesting, I found that when running the WPT WebDriver tests, the width axis would get updated before the height axis.  I discovered this only after adding a synchronous wait condition for outerWidth/outerHeight to change.  To add to the fun, requestAnimationFrame, which is what we should be using for this, does not want to play well with our praticular flavour of XUL.  On the upside, it is good to know that messaging passing in Marionette is fast, and also that a test I wrote for WPT ages ago saved us from integrating faulty code into Gecko.  The downside is, I guess, that the web still does suck.
Flags: needinfo?(ato)
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/60b6ab14650a
Lower timeout elapse evaluation count; r=me a=followup-to-prevent-back out
a=test only
Whiteboard: [checkin-needed-beta][checkin-needed-aurora]
Needs rebasing for Beta uplift.
status-firefox52: --- → affected
Whiteboard: [checkin-needed-beta]
See Also: → 1340775
Too late for 52.  If you want this in esr52 feel free to nominate rebased patches.
status-firefox52: affected → wontfix
Andreas, having this patch series not uplifted to esr52 causes merge conflicts for the required uplift of all the patches related to bug 1322383 (valid window checks). 

I tried to check if all can be uplifted but looks like two changesets cause merge conflicts. Both are for the webplatform tests, and updating them seems to be not that difficult. 

Would you do us the favor and check what's necessary?

grafting 380670:a8a6d17f60af "Bug 1319237 - Correct window size- and position test. r=automatedtester, r=jgraham, a=test-only"
merging testing/web-platform/tests/webdriver/contexts.py

grafting 380681:7b41cdbb9c97 "Bug 1319237 - Enable WebDriver context test. r=maja_zf, a=test-only"
merging testing/web-platform/meta/webdriver/contexts.py.ini

Thanks!
status-firefox-esr52: --- → affected
Flags: needinfo?(ato)
Necessary in what regard?  I think if you do not resolve the conflicts or uplift them, there is a chance Wd tests will fail.

If the changes don’t affect tests, I don’t necessarily think they are very useful to uplift.
Flags: needinfo?(ato)
Necessary to get the patches updated so that everything on this bug can be uplifted to esr52.
In our meeting yesterday we agreed to not uplift this patch to esr52. So marking this branch as wontfix.
status-firefox-esr52: affected → wontfix
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: