From 3980026708589eb896c016f899fc81adcb968f2a Mon Sep 17 00:00:00 2001
From: Yael Hermon <yaelhe@wix.com>
Date: Fri, 4 Jan 2019 20:02:25 +0200
Subject: [PATCH 1/3] worker: enable passing command line flags

This PR adds the ability to provide Workers with their own
execArgv flags in replacement of the main thread's execArgv. Only
per-Isolate/per-Environment options are allowed. Per-Process options
and V8 flags are not allowed. Passing an empty execArgv array will
reset per-Isolate and per-Environment options of the Worker to their
defaults. If execArgv option is not passed, the Worker will get
the same flags as the main thread.

Usage example:
```
const worker = new Worker(__filename, {
    execArgv: ['--trace-warnings'],
});
```
---
 doc/api/errors.md                             |  6 ++
 doc/api/worker_threads.md                     |  7 +-
 lib/internal/errors.js                        |  3 +
 lib/internal/validators.js                    |  8 ++-
 lib/internal/worker.js                        | 12 +++-
 src/env-inl.h                                 |  5 ++
 src/env.h                                     |  1 +
 src/node_worker.cc                            | 72 +++++++++++++++++--
 src/node_worker.h                             |  5 +-
 test/parallel/test-internal-errors.js         | 10 +++
 test/parallel/test-worker-execargv-invalid.js | 35 +++++++++
 test/parallel/test-worker-execargv.js         | 22 ++++++
 12 files changed, 171 insertions(+), 15 deletions(-)
 create mode 100644 test/parallel/test-worker-execargv-invalid.js
 create mode 100644 test/parallel/test-worker-execargv.js

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 610060a15106cd..3fb129fd85a6c0 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1869,6 +1869,12 @@ The fulfilled value of a linking promise is not a `vm.SourceTextModule` object.
 The current module's status does not allow for this operation. The specific
 meaning of the error depends on the specific function.
 
+<a id="ERR_WORKER_INVALID_EXEC_ARGV"></a>
+### ERR_WORKER_INVALID_EXEC_ARGV
+
+The `execArgv` option passed to the `Worker` constructor contains
+invalid flags.
+
 <a id="ERR_WORKER_PATH"></a>
 ### ERR_WORKER_PATH
 
diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index 78f35412c32a65..c8d55992be5004 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -316,13 +316,14 @@ if (isMainThread) {
     occur as described in the [HTML structured clone algorithm][], and an error
     will be thrown if the object cannot be cloned (e.g. because it contains
     `function`s).
-  * stdin {boolean} If this is set to `true`, then `worker.stdin` will
+  * `stdin` {boolean} If this is set to `true`, then `worker.stdin` will
     provide a writable stream whose contents will appear as `process.stdin`
     inside the Worker. By default, no data is provided.
-  * stdout {boolean} If this is set to `true`, then `worker.stdout` will
+  * `stdout` {boolean} If this is set to `true`, then `worker.stdout` will
     not automatically be piped through to `process.stdout` in the parent.
-  * stderr {boolean} If this is set to `true`, then `worker.stderr` will
+  * `stderr` {boolean} If this is set to `true`, then `worker.stderr` will
     not automatically be piped through to `process.stderr` in the parent.
+  * `execArgv` {string[]} List of node CLI options passed to the worker.
 
 ### Event: 'error'
 <!-- YAML
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 700277630054a6..91f0b511282430 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -967,6 +967,9 @@ E('ERR_VM_MODULE_NOT_LINKED',
 E('ERR_VM_MODULE_NOT_MODULE',
   'Provided module is not an instance of Module', Error);
 E('ERR_VM_MODULE_STATUS', 'Module status %s', Error);
+E('ERR_WORKER_INVALID_EXEC_ARGV', (errors) =>
+  `Initiated Worker with invalid execArgv flags: ${errors.join(', ')}`,
+  Error);
 E('ERR_WORKER_PATH',
   'The worker script filename must be an absolute path or a relative ' +
   'path starting with \'./\' or \'../\'. Received "%s"',
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
index 0ecf286266678a..76a2e457c4fad9 100644
--- a/lib/internal/validators.js
+++ b/lib/internal/validators.js
@@ -130,6 +130,11 @@ function validateNumber(value, name) {
     throw new ERR_INVALID_ARG_TYPE(name, 'number', value);
 }
 
+function validateArray(value, name) {
+  if (!Array.isArray(value))
+    throw new ERR_INVALID_ARG_TYPE(name, 'array', value);
+}
+
 module.exports = {
   isInt32,
   isUint32,
@@ -138,5 +143,6 @@ module.exports = {
   validateInt32,
   validateUint32,
   validateString,
-  validateNumber
+  validateNumber,
+  validateArray,
 };
diff --git a/lib/internal/worker.js b/lib/internal/worker.js
index 2e1568a73822b9..a6811322c5b76d 100644
--- a/lib/internal/worker.js
+++ b/lib/internal/worker.js
@@ -8,8 +8,9 @@ const {
   ERR_WORKER_PATH,
   ERR_WORKER_UNSERIALIZABLE_ERROR,
   ERR_WORKER_UNSUPPORTED_EXTENSION,
+  ERR_WORKER_INVALID_EXEC_ARGV,
 } = require('internal/errors').codes;
-const { validateString } = require('internal/validators');
+const { validateString, validateArray } = require('internal/validators');
 
 const {
   drainMessagePort,
@@ -49,7 +50,9 @@ class Worker extends EventEmitter {
     super();
     debug(`[${threadId}] create new worker`, filename, options);
     validateString(filename, 'filename');
-
+    if (options.execArgv) {
+      validateArray(options.execArgv, 'options.execArgv');
+    }
     if (!options.eval) {
       if (!path.isAbsolute(filename) &&
           !filename.startsWith('./') &&
@@ -68,7 +71,10 @@ class Worker extends EventEmitter {
 
     const url = options.eval ? null : pathToFileURL(filename);
     // Set up the C++ handle for the worker, as well as some internal wiring.
-    this[kHandle] = new WorkerImpl(url);
+    this[kHandle] = new WorkerImpl(url, options.execArgv);
+    if (this[kHandle].invalidExecArgv) {
+      throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv);
+    }
     this[kHandle].onexit = (code) => this[kOnExit](code);
     this[kPort] = this[kHandle].messagePort;
     this[kPort].on('message', (data) => this[kOnMessage](data));
diff --git a/src/env-inl.h b/src/env-inl.h
index 7c643539ad94ac..d5bf6bc086ab77 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -595,6 +595,11 @@ inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
   return options_;
 }
 
+inline void IsolateData::set_options(
+    std::shared_ptr<PerIsolateOptions> options) {
+  options_ = options;
+}
+
 void Environment::CreateImmediate(native_immediate_callback cb,
                                   void* data,
                                   v8::Local<v8::Object> obj,
diff --git a/src/env.h b/src/env.h
index fdd5c2382cb7c1..711d07963a5f10 100644
--- a/src/env.h
+++ b/src/env.h
@@ -394,6 +394,7 @@ class IsolateData {
   inline uint32_t* zero_fill_field() const;
   inline MultiIsolatePlatform* platform() const;
   inline std::shared_ptr<PerIsolateOptions> options();
+  inline void set_options(std::shared_ptr<PerIsolateOptions> options);
 
 #define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
 #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
diff --git a/src/node_worker.cc b/src/node_worker.cc
index 65d3b7297cc65b..e4d0efeb85ec9f 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -10,7 +10,9 @@
 #include "async_wrap-inl.h"
 
 #include <string>
+#include <vector>
 
+using node::options_parser::kDisallowedInEnvironment;
 using v8::ArrayBuffer;
 using v8::Context;
 using v8::Function;
@@ -68,7 +70,10 @@ void WaitForWorkerInspectorToStop(Environment* child) {}
 
 }  // anonymous namespace
 
-Worker::Worker(Environment* env, Local<Object> wrap, const std::string& url)
+Worker::Worker(Environment* env,
+               Local<Object> wrap,
+               const std::string& url,
+               std::shared_ptr<PerIsolateOptions> per_isolate_opts)
     : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), url_(url) {
   // Generate a new thread id.
   {
@@ -109,10 +114,15 @@ Worker::Worker(Environment* env, Local<Object> wrap, const std::string& url)
     Isolate::Scope isolate_scope(isolate_);
     HandleScope handle_scope(isolate_);
 
-    isolate_data_.reset(CreateIsolateData(isolate_,
-                                          &loop_,
-                                          env->isolate_data()->platform(),
-                                          array_buffer_allocator_.get()));
+    IsolateData* isolate_data =
+        CreateIsolateData(isolate_,
+                          &loop_,
+                          env->isolate_data()->platform(),
+                          array_buffer_allocator_.get());
+    if (per_isolate_opts != nullptr) {
+      isolate_data->set_options(per_isolate_opts);
+    }
+    isolate_data_.reset(isolate_data);
     CHECK(isolate_data_);
 
     Local<Context> context = NewContext(isolate_);
@@ -389,14 +399,62 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
   }
 
   std::string url;
+  std::shared_ptr<PerIsolateOptions> per_isolate_opts = nullptr;
+
   // Argument might be a string or URL
-  if (args.Length() == 1 && !args[0]->IsNullOrUndefined()) {
+  if (args.Length() > 0 && !args[0]->IsNullOrUndefined()) {
     Utf8Value value(
         args.GetIsolate(),
         args[0]->ToString(env->context()).FromMaybe(v8::Local<v8::String>()));
     url.append(value.out(), value.length());
+
+    if (args.Length() > 1 && args[1]->IsArray()) {
+      v8::Local<v8::Array> array = args[1].As<v8::Array>();
+      // The first argument is reserved for program name, but we don't need it
+      // in workers.
+      std::vector<std::string> exec_argv = {""};
+      uint32_t length = array->Length();
+      for (uint32_t i = 0; i < length; i++) {
+        v8::Local<v8::Value> arg;
+        if (!array->Get(env->context(), i).ToLocal(&arg)) {
+          return;
+        }
+        Utf8Value arg_utf8_value(
+            args.GetIsolate(),
+            arg->ToString(env->context()).FromMaybe(v8::Local<v8::String>()));
+        std::string arg_string(arg_utf8_value.out(), arg_utf8_value.length());
+        exec_argv.push_back(arg_string);
+      }
+
+      std::vector<std::string> invalid_args{};
+      std::vector<std::string> errors{};
+      per_isolate_opts.reset(new PerIsolateOptions());
+
+      // Using invalid_args as the v8_args argument as it stores unknown
+      // options for the per isolate parser.
+      options_parser::PerIsolateOptionsParser::instance.Parse(
+          &exec_argv,
+          nullptr,
+          &invalid_args,
+          per_isolate_opts.get(),
+          kDisallowedInEnvironment,
+          &errors);
+
+      // The first argument is program name.
+      invalid_args.erase(invalid_args.begin());
+      if (errors.size() > 0 || invalid_args.size() > 0) {
+        v8::Local<v8::Value> value =
+            ToV8Value(env->context(),
+                      errors.size() > 0 ? errors : invalid_args)
+                .ToLocalChecked();
+        Local<String> key =
+            FIXED_ONE_BYTE_STRING(env->isolate(), "invalidExecArgv");
+        args.This()->Set(env->context(), key, value).FromJust();
+        return;
+      }
+    }
   }
-  new Worker(env, args.This(), url);
+  new Worker(env, args.This(), url, per_isolate_opts);
 }
 
 void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {
diff --git a/src/node_worker.h b/src/node_worker.h
index cbd4a861570b27..dd054ac38d5f76 100644
--- a/src/node_worker.h
+++ b/src/node_worker.h
@@ -12,7 +12,10 @@ namespace worker {
 // A worker thread, as represented in its parent thread.
 class Worker : public AsyncWrap {
  public:
-  Worker(Environment* env, v8::Local<v8::Object> wrap, const std::string& url);
+  Worker(Environment* env,
+         v8::Local<v8::Object> wrap,
+         const std::string& url,
+         std::shared_ptr<PerIsolateOptions> per_isolate_opts);
   ~Worker();
 
   // Run the worker. This is only called from the worker thread.
diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js
index bf4688d89bd0ca..e53a9a9fe42ca8 100644
--- a/test/parallel/test-internal-errors.js
+++ b/test/parallel/test-internal-errors.js
@@ -269,3 +269,13 @@ assert.strictEqual(
 
   restoreStdout();
 }
+
+{
+  const error = new errors.codes.ERR_WORKER_INVALID_EXEC_ARGV(
+    ['--foo, --bar']
+  );
+  assert.strictEqual(
+    error.message,
+    'Initiated Worker with invalid execArgv flags: --foo, --bar'
+  );
+}
diff --git a/test/parallel/test-worker-execargv-invalid.js b/test/parallel/test-worker-execargv-invalid.js
new file mode 100644
index 00000000000000..e2deaf5464113a
--- /dev/null
+++ b/test/parallel/test-worker-execargv-invalid.js
@@ -0,0 +1,35 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const { Worker } = require('worker_threads');
+
+{
+  const expectedErr = common.expectsError({
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError
+  }, 2);
+
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: 'hello' });
+  }, expectedErr);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: 6 });
+  }, expectedErr);
+}
+
+{
+  const expectedErr = common.expectsError({
+    code: 'ERR_WORKER_INVALID_EXEC_ARGV',
+    type: Error
+  }, 3);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: ['--foo'] });
+  }, expectedErr);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: ['--title=blah'] });
+  }, expectedErr);
+  assert.throws(() => {
+    new Worker(__filename, { execArgv: ['--redirect-warnings'] });
+  }, expectedErr);
+}
diff --git a/test/parallel/test-worker-execargv.js b/test/parallel/test-worker-execargv.js
new file mode 100644
index 00000000000000..e04f015e03e2ff
--- /dev/null
+++ b/test/parallel/test-worker-execargv.js
@@ -0,0 +1,22 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+// This test ensures that Workers have the ability to get
+// their own command line flags.
+
+const { Worker, isMainThread } = require('worker_threads');
+const { StringDecoder } = require('string_decoder');
+const decoder = new StringDecoder('utf8');
+
+if (isMainThread) {
+  const w = new Worker(__filename, { execArgv: ['--trace-warnings'] });
+  w.stderr.on('data', common.mustCall((chunk) => {
+    const error = decoder.write(chunk);
+    assert.ok(
+      /Warning: some warning[\s\S]*at Object\.<anonymous>/.test(error)
+    );
+  }));
+} else {
+  process.emitWarning('some warning');
+}

