Spec percent-scripts-timeout #1341
Conversation
more. Actually implement the get.
|
Pretty small one this time. |
|
|
||
| 1. [=Assert=] that these steps are running [=in parallel=]. | ||
|
|
||
| 1. If |timeout| ≤ 0, [=immediately=] interrupt the execution and set |finalCompletion| to a | ||
| new [=ECMAScript/throw completion=] given null. | ||
| 1. If |timeout| ≤ 0, return (new [=ECMAScript/throw completion=] given null, true). |
There was a problem hiding this comment.
need to update line 5745 below as well to return an extra false?
There was a problem hiding this comment.
.... good catch, though this needs more work: the timeout applies to top-level execution as well. I am gonna need an extra moment on that.
(Actually it also affects WebIDL conversions of return values, since otherwise one could smuggle entire computation into them, but I am not sure how one even hooks that)
There was a problem hiding this comment.
Fixed, but kinda hand-wavish. PTAL.
There was a problem hiding this comment.
for webIDL conversion you mentioned, is it https://github.com/WICG/turtledove/blob/main/spec.bs#L5620, or something else?
There was a problem hiding this comment.
Yeah. That can run arbitrary code[1] (so it probably ought to be explicit about what context it runs in).
Same for scoreAd. Compare to:
(which I think I owe refactor of, but it's a little easier to read for this purpose than the bidder equivalent since the business stuff isn't hidden in the binding. Notice how it uses the same time limit; the v8_helper also provides the context).
[1] We have a couple other places this used where we start with more restricted types (sometimes infra types) where nothing funky can happen, as it's doing some sort of "round a double to int" type deals. It's where the source is an Object that JS gets... JS.
There was a problem hiding this comment.
There was a problem hiding this comment.
does the js->idl conversion need the script's context? It does not seem so, IIUC. Then we can probably return the remaining timeout after script execution, and apply it to the js->IDL conversion step?
Not suggesting to do it in this PR, but wondering whether and how we fix it in spec.
There was a problem hiding this comment.
I think the context isn't explicit in the way the WebIDL and ES262 specs works, e.g. it calls ops like this:
https://tc39.es/ecma262/multipage/abstract-operations.html#sec-tonumber
...so I suppose returning the remaining timeout and having a similar thing about timeouts on those spec is appropriate.
|
|
||
| 1. [=Assert=] that these steps are running [=in parallel=]. | ||
|
|
||
| 1. If |timeout| ≤ 0, [=immediately=] interrupt the execution and set |finalCompletion| to a | ||
| new [=ECMAScript/throw completion=] given null. | ||
| 1. If |timeout| ≤ 0, return (new [=ECMAScript/throw completion=] given null, true). |
There was a problem hiding this comment.
for webIDL conversion you mentioned, is it https://github.com/WICG/turtledove/blob/main/spec.bs#L5620, or something else?
SHA: 4a5cbfb Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 4a5cbfb Reason: push, by morlovich Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Also fixes a bug where script timeout wasn't applied to top-level execution.
Preview | Diff