Closed
Bug 1128997
Opened 9 years ago
Closed 5 years ago
Support indefinite script timeout
Categories
(Remote Protocol :: Marionette, defect, P2)
Remote Protocol
Marionette
Tracking
(firefox66 fixed)
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: ato, Assigned: r2hkri, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, pi-marionette-server, pi-marionette-spec, Whiteboard: [lang=js][wptsync upstream])
Attachments
(1 file)
According to the Selenium API docs a scriptTimeout parameter that is a negative integer should allow a script to run indefinitely: https://selenium.googlecode.com/git/docs/api/java/org/openqa/selenium/WebDriver.Timeouts.html#setScriptTimeout%28long, java.util.concurrent.TimeUnit%29
Reporter | ||
Updated•9 years ago
|
Blocks: webdriver
Keywords: ateam-marionette-server,
ateam-marionette-spec
Comment 1•9 years ago
|
||
Marionette also seems to have an upper limit on the allowed timeout. I'm not sure this is expected.
Reporter | ||
Comment 2•6 years ago
|
||
This is also a conformance issue, as the spec says we should allow scripts to run indefintely.
Reporter | ||
Updated•6 years ago
|
Mentor: ato
Comment 3•6 years ago
|
||
Hello, I'd like to work on this bug. This is my first time contributing to an open source project. Could I get some assistance to get started?
Reporter | ||
Comment 4•6 years ago
|
||
You might find some useful resources in the developer documentation: https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Contributing.html Please ask me for review when you submit your patch.
Reporter | ||
Comment 5•6 years ago
|
||
The description was written four years ago and is out of date. Let me attempt to summarise the work involved here. The WebDriver standard says that if a session’s script timeout duration is null, an injected script by the Execute Script or Execute Async Script commands should never time out: https://w3c.github.io/webdriver/#dfn-session-script-timeout The script timeout duration is a capability that is part of the timeouts object, which you can see more documentation about on MDN: https://developer.mozilla.org/en-US/docs/Web/WebDriver/Timeouts The timeouts object gets passed alongside capabilities when a WebDriver session is created, and geckodriver eventually passes it to Marionette as request body of the WebDriver:NewSession command. The timeouts then apply until they are set updated with WebDriver:SetTimeouts or otherwise for the entire lifetime of the session. When scripts are evaluated we pass the session script timeout value on to evaluate.sandbox in https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/testing/marionette/evaluate.js#35-143. If it is null the DEFAULT_TIMEOUT value gets used. This needs to change so that it supports indefinite timeouts. Further, the function registers a timeout handler: > timeoutHandler = () => reject(new ScriptTimeoutError(`Timed out after ${timeout} ms`)); And sets a timer that invokes this: > scriptTimeoutID = setTimeout(timeoutHandler, timeout); Finally it is cleared after the script is finished evaluating: > clearTimeout(scriptTimeoutID); A simple implementation would put these three lines behind if-conditions so that we don’t end up setting a timeout handler when handed null. Finally we have a lot of Marionette unit tests for this functionality, but unless this change were to break any of them, I propose new test cases are added directly to WPT, since the long-term plan is to deprecate those Marionette unit tests that duplicate WPT tests. I hope this helps!
Updated•6 years ago
|
Assignee: nobody → vkatsikaros
Updated•6 years ago
|
Assignee: vkatsikaros → nobody
Just one overarching question: how will tests work for this? I've been running `./mach marionette test --gecko-log - testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py` for testing. I looked around the tests to see how they work and noticed that the requests are going through Gecko. In GeckoDriver.prototype.execute, there are a few lines that prevent timeout from being null, specifically: https://searchfox.org/mozilla-central/source/testing/marionette/driver.js#983,992 and I was wondering if this should be changed. Also I was wondering where in testing/web-platform/tests new tests should go (assuming they should go there). Thanks.
Comment 8•6 years ago
|
||
r2hkri, thanks for having a look into this and even following the data flow through the different components. It's a great to be able to get the feature implemented. (In reply to r2hkri from comment #7) > lines that prevent timeout from being null, specifically: > https://searchfox.org/mozilla-central/source/testing/marionette/driver. > js#983,992 > and I was wondering if this should be changed. Totally, if we want to support infinite timeouts those checks have to be changed to allow `null`, and positive integers. Timeout values can be set via the `New Session` and `Set Timeouts` commands: https://w3c.github.io/webdriver/#dfn-session-timeouts-configuration https://w3c.github.io/webdriver/#set-timeouts Note, that it might also need a change in geckodriver before wpt (webdriver) tests can be written: https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/testing/webdriver/src/capabilities.rs > Also I was wondering where in testing/web-platform/tests new tests should go > (assuming they should go there). Pretty much into those two directories: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/new_session/timeouts.py https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/set_timeouts/ Let us know if you have other questions where you need help with.
Thanks for the information! I think I've finished the implementation, I'll attach a patch in a bit. I ended up changing a lot more things and the changes are spread out so I don't know if I should make multiple patches or not. I do have a question about a few older tests, for example: https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py#139-146 I was running this and it was not timing out at all. The timeout relies on: https://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1785 so `marionette.timeout.script = 0.2` doesn't seem to do anything for the timeout (leaves it as null which means will not time out at all). I ran it with `script_timeout=0.2` as an argument and it ended up working, albeit I got the timeout must be a positive integer error. This happens for a few tests.
Assignee | ||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
(In reply to r2hkri from comment #9) > Thanks for the information! I think I've finished the implementation, I'll > attach a patch in a bit. I ended up changing a lot more things and the > changes are spread out so I don't know if I should make multiple patches or > not. Basically it would be good to split the patch into multiple revisions if it's getting too large. This could be done by component, or by incremental changes which only touch a specific portion of the code. This will make the patches much easier and faster to review. > https://searchfox.org/mozilla-central/source/testing/marionette/harness/ > marionette_harness/tests/unit/test_execute_async_script.py#139-146 > > I was running this and it was not timing out at all. The timeout relies on: > > https://searchfox.org/mozilla-central/source/testing/marionette/client/ > marionette_driver/marionette.py#1785 > > so `marionette.timeout.script = 0.2` doesn't seem to do anything for the > timeout (leaves it as null which means will not time out at all). I ran it > with `script_timeout=0.2` as an argument and it ended up working, albeit I > got the timeout must be a positive integer error. This happens for a few > tests. There can be clearly a different bug here. For me it was always confusing why both `execute_script` and `execute_async_script` allow to specify a timeout parameter for the Marionette client, while we don't have it for geckodriver. Also that you can set the timeout via the new session capabilities, and the `Set Timeouts` command seems to be enough. I wouldn't feel bad to get this parameter from the client removed. Andreas, what's your opinion? The above mentioned test also looks strange to me. A timeout of 200ms is set, but inside the script the callback is executed with a 50ms delay. This feel racy. Are you saying that `marionette.timeout.script` doesn't set the value, or that exactly this value is not evaluated by the execute async script command? I assume it's the latter, right? Further some parts of the test should be rewritten a bit to use the `with` statement in combination with `assertRaises`, and not to use a string as first argument of `setTimeout` but a function. This can easily be done once `with` is in use, and is more secure.
Flags: needinfo?(ato)
Updated•6 years ago
|
Assignee: nobody → r2hkri
Status: NEW → ASSIGNED
Reporter | ||
Comment 12•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #11) > (In reply to r2hkri from comment #9) > > > so `marionette.timeout.script = 0.2` doesn't seem to do anything > > for the timeout (leaves it as null which means will not time out > > at all). I ran it with `script_timeout=0.2` as an argument and > > it ended up working, albeit I got the timeout must be a positive > > integer error. This happens for a few tests. > > There can be clearly a different bug here. For me it was always > confusing why both `execute_script` and `execute_async_script` > allow to specify a timeout parameter for the Marionette client, > while we don't have it for geckodriver. Also that you can set the > timeout via the new session capabilities, and the `Set Timeouts` > command seems to be enough. I wouldn't feel bad to get this > parameter from the client removed. The timeout argument there mostly for historical reasons. I don’t think we have anything that depends on it, or something that depends on it but that can’t be changed to use session timeouts. I’d support dropping the timeout argument, but I domnt know if this is the right bug to do that in? > The above mentioned test also looks strange to me. A timeout of > 200ms is set, but inside the script the callback is executed > with a 50ms delay. This feel racy. Are you saying that > `marionette.timeout.script` doesn't set the value, or that exactly > this value is not evaluated by the execute async script command? I > assume it's the latter, right? > > Further some parts of the test should be rewritten a bit to use > the `with` statement in combination with `assertRaises`, and > not to use a string as first argument of `setTimeout` but a > function. This can easily be done once `with` is in use, and is > more secure. Since these tests are mostly duplicated in WPT, it is in my opinion not worth spending time fixing them. If we should spot gaps in coverage between Marionette unit tests and WPT, we could port the tests over.
Flags: needinfo?(ato)
Assignee | ||
Comment 13•6 years ago
|
||
> Are you saying that `marionette.timeout.script` doesn't set the
> value, or that exactly this value is not evaluated by the execute async
> script command? I assume it's the latter, right?
At least the latter, the script timeout is `null` so there the test just hangs because
1. The script throws an error: callback not defined (intended I guess based on the test case)
2. There is no timeout now because `null` means indefinite so the promise never resolves or rejects which means there is no response.
In general, I was just running the marionette tests to check if I broke something. If this can be ignored then it's fine.
Comment 14•6 years ago
|
||
(In reply to r2hkri from comment #13) > > Are you saying that `marionette.timeout.script` doesn't set the > > value, or that exactly this value is not evaluated by the execute async > > script command? I assume it's the latter, right? > > At least the latter, the script timeout is `null` so there the test just > hangs because > 1. The script throws an error: callback not defined (intended I guess based > on the test case) Ouch, actually not. I didn't know that this could happen. So I had a look at bug 1128760, and on comment 10 is an interesting comment, which never seems to have followed-up on. Maybe we should just file a bug now to find a way to make that happen. > 2. There is no timeout now because `null` means indefinite so the promise > never resolves or rejects which means there is no response. > > In general, I was just running the marionette tests to check if I broke > something. If this can be ignored then it's fine. This particular script would need a timeout to be set. If a workaround is to replace `self.marionette.timeouts.script` with the `script_timeout` argument it will be fine. (In reply to Andreas Tolfsen ❲:ato❳ from comment #12) > The timeout argument there mostly for historical reasons. I don’t > think we have anything that depends on it, or something that depends > on it but that can’t be changed to use session timeouts. > > I’d support dropping the timeout argument, but I domnt know if this > is the right bug to do that in? Great. So I filed bug 1510929 to keep this tracked.
Reporter | ||
Comment 15•5 years ago
|
||
I also posted a comment here: https://phabricator.services.mozilla.com/D13181#347998
Comment 16•5 years ago
|
||
r2hkri, are you still interested in continuing on this bug? The dependency is fixed now, so that using indefinite timeouts should be possible now.
Flags: needinfo?(r2hkri)
Comment 18•5 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b97e2750407e Support indefinite script timeout r=ato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14736 for changes under testing/web-platform/tests
Whiteboard: [lang=js] → [lang=js][wptsync upstream]
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/14736 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/JmSJgnJRTceXlWTbiR8pqw)
Whiteboard: [lang=js][wptsync upstream] → [lang=js][wptsync upstream error]
Comment 21•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b97e2750407e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Reporter | ||
Comment 22•5 years ago
|
||
Thanks reimu, for fixing this bug! I’m sorry it took so long for
us to be able to land the change, but I believe we are all in a
better place now because of it.
Comment 23•5 years ago
|
||
https://hg.mozilla.org/projects/cedar/rev/b97e2750407e3e98d7165c5605732ce2a7b0e68c Bug 1128997 - Support indefinite script timeout r=ato
Whiteboard: [lang=js][wptsync upstream error] → [lang=js][wptsync upstream]
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•