Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

While working on bug 1332122 I noticed that the message for "Marionette:listenersAttached" doesn't contain the JSON version of the capabilities object and as such we fail to retrieve the current capabilities from inside the listener script. Which means that capabilities set for newSession are getting all reset:

https://dxr.mozilla.org/mozilla-central/rev/e6a7a778ba132b87f346a5458b0879c45a3061b7/testing/marionette/driver.js#3186-3194

Maybe this might also cause bug 1364245, but hard to say without any debug output for tests on Android.

We should get this fixed for all currently supported versions.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 7 years ago

The last patch missed the @run_if decorator because the test can only be run if remoteness changes.

	David Burns :automatedtester
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8883379 [details]
Bug 1378191 - Use JSON to send capabilities in "Marionette:listenersAttached"

https://reviewboard.mozilla.org/r/154260/#review159364

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py
(Diff revision 2)
>  class TestPageLoadStrategy(BaseNavigationTestCase):
>  
> -    def setUp(self):
> +    def tearDown(self):
> -        super(TestPageLoadStrategy, self).setUp()
> -
> -        if self.marionette.session is not None:

By removing the setup you have duplicated code in the tests. Why have you done this?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 7 years ago
mozreview-review-reply

Comment on attachment 8883379 [details]
Bug 1378191 - Use JSON to send capabilities in "Marionette:listenersAttached"

https://reviewboard.mozilla.org/r/154260/#review159364

> By removing the setup you have duplicated code in the tests. Why have you done this?

If there is a run_if decorator as we have here, a more complex tearDown() method would have to be added to ensure that we start a session again, because it got deleted in setUp. So I find that better this way.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 7 years ago
mozreview-review-reply

Comment on attachment 8883379 [details]
Bug 1378191 - Use JSON to send capabilities in "Marionette:listenersAttached"

https://reviewboard.mozilla.org/r/154260/#review159364

> If there is a run_if decorator as we have here, a more complex tearDown() method would have to be added to ensure that we start a session again, because it got deleted in setUp. So I find that better this way.

And what's more important to note here is that if we delete the session already in `setUp` the skip decorator will fail with an invalid session exception. It means I would have to update all the decorators to handle a `None` session, and to create one themselves. Let me know if that is what you want to have, but I would better move this to a new bug.

	David Burns :automatedtester
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8883379 [details]
Bug 1378191 - Use JSON to send capabilities in "Marionette:listenersAttached"

https://reviewboard.mozilla.org/r/154260/#review159550

	Pulsebot
	Comment 8 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67cc62be4759
Use JSON to send capabilities in "Marionette:listenersAttached" r=automatedtester

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 9 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/67cc62be4759

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 7 years ago

Test-only patch which fixes a major issue with session capabilities. Please uplift to both beta, and esr52. Thanks.

	Carsten Book [:Tomcat]
	Comment 11 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/82893ea5d603

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 7 years ago

Needs a rebased patch for ESR52.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 7 years ago

An uplift would depend on a couple of other uplifts including new features. Given that it's basically the page load strategy which suffers from this bug and which got landed for Firefox 54, I don't see actually a reason for an uplift to esr52.

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago

This is an aside, but generally I think we need to find a better
way to communicate capabilities configuration to the content frame
script.  We have discussed problems with maintaining state in the
past.

We could circumvent problems like that by providing the
necessary data as part of the listener IPC message or by
introducing a synchronous capabilities query service.  I filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1379490 to track this.