Bug 204114 – WebDriver: fix handling of session timeouts for values higher than MAX_INT

• Component: WebDriver
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Timeout values can be between 0 and the maximum size integer (2^53 - 1) which is higher than INT_MAX. There are several tests failing because of this:

imported/w3c/webdriver/tests/set_timeouts/set.py::test_positive_integer[9007199254740991-implicit]
imported/w3c/webdriver/tests/set_timeouts/set.py::test_positive_integer[9007199254740991-pageLoad]
imported/w3c/webdriver/tests/set_timeouts/set.py::test_positive_integer[9007199254740991-script]

>       assert session.timeouts._get(typ) == value
E       AssertionError: assert 2147483647 == 9007199254740991

We should just use double everywhere and avoid the seconds to milliseconds conversions too.

[Carlos Garcia Campos]

Created attachment 383354 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Carlos Garcia Campos]

Brian, could you review this one? and confirm it doesn't break safari driver?

[Blaze Burg]

Taking a look.

[Blaze Burg]

Comment on attachment 383354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383354&action=review

r- because its needs to be rebased.

Code changes look otherwise fine.

Only concern: I understand moving from int to Seconds, but not from Seconds to double. Is there any practical benefit to using raw double over Seconds to represent seconds? It seems like the entire point of Seconds is to replace usage of doubles.

> Source/WebDriver/Session.h:54
> +    double pageLoadTimeout() const { return m_pageLoadTimeout; }

Why?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:269
> +        JSValueMakeNumber(context, callbackTimeout.valueOr(-1))

Okay.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:67
> +            if (!resultReported && callbackTimeout >= 0)

Okay.

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #5)
> Comment on attachment 383354 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383354&action=review
> 
> r- because its needs to be rebased.

Ok, that's more a r+ cq- :-)

> Code changes look otherwise fine.
> 
> Only concern: I understand moving from int to Seconds, but not from Seconds
> to double. Is there any practical benefit to using raw double over Seconds
> to represent seconds? It seems like the entire point of Seconds is to
> replace usage of doubles.

IIRC I got rounding errors that made some tests fail. I'll check again, I don't remember now.

> > Source/WebDriver/Session.h:54
> > +    double pageLoadTimeout() const { return m_pageLoadTimeout; }
> 
> Why?
> 
> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:269
> > +        JSValueMakeNumber(context, callbackTimeout.valueOr(-1))
> 
> Okay.
> 
> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.js:67
> > +            if (!resultReported && callbackTimeout >= 0)
> 
> Okay.

[Carlos Garcia Campos]

Comment on attachment 383354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383354&action=review

>>> Source/WebDriver/Session.h:54
>>> +    double pageLoadTimeout() const { return m_pageLoadTimeout; }
>> 
>> Why?
> 
> 

When using Seconds we first get a double value in milliseconds that is converted to seconds. That value is then converted again to milliseconds when used as a timeout in js code. So we get milliseconds and we need milliseconds, there's no need to convert to seconds at all. There are also rounding issue when doing so, for example the tests that check that maximum value 9007199254740991 fail if we convert to and from seconds, see:

v = 9007199254740991.
v / 1000 -> 9007199254740.99
(v / 1000) * 1000 -> 9007199254740990.0
9007199254740991. != 9007199254740990.0

[Carlos Garcia Campos]

Created attachment 385872 [details]
Rebased patch

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Brian Burg from comment #5)
> > Comment on attachment 383354 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=383354&action=review
> > 
> > r- because its needs to be rebased.
> 
> Ok, that's more a r+ cq- :-)

I'll assume it and land this patch before I have to rebase it again. I'll rebase the other patches waiting review that depend on this one.

[Carlos Garcia Campos]

Created attachment 386333 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r253883: <https://trac.webkit.org/changeset/253883>

[Radar WebKit Bug Importer]

<rdar://problem/58156898>