Capabilities from geckodriver are not recognised since bug 1387380

RESOLVED FIXED in Firefox 57

Status

Testing
Marionette
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks: 1 bug, {regression})

Version 3
mozilla57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

MozReview Requests

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

Attachments

(3 attachments)

(Assignee)

Description

5 months ago
Since https://bugzilla.mozilla.org/show_bug.cgi?id=1387380 capabilities received from geckodriver are not recognised as can be seen from https://gist.github.com/barancev/bf99c3d49045025dbb02763a055dfe59#file-gistfile1-txt-L216-L218:

> 1502207295956	geckodriver::marionette	TRACE	-> 215:[0,1,"newSession",{"acceptInsecureCerts":true,"browserName":"firefox","capabilities":{"desiredCapabilities":{"acceptInsecureCerts":true,"browserName":"firefox","pageLoadStrategy":"none"}},"pageLoadStrategy":"none"}]
> 1502207295958	Marionette	TRACE	0 -> [0,1,"newSession",{"acceptInsecureCerts":true,"browserName":"firefox","capabilities":{"desiredCapabilities":{"acceptInsecureCerts":true,"browserName":"firefox","pageLoadStrategy":"none"}},"pageLoadStrategy":"none"}]
> 1502207295989	Marionette	DEBUG	Register listener.js for window 4294967297
> 1502207296010	Marionette	TRACE	0 <- [1,1,null,{"sessionId":"dd9b56a9-5e0f-4e74-a752-330fe0719d3e","capabilities":{"browserName":"firefox","browserVersion":"57.0a1","platformName":"windows_nt","platformVersion":"6.1","pageLoadStrategy":"normal","acceptInsecureCerts":false,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000},"rotatable":false,"specificationLevel":0,"moz:processID":2756,"moz:profile":"C:\\Users\\alexei\\AppData\\Local\\Temp\\rust_mozprofile.j8M8BAyUlCNn","moz:accessibilityChecks":false,"moz:headless":false}}]
> 1502207296011 geckodriver::marionette TRACE <- [1,1,null,{"sessionId":"dd9b56a9-5e0f-4e74-a752-330fe0719d3e","capabilities":{"browserName":"firefox","browserVersion":"57.0a1","platformName":"windows_nt","platformVersion":"6.1","pageLoadStrategy":"normal","acceptInsecureCerts":false,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000},"rotatable":false,"specificationLevel":0,"moz:processID":2756,"moz:profile":"C:\\Users\\alexei\\AppData\\Local\\Temp\\rust_mozprofile.j8M8BAyUlCNn","moz:accessibilityChecks":false,"moz:headless":false}}]

geckodriver sends the capabilities at the top-level of the command parameters element as well as in the backwards compat field:

> [0,1,"newSession",{"pageLoadStrategy": "none", "capabilities": {"desiredCapabilities": {"pageLoadStrategy": "none"}}, …}]

But since bug 1387380, Marionette only reads the "capabilities" field off this JSON Object:

>     this.capabilities = session.Capabilities.fromJSON(
>         cmd.parameters.capabilities);

This is a bug and it should read cmd.parameters instead.

Unfortunately, Marionette unit tests expect {sessionId: <old session ID>, capabilities: <dict>} to be the data format.  I think we should remove the ability to override the session ID.  As far as I can tell, this functionality is only used for in-app restart tests, but it’s arguably not the same Marionette session when Firefox has restarted.
(Assignee)

Updated

5 months ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
(Assignee)

Updated

5 months ago
Depends on: 1387380
Comment hidden (mozreview-request)
(In reply to Andreas Tolfsen ‹:ato› from comment #0)
> ability to override the session ID.  As far as I can tell, this
> functionality is only used for in-app restart tests, but it’s arguably not
> the same Marionette session when Firefox has restarted.

I'm happy to get this removed.

Comment 3

5 months ago
mozreview-review
Comment on attachment 8894975 [details]
Bug 1388424 - Read capabilities off top-level object.

https://reviewboard.mozilla.org/r/166086/#review171294
Attachment #8894975 - Flags: review?(hskupin) → review+
status-firefox56: --- → unaffected
status-firefox57: --- → affected
Keywords: regression
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

5 months ago
Slight update because I was copying a reference, causing circular references in the Python dictionaries.

Updated

5 months ago
status-firefox57: affected → fix-optional
I assume this is done and we can land. It blocks my work, so I pushed it to autoland.

Comment 8

5 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2cf4290c2eef
Read capabilities off top-level object. r=whimboo
Backed out for failing marionette's test_quit_restart.py TestQuitRestart.test_force_clean_restart:

https://hg.mozilla.org/integration/autoland/rev/d3cb33a38211dfb131958fec2a52cb27bf4abc49

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2cf4290c2eeff7dd6f3c6c21e8393cb3670ca9b3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122516677&repo=autoland

[task 2017-08-11T09:45:33.397142Z] 09:45:33     INFO -  1502444733377	Marionette	TRACE	2 -> [0,6,"setContext",{"value":"content"}]
[task 2017-08-11T09:45:33.397426Z] 09:45:33     INFO -  1502444733378	Marionette	TRACE	2 <- [1,6,null,{}]
[task 2017-08-11T09:45:33.397817Z] 09:45:33     INFO -  1502444733379	Marionette	TRACE	2 -> [0,7,"getContext",{}]
[task 2017-08-11T09:45:33.398346Z] 09:45:33     INFO -  1502444733380	Marionette	TRACE	2 <- [1,7,null,{"value":"content"}]
[task 2017-08-11T09:45:33.404980Z] 09:45:33     INFO -  1502444733381	Marionette	TRACE	2 -> [0,8,"setContext",{"value":"content"}]
[task 2017-08-11T09:45:33.405211Z] 09:45:33     INFO -  1502444733382	Marionette	TRACE	2 <- [1,8,null,{}]
[task 2017-08-11T09:45:33.405580Z] 09:45:33     INFO -  1502444733384	Marionette	TRACE	2 -> [0,9,"getPageSource",{}]
[task 2017-08-11T09:45:33.406367Z] 09:45:33     INFO -  1502444733396	Marionette	TRACE	2 <- [1,9,null,{"value":"<html><head></head><body></body></html>"}]
[task 2017-08-11T09:45:33.407403Z] 09:45:33     INFO -  1502444733400	Marionette	TRACE	2 -> [0,10,"setContext",{"value":"content"}]
[task 2017-08-11T09:45:33.419312Z] 09:45:33     INFO - TEST-UNEXPECTED-FAIL | test_quit_restart.py TestQuitRestart.test_force_clean_restart | AssertionError: u'dcdb0ce1-7513-46bc-bb22-88fd6ac65220' != u'46f9a717-7220-4b38-9b3c-98515dc561f5'
[task 2017-08-11T09:45:33.419554Z] 09:45:33     INFO - - dcdb0ce1-7513-46bc-bb22-88fd6ac65220
[task 2017-08-11T09:45:33.419644Z] 09:45:33     INFO - + 46f9a717-7220-4b38-9b3c-98515dc561f5
[task 2017-08-11T09:45:33.419723Z] 09:45:33     INFO - Traceback (most recent call last):
[task 2017-08-11T09:45:33.420061Z] 09:45:33     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 156, in run
[task 2017-08-11T09:45:33.420216Z] 09:45:33     INFO -     testMethod()
[task 2017-08-11T09:45:33.420365Z] 09:45:33     INFO -   File "/home/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py", line 105, in test_force_clean_restart
[task 2017-08-11T09:45:33.421258Z] 09:45:33     INFO -     self.assertEqual(self.marionette.session_id, self.session_id)
Flags: needinfo?(ato)
The problem here is that the patch missed to update both of the forced restart tests which are failing now because the session id is reset in between:

test_quit_restart.py test_quit_restart.TestQuitRestart.test_force_clean_restart
test_quit_restart.py test_quit_restart.TestQuitRestart.test_force_restart

I will fix it locally and upload a patch which we can then land via inbound.
Flags: needinfo?(ato)
Created attachment 8896206 [details] [diff] [review]
Updated patch with tests fixed
I triggered a try push of the latest version of the patch together with my changes on bug 1387092:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08bd74be88a2
The try build is green. Can someone please land attachment 8896206 [details] [diff] [review] on inbound? Thanks.
Keywords: checkin-needed

Comment 14

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/923bb6366e72
Read capabilities off top-level object. r=whimboo
Keywords: checkin-needed
Created attachment 8896433 [details] [diff] [review]
follow-up patch for flake8 failure
(Assignee)

Comment 17

5 months ago
Thanks for picking this up whilst I’ve been away, Henrik.
https://hg.mozilla.org/mozilla-central/rev/923bb6366e72
https://hg.mozilla.org/mozilla-central/rev/fce0b08bf07c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox57: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1387380
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
No longer depends on: 1387380
Duplicate of this bug: 1333718
Blocks: 1389103
You need to log in before you can comment on or make changes to this bug.