From a0993a34d37b7a727fb291e5a1c75aecddd65883 Mon Sep 17 00:00:00 2001
From: Yael Hermon <yaelhe@wix.com>
Date: Sun, 13 Jan 2019 14:59:39 +0200
Subject: [PATCH 2/3] fixup

---
 doc/api/worker_threads.md  |  1 +
 lib/internal/validators.js |  8 +-------
 lib/internal/worker.js     |  9 ++++++---
 src/node_worker.cc         | 19 +++++++++++--------
 4 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index c8d55992be5004..ef894f2dfddcca 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -324,6 +324,7 @@ if (isMainThread) {
   * `stderr` {boolean} If this is set to `true`, then `worker.stderr` will
     not automatically be piped through to `process.stderr` in the parent.
   * `execArgv` {string[]} List of node CLI options passed to the worker.
+    V8 options and options that affect the process are not supported.
 
 ### Event: 'error'
 <!-- YAML
diff --git a/lib/internal/validators.js b/lib/internal/validators.js
index 76a2e457c4fad9..0ecf286266678a 100644
--- a/lib/internal/validators.js
+++ b/lib/internal/validators.js
@@ -130,11 +130,6 @@ function validateNumber(value, name) {
     throw new ERR_INVALID_ARG_TYPE(name, 'number', value);
 }
 
-function validateArray(value, name) {
-  if (!Array.isArray(value))
-    throw new ERR_INVALID_ARG_TYPE(name, 'array', value);
-}
-
 module.exports = {
   isInt32,
   isUint32,
@@ -143,6 +138,5 @@ module.exports = {
   validateInt32,
   validateUint32,
   validateString,
-  validateNumber,
-  validateArray,
+  validateNumber
 };
diff --git a/lib/internal/worker.js b/lib/internal/worker.js
index a6811322c5b76d..e20a37af8a7d6b 100644
--- a/lib/internal/worker.js
+++ b/lib/internal/worker.js
@@ -9,8 +9,9 @@ const {
   ERR_WORKER_UNSERIALIZABLE_ERROR,
   ERR_WORKER_UNSUPPORTED_EXTENSION,
   ERR_WORKER_INVALID_EXEC_ARGV,
+  ERR_INVALID_ARG_TYPE,
 } = require('internal/errors').codes;
-const { validateString, validateArray } = require('internal/validators');
+const { validateString } = require('internal/validators');
 
 const {
   drainMessagePort,
@@ -50,8 +51,10 @@ class Worker extends EventEmitter {
     super();
     debug(`[${threadId}] create new worker`, filename, options);
     validateString(filename, 'filename');
-    if (options.execArgv) {
-      validateArray(options.execArgv, 'options.execArgv');
+    if (options.execArgv && !Array.isArray(options.execArgv)) {
+      throw new ERR_INVALID_ARG_TYPE('options.execArgv',
+                                     'array',
+                                     options.execArgv);
     }
     if (!options.eval) {
       if (!path.isAbsolute(filename) &&
diff --git a/src/node_worker.cc b/src/node_worker.cc
index e4d0efeb85ec9f..dd6d805e55b321 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -114,15 +114,13 @@ Worker::Worker(Environment* env,
     Isolate::Scope isolate_scope(isolate_);
     HandleScope handle_scope(isolate_);
 
-    IsolateData* isolate_data =
-        CreateIsolateData(isolate_,
-                          &loop_,
-                          env->isolate_data()->platform(),
-                          array_buffer_allocator_.get());
+    isolate_data_.reset(CreateIsolateData(isolate_,
+                                          &loop_,
+                                          env->isolate_data()->platform(),
+                                          array_buffer_allocator_.get()));
     if (per_isolate_opts != nullptr) {
-      isolate_data->set_options(per_isolate_opts);
+      isolate_data_->set_options(per_isolate_opts);
     }
-    isolate_data_.reset(isolate_data);
     CHECK(isolate_data_);
 
     Local<Context> context = NewContext(isolate_);
@@ -419,9 +417,14 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
         if (!array->Get(env->context(), i).ToLocal(&arg)) {
           return;
         }
+        v8::MaybeLocal<v8::String> arg_v8_string =
+            arg->ToString(env->context());
+        if (arg_v8_string.IsEmpty()) {
+          return;
+        }
         Utf8Value arg_utf8_value(
             args.GetIsolate(),
-            arg->ToString(env->context()).FromMaybe(v8::Local<v8::String>()));
+            arg_v8_string.FromMaybe(v8::Local<v8::String>()));
         std::string arg_string(arg_utf8_value.out(), arg_utf8_value.length());
         exec_argv.push_back(arg_string);
       }

From a0ee30412a90b4bc007d42aa7701b5c9db0142f0 Mon Sep 17 00:00:00 2001
From: Yael Hermon <yaelhe@wix.com>
Date: Sun, 13 Jan 2019 20:22:11 +0200
Subject: [PATCH 3/3] fixup update doc

---
 doc/api/worker_threads.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index ef894f2dfddcca..05cafa9cb178e7 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -324,7 +324,8 @@ if (isMainThread) {
   * `stderr` {boolean} If this is set to `true`, then `worker.stderr` will
     not automatically be piped through to `process.stderr` in the parent.
   * `execArgv` {string[]} List of node CLI options passed to the worker.
-    V8 options and options that affect the process are not supported.
+    V8 options (such as `--max-old-space-size`) and options that affect the
+    process (such as `--title`) are not supported.
 
 ### Event: 'error'
 <!-- YAML