[DevTools] Split DevToolsHttpHandler into two classes

[DevTools] Split DevToolsHttpHandler into two classes

Also added test for memory leak this patch fixes. The leak happened if DevToolsHttpHandler failed to create socket and Stop is called after thread_ is cleared.

BUG=430909
Committed: https://crrev.com/3a7ffeaa33886408d08ce896b2e9de47eef7758d
Cr-Commit-Position: refs/heads/master@{#303818}

[vkuzkokov, 2014-10-30 18:12:29]

vkuzkokov@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2014-11-06 14:46:22]

We need a BUG with description of the problem to judge the solution.

https://codereview.chromium.org/693603003/diff/1/content/browser/devtools/dev...
File content/browser/devtools/devtools_http_handler_unittest.cc (right):

https://codereview.chromium.org/693603003/diff/1/content/browser/devtools/dev...
content/browser/devtools/devtools_http_handler_unittest.cc:102:
base::MessageLoopProxy::current()->PostTask(FROM_HERE,
Can we use RunAllPendingInMessageLoop for all the threads instead of this
error-prone thread-hopping?

[vkuzkokov, 2014-11-06 15:46:06]

https://codereview.chromium.org/693603003/diff/1/content/browser/devtools/dev...
File content/browser/devtools/devtools_http_handler_unittest.cc (right):

https://codereview.chromium.org/693603003/diff/1/content/browser/devtools/dev...
content/browser/devtools/devtools_http_handler_unittest.cc:102:
base::MessageLoopProxy::current()->PostTask(FROM_HERE,
On 2014/11/06 14:46:22, dgozman wrote:
> Can we use RunAllPendingInMessageLoop for all the threads instead of this
> error-prone thread-hopping?

Done.

[dgozman, 2014-11-07 13:03:56]

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.h (right):

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.h:55: void OnJsonRequest(int
connection_id,
Can we extract meaningful methods to a separate "dispatcher" class, and let the
handler just manage lifetime of thread, server wrapper and dispatcher?

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.h:65: static void
OnTargetListReceivedWeak(
Let's comment on the purpose of this method.

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.h:110:
scoped_ptr<ServerWrapper> server_;
Let's call this |server_wrapper_|.

[vkuzkokov, 2014-11-10 09:02:11]

Patchset #4 (id:60001) has been deleted

[vkuzkokov, 2014-11-10 09:04:34]

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.h (right):

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.h:55: void OnJsonRequest(int
connection_id,
On 2014/11/07 13:03:55, dgozman wrote:
> Can we extract meaningful methods to a separate "dispatcher" class, and let
the
> handler just manage lifetime of thread, server wrapper and dispatcher?

Extracted to static methods.

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.h:65: static void
OnTargetListReceivedWeak(
On 2014/11/07 13:03:56, dgozman wrote:
> Let's comment on the purpose of this method.

File removed.

https://codereview.chromium.org/693603003/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.h:110:
scoped_ptr<ServerWrapper> server_;
On 2014/11/07 13:03:55, dgozman wrote:
> Let's call this |server_wrapper_|.

Done.

[dgozman, 2014-11-10 11:23:36]

Overall looks good.

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.cc (left):

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:403: page_url =
target->GetURL();
Wow! Good you are fixing this.

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:205:
thread->message_loop()->DeleteSoon(FROM_HERE, server_wrapper);
Could this leak if method below is called first?

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:225: void
ServerWrapper::StartServer(
I'd make move this method out of ServerWrapper to the namespace.

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:298: void
StartServerOnFile(
nit: can we group thread/server initialization and termination methods together
and make a section for them?

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:1006: void
ServerWrapper::WriteActivePortToUserProfile(
nit: move this closer to other ServerWrapper methods?

[vkuzkokov, 2014-11-10 13:44:43]

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:205:
thread->message_loop()->DeleteSoon(FROM_HERE, server_wrapper);
On 2014/11/10 11:23:36, dgozman wrote:
> Could this leak if method below is called first?

No.

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:225: void
ServerWrapper::StartServer(
On 2014/11/10 11:23:35, dgozman wrote:
> I'd make move this method out of ServerWrapper to the namespace.

Done.

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:298: void
StartServerOnFile(
On 2014/11/10 11:23:36, dgozman wrote:
> nit: can we group thread/server initialization and termination methods
together
> and make a section for them?

Done.

https://codereview.chromium.org/693603003/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:1006: void
ServerWrapper::WriteActivePortToUserProfile(
On 2014/11/10 11:23:36, dgozman wrote:
> nit: move this closer to other ServerWrapper methods?

DevToolsHttpHandlerImpl and ServerWrapper methods are mixed anyway. Old ordering
is maintained to get a nicer diff.

[dgozman, 2014-11-10 15:39:19]

lgtm

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:154: class ServerWrapper
: net::HttpServer::Delegate {
Let's comment that this class lives on handler thread.

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:247: void
Terminate(base::Thread* thread, ServerWrapper* server_wrapper) {
TerminateOnUI

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:257: void
ServerStartedOnUI(
Let's DCHECK that thread is valid in all these static methods.

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:712: static void
OnTargetListReceivedWeak(
nit: I think this should be a static method on a class, and moved near to
OnTargetListReceived.

[vkuzkokov, 2014-11-11 08:47:25]

Patchset #6 (id:120001) has been deleted

[vkuzkokov, 2014-11-11 08:52:05]

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:154: class ServerWrapper
: net::HttpServer::Delegate {
On 2014/11/10 15:39:19, dgozman wrote:
> Let's comment that this class lives on handler thread.

Done.

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:247: void
Terminate(base::Thread* thread, ServerWrapper* server_wrapper) {
On 2014/11/10 15:39:19, dgozman wrote:
> TerminateOnUI

Done.

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:257: void
ServerStartedOnUI(
On 2014/11/10 15:39:19, dgozman wrote:
> Let's DCHECK that thread is valid in all these static methods.

Done.

https://codereview.chromium.org/693603003/diff/100001/content/browser/devtool...
content/browser/devtools/devtools_http_handler_impl.cc:712: static void
OnTargetListReceivedWeak(
On 2014/11/10 15:39:19, dgozman wrote:
> nit: I think this should be a static method on a class, and moved near to
> OnTargetListReceived.

Done.

[vkuzkokov, 2014-11-11 08:52:15]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-11-11 08:52:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/693603003/140001

[vkuzkokov, 2014-11-11 10:55:46]

vkuzkokov@chromium.org changed reviewers:
+ jamescook@chromium.org

[vkuzkokov, 2014-11-11 10:55:46]

jamescook, PTAL:
extensions/shell/browser/shell_browser_main_parts.cc

DevToolsHttpHandler::Stop needs to be called while FILE thread is still alive.
Turns out, ~ShellBrowserMainParts is too late for that.

[James Cook, 2014-11-11 18:32:31]

LGTM with nit

https://codereview.chromium.org/693603003/diff/180001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/693603003/diff/180001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:214: if
(devtools_http_handler_) {
If possible please move this to immediately after
browser_main_delegate_->Shutdown() so that the various services are shut down
and deleted in the opposite order to which they are created.

If you can't do that, please add a comment explaining why it must be destroyed
in this order.

[vkuzkokov, 2014-11-12 09:41:20]

https://codereview.chromium.org/693603003/diff/180001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/693603003/diff/180001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:214: if
(devtools_http_handler_) {
On 2014/11/11 18:32:31, James Cook wrote:
> If possible please move this to immediately after
> browser_main_delegate_->Shutdown() so that the various services are shut down
> and deleted in the opposite order to which they are created.
> 
> If you can't do that, please add a comment explaining why it must be destroyed
> in this order.

Moved.

[vkuzkokov, 2014-11-12 09:47:03]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-11-12 09:48:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/693603003/200001

[commit-bot: I haz the power, 2014-11-12 09:51:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-12 09:51:07]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/80439)

[vkuzkokov, 2014-11-12 10:41:29]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-11-12 10:41:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/693603003/220001

[commit-bot: I haz the power, 2014-11-12 12:02:51]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2014-11-12 12:03:29]

Patchset 10 (id:??) landed as
https://crrev.com/3a7ffeaa33886408d08ce896b2e9de47eef7758d
Cr-Commit-Position: refs/heads/master@{#303818}