feat(ses): Shim compatible with Hermes compiler #2334
Conversation
leotm
changed the title
|
ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small |
There was a problem hiding this comment.
This is indeed far more surgical than I expected.
I only request that we capture the async generator function instance in a single try{eval}catch in commons.js so we can just use if blocks to decide whether to include async generators in the various points you’re currently using try/catch.
I would like to make sure @erights looks at these changes as well.
Are you familiar with stacked PRs? You can propose the test changes in a PR based on this branch so they can be reviewed together. I would hesitate to approve code that doesn’t come with tests in the same merge, though I’m fine with reviewing them separately. Much depends on your workflow. One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge. Another workflow is to create “stacked” PRs for each review artifact, then merge them top to bottom, so that ultimately the feature and its tests arrive in |
|
Glad to see this!
Hopefully tomorrow. |
leotm
force-pushed
the
ses-hermes
branch
4 times, most recently
from
708e9ae to
2d664b7
Compare
Closes: #2598 Refs: #2563 #2334 #1221 ## Description #1221 was supposed to make ses tolerate undeletable `func.prototype` properties that should be absent, so long as they could be set to `undefined` instead, making them harmless. This tolerance came with a warning to flag the remaining non-conformance. However #2598 explains why #1221 sometimes fails to do this. #1221 did come with a test, but it fell into the case where #1221 works, which is a non-toplevel function. #2563 (and #2334 ?) fell into the trap explained by #2598 and untested by #1221, which is an undeletable `func.prototype` on a top-level instrinsic. As a result, #2563 currently contains a workaround for #2598 which this PR would make unnecessary. This PR fixes the problem by factoring out the `func.prototype`-tolerant property deletion into a separate `cauterizeProperty` function which it calls from both places. This PR also adds the test that was missing from #1221 , having first checked that the test detects #2598 when run without the rest of this PR. If this PR gets merged before #2563, then #2563's workaround for #2598 can first be removed before it is merged. - [ ] TODO should pass a genuine reporter in to all calls to `cauterizeProperty`. @kriskowal , please advise how intrinsics.js should arrange to do so. ### Security Considerations Allowing a `func.prototype` property that really shouldn't be there seems safe, so long as it is safely set to `undefined` first, which this PR does, and then checks that it has done so. ### Scaling Considerations none ### Documentation Considerations generally, this would be one less thing to worry about, and thus one less thing that needs to be documented for most users. ### Testing Considerations Adds the test that was missing from #1221 that let #2598 go unnoticed until #2563 ### Compatibility Considerations Should be none. ### Upgrade Considerations Should be none.
|
What is the status of this? Is it still needed? |
leotm
force-pushed
the
ses-hermes
branch
3 times, most recently
from
f6fb020 to
7b9eca2
Compare
yes this is still needed for CI feedback keeping SES compatible with Hermes I plan to add draft release notes to |
leotm
force-pushed
the
ses-hermes
branch
3 times, most recently
from
182b0e2 to
aa0e8ca
Compare
Co-authored-by: naugtur <[email protected]>
Description
Add lockdown shim compatible with HermesMake current shim compatible with Hermes compiler
for building React Native (RN) prod apps with SES
Generating the release AAB (Android App Bundle)
i.e.
npx react-native build-android --mode=releasevia RN CLIcalls Gradle's
bundleReleasetask under the hood (bundlebuild task onreleasevariant)which calls RNGP (React Native Gradle Plugin) React task
createBundleReleaseJsAndAssetsand failsafter Metro finishes writing the release bundle (bundle, sourcemaps, assets)
Gradle emits these Hermes errors
async functions are unsupportedasync arrow functions are unsupportedfacebook/hermes#1395async generators are unsupported(at runtime we can see both are
SyntaxErrors)Resulting in vague
java.lang.StackOverflowError (no error message)The try/catch approach testing language ft support via a new fn works
since RNGP no longer emits the Hermes errors in the task after Metro bundles
and we're conditionally using parts of SES compatible with the Hermes engine
The initial approach involved building a new shim via an env var
which involved a lot of duplicates
/srcfilesNb: clean before bundling to see changes reflected (avoid cache)
i.e.
./gradlew clean :app:bundleReleaseNb:
async function* a() {};alone won't emit an errorbut using/referencing it and beyond
const b = a;willNb: eventually we hit RNGP BundleHermesCTask.kt > run > detectedHermesCommand > detectOSAwareHermesCommand from PathUtils.kt, which calls the Hermes compiler default command
hermesc- the path of the binary file, to create the optimised bytecode bundle to load/exec at runtimeTODO
./gradlew :app:createBundleReleaseJsAndAssets./gradlew :app:installRelease -PreactNativeArchitectures=arm64-v8ayarn <android/ios> --mode releaseFollow-up, CI testing options discussed
cd android && ./gradlew :app:bundleReleaseCI(macos): RN app test + SES, Xcode releasetest262:hermesscript (liketest262:xs)Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Compatibility Considerations
Upgrade Considerations