Closed Bug 1342162 Opened 7 years ago Closed 7 years ago

Align page load duration key with spec

Categories

(Remote Protocol :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

The WebDriver specification recently changed to use the key "pageLoad" instead of "page load" in the session timeout configuration object.  This makes Marionette out of sync with the specification.

Fortunately, the current format we used landed in https://bugzilla.mozilla.org/show_bug.cgi?id=1316622 so if we patch this quickly and uplift the patch to Aurora (53) and Beta (52) we could avoid the backwards compatibility dance on this.
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Comment on attachment 8840527 [details]
Bug 1342162 - Align pageLoad key with WebDriver;

https://reviewboard.mozilla.org/r/115014/#review116732

::: testing/marionette/client/marionette_driver/timeout.py:66
(Diff revision 1)
>          """Get the session's page load timeout.  This specifies the time
>          to wait for the page loading to complete.  It is by default 5
>          minutes (or 300 seconds).
>  
>          """
> -        return self._get("page load")
> +        return self._get("pageLoad")

Judging by the try run, some tests need to be updated, too. e.g. https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py#38
Attachment #8840527 - Flags: review?(mjzffr) → review-
Comment on attachment 8840527 [details]
Bug 1342162 - Align pageLoad key with WebDriver;

https://reviewboard.mozilla.org/r/115014/#review117194
Attachment #8840527 - Flags: review?(mjzffr) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01bc4c9ad3b5
Align pageLoad key with WebDriver; r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/01bc4c9ad3b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Sheriffs: Please uplift to Aurora and Beta under test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
It's too late for beta given that the merge to release has already happened. You can try esr52 if needed later.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-aurora]
According to http://whattrainisitnow.com/, the next uplift is scheduled to happen 6 March?
In any case, this needs to be uplifted to 52, lest we want to provide backwards compatibility fixes to the Marionette Python client.
(In reply to Andreas Tolfsen from comment #10)
> According to http://whattrainisitnow.com/, the next uplift is scheduled to
> happen 6 March?

Beta -> Release merge is always done a complete week before all the other branches are getting merged. This is to produce the RC builds and to be ready for a release on Merge Day + 1.

The only realistic chance to get it in for 52.0 release is if another RC build has to be build and this patch gets landed before. But I doubt that it is getting accepted. So I will set the landing flag for esr52 instead.
Whiteboard: [checkin-needed-aurora] → [checkin-needed-aurora][checkin-needed-esr52]
So we have bug 1343520 for update tests now. Given that we cannot land this patch for 52.0 release anymore, I request a backout of this patch given that we definitely need backward compatible code. This will apply to releases for Firefox 52, 53, 54, and 55.
Status: RESOLVED → REOPENED
Flags: needinfo?(ato)
Resolution: FIXED → ---
Whiteboard: [checkin-needed-aurora][checkin-needed-esr52]
It’s unfortunate we were unable to uplift this to beta, which I understand is causing these issues because it now tries to use a Python client which is incompatible with the server to do upgrade tests.  This is partly my fault because I didn’t realise beta was closed for uplifts a week before merge day.

QA tests upgrade from 52b (last commit before freeze) → 52rcN (where N is latest RC), meaning an uplift of this to latest 52rc2 wouldn’t be of much help, since the client in 52b would be incompatible with the server API in 52rc2.

This inevitably means we need to work around the issue by providing some backwards compatibility in the client so that it supports both the new ("pageLoad") and the old ("page load") key.  I don’t think this is a big job, but it will probably be rather ugly.  Normally unknown keys in JSON Objects are ignored, but the old implementation iterated over entries and threw on an unknown key: https://hg.mozilla.org/mozilla-central/file/FIREFOX_AURORA_52_BASE/testing/marionette/driver.js#l1572
Sheriffs, can one of you please backout the patch from central until we figured out a good solution for backward compatibility? Thanks.
Flags: needinfo?(sheriffs)
It looks like People are having issues now with Firefox 52.0.1 when using Selenium 3.3.1 and GeckoDriver 0.15.0. Does Selenium or GeckoDriver force `pageLoad` now? We don't have that in Marionette yet.

Failure report:
---------------
https://www.hskupin.info/2017/01/23/using-selenium-and-webdriver-to-interact-with-insecure-ssl-pages-in-firefox/#comment-69772

Stack looks like:
-----------------
File “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”, line 727, in set_page_load_timeout
‘pageLoad’: int(float(time_to_wait) * 1000)})
File “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”, line 238, in execute
self.error_handler.check_response(response)
Flags: needinfo?(dburns)
(In reply to Henrik Skupin (:whimboo) from comment #18)
> It looks like People are having issues now with Firefox 52.0.1 when using
> Selenium 3.3.1 and GeckoDriver 0.15.0. Does Selenium or GeckoDriver force
> `pageLoad` now? We don't have that in Marionette yet.
> 
> Failure report:
> ---------------
> https://www.hskupin.info/2017/01/23/using-selenium-and-webdriver-to-interact-
> with-insecure-ssl-pages-in-firefox/#comment-69772
> 
> Stack looks like:
> -----------------
> File
> “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/
> env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”,
> line 727, in set_page_load_timeout
> ‘pageLoad’: int(float(time_to_wait) * 1000)})
> File
> “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/
> env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”,
> line 238, in execute
> self.error_handler.check_response(response)

