Commit 0ab3a0b1 authored by Yuki Yamada's avatar Yuki Yamada Committed by Commit Bot

Check IsContextDestroyed() before calling ProcessSendQueue()

We shouldn't call ProcessSendQueue() in WebSocketChannelImpl::Close()
when execution_context_->IsContextDestroyed() returns true.
In current implementation, it is guaranteed that
execution_context_->IsContextDestroyed() returns false in
WebSocketChannelImpl::Close(), but it will return both of true/false
with another CL https://crrev.com/c/2359657

Bug: 1116995, 1100257
Change-Id: Id5157f78859e2d9aa5dbd02a7b82bafd58808e17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362042
Commit-Queue: Yuki Yamada <yukiy@chromium.org>
Reviewed-by: default avatarKeishi Hattori <keishi@chromium.org>
Reviewed-by: default avatarYutaka Hirano <yhirano@chromium.org>
Reviewed-by: default avatarBartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799524}
parent 2b232cbf
...@@ -498,9 +498,6 @@ void DOMWebSocket::ContextDestroyed() { ...@@ -498,9 +498,6 @@ void DOMWebSocket::ContextDestroyed() {
NETWORK_DVLOG(1) << "WebSocket " << this << " contextDestroyed()"; NETWORK_DVLOG(1) << "WebSocket " << this << " contextDestroyed()";
event_queue_->ContextDestroyed(); event_queue_->ContextDestroyed();
if (channel_) { if (channel_) {
if (common_.GetState() == kOpen) {
channel_->Close(WebSocketChannel::kCloseEventCodeGoingAway, String());
}
ReleaseChannel(); ReleaseChannel();
} }
if (common_.GetState() != kClosed) { if (common_.GetState() != kClosed) {
......
...@@ -362,6 +362,7 @@ WebSocketChannel::SendResult WebSocketChannelImpl::Send( ...@@ -362,6 +362,7 @@ WebSocketChannel::SendResult WebSocketChannelImpl::Send(
void WebSocketChannelImpl::Close(int code, const String& reason) { void WebSocketChannelImpl::Close(int code, const String& reason) {
DCHECK_EQ(GetState(), State::kOpen); DCHECK_EQ(GetState(), State::kOpen);
DCHECK(!execution_context_->IsContextDestroyed());
NETWORK_DVLOG(1) << this << " Close(" << code << ", " << reason << ")"; NETWORK_DVLOG(1) << this << " Close(" << code << ", " << reason << ")";
uint16_t code_to_send = static_cast<uint16_t>( uint16_t code_to_send = static_cast<uint16_t>(
code == kCloseEventCodeNotSpecified ? kCloseEventCodeNoStatusRcvd : code); code == kCloseEventCodeNotSpecified ? kCloseEventCodeNoStatusRcvd : code);
...@@ -661,6 +662,7 @@ bool WebSocketChannelImpl::MaybeSendSynchronously( ...@@ -661,6 +662,7 @@ bool WebSocketChannelImpl::MaybeSendSynchronously(
void WebSocketChannelImpl::ProcessSendQueue() { void WebSocketChannelImpl::ProcessSendQueue() {
// TODO(yhirano): This should be DCHECK_EQ(GetState(), State::kOpen). // TODO(yhirano): This should be DCHECK_EQ(GetState(), State::kOpen).
DCHECK(GetState() == State::kOpen || GetState() == State::kConnecting); DCHECK(GetState() == State::kOpen || GetState() == State::kConnecting);
DCHECK(!execution_context_->IsContextDestroyed());
while (!messages_.IsEmpty() && !blob_loader_ && !wait_for_writable_) { while (!messages_.IsEmpty() && !blob_loader_ && !wait_for_writable_) {
Message& message = messages_.front(); Message& message = messages_.front();
network::mojom::blink::WebSocketMessageType message_type = network::mojom::blink::WebSocketMessageType message_type =
......
...@@ -573,9 +573,6 @@ void WebSocketStream::DidClose( ...@@ -573,9 +573,6 @@ void WebSocketStream::DidClose(
void WebSocketStream::ContextDestroyed() { void WebSocketStream::ContextDestroyed() {
DVLOG(1) << "WebSocketStream " << this << " ContextDestroyed()"; DVLOG(1) << "WebSocketStream " << this << " ContextDestroyed()";
if (channel_) { if (channel_) {
if (common_.GetState() == WebSocketCommon::kOpen) {
channel_->Close(WebSocketChannel::kCloseEventCodeGoingAway, String());
}
channel_ = nullptr; channel_ = nullptr;
} }
if (common_.GetState() != WebSocketCommon::kClosed) { if (common_.GetState() != WebSocketCommon::kClosed) {
......
...@@ -199,7 +199,6 @@ TEST_F(WebSocketStreamTest, ConnectWithFailedHandshake) { ...@@ -199,7 +199,6 @@ TEST_F(WebSocketStreamTest, ConnectWithFailedHandshake) {
TEST_F(WebSocketStreamTest, ConnectWithSuccessfulHandshake) { TEST_F(WebSocketStreamTest, ConnectWithSuccessfulHandshake) {
V8TestingScope scope; V8TestingScope scope;
Checkpoint checkpoint;
{ {
InSequence s; InSequence s;
...@@ -207,8 +206,6 @@ TEST_F(WebSocketStreamTest, ConnectWithSuccessfulHandshake) { ...@@ -207,8 +206,6 @@ TEST_F(WebSocketStreamTest, ConnectWithSuccessfulHandshake) {
EXPECT_CALL(Channel(), EXPECT_CALL(Channel(),
Connect(KURL("ws://example.com/chat"), String("chat"))) Connect(KURL("ws://example.com/chat"), String("chat")))
.WillOnce(Return(true)); .WillOnce(Return(true));
EXPECT_CALL(checkpoint, Call(1));
EXPECT_CALL(Channel(), Close(1001, String()));
} }
auto* options = WebSocketStreamOptions::Create(); auto* options = WebSocketStreamOptions::Create();
...@@ -238,9 +235,6 @@ TEST_F(WebSocketStreamTest, ConnectWithSuccessfulHandshake) { ...@@ -238,9 +235,6 @@ TEST_F(WebSocketStreamTest, ConnectWithSuccessfulHandshake) {
EXPECT_EQ(PropertyAsString(script_state, value, "protocol"), "chat"); EXPECT_EQ(PropertyAsString(script_state, value, "protocol"), "chat");
EXPECT_EQ(PropertyAsString(script_state, value, "extensions"), EXPECT_EQ(PropertyAsString(script_state, value, "extensions"),
"permessage-deflate"); "permessage-deflate");
// Destruction of V8TestingScope causes Close() to be called.
checkpoint.Call(1);
} }
TEST_F(WebSocketStreamTest, ConnectThenCloseCleanly) { TEST_F(WebSocketStreamTest, ConnectThenCloseCleanly) {
......
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