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

Right now we have a default socket timeout of 60s. This works totally fine for not long running commands. But it can embarrassing fail for commands like get() or execute_(async_)script which can take longer. Default timeouts there are 300s and 30s.

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/marionette/driver.js#124

We should also consider that users can bump up those values even more.

To not run into socket errors before such long running commands would timeout themselves, we should consider to:

a) Dynamically adjust the socket timeout to the underlying command's timeout value. Eg. if the page load timeout is set to 300s, adjust the socket timeout to 305s.

b) Integrate a heartbeat which checks in regular intervals for an existing and working connection. 

I think b) is hard with the synchronized communication between client and server. So the only way seems to be option a).

Andreas and David, mind telling your opinions? It is actually a critical issue given that we can see a lot of killed Firefox processes lately due to socket timeouts in get().

To note the original value of 360s for the socket timeout might have been related to default page load timeout. I dropped this value to 60s on bug 1284457 which landed in Firefox 49. As such we should backport a patch on this bug down to 51.

	David Burns :automatedtester
	Comment 1 • 8 years ago

What is the cause of the pages taking so long to load? A page taking 60s+ for DOMContentLoaded to fire is a huge problem and we should be fixing that issue.

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

In that case it's an invalid proxy setting because at the moment we use the public IP to serve locally hosted data instead of localhost. See bug 1322199 and bug 1320643.

But situations like those can happen at any time and as long as a command takes longer to answer as the set socket timeout, we would always run into this issue. Keep in mind that the proposed default timeout for page load is 300s!

	David Burns :automatedtester
	Comment 3 • 8 years ago

(In reply to Henrik Skupin (:whimboo) from comment #2)
> In that case it's an invalid proxy setting because at the moment we use the
> public IP to serve locally hosted data instead of localhost. See bug 1322199
> and bug 1320643.

Right, those are problems that need solving. Trying to "hide" the issue with dynamic timeouts is silly and just going to hide other issues.

> 
> But situations like those can happen at any time and as long as a command
> takes longer to answer as the set socket timeout, we would always run into
> this issue. Keep in mind that the proposed default timeout for page load is
> 300s!

Sure, and if they happen we need to figure out the root cause instead of hiding the issue. Unless you are going to use this bug to solve the underlying issue I suggest you close this bug as resolved/won't fix.

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

I think you misunderstand this bug. The problem here is the socket communication between the client and the server. If page load timeouts of 300s are allowed we cannot raise an IOError after 60s because no response has come back yet. The real failure which should appear here is a TimeoutError for get(). See the list of blocked bugs how it currently looks like.

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

I would still like to hear Andreas feedback whose ni? flag got removed.

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

I'm also not sure if the rename of the summary is something we can do. If I understand David correctly he wants to transform a IOError exception into a TimeoutException in case of commands which have longer timeouts set? I don't think that this will work, and we will miss important information which is delivered for this kind of exception from Marionette server.

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

(In reply to Henrik Skupin (:whimboo) from comment #0)
> b) Integrate a heartbeat which checks in regular intervals for an existing
> and working connection. 
> 
> I think b) is hard with the synchronized communication between client and
> server. So the only way seems to be option a).

The synchronisation is only enforced client side.  The Marionette server, by definition, is asynchronous.  You should be able to send pings to the server whilst it is processing another command.

> Andreas and David, mind telling your opinions? It is actually a critical
> issue given that we can see a lot of killed Firefox processes lately due to
> socket timeouts in get().

I personally think the 300s default timeout in WebDriver is too high.  We can lower this to 60s like you did earlier by sending it as a capability when we start the session or by using the Set Timeouts command.

If the user sets an unrealistic page load timeout such as 1000s, as I understand it the Python client socket will time out at 360s?  Is there a chance we can set this to None (infinite)?

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

(In reply to Andreas Tolfsen ‹:ato› from comment #7)
> > I think b) is hard with the synchronized communication between client and
> > server. So the only way seems to be option a).
> 
> The synchronisation is only enforced client side.  The Marionette server, by
> definition, is asynchronous.  You should be able to send pings to the server
> whilst it is processing another command.

