[DevTools] Keep IP address of HttpServer as a member of DevToolsHttpHandlerImpl.

[DevTools] Keep IP address of HttpServer as a member of DevToolsHttpHandlerImpl.

Fix the problem that loading inspector from content_shell is failing with
thread_checker_.CalledOnValidThread() failed.

At ShellDevToolsFrontend::Show(), DevToolsHttpHandlerImpl::GetFrontendURL()
is called to get the frontend url, and inside the function,
HttpServer::GetLocalAddress() is called directly which is expected to be
called on the DevToolsHandler thread only.

To prevent the DCHECK failure, this patch keeps the ip address of HttpServer
as a member of DevToolsHttpHandlerImpl instance and use it.

BUG=425423
Committed: https://crrev.com/9fd9f0a04570cefb62b18bdac864636fc7e05026
Cr-Commit-Position: refs/heads/master@{#301684}

[byungwoo, 2014-10-21 04:48:26]

bw80.lee@samsung.com changed reviewers:
+ dgozman@chromium.org, kaznacheev@chromium.org

[byungwoo, 2014-10-21 05:05:19]

bw80.lee@samsung.com changed reviewers:
+ pfeldman@chromium.org, vkuzkokov@chromium.org

[pfeldman, 2014-10-21 11:32:47]

It still has a race since the server address is assigned on one thread and is
read on another.

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

https://codereview.chromium.org/666673007/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:336: if
(!server_ip_address_)
This is called on the UI thread.

https://codereview.chromium.org/666673007/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:800:
server_ip_address_.reset(new net::IPEndPoint(ip_address));
This is called on the handler thread.

[byungwoo, 2014-10-21 11:52:02]

On 2014/10/21 11:32:47, pfeldman wrote:
> It still has a race since the server address is assigned on one thread and is
> read on another.

Yes right. This fixed the DCHECK failure but still has the race as previous.
So, is it better to add a mutex for server_ip_address_ to handle the race
condition?

[byungwoo, 2014-10-22 00:51:50]

I updated the patch with adding lock for the server_ip_address_.
Please take a look again :)

[vkuzkokov, 2014-10-22 13:20:02]

vkuzkokov@chromium.org changed reviewers:
- kaznacheev@chromium.org

[vkuzkokov, 2014-10-22 13:20:03]

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

https://codereview.chromium.org/666673007/diff/40001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:799: base::AutoLock
locked(server_ip_address_lock_);
Use of locks is very limited in chromium, especially on the UI thread.
The go-to way for doing this is to post IPEndPoint using BrowserThread::PostTask
and read and modify server_ip_address_ only on UI.

[byungwoo, 2014-10-23 02:10:20]

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

https://codereview.chromium.org/666673007/diff/40001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:799: base::AutoLock
locked(server_ip_address_lock_);
On 2014/10/22 13:20:03, vkuzkokov wrote:
> Use of locks is very limited in chromium, especially on the UI thread.
> The go-to way for doing this is to post IPEndPoint using
BrowserThread::PostTask
> and read and modify server_ip_address_ only on UI.

Thank you for the review and suggestion.
I'll apply it.

[byungwoo, 2014-10-27 00:43:49]

Could you please take a look?

[vkuzkokov, 2014-10-27 09:51:44]

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

