V8 6.1 backports #17354
V8 6.1 backports #17354
Conversation
nodejs-github-bot
added
v8 engine
`test-inspector-async-hook-setup-at-signal` is also flaky on VS2017
|
The windows flake is a known flakey, I backported the commit that marks it as such. I'm unsure if the V8 failure is new, here is a run directly against v8.x-staging so we can check https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1084/ |
|
The V8 failure is new. It's from a test added in this PR. |
Original commit message:
[string] Fix regexp fast path in MaybeCallFunctionAtSymbol
The regexp fast path in MaybeCallFunctionAtSymbol had an issue in which
we'd call ToString after checking that the given {object} was a fast
regexp and deciding to take the fast path. This is invalid since
ToString() can call into user-controlled JS and may mutate {object}.
There's no way to place the ToString call correctly in this instance:
1 before BranchIfFastRegExp, it's a spec violation if we end up on the
slow regexp path;
2 the problem with the current location is already described above;
3 and we can't place it into the fast-path regexp builtin (e.g.
RegExpReplace) either due to the same reasons as 1.
The solution in this CL is to restrict the fast path to string
arguments only, i.e. cases where ToString would be a nop and can safely
be skipped.
Bug: chromium:782145
Change-Id: Ifd35b3a9a6cf2e77c96cb860a8ec98eaec35aa85
Reviewed-on: https://chromium-review.googlesource.com/758257
Commit-Queue: Jakob Gruber <[email protected]>
Reviewed-by: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{nodejs#49213}
Ref: v8/v8@cfc3404
Ref: v8/v8@55a9807
ofrobots
force-pushed
the
61-backports
branch
from
f4063dc
to
cc77b16
Compare
|
Dropped one of the commits – it was not needed on V8 6.1. New V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1090/ |
|
@nodejs/v8 PTAL. The PPC machines have problems ATM, but here's another run: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1093/ |
|
Fixed the PPC problem see nodejs/build#1020 (comment) is you are interested. Looks like it was on on the latest run that @ofrobots launched. |
|
@gibfahn Leaving this in your hands as the release manager for |
|
Thanks @ofrobots . @ofrobots @MylesBorins Do you think this is urgent? Normally I'd leave this till after v8.9.2 goes out on Tuesday, but we can land now if there's a need. |
|
I missed your ping earlier @gibfahn, sorry about that. It would have been good to have this in 8.9.2 but that ship has sailed. |
Once we've done the rc we try not to make any changes unless there's a reason to fast-track. I'm happy to include bugfixes that we think are important, but we need to have that discussion first. |
MylesBorins
force-pushed
the
v8.x-staging
branch
from
468562c
to
b05ef97
Compare
Original commit message:
[string] Fix regexp fast path in MaybeCallFunctionAtSymbol
The regexp fast path in MaybeCallFunctionAtSymbol had an issue in which
we'd call ToString after checking that the given {object} was a fast
regexp and deciding to take the fast path. This is invalid since
ToString() can call into user-controlled JS and may mutate {object}.
There's no way to place the ToString call correctly in this instance:
1 before BranchIfFastRegExp, it's a spec violation if we end up on the
slow regexp path;
2 the problem with the current location is already described above;
3 and we can't place it into the fast-path regexp builtin (e.g.
RegExpReplace) either due to the same reasons as 1.
The solution in this CL is to restrict the fast path to string
arguments only, i.e. cases where ToString would be a nop and can safely
be skipped.
Bug: chromium:782145
Change-Id: Ifd35b3a9a6cf2e77c96cb860a8ec98eaec35aa85
Reviewed-on: https://chromium-review.googlesource.com/758257
Commit-Queue: Jakob Gruber <[email protected]>
Reviewed-by: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#49213}
Refs: v8/v8@cfc3404
Refs: v8/v8@55a9807
PR-URL: #17354
|
Landed in c57cd9b |
Some backports from upstream needed for 6.1.
/cc @nodejs/v8
CI: https://ci.nodejs.org/job/node-test-pull-request/11755/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1083/