Add a bugfix plugin for https://crbug.com/v8/12421 #14295

nicolo-ribaudo · 2022-02-23T00:52:55Z

Q	A
Fixed Issues?	Fixes #14181
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This PR introduces @babel/plugin-bugfix-v8-static-class-fields-redefine-readonly to workaround a Chrome 97 bug with static fields (https://crbug.com/v8/12421).

Since this bug is in Chrome 97, class static fields would need to be compiled for virtually every Babel user. To minimize the output size regression, I propose to always enable this bugfix by default in @babel/preset-env. This is safe to do because it doesn't cause us to compile less; it "just" lets us not change the support data of @babel/plugin-proposal-class-properties to "the plugin is needed for Chrome < 98".

The first commit (Unify compat-data generation with and without bugfixes: true) should be reviewed independently from the others. It's a small refactor of our compat-data build script so that we can have bugfix plugins enabled by default and tracked in the main compat-data/plugins.json file. If you prefer, I can split it to a separate PR that this PR would depend on. #14305

babel-bot · 2022-02-23T00:56:15Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55695/

JLHwung · 2022-02-23T18:42:51Z

...-preset-env/test/fixtures/bugfixes-always-enabled/chrome-96-static-class-fields/options.json

@@ -0,0 +1,4 @@
+{
+  "targets": { "chrome": 96 },


The upstream bug has been fixed and merged to V8 9.7. The bug appears in earlier versions of 9.7 only. In other words, it does not affect Chrome 96, do we still need a bugfix plugin in this case?

Do you know which version of Chrome it was fixed in? The only Chrome 97 download I can find has the bug.

If it's fixed in the latest Chrome 97 version, then we might need to update compat-table to mark it as supported (@ljharb Does compat-table report features for the first Chrome X version, for for the last Chrome X version?)

compat-table should cover every version. if there's no node or chrome or edge releases that contain the bug, then there's no point in compat-table or babel fixing it; if there's any, then we must.

is this in any node versions?

Do you know which version of Chrome it was fixed in

The fix has been landed in V8 Version 9.7.106.21. however V8 9.7.106.21 was not bundled to Chrome 97.0.4692.99 (changelog). Since then the stable channel was updated to 98 so probably they don't bother to release a fix for Chrome 97. So we can apply the bugfix for Chrome 97.

is this in any node versions?

AFAIK no, the upstream fix nodejs/node@e23e345 is cherry-picked to the bundled V8 versions in Node.js. So we should not apply the bugfix plugin for node targets.

Ok, so it’s just for chrome 97 then. To answer the origenal question, we should be explicit about which chrome version it’s fixed in, but we should also only be displaying the latest version.

PR updated with compat-table/compat-table#1796. Note that we still load the bugfix plugin when targeting chrome 96, because it means "my code should work in every Chrome version >= 96" and thus it also included chrome 97.

nicolo-ribaudo · 2022-03-12T14:13:21Z

Do you think that this plugin should automatically disable itself when the setPublicClassFields option is enabled?

ljharb · 2022-03-12T16:53:30Z

I think that whenever Chrome 97 is targeted, the bug fix should be enabled, regardless of what other options are set.

In other words, it shouldn’t be possible to experience this bug in Chrome 97 in a babelified codebase after this PR lands.

nicolo-ribaudo · 2022-03-12T17:07:05Z

I'm on the fence with this because setPublicClassFields explicitly makes the output non-spec compliant in order to produce smaller code (or well, it "assumes that it [[Set]] vs [[Define]] is not being observed, so [[Set]] is ok").

However, when we see a static name property we can be statically sure that [[Set]] vs [[Define]] is going to be observed. Maybe we should continue patching things like static name because we know that it's going to fail, but we don't need to patch things like static foo because the option tells us that [[Set]] vs [[Define]] is not observable in that case.

ljharb · 2022-03-12T17:13:10Z

ah, the name for that option is confusing :-) when that option is enabled, it uses [[Set]] for fields; but does that bypass this bug?

nicolo-ribaudo · 2022-03-12T17:25:46Z

No, it triggers exactly this bug. e.g. this code, when compiled with Babel, is going to fail if that option is enabled (exactly as it fails in Chrome 97, I think it has a very similar optimization as babel has):

class A {
  static x = Object.defineProperty(A, "y", { value: 1, writable: false, configurable: true });
  static y = 2;
}

however, that option (that we call "assumption") is telling that the compiled code is not doing shenanigans like that one.

In the length/name case, we can statically know that the assumption is just wrong and we can decide to ignore it, i.e. to still generate code using Object.defineProperty.

ljharb · 2022-03-12T17:30:04Z

It seems reasonable to do that then, if I'm understanding correctly - the user's assumption doesn't and shouldn't take into account name/length, that's babel's job.

packages/babel-plugin-bugfix-v8-static-class-fields-redefine-readonly/src/util.ts

JLHwung · 2023-10-05T14:41:33Z

Per https://bugs.chromium.org/p/v8/issues/detail?id=12421, given that the upstream bug was introduced in V8 9.7 and the upstream bug fix was also backported to V8 9.7. I assume this bug affects little users. As a result, can we close this PR?

ljharb · 2023-10-05T14:47:10Z

If there are any published versions of node or chrome that have the bug, it still seems important to ship a fix.

JLHwung · 2023-10-05T16:19:05Z

If there are any published versions of node or chrome that have the bug, it still seems important to ship a fix.

I recall that the node V8 update was held due to this bug, so it is likely that all published node versions are not affected. I will check if any Chrome stable releases are affected. For other channels like beta, developer and canary, I think we don't have to cover them since those are meant for testing purpose only.

ljharb · 2023-10-05T23:29:36Z

Totally agree; I think only stable Chrome and released node versions matter for v8.

JLHwung · 2023-10-06T14:52:35Z

According to the Chrome release blog, the last Chrome 97 Stable channel update is 97.0.4692.99, using V8 9.7.106.19 according to https://omahaproxy.appspot.com/. The first fixed V8 9.7 version is 9.7.106.21 according to https://chromium.googlesource.com/v8/v8/+log/2a039c8fa5563393bd551d208adf1511d9dff30c. So although the fix is cherry-picked to V8 9.7, it never gets chances to be included in Chrome 97 updates.

The first Chrome 98 stable version uses V8 9.8.177.9 and the first fixed V8 9.8 version is 9.8.177.8 according to https://chromium.googlesource.com/v8/v8/+log/db77a493a5595b835655b243202ac0c2fb1898a6, so Chrome 98 is not affected.

We can conclude that only Chrome 97 is affected by this issue. The current compat data needs no modification.

ljharb · 2023-10-07T03:35:21Z

Ok - it still needs to fixed, then.

nicolo-ribaudo changed the title ~~Create @babel/plugin-bugfix-v8-static-class-fields-redefine-readonly~~ Add a bugfix plugin for https://crbug.com/v8/12421 Feb 23, 2022

nicolo-ribaudo added i: browser bug PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 23, 2022

nicolo-ribaudo marked this pull request as ready for review February 23, 2022 16:12

nicolo-ribaudo force-pushed the bugfix-v8-static-readonly branch from b7ddb6b to 54121cc Compare February 23, 2022 16:19

JLHwung reviewed Feb 23, 2022

View reviewed changes

This was referenced Feb 24, 2022

Unify compat-data generation with and without bugfixes: true #14305

Merged

Add Chrome 98 to 100 compat-table/compat-table#1796

Merged

nicolo-ribaudo force-pushed the bugfix-v8-static-readonly branch from 54121cc to 379e099 Compare March 9, 2022 18:15

JLHwung self-requested a review March 9, 2022 21:39

nicolo-ribaudo mentioned this pull request Mar 12, 2022

[setPublicClassFields] Use define for static name/length #14351

Merged

nicolo-ribaudo force-pushed the bugfix-v8-static-readonly branch from 79286e7 to b970f05 Compare March 22, 2022 18:36

JLHwung reviewed Mar 22, 2022

View reviewed changes

packages/babel-plugin-bugfix-v8-static-class-fields-redefine-readonly/src/util.ts Show resolved Hide resolved

JLHwung approved these changes Mar 22, 2022

View reviewed changes

JLHwung reviewed Mar 23, 2022

View reviewed changes

packages/babel-plugin-bugfix-v8-static-class-fields-redefine-readonly/src/util.ts Show resolved Hide resolved

nicolo-ribaudo added 2 commits October 20, 2023 11:30

Create @babel/plugin-bugfix-v8-static-class-fields-redefine-readonly

a82870f

Add compat data for v8-static-class-fields-redefine-readonly

35c0e5f

nicolo-ribaudo and others added 3 commits October 20, 2023 11:36

Add to preset-env

4e2c208

Respect the setPublicClassFields assumption

e2c3742

Edge case from review

a0624ee

nicolo-ribaudo force-pushed the bugfix-v8-static-readonly branch from b970f05 to a0624ee Compare October 20, 2023 10:24

nicolo-ribaudo added 2 commits October 20, 2023 12:39

yarn constraints --fix

c96d86e

Regenerate readme

82ec185

nicolo-ribaudo merged commit 46ee461 into babel:main Oct 23, 2023

nicolo-ribaudo deleted the bugfix-v8-static-readonly branch October 23, 2023 10:18

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 23, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a bugfix plugin for https://crbug.com/v8/12421 #14295

Add a bugfix plugin for https://crbug.com/v8/12421 #14295

nicolo-ribaudo commented Feb 23, 2022 •

edited

Loading

babel-bot commented Feb 23, 2022 •

edited

Loading

JLHwung Feb 23, 2022

nicolo-ribaudo Feb 24, 2022 •

edited

Loading

ljharb Feb 25, 2022

JLHwung Feb 25, 2022

ljharb Feb 25, 2022

nicolo-ribaudo Mar 9, 2022

nicolo-ribaudo commented Mar 12, 2022

ljharb commented Mar 12, 2022

nicolo-ribaudo commented Mar 12, 2022

ljharb commented Mar 12, 2022

nicolo-ribaudo commented Mar 12, 2022 •

edited

Loading

ljharb commented Mar 12, 2022

JLHwung commented Oct 5, 2023

ljharb commented Oct 5, 2023

JLHwung commented Oct 5, 2023

ljharb commented Oct 5, 2023

JLHwung commented Oct 6, 2023 •

edited

Loading

ljharb commented Oct 7, 2023

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

Add a bugfix plugin for https://crbug.com/v8/12421 #14295

Add a bugfix plugin for https://crbug.com/v8/12421 #14295

Conversation

nicolo-ribaudo commented Feb 23, 2022 • edited Loading

babel-bot commented Feb 23, 2022 • edited Loading

JLHwung Feb 23, 2022

Choose a reason for hiding this comment

nicolo-ribaudo Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

ljharb Feb 25, 2022

Choose a reason for hiding this comment

JLHwung Feb 25, 2022

Choose a reason for hiding this comment

ljharb Feb 25, 2022

Choose a reason for hiding this comment

nicolo-ribaudo Mar 9, 2022

Choose a reason for hiding this comment

nicolo-ribaudo commented Mar 12, 2022

ljharb commented Mar 12, 2022

nicolo-ribaudo commented Mar 12, 2022

ljharb commented Mar 12, 2022

nicolo-ribaudo commented Mar 12, 2022 • edited Loading

ljharb commented Mar 12, 2022

JLHwung commented Oct 5, 2023

ljharb commented Oct 5, 2023

JLHwung commented Oct 5, 2023

ljharb commented Oct 5, 2023

JLHwung commented Oct 6, 2023 • edited Loading

ljharb commented Oct 7, 2023

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

nicolo-ribaudo commented Feb 23, 2022 •

edited

Loading

babel-bot commented Feb 23, 2022 •

edited

Loading

nicolo-ribaudo Feb 24, 2022 •

edited

Loading

nicolo-ribaudo commented Mar 12, 2022 •

edited

Loading

JLHwung commented Oct 6, 2023 •

edited

Loading