Dynamic `import` attributes broken with multiple attributes

[targos]

I was trying to adapt a V8 test to run it in Node.js

The following version fails in a C++ CHECK (it doesn't matter if the json file exists or not):

import assert from 'assert';

var life;
import('./modules-skip-1.json', { with: { type: 'json', notARealAssertion: 'value' } }).then(
 namespace => life = namespace.default.life).then(() => {
 assert.strictEqual(life, 42);
 });

var life2;
import('./modules-skip-1.json', { with: { 0: 'value', type: 'json' } }).then(
 namespace => life2 = namespace.default.life).then(() => {
 assert.strictEqual(life2, 42);
 });

#
# Fatal error in , line 0
# Check failed: i < self->length().
#
#
#
#FailureMessage Object: 0x16d934a68
 1: 0x1025fad74 node::NodePlatform::GetStackTracePrinter()::$_3::__invoke() [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 2: 0x1036dbcc4 V8_Fatal(char const*, ...) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 3: 0x10273d8bc v8::FixedArray::Get(v8::Local<v8::Context>, int) const [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 4: 0x10255ad78 node::loader::createImportAttributesContainer(node::Environment*, v8::Isolate*, v8::Local<v8::FixedArray>) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 5: 0x10255c3d8 node::loader::ImportModuleDynamically(v8::Local<v8::Context>, v8::Local<v8::Data>, v8::Local<v8::Value>, v8::Local<v8::String>, v8::Local<v8::FixedArray>) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 6: 0x102895ab0 v8::internal::Isolate::RunHostImportModuleDynamicallyCallback(v8::internal::MaybeHandle<v8::internal::Script>, v8::internal::Handle<v8::internal::Object>, v8::internal::MaybeHandle<v8::internal::Object>) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 7: 0x102cc3670 v8::internal::Runtime_DynamicImportCall(int, unsigned long*, v8::internal::Isolate*) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 8: 0x103057768 Builtins_CEntry_Return1_ArgvInRegister_NoBuiltinExit [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]

This happens in Node.js 21 and v20.x-staging.

[targos]

I thought this would be easy to fix when I saw

node/src/module_wrap.cc

Line 256 in 0304da2

for (int i = 0; i < raw_attributes->Length(); i += 3){

But this is not just an off-by-one issue.

For import(jsModuleDataUrl,{with:{type: 'json', other: 'unsupported' } }) (test I added), the raw_attributes array contains 4 elements.

For import(`data:text/javascript,import${JSON.stringify(jsModuleDataUrl)}with{type:"json"}`) (existing test), it contains 3 elements.

[targos]

I don't understand how it's possible. The array is guaranteed to have a size that's a multiple of two:

node/deps/v8/src/execution/isolate.cc

Lines 5446 to 5448 in 0304da2

	constexpr size_t kAssertionEntrySizeForDynamicImport = 2;
	import_assertions_array = factory()->NewFixedArray(static_cast<int>(
	assertion_keys->length() * kAssertionEntrySizeForDynamicImport));

[targos]

Debug build makes it a bit more understandable:

$ ./node_g test/es-module/test-esm-import-attributes-errors.mjs
FATAL ERROR: v8::String::Cast Value is not a String
----- Native stack trace -----

 1: 0x100645944 node::DumpNativeBacktrace(__sFILE*) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 2: 0x100784638 node::Abort() [/Users/mzasso/git/nodejs/node/out/Debug/node]
 3: 0x100784798 node::OOMErrorHandler(char const*, v8::OOMDetails const&) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 4: 0x100b344fc v8::String::CheckCast(v8::Data*) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 5: 0x1005fd248 v8::String::Cast(v8::Data*) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 6: 0x1006ed5e8 v8::Local<v8::String> v8::Local<v8::String>::Cast<v8::Data>(v8::Local<v8::Data>) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 7: 0x1006ed590 v8::Local<v8::String> v8::Local<v8::Data>::As<v8::String>() const [/Users/mzasso/git/nodejs/node/out/Debug/node]
 8: 0x1006ea2dc node::loader::createImportAttributesContainer(node::Environment*, v8::Isolate*, v8::Local<v8::FixedArray>) [/Users/mzasso/git/nodejs/node/out/Debug/node]

[targos]

Found the issue: #50703