Skip to content

Commit

Permalink
Capture the interface name for debug purposes when an endpoint closes.
Browse files Browse the repository at this point in the history
This is intended to help narrow down what is causing an increase in
crashes with NotifyError() on the stack. Due to the magic of tail call
optimizations, the actual responsible stack frames are not actually
present in the crash report.

The initial CL attempted to annotate this debug info using a crash key;
since notifications for Mojo endpoints can be dispatched on any thread,
the CL used a thread_local variable to cache a per-thread crash key.
The actual crash key name included the thread ID since crash keys are
global.

However, this turned out not to work well either: thread IDs can be
reused, but crash key names must be unique for the lifetime of the
process. Thus, a thread that ended up reusing a previous thread ID would
end up trying to recreate a crash key with the same name.

Bug: 1414218
Change-Id: Ie7753fc501b10ce1c2d904bf285caba54a8ee365
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4310734
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1113710}
  • Loading branch information
zetafunction authored and Chromium LUCI CQ committed Mar 7, 2023
1 parent 4994140 commit e9742bc
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/check.h"
#include "base/containers/contains.h"
#include "base/containers/cxx20_erase.h"
#include "base/debug/alias.h"
#include "base/functional/bind.h"
#include "base/location.h"
#include "base/logging.h"
Expand Down Expand Up @@ -714,6 +715,12 @@ void InterfaceEndpointClient::NotifyError(
return;
encountered_error_ = true;

// TODO(dcheng): interface_name_ *should* never be null, but for the purposes
// of not introducing a new crash in code intended to help debug crashes, do a
// null check for safety.
DEBUG_ALIAS_FOR_CSTR(interface_name, interface_name_ ? interface_name_ : "",
256);

// Response callbacks may hold on to resource, and there's no need to keep
// them alive any longer. Note that it's allowed that a pending response
// callback may own this endpoint, so we simply move the responders onto the
Expand Down

0 comments on commit e9742bc

Please sign in to comment.