Andreas Tolfsen ❲:ato❳ Reporter
	Description • 9 years ago

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

	James Graham [:jgraham]
	Comment 1 • 9 years ago

Marionette also seems to have an upper limit on the allowed timeout. I'm not sure this is expected.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 2 • 6 years ago

This is also a conformance issue, as the spec says we should allow
scripts to run indefintely.

	Andrew Gobis
	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?

	Andreas Tolfsen ❲:ato❳ 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.

	Andreas Tolfsen ❲:ato❳ 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!

	r2hkri Assignee
	Comment 6 • 6 years ago

I'll try this if no one is working on it.

	r2hkri Assignee
	Comment 7 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	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.

	r2hkri Assignee
	Comment 9 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	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.

	Andreas Tolfsen ❲:ato❳ 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.

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 15 • 5 years ago

I also posted a comment here:
https://phabricator.services.mozilla.com/D13181#347998

	Henrik Skupin [:whimboo][⌚️UTC+1]
	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.

	r2hkri Assignee
	Comment 17 • 5 years ago

Yes, I'll continue working on this.

	Pulsebot
	Comment 18 • 5 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b97e2750407e
Support indefinite script timeout r=ato

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 19 • 5 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14736 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 20 • 5 years ago

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)

	Cristian Brindusan [:cbrindusan]
	Comment 21 • 5 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b97e2750407e

	Mike de Boer [:mikedeboer]
	Comment 23 • 5 years ago

https://hg.mozilla.org/projects/cedar/rev/b97e2750407e3e98d7165c5605732ce2a7b0e68c
Bug 1128997 - Support indefinite script timeout r=ato