fix(worker): cross-runtime shared tests, worker path resolution parity, and native object lifetime fixes#387
Open
edusperoni wants to merge 3 commits into
Open
fix(worker): cross-runtime shared tests, worker path resolution parity, and native object lifetime fixes#387edusperoni wants to merge 3 commits into
edusperoni wants to merge 3 commits into
Conversation
This solves a crash when releasing blocks like when we close/terminate workers
…dling for missing scripts
📝 WalkthroughWalkthroughThis PR updates CI workflows to enable git submodule checkout, refactors native memory management in ObjectManager for block and object disposal, adds worker script path resolution with validation, and reorganizes test infrastructure with large-scale test suite removal. ChangesCI and Native Runtime Updates with Test Reorganization
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/npm_release.yml (1)
27-30: 💤 Low valueConsider adding
persist-credentials: falsefor defense-in-depth.While not critical given the existing
harden-runnerstep and limited artifact uploads, settingpersist-credentials: falseon allactions/checkoutsteps prevents GitHub credentials from persisting in the checked-out workspace, reducing the attack surface if credentials were to be inadvertently exposed.🔒 Example for the build job checkout in npm_release.yml
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 submodules: true + persist-credentials: falseApply the same pattern to all four checkout steps across both workflow files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/npm_release.yml around lines 27 - 30, Add the persist-credentials: false option to every actions/checkout step (e.g., the checkout using actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd) so credentials are not left in the workspace; locate each checkout block that already sets fetch-depth and submodules and add persist-credentials: false, and apply this change to all four checkout steps across the workflow files for defense-in-depth.Source: Linters/SAST tools
NativeScript/runtime/ObjectManager.mm (1)
108-121: 💤 Low valueBlock ownership handling is correct; consider optional null guard.
The fix properly distinguishes between wrapper-owned blocks (needing
CFReleaseto balance theCFRetainfrom wrapping) and native-owned blocks (which must not be released here). This correctly addresses the prior use-after-free and leak issues described in the PR objectives.One minor defensive consideration: if
Block()were ever null whenOwnsBlock() == true(e.g., due to a bug upstream),CFRelease(nullptr)is undefined. This is low probability given the invariant fromInterop::GetResult, but a null check would be cheap insurance.🛡️ Optional defensive null check
case WrapperType::Block: { BlockWrapper* blockWrapper = static_cast<BlockWrapper*>(wrapper); - if (blockWrapper->OwnsBlock()) { + if (blockWrapper->OwnsBlock() && blockWrapper->Block() != nullptr) { // Balance the CFRetain taken when a native block was wrapped for JS🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ObjectManager.mm` around lines 108 - 121, Add a defensive null check before calling CFRelease in the WrapperType::Block case: when handling BlockWrapper in the ObjectManager, check blockWrapper->OwnsBlock() and also ensure blockWrapper->Block() is non-null before calling CFRelease to avoid invoking CFRelease(nullptr); keep the existing ownership logic and comments intact so wrappers that don't own the block still skip release.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 27-30: Add the persist-credentials: false option to every
actions/checkout step (e.g., the checkout using
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd) so credentials are
not left in the workspace; locate each checkout block that already sets
fetch-depth and submodules and add persist-credentials: false, and apply this
change to all four checkout steps across the workflow files for
defense-in-depth.
In `@NativeScript/runtime/ObjectManager.mm`:
- Around line 108-121: Add a defensive null check before calling CFRelease in
the WrapperType::Block case: when handling BlockWrapper in the ObjectManager,
check blockWrapper->OwnsBlock() and also ensure blockWrapper->Block() is
non-null before calling CFRelease to avoid invoking CFRelease(nullptr); keep the
existing ownership logic and comments intact so wrappers that don't own the
block still skip release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: acdb70ac-e2b5-4866-a396-2c4995b5a78c
📒 Files selected for processing (146)
.github/workflows/npm_release.yml.github/workflows/pull_request.ymlNativeScript/runtime/ObjectManager.mmNativeScript/runtime/Worker.mmTestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.jsTestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/jasmine.jsTestRunner/app/Infrastructure/Jasmine/jasmine-reporters/junit_reporter.jsTestRunner/app/Infrastructure/Jasmine/jasmine-reporters/terminal_reporter.jsTestRunner/app/Infrastructure/Jasmine/jasmine.d.tsTestRunner/app/Infrastructure/simulator.jsTestRunner/app/Infrastructure/timers.jsTestRunner/app/Infrastructure/utf8.jsTestRunner/app/sharedTestRunner/app/tests/index.jsTestRunner/app/tests/shared/.gitattributesTestRunner/app/tests/shared/Import/ImportCommonJS/file.jsTestRunner/app/tests/shared/Import/ImportCommonJS/index.jsTestRunner/app/tests/shared/Import/ImportJSON/data.jsonTestRunner/app/tests/shared/Import/ImportJSON/index.jsTestRunner/app/tests/shared/Import/index.jsTestRunner/app/tests/shared/README.mdTestRunner/app/tests/shared/Require/AbsolutePath/dependency.jsTestRunner/app/tests/shared/Require/AbsolutePath/index.jsTestRunner/app/tests/shared/Require/CaseSensitive/MyModule.jsTestRunner/app/tests/shared/Require/CaseSensitive/index.jsTestRunner/app/tests/shared/Require/CaseSensitive/mymodule/index.jsTestRunner/app/tests/shared/Require/ChangingRequireObject/dependency.jsTestRunner/app/tests/shared/Require/ChangingRequireObject/index.jsTestRunner/app/tests/shared/Require/CircularIndexRequire/dependency1.jsTestRunner/app/tests/shared/Require/CircularIndexRequire/dependency2.jsTestRunner/app/tests/shared/Require/CircularIndexRequire/index.jsTestRunner/app/tests/shared/Require/CircularRequire/dependency1.jsTestRunner/app/tests/shared/Require/CircularRequire/dependency2.jsTestRunner/app/tests/shared/Require/CircularRequire/dependency3.jsTestRunner/app/tests/shared/Require/CircularRequire/index.jsTestRunner/app/tests/shared/Require/CircularRequireWithExports/dependency1.jsTestRunner/app/tests/shared/Require/CircularRequireWithExports/dependency2.jsTestRunner/app/tests/shared/Require/CircularRequireWithExports/dependency3.jsTestRunner/app/tests/shared/Require/CircularRequireWithExports/index.jsTestRunner/app/tests/shared/Require/DirectoryWithIndexJson/index.jsTestRunner/app/tests/shared/Require/DirectoryWithIndexJson/module/index.jsonTestRunner/app/tests/shared/Require/DirnameAndFilenameParameters/dep1/dependency1.jsTestRunner/app/tests/shared/Require/DirnameAndFilenameParameters/dep2/dep2-inner/index.jsTestRunner/app/tests/shared/Require/DirnameAndFilenameParameters/index.jsTestRunner/app/tests/shared/Require/ExportsBubbling/dependency1.jsTestRunner/app/tests/shared/Require/ExportsBubbling/dependency2.jsTestRunner/app/tests/shared/Require/ExportsBubbling/dependency3.jsTestRunner/app/tests/shared/Require/ExportsBubbling/dependency4.jsTestRunner/app/tests/shared/Require/ExportsBubbling/dependency5.jsTestRunner/app/tests/shared/Require/ExportsBubbling/index.jsTestRunner/app/tests/shared/Require/FileAndDirectoryWithSameName/index.jsTestRunner/app/tests/shared/Require/FileAndDirectoryWithSameName/module.jsTestRunner/app/tests/shared/Require/FileAndDirectoryWithSameName/module/submodule.jsTestRunner/app/tests/shared/Require/FileEndingWithCommentedLine/index.jsTestRunner/app/tests/shared/Require/FileWithDots/file.name.jsTestRunner/app/tests/shared/Require/FileWithDots/index.jsTestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency1.jsTestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency2.jsTestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency3.jsTestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency4.jsTestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency5.jsTestRunner/app/tests/shared/Require/FiveLevelsOfRequire/index.jsTestRunner/app/tests/shared/Require/GlobalIsDefined/module.jsTestRunner/app/tests/shared/Require/GlobalRequire/index.jsTestRunner/app/tests/shared/Require/ModuleErrorCache/index.jsTestRunner/app/tests/shared/Require/ModuleErrorCache/maybeThrow.jsTestRunner/app/tests/shared/Require/ModuleRequireFunction/module1.jsTestRunner/app/tests/shared/Require/ModuleRequireFunction/module2.jsTestRunner/app/tests/shared/Require/ModuleVariable/dependency1.jsTestRunner/app/tests/shared/Require/ModuleVariable/dependency2.jsTestRunner/app/tests/shared/Require/ModuleVariable/dependency3.jsTestRunner/app/tests/shared/Require/ModuleVariable/dependency4.jsTestRunner/app/tests/shared/Require/ModuleVariable/dependency5.jsTestRunner/app/tests/shared/Require/ModuleVariable/index.jsTestRunner/app/tests/shared/Require/ModuleWith.js/index.jsTestRunner/app/tests/shared/Require/ModuleWithNullChar/index.jsTestRunner/app/tests/shared/Require/MultipleRequireOfAFile/index.jsTestRunner/app/tests/shared/Require/MultipleRequireOfAFile/module.jsTestRunner/app/tests/shared/Require/NestedGlobalObject/dependency1.jsTestRunner/app/tests/shared/Require/NestedGlobalObject/dependency2.jsTestRunner/app/tests/shared/Require/NestedGlobalObject/dependency3.jsTestRunner/app/tests/shared/Require/NestedGlobalObject/dependency4.jsTestRunner/app/tests/shared/Require/NestedGlobalObject/dependency5.jsTestRunner/app/tests/shared/Require/NestedGlobalObject/index.jsTestRunner/app/tests/shared/Require/NotExistingFileRequire/index.jsTestRunner/app/tests/shared/Require/PackageJsonApp/index.jsTestRunner/app/tests/shared/Require/PackageJsonApp/io/io.jsTestRunner/app/tests/shared/Require/PackageJsonApp/io/package.jsonTestRunner/app/tests/shared/Require/PackageJsonAppNoMain/index.jsTestRunner/app/tests/shared/Require/PackageJsonAppNoMain/io/index.jsTestRunner/app/tests/shared/Require/PackageJsonAppNoMain/io/package.jsonTestRunner/app/tests/shared/Require/PackageJsonAppWithoutExtension/index.jsTestRunner/app/tests/shared/Require/PackageJsonAppWithoutExtension/io/io.jsTestRunner/app/tests/shared/Require/PackageJsonAppWithoutExtension/io/package.jsonTestRunner/app/tests/shared/Require/PackageJsonMainPointsToDir/package.jsonTestRunner/app/tests/shared/Require/PackageJsonMainPointsToDir/subdirectory/index.jsTestRunner/app/tests/shared/Require/PackageJsonSyntaxError/index.jsTestRunner/app/tests/shared/Require/PackageJsonSyntaxError/io/index.jsTestRunner/app/tests/shared/Require/PackageJsonSyntaxError/io/package.jsonTestRunner/app/tests/shared/Require/PackageJsonTns/index.jsTestRunner/app/tests/shared/Require/RequireExtensions/dependency1.jsTestRunner/app/tests/shared/Require/RequireExtensions/index.jsTestRunner/app/tests/shared/Require/RequireJsonCorruptFile/index.jsTestRunner/app/tests/shared/Require/RequireJsonCorruptFile/test.jsonTestRunner/app/tests/shared/Require/RequireJsonCorruptFile1/badJSON.jsonTestRunner/app/tests/shared/Require/RequireJsonCorruptFile1/index.jsTestRunner/app/tests/shared/Require/RequireJsonFile/index.jsTestRunner/app/tests/shared/Require/RequireJsonFile/test.jsonTestRunner/app/tests/shared/Require/RequireModuleFolder/index.jsTestRunner/app/tests/shared/Require/RequireModuleFolderConflict/dependency.jsTestRunner/app/tests/shared/Require/RequireModuleFolderConflict/index.jsTestRunner/app/tests/shared/Require/RequirePriority/dependency1.jsTestRunner/app/tests/shared/Require/RequirePriority/dependency2.jsTestRunner/app/tests/shared/Require/RequirePriority/dependency3/index.jsTestRunner/app/tests/shared/Require/RequirePriority/dependency4.jsTestRunner/app/tests/shared/Require/RequirePriority/dependency5/index.jsTestRunner/app/tests/shared/Require/RequirePriority/index.jsTestRunner/app/tests/shared/Require/RequireWithTildeSyntax/module.jsTestRunner/app/tests/shared/Require/ResolveCanonicalPath/index.jsTestRunner/app/tests/shared/Require/ResolveCanonicalPath/package.jsonTestRunner/app/tests/shared/Require/ResolveCanonicalPath/second.jsTestRunner/app/tests/shared/Require/RuntimeErrorInModule/dependency.jsTestRunner/app/tests/shared/Require/RuntimeErrorInModule/index.jsTestRunner/app/tests/shared/Require/SimpleGlobalObject/dependency.jsTestRunner/app/tests/shared/Require/SimpleGlobalObject/index.jsTestRunner/app/tests/shared/Require/SimpleRequire/dependency.jsTestRunner/app/tests/shared/Require/SimpleRequire/index.jsTestRunner/app/tests/shared/Require/SimpleRequireWithExports/dependency.jsTestRunner/app/tests/shared/Require/SimpleRequireWithExports/index.jsTestRunner/app/tests/shared/Require/SyntaxErrorInModule/dependency.jsTestRunner/app/tests/shared/Require/SyntaxErrorInModule/index.jsTestRunner/app/tests/shared/Require/TwoLevelsOfRequire/dependency1.jsTestRunner/app/tests/shared/Require/TwoLevelsOfRequire/dependency2.jsTestRunner/app/tests/shared/Require/TwoLevelsOfRequire/index.jsTestRunner/app/tests/shared/Require/TwoLevelsOfRequireWithExports/dependency1.jsTestRunner/app/tests/shared/Require/TwoLevelsOfRequireWithExports/dependency2.jsTestRunner/app/tests/shared/Require/TwoLevelsOfRequireWithExports/index.jsTestRunner/app/tests/shared/Require/index.jsTestRunner/app/tests/shared/RuntimeTests.jsTestRunner/app/tests/shared/WeakRef.jsTestRunner/app/tests/shared/Workers/EvalWorker.jsTestRunner/app/tests/shared/Workers/WorkerInvalidSyntax.jsTestRunner/app/tests/shared/Workers/WorkerStressJSTest.jsTestRunner/app/tests/shared/Workers/index.jsTestRunner/app/tests/shared/index.jsTestRunner/app/tns_modules/tns-core-modules/shared/package.json
💤 Files with no reviewable changes (130)
- TestRunner/app/tests/shared/Require/FileAndDirectoryWithSameName/index.js
- TestRunner/app/tests/shared/Require/CircularRequire/dependency3.js
- TestRunner/app/tests/shared/Import/ImportJSON/data.json
- TestRunner/app/tests/shared/Require/RequireJsonCorruptFile1/badJSON.json
- TestRunner/app/tests/shared/Require/FileAndDirectoryWithSameName/module/submodule.js
- TestRunner/app/tests/shared/Require/PackageJsonAppWithoutExtension/io/package.json
- TestRunner/app/tests/shared/Require/ResolveCanonicalPath/second.js
- TestRunner/app/tests/shared/README.md
- TestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency4.js
- TestRunner/app/tests/shared/Require/AbsolutePath/index.js
- TestRunner/app/tests/shared/Require/SimpleRequire/dependency.js
- TestRunner/app/tests/shared/Require/SyntaxErrorInModule/dependency.js
- TestRunner/app/tests/shared/Require/ChangingRequireObject/index.js
- TestRunner/app/tests/shared/Require/GlobalIsDefined/module.js
- TestRunner/app/tests/shared/Workers/EvalWorker.js
- TestRunner/app/tests/shared/Require/PackageJsonSyntaxError/io/index.js
- TestRunner/app/tests/shared/Require/TwoLevelsOfRequireWithExports/index.js
- TestRunner/app/tests/shared/Import/ImportCommonJS/index.js
- TestRunner/app/tests/shared/RuntimeTests.js
- TestRunner/app/tests/shared/Require/DirectoryWithIndexJson/module/index.json
- TestRunner/app/tests/shared/Require/DirnameAndFilenameParameters/dep2/dep2-inner/index.js
- TestRunner/app/tests/shared/Require/CircularRequire/dependency2.js
- TestRunner/app/tests/shared/Require/PackageJsonAppWithoutExtension/io/io.js
- TestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency2.js
- TestRunner/app/tests/shared/Require/FileWithDots/file.name.js
- TestRunner/app/tests/shared/Require/CircularRequire/index.js
- TestRunner/app/tests/shared/Require/RequirePriority/dependency2.js
- TestRunner/app/tests/shared/Require/TwoLevelsOfRequire/index.js
- TestRunner/app/tests/shared/Require/RequireJsonCorruptFile/index.js
- TestRunner/app/tests/shared/Import/ImportJSON/index.js
- TestRunner/app/tests/shared/Require/AbsolutePath/dependency.js
- TestRunner/app/tests/shared/Require/CircularRequireWithExports/dependency3.js
- TestRunner/app/tests/shared/Require/ExportsBubbling/dependency3.js
- TestRunner/app/tests/shared/Require/CaseSensitive/index.js
- TestRunner/app/tests/shared/Require/NotExistingFileRequire/index.js
- TestRunner/app/tests/shared/Require/PackageJsonAppNoMain/io/index.js
- TestRunner/app/tests/shared/Require/PackageJsonTns/index.js
- TestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency1.js
- TestRunner/app/tests/shared/Require/SimpleGlobalObject/dependency.js
- TestRunner/app/tests/shared/Require/TwoLevelsOfRequire/dependency2.js
- TestRunner/app/tests/shared/Require/CircularRequireWithExports/dependency2.js
- TestRunner/app/tests/shared/Require/ExportsBubbling/dependency4.js
- TestRunner/app/tests/shared/Require/GlobalRequire/index.js
- TestRunner/app/tests/shared/Require/index.js
- TestRunner/app/tests/shared/Require/SyntaxErrorInModule/index.js
- TestRunner/app/tests/shared/Require/ModuleVariable/dependency4.js
- TestRunner/app/tests/shared/Require/FileAndDirectoryWithSameName/module.js
- TestRunner/app/tests/shared/Require/CircularIndexRequire/index.js
- TestRunner/app/tests/shared/Require/RequireExtensions/dependency1.js
- TestRunner/app/tests/shared/Require/RequireModuleFolderConflict/index.js
- TestRunner/app/tests/shared/Require/SimpleRequire/index.js
- TestRunner/app/tests/shared/Require/PackageJsonAppWithoutExtension/index.js
- TestRunner/app/tests/shared/Require/RequireJsonFile/index.js
- TestRunner/app/tests/shared/Require/ModuleErrorCache/index.js
- TestRunner/app/tests/shared/Require/RequirePriority/dependency5/index.js
- TestRunner/app/tests/shared/Require/CaseSensitive/mymodule/index.js
- TestRunner/app/tests/shared/Require/RequirePriority/dependency4.js
- TestRunner/app/tests/shared/Require/RequireJsonCorruptFile/test.json
- TestRunner/app/tests/shared/Require/PackageJsonMainPointsToDir/subdirectory/index.js
- TestRunner/app/tests/shared/Require/ModuleErrorCache/maybeThrow.js
- TestRunner/app/tests/shared/Workers/WorkerInvalidSyntax.js
- TestRunner/app/tests/shared/Require/FileWithDots/index.js
- TestRunner/app/tests/shared/Require/DirnameAndFilenameParameters/index.js
- TestRunner/app/tests/shared/Require/PackageJsonApp/io/io.js
- TestRunner/app/tests/shared/Require/CircularRequireWithExports/dependency1.js
- TestRunner/app/tests/shared/Require/PackageJsonSyntaxError/index.js
- TestRunner/app/tests/shared/Require/CaseSensitive/MyModule.js
- TestRunner/app/tests/shared/Require/CircularRequireWithExports/index.js
- TestRunner/app/tests/shared/Require/ModuleVariable/dependency3.js
- TestRunner/app/tests/shared/Require/ModuleRequireFunction/module1.js
- TestRunner/app/tests/shared/.gitattributes
- TestRunner/app/tests/shared/Require/FiveLevelsOfRequire/index.js
- TestRunner/app/tests/shared/Require/SimpleRequireWithExports/dependency.js
- TestRunner/app/tests/shared/Require/CircularIndexRequire/dependency2.js
- TestRunner/app/tests/shared/Require/RequireWithTildeSyntax/module.js
- TestRunner/app/tests/shared/Require/FileEndingWithCommentedLine/index.js
- TestRunner/app/tests/shared/Require/NestedGlobalObject/dependency2.js
- TestRunner/app/tests/shared/Require/RuntimeErrorInModule/index.js
- TestRunner/app/tests/shared/Require/RequireModuleFolderConflict/dependency.js
- TestRunner/app/tests/shared/Require/RequireJsonCorruptFile1/index.js
- TestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency5.js
- TestRunner/app/tests/shared/Require/FiveLevelsOfRequire/dependency3.js
- TestRunner/app/tests/shared/Require/ModuleVariable/dependency5.js
- TestRunner/app/tests/shared/Require/ExportsBubbling/dependency1.js
- TestRunner/app/tests/shared/Require/NestedGlobalObject/dependency3.js
- TestRunner/app/tests/shared/Require/NestedGlobalObject/index.js
- TestRunner/app/tests/shared/Require/ModuleVariable/index.js
- TestRunner/app/tests/shared/Require/RequirePriority/dependency3/index.js
- TestRunner/app/tests/shared/Require/DirnameAndFilenameParameters/dep1/dependency1.js
- TestRunner/app/tests/shared/Require/TwoLevelsOfRequire/dependency1.js
- TestRunner/app/tests/shared/Require/RequireJsonFile/test.json
- TestRunner/app/tests/shared/Require/CircularIndexRequire/dependency1.js
- TestRunner/app/tests/shared/Require/ExportsBubbling/dependency2.js
- TestRunner/app/tests/shared/Require/RequirePriority/index.js
- TestRunner/app/tests/shared/Require/MultipleRequireOfAFile/module.js
- TestRunner/app/tests/shared/Require/TwoLevelsOfRequireWithExports/dependency2.js
- TestRunner/app/tests/shared/Require/ExportsBubbling/index.js
- TestRunner/app/tests/shared/Require/NestedGlobalObject/dependency5.js
- TestRunner/app/tests/shared/index.js
- TestRunner/app/tests/shared/Require/ModuleRequireFunction/module2.js
- TestRunner/app/tests/shared/Require/RequirePriority/dependency1.js
- TestRunner/app/tests/shared/Import/index.js
- TestRunner/app/tests/shared/Require/RuntimeErrorInModule/dependency.js
- TestRunner/app/tests/shared/Require/SimpleRequireWithExports/index.js
- TestRunner/app/tests/shared/WeakRef.js
- TestRunner/app/tests/shared/Require/ChangingRequireObject/dependency.js
- TestRunner/app/tests/shared/Require/PackageJsonApp/index.js
- TestRunner/app/tests/shared/Require/RequireExtensions/index.js
- TestRunner/app/tests/shared/Require/ModuleVariable/dependency1.js
- TestRunner/app/tests/shared/Require/PackageJsonApp/io/package.json
- TestRunner/app/tests/shared/Require/RequireModuleFolder/index.js
- TestRunner/app/tests/shared/Workers/index.js
- TestRunner/app/tests/shared/Require/DirectoryWithIndexJson/index.js
- TestRunner/app/tests/shared/Require/ModuleVariable/dependency2.js
- TestRunner/app/tests/shared/Require/PackageJsonMainPointsToDir/package.json
- TestRunner/app/tests/shared/Import/ImportCommonJS/file.js
- TestRunner/app/tests/shared/Require/TwoLevelsOfRequireWithExports/dependency1.js
- TestRunner/app/tests/shared/Require/ModuleWith.js/index.js
- TestRunner/app/tests/shared/Require/NestedGlobalObject/dependency1.js
- TestRunner/app/tests/shared/Require/PackageJsonSyntaxError/io/package.json
- TestRunner/app/tests/shared/Require/MultipleRequireOfAFile/index.js
- TestRunner/app/tests/shared/Require/NestedGlobalObject/dependency4.js
- TestRunner/app/tests/shared/Require/PackageJsonAppNoMain/index.js
- TestRunner/app/tests/shared/Require/SimpleGlobalObject/index.js
- TestRunner/app/tests/shared/Require/ExportsBubbling/dependency5.js
- TestRunner/app/tests/shared/Require/PackageJsonAppNoMain/io/package.json
- TestRunner/app/tests/shared/Require/CircularRequire/dependency1.js
- TestRunner/app/tests/shared/Workers/WorkerStressJSTest.js
- TestRunner/app/tests/shared/Require/ResolveCanonicalPath/index.js
- TestRunner/app/tests/shared/Require/ResolveCanonicalPath/package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
Three commits that should land as-is (see merge instructions below):
fix: release our own blocks and release instead of dealloc dataTwo object-lifetime bugs in the runtime, both use-after-free classes:
__releaseNativeCounterpartcalled[data dealloc]directly, destroying natives thatwere still referenced elsewhere. The shared worker teardown test caught this: the
NSNotificationCenterobserver token (__NSObserver) returned byaddObserverForName:object:queue:usingBlock:was force-deallocated while the mainthread had an in-flight notification post referencing it →
EXC_BAD_ACCESSinobjc_releaseunder_CFXRegistrationPost. Now it does a proper[data release]: instances solely owned by JS (alloc().init(),new) stilldeallocate immediately (the
__releaseNativeCounterpartAPI tests assert this),shared natives survive until their remaining owners release them.
ObjectManager::DisposeValueraw-free()d block objects it did not own(blocks created from JS callbacks and handed to native code), bypassing the ObjC
refcount and the block's dispose helper. Now:
CFReleaseonly for blocks the wrapperowns (balancing the
CFRetaintaken when a native block is wrapped for JS — this alsofixes a leak), and hands-off for native-owned blocks, whose
JSBlockdispose helperruns when the last native reference goes away.
fix(worker): worker path resolution parity with android and error handling for missing scriptsnew Worker("./script.js")now resolves relative to the calling module's directory,matching the Android runtime and the legacy JSC iOS runtime (the documented behavior).
If nothing exists there, it falls back to the historical app-root-relative resolution,
so existing apps keep working.
worker.onerrorinstead of silently never starting (closes the old
TODOin the constructor).test: use shared tests across android and ios(
TestRunner/app/tests/shared) with a git submodule ofcommon-runtime-tests-app
mounted at
TestRunner/app/shared— the same mount point android-runtime uses, and thelocation the tests' own path assertions require.
Infrastructure/fromapp/tests/toapp/so worker scripts can reach../../Infrastructure/timers, again matching the android test app layout.submodules: trueto the checkout steps of the build and test jobs inpull_request.ymlandnpm_release.ymlso CI fetches the submodule (this also coversthe pre-existing
libffisubmodule).guards (
global.NSObject && !global.TNSRuntime) and merged to the shared repo'smaster, together with new Android worker tests and unified structured-clone tests.Android behavior is unchanged by the guards; both runtimes now run the same suite.
Why
The vendored test copy had drifted so far from upstream (path rewrites, deleted tests,
weakened assertions) that tests added on one runtime never ran on the other. Restoring the
shared suite immediately paid off: the upstream teardown test — which had never run on
v8ios — exposed the
[data dealloc]use-after-free fixed in commit 1, and the upstreamworker tests exposed the path-resolution divergence fixed in commit 2.
Verification
Full TestRunner suite green on iPad Pro 13-inch (M4) simulator, iOS 18.5: 781 passed, 0
failed, including the previously-crashing "no crash during or after runtime teardown on
iOS" stress test and the new structured-clone round-trip tests (circular references with
identity preservation,
DataCloneErrorfor functions, Date/RegExp/Map/Set/TypedArray/BigInt/
undefined).Rebase-merge this PR on top of
main— do NOT squash.The three commits are deliberately separate: two independent runtime fixes and one test
restructure. Squashing would bury two unrelated bugfixes inside a 140+-file test-layout
change and make them impossible to revert or bisect individually.
Notes for reviewers
git submodule update --initforTestRunner/app/shared(
.gitmodulestracks the shared repo'smaster); CI does this automatically viasubmodules: trueon checkout.__releaseNativeCounterpartsemantics change from "force dealloc" to "release ourstrong reference". The existing API tests still pass because JS-owned objects hit
refcount 0 on that release; code relying on force-destroying a native that other native
code still references was crashing anyway.
the fallback keeps old app-root-relative usage working.
Summary by CodeRabbit
Bug Fixes
Chores