Commit f06fd249 authored by Bence Béky's avatar Bence Béky Committed by Commit Bot

[WS] Handle no handshake stream in PerformUpgrade().

The vast majority of recent crash reports point to line 190:
  CHECK(on_handshake_stream_created_has_been_called_);

Next branch cut is around the corner and this is still number one //net
crasher on M69.  https://crrev.com/c/940463 has been clearly identified
as the culprit.  Much pondering was still not enough to understand the
real underlying cause of this crash since the last branch point.  I
propose to be defensive in WebsocketStream.  While this is not the ideal
solution, I am not hopeful that we will find anything better any time
soon.  This approach allows the request to fail safely, while still
retaining the new WebSocket over HTTP/2 functionality.

Bug: 850183
Change-Id: I21a7c3919466c4339bebce41ee8ad1342fb9c179
Reviewed-on: https://chromium-review.googlesource.com/1140916
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: default avatarAdam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576510}
parent 11227062
...@@ -120,7 +120,6 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI { ...@@ -120,7 +120,6 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI {
kTrafficAnnotation)), kTrafficAnnotation)),
connect_delegate_(std::move(connect_delegate)), connect_delegate_(std::move(connect_delegate)),
handshake_stream_(nullptr), handshake_stream_(nullptr),
on_handshake_stream_created_has_been_called_(false),
perform_upgrade_has_been_called_(false), perform_upgrade_has_been_called_(false),
api_delegate_(std::move(api_delegate)) { api_delegate_(std::move(api_delegate)) {
create_helper->set_stream_request(this); create_helper->set_stream_request(this);
...@@ -187,15 +186,20 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI { ...@@ -187,15 +186,20 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI {
void PerformUpgrade() { void PerformUpgrade() {
DCHECK(timer_); DCHECK(timer_);
CHECK(!perform_upgrade_has_been_called_); CHECK(!perform_upgrade_has_been_called_);
CHECK(on_handshake_stream_created_has_been_called_);
// TODO(bnc): Change to DCHECK after https://crbug.com/850183 is fixed. // TODO(bnc): Change to DCHECK after https://crbug.com/850183 is fixed.
CHECK(handshake_stream_);
CHECK(connect_delegate_); CHECK(connect_delegate_);
perform_upgrade_has_been_called_ = true; perform_upgrade_has_been_called_ = true;
timer_->Stop(); timer_->Stop();
if (!handshake_stream_) {
// TODO(https://crbug.com/850183):
// Find out why this can happen and make it stop.
ReportFailureWithMessage("No handshake stream has been created.");
return;
}
std::unique_ptr<URLRequest> url_request = std::move(url_request_); std::unique_ptr<URLRequest> url_request = std::move(url_request_);
WebSocketHandshakeStreamBase* handshake_stream = handshake_stream_; WebSocketHandshakeStreamBase* handshake_stream = handshake_stream_;
handshake_stream_ = nullptr; handshake_stream_ = nullptr;
...@@ -267,8 +271,6 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI { ...@@ -267,8 +271,6 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI {
// TODO(bnc): Change to DCHECK after https://crbug.com/850183 is fixed. // TODO(bnc): Change to DCHECK after https://crbug.com/850183 is fixed.
CHECK(handshake_stream); CHECK(handshake_stream);
on_handshake_stream_created_has_been_called_ = true;
handshake_stream_ = handshake_stream; handshake_stream_ = handshake_stream;
} }
...@@ -291,7 +293,6 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI { ...@@ -291,7 +293,6 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI {
WebSocketHandshakeStreamBase* handshake_stream_; WebSocketHandshakeStreamBase* handshake_stream_;
// TODO(bnc): Remove after https://crbug.com/850183 is fixed. // TODO(bnc): Remove after https://crbug.com/850183 is fixed.
bool on_handshake_stream_created_has_been_called_;
bool perform_upgrade_has_been_called_; bool perform_upgrade_has_been_called_;
// The failure message supplied by WebSocketBasicHandshakeStream, if any. // The failure message supplied by WebSocketBasicHandshakeStream, if any.
......
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