Ah, ok. So for the client we still want to have a blocking connection. Which means that we could use threads to handle the sending and receiving of data. `_send_message()` could be modified to spawn two threads. One for the real command, and another one for the heartbeat. With such a change we can set the timeout for the socket connection of the real command to undefined, and kill it if the heartbeat doesn't work anymore. It means we can keep the default 60 seconds, and we dot run into a timeout if any of the send commands return after 60s.

An option could be to always have the heartbeat thread running if a connection has been established. It would safe us the costs from creating it for each command. Something we would have to check how thread-safe our transport.py module is.

> I personally think the 300s default timeout in WebDriver is too high.  We
> can lower this to 60s like you did earlier by sending it as a capability
> when we start the session or by using the Set Timeouts command.

But this is enforced in the W3C spec:

https://w3c.github.io/webdriver/webdriver-spec.html#sessions

> A session has an associated session page load timeout that specifies a time to wait for the page loading to complete. Unless stated otherwise it is 300,000 milliseconds.

Shall I open an issue for it?

> If the user sets an unrealistic page load timeout such as 1000s, as I
> understand it the Python client socket will time out at 360s?  Is there a
> chance we can set this to None (infinite)?

The Python client actually sees a socket timeout after 60s and not 360s. This is since I landed the patch for bug 1284457. Regarding your None suggestion please see above.

	Andreas Tolfsen ❲:ato❳
	Comment 9 • 8 years ago

When I say that the 300s page loading timeout in WebDriver is too high, I mean that it is probably too high for _our tests_.  We can configure the page loading timeout to be whatever we want through a capability or by calling the Set Timeouts command.  We probably should configure it to be 60s as it was before.

I think a heartbeat thread is an overcomplicated solution seeing as the client and the server runs on the same machine.  My guess is that the best (and easiest) solution here is to move to using a ‘blocking socket’ described here: https://docs.python.org/3/library/socket.html#socket-timeouts

	Astley Chen (inactive)
	Comment 10 • 7 years ago

P2 since it blocks regression bug 1303346.

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

