src: add Atomics.notify alias

[devsnek]

Refs: #21219

/cc @rwaldron

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/101/pipeline

[addaleax]

maybe add a test for vm as well?

[leobalter]

To prevent any sort of hazard, I wonder if Atomics.notify should be preserved if it already exists from V8, this way it's not necessary to clone anything from Atomics.wake.

[devsnek]

@leobalter when we update v8 to a version that has Atomics.notify we can just remove this change. i would prefer that actually so we don't have dead code hanging around.

[leobalter]

@devsnek I'm a newbie wrt node ecosystem, if this can be done, I'm +1. Having Atomics.notify here is great anyway.

[joyeecheung]

Does this actually help with the issue raised in #21219 if Atomics.wake is still available?

that no new code featuring Atomics.wake has been written since the Spectre/Meltdown mitigations.

At least if I go to MDN to look for docs I'll still see Atomics.wake and nothing about Atomics.notify so if I were not aware of that issue I'll still use Atomics.wake. Maybe a warning for accessing Atomics.wake could help, but I am not sure if we step over the line regarding tampering with ECMAScript features if we do that

[joyeecheung]

Is there a reason these lines got moved?

[devsnek]

just a side-effect of the changes i was making before i finished up. should i change it back? they kinda make the order better anyway.

[devsnek]

@joyeecheung i wanted to add warnings and stuff but since we have to do it in c++ i don't know how viable it is. maybe we could move the intl and atomics stuff into a js file somewhere that's only called when making a new context. iirc v8_extras also run per-context so those could help too.

[devsnek]

@nodejs/v8 out of curiosity i tried to implement this with v8 extras but any attempt to access or set properties on global.Atomics results in mksnapshot failing. is this a bug or something else? i can't find any other global objects where messing with them fails.

[hashseed]

Do you have any backtrace from a debug build? I suspect that some external references are missing.

[devsnek]

@hashseed i don't have the power atm to fully recompile with debug but i've got

  LD_LIBRARY_PATH=/Users/gus/Desktop/misc/nodejs/node/out/Release/lib.host:/Users/gus/Desktop/misc/nodejs/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/gypfiles; mkdir -p /Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni; "/Users/gus/Desktop/misc/nodejs/node/out/Release/mksnapshot" --startup_src "/Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni/snapshot.cc" ""
/bin/sh: line 1: 54182 Segmentation fault: 11  "/Users/gus/Desktop/misc/nodejs/node/out/Release/mksnapshot" --startup_src "/Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni/snapshot.cc" ""
make[1]: *** [/Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni/snapshot.cc] Error 139

also running mksnapshot (which iirc should show a help message?):

(lldb) run
Process 54499 launched: 'node/out/Release/mksnapshot' (x86_64)
mksnapshot was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 54499 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100007928 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::MemoryChunk::FromAddress(a=<unavailable>) at spaces.h:410 [opt]
   407
   408 	  // Only works if the pointer is in the first kPageSize of the MemoryChunk.
   409 	  static MemoryChunk* FromAddress(Address a) {
-> 410 	    return reinterpret_cast<MemoryChunk*>(OffsetFrom(a) & ~kAlignmentMask);
   411 	  }
   412
   413 	  static inline MemoryChunk* FromAnyPointerAddress(Heap* heap, Address addr);
Target 0: (mksnapshot) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100007928 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::MemoryChunk::FromAddress(a=<unavailable>) at spaces.h:410 [opt]
    frame #1: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::HeapObject::GetHeap(this=<unavailable>) const at objects-inl.h:957 [opt]
    frame #2: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::HeapObject::GetIsolate(this=<unavailable>) const at objects-inl.h:966 [opt]
    frame #3: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::Context::GetIsolate(this=0x0000000000000000) at api.cc:6432 [opt]
    frame #4: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(this=0x00007ffeefbff758, context=<unavailable>, callback=SerializeInternalFieldsCallback @ 0x00007f99804143c0) at api.cc:583 [opt]
    frame #5: 0x0000000100001995 mksnapshot`main at mksnapshot.cc:299 [opt]
    frame #6: 0x000000010000192b mksnapshot`main(argc=1, argv=<unavailable>) at mksnapshot.cc:409 [opt]
    frame #7: 0x00000001000017e4 mksnapshot`start + 52
(lldb) ^D

[hashseed]

I think you introduced some code that caused bootstrapping (creating a context from scratch) to fail. It then returns an empty handle, so that using it as a default context in the snapshot causes a crash.

[devsnek]

@hashseed here's the script but the repro is as simple as (function (global, binding, v8) { global.Atomics.wake; })

https://gist.github.com/devsnek/3679af4029594f31a62b70e35ed035d9

[hashseed]

That's because Atomics are an experimental feature and not available for contexts intended for a snapshot.

