Closed Bug 1381876 Opened 7 years ago Closed 7 years ago

Ensure Maximize Window command is synchronous

Categories

(Remote Protocol :: Marionette, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 1 obsolete file)

59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
It has recently been discovered through various intermittents (1, 2) that the Maximize Window command in Marionette is not as synchronous as we first thought.

This will also affect the Minimize Window command (3) once it lands.

This bug is to identify and fix the cause of the fragility.

1. https://bugzilla.mozilla.org/show_bug.cgi?id=1380309
2. https://bugzilla.mozilla.org/show_bug.cgi?id=1380513
3. https://bugzilla.mozilla.org/show_bug.cgi?id=1378121
Assignee: nobody → ato
Status: NEW → ASSIGNED
Could this be again because the `resize` event is getting fired with a high rate as described here?

https://developer.mozilla.org/en-US/docs/Web/Events/resize#Examples
Ok, I think I found what we have to do here:
https://developer.mozilla.org/en-US/docs/Web/Events/sizemodechange

> Note: This event may fire several times or while resizing the window (for example, see bug 715867), so always check if the window state changed by inspecting window.windowState in the event.
Blocks: 1375185
Comment on attachment 8892527 [details]
Bug 1381876 - Return window rect from Context#rect consistently.

https://reviewboard.mozilla.org/r/163514/#review168990
Attachment #8892527 - Flags: review?(dburns) → review+
Comment on attachment 8892526 [details]
Bug 1381876 - Rename Maximize Window test to match command name.

https://reviewboard.mozilla.org/r/163512/#review168992
Attachment #8892526 - Flags: review?(dburns) → review+
Comment on attachment 8892528 [details]
Bug 1381876 - Include window state in window rect.

https://reviewboard.mozilla.org/r/163516/#review168994
Attachment #8892528 - Flags: review?(dburns) → review+
Comment on attachment 8892529 [details]
Bug 1381876 - Fix window maximised assertions.

https://reviewboard.mozilla.org/r/163518/#review169006
Attachment #8892529 - Flags: review?(dburns) → review+
Comment on attachment 8892530 [details]
Bug 1381876 - Remove unused import from test_window_maximize.py.

https://reviewboard.mozilla.org/r/163520/#review169008
Attachment #8892530 - Flags: review?(dburns) → review+
Comment on attachment 8892531 [details]
Bug 1381876 - Compare window rect, not window size.

https://reviewboard.mozilla.org/r/163522/#review169010
Attachment #8892531 - Flags: review?(dburns) → review+
Comment on attachment 8892532 [details]
Bug 1381876 - Allow assert_success to be used without an expected value.

https://reviewboard.mozilla.org/r/163524/#review169012
Attachment #8892532 - Flags: review?(dburns) → review+
Comment on attachment 8892533 [details]
Bug 1381876 - Make client.Session#maximize a method.

https://reviewboard.mozilla.org/r/163526/#review169016
Attachment #8892533 - Flags: review?(dburns) → review+
Comment on attachment 8892534 [details]
Bug 1381876 - Skip resetting namespaced interfaces in client.Session.

https://reviewboard.mozilla.org/r/163528/#review169022
Attachment #8892534 - Flags: review?(dburns) → review+
Comment on attachment 8892535 [details]
Bug 1381876 - Correct WPT maximize window tests.

https://reviewboard.mozilla.org/r/163530/#review169024

::: testing/web-platform/tests/webdriver/tests/maximize_window.py:62
(Diff revision 2)
> +    # Determine the largest available window size by first maximising
> +    # the window and getting the window rect dimensions.
> +    #
> +    # Then resize the window to the maximum available size.
> +    session.end()
> +    available = session.window.maximize()

How can you `end()` a session and still use it? This looks really weird
Attachment #8892535 - Flags: review?(dburns) → review-
Comment on attachment 8892536 [details]
Bug 1381876 - Rename WaitCondition to Condition.

https://reviewboard.mozilla.org/r/163532/#review169026
Attachment #8892536 - Flags: review?(dburns) → review+
Comment on attachment 8892537 [details]
Bug 1381876 - Introduce TimedPromise.

https://reviewboard.mozilla.org/r/163534/#review169030

::: commit-message-a3ff1:7
(Diff revision 2)
> +
> +This introduces a specialisation of the well-known Promise that can
> +time out after a set limit, causing the promises' "reject" callback to
> +be invoked.
> +
> +The TimedPromise object represents the tiemd, eventual completion

s/tiemd/timed/
Attachment #8892537 - Flags: review?(dburns) → review+
Comment on attachment 8892538 [details]
Bug 1381876 - Ensure Maximize Window is synchronous.

https://reviewboard.mozilla.org/r/163536/#review169036
Attachment #8892538 - Flags: review?(dburns) → review+
Comment on attachment 8892535 [details]
Bug 1381876 - Correct WPT maximize window tests.

https://reviewboard.mozilla.org/r/163530/#review169024

> How can you `end()` a session and still use it? This looks really weird

API methods on the WPT WebDriver test client are decorated with @command, which implicitly creates a session if one does not exist.  So when session.end() ends the session, session.window.maximize()’s decorator creates a new one for us.
Comment on attachment 8892535 [details]
Bug 1381876 - Correct WPT maximize window tests.