https://codereview.chromium.org/666673007/diff/80001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:830:
thread_->message_loop()->PostTask(
It seems like you could clear server_ip_address_ at this point instead of
posting back to UI from Teardown.

[byungwoo, 2014-10-27 10:51:55]

On 2014/10/27 09:51:44, vkuzkokov wrote:
>
https://codereview.chromium.org/666673007/diff/80001/content/browser/devtools...
> content/browser/devtools/devtools_http_handler_impl.cc:830:
> thread_->message_loop()->PostTask(
> It seems like you could clear server_ip_address_ at this point instead of
> posting back to UI from Teardown.
Yes, right. Thank you for the review.
I applied and uploaded new patch set. :)

[vkuzkokov, 2014-10-27 11:31:58]

On 2014/10/27 10:51:55, byungwoo wrote:
> On 2014/10/27 09:51:44, vkuzkokov wrote:
> >
>
https://codereview.chromium.org/666673007/diff/80001/content/browser/devtools...
> > content/browser/devtools/devtools_http_handler_impl.cc:830:
> > thread_->message_loop()->PostTask(
> > It seems like you could clear server_ip_address_ at this point instead of
> > posting back to UI from Teardown.
> Yes, right. Thank you for the review.
> I applied and uploaded new patch set. :)

I meant clearing it in StopHandlerThread before (or after - that shouldn't
matter) posting Teardown.
Teardown itself is called on handler thread and changing server_ip_address_ in
it will cause race condition.

Also, a nit: instead of .reset(NULL) just .reset() is used.

[byungwoo, 2014-10-27 11:48:26]

On 2014/10/27 11:31:58, vkuzkokov wrote:
> On 2014/10/27 10:51:55, byungwoo wrote:
> > On 2014/10/27 09:51:44, vkuzkokov wrote:
> > >
> >
>
https://codereview.chromium.org/666673007/diff/80001/content/browser/devtools...
> > > content/browser/devtools/devtools_http_handler_impl.cc:830:
> > > thread_->message_loop()->PostTask(
> > > It seems like you could clear server_ip_address_ at this point instead of
> > > posting back to UI from Teardown.
> > Yes, right. Thank you for the review.
> > I applied and uploaded new patch set. :)
> 
> I meant clearing it in StopHandlerThread before (or after - that shouldn't
> matter) posting Teardown.
> Teardown itself is called on handler thread and changing server_ip_address_ in
> it will cause race condition.
> 
> Also, a nit: instead of .reset(NULL) just .reset() is used.

Ok. I thought that you told it would be ok to clear it in TearDown because it is
called during the cleanup sequence.
Sorry for my misunderstanding. 
As you told, accessing the ip address member in handler thread can cause race
condition as pointed previously.
I will apply it again.
(And I'll fix the server_.reset(NULL) to .reset() as you pointed)

[vkuzkokov, 2014-10-27 13:09:58]

lgtm

[pfeldman, 2014-10-27 13:43:01]

lgtm

[vkuzkokov, 2014-10-27 13:52:00]

On 2014/10/27 13:09:58, vkuzkokov wrote:
> lgtm

My bad: StopHandlerThread doesn't run on UI - it runs on FILE as is written in
the comment right above it (Duh).
So server_ip_address_ should be cleared even earlier: from Stop and
StopWithoutRelease.

[byungwoo, 2014-10-27 15:31:32]

On 2014/10/27 13:52:00, vkuzkokov wrote:
> So server_ip_address_ should be cleared even earlier: from Stop and
> StopWithoutRelease.

How about clearing it at ResetHandlerThread() ?
ResetHandlerThread() will be called on the UI thread after StopHandlerThread()
is finished on the FILE thread.

[vkuzkokov, 2014-10-27 15:36:27]

On 2014/10/27 15:31:32, byungwoo wrote:
> On 2014/10/27 13:52:00, vkuzkokov wrote:
> > So server_ip_address_ should be cleared even earlier: from Stop and
> > StopWithoutRelease.
> 
> How about clearing it at ResetHandlerThread() ?
> ResetHandlerThread() will be called on the UI thread after StopHandlerThread()
> is finished on the FILE thread.

That should work just as well.

[byungwoo, 2014-10-27 15:53:01]

On 2014/10/27 15:36:27, vkuzkokov wrote:
> On 2014/10/27 15:31:32, byungwoo wrote:
> > On 2014/10/27 13:52:00, vkuzkokov wrote:
> > > So server_ip_address_ should be cleared even earlier: from Stop and
> > > StopWithoutRelease.
> > 
> > How about clearing it at ResetHandlerThread() ?
> > ResetHandlerThread() will be called on the UI thread after
StopHandlerThread()
> > is finished on the FILE thread.
> 
> That should work just as well.

Applied and uploaded again. Thanks :)

[vkuzkokov, 2014-10-27 15:55:47]

On 2014/10/27 15:53:01, byungwoo wrote:
> On 2014/10/27 15:36:27, vkuzkokov wrote:
> > On 2014/10/27 15:31:32, byungwoo wrote:
> > > On 2014/10/27 13:52:00, vkuzkokov wrote:
> > > > So server_ip_address_ should be cleared even earlier: from Stop and
> > > > StopWithoutRelease.
> > > 
> > > How about clearing it at ResetHandlerThread() ?
> > > ResetHandlerThread() will be called on the UI thread after
> StopHandlerThread()
> > > is finished on the FILE thread.
> > 
> > That should work just as well.
> 
> Applied and uploaded again. Thanks :)

lgtm

[vkuzkokov, 2014-10-28 15:49:29]

The CQ bit was checked by vkuzkokov@chromium.org

[vkuzkokov, 2014-10-28 15:49:48]

The CQ bit was unchecked by vkuzkokov@chromium.org

[vkuzkokov, 2014-10-28 15:57:11]

If you are ready to commit this feel free to do so.

[byungwoo, 2014-10-28 16:08:31]

The CQ bit was checked by bw80.lee@samsung.com

[commit-bot: I haz the power, 2014-10-28 16:09:55]

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

[commit-bot: I haz the power, 2014-10-28 16:27:59]

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

[commit-bot: I haz the power, 2014-10-28 16:28:01]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vkuzkokov, 2014-10-28 18:19:52]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-10-28 18:22:18]

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

[commit-bot: I haz the power, 2014-10-28 19:23:17]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-28 19:24:02]

Patchset 8 (id:??) landed as
https://crrev.com/9fd9f0a04570cefb62b18bdac864636fc7e05026
Cr-Commit-Position: refs/heads/master@{#301684}