[devsnek]

@hashseed is that the kind of thing where a flag can be flipped or do we just have to wait and use another solution for now.

[hashseed]

I don't think there is a good solution currently. By default, V8 ships without Atomics right now. Requiring them to create a snapshot fails as intended. Flipping --harmony-sharedarraybuffer does not help because we don't enable any experimental features to create the snapshot even if they are enabled by default. That way you preserve the ability to disable them at runtime.

So at this point adding it to the snapshot is not really an option, unfortunately.

[devsnek]

@joyeecheung @addaleax @hashseed this works but is it a good idea? i'm having trouble weighting like the pros and cons here. the diff below lets us set the function name and warn and stuff but i don't know if it causes a slow-down to call it all on every new context created, especially since we can't create a cache for it.

diff --git a/lib/internal/per_context.js b/lib/internal/per_context.js
new file mode 100644
index 0000000000..3dce7f188b
--- /dev/null
+++ b/lib/internal/per_context.js
@@ -0,0 +1,52 @@
+'use strict';
+
+// node::NewContext calls this script
+
+(function(global, binding, v8) {
+  // https://github.com/nodejs/node/issues/14909
+  delete global.Intl.v8BreakIterator;
+
+  // https://github.com/nodejs/node/issues/21219
+  // Adds Atomics.notify and warns on first usage of Atomics.wake
+
+  const AtomicsWake = global.Atomics.wake;
+  const ReflectApply = global.Reflect.apply;
+
+  // wrap for function.name
+  function notify(...args) {
+    return ReflectApply(AtomicsWake, this, args);
+  }
+
+  const warning = 'Atomics.wake will be removed in a future version, ' +
+    'use Atomics.notify instead.';
+
+  let wakeWarned = false;
+  function wake(...args) {
+    if (!wakeWarned) {
+      wakeWarned = true;
+
+      if (global.process !== undefined) {
+        global.process.emitWarning(warning, 'Atomics');
+      } else {
+        global.console.error(`Atomics: ${warning}`);
+      }
+    }
+
+    return ReflectApply(AtomicsWake, this, args);
+  }
+
+  global.Object.defineProperties(global.Atomics, {
+    notify: {
+      value: notify,
+      enumerable: false,
+      configurable: true,
+      writable: true,
+    },
+    wake: {
+      value: wake,
+      enumerable: false,
+      configurable: true,
+      writable: true,
+    },
+  });
+}(this));
diff --git a/node.gyp b/node.gyp
index 301a0e20a1..4f70ee5f29 100644
--- a/node.gyp
+++ b/node.gyp
@@ -24,6 +24,7 @@
     'node_lib_target_name%': 'node_lib',
     'node_intermediate_lib_type%': 'static_library',
     'library_files': [
+      'lib/internal/per_context.js',
       'lib/internal/bootstrap/loaders.js',
       'lib/internal/bootstrap/node.js',
       'lib/async_hooks.js',
diff --git a/src/node.cc b/src/node.cc
index 6cf5f2ad84..70c5a132cc 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -3478,35 +3478,21 @@ Local<Context> NewContext(Isolate* isolate,
   auto context = Context::New(isolate, nullptr, object_template);
   if (context.IsEmpty()) return context;
   HandleScope handle_scope(isolate);
+
   context->SetEmbedderData(
       ContextEmbedderIndex::kAllowWasmCodeGeneration, True(isolate));

-  auto intl_key = FIXED_ONE_BYTE_STRING(isolate, "Intl");
-  auto break_iter_key = FIXED_ONE_BYTE_STRING(isolate, "v8BreakIterator");
-  Local<Value> intl_v;
-  if (context->Global()->Get(context, intl_key).ToLocal(&intl_v) &&
-      intl_v->IsObject()) {
-    Local<Object> intl = intl_v.As<Object>();
-    intl->Delete(context, break_iter_key).FromJust();
-  }
-
-  // https://github.com/nodejs/node/issues/21219
-  // TODO(devsnek): remove when v8 supports Atomics.notify
-  auto atomics_key = FIXED_ONE_BYTE_STRING(isolate, "Atomics");
-  Local<Value> atomics_v;
-  if (context->Global()->Get(context, atomics_key).ToLocal(&atomics_v) &&
-      atomics_v->IsObject()) {
-    Local<Object> atomics = atomics_v.As<Object>();
-    auto wake_key = FIXED_ONE_BYTE_STRING(isolate, "wake");
-
-    Local<Value> wake = atomics->Get(context, wake_key).ToLocalChecked();
-    auto notify_key = FIXED_ONE_BYTE_STRING(isolate, "notify");
-
-    v8::PropertyDescriptor desc(wake, true);
-    desc.set_enumerable(false);
-    desc.set_configurable(true);
-
-    atomics->DefineProperty(context, notify_key, desc).ToChecked();
+  {
+    // Run lib/internal/per_context.js
+    Context::Scope context_scope(context);
+    Local<String> per_context = NodePerContextSource(isolate);
+    v8::ScriptCompiler::Source per_context_src(per_context, nullptr);
+    Local<v8::Script> s = v8::ScriptCompiler::Compile(
+        context,
+        &per_context_src,
+        v8::ScriptCompiler::kNoCompileOptions,
+        v8::ScriptCompiler::kNoCacheBecauseCachingDisabled).ToLocalChecked();
+    s->Run(context).ToLocalChecked();
   }

   return context;
diff --git a/src/node.h b/src/node.h
index 90b2a8cbf6..b551c8bb09 100644
--- a/src/node.h
+++ b/src/node.h
@@ -241,9 +241,7 @@ class MultiIsolatePlatform : public v8::Platform {
 // Creates a new isolate with Node.js-specific settings.
 NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator);

-// Creates a new context with Node.js-specific tweaks.  Currently, it removes
-// the `v8BreakIterator` property from the global `Intl` object if present.
-// See https://github.com/nodejs/node/issues/14909 for more info.
+// Creates a new context with Node.js-specific tweaks.
 NODE_EXTERN v8::Local<v8::Context> NewContext(
     v8::Isolate* isolate,
     v8::Local<v8::ObjectTemplate> object_template =
diff --git a/src/node_javascript.h b/src/node_javascript.h
index d1ad9a2574..00cdc0c0b6 100644
--- a/src/node_javascript.h
+++ b/src/node_javascript.h
@@ -29,6 +29,7 @@
 namespace node {

 void DefineJavaScript(Environment* env, v8::Local<v8::Object> target);
+v8::Local<v8::String> NodePerContextSource(v8::Isolate* isolate);
 v8::Local<v8::String> LoadersBootstrapperSource(Environment* env);
 v8::Local<v8::String> NodeBootstrapperSource(Environment* env);

diff --git a/tools/js2c.py b/tools/js2c.py
index 38cf39f816..249df58085 100755
--- a/tools/js2c.py
+++ b/tools/js2c.py
@@ -189,6 +189,10 @@ namespace {{

 }}  // anonymous namespace

+v8::Local<v8::String> NodePerContextSource(v8::Isolate* isolate) {{
+  return internal_per_context_value.ToStringChecked(isolate);
+}}
+
 v8::Local<v8::String> LoadersBootstrapperSource(Environment* env) {{
   return internal_bootstrap_loaders_value.ToStringChecked(env->isolate());
 }}

[addaleax]

@devsnek I’d stick to the current approach for this PR

Also, if you want to pursue the extras approach for later, maybe have a look at ayojs/ayo@84fc9ce ? /cc @TimothyGu

[devsnek]

CI https://ci.nodejs.org/job/node-test-pull-request/15543/

[devsnek]

this needs another approval

[rwaldron]

@joyeecheung I'm on the MDN-PAB and moderate all the JS docs :) I started planning for the Atomics.wake => Atomics.notify docs change/update earlier this week.

[rwaldron]

@hashseed re:

By default, V8 ships without Atomics right now.

I'm not sure I understand this, but that's likely because I missing some important context. If V8 wasn't shipping Atomics by default, and Atomics wasn't available in Node.js, then I wouldn't have any concerns about the urgency of renaming Atomics.wake in the presence of a Worker API, however:

$ nvm use 8
Now using node v8.11.3 (npm v5.6.0)

$ node -e "console.log(process.versions.v8);"
6.2.414.54

$ node -e "console.log(Atomics.wake);"
[Function: wake]

$ nvm use 10
Now using node v10.5.0 (npm v6.1.0)

$ node -e "console.log(process.versions.v8);"
6.7.288.46-node.8

$ node -e "console.log(Atomics.wake);"
[Function: wake]

Flipping --harmony-sharedarraybuffer does not help because we don't enable any experimental features to create the snapshot even if they are enabled by default.

Again, I think I'm missing some important context, so please correct as necessary—but --harmony-sharedarraybuffer is on by default ¯\_(ツ)_/¯

(Also, I noted just now that you were the last person to commit to that file!)

[devsnek]

@rwaldron so basically since Atomics can be disabled at runtime (i'm not sure on the defaults there) they can't make memory snapshots that use Atomics because it might not exist, in which case the snapshot is invalid. i do agree that i'm confused about atomics being disabled by default, as i think its the other way around.

[hashseed]

Yup. What @devsnek said.

[rwaldron]

@devsnek @hashseed thanks for the additional info and your patience!

[devsnek]

landed in 84f3769

[targos]

atomis (or atomics) isn't a subsystem of Node. This should have been src: add Atomics.notify alias (not a big deal).