Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spec percent-scripts-timeout #1341
Spec percent-scripts-timeout #1341
Changes from all commits
b0aa11e
633def4
055b0f5
0ed6ce3
0a71b5b
5167149
afddb46
3cbf3f9
22426a1
98197a9
36c79a2
72f1b03
0e4697f
03fc7ab
a427a2b
89f62b9
fa06d95
f5a3249
00d698c
0b1b2c3
4caa43a
9d452c1
956b1de
4343fc5
0f5a97b
e363f8c
5f1dc7b
c9fb6d1
136ca90
c5e3ba4
5b376a5
64f0e34
318c480
f16bcef
0d9fb70
af730a8
bb5466a
c8fec8f
fd5dd85
0d71e23
2012e65
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to update line 5745 below as well to return an extra false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.... 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, but kinda hand-wavish. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
https://source.chromium.org/chromium/chromium/src/+/main:content/services/auction_worklet/seller_worklet.cc;drc=b41d6ee12deac18fac89f4d27c2a662e311406fc;l=1232
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and a testcase:
https://source.chromium.org/chromium/chromium/src/+/main:content/services/auction_worklet/bidder_worklet_unittest.cc;drc=33161dc8d856d514f774c8252763cc49bdc83c42;l=1539
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Compare to:
https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-value.h;drc=6ac7875f61c72fa111bc501039e4673693be6b64;l=384
...so I suppose returning the remaining timeout and having a similar thing about timeouts on those spec is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good.