Commit 66b4c39d authored by Benedikt Meurer's avatar Benedikt Meurer Committed by V8 LUCI CQ

[inspector] Drop broken instrumentation self healing in AsyncStackTrace.

The AsyncStackTrace had some magical self-healing where it'd try to not
stitch together async stack traces when the instrumentation seemed to be
broken. This silent self-healing however seems to be broken itself, and
instead of papering over the problem we should fix instrumentation bugs
when they are observed.

Fixed: chromium:1231064
Change-Id: I2bcc85679abdbe6f4df4866cb951c5f6cefb4f67
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3048181
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: 's avatarYang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75885}
parent 2654671e
......@@ -276,7 +276,7 @@ static String16 identifierFromTitleOrStackTrace(
String16 identifier;
if (title.isEmpty()) {
std::unique_ptr<V8StackTraceImpl> stackTrace =
V8StackTraceImpl::capture(inspector->debugger(), helper.groupId(), 1);
V8StackTraceImpl::capture(inspector->debugger(), 1);
if (stackTrace && !stackTrace->isEmpty()) {
identifier = toString16(stackTrace->topSourceURL()) + ":" +
String16::fromInteger(stackTrace->topLineNumber());
......
......@@ -1618,7 +1618,7 @@ void V8DebuggerAgentImpl::didParseSource(
hasSourceURLComment ? &hasSourceURLComment : nullptr;
const bool* isModuleParam = isModule ? &isModule : nullptr;
std::unique_ptr<V8StackTraceImpl> stack =
V8StackTraceImpl::capture(m_inspector->debugger(), contextGroupId, 1);
V8StackTraceImpl::capture(m_inspector->debugger(), 1);
std::unique_ptr<protocol::Runtime::StackTrace> stackTrace =
stack && !stack->isEmpty()
? stack->buildInspectorObjectImpl(m_debugger, 0)
......
......@@ -859,7 +859,7 @@ v8::Local<v8::Array> V8Debugger::queryObjects(v8::Local<v8::Context> context,
std::unique_ptr<V8StackTraceImpl> V8Debugger::createStackTrace(
v8::Local<v8::StackTrace> v8StackTrace) {
return V8StackTraceImpl::create(this, currentContextGroupId(), v8StackTrace,
return V8StackTraceImpl::create(this, v8StackTrace,
V8StackTraceImpl::maxCallStackSizeToCapture);
}
......@@ -902,7 +902,7 @@ V8StackTraceId V8Debugger::storeCurrentStackTrace(
if (!contextGroupId) return V8StackTraceId();
std::shared_ptr<AsyncStackTrace> asyncStack =
AsyncStackTrace::capture(this, contextGroupId, toString16(description),
AsyncStackTrace::capture(this, toString16(description),
V8StackTraceImpl::maxCallStackSizeToCapture);
if (!asyncStack) return V8StackTraceId();
......@@ -980,9 +980,8 @@ void V8Debugger::asyncTaskScheduledForStack(const String16& taskName,
void* task, bool recurring) {
if (!m_maxAsyncCallStackDepth) return;
v8::HandleScope scope(m_isolate);
std::shared_ptr<AsyncStackTrace> asyncStack =
AsyncStackTrace::capture(this, currentContextGroupId(), taskName,
V8StackTraceImpl::maxCallStackSizeToCapture);
std::shared_ptr<AsyncStackTrace> asyncStack = AsyncStackTrace::capture(
this, taskName, V8StackTraceImpl::maxCallStackSizeToCapture);
if (asyncStack) {
m_asyncTaskStacks[task] = asyncStack;
if (recurring) m_recurringTasks.insert(task);
......@@ -1104,7 +1103,7 @@ std::unique_ptr<V8StackTraceImpl> V8Debugger::captureStackTrace(
stackSize = V8StackTraceImpl::maxCallStackSizeToCapture;
});
}
return V8StackTraceImpl::capture(this, contextGroupId, stackSize);
return V8StackTraceImpl::capture(this, stackSize);
}
int V8Debugger::currentContextGroupId() {
......
......@@ -53,7 +53,7 @@ std::vector<std::shared_ptr<StackFrame>> toFramesVector(
return frames;
}
void calculateAsyncChain(V8Debugger* debugger, int contextGroupId,
void calculateAsyncChain(V8Debugger* debugger,
std::shared_ptr<AsyncStackTrace>* asyncParent,
V8StackTraceId* externalParent, int* maxAsyncDepth) {
*asyncParent = debugger->currentAsyncParent();
......@@ -61,18 +61,6 @@ void calculateAsyncChain(V8Debugger* debugger, int contextGroupId,
DCHECK(externalParent->IsInvalid() || !*asyncParent);
if (maxAsyncDepth) *maxAsyncDepth = debugger->maxAsyncCallChainDepth();
// Do not accidentally append async call chain from another group. This should
// not happen if we have proper instrumentation, but let's double-check to be
// safe.
if (contextGroupId && *asyncParent &&
(*asyncParent)->externalParent().IsInvalid() &&
(*asyncParent)->contextGroupId() != contextGroupId) {
asyncParent->reset();
*externalParent = V8StackTraceId();
if (maxAsyncDepth) *maxAsyncDepth = 0;
return;
}
// Only the top stack in the chain may be empty, so ensure that second stack
// is non-empty (it's the top of appended chain).
if (*asyncParent && (*asyncParent)->isEmpty()) {
......@@ -243,8 +231,8 @@ void V8StackTraceImpl::setCaptureStackTraceForUncaughtExceptions(
// static
std::unique_ptr<V8StackTraceImpl> V8StackTraceImpl::create(
V8Debugger* debugger, int contextGroupId,
v8::Local<v8::StackTrace> v8StackTrace, int maxStackSize) {
V8Debugger* debugger, v8::Local<v8::StackTrace> v8StackTrace,
int maxStackSize) {
DCHECK(debugger);
v8::Isolate* isolate = debugger->isolate();
......@@ -258,8 +246,7 @@ std::unique_ptr<V8StackTraceImpl> V8StackTraceImpl::create(
int maxAsyncDepth = 0;
std::shared_ptr<AsyncStackTrace> asyncParent;
V8StackTraceId externalParent;
calculateAsyncChain(debugger, contextGroupId, &asyncParent, &externalParent,
&maxAsyncDepth);
calculateAsyncChain(debugger, &asyncParent, &externalParent, &maxAsyncDepth);
if (frames.empty() && !asyncParent && externalParent.IsInvalid())
return nullptr;
return std::unique_ptr<V8StackTraceImpl>(new V8StackTraceImpl(
......@@ -268,7 +255,7 @@ std::unique_ptr<V8StackTraceImpl> V8StackTraceImpl::create(
// static
std::unique_ptr<V8StackTraceImpl> V8StackTraceImpl::capture(
V8Debugger* debugger, int contextGroupId, int maxStackSize) {
V8Debugger* debugger, int maxStackSize) {
DCHECK(debugger);
TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("v8.stack_trace"),
......@@ -281,8 +268,7 @@ std::unique_ptr<V8StackTraceImpl> V8StackTraceImpl::capture(
v8StackTrace = v8::StackTrace::CurrentStackTrace(isolate, maxStackSize,
stackTraceOptions);
}
return V8StackTraceImpl::create(debugger, contextGroupId, v8StackTrace,
maxStackSize);
return V8StackTraceImpl::create(debugger, v8StackTrace, maxStackSize);
}
V8StackTraceImpl::V8StackTraceImpl(
......@@ -419,8 +405,7 @@ StackFrame* V8StackTraceImpl::StackFrameIterator::frame() {
// static
std::shared_ptr<AsyncStackTrace> AsyncStackTrace::capture(
V8Debugger* debugger, int contextGroupId, const String16& description,
int maxStackSize) {
V8Debugger* debugger, const String16& description, int maxStackSize) {
DCHECK(debugger);
TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("v8.stack_trace"),
......@@ -438,8 +423,7 @@ std::shared_ptr<AsyncStackTrace> AsyncStackTrace::capture(
std::shared_ptr<AsyncStackTrace> asyncParent;
V8StackTraceId externalParent;
calculateAsyncChain(debugger, contextGroupId, &asyncParent, &externalParent,
nullptr);
calculateAsyncChain(debugger, &asyncParent, &externalParent, nullptr);
if (frames.empty() && !asyncParent && externalParent.IsInvalid())
return nullptr;
......@@ -452,30 +436,21 @@ std::shared_ptr<AsyncStackTrace> AsyncStackTrace::capture(
return asyncParent;
}
DCHECK(contextGroupId || asyncParent || !externalParent.IsInvalid());
if (!contextGroupId && asyncParent) {
contextGroupId = asyncParent->m_contextGroupId;
}
return std::shared_ptr<AsyncStackTrace>(
new AsyncStackTrace(contextGroupId, description, std::move(frames),
asyncParent, externalParent));
return std::shared_ptr<AsyncStackTrace>(new AsyncStackTrace(
description, std::move(frames), asyncParent, externalParent));
}
AsyncStackTrace::AsyncStackTrace(
int contextGroupId, const String16& description,
const String16& description,
std::vector<std::shared_ptr<StackFrame>> frames,
std::shared_ptr<AsyncStackTrace> asyncParent,
const V8StackTraceId& externalParent)
: m_contextGroupId(contextGroupId),
m_id(0),
: m_id(0),
m_suspendedTaskId(nullptr),
m_description(description),
m_frames(std::move(frames)),
m_asyncParent(std::move(asyncParent)),
m_externalParent(externalParent) {
DCHECK(m_contextGroupId || (!externalParent.IsInvalid() && m_frames.empty()));
}
m_externalParent(externalParent) {}
std::unique_ptr<protocol::Runtime::StackTrace>
AsyncStackTrace::buildInspectorObject(V8Debugger* debugger,
......@@ -485,8 +460,6 @@ AsyncStackTrace::buildInspectorObject(V8Debugger* debugger,
maxAsyncDepth);
}
int AsyncStackTrace::contextGroupId() const { return m_contextGroupId; }
void AsyncStackTrace::setSuspendedTaskId(void* task) {
m_suspendedTaskId = task;
}
......
......@@ -49,11 +49,9 @@ class V8StackTraceImpl : public V8StackTrace {
bool capture);
static int maxCallStackSizeToCapture;
static std::unique_ptr<V8StackTraceImpl> create(V8Debugger*,
int contextGroupId,
v8::Local<v8::StackTrace>,
int maxStackSize);
static std::unique_ptr<V8StackTraceImpl> capture(V8Debugger*,
int contextGroupId,
int maxStackSize);
~V8StackTraceImpl() override;
......@@ -114,7 +112,6 @@ class AsyncStackTrace {
AsyncStackTrace(const AsyncStackTrace&) = delete;
AsyncStackTrace& operator=(const AsyncStackTrace&) = delete;
static std::shared_ptr<AsyncStackTrace> capture(V8Debugger*,
int contextGroupId,
const String16& description,
int maxStackSize);
static uintptr_t store(V8Debugger* debugger,
......@@ -133,7 +130,6 @@ class AsyncStackTrace {
void setSuspendedTaskId(void* task);
void* suspendedTaskId() const;
int contextGroupId() const;
const String16& description() const;
std::weak_ptr<AsyncStackTrace> parent() const;
bool isEmpty() const;
......@@ -144,12 +140,11 @@ class AsyncStackTrace {
}
private:
AsyncStackTrace(int contextGroupId, const String16& description,
AsyncStackTrace(const String16& description,
std::vector<std::shared_ptr<StackFrame>> frames,
std::shared_ptr<AsyncStackTrace> asyncParent,
const V8StackTraceId& externalParent);
int m_contextGroupId;
uintptr_t m_id;
void* m_suspendedTaskId;
String16 m_description;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment