Skip to content

Commit

Permalink
Improve crash reporting of frame connection retry failures (see #3664)
Browse files Browse the repository at this point in the history 
Introduce different call stacks for different types of disconnects,
and log additional state information in the FATAL message.
  • Loading branch information
@magreenblatt
magreenblatt committed Mar 8, 2024
1 parent 947a2e7 commit 205f769
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 57 deletions.
137 changes: 81 additions & 56 deletions libcef/renderer/frame_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ void CefFrameImpl::OnDetached() {
browser_->FrameDetached(frame_);
frame_ = nullptr;

OnDisconnect(DisconnectReason::DETACHED);
OnDisconnect(DisconnectReason::DETACHED, std::string());

browser_ = nullptr;

Expand Down Expand Up @@ -511,9 +511,8 @@ void CefFrameImpl::ConnectBrowserFrame(ConnectReason reason) {
// connection.
browser_frame->FrameAttached(receiver_.BindNewPipeAndPassRemote(),
reattached);
receiver_.set_disconnect_handler(
base::BindOnce(&CefFrameImpl::OnDisconnect, this,
DisconnectReason::RENDER_FRAME_DISCONNECT));
receiver_.set_disconnect_with_reason_handler(
base::BindOnce(&CefFrameImpl::OnRenderFrameDisconnect, this));
}

const mojo::Remote<cef::mojom::BrowserFrame>& CefFrameImpl::GetBrowserFrame(
Expand All @@ -527,20 +526,82 @@ const mojo::Remote<cef::mojom::BrowserFrame>& CefFrameImpl::GetBrowserFrame(
// Triggers creation of a CefBrowserFrame in the browser process.
render_frame->GetBrowserInterfaceBroker()->GetInterface(
browser_frame_.BindNewPipeAndPassReceiver());
browser_frame_.set_disconnect_handler(
base::BindOnce(&CefFrameImpl::OnDisconnect, this,
DisconnectReason::BROWSER_FRAME_DISCONNECT));
browser_frame_.set_disconnect_with_reason_handler(
base::BindOnce(&CefFrameImpl::OnBrowserFrameDisconnect, this));
}
}
return browser_frame_;
}

void CefFrameImpl::OnBrowserFrameTimeout() {
LOG(ERROR) << frame_debug_str_ << " connection timeout";
OnDisconnect(DisconnectReason::CONNECT_TIMEOUT);
}

void CefFrameImpl::OnDisconnect(DisconnectReason reason) {
OnDisconnect(DisconnectReason::CONNECT_TIMEOUT, std::string());
}

void CefFrameImpl::OnBrowserFrameDisconnect(uint32_t custom_reason,
const std::string& description) {
OnDisconnect(DisconnectReason::BROWSER_FRAME_DISCONNECT, description);
}

void CefFrameImpl::OnRenderFrameDisconnect(uint32_t custom_reason,
const std::string& description) {
OnDisconnect(DisconnectReason::RENDER_FRAME_DISCONNECT, description);
}

// static
std::string CefFrameImpl::GetDisconnectDebugString(
ConnectionState connection_state,
bool frame_is_valid,
DisconnectReason reason,
const std::string& description) {
std::string reason_str;
switch (reason) {
case DisconnectReason::DETACHED:
reason_str = "DETACHED";
break;
case DisconnectReason::BROWSER_FRAME_DETACHED:
reason_str = "BROWSER_FRAME_DETACHED";
break;
case DisconnectReason::CONNECT_TIMEOUT:
reason_str = "CONNECT_TIMEOUT";
break;
case DisconnectReason::RENDER_FRAME_DISCONNECT:
reason_str = "RENDER_FRAME_DISCONNECT";
break;
case DisconnectReason::BROWSER_FRAME_DISCONNECT:
reason_str = "BROWSER_FRAME_DISCONNECT";
break;
};

std::string state_str;
switch (connection_state) {
case ConnectionState::DISCONNECTED:
state_str = "DISCONNECTED";
break;
case ConnectionState::CONNECTION_PENDING:
state_str = "CONNECTION_PENDING";
break;
case ConnectionState::CONNECTION_ACKED:
state_str = "CONNECTION_ACKED";
break;
case ConnectionState::RECONNECT_PENDING:
state_str = "RECONNECT_PENDING";
break;
}

if (!frame_is_valid) {
state_str += ", FRAME_INVALID";
}

if (!description.empty()) {
state_str += ", " + description;
}

return "(reason=" + reason_str + ", current_state=" + state_str + ")";
}

void CefFrameImpl::OnDisconnect(DisconnectReason reason,
const std::string& description) {
// Ignore multiple calls in close proximity (which may occur if both
// |browser_frame_| and |receiver_| disconnect). |frame_| will be nullptr
// when called from/after OnDetached().
Expand All @@ -549,49 +610,11 @@ void CefFrameImpl::OnDisconnect(DisconnectReason reason) {
return;
}

if (VLOG_IS_ON(1)) {
std::string reason_str;
switch (reason) {
case DisconnectReason::DETACHED:
reason_str = "DETACHED";
break;
case DisconnectReason::BROWSER_FRAME_DETACHED:
reason_str = "BROWSER_FRAME_DETACHED";
break;
case DisconnectReason::CONNECT_TIMEOUT:
reason_str = "CONNECT_TIMEOUT";
break;
case DisconnectReason::RENDER_FRAME_DISCONNECT:
reason_str = "RENDER_FRAME_DISCONNECT";
break;
case DisconnectReason::BROWSER_FRAME_DISCONNECT:
reason_str = "BROWSER_FRAME_DISCONNECT";
break;
};

std::string state_str;
switch (browser_connection_state_) {
case ConnectionState::DISCONNECTED:
state_str = "DISCONNECTED";
break;
case ConnectionState::CONNECTION_PENDING:
state_str = "CONNECTION_PENDING";
break;
case ConnectionState::CONNECTION_ACKED:
state_str = "CONNECTION_ACKED";
break;
case ConnectionState::RECONNECT_PENDING:
state_str = "RECONNECT_PENDING";
break;
}

if (!frame_) {
state_str += ", FRAME_INVALID";
}

VLOG(1) << frame_debug_str_ << " disconnected (reason=" << reason_str
<< ", current_state=" << state_str << ")";
}
const auto connection_state = browser_connection_state_;
const bool frame_is_valid = !!frame_;
VLOG(1) << frame_debug_str_ << " disconnected "
<< GetDisconnectDebugString(connection_state, frame_is_valid,
reason, description);

browser_frame_.reset();
receiver_.reset();
Expand All @@ -617,7 +640,9 @@ void CefFrameImpl::OnDisconnect(DisconnectReason reason) {
ConnectReason::RETRY));
} else {
// Trigger a crash in official builds.
LOG(FATAL) << frame_debug_str_ << " connection retry failed";
LOG(FATAL) << frame_debug_str_ << " connection retry failed "
<< GetDisconnectDebugString(connection_state, frame_is_valid,
reason, description);
}
}
}
Expand Down Expand Up @@ -690,7 +715,7 @@ void CefFrameImpl::FrameAttachedAck() {
void CefFrameImpl::FrameDetached() {
// Sent from the browser process in response to CefFrameHostImpl::Detach().
CHECK_EQ(ConnectionState::CONNECTION_ACKED, browser_connection_state_);
OnDisconnect(DisconnectReason::BROWSER_FRAME_DETACHED);
OnDisconnect(DisconnectReason::BROWSER_FRAME_DETACHED, std::string());
}

void CefFrameImpl::SendMessage(const std::string& name,
Expand Down
14 changes: 13 additions & 1 deletion libcef/renderer/frame_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,13 @@ class CefFrameImpl
// Called if the BrowserFrame connection attempt times out.
void OnBrowserFrameTimeout();

// Called if the BrowserFrame connection is disconnected.
void OnBrowserFrameDisconnect(uint32_t custom_reason,
const std::string& description);
// Called if the RenderFrame connection is disconnected.
void OnRenderFrameDisconnect(uint32_t custom_reason,
const std::string& description);

enum class DisconnectReason {
DETACHED,
BROWSER_FRAME_DETACHED,
Expand All @@ -127,7 +134,7 @@ class CefFrameImpl
// Called if/when a disconnect occurs. This may occur due to frame navigation,
// destruction, or insertion into the bfcache (when the browser-side frame
// representation is destroyed and closes the connection).
void OnDisconnect(DisconnectReason reason);
void OnDisconnect(DisconnectReason reason, const std::string& description);

// Send an action to the remote BrowserFrame. This will queue the action if
// the remote frame is not yet attached.
Expand Down Expand Up @@ -181,6 +188,11 @@ class CefFrameImpl
RECONNECT_PENDING,
} browser_connection_state_ = ConnectionState::DISCONNECTED;

static std::string GetDisconnectDebugString(ConnectionState connection_state,
bool frame_is_valid,
DisconnectReason reason,
const std::string& description);

base::OneShotTimer browser_connect_timer_;

std::queue<std::pair<std::string, BrowserFrameAction>>
Expand Down

0 comments on commit 205f769

Please sign in to comment.