This is a selenium bug and the people need to raise a bug with Selenium to get it fixed. It has been fixed already in the python client but not have been released.
Flags: needinfo?(dburns)
I have now added a second patch to make the changes to the session timeout duration object used in the Python client backwards compatible with earlier Firefoxen, which should address the concerns that caused this to be backed out.
Flags: needinfo?(ato)
Comment on attachment 8850581 [details]
Bug 1342162 - Make timeout API in client backwards compatible;

https://reviewboard.mozilla.org/r/123126/#review125658

Nice solution.

::: testing/marionette/client/marionette_driver/timeout.py:45
(Diff revision 1)
>              self._marionette._send_message("timeouts", {"type": name, "ms": ms})
>  
>      def _get(self, name):
> -        ms = self._marionette._send_message("getTimeouts", key=name)
> +        ts = self._marionette._send_message("getTimeouts")
> +        if name not in ts:
> +            raise UnknownKey()

Why not throwing the internal KeyError? It would save us from having to define an extra exception class, for a single usage.

::: testing/marionette/client/marionette_driver/timeout.py:73
(Diff revision 1)
>          """Get the session's page load timeout.  This specifies the time
>          to wait for the page loading to complete.  It is by default 5
>          minutes (or 300 seconds).
>  
>          """
> +        # remove fallback when Firefox 55 is stable

This only applies if we can get it into 52 release. Otherwise it will be 56.

::: testing/marionette/client/marionette_driver/timeout.py:85
(Diff revision 1)
>      def page_load(self, sec):
>          """Set the session's page load timeout.  This specifies the time
>          to wait for the page loading to complete.
>  
>          """
> +        # remove fallback when Firefox 55 is stable

Same as above.
Attachment #8850581 - Flags: review?(hskupin) → review+
Comment on attachment 8850581 [details]
Bug 1342162 - Make timeout API in client backwards compatible;

https://reviewboard.mozilla.org/r/123126/#review125658

> Why not throwing the internal KeyError? It would save us from having to define an extra exception class, for a single usage.

Good idea.

> This only applies if we can get it into 52 release. Otherwise it will be 56.

OK, addressed.

> Same as above.

And done.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84948ee2d8ff
Align pageLoad key with WebDriver; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/a6380012133b
Make timeout API in client backwards compatible; r=whimboo
https://hg.mozilla.org/mozilla-central/rev/84948ee2d8ff
https://hg.mozilla.org/mozilla-central/rev/a6380012133b
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Sheriffs: Please uplift to Aurora and Beta as test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-aurora/rev/90b35818bd83
https://hg.mozilla.org/releases/mozilla-aurora/rev/fb88f196fe8a
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
Did this not make it into 52 ESR?
It did not.
Andreas, I'm not sure what this exactly is but for the firefox-ui tests on the release candidate builds I see complete bustage due to those changes:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&revision=d345b657d381ade5195f1521313ac651618f54a2&filter-searchStr=fxfn%20linux&filter-tier=1&filter-tier=2&filter-tier=3

> TEST-UNEXPECTED-ERROR | test_appinfo.py TestAppInfo.test_invalid_properties | InvalidArgumentException: Unrecognised timeout: page load

Is that a KeyError vs InvalidArgumentError issue when catching for the fallback?

https://dxr.mozilla.org/mozilla-central/rev/20dff607fb88ee69135a280bbb7f32df75a86237/testing/marionette/client/marionette_driver/timeout.py#63

Btw this was never present on beta, and also for commit based tests via Taskcluster it works, but not for the final builds.
Flags: needinfo?(ato)
Oh wait. I had a look at the mozmill-ci job and have seen that we made use of an older test package which would totally explain this misbehavior.

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-release-linux/1491732920/firefox-52.0.3.en-US.linux-i686.test_packages.json

So that should be a problem on our side. Sorry for the noise.
Flags: needinfo?(ato)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.