Add IPC for DNS host address resolution.

Add IPC for DNS host address resolution.

TEST=None
BUG=83242
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96047

[Sergey Ulanov, 2011-08-08 18:51:34]

John, please review
 content/browser/renderer_host/browser_render_process_host.cc
 content/common/p2p_messages.h
 content/content_renderer.gypi

Albert - everything else.

[Wez, 2011-08-08 21:47:18]

Some unsolicited comments.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:101:
DCHECK(sockets_.empty());
Check |dns_requests_| here, or clean it up?

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:207: Send(new
P2PMsg_GetHostAddressResult(
Can we bubble the net error-code back across the IPC channel to the renderer,
rather than just success or failure?

http://codereview.chromium.org/7599003/diff/1/content/common/p2p_messages.h
File content/common/p2p_messages.h (right):

http://codereview.chromium.org/7599003/diff/1/content/common/p2p_messages.h#n...
content/common/p2p_messages.h:29: net::IPAddressNumber /* address */)
This only seems to allow a host to resolve to a single address?

http://codereview.chromium.org/7599003/diff/1/content/renderer/p2p/host_addre...
File content/renderer/p2p/host_address_request.cc (right):

http://codereview.chromium.org/7599003/diff/1/content/renderer/p2p/host_addre...
content/renderer/p2p/host_address_request.cc:21:
P2PHostAddressRequest::~P2PHostAddressRequest() {
nit: Check that you're being torn-down on the right thread?

http://codereview.chromium.org/7599003/diff/1/content/renderer/p2p/host_addre...
content/renderer/p2p/host_address_request.cc:31:
&P2PHostAddressRequest::DoSendRequest, this, host_name, done_callback));
If the IPC message-loop has been torn down, this callback will never arrive, so
this request will stay in the SENT state and we'll DCHECK() when we tear it
down?

[awong, 2011-08-09 00:25:56]

looks mostly good.  Only one major comment question about lifetime of tasks.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:33: const
DoneCallback& done_callback) {
DCHECK here that done_callback is initialized?

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:61:
done_callback_.Run(net::IPAddressNumber());
I'd prefer a return here, and then move the whole else clouse back up a level.

Seems to fit better with the early exist style of error handling we usually use.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:200:
base::Unretained(this), request));
Can the P2PSocketDispatcherHost be deleted before this callback runs?

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:17: }  // content
namespace content

[Sergey Ulanov, 2011-08-09 00:41:35]

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right):

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:33: const
DoneCallback& done_callback) {
On 2011/08/09 00:25:56, awong wrote:
> DCHECK here that done_callback is initialized?

Done.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:61:
done_callback_.Run(net::IPAddressNumber());
On 2011/08/09 00:25:56, awong wrote:
> I'd prefer a return here, and then move the whole else clouse back up a level.
> 
> Seems to fit better with the early exist style of error handling we usually
use.

Done.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:101:
DCHECK(sockets_.empty());
On 2011/08/08 21:47:18, Wez wrote:
> Check |dns_requests_| here, or clean it up?

Done.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:200:
base::Unretained(this), request));
On 2011/08/09 00:25:56, awong wrote:
> Can the P2PSocketDispatcherHost be deleted before this callback runs?
Yes, but all pending requests will be deleted before that (see
OnChannelClosing()), so the callback will never be called after this object is
destroyed.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.cc:207: Send(new
P2PMsg_GetHostAddressResult(
On 2011/08/08 21:47:18, Wez wrote:
> Can we bubble the net error-code back across the IPC channel to the renderer,
> rather than just success or failure?
We currently don't do it for any other P2P IPC, and there isn't much renderer
can do with the code anyway. Beside that, there may be some security
implications of returning error to the renderer, so I would rather avoid it,
unless we know we need it.

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

http://codereview.chromium.org/7599003/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:17: }  // content
On 2011/08/09 00:25:56, awong wrote:
> namespace content

Done.

http://codereview.chromium.org/7599003/diff/1/content/common/p2p_messages.h
File content/common/p2p_messages.h (right):

http://codereview.chromium.org/7599003/diff/1/content/common/p2p_messages.h#n...
content/common/p2p_messages.h:29: net::IPAddressNumber /* address */)
On 2011/08/08 21:47:18, Wez wrote:
> This only seems to allow a host to resolve to a single address?
This reflects current behavior of libjingle (which simply calls gethostbyname()
), and normally STUN server domain names resolve to a single IP address.

http://codereview.chromium.org/7599003/diff/1/content/renderer/p2p/host_addre...
File content/renderer/p2p/host_address_request.cc (right):

http://codereview.chromium.org/7599003/diff/1/content/renderer/p2p/host_addre...
content/renderer/p2p/host_address_request.cc:21:
P2PHostAddressRequest::~P2PHostAddressRequest() {
On 2011/08/08 21:47:18, Wez wrote:
> nit: Check that you're being torn-down on the right thread?
Because this object is refcounted, there are not guarantees about thread that
destroys this object.

http://codereview.chromium.org/7599003/diff/1/content/renderer/p2p/host_addre...
content/renderer/p2p/host_address_request.cc:31:
&P2PHostAddressRequest::DoSendRequest, this, host_name, done_callback));
On 2011/08/08 21:47:18, Wez wrote:
> If the IPC message-loop has been torn down, this callback will never arrive,
so
> this request will stay in the SENT state and we'll DCHECK() when we tear it
> down?

The DCHECK in the destructor is supposed to check that ether the request was
finished (callback called) or Cancel() was called. In the case when IPC thread
is destroyed, the request may never be sent, so the code that uses this class
must call Cancel() before dropping reference to this object.

[awong, 2011-08-09 19:37:44]

LGTM

[jam, 2011-08-09 19:52:53]

lgtm, sorryI thought I already lgtm'd this before