Closed Bug 1128997 Opened 5 years ago Closed 1 year ago

Support indefinite script timeout

Categories

(Testing :: Marionette, defect, P2)

defect

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
Marionette also seems to have an upper limit on the allowed timeout. I'm not sure this is expected.
This is also a conformance issue, as the spec says we should allow
scripts to run indefintely.
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: [lang=js]
Version: unspecified → Trunk
Mentor: ato
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?
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.
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!
Assignee: nobody → vkatsikaros
Assignee: vkatsikaros → nobody
I'll try this if no one is working on it.
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.
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.
(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)
Assignee: nobody → r2hkri
Status: NEW → ASSIGNED
(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)
> 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.
(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.
Depends on: 1510929
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)
Yes, I'll continue working on this.
Flags: needinfo?(r2hkri)
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]
https://hg.mozilla.org/mozilla-central/rev/b97e2750407e
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

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.

Whiteboard: [lang=js][wptsync upstream error] → [lang=js][wptsync upstream]
You need to log in before you can comment on or make changes to this bug.