Summary: | WebDriver: fix handling of session timeouts for values higher than MAX_INT | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
Component: | WebDriver | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, bugs-noreply, ews-watchlist, hi, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Created attachment 383354 [details]
Patch
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`) Brian, could you review this one? and confirm it doesn't break safari driver? Taking a look. 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. (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. 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 Created attachment 385872 [details]
Rebased patch
(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. Created attachment 386333 [details]
Patch for landing
Committed r253883: <https://trac.webkit.org/changeset/253883> |
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.