From b0ec7c0643c7f84db377db2b8fbe23917088b91c Mon Sep 17 00:00:00 2001 From: "kaznacheev@chromium.org" Date: Fri, 25 Jul 2014 11:57:13 +0000 Subject: Revert of [WebSocket] Send a close frame when the renderer process is gone. (https://codereview.chromium.org/390773002/) Reason for revert: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%282%29/builds/5350 Original issue's description: > [WebSocket] Send a close frame when the renderer process is gone. > > Renderer processes may die without clearing its |WebSocketBridge|s. > It is helpful to send a close frame with code = 1001 in such a case. > > BUG=392534 > R=ricea@chromium.org > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285557 TBR=ricea@chromium.org,jgraettinger@chromium.org,eroman@chromium.org,mmenke@chromium.org,yhirano@chromium.org NOTREECHECKS=true NOTRY=true BUG=392534 Review URL: https://codereview.chromium.org/416333004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285564 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/net/websocket_browsertest.cc | 89 ++++++---------------- .../renderer_host/websocket_dispatcher_host.cc | 16 ---- .../websocket_dispatcher_host_unittest.cc | 49 +----------- content/browser/renderer_host/websocket_host.cc | 5 -- content/browser/renderer_host/websocket_host.h | 6 -- net/data/websocket/README | 15 ---- net/data/websocket/connect_check.html | 13 +++- net/data/websocket/count-connection_wsh.py | 26 ------- net/data/websocket/count_connection.html | 22 ------ net/data/websocket/counted_connection.html | 19 ----- net/data/websocket/split_packet_check.html | 12 ++- net/data/websocket/websocket_shared_worker.html | 11 ++- net/websockets/websocket_channel.cc | 2 - 13 files changed, 59 insertions(+), 226 deletions(-) delete mode 100644 net/data/websocket/count-connection_wsh.py delete mode 100644 net/data/websocket/count_connection.html delete mode 100644 net/data/websocket/counted_connection.html diff --git a/chrome/browser/net/websocket_browsertest.cc b/chrome/browser/net/websocket_browsertest.cc index a0a65b8..58605de 100644 --- a/chrome/browser/net/websocket_browsertest.cc +++ b/chrome/browser/net/websocket_browsertest.cc @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include - #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" #include "chrome/browser/ui/browser.h" @@ -13,39 +11,21 @@ #include "content/public/test/browser_test_utils.h" #include "net/base/test_data_directory.h" #include "net/test/spawned_test_server/spawned_test_server.h" -#include "url/gurl.h" namespace { class WebSocketBrowserTest : public InProcessBrowserTest { public: WebSocketBrowserTest() - : ws_server_(net::SpawnedTestServer::TYPE_WS, - net::SpawnedTestServer::kLocalhost, - net::GetWebSocketTestDataDirectory()), - wss_server_(net::SpawnedTestServer::TYPE_WSS, - SSLOptions(SSLOptions::CERT_OK), - net::GetWebSocketTestDataDirectory()) {} - - protected: - void NavigateToHTTP(const std::string& path) { - // Visit a HTTP page for testing. - std::string scheme("http"); - GURL::Replacements replacements; - replacements.SetSchemeStr(scheme); - ui_test_utils::NavigateToURL( - browser(), ws_server_.GetURL(path).ReplaceComponents(replacements)); - } - - void NavigateToHTTPS(const std::string& path) { - // Visit a HTTPS page for testing. - std::string scheme("https"); - GURL::Replacements replacements; - replacements.SetSchemeStr(scheme); - ui_test_utils::NavigateToURL( - browser(), wss_server_.GetURL(path).ReplaceComponents(replacements)); + : ws_server_(net::SpawnedTestServer::TYPE_WS, + net::SpawnedTestServer::kLocalhost, + net::GetWebSocketTestDataDirectory()), + wss_server_(net::SpawnedTestServer::TYPE_WSS, + SSLOptions(SSLOptions::CERT_OK), + net::GetWebSocketTestDataDirectory()) { } + protected: net::SpawnedTestServer ws_server_; net::SpawnedTestServer wss_server_; @@ -67,10 +47,17 @@ IN_PROC_BROWSER_TEST_F(WebSocketBrowserTest, WebSocketSplitSegments) { content::TitleWatcher watcher(tab, base::ASCIIToUTF16("PASS")); watcher.AlsoWaitForTitle(base::ASCIIToUTF16("FAIL")); - NavigateToHTTP("split_packet_check.html"); + // Visit a HTTP page for testing. + std::string scheme("http"); + GURL::Replacements replacements; + replacements.SetSchemeStr(scheme); + ui_test_utils::NavigateToURL( + browser(), + ws_server_.GetURL( + "split_packet_check.html").ReplaceComponents(replacements)); const base::string16 result = watcher.WaitAndGetTitle(); - EXPECT_EQ(base::ASCIIToUTF16("PASS"), result); + EXPECT_TRUE(EqualsASCII(result, "PASS")); } IN_PROC_BROWSER_TEST_F(WebSocketBrowserTest, SecureWebSocketSplitRecords) { @@ -83,43 +70,17 @@ IN_PROC_BROWSER_TEST_F(WebSocketBrowserTest, SecureWebSocketSplitRecords) { content::TitleWatcher watcher(tab, base::ASCIIToUTF16("PASS")); watcher.AlsoWaitForTitle(base::ASCIIToUTF16("FAIL")); - NavigateToHTTPS("split_packet_check.html"); + // Visit a HTTPS page for testing. + std::string scheme("https"); + GURL::Replacements replacements; + replacements.SetSchemeStr(scheme); + ui_test_utils::NavigateToURL( + browser(), + wss_server_.GetURL( + "split_packet_check.html").ReplaceComponents(replacements)); const base::string16 result = watcher.WaitAndGetTitle(); - EXPECT_EQ(base::ASCIIToUTF16("PASS"), result); -} - -IN_PROC_BROWSER_TEST_F(WebSocketBrowserTest, SendCloseFrameWhenTabIsClosed) { - // Launch a WebSocket server. - ASSERT_TRUE(ws_server_.Start()); - - content::WebContents* tab = - browser()->tab_strip_model()->GetActiveWebContents(); - { - // Create a new tab, establish a WebSocket connection and close the tab. - content::WebContents* new_tab = content::WebContents::Create( - content::WebContents::CreateParams(tab->GetBrowserContext())); - browser()->tab_strip_model()->AppendWebContents(new_tab, true); - ASSERT_EQ(new_tab, browser()->tab_strip_model()->GetWebContentsAt(1)); - - content::TitleWatcher connected_title_watcher( - new_tab, base::ASCIIToUTF16("CONNECTED")); - connected_title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16("CLOSED")); - NavigateToHTTP("counted_connection.html"); - const base::string16 result = connected_title_watcher.WaitAndGetTitle(); - EXPECT_TRUE(EqualsASCII(result, "CONNECTED")); - - content::WebContentsDestroyedWatcher destroyed_watcher(new_tab); - browser()->tab_strip_model()->CloseWebContentsAt(1, 0); - destroyed_watcher.Wait(); - } - - content::TitleWatcher title_watcher(tab, base::ASCIIToUTF16("PASS")); - title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16("FAIL")); - - NavigateToHTTP("count_connection.html"); - const base::string16 result = title_watcher.WaitAndGetTitle(); - EXPECT_EQ(base::ASCIIToUTF16("PASS"), result); + EXPECT_TRUE(EqualsASCII(result, "PASS")); } } // namespace diff --git a/content/browser/renderer_host/websocket_dispatcher_host.cc b/content/browser/renderer_host/websocket_dispatcher_host.cc index d49199a..2af2f08 100644 --- a/content/browser/renderer_host/websocket_dispatcher_host.cc +++ b/content/browser/renderer_host/websocket_dispatcher_host.cc @@ -5,7 +5,6 @@ #include "content/browser/renderer_host/websocket_dispatcher_host.h" #include -#include #include "base/callback.h" #include "base/logging.h" @@ -179,21 +178,6 @@ WebSocketHostState WebSocketDispatcherHost::DoDropChannel( } WebSocketDispatcherHost::~WebSocketDispatcherHost() { - std::vector hosts; - for (base::hash_map::const_iterator i = hosts_.begin(); - i != hosts_.end(); ++i) { - // In order to avoid changing the container while iterating, we copy - // the hosts. - hosts.push_back(i->second); - } - - for (size_t i = 0; i < hosts.size(); ++i) { - // Note that some calls to GoAway could fail. In that case hosts[i] will be - // deleted and removed from |hosts_| in |DoDropChannel|. - hosts[i]->GoAway(); - hosts[i] = NULL; - } - STLDeleteContainerPairSecondPointers(hosts_.begin(), hosts_.end()); } diff --git a/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc b/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc index 8504d8f..e1506d9 100644 --- a/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc +++ b/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc @@ -4,7 +4,6 @@ #include "content/browser/renderer_host/websocket_dispatcher_host.h" -#include #include #include "base/bind.h" @@ -24,30 +23,23 @@ namespace { // This number is unlikely to occur by chance. static const int kMagicRenderProcessId = 506116062; -class WebSocketDispatcherHostTest; - // A mock of WebsocketHost which records received messages. class MockWebSocketHost : public WebSocketHost { public: MockWebSocketHost(int routing_id, WebSocketDispatcherHost* dispatcher, - net::URLRequestContext* url_request_context, - WebSocketDispatcherHostTest* owner) - : WebSocketHost(routing_id, dispatcher, url_request_context), - owner_(owner) { + net::URLRequestContext* url_request_context) + : WebSocketHost(routing_id, dispatcher, url_request_context) { } virtual ~MockWebSocketHost() {} - virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE { + virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE{ received_messages_.push_back(message); return true; } - virtual void GoAway() OVERRIDE; - std::vector received_messages_; - WebSocketDispatcherHostTest* owner_; }; class WebSocketDispatcherHostTest : public ::testing::Test { @@ -63,17 +55,12 @@ class WebSocketDispatcherHostTest : public ::testing::Test { virtual ~WebSocketDispatcherHostTest() {} - void GoAway(int routing_id) { - gone_hosts_.push_back(routing_id); - } - protected: scoped_refptr dispatcher_host_; // Stores allocated MockWebSocketHost instances. Doesn't take ownership of // them. std::vector mock_hosts_; - std::vector gone_hosts_; private: net::URLRequestContext* OnGetRequestContext() { @@ -82,16 +69,12 @@ class WebSocketDispatcherHostTest : public ::testing::Test { WebSocketHost* CreateWebSocketHost(int routing_id) { MockWebSocketHost* host = - new MockWebSocketHost(routing_id, dispatcher_host_.get(), NULL, this); + new MockWebSocketHost(routing_id, dispatcher_host_.get(), NULL); mock_hosts_.push_back(host); return host; } }; -void MockWebSocketHost::GoAway() { - owner_->GoAway(routing_id()); -} - TEST_F(WebSocketDispatcherHostTest, Construct) { // Do nothing. } @@ -173,29 +156,5 @@ TEST_F(WebSocketDispatcherHostTest, SendFrame) { } } -TEST_F(WebSocketDispatcherHostTest, Destruct) { - WebSocketHostMsg_AddChannelRequest message1( - 123, GURL("ws://example.com/test"), std::vector(), - url::Origin("http://example.com"), -1); - WebSocketHostMsg_AddChannelRequest message2( - 456, GURL("ws://example.com/test2"), std::vector(), - url::Origin("http://example.com"), -1); - - ASSERT_TRUE(dispatcher_host_->OnMessageReceived(message1)); - ASSERT_TRUE(dispatcher_host_->OnMessageReceived(message2)); - - ASSERT_EQ(2u, mock_hosts_.size()); - - mock_hosts_.clear(); - dispatcher_host_ = NULL; - - ASSERT_EQ(2u, gone_hosts_.size()); - // The gone_hosts_ ordering is not predictable because it depends on the - // hash_map ordering. - std::sort(gone_hosts_.begin(), gone_hosts_.end()); - EXPECT_EQ(123, gone_hosts_[0]); - EXPECT_EQ(456, gone_hosts_[1]); -} - } // namespace } // namespace content diff --git a/content/browser/renderer_host/websocket_host.cc b/content/browser/renderer_host/websocket_host.cc index f778f06..7f63918 100644 --- a/content/browser/renderer_host/websocket_host.cc +++ b/content/browser/renderer_host/websocket_host.cc @@ -17,7 +17,6 @@ #include "net/http/http_util.h" #include "net/ssl/ssl_info.h" #include "net/websockets/websocket_channel.h" -#include "net/websockets/websocket_errors.h" #include "net/websockets/websocket_event_interface.h" #include "net/websockets/websocket_frame.h" // for WebSocketFrameHeader::OpCode #include "net/websockets/websocket_handshake_request_info.h" @@ -331,10 +330,6 @@ WebSocketHost::WebSocketHost(int routing_id, WebSocketHost::~WebSocketHost() {} -void WebSocketHost::GoAway() { - OnDropChannel(false, static_cast(net::kWebSocketErrorGoingAway), ""); -} - bool WebSocketHost::OnMessageReceived(const IPC::Message& message) { bool handled = true; IPC_BEGIN_MESSAGE_MAP(WebSocketHost, message) diff --git a/content/browser/renderer_host/websocket_host.h b/content/browser/renderer_host/websocket_host.h index 39c1e41..21c77dc 100644 --- a/content/browser/renderer_host/websocket_host.h +++ b/content/browser/renderer_host/websocket_host.h @@ -40,16 +40,10 @@ class CONTENT_EXPORT WebSocketHost { net::URLRequestContext* url_request_context); virtual ~WebSocketHost(); - // The renderer process is going away. - // This function is virtual for testing. - virtual void GoAway(); - // General message dispatch. WebSocketDispatcherHost::OnMessageReceived // delegates to this method after looking up the |routing_id|. virtual bool OnMessageReceived(const IPC::Message& message); - int routing_id() const { return routing_id_; } - private: // Handlers for each message type, dispatched by OnMessageReceived(), as // defined in content/common/websocket_messages.h diff --git a/net/data/websocket/README b/net/data/websocket/README index 9282a38..9dab697 100644 --- a/net/data/websocket/README +++ b/net/data/websocket/README @@ -19,16 +19,6 @@ Multiple tests may share one resource, or URI handler. content::TitleWatcher. Used by WorkerTest.WebSocketSharedWorker. -- counted_connection.html : A page that creates a WebSocket connection - to count-connection_wsh. - This file does NOT close the established connection. - Used by SendCloseFrameWhenTabIsClosed. - -- count_connection.html : A page that creates a WebSocket connection - to count-connection_wsh and checks the number of open and closed - websocket connections to the handler. - Used by SendCloseFrameWhenTabIsClosed. - - websocket_worker_simple.js : A JavaScript runs on Workers created from the websocket_shared_worker.html. Used by WorkerTest.WebSocketSharedWorker. @@ -54,11 +44,6 @@ Multiple tests may share one resource, or URI handler. Used by WebSocketBrowserTest.WebSocketSplitSegments and WebSocketBrowserTest.SecureWebSocketSplitRecords. -- count-connection_wsh.py : counts the number of open and closed websocket - connections to this handler. - It sends the numbers after each connection establishment. - Used by SendCloseFrameWhenTabIsClosed. - - protocol-test_wsh.py : A WebSocket URL handler for testing outgoing opening handshake protocol. Used by kinds of PPAPI tests for WebSocket. diff --git a/net/data/websocket/connect_check.html b/net/data/websocket/connect_check.html index 8d8e815..4a1ca8c 100644 --- a/net/data/websocket/connect_check.html +++ b/net/data/websocket/connect_check.html @@ -3,8 +3,17 @@ test ws connection - - diff --git a/net/data/websocket/counted_connection.html b/net/data/websocket/counted_connection.html deleted file mode 100644 index bb53ec1..0000000 --- a/net/data/websocket/counted_connection.html +++ /dev/null @@ -1,19 +0,0 @@ - - - -Connect to a WebSocket server - - - diff --git a/net/data/websocket/split_packet_check.html b/net/data/websocket/split_packet_check.html index 6b7e980a..a7f4c36 100644 --- a/net/data/websocket/split_packet_check.html +++ b/net/data/websocket/split_packet_check.html @@ -3,8 +3,16 @@ test ws split packet + diff --git a/net/websockets/websocket_channel.cc b/net/websockets/websocket_channel.cc index 767d632..12f152f 100644 --- a/net/websockets/websocket_channel.cc +++ b/net/websockets/websocket_channel.cc @@ -465,8 +465,6 @@ void WebSocketChannel::SendFlowControl(int64 quota) { void WebSocketChannel::StartClosingHandshake(uint16 code, const std::string& reason) { if (InClosingState()) { - // When the associated renderer process is killed while the channel is in - // CLOSING state we reach here. DVLOG(1) << "StartClosingHandshake called in state " << state_ << ". This may be a bug, or a harmless race."; return; -- cgit v1.1