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.

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.

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!

(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.

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.

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

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.

(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)?

(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.

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

P2 since it blocks regression bug 1303346.

(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.

(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.

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.

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?

(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.

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.

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.

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

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.

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.

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.

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?

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.

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.

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": …}}) ```

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.

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.

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.

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

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.

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.

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!

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

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

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

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

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

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

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!

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

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

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

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.

This is not fixed btw. so reopening the bug.

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

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

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

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.

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

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