https://reviewboard.mozilla.org/r/163530/#review169356
Attachment #8892535 - Flags: review?(dburns) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 949bb36f8db1 -d f034b3a5d6a8: rebasing 411068:949bb36f8db1 "Bug 1381876 - Rename Maximize Window test to match command name. r=automatedtester"
rebasing 411069:8bb14677a89c "Bug 1381876 - Return window rect from Context#rect consistently. r=automatedtester"
rebasing 411070:427852655e74 "Bug 1381876 - Include window state in window rect. r=automatedtester"
rebasing 411071:4f3711d64153 "Bug 1381876 - Fix window maximised assertions. r=automatedtester"
rebasing 411072:83f0fac35b63 "Bug 1381876 - Remove unused import from test_window_maximize.py. r=automatedtester"
rebasing 411073:61f05633511e "Bug 1381876 - Compare window rect, not window size. r=automatedtester"
rebasing 411074:2b7a435fc36c "Bug 1381876 - Allow assert_success to be used without an expected value. r=automatedtester"
rebasing 411075:c4c0c2b74582 "Bug 1381876 - Make client.Session#maximize a method. r=automatedtester"
merging testing/web-platform/tests/tools/webdriver/webdriver/client.py
rebasing 411076:244142f43561 "Bug 1381876 - Skip resetting namespaced interfaces in client.Session. r=automatedtester"
merging testing/web-platform/tests/tools/webdriver/webdriver/client.py
rebasing 411077:b5a95d7f9587 "Bug 1381876 - Correct WPT maximize window tests. r=automatedtester"
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Attachment #8892535 - Attachment is obsolete: true
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86cb93e08b08
Rename Maximize Window test to match command name. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/92a8be47bbd7
Return window rect from Context#rect consistently. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/03c1eabbcbc6
Include window state in window rect. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/e194d3ec9c1a
Fix window maximised assertions. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f17acd4a5bbb
Remove unused import from test_window_maximize.py. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/d72b664302bc
Compare window rect, not window size. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/2e2a7ad88909
Allow assert_success to be used without an expected value. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/e013a69621be
Make client.Session#maximize a method. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ea49436af9d9
Skip resetting namespaced interfaces in client.Session. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/66ce6c8cd053
Rename WaitCondition to Condition. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/4f1c4c9fd451
Introduce TimedPromise. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ec8c6742603a
Ensure Maximize Window is synchronous. r=automatedtester
Blocks: 1380310
Comment on attachment 8892537 [details]
Bug 1381876 - Introduce TimedPromise.

https://reviewboard.mozilla.org/r/163534/#review178558

::: testing/marionette/wait.js:147
(Diff revision 7)
> + *     instead resolved on timeout.
> + *
> + * @return {Promise.<*>}
> + *     Timed promise.
> + */
> +function TimedPromise(fn, {timeout = 1500, throws = TimeoutError} = {}) {

Sorry to come back to this that late, but what is the reason to use a different timeout here compared to wait. It would have made more sense to really use the same duration.
Comment on attachment 8892538 [details]
Bug 1381876 - Ensure Maximize Window is synchronous.

https://reviewboard.mozilla.org/r/163536/#review178566

::: testing/marionette/driver.js:2982
(Diff revision 7)
>      if (win.windowState == win.STATE_MAXIMIZED) {
>        win.restore();
>      } else {
>        win.maximize();
>      }
> -  });
> +  }, {throws: null});

I feel there is a case when a slowly changing window state can fail... Or I read this wrong?

Why do we ignore the reject case case here and don't do anything in case the TimedPromise times out? By default this is 1.5s, and especially for debug builds I could imaging that window size changes can take a while and exceed this duration.

When will there be cases when we don't expect a `sizemodechange` to happen?
(In reply to Henrik Skupin (:whimboo) from comment #94)

> ::: testing/marionette/wait.js:147 (Diff revision 7)
> 
> > + * instead resolved on timeout. * * @return {Promise.<*>} *
> > +Timed promise. */ function TimedPromise(fn, {timeout = 1500,
> > +throws = TimeoutError} = {}) {
> 
> Sorry to come back to this that late, but what is the reason to
> use a different timeout here compared to wait. It would have made
> more sense to really use the same duration.

They are two distinct computing ideas.

(In reply to Henrik Skupin (:whimboo) from comment #95)

> ::: testing/marionette/driver.js:2982
> (Diff revision 7)
> >      if (win.windowState == win.STATE_MAXIMIZED) {
> >        win.restore();
> >      } else {
> >        win.maximize();
> >      }
> > -  });
> > +  }, {throws: null});
> 
> I feel there is a case when a slowly changing window state can
> fail... Or I read this wrong?
> 
> Why do we ignore the reject case case here and don't do anything
> in case the TimedPromise times out? By default this is 1.5s, and
> especially for debug builds I could imaging that window size
> changes can take a while and exceed this duration.
> 
> When will there be cases when we don't expect a `sizemodechange`
> to happen?

This is already described in detail in the commit message, but we
don’t expect a sizemodechange on Linux when the window manager
doesn’t support the concept of window maximisation.  In those
cases we time out and hope we have given the window enough time to
transition into whatever state the WM has commanded.
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: