From d501fa2c0c47989d13aaf7e6b33ecb722240ad1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Wed, 16 Aug 2017 15:56:03 +0200 Subject: [PATCH] deps: cherry-pick f19b889 from upstream V8 Original commit message: [inspector] support for cases when embedder doesn't call contextDestroyed Node.js doesn't have good place to call contextDestroyed. We need to cleanup everything on our side to allow clients to not call contextDestroyed method. R=dgozman@chromium.org,eostroukhov@chromium.com Bug: none Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng Reviewed-on: https://chromium-review.googlesource.com/575519 Reviewed-by: Dmitry Gozman Commit-Queue: Aleksey Kozyatinskiy Cr-Original-Commit-Position: refs/heads/master@{#46849} Reviewed-on: https://chromium-review.googlesource.com/596549 Cr-Commit-Position: refs/heads/master@{#47060} PR-URL: https://github.com/nodejs/node/pull/14730 Reviewed-By: Ben Noordhuis Reviewed-By: Ali Ijaz Sheikh Reviewed-By: Colin Ihrig Reviewed-By: Matteo Collina --- deps/v8/src/inspector/inspected-context.cc | 41 +++++++++++++++++++ deps/v8/src/inspector/inspected-context.h | 3 ++ deps/v8/src/inspector/v8-inspector-impl.cc | 4 ++ deps/v8/src/inspector/v8-inspector-impl.h | 1 + deps/v8/test/inspector/inspector-test.cc | 9 ++++ deps/v8/test/inspector/isolate-data.cc | 7 ++++ deps/v8/test/inspector/isolate-data.h | 1 + ...estroyed-on-context-collected-expected.txt | 7 ++++ .../context-destroyed-on-context-collected.js | 14 +++++++ 9 files changed, 87 insertions(+) create mode 100644 deps/v8/test/inspector/runtime/context-destroyed-on-context-collected-expected.txt create mode 100644 deps/v8/test/inspector/runtime/context-destroyed-on-context-collected.js diff --git a/deps/v8/src/inspector/inspected-context.cc b/deps/v8/src/inspector/inspected-context.cc index d7a2f810db315b..5b6b97185cb0dc 100644 --- a/deps/v8/src/inspector/inspected-context.cc +++ b/deps/v8/src/inspector/inspected-context.cc @@ -15,6 +15,39 @@ namespace v8_inspector { +class InspectedContext::WeakCallbackData { + public: + WeakCallbackData(InspectedContext* context, V8InspectorImpl* inspector, + int groupId, int contextId) + : m_context(context), + m_inspector(inspector), + m_groupId(groupId), + m_contextId(contextId) {} + + static void resetContext(const v8::WeakCallbackInfo& data) { + // InspectedContext is alive here because weak handler is still alive. + data.GetParameter()->m_context->m_weakCallbackData = nullptr; + data.GetParameter()->m_context->m_context.Reset(); + data.SetSecondPassCallback(&callContextCollected); + } + + static void callContextCollected( + const v8::WeakCallbackInfo& data) { + // InspectedContext can be dead here since anything can happen between first + // and second pass callback. + WeakCallbackData* callbackData = data.GetParameter(); + callbackData->m_inspector->contextCollected(callbackData->m_groupId, + callbackData->m_contextId); + delete callbackData; + } + + private: + InspectedContext* m_context; + V8InspectorImpl* m_inspector; + int m_groupId; + int m_contextId; +}; + InspectedContext::InspectedContext(V8InspectorImpl* inspector, const V8ContextInfo& info, int contextId) : m_inspector(inspector), @@ -25,6 +58,11 @@ InspectedContext::InspectedContext(V8InspectorImpl* inspector, m_humanReadableName(toString16(info.humanReadableName)), m_auxData(toString16(info.auxData)) { v8::debug::SetContextId(info.context, contextId); + m_weakCallbackData = + new WeakCallbackData(this, m_inspector, m_contextGroupId, m_contextId); + m_context.SetWeak(m_weakCallbackData, + &InspectedContext::WeakCallbackData::resetContext, + v8::WeakCallbackType::kParameter); if (!info.hasMemoryOnConsole) return; v8::Context::Scope contextScope(info.context); v8::Local global = info.context->Global(); @@ -38,6 +76,9 @@ InspectedContext::InspectedContext(V8InspectorImpl* inspector, } InspectedContext::~InspectedContext() { + // If we destory InspectedContext before weak callback is invoked then we need + // to delete data here. + if (!m_context.IsEmpty()) delete m_weakCallbackData; } // static diff --git a/deps/v8/src/inspector/inspected-context.h b/deps/v8/src/inspector/inspected-context.h index b32263bc2e7033..ac33071f623d3d 100644 --- a/deps/v8/src/inspector/inspected-context.h +++ b/deps/v8/src/inspector/inspected-context.h @@ -47,6 +47,8 @@ class InspectedContext { friend class V8InspectorImpl; InspectedContext(V8InspectorImpl*, const V8ContextInfo&, int contextId); + class WeakCallbackData; + V8InspectorImpl* m_inspector; v8::Global m_context; int m_contextId; @@ -56,6 +58,7 @@ class InspectedContext { const String16 m_auxData; std::unordered_set m_reportedSessionIds; std::unordered_map> m_injectedScripts; + WeakCallbackData* m_weakCallbackData; DISALLOW_COPY_AND_ASSIGN(InspectedContext); }; diff --git a/deps/v8/src/inspector/v8-inspector-impl.cc b/deps/v8/src/inspector/v8-inspector-impl.cc index 6b8e7324f569a2..0a7b19a36a7ecc 100644 --- a/deps/v8/src/inspector/v8-inspector-impl.cc +++ b/deps/v8/src/inspector/v8-inspector-impl.cc @@ -203,6 +203,10 @@ void V8InspectorImpl::contextCreated(const V8ContextInfo& info) { void V8InspectorImpl::contextDestroyed(v8::Local context) { int contextId = InspectedContext::contextId(context); int groupId = contextGroupId(context); + contextCollected(groupId, contextId); +} + +void V8InspectorImpl::contextCollected(int groupId, int contextId) { m_contextIdToGroupIdMap.erase(contextId); ConsoleStorageMap::iterator storageIt = m_consoleStorageMap.find(groupId); diff --git a/deps/v8/src/inspector/v8-inspector-impl.h b/deps/v8/src/inspector/v8-inspector-impl.h index 3effb39f7cb70f..a7666dfd429aa1 100644 --- a/deps/v8/src/inspector/v8-inspector-impl.h +++ b/deps/v8/src/inspector/v8-inspector-impl.h @@ -74,6 +74,7 @@ class V8InspectorImpl : public V8Inspector { const StringView& state) override; void contextCreated(const V8ContextInfo&) override; void contextDestroyed(v8::Local) override; + void contextCollected(int contextGroupId, int contextId); void resetContextGroup(int contextGroupId) override; void idleStarted() override; void idleFinished() override; diff --git a/deps/v8/test/inspector/inspector-test.cc b/deps/v8/test/inspector/inspector-test.cc index 930d6c9477c243..767168b29769b8 100644 --- a/deps/v8/test/inspector/inspector-test.cc +++ b/deps/v8/test/inspector/inspector-test.cc @@ -642,6 +642,9 @@ class InspectorExtension : public IsolateData::SetupGlobalTask { inspector->Set(ToV8String(isolate, "fireContextDestroyed"), v8::FunctionTemplate::New( isolate, &InspectorExtension::FireContextDestroyed)); + inspector->Set( + ToV8String(isolate, "freeContext"), + v8::FunctionTemplate::New(isolate, &InspectorExtension::FreeContext)); inspector->Set(ToV8String(isolate, "addInspectedObject"), v8::FunctionTemplate::New( isolate, &InspectorExtension::AddInspectedObject)); @@ -683,6 +686,12 @@ class InspectorExtension : public IsolateData::SetupGlobalTask { data->FireContextDestroyed(context); } + static void FreeContext(const v8::FunctionCallbackInfo& args) { + v8::Local context = args.GetIsolate()->GetCurrentContext(); + IsolateData* data = IsolateData::FromContext(context); + data->FreeContext(context); + } + static void AddInspectedObject( const v8::FunctionCallbackInfo& args) { if (args.Length() != 2 || !args[0]->IsInt32()) { diff --git a/deps/v8/test/inspector/isolate-data.cc b/deps/v8/test/inspector/isolate-data.cc index 74c367a5e9200e..bd97a927e89085 100644 --- a/deps/v8/test/inspector/isolate-data.cc +++ b/deps/v8/test/inspector/isolate-data.cc @@ -303,6 +303,13 @@ void IsolateData::FireContextDestroyed(v8::Local context) { inspector_->contextDestroyed(context); } +void IsolateData::FreeContext(v8::Local context) { + int context_group_id = GetContextGroupId(context); + auto it = contexts_.find(context_group_id); + if (it == contexts_.end()) return; + contexts_.erase(it); +} + std::vector IsolateData::GetSessionIds(int context_group_id) { std::vector result; for (auto& it : sessions_) { diff --git a/deps/v8/test/inspector/isolate-data.h b/deps/v8/test/inspector/isolate-data.h index a94316ff9bedbb..c96a8d1bbd2bf8 100644 --- a/deps/v8/test/inspector/isolate-data.h +++ b/deps/v8/test/inspector/isolate-data.h @@ -68,6 +68,7 @@ class IsolateData : public v8_inspector::V8InspectorClient { void DumpAsyncTaskStacksStateForTest(); void FireContextCreated(v8::Local context, int context_group_id); void FireContextDestroyed(v8::Local context); + void FreeContext(v8::Local context); private: struct VectorCompare { diff --git a/deps/v8/test/inspector/runtime/context-destroyed-on-context-collected-expected.txt b/deps/v8/test/inspector/runtime/context-destroyed-on-context-collected-expected.txt new file mode 100644 index 00000000000000..9a5e1708c1d589 --- /dev/null +++ b/deps/v8/test/inspector/runtime/context-destroyed-on-context-collected-expected.txt @@ -0,0 +1,7 @@ +Tests that contextDesrtoyed nofitication is fired when context is collected. +{ + method : Runtime.executionContextDestroyed + params : { + executionContextId : + } +} diff --git a/deps/v8/test/inspector/runtime/context-destroyed-on-context-collected.js b/deps/v8/test/inspector/runtime/context-destroyed-on-context-collected.js new file mode 100644 index 00000000000000..9f715937c6db0d --- /dev/null +++ b/deps/v8/test/inspector/runtime/context-destroyed-on-context-collected.js @@ -0,0 +1,14 @@ +// Copyright 2017 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +let {session, contextGroup, Protocol} = + InspectorTest.start('Tests that contextDesrtoyed nofitication is fired when context is collected.'); + +(async function test() { + await Protocol.Runtime.enable(); + Protocol.Runtime.onExecutionContextDestroyed(InspectorTest.logMessage); + contextGroup.addScript('inspector.freeContext()'); + await Protocol.HeapProfiler.collectGarbage(); + InspectorTest.completeTest(); +})();