Closed Bug 1378191 Opened 2 years ago Closed 2 years ago

Async message for "Marionette:listenersAttached" has to send a JSON object for capabilities

Categories

(Testing :: Marionette, enhancement, P1)

52 Branch
enhancement

Tracking

(firefox-esr52 wontfix, firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [spec][17/07])

Attachments

(1 file)

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.
The last patch missed the @run_if decorator because the test can only be run if remoteness changes.
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?
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.
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.
Comment on attachment 8883379 [details]
Bug 1378191 - Use JSON to send capabilities in "Marionette:listenersAttached"

https://reviewboard.mozilla.org/r/154260/#review159550
Attachment #8883379 - Flags: review?(dburns) → review+
Priority: -- → P1
Whiteboard: [spec][17/07]
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67cc62be4759
Use JSON to send capabilities in "Marionette:listenersAttached" r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/67cc62be4759
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Test-only patch which fixes a major issue with session capabilities. Please uplift to both beta, and esr52. Thanks.
Whiteboard: [spec][17/07] → [checkin-needed-beta][checkin-needed-esr52]
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [spec][17/07][checkin-needed-beta][checkin-needed-esr52]
https://hg.mozilla.org/releases/mozilla-beta/rev/82893ea5d603
Whiteboard: [spec][17/07][checkin-needed-beta][checkin-needed-esr52] → [spec][17/07][checkin-needed-esr52]
Needs a rebased patch for ESR52.
Flags: needinfo?(hskupin)
Whiteboard: [spec][17/07][checkin-needed-esr52] → [spec][17/07]
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.
Flags: needinfo?(hskupin)
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.
You need to log in before you can comment on or make changes to this bug.