(In reply to Andreas Tolfsen ‹:ato› from comment #9)
> When I say that the 300s page loading timeout in WebDriver is too high, I
> mean that it is probably too high for _our tests_.  We can configure the
> page loading timeout to be whatever we want through a capability or by
> calling the Set Timeouts command.  We probably should configure it to be 60s
> as it was before.

So where would be the best place for doing so? When I understand you correctly we should do set it for MarionetteTestCase directly? With that only our harness gets this change, which should be fine since we handle the socket timeout. 

If we define it as 60s it would be equal to the 60s socket timeout and could cause races. So whether we decrease the page load timeout to 55s or bump up the socket timeout to 70s to also include the default shutdown timeout of 65s.

> I think a heartbeat thread is an overcomplicated solution seeing as the
> client and the server runs on the same machine.  My guess is that the best
> (and easiest) solution here is to move to using a ‘blocking socket’
> described here: https://docs.python.org/3/library/socket.html#socket-timeouts

The blocking mode also has a timeout parameter and causes the connection to fail in case of timeouts. So I don't think that this is helpful here. I would say that option 1) is indeed the best bet right now.

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

(In reply to Henrik Skupin (:whimboo) from comment #11)
> (In reply to Andreas Tolfsen ‹:ato› from comment #9)
> > When I say that the 300s page loading timeout in WebDriver is too high, I
> > mean that it is probably too high for _our tests_.  We can configure the
> > page loading timeout to be whatever we want through a capability or by
> > calling the Set Timeouts command.  We probably should configure it to be 60s
> > as it was before.
> 
> So where would be the best place for doing so? When I understand you
> correctly we should do set it for MarionetteTestCase directly? With that
> only our harness gets this change, which should be fine since we handle the
> socket timeout.

I would suggest configuring the timeout durations during session creation, if no other capabilities are given.  It’s not clear to me where in https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/harness/marionette_harness/runner/base.py Marionette.start_session() is actually called.

But the configuration would look like this:

    self.marionette.start_session({"requiredCapabilities": {"timeouts": {"page load": 60}}})

> If we define it as 60s it would be equal to the 60s socket timeout and could
> cause races. So whether we decrease the page load timeout to 55s or bump up
> the socket timeout to 70s to also include the default shutdown timeout of
> 65s.
>
> > I think a heartbeat thread is an overcomplicated solution seeing as the
> > client and the server runs on the same machine.  My guess is that the best
> > (and easiest) solution here is to move to using a ‘blocking socket’
> > described here: https://docs.python.org/3/library/socket.html#socket-timeouts
> 
> The blocking mode also has a timeout parameter and causes the connection to
> fail in case of timeouts. So I don't think that this is helpful here. I
> would say that option 1) is indeed the best bet right now.

I feel it’s wrong to have a socket timeout since many commands sent to Marionette may take longer than the preset value currently used in the Marionette client.  Examples of this is if the user configures the page load timeout to be above the predefined value, or a script takes longer than the default script timeout.

Test timeout could be handled at by the test harness or by mozharness.  I don’t see why we need to set the timeout for the socket specifically.

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

Ok, lets try with setting the timeouts during the session creation. I might have to check how tests which start a session themselves behave. 

As I noticed this bug is hiding a couple of hangs during page load for Fennec and causes the application to force shutdown.

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

While trying to implement this I'm facing a couple of issues which needs more thinking and additional work.

(In reply to Andreas Tolfsen ‹:ato› from comment #12)
>     self.marionette.start_session({"requiredCapabilities": {"timeouts":
> {"page load": 60}}})

This would actually set the page load timeout to 60ms. So if we want 60s we have to set 60000 here. Reason is that the conversion is done in the timeouts.py module, which is tied to self.timeouts but not start_session(). 

Another point is that we would have to obey if consumers are already setting the timeout themselves via requiredCapabilities or desiredCapabilities. I assume that the latter have lesser priority? Maybe we set the above via desiredCapabilities, so that tests can always overwrite the setting?

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

(In reply to Henrik Skupin (:whimboo) from comment #14)
> While trying to implement this I'm facing a couple of issues which needs
> more thinking and additional work.
> 
> (In reply to Andreas Tolfsen ‹:ato› from comment #12)
> >     self.marionette.start_session({"requiredCapabilities": {"timeouts":
> > {"page load": 60}}})
> 
> This would actually set the page load timeout to 60ms. So if we want 60s we
> have to set 60000 here. Reason is that the conversion is done in the
> timeouts.py module, which is tied to self.timeouts but not start_session(). 

Yes, sorry.  I meant 60000 as you said.

> Another point is that we would have to obey if consumers are already setting
> the timeout themselves via requiredCapabilities or desiredCapabilities. I
> assume that the latter have lesser priority? Maybe we set the above via
> desiredCapabilities, so that tests can always overwrite the setting?

If consumers provide an override we ought to respect that.  Using desiredCapabilities for our defaults is a clever, like you also said.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8829837 [details]
Bug 1322277 - Fix usage of capabilities in start_session.

https://reviewboard.mozilla.org/r/106822/#review107958

::: testing/marionette/client/marionette_driver/marionette.py:1281
(Diff revision 1)
>          # Call wait_for_port() before attempting to connect in
>          # the event gecko hasn't started yet.
>          self.wait_for_port(timeout=timeout)
>          self.protocol, _ = self.client.connect()
>  
> -        body = {"capabilities": desired_capabilities, "sessionId": session_id}
> +        body = {"capabilities": capabilities, "sessionId": session_id}

I think maybe we need the following somewhere here for correctness:

```python
if capabilities is None:
    capabilities = {}
```

Even though sending `None` appears to cause no issues.

	Andreas Tolfsen ❲:ato❳
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

::: testing/marionette/client/marionette_driver/marionette.py:1294
(Diff revision 1)
> +                }
> +            }}
> +        if capabilities is not None:
> +            caps.update(capabilities)
> +
> +        body = {"capabilities": caps, "sessionId": session_id}

We shouldn’t pass in `sessionId` if it is not provided.

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:36
(Diff revision 1)
> -        self.assertDictEqual(self.caps["timeouts"],
> +        self.assertEqual(self.caps["timeouts"],
> -                             {"implicit": 0,
> +                         {"implicit": 0,
> -                              "page load": 300000,
> +                          "page load": 60000,  # default for Marionette client only

Hm, it makes it difficult to test the Marionette (server) implementation for correctness when it always uses a client-set configuration.  I wonder if using `desiredCapabilities` is not such a good idea at all.

If we did not use it, we could imagine closing the session in this test’s `setUp` method and starting a new one with `marionette.start_session({})` to enforce the server defaults.

What are your thoughts?  I’m open to pursuation here.

	Andreas Tolfsen ❲:ato❳
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8829839 [details]
Bug 1322277 - Default socket timeout has to be larger than the default page load timeout.

https://reviewboard.mozilla.org/r/106826/#review107968

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

Comment on attachment 8829837 [details]
Bug 1322277 - Fix usage of capabilities in start_session.

https://reviewboard.mozilla.org/r/106822/#review107958

> I think maybe we need the following somewhere here for correctness:
> 
> ```python
> if capabilities is None:
>     capabilities = {}
> ```
> 
> Even though sending `None` appears to cause no issues.

We don't need that given that the next patch in this series is modifying the behavior anyway. So I kept this commit simple by only renaming the parameter.

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

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> We shouldn’t pass in `sessionId` if it is not provided.

The spec doesn't say anything that a sessionId should be passed in at all. Even not optionally. Should we remove that, or why has this been added for the client?

> Hm, it makes it difficult to test the Marionette (server) implementation for correctness when it always uses a client-set configuration.  I wonder if using `desiredCapabilities` is not such a good idea at all.
> 
> If we did not use it, we could imagine closing the session in this test’s `setUp` method and starting a new one with `marionette.start_session({})` to enforce the server defaults.
> 
> What are your thoughts?  I’m open to pursuation here.

Using an empty dict here wouldn't help, given that updating the previously initiated dict is a no-op. Another way I could see here is that we could bind setting the page load timeout to the MarionetteTestCase. In that case only tests using the harness would change the value, and other like WPT won't. I think this should be even a better method.

With that the above change shouldn't be necessary.

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 7 years ago
mozreview-review-reply

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> The spec doesn't say anything that a sessionId should be passed in at all. Even not optionally. Should we remove that, or why has this been added for the client?

Good point.  If it’s not in the spec, remove it.

I think the intention here was to have the ability to re-use an existing session.  That _might_ be a valid use case when using Marionette directly since Marionette does not itself shut down the browser process (I’m ignoring the Firefox UI tests for simplicity), and every time a new session is started with geckodriver you get a new browser and a new session ID.

	Andreas Tolfsen ❲:ato❳
	Comment 25 • 7 years ago
mozreview-review-reply

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> Using an empty dict here wouldn't help, given that updating the previously initiated dict is a no-op. Another way I could see here is that we could bind setting the page load timeout to the MarionetteTestCase. In that case only tests using the harness would change the value, and other like WPT won't. I think this should be even a better method.
> 
> With that the above change shouldn't be necessary.

Maybe you could pass `{"desiredCapabilities": {}, "requiredCapabilities": {}}` to `start_session` in the setup so that `start_session` does not override it with its timeout configuration?

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

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> Good point.  If it’s not in the spec, remove it.
> 
> I think the intention here was to have the ability to re-use an existing session.  That _might_ be a valid use case when using Marionette directly since Marionette does not itself shut down the browser process (I’m ignoring the Firefox UI tests for simplicity), and every time a new session is started with geckodriver you get a new browser and a new session ID.

Oh, I checked the current Marionette client code and actually see that it is in use for us for at least restart(). So we should leave this option in, also we might need it for Geckodriver in the future when it should support restarts of Firefox.

> Maybe you could pass `{"desiredCapabilities": {}, "requiredCapabilities": {}}` to `start_session` in the setup so that `start_session` does not override it with its timeout configuration?

I would have to change the current logic. Because start_session() currently initializes the capability dict, and updates it with whatever is passed in. It means once the timeout is set an update for that sub-dict is a no-op if empty. I will have to think about.

	Andreas Tolfsen ❲:ato❳
	Comment 27 • 7 years ago
mozreview-review-reply

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> Oh, I checked the current Marionette client code and actually see that it is in use for us for at least restart(). So we should leave this option in, also we might need it for Geckodriver in the future when it should support restarts of Firefox.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1333718 about this.  Please comment if you have a use case for maintaining the session ID across multiple sessions.

The conclusion here is that we will keep `sessionId` for the time being.

	Andreas Tolfsen ❲:ato❳
	Comment 28 • 7 years ago
mozreview-review-reply

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> I would have to change the current logic. Because start_session() currently initializes the capability dict, and updates it with whatever is passed in. It means once the timeout is set an update for that sub-dict is a no-op if empty. I will have to think about.

I think if you explicitly pass in `desiredCapabilities` it shouldn’t try to override that.  We could change it to something along the lines of:

```python
if "desiredCapabilities" not in caps:
    caps.update({"desiredCapabilities": {"timeouts": …}})
```

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

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> I think if you explicitly pass in `desiredCapabilities` it shouldn’t try to override that.  We could change it to something along the lines of:
> 
> ```python
> if "desiredCapabilities" not in caps:
>     caps.update({"desiredCapabilities": {"timeouts": …}})
> ```

That would be not enough. We have to do the above check AND also if timeouts for page load is present. So overall three checks. Only if page load is not present we should replace it. With the checks above we will have the same end-results as when directly updating caps with the capabilities parameter value.

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

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> That would be not enough. We have to do the above check AND also if timeouts for page load is present. So overall three checks. Only if page load is not present we should replace it. With the checks above we will have the same end-results as when directly updating caps with the capabilities parameter value.

Which means whatever is passed in as capabilities["desiredCapabilities"] will be accounted with higher priority compared to the defaults (like 60s page load timeout) as currently set.

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

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> Which means whatever is passed in as capabilities["desiredCapabilities"] will be accounted with higher priority compared to the defaults (like 60s page load timeout) as currently set.

Ok, i see now what you mean with your last comment. You want to ignore any custom defaults for the client if desiredCapabilities have been specified, even if they do not contain timeouts. But keep in mind that this will cause inconsistencies if requiredCapabilities are specified. Well, right now those won't be visible because we only set a single default, but once this changes this logic won't work anymore.

	Andreas Tolfsen ❲:ato❳
	Comment 32 • 7 years ago
mozreview-review-reply

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> Ok, i see now what you mean with your last comment. You want to ignore any custom defaults for the client if desiredCapabilities have been specified, even if they do not contain timeouts. But keep in mind that this will cause inconsistencies if requiredCapabilities are specified. Well, right now those won't be visible because we only set a single default, but once this changes this logic won't work anymore.

The effect we want to achieve is _iff_ capabilities are passed to `start_session`, to not have them overridden with defaults so that we can use the Marionette client to test the server.

I would expect:

- `start_session({"desiredCapabilities": {})` to pass `{}` to the server
- `start_session()` to pass whatever sensible defaults the client finds appropriate to the server

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

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> The effect we want to achieve is _iff_ capabilities are passed to `start_session`, to not have them overridden with defaults so that we can use the Marionette client to test the server.
> 
> I would expect:
> 
> - `start_session({"desiredCapabilities": {})` to pass `{}` to the server
> - `start_session()` to pass whatever sensible defaults the client finds appropriate to the server

Ok, the upcoming change will fix this issue. Please have a look.

	Andreas Tolfsen ❲:ato❳
	Comment 36 • 7 years ago
mozreview-review-reply

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> Ok, the upcoming change will fix this issue. Please have a look.

r+ on the latest revision.

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

Comment on attachment 8829838 [details]
Bug 1322277 - Marionette client should be able to set default capabilities.

https://reviewboard.mozilla.org/r/106824/#review107962

> r+ on the latest revision.

And the try build is green. So I'm going to push this now. Thanks for your help, Andreas!

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

Pulse bot is not working. So this got landed on autoland as:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4dcb658426570956f5551e0cbba883b088d3a496

	Pulsebot
	Comment 39 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55cc36b0d408
Fix usage of capabilities in start_session r=ato
https://hg.mozilla.org/integration/autoland/rev/35d9f271da8d
Marionette client should be able to set default capabilities. r=ato

	Phil Ringnalda (:philor)
	Comment 40 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/55cc36b0d408
https://hg.mozilla.org/mozilla-central/rev/35d9f271da8d
https://hg.mozilla.org/mozilla-central/rev/4dcb65842657

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

Please get this test-only patch uplifted to both aurora and beta. Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c33db038437
https://hg.mozilla.org/releases/mozilla-aurora/rev/c39891c2a2f6
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1949e439f0e

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/6f4c97200bb7
https://hg.mozilla.org/releases/mozilla-beta/rev/96e2a1ec523d
https://hg.mozilla.org/releases/mozilla-beta/rev/7ce8d1a5678c

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

As investigated and then discussed on bug 1334149 the solution we have taken here was sub-optimal and as such I want to request a backout from all branches this patch got landed on.

If there is no simple backout possible, I could do a follow-up patch on the other bug for the newly and wanted behavior.

I discussed with Ryan on IRC, and he could have a look at this later. Thanks!

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

Also setting ni? for Carsten in case he finds the time earlier on Monday. Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 7 years ago
backout

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d763f7c2d4ca95bb9f59d73091e075528b8373e

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 7 years ago
backout

https://hg.mozilla.org/releases/mozilla-aurora/rev/53e94ef6ec0c
https://hg.mozilla.org/releases/mozilla-beta/rev/2e4f262fdaa6

	Carsten Book [:Tomcat]
	Comment 48 • 7 years ago
backout bugherder

backed out from m-c
https://hg.mozilla.org/mozilla-central/rev/9d763f7c2d4c

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

Comment on attachment 8829839 [details]
Bug 1322277 - Default socket timeout has to be larger than the default page load timeout.

This is an updated commit for the socket-timeout changes back to 360s. Now with a good explanation why it has to be that high included.

It should fix all of our remaining highly occurring Android test failures.

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

This is not fixed btw. so reopening the bug.

	Andreas Tolfsen ❲:ato❳
	Comment 53 • 7 years ago
mozreview-review

Comment on attachment 8829839 [details]
Bug 1322277 - Default socket timeout has to be larger than the default page load timeout.

https://reviewboard.mozilla.org/r/106826/#review111096

	Pulsebot
	Comment 54 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/755e18391553
Fix usage of capabilities in start_session. r=ato
https://hg.mozilla.org/integration/autoland/rev/9908b674ae2c
Default socket timeout has to be larger than the default page load timeout. r=ato

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

https://hg.mozilla.org/mozilla-central/rev/755e18391553
https://hg.mozilla.org/mozilla-central/rev/9908b674ae2c

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

All the intermittent Mn jobs on Android are fixed since this patch landed! Due to that we want to get this test-only patch uplifted to aurora and beta. Thanks.

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6aea5d06cc3e
https://hg.mozilla.org/releases/mozilla-aurora/rev/2a4e9d28de71

	Ryan VanderMeulen [:RyanVM]
	Comment 58 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/70d37a312952
https://hg.mozilla.org/releases/mozilla-beta/rev/5304df47b50b