From dc2d5e9aa8e08755bb771beebfaa643d4bff5849 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Feb 2019 19:56:08 +0800 Subject: [PATCH 1/4] process: start coverage collection before bootstrap This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is. --- lib/internal/bootstrap/loaders.js | 13 -- lib/internal/coverage-gen/with_profiler.js | 44 +----- src/env.h | 9 +- src/inspector/node_inspector.gypi | 1 + src/inspector_coverage.cc | 172 +++++++++++++++++++++ src/node.cc | 13 ++ src/node_binding.cc | 1 + src/node_internals.h | 4 +- 8 files changed, 200 insertions(+), 57 deletions(-) create mode 100644 src/inspector_coverage.cc diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index d21fddbf5239a1..28f7f5277b9ee0 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -311,18 +311,5 @@ NativeModule.prototype.compile = function() { } }; -// Coverage must be turned on early, so that we can collect -// it for Node.js' own internal libraries. -if (process.env.NODE_V8_COVERAGE) { - if (internalBinding('config').hasInspector) { - const coverage = - NativeModule.require('internal/coverage-gen/with_profiler'); - // Inform the profiler to start collecting coverage - coverage.startCoverageCollection(); - } else { - process._rawDebug('NODE_V8_COVERAGE cannot be used without inspector'); - } -} - // This will be passed to internal/bootstrap/node.js. return loaderExports; diff --git a/lib/internal/coverage-gen/with_profiler.js b/lib/internal/coverage-gen/with_profiler.js index 6b17073939b658..7dd4457f8adde7 100644 --- a/lib/internal/coverage-gen/with_profiler.js +++ b/lib/internal/coverage-gen/with_profiler.js @@ -3,14 +3,9 @@ // Implements coverage collection exposed by the `NODE_V8_COVERAGE` // environment variable which can also be used in the user land. -let coverageConnection = null; let coverageDirectory; function writeCoverage() { - if (!coverageConnection && coverageDirectory) { - return; - } - const { join } = require('path'); const { mkdirSync, writeFileSync } = require('fs'); const { threadId } = require('internal/worker'); @@ -28,21 +23,12 @@ function writeCoverage() { const target = join(coverageDirectory, filename); try { disableAllAsyncHooks(); - let msg; - coverageConnection._coverageCallback = function(_msg) { - msg = _msg; - }; - coverageConnection.dispatch(JSON.stringify({ - id: 3, - method: 'Profiler.takePreciseCoverage' - })); - const coverageInfo = JSON.parse(msg).result; - writeFileSync(target, JSON.stringify(coverageInfo)); + internalBinding('coverage').end((msg) => { + const coverageInfo = JSON.parse(msg).result; + writeFileSync(target, JSON.stringify(coverageInfo)); + }); } catch (err) { console.error(err); - } finally { - coverageConnection.disconnect(); - coverageConnection = null; } } @@ -52,33 +38,11 @@ function disableAllAsyncHooks() { hooks_array.forEach((hook) => { hook.disable(); }); } -function startCoverageCollection() { - const { Connection } = internalBinding('inspector'); - coverageConnection = new Connection((res) => { - if (coverageConnection._coverageCallback) { - coverageConnection._coverageCallback(res); - } - }); - coverageConnection.dispatch(JSON.stringify({ - id: 1, - method: 'Profiler.enable' - })); - coverageConnection.dispatch(JSON.stringify({ - id: 2, - method: 'Profiler.startPreciseCoverage', - params: { - callCount: true, - detailed: true - } - })); -} - function setCoverageDirectory(dir) { coverageDirectory = dir; } module.exports = { - startCoverageCollection, writeCoverage, setCoverageDirectory }; diff --git a/src/env.h b/src/env.h index 40bd0797a2ffd0..9d3deb873c3512 100644 --- a/src/env.h +++ b/src/env.h @@ -329,6 +329,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2; V(async_wrap_ctor_template, v8::FunctionTemplate) \ V(async_wrap_object_ctor_template, v8::FunctionTemplate) \ V(buffer_prototype_object, v8::Object) \ + V(coverage_connection, v8::Object) \ V(context, v8::Context) \ V(crypto_key_object_constructor, v8::Function) \ V(domain_callback, v8::Function) \ @@ -363,6 +364,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2; V(message_port, v8::Object) \ V(message_port_constructor_template, v8::FunctionTemplate) \ V(native_module_require, v8::Function) \ + V(on_coverage_message_function, v8::Function) \ V(performance_entry_callback, v8::Function) \ V(performance_entry_template, v8::Function) \ V(pipe_constructor_template, v8::FunctionTemplate) \ @@ -447,9 +449,10 @@ struct ContextInfo { // Listing the AsyncWrap provider types first enables us to cast directly // from a provider type to a debug category. -#define DEBUG_CATEGORY_NAMES(V) \ - NODE_ASYNC_PROVIDER_TYPES(V) \ - V(INSPECTOR_SERVER) +#define DEBUG_CATEGORY_NAMES(V) \ + NODE_ASYNC_PROVIDER_TYPES(V) \ + V(INSPECTOR_SERVER) \ + V(COVERAGE) enum class DebugCategory { #define V(name) name, diff --git a/src/inspector/node_inspector.gypi b/src/inspector/node_inspector.gypi index 21f98cd9002cab..9483a0712e22bd 100644 --- a/src/inspector/node_inspector.gypi +++ b/src/inspector/node_inspector.gypi @@ -45,6 +45,7 @@ '../../src/inspector_io.cc', '../../src/inspector_agent.h', '../../src/inspector_io.h', + '../../src/inspector_coverage.cc', '../../src/inspector_js_api.cc', '../../src/inspector_socket.cc', '../../src/inspector_socket.h', diff --git a/src/inspector_coverage.cc b/src/inspector_coverage.cc new file mode 100644 index 00000000000000..fedb5789f94c3f --- /dev/null +++ b/src/inspector_coverage.cc @@ -0,0 +1,172 @@ +#include "base_object-inl.h" +#include "debug_utils.h" +#include "inspector_agent.h" +#include "node_internals.h" +#include "v8-inspector.h" + +namespace node { +namespace coverage { + +using v8::Context; +using v8::Function; +using v8::FunctionCallbackInfo; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::MaybeLocal; +using v8::NewStringType; +using v8::Object; +using v8::ObjectTemplate; +using v8::String; +using v8::Value; + +using v8_inspector::StringBuffer; +using v8_inspector::StringView; + +std::unique_ptr ToProtocolString(Isolate* isolate, + Local value) { + TwoByteValue buffer(isolate, value); + return StringBuffer::create(StringView(*buffer, buffer.length())); +} + +class V8CoverageConnection : public BaseObject { + public: + class V8CoverageSessionDelegate : public inspector::InspectorSessionDelegate { + public: + explicit V8CoverageSessionDelegate(V8CoverageConnection* connection) + : connection_(connection) {} + + void SendMessageToFrontend( + const v8_inspector::StringView& message) override { + Environment* env = connection_->env_; + Debug(env, + DebugCategory::COVERAGE, + "Sending message to frontend, ending = %s\n", + connection_->ending_ ? "true" : "false"); + if (!connection_->ending_) { + return; + } + Isolate* isolate = env->isolate(); + Local context = env->context(); + + HandleScope handle_scope(isolate); + Context::Scope context_scope(env->context()); + MaybeLocal v8string = + String::NewFromTwoByte(isolate, + message.characters16(), + NewStringType::kNormal, + message.length()); + Local result = v8string.ToLocalChecked().As(); + Local fn = connection_->env_->on_coverage_message_function(); + CHECK(!fn.IsEmpty()); + USE(fn->Call(context, v8::Null(isolate), 1, &result)); + } + + private: + V8CoverageConnection* connection_; + }; + + SET_MEMORY_INFO_NAME(V8CoverageConnection) + SET_SELF_SIZE(V8CoverageConnection) + + void MemoryInfo(MemoryTracker* tracker) const override { + tracker->TrackFieldWithSize( + "session", sizeof(*session_), "InspectorSession"); + } + + explicit V8CoverageConnection(Environment* env) + : BaseObject(env, env->coverage_connection()), + env_(env), + session_(nullptr), + ending_(false) { + inspector::Agent* inspector = env->inspector_agent(); + std::unique_ptr session = + inspector->Connect(std::unique_ptr( + new V8CoverageSessionDelegate(this)), + false); + session_.reset(session.release()); + MakeWeak(); + } + + void Start() { + Debug(env_, + DebugCategory::COVERAGE, + "Sending Profiler.startPreciseCoverage\n"); + Isolate* isolate = env_->isolate(); + Local enable = FIXED_ONE_BYTE_STRING( + isolate, "{\"id\": 1, \"method\": \"Profiler.enable\"}"); + Local start = FIXED_ONE_BYTE_STRING( + isolate, + "{" + "\"id\": 2," + "\"method\": \"Profiler.startPreciseCoverage\"," + "\"params\": {\"callCount\": true, \"detailed\": true}" + "}"); + session_->Dispatch(ToProtocolString(isolate, enable)->string()); + session_->Dispatch(ToProtocolString(isolate, start)->string()); + } + + void End() { + Debug(env_, + DebugCategory::COVERAGE, + "Sending Profiler.takePreciseCoverage\n"); + Isolate* isolate = env_->isolate(); + Local end = + FIXED_ONE_BYTE_STRING(isolate, + "{" + "\"id\": 3," + "\"method\": \"Profiler.takePreciseCoverage\"" + "}"); + ending_ = true; + session_->Dispatch(ToProtocolString(isolate, end)->string()); + } + + friend class V8CoverageSessionDelegate; + + private: + Environment* env_; + std::unique_ptr session_; + bool ending_; +}; + +bool StartCoverageCollection(Environment* env) { + HandleScope scope(env->isolate()); + + Local t = ObjectTemplate::New(env->isolate()); + t->SetInternalFieldCount(1); + Local obj; + if (!t->NewInstance(env->context()).ToLocal(&obj)) { + return false; + } + + obj->SetAlignedPointerInInternalField(0, nullptr); + + CHECK(env->coverage_connection().IsEmpty()); + env->set_coverage_connection(obj); + V8CoverageConnection* connection = new V8CoverageConnection(env); + connection->Start(); + return true; +} + +static void EndCoverageCollection(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + CHECK(args[0]->IsFunction()); + Debug(env, DebugCategory::COVERAGE, "Ending coverage collection\n"); + env->set_on_coverage_message_function(args[0].As()); + V8CoverageConnection* connection = + Unwrap(env->coverage_connection()); + CHECK_NOT_NULL(connection); + connection->End(); +} + +static void Initialize(Local target, + Local unused, + Local context, + void* priv) { + Environment* env = Environment::GetCurrent(context); + env->SetMethod(target, "end", EndCoverageCollection); +} +} // namespace coverage +} // namespace node + +NODE_MODULE_CONTEXT_AWARE_INTERNAL(coverage, node::coverage::Initialize) diff --git a/src/node.cc b/src/node.cc index 7d9075b5e0a16e..80e95f2cc049a6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -19,6 +19,7 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. +#include "debug_utils.h" #include "node_binding.h" #include "node_buffer.h" #include "node_constants.h" @@ -230,6 +231,18 @@ MaybeLocal RunBootstrapping(Environment* env) { Isolate* isolate = env->isolate(); Local context = env->context(); + std::string coverage; + bool rc = credentials::SafeGetenv("NODE_V8_COVERAGE", &coverage); + if (rc && !coverage.empty()) { +#if HAVE_INSPECTOR + if (!coverage::StartCoverageCollection(env)) { + return MaybeLocal(); + } +#else + fprintf(stderr, 'NODE_V8_COVERAGE cannot be used without inspector'); +#endif // HAVE_INSPECTOR + } + // Add a reference to the global object Local global = context->Global(); diff --git a/src/node_binding.cc b/src/node_binding.cc index 08d55567d4904a..c54dd8a7e65326 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -33,6 +33,7 @@ V(cares_wrap) \ V(config) \ V(contextify) \ + V(coverage) \ V(credentials) \ V(domain) \ V(fs) \ diff --git a/src/node_internals.h b/src/node_internals.h index c94be6ebe9155d..905f0376710295 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -269,7 +269,9 @@ void DefineZlibConstants(v8::Local target); v8::MaybeLocal RunBootstrapping(Environment* env); v8::MaybeLocal StartExecution(Environment* env, const char* main_script_id); - +namespace coverage { +bool StartCoverageCollection(Environment* env); +} } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS From 0bf9ae95d8d1f6bb1f0554ba43f5e364d5045b20 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Feb 2019 22:24:57 +0800 Subject: [PATCH 2/4] fixup! process: start coverage collection before bootstrap --- lib/internal/coverage-gen/with_profiler.js | 4 +- src/inspector_coverage.cc | 44 ++++++++++------------ 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/lib/internal/coverage-gen/with_profiler.js b/lib/internal/coverage-gen/with_profiler.js index 7dd4457f8adde7..573d2c3712b281 100644 --- a/lib/internal/coverage-gen/with_profiler.js +++ b/lib/internal/coverage-gen/with_profiler.js @@ -25,7 +25,9 @@ function writeCoverage() { disableAllAsyncHooks(); internalBinding('coverage').end((msg) => { const coverageInfo = JSON.parse(msg).result; - writeFileSync(target, JSON.stringify(coverageInfo)); + if (coverageInfo) { + writeFileSync(target, JSON.stringify(coverageInfo)); + } }); } catch (err) { console.error(err); diff --git a/src/inspector_coverage.cc b/src/inspector_coverage.cc index fedb5789f94c3f..8cbec586fba718 100644 --- a/src/inspector_coverage.cc +++ b/src/inspector_coverage.cc @@ -38,16 +38,17 @@ class V8CoverageConnection : public BaseObject { void SendMessageToFrontend( const v8_inspector::StringView& message) override { - Environment* env = connection_->env_; + Environment* env = connection_->env(); + Local fn = connection_->env()->on_coverage_message_function(); + bool ending = !fn.IsEmpty(); Debug(env, DebugCategory::COVERAGE, "Sending message to frontend, ending = %s\n", - connection_->ending_ ? "true" : "false"); - if (!connection_->ending_) { + ending ? "true" : "false"); + if (!ending) { return; } Isolate* isolate = env->isolate(); - Local context = env->context(); HandleScope handle_scope(isolate); Context::Scope context_scope(env->context()); @@ -56,10 +57,13 @@ class V8CoverageConnection : public BaseObject { message.characters16(), NewStringType::kNormal, message.length()); - Local result = v8string.ToLocalChecked().As(); - Local fn = connection_->env_->on_coverage_message_function(); - CHECK(!fn.IsEmpty()); - USE(fn->Call(context, v8::Null(isolate), 1, &result)); + Local args[] = {v8string.ToLocalChecked().As()}; + USE(MakeCallback(isolate, + connection_->object(), + fn, + arraysize(args), + args, + async_context{0, 0})); } private: @@ -75,24 +79,19 @@ class V8CoverageConnection : public BaseObject { } explicit V8CoverageConnection(Environment* env) - : BaseObject(env, env->coverage_connection()), - env_(env), - session_(nullptr), - ending_(false) { + : BaseObject(env, env->coverage_connection()), session_(nullptr) { inspector::Agent* inspector = env->inspector_agent(); - std::unique_ptr session = - inspector->Connect(std::unique_ptr( - new V8CoverageSessionDelegate(this)), - false); - session_.reset(session.release()); + std::unique_ptr session = inspector->Connect( + std::make_unique(this), false); + session_ = std::move(session); MakeWeak(); } void Start() { - Debug(env_, + Debug(this->env(), DebugCategory::COVERAGE, "Sending Profiler.startPreciseCoverage\n"); - Isolate* isolate = env_->isolate(); + Isolate* isolate = this->env()->isolate(); Local enable = FIXED_ONE_BYTE_STRING( isolate, "{\"id\": 1, \"method\": \"Profiler.enable\"}"); Local start = FIXED_ONE_BYTE_STRING( @@ -107,26 +106,23 @@ class V8CoverageConnection : public BaseObject { } void End() { - Debug(env_, + Debug(this->env(), DebugCategory::COVERAGE, "Sending Profiler.takePreciseCoverage\n"); - Isolate* isolate = env_->isolate(); + Isolate* isolate = this->env()->isolate(); Local end = FIXED_ONE_BYTE_STRING(isolate, "{" "\"id\": 3," "\"method\": \"Profiler.takePreciseCoverage\"" "}"); - ending_ = true; session_->Dispatch(ToProtocolString(isolate, end)->string()); } friend class V8CoverageSessionDelegate; private: - Environment* env_; std::unique_ptr session_; - bool ending_; }; bool StartCoverageCollection(Environment* env) { From 44ff18bc159c92a92e95310048cbabc5caa9169e Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Feb 2019 22:31:01 +0800 Subject: [PATCH 3/4] fixup! fixup! process: start coverage collection before bootstrap --- src/node.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index 80e95f2cc049a6..d72c462a53f247 100644 --- a/src/node.cc +++ b/src/node.cc @@ -239,7 +239,7 @@ MaybeLocal RunBootstrapping(Environment* env) { return MaybeLocal(); } #else - fprintf(stderr, 'NODE_V8_COVERAGE cannot be used without inspector'); + fprintf(stderr, "NODE_V8_COVERAGE cannot be used without inspector"); #endif // HAVE_INSPECTOR } From de2845b25dab839bbafc0dd1255afd0ee9e6b51d Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 18 Feb 2019 13:39:26 +0800 Subject: [PATCH 4/4] fixup! fixup! fixup! process: start coverage collection before bootstrap --- src/node_binding.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/node_binding.cc b/src/node_binding.cc index c54dd8a7e65326..c9ff7be46f80fc 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -21,6 +21,12 @@ #define NODE_BUILTIN_REPORT_MODULES(V) #endif +#if HAVE_INSPECTOR +#define NODE_BUILTIN_COVERAGE_MODULES(V) V(coverage) +#else +#define NODE_BUILTIN_COVERAGE_MODULES(V) +#endif + // A list of built-in modules. In order to do module registration // in node::Init(), need to add built-in modules in the following list. // Then in binding::RegisterBuiltinModules(), it calls modules' registration @@ -33,7 +39,6 @@ V(cares_wrap) \ V(config) \ V(contextify) \ - V(coverage) \ V(credentials) \ V(domain) \ V(fs) \ @@ -78,7 +83,8 @@ NODE_BUILTIN_STANDARD_MODULES(V) \ NODE_BUILTIN_OPENSSL_MODULES(V) \ NODE_BUILTIN_ICU_MODULES(V) \ - NODE_BUILTIN_REPORT_MODULES(V) + NODE_BUILTIN_REPORT_MODULES(V) \ + NODE_BUILTIN_COVERAGE_MODULES(V) // This is used to load built-in modules. Instead of using // __attribute__((constructor)), we call the _register_