Issue 15733008: Multicast DNS implementation (initial)

Noam Samuel

7 years, 7 months ago (2013-05-23 20:26:38 UTC) #1

szym

I have only skimmed it, but overall it seems that the reason you need to ...

7 years, 7 months ago (2013-05-24 15:32:22 UTC) #2

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h File net/dns/mdns_listener.h (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newcode31 net/dns/mdns_listener.h:31: }; MDnsCache::UpdateType contains a NoChange member, which can never ...

7 years, 7 months ago (2013-05-24 19:00:49 UTC) #3

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h
File net/dns/mdns_listener.h (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:31: };
MDnsCache::UpdateType contains a NoChange member, which can never be used as an
update type. I thought it would be unclear if they used the same enum (users
will think they have to handle the NoChange case, and compilers will produce
errors for switch statements that don't handle it).

On 2013/05/24 15:32:22, szym wrote:
> Seems redundant with MDnsCache::UpdateType.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:49: // record.
Maybe the TODO should be moved somewhere else. The TODO refers to actually
implementing NSEC record parsing.

On 2013/05/24 15:32:22, szym wrote:
> Not sure why there's TODO here. What part is missing?

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:107: // Whether this listener is active (true) or
passive (false)
On 2013/05/24 15:32:22, szym wrote:
> What does that mean?

An active listener is allowed to send periodic queries to see if new records of
a given type are available.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:49: if (!RecieveOnePacket(socket_ipv4_.get(),
On 2013/05/24 15:32:22, szym wrote:
> What is the point of doing this?

Oh. I guess this method is somewhat misnamed. RecieveOnePacket is used to
request a packet. It sets a handler for receiving the packet that processes it
and then requests the next packet. This is the "main loop" for the mDNS
listener. Not 100% sure what to name it; maybe "RecieveNextPacket"?

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) );
Listener delegates may create or delete any listeners. Thus, the iterator may
become invalid while iterating over the listeners. 

On a second look, this can be solved by storing weak pointers to the listeners
and copying them to a separate collection before iterating over them.

Another problem is that a listener delegate is allowed to delete all listeners,
which would stop the client from listening for MDns packets, and would in turn
delete the cache (since it can no longer be kept up to date), which owns the
record, and thus delete the record itself. Therefore, IMO, it makes sense to at
least copy the record, even if we call the delegates synchronously.

Alternately, it is be possible to make the starting/stopping of listening for
mDNS more explicit by requiring users of the API to call AddListenRef and
RemoveListenRef explicitly when they start/stop using mDNS.

On 2013/05/24 15:32:22, szym wrote:
> Why not just call: l->second->AlertDelegate(update_type, record)?

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h
File net/dns/mdns_listener_impl.h (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h#...
net/dns/mdns_listener_impl.h:28: class Core : public base::SupportsWeakPtr<Core>
{
On 2013/05/24 15:32:22, szym wrote:
> Needs comment regarding lifetime. When is Core created? Who can hold weak
> pointers to it?

The weak pointer-ability of these classes is mostly used to allow scheduling
methods on them through the message loop.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h#...
net/dns/mdns_listener_impl.h:67: // Request another packet from the network.
It does wait indefinitely. It does so in an asynchronous fashion. Since every
MDns client has to act as a server, this is actually part of a listening loop
that continually listens for packets sent to the mDNS multicast group.

On 2013/05/24 15:32:22, szym wrote:
> The implementation shows that this calls RecvFrom without sending anything
out.
> Why would anyone send us a packet? Can it wait indefinitely?
> 
> What does this return?

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h File net/dns/mdns_listener.h (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newcode18 net/dns/mdns_listener.h:18: extern const char kMDNSMulticastGroupIPv4[]; I put it here so ...

7 years, 7 months ago (2013-05-24 21:59:17 UTC) #4

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc File net/dns/mdns_listener_impl.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc#newcode233 net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) ); On second thought, deferring shutting down listening ...

7 years, 7 months ago (2013-05-24 22:34:20 UTC) #5

szym

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc File net/dns/mdns_listener_impl.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc#newcode120 net/dns/mdns_listener_impl.cc:120: AsWeakPtr(), socket, response, recv_addr)); You don't really need WeakPtr ...

7 years, 7 months ago (2013-05-25 02:51:01 UTC) #6

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:120: AsWeakPtr(), socket, response, recv_addr));
You don't really need WeakPtr here since if Core is gone, so is |socket|.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:125:
base::Bind(&MDnsListenerFactoryImpl::Core::OnDatagramRecieved,
Why not call synchronously?

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) );
You could consider using std::map<ListenerKey, ObserverList<MDnsListenerImpl> >
instead of std::multimap. ObserverList uses very simple technique to safely
handle removals during iteration.

If you go this route, you can avoid losing the whole client during callbacks by
simply taking a "scoped" reference in AlertListeners.

On 2013/05/24 22:34:20, Noam Samuel wrote:
> On second thought, deferring shutting down listening (i.e. having it not occur
> synchronously) is probably a better solution. That way we also don't wipe the
> cache if someone has exactly one listener and then destroys it and immediately
> creates another listener.
> 
> On 2013/05/24 19:00:49, Noam Samuel wrote:
> > Listener delegates may create or delete any listeners. Thus, the iterator
may
> > become invalid while iterating over the listeners. 
> > 
> > On a second look, this can be solved by storing weak pointers to the
listeners
> > and copying them to a separate collection before iterating over them.
> > 
> > Another problem is that a listener delegate is allowed to delete all
> listeners,
> > which would stop the client from listening for MDns packets, and would in
turn
> > delete the cache (since it can no longer be kept up to date), which owns the
> > record, and thus delete the record itself. Therefore, IMO, it makes sense to
> at
> > least copy the record, even if we call the delegates synchronously.
> > 
> > Alternately, it is be possible to make the starting/stopping of listening
for
> > mDNS more explicit by requiring users of the API to call AddListenRef and
> > RemoveListenRef explicitly when they start/stop using mDNS.
> > 
> > On 2013/05/24 15:32:22, szym wrote:
> > > Why not just call: l->second->AlertDelegate(update_type, record)?
> > 
>

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h
File net/dns/mdns_listener_impl.h (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h#...
net/dns/mdns_listener_impl.h:28: class Core : public base::SupportsWeakPtr<Core>
{
Still needs a comment in the code.

On 2013/05/24 19:00:49, Noam Samuel wrote:
> On 2013/05/24 15:32:22, szym wrote:
> > Needs comment regarding lifetime. When is Core created? Who can hold weak
> > pointers to it?
> 
> The weak pointer-ability of these classes is mostly used to allow scheduling
> methods on them through the message loop.

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc File net/dns/mdns_listener_impl.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc#newcode125 net/dns/mdns_listener_impl.cc:125: base::Bind(&MDnsListenerFactoryImpl::Core::OnDatagramRecieved, If an attacker were to spam the mdns ...

7 years, 6 months ago (2013-05-29 17:53:03 UTC) #7

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:125:
base::Bind(&MDnsListenerFactoryImpl::Core::OnDatagramRecieved,
If an attacker were to spam the mdns port with packets while you're listening,
they could cause a stack overflow if this call is synchronized. If this call is
not synchronized it will gladly accept infinitely many packets.

On 2013/05/25 02:51:01, szym wrote:
> Why not call synchronously?

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) );
Taking a scoped reference in the class entrypoints that may call AlertListeners
does work, but since it's more complicated to reason about (What if someone
creates another class entrypoint that may call AlertListeners and forgets to add
a scoped listen reference? The problem may not be immediately apparent), I
prefer to go the route of using the message loop to defer deletion.

On 2013/05/25 02:51:01, szym wrote:
> You could consider using std::map<ListenerKey, ObserverList<MDnsListenerImpl>
>
> instead of std::multimap. ObserverList uses very simple technique to safely
> handle removals during iteration.
> 
> If you go this route, you can avoid losing the whole client during callbacks
by
> simply taking a "scoped" reference in AlertListeners.
> 
> 
> On 2013/05/24 22:34:20, Noam Samuel wrote:
> > On second thought, deferring shutting down listening (i.e. having it not
occur
> > synchronously) is probably a better solution. That way we also don't wipe
the
> > cache if someone has exactly one listener and then destroys it and
immediately
> > creates another listener.
> > 
> > On 2013/05/24 19:00:49, Noam Samuel wrote:
> > > Listener delegates may create or delete any listeners. Thus, the iterator
> may
> > > become invalid while iterating over the listeners. 
> > > 
> > > On a second look, this can be solved by storing weak pointers to the
> listeners
> > > and copying them to a separate collection before iterating over them.
> > > 
> > > Another problem is that a listener delegate is allowed to delete all
> > listeners,
> > > which would stop the client from listening for MDns packets, and would in
> turn
> > > delete the cache (since it can no longer be kept up to date), which owns
the
> > > record, and thus delete the record itself. Therefore, IMO, it makes sense
to
> > at
> > > least copy the record, even if we call the delegates synchronously.
> > > 
> > > Alternately, it is be possible to make the starting/stopping of listening
> for
> > > mDNS more explicit by requiring users of the API to call AddListenRef and
> > > RemoveListenRef explicitly when they start/stop using mDNS.
> > > 
> > > On 2013/05/24 15:32:22, szym wrote:
> > > > Why not just call: l->second->AlertDelegate(update_type, record)?
> > > 
> > 
>

szym

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc File net/dns/mdns_listener_impl.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc#newcode125 net/dns/mdns_listener_impl.cc:125: base::Bind(&MDnsListenerFactoryImpl::Core::OnDatagramRecieved, On 2013/05/29 17:53:03, Noam Samuel wrote: > If ...

7 years, 6 months ago (2013-05-29 17:59:40 UTC) #8

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:125:
base::Bind(&MDnsListenerFactoryImpl::Core::OnDatagramRecieved,
On 2013/05/29 17:53:03, Noam Samuel wrote:
> If an attacker were to spam the mdns port with packets while you're listening,
> they could cause a stack overflow if this call is synchronized. If this call
is
> not synchronized it will gladly accept infinitely many packets.
> 
> On 2013/05/25 02:51:01, szym wrote:
> > Why not call synchronously?

The solution to this is to replace recursion with iteration. This is the common
"DoLoop" pattern in net/. OnDatagramReceived should return to the loop rather
than call through to ReceiveOnePacket.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) );
Ok, but now you have to deal with a situation where a listener says "remove me",
so you should not call it again. But in another callback the listener says "add
me".

One solution is to keep per-listener state ("is active") and only clean the
listeners up after all callbacks finished. You now need to go through all
listeners twice, but it shouldn't be very expensive (and no more expensive than
RemoveListener).

On 2013/05/29 17:53:03, Noam Samuel wrote:
> Taking a scoped reference in the class entrypoints that may call
AlertListeners
> does work, but since it's more complicated to reason about (What if someone
> creates another class entrypoint that may call AlertListeners and forgets to
add
> a scoped listen reference? The problem may not be immediately apparent), I
> prefer to go the route of using the message loop to defer deletion.
> 
> On 2013/05/25 02:51:01, szym wrote:
> > You could consider using std::map<ListenerKey,
ObserverList<MDnsListenerImpl>
> >
> > instead of std::multimap. ObserverList uses very simple technique to safely
> > handle removals during iteration.
> > 
> > If you go this route, you can avoid losing the whole client during callbacks
> by
> > simply taking a "scoped" reference in AlertListeners.
> > 
> > 
> > On 2013/05/24 22:34:20, Noam Samuel wrote:
> > > On second thought, deferring shutting down listening (i.e. having it not
> occur
> > > synchronously) is probably a better solution. That way we also don't wipe
> the
> > > cache if someone has exactly one listener and then destroys it and
> immediately
> > > creates another listener.
> > > 
> > > On 2013/05/24 19:00:49, Noam Samuel wrote:
> > > > Listener delegates may create or delete any listeners. Thus, the
iterator
> > may
> > > > become invalid while iterating over the listeners. 
> > > > 
> > > > On a second look, this can be solved by storing weak pointers to the
> > listeners
> > > > and copying them to a separate collection before iterating over them.
> > > > 
> > > > Another problem is that a listener delegate is allowed to delete all
> > > listeners,
> > > > which would stop the client from listening for MDns packets, and would
in
> > turn
> > > > delete the cache (since it can no longer be kept up to date), which owns
> the
> > > > record, and thus delete the record itself. Therefore, IMO, it makes
sense
> to
> > > at
> > > > least copy the record, even if we call the delegates synchronously.
> > > > 
> > > > Alternately, it is be possible to make the starting/stopping of
listening
> > for
> > > > mDNS more explicit by requiring users of the API to call AddListenRef
and
> > > > RemoveListenRef explicitly when they start/stop using mDNS.
> > > > 
> > > > On 2013/05/24 15:32:22, szym wrote:
> > > > > Why not just call: l->second->AlertDelegate(update_type, record)?
> > > > 
> > > 
> >

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc File net/dns/mdns_listener_impl.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc#newcode233 net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) ); Each individual listener is added/removed in an ...

7 years, 6 months ago (2013-05-29 18:35:34 UTC) #9

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) );
Each individual listener is added/removed in an RAII fashion, so it should be
impossible for a listener to add itself after it removes itself. 

When using deferred deletion, if another listener is added after the last listen
ref is removed and before the core is actually deleted, a listen ref will be
added and when the scheduled core deletion happens, the client will see that
there is a non-zero number of listen refs and will not delete the core. 

On 2013/05/29 17:59:41, szym wrote:
> Ok, but now you have to deal with a situation where a listener says "remove
me",
> so you should not call it again. But in another callback the listener says
"add
> me".
> 
> One solution is to keep per-listener state ("is active") and only clean the
> listeners up after all callbacks finished. You now need to go through all
> listeners twice, but it shouldn't be very expensive (and no more expensive
than
> RemoveListener).
> 
> On 2013/05/29 17:53:03, Noam Samuel wrote:
> > Taking a scoped reference in the class entrypoints that may call
> AlertListeners
> > does work, but since it's more complicated to reason about (What if someone
> > creates another class entrypoint that may call AlertListeners and forgets to
> add
> > a scoped listen reference? The problem may not be immediately apparent), I
> > prefer to go the route of using the message loop to defer deletion.
> > 
> > On 2013/05/25 02:51:01, szym wrote:
> > > You could consider using std::map<ListenerKey,
> ObserverList<MDnsListenerImpl>
> > >
> > > instead of std::multimap. ObserverList uses very simple technique to
safely
> > > handle removals during iteration.
> > > 
> > > If you go this route, you can avoid losing the whole client during
callbacks
> > by
> > > simply taking a "scoped" reference in AlertListeners.
> > > 
> > > 
> > > On 2013/05/24 22:34:20, Noam Samuel wrote:
> > > > On second thought, deferring shutting down listening (i.e. having it not
> > occur
> > > > synchronously) is probably a better solution. That way we also don't
wipe
> > the
> > > > cache if someone has exactly one listener and then destroys it and
> > immediately
> > > > creates another listener.
> > > > 
> > > > On 2013/05/24 19:00:49, Noam Samuel wrote:
> > > > > Listener delegates may create or delete any listeners. Thus, the
> iterator
> > > may
> > > > > become invalid while iterating over the listeners. 
> > > > > 
> > > > > On a second look, this can be solved by storing weak pointers to the
> > > listeners
> > > > > and copying them to a separate collection before iterating over them.
> > > > > 
> > > > > Another problem is that a listener delegate is allowed to delete all
> > > > listeners,
> > > > > which would stop the client from listening for MDns packets, and would
> in
> > > turn
> > > > > delete the cache (since it can no longer be kept up to date), which
owns
> > the
> > > > > record, and thus delete the record itself. Therefore, IMO, it makes
> sense
> > to
> > > > at
> > > > > least copy the record, even if we call the delegates synchronously.
> > > > > 
> > > > > Alternately, it is be possible to make the starting/stopping of
> listening
> > > for
> > > > > mDNS more explicit by requiring users of the API to call AddListenRef
> and
> > > > > RemoveListenRef explicitly when they start/stop using mDNS.
> > > > > 
> > > > > On 2013/05/24 15:32:22, szym wrote:
> > > > > > Why not just call: l->second->AlertDelegate(update_type, record)?
> > > > > 
> > > > 
> > > 
>

szym

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc File net/dns/mdns_listener_impl.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc#newcode233 net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) ); With deferred deletion, how do you avoid ...

7 years, 6 months ago (2013-05-29 18:40:43 UTC) #10

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) );
With deferred deletion, how do you avoid calling back a listener removed during
callback.

On 2013/05/29 18:35:35, Noam Samuel wrote:
> Each individual listener is added/removed in an RAII fashion, so it should be
> impossible for a listener to add itself after it removes itself. 
> 
> When using deferred deletion, if another listener is added after the last
listen
> ref is removed and before the core is actually deleted, a listen ref will be
> added and when the scheduled core deletion happens, the client will see that
> there is a non-zero number of listen refs and will not delete the core. 
> 
> On 2013/05/29 17:59:41, szym wrote:
> > Ok, but now you have to deal with a situation where a listener says "remove
> me",
> > so you should not call it again. But in another callback the listener says
> "add
> > me".
> > 
> > One solution is to keep per-listener state ("is active") and only clean the
> > listeners up after all callbacks finished. You now need to go through all
> > listeners twice, but it shouldn't be very expensive (and no more expensive
> than
> > RemoveListener).
> > 
> > On 2013/05/29 17:53:03, Noam Samuel wrote:
> > > Taking a scoped reference in the class entrypoints that may call
> > AlertListeners
> > > does work, but since it's more complicated to reason about (What if
someone
> > > creates another class entrypoint that may call AlertListeners and forgets
to
> > add
> > > a scoped listen reference? The problem may not be immediately apparent), I
> > > prefer to go the route of using the message loop to defer deletion.
> > > 
> > > On 2013/05/25 02:51:01, szym wrote:
> > > > You could consider using std::map<ListenerKey,
> > ObserverList<MDnsListenerImpl>
> > > >
> > > > instead of std::multimap. ObserverList uses very simple technique to
> safely
> > > > handle removals during iteration.
> > > > 
> > > > If you go this route, you can avoid losing the whole client during
> callbacks
> > > by
> > > > simply taking a "scoped" reference in AlertListeners.
> > > > 
> > > > 
> > > > On 2013/05/24 22:34:20, Noam Samuel wrote:
> > > > > On second thought, deferring shutting down listening (i.e. having it
not
> > > occur
> > > > > synchronously) is probably a better solution. That way we also don't
> wipe
> > > the
> > > > > cache if someone has exactly one listener and then destroys it and
> > > immediately
> > > > > creates another listener.
> > > > > 
> > > > > On 2013/05/24 19:00:49, Noam Samuel wrote:
> > > > > > Listener delegates may create or delete any listeners. Thus, the
> > iterator
> > > > may
> > > > > > become invalid while iterating over the listeners. 
> > > > > > 
> > > > > > On a second look, this can be solved by storing weak pointers to the
> > > > listeners
> > > > > > and copying them to a separate collection before iterating over
them.
> > > > > > 
> > > > > > Another problem is that a listener delegate is allowed to delete all
> > > > > listeners,
> > > > > > which would stop the client from listening for MDns packets, and
would
> > in
> > > > turn
> > > > > > delete the cache (since it can no longer be kept up to date), which
> owns
> > > the
> > > > > > record, and thus delete the record itself. Therefore, IMO, it makes
> > sense
> > > to
> > > > > at
> > > > > > least copy the record, even if we call the delegates synchronously.
> > > > > > 
> > > > > > Alternately, it is be possible to make the starting/stopping of
> > listening
> > > > for
> > > > > > mDNS more explicit by requiring users of the API to call
AddListenRef
> > and
> > > > > > RemoveListenRef explicitly when they start/stop using mDNS.
> > > > > > 
> > > > > > On 2013/05/24 15:32:22, szym wrote:
> > > > > > > Why not just call: l->second->AlertDelegate(update_type, record)?
> > > > > > 
> > > > > 
> > > > 
> > 
>

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc File net/dns/mdns_listener_impl.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc#newcode233 net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) ); Oh, sorry. The deferred deletion applies only ...

7 years, 6 months ago (2013-05-29 20:14:49 UTC) #11

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:233: base::Owned(record_clone.release())) );
Oh, sorry. The deferred deletion applies only to the core as a whole, so I still
remove listeners immediately, but then if the last listener is removed, forcing
a deletion of the core (the object that contains the connection and cache), I
defer the deletion of the core using the message loop. The code looks like this:


void MDnsClientImpl::Core::RemoveListener(MDnsListenerImpl* listener) {
  <Irrelevant>

  client_->SubtractListenRef();
}


void MDnsClientImpl::SubtractListenRef() {
  listen_refs_--;
  if (listen_refs_ == 0) {
    task_runner_->PostTask(FROM_HERE, base::Bind(
        &MDnsClientImpl::Shutdown, base::Unretained(this)));
  }
}

void MDnsClientImpl::Shutdown() {
  // We need to check that new listeners haven't been created.
  if (listen_refs_ == 0) {
    core_.reset();
  }
}


On 2013/05/29 18:40:43, szym wrote:
> With deferred deletion, how do you avoid calling back a listener removed
during
> callback.
> 
> On 2013/05/29 18:35:35, Noam Samuel wrote:
> > Each individual listener is added/removed in an RAII fashion, so it should
be
> > impossible for a listener to add itself after it removes itself. 
> > 
> > When using deferred deletion, if another listener is added after the last
> listen
> > ref is removed and before the core is actually deleted, a listen ref will be
> > added and when the scheduled core deletion happens, the client will see that
> > there is a non-zero number of listen refs and will not delete the core. 
> > 
> > On 2013/05/29 17:59:41, szym wrote:
> > > Ok, but now you have to deal with a situation where a listener says
"remove
> > me",
> > > so you should not call it again. But in another callback the listener says
> > "add
> > > me".
> > > 
> > > One solution is to keep per-listener state ("is active") and only clean
the
> > > listeners up after all callbacks finished. You now need to go through all
> > > listeners twice, but it shouldn't be very expensive (and no more expensive
> > than
> > > RemoveListener).
> > > 
> > > On 2013/05/29 17:53:03, Noam Samuel wrote:
> > > > Taking a scoped reference in the class entrypoints that may call
> > > AlertListeners
> > > > does work, but since it's more complicated to reason about (What if
> someone
> > > > creates another class entrypoint that may call AlertListeners and
forgets
> to
> > > add
> > > > a scoped listen reference? The problem may not be immediately apparent),
I
> > > > prefer to go the route of using the message loop to defer deletion.
> > > > 
> > > > On 2013/05/25 02:51:01, szym wrote:
> > > > > You could consider using std::map<ListenerKey,
> > > ObserverList<MDnsListenerImpl>
> > > > >
> > > > > instead of std::multimap. ObserverList uses very simple technique to
> > safely
> > > > > handle removals during iteration.
> > > > > 
> > > > > If you go this route, you can avoid losing the whole client during
> > callbacks
> > > > by
> > > > > simply taking a "scoped" reference in AlertListeners.
> > > > > 
> > > > > 
> > > > > On 2013/05/24 22:34:20, Noam Samuel wrote:
> > > > > > On second thought, deferring shutting down listening (i.e. having it
> not
> > > > occur
> > > > > > synchronously) is probably a better solution. That way we also don't
> > wipe
> > > > the
> > > > > > cache if someone has exactly one listener and then destroys it and
> > > > immediately
> > > > > > creates another listener.
> > > > > > 
> > > > > > On 2013/05/24 19:00:49, Noam Samuel wrote:
> > > > > > > Listener delegates may create or delete any listeners. Thus, the
> > > iterator
> > > > > may
> > > > > > > become invalid while iterating over the listeners. 
> > > > > > > 
> > > > > > > On a second look, this can be solved by storing weak pointers to
the
> > > > > listeners
> > > > > > > and copying them to a separate collection before iterating over
> them.
> > > > > > > 
> > > > > > > Another problem is that a listener delegate is allowed to delete
all
> > > > > > listeners,
> > > > > > > which would stop the client from listening for MDns packets, and
> would
> > > in
> > > > > turn
> > > > > > > delete the cache (since it can no longer be kept up to date),
which
> > owns
> > > > the
> > > > > > > record, and thus delete the record itself. Therefore, IMO, it
makes
> > > sense
> > > > to
> > > > > > at
> > > > > > > least copy the record, even if we call the delegates
synchronously.
> > > > > > > 
> > > > > > > Alternately, it is be possible to make the starting/stopping of
> > > listening
> > > > > for
> > > > > > > mDNS more explicit by requiring users of the API to call
> AddListenRef
> > > and
> > > > > > > RemoveListenRef explicitly when they start/stop using mDNS.
> > > > > > > 
> > > > > > > On 2013/05/24 15:32:22, szym wrote:
> > > > > > > > Why not just call: l->second->AlertDelegate(update_type,
record)?
> > > > > > > 
> > > > > > 
> > > > > 
> > > 
> > 
>

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h File net/dns/mdns_listener.h (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newcode18 net/dns/mdns_listener.h:18: extern const char kMDNSMulticastGroupIPv4[]; On 2013/05/24 21:59:18, Noam Samuel ...

7 years, 6 months ago (2013-05-29 21:25:16 UTC) #12

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h
File net/dns/mdns_listener.h (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:18: extern const char kMDNSMulticastGroupIPv4[];
On 2013/05/24 21:59:18, Noam Samuel wrote:
> I put it here so that classes other than the implementation can access it.
> However, this seems excessive. I will move it to the implmentation .cc.
> On 2013/05/24 15:32:22, szym wrote:
> > Why do you need this?
> 

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:23: class MDnsTransaction;
On 2013/05/24 15:32:22, szym wrote:
> I suggest you define MDnsTransaction first so that you don't have to forward
> declare it.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:39: class MDnsListenerFactory {
On 2013/05/24 15:32:22, szym wrote:
> Needs a comment. I find the name confusing. It can create Listeners and
> Transactions. I'd call it MDnsClient.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:49: // record.
On 2013/05/24 19:00:49, Noam Samuel wrote:
> Maybe the TODO should be moved somewhere else. The TODO refers to actually
> implementing NSEC record parsing.
> 
> On 2013/05/24 15:32:22, szym wrote:
> > Not sure why there's TODO here. What part is missing?
> 

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:50: virtual void OnNSecRecord(const std::string& name,
unsigned type) = 0;
On 2013/05/24 15:32:22, szym wrote:
> Spell "NSec" vs. "Nsec" consistently here and in MDnsTransactionResult.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:58: // Add delegate for RRType |rrtype| and name |name|.
On 2013/05/24 15:32:22, szym wrote:
> Comment says "Add delegate" but it creates a Listener. Probably best to have a
> class-level comment explaining what each class represents.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:64: bool active,
On 2013/05/24 15:32:22, szym wrote:
> Needs comment.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:65: bool alert_existing_records,
On 2013/05/24 15:32:22, szym wrote:
> Needs comment.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:85: // Destroing the transaction cancels pending
transactions.
On 2013/05/24 15:32:22, szym wrote:
> "Destroying"
> "it" would be better than "pending transactions"

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener.h#newco...
net/dns/mdns_listener.h:107: // Whether this listener is active (true) or
passive (false)
On 2013/05/24 19:00:49, Noam Samuel wrote:
> On 2013/05/24 15:32:22, szym wrote:
> > What does that mean?
> 
> An active listener is allowed to send periodic queries to see if new records
of
> a given type are available.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:49: if (!RecieveOnePacket(socket_ipv4_.get(),
Renamed method and moved it to MDnsConnectionImpl.

On 2013/05/24 19:00:49, Noam Samuel wrote:
> On 2013/05/24 15:32:22, szym wrote:
> > What is the point of doing this?
> 
> Oh. I guess this method is somewhat misnamed. RecieveOnePacket is used to
> request a packet. It sets a handler for receiving the packet that processes it
> and then requests the next packet. This is the "main loop" for the mDNS
> listener. Not 100% sure what to name it; maybe "RecieveNextPacket"?

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:120: AsWeakPtr(), socket, response, recv_addr));
On 2013/05/25 02:51:01, szym wrote:
> You don't really need WeakPtr here since if Core is gone, so is |socket|.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.cc...
net/dns/mdns_listener_impl.cc:125:
base::Bind(&MDnsListenerFactoryImpl::Core::OnDatagramRecieved,
On 2013/05/29 17:59:41, szym wrote:
> On 2013/05/29 17:53:03, Noam Samuel wrote:
> > If an attacker were to spam the mdns port with packets while you're
listening,
> > they could cause a stack overflow if this call is synchronized. If this call
> is
> > not synchronized it will gladly accept infinitely many packets.
> > 
> > On 2013/05/25 02:51:01, szym wrote:
> > > Why not call synchronously?
> 
> The solution to this is to replace recursion with iteration. This is the
common
> "DoLoop" pattern in net/. OnDatagramReceived should return to the loop rather
> than call through to ReceiveOnePacket.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h
File net/dns/mdns_listener_impl.h (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h#...
net/dns/mdns_listener_impl.h:28: class Core : public base::SupportsWeakPtr<Core>
{
On 2013/05/25 02:51:01, szym wrote:
> Still needs a comment in the code.
> 
> On 2013/05/24 19:00:49, Noam Samuel wrote:
> > On 2013/05/24 15:32:22, szym wrote:
> > > Needs comment regarding lifetime. When is Core created? Who can hold weak
> > > pointers to it?
> > 
> > The weak pointer-ability of these classes is mostly used to allow scheduling
> > methods on them through the message loop.
> 

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h#...
net/dns/mdns_listener_impl.h:62: void OnDatagramRecieved(UDPSocket* socket,
On 2013/05/24 15:32:22, szym wrote:
> Throughout the file: s/Recieve/Receive/

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h#...
net/dns/mdns_listener_impl.h:107: base::Time scheduled_cleanup_;
On 2013/05/24 15:32:22, szym wrote:
> DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_impl.h#...
net/dns/mdns_listener_impl.h:125: MDnsListenerFactory::Delegate* delegate)
OVERRIDE;
Moved all the delegates and callbacks to their respective objects.

On 2013/05/24 15:32:22, szym wrote:
> You don't need MDnsListenerFactory:: here and elsewhere in this file.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
File net/dns/mdns_listener_unittest.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())),
On 2013/05/24 21:59:18, Noam Samuel wrote:
> Hm. I tend to agree that, in retrospect, using mock sockets rather than
loopback
> network traffic may be simpler. These sockets are actually used to create mock
> network traffic for the MDnsClient. The sockets inside MDnsClient are used as
> servers. If I added JoinGroup and LeaveGroup to UDPServerSocket, I could use
it
> in the MDnsClient and then create a mock version of UDPServerSocket.
> 
> On 2013/05/24 15:32:22, szym wrote:
> > To minimize flakiness, we'd test this with MockUDPClientSocket. Do you
really
> > need to test on UDPSocket?
> 

Done.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
net/dns/mdns_listener_unittest.cc:394: client_socket_->Close();
On 2013/05/24 15:32:22, szym wrote:
> Not necessary. ~MDnsTest (called immediately after TearDown) will destroy and
> close all sockets.

Done.

https://codereview.chromium.org/15733008/diff/1/net/test/fake_time_system.h
File net/test/fake_time_system.h (right):

https://codereview.chromium.org/15733008/diff/1/net/test/fake_time_system.h#n...
net/test/fake_time_system.h:16: class FakeTimeSystem : public base::Clock,
public base::TaskRunner {
On 2013/05/24 15:32:22, szym wrote:
> Needs class-level comment. Not sure you need this.

Done.

szym

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittest.cc File net/dns/mdns_listener_unittest.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittest.cc#newcode361 net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())), On 2013/05/29 21:25:16, Noam Samuel wrote: > On ...

7 years, 6 months ago (2013-06-02 19:01:23 UTC) #13

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
File net/dns/mdns_listener_unittest.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())),
On 2013/05/29 21:25:16, Noam Samuel wrote:
> On 2013/05/24 21:59:18, Noam Samuel wrote:
> > Hm. I tend to agree that, in retrospect, using mock sockets rather than
> loopback
> > network traffic may be simpler. These sockets are actually used to create
mock
> > network traffic for the MDnsClient. The sockets inside MDnsClient are used
as
> > servers. If I added JoinGroup and LeaveGroup to UDPServerSocket, I could use
> it
> > in the MDnsClient and then create a mock version of UDPServerSocket.
> > 
> > On 2013/05/24 15:32:22, szym wrote:
> > > To minimize flakiness, we'd test this with MockUDPClientSocket. Do you
> really
> > > need to test on UDPSocket?
> > 
> 
> Done.

I still see UDPSocket rather than DatagramServerSocket.

https://codereview.chromium.org/15733008/diff/17001/net/dns/mdns_listener_imp...
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/17001/net/dns/mdns_listener_imp...
net/dns/mdns_listener_impl.cc:579: delegate_->HandlePacket(response, rval);
This needs to be adjusted to follow the common design pattern in net. Please
take a look at other DoLoop methods in src/net.

In your case you have two sockets that you want to process "in parallel", so I
suggest creating two instances of a class implementing DoLoop, each wrapping one
socket and response buffer.

Because the processing is very simple (recv and handle), you don't really need
States. It could really be as simple as:

Receiver::DoLoop(int rv) {
  do {
    if (rv > 0)
      delegate_->HandlePacket(rv, buffer_);
    rv = socket_->Recv(buffer, Bind(Receiver::OnDatagramReceived,
Unretained(this)));
  } while (rv > 0);
  if (rv != ERR_IO_PENDING)
    delegate_->HandleError(rv);
}

Receiver::OnDatagramReceived(int rv) {
  DoLoop(rv);
}

Connection::Start() {
  ipv4_receiver_->DoLoop(0);
  ipv6_receiver_->DoLoop(0);
}

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:30: : client_(client), task_runner_(task_runner),
clock_(clock),
The only reason I see for this dependency on TaskRunner and Clock is that you
want to be able to inject mocks to test timeout and cache clean-up.

In this trade: dependency plumbing vs. waiting a bit longer in tests, I don't
think the price is right. Timeout is 3 seconds and clean-up depends on TTL, so
no test would wait longer than 3 seconds if you used the current
MessageLoopProxy and base::Time::Now().

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:372: scoped_ptr<const RecordParsed> record_clone =
records.front()->Clone();
Why do you need to Clone?

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:373: task_runner_->PostTask(
Why not call CacheRecordFound directly?

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:410: triggered_ = true;
Instead of adding |triggered_|, I suggest:
if (callback_.is_null()) return;

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:30: class MDnsConnection {
Since this is internal to mdns_client_impl, I don't think you need this
interface. Instead, I'd suggest making this class internal to MDnsClientImpl so
that it can call its private methods directly.

Noam Samuel

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittest.cc File net/dns/mdns_listener_unittest.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittest.cc#newcode361 net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())), DatagramServerSocket doesn't support multicasting (JoinGroup and AllowAddressReuse). Should ...

7 years, 6 months ago (2013-06-03 17:57:55 UTC) #14

szym

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittest.cc File net/dns/mdns_listener_unittest.cc (right): https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittest.cc#newcode361 net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())), On 2013/06/03 17:57:55, Noam Samuel wrote: > DatagramServerSocket ...

7 years, 6 months ago (2013-06-03 18:38:29 UTC) #15

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
File net/dns/mdns_listener_unittest.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())),
On 2013/06/03 17:57:55, Noam Samuel wrote:
> DatagramServerSocket doesn't support multicasting (JoinGroup and
> AllowAddressReuse). Should I add support for it?
> 
> On 2013/06/02 19:01:23, szym wrote:
> > On 2013/05/29 21:25:16, Noam Samuel wrote:
> > > On 2013/05/24 21:59:18, Noam Samuel wrote:
> > > > Hm. I tend to agree that, in retrospect, using mock sockets rather than
> > > loopback
> > > > network traffic may be simpler. These sockets are actually used to
create
> > mock
> > > > network traffic for the MDnsClient. The sockets inside MDnsClient are
used
> > as
> > > > servers. If I added JoinGroup and LeaveGroup to UDPServerSocket, I could
> use
> > > it
> > > > in the MDnsClient and then create a mock version of UDPServerSocket.
> > > > 
> > > > On 2013/05/24 15:32:22, szym wrote:
> > > > > To minimize flakiness, we'd test this with MockUDPClientSocket. Do you
> > > really
> > > > > need to test on UDPSocket?
> > > > 
> > > 
> > > Done.
> > 
> > I still see UDPSocket rather than DatagramServerSocket.
> 

I think so.

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:30: class MDnsConnection {
On 2013/06/03 17:57:55, Noam Samuel wrote:
> On 2013/06/02 19:01:23, szym wrote:
> > Since this is internal to mdns_client_impl, I don't think you need this
> > interface. Instead, I'd suggest making this class internal to MDnsClientImpl
> so
> > that it can call its private methods directly.
> 
> This class is mocked in the tests to allow separating the testing of the
socket
> read loop and networking aspects from the testing of other parts of the mdns
> logic.

Whelp! I didn't notice it. This CL is a bit large.

Noam Samuel

Additional change: After running into some issues with synchronous callbacks being called on initialization, moved ...

7 years, 6 months ago (2013-06-04 00:08:02 UTC) #16

Additional change: After running into some issues with synchronous callbacks
being called on initialization, moved actual initialization logic to Start()
methods on both the listener and the transaction.

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
File net/dns/mdns_listener_unittest.cc (right):

https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())),
On 2013/06/03 18:38:30, szym wrote:
> On 2013/06/03 17:57:55, Noam Samuel wrote:
> > DatagramServerSocket doesn't support multicasting (JoinGroup and
> > AllowAddressReuse). Should I add support for it?
> > 
> > On 2013/06/02 19:01:23, szym wrote:
> > > On 2013/05/29 21:25:16, Noam Samuel wrote:
> > > > On 2013/05/24 21:59:18, Noam Samuel wrote:
> > > > > Hm. I tend to agree that, in retrospect, using mock sockets rather
than
> > > > loopback
> > > > > network traffic may be simpler. These sockets are actually used to
> create
> > > mock
> > > > > network traffic for the MDnsClient. The sockets inside MDnsClient are
> used
> > > as
> > > > > servers. If I added JoinGroup and LeaveGroup to UDPServerSocket, I
could
> > use
> > > > it
> > > > > in the MDnsClient and then create a mock version of UDPServerSocket.
> > > > > 
> > > > > On 2013/05/24 15:32:22, szym wrote:
> > > > > > To minimize flakiness, we'd test this with MockUDPClientSocket. Do
you
> > > > really
> > > > > > need to test on UDPSocket?
> > > > > 
> > > > 
> > > > Done.
> > > 
> > > I still see UDPSocket rather than DatagramServerSocket.
> > 
> 
> I think so.

Added support in interface: https://codereview.chromium.org/15995023/

https://codereview.chromium.org/15733008/diff/17001/net/dns/mdns_listener_imp...
File net/dns/mdns_listener_impl.cc (right):

https://codereview.chromium.org/15733008/diff/17001/net/dns/mdns_listener_imp...
net/dns/mdns_listener_impl.cc:579: delegate_->HandlePacket(response, rval);
On 2013/06/02 19:01:23, szym wrote:
> This needs to be adjusted to follow the common design pattern in net. Please
> take a look at other DoLoop methods in src/net.
> 
> In your case you have two sockets that you want to process "in parallel", so I
> suggest creating two instances of a class implementing DoLoop, each wrapping
one
> socket and response buffer.
> 
> Because the processing is very simple (recv and handle), you don't really need
> States. It could really be as simple as:
> 
> Receiver::DoLoop(int rv) {
>   do {
>     if (rv > 0)
>       delegate_->HandlePacket(rv, buffer_);
>     rv = socket_->Recv(buffer, Bind(Receiver::OnDatagramReceived,
> Unretained(this)));
>   } while (rv > 0);
>   if (rv != ERR_IO_PENDING)
>     delegate_->HandleError(rv);
> }
> 
> Receiver::OnDatagramReceived(int rv) {
>   DoLoop(rv);
> }
> 
> Connection::Start() {
>   ipv4_receiver_->DoLoop(0);
>   ipv6_receiver_->DoLoop(0);
> }

Done.

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:30: : client_(client), task_runner_(task_runner),
clock_(clock),
On 2013/06/02 19:01:23, szym wrote:
> The only reason I see for this dependency on TaskRunner and Clock is that you
> want to be able to inject mocks to test timeout and cache clean-up.
> 
> In this trade: dependency plumbing vs. waiting a bit longer in tests, I don't
> think the price is right. Timeout is 3 seconds and clean-up depends on TTL, so
> no test would wait longer than 3 seconds if you used the current
> MessageLoopProxy and base::Time::Now().
> 

Done.

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:372: scoped_ptr<const RecordParsed> record_clone =
records.front()->Clone();
On 2013/06/02 19:01:23, szym wrote:
> Why do you need to Clone?

Removed.

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:373: task_runner_->PostTask(
On 2013/06/02 19:01:23, szym wrote:
> Why not call CacheRecordFound directly?

Synchronized.

https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:410: triggered_ = true;
On 2013/06/02 19:01:23, szym wrote:
> Instead of adding |triggered_|, I suggest:
> if (callback_.is_null()) return;

Done.

Noam Samuel

Hey, In order to make the API surface easier to use, I simplified it a ...

7 years, 6 months ago (2013-06-07 01:40:45 UTC) #17

Hey,

In order to make the API surface easier to use, I simplified it a bit by
removing the concept of "active" listeners and of alert existing records in
listeners. Now listeners are simple change notifiers, and most of the logic for
actually querying for all services has been moved to transactions. Now
transaction creation takes an extra "flags" parameter, which lets you specify
whether you want to query the network, the cache, or both, and whether the
transaction should return one result (call the callback once) or multiple (call
the callback multiple times).

--Noam

On 2013/06/04 00:08:02, Noam Samuel wrote:
> Additional change: After running into some issues with synchronous callbacks
> being called on initialization, moved actual initialization logic to Start()
> methods on both the listener and the transaction.
> 
>
https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
> File net/dns/mdns_listener_unittest.cc (right):
> 
>
https://codereview.chromium.org/15733008/diff/1/net/dns/mdns_listener_unittes...
> net/dns/mdns_listener_unittest.cc:361: net::NetLog::Source())),
> On 2013/06/03 18:38:30, szym wrote:
> > On 2013/06/03 17:57:55, Noam Samuel wrote:
> > > DatagramServerSocket doesn't support multicasting (JoinGroup and
> > > AllowAddressReuse). Should I add support for it?
> > > 
> > > On 2013/06/02 19:01:23, szym wrote:
> > > > On 2013/05/29 21:25:16, Noam Samuel wrote:
> > > > > On 2013/05/24 21:59:18, Noam Samuel wrote:
> > > > > > Hm. I tend to agree that, in retrospect, using mock sockets rather
> than
> > > > > loopback
> > > > > > network traffic may be simpler. These sockets are actually used to
> > create
> > > > mock
> > > > > > network traffic for the MDnsClient. The sockets inside MDnsClient
are
> > used
> > > > as
> > > > > > servers. If I added JoinGroup and LeaveGroup to UDPServerSocket, I
> could
> > > use
> > > > > it
> > > > > > in the MDnsClient and then create a mock version of UDPServerSocket.
> > > > > > 
> > > > > > On 2013/05/24 15:32:22, szym wrote:
> > > > > > > To minimize flakiness, we'd test this with MockUDPClientSocket. Do
> you
> > > > > really
> > > > > > > need to test on UDPSocket?
> > > > > > 
> > > > > 
> > > > > Done.
> > > > 
> > > > I still see UDPSocket rather than DatagramServerSocket.
> > > 
> > 
> > I think so.
> 
> Added support in interface: https://codereview.chromium.org/15995023/
> 
>
https://codereview.chromium.org/15733008/diff/17001/net/dns/mdns_listener_imp...
> File net/dns/mdns_listener_impl.cc (right):
> 
>
https://codereview.chromium.org/15733008/diff/17001/net/dns/mdns_listener_imp...
> net/dns/mdns_listener_impl.cc:579: delegate_->HandlePacket(response, rval);
> On 2013/06/02 19:01:23, szym wrote:
> > This needs to be adjusted to follow the common design pattern in net. Please
> > take a look at other DoLoop methods in src/net.
> > 
> > In your case you have two sockets that you want to process "in parallel", so
I
> > suggest creating two instances of a class implementing DoLoop, each wrapping
> one
> > socket and response buffer.
> > 
> > Because the processing is very simple (recv and handle), you don't really
need
> > States. It could really be as simple as:
> > 
> > Receiver::DoLoop(int rv) {
> >   do {
> >     if (rv > 0)
> >       delegate_->HandlePacket(rv, buffer_);
> >     rv = socket_->Recv(buffer, Bind(Receiver::OnDatagramReceived,
> > Unretained(this)));
> >   } while (rv > 0);
> >   if (rv != ERR_IO_PENDING)
> >     delegate_->HandleError(rv);
> > }
> > 
> > Receiver::OnDatagramReceived(int rv) {
> >   DoLoop(rv);
> > }
> > 
> > Connection::Start() {
> >   ipv4_receiver_->DoLoop(0);
> >   ipv6_receiver_->DoLoop(0);
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl.cc
> File net/dns/mdns_client_impl.cc (right):
> 
>
https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
> net/dns/mdns_client_impl.cc:30: : client_(client), task_runner_(task_runner),
> clock_(clock),
> On 2013/06/02 19:01:23, szym wrote:
> > The only reason I see for this dependency on TaskRunner and Clock is that
you
> > want to be able to inject mocks to test timeout and cache clean-up.
> > 
> > In this trade: dependency plumbing vs. waiting a bit longer in tests, I
don't
> > think the price is right. Timeout is 3 seconds and clean-up depends on TTL,
so
> > no test would wait longer than 3 seconds if you used the current
> > MessageLoopProxy and base::Time::Now().
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
> net/dns/mdns_client_impl.cc:372: scoped_ptr<const RecordParsed> record_clone =
> records.front()->Clone();
> On 2013/06/02 19:01:23, szym wrote:
> > Why do you need to Clone?
> 
> Removed.
> 
>
https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
> net/dns/mdns_client_impl.cc:373: task_runner_->PostTask(
> On 2013/06/02 19:01:23, szym wrote:
> > Why not call CacheRecordFound directly?
> 
> Synchronized.
> 
>
https://codereview.chromium.org/15733008/diff/20002/net/dns/mdns_client_impl....
> net/dns/mdns_client_impl.cc:410: triggered_ = true;
> On 2013/06/02 19:01:23, szym wrote:
> > Instead of adding |triggered_|, I suggest:
> > if (callback_.is_null()) return;
> 
> Done.

szym

A lot of nits, but the two major points are: 1) Please move repeated code ...

7 years, 6 months ago (2013-06-07 16:40:02 UTC) #18

Noam Samuel

https://codereview.chromium.org/15733008/diff/64001/net/dns/mdns_client.h File net/dns/mdns_client.h (right): https://codereview.chromium.org/15733008/diff/64001/net/dns/mdns_client.h#newcode20 net/dns/mdns_client.h:20: enum MDnsUpdateType { On 2013/06/07 16:40:02, szym wrote: > ...

7 years, 6 months ago (2013-06-07 23:54:43 UTC) #19

szym

https://codereview.chromium.org/15733008/diff/64001/net/dns/mdns_query.h File net/dns/mdns_query.h (right): https://codereview.chromium.org/15733008/diff/64001/net/dns/mdns_query.h#newcode22 net/dns/mdns_query.h:22: class MDnsQuery { On 2013/06/07 23:54:43, Noam Samuel wrote: ...

7 years, 6 months ago (2013-06-10 21:58:28 UTC) #20

https://codereview.chromium.org/15733008/diff/64001/net/dns/mdns_query.h
File net/dns/mdns_query.h (right):

https://codereview.chromium.org/15733008/diff/64001/net/dns/mdns_query.h#newc...
net/dns/mdns_query.h:22: class MDnsQuery {
On 2013/06/07 23:54:43, Noam Samuel wrote:
> On 2013/06/07 16:40:02, szym wrote:
> > Could we make this CL a little bit smaller, by using DnsQuery now and moving
> > this to a subsequent CL?
> 
> Done. Note that this adds the RD flag to queries, which isn't compliant with
the
> mDNS spec.

I missed the RD bit. We certainly don't want clearly incorrect code. I'm ok with
MDnsQuery, but the comment needs to legitimize its presence. Alternatively, you
could add DnsQuery::set_flags(uint16)  (pass them in host order).

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client.h
File net/dns/mdns_client.h (right):

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client.h#new...
net/dns/mdns_client.h:141: int flags,  // See MDnsTransactionFlags
Move this to the comment above.

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:530: : socket_handler_ipv4_(this,
GetIPEndPoint(kMDnsMulticastGroupIPv4,
If you always use this with the same port, why not make it
GetMDnsIPEndPoint(const string& multicast_group)?

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:220: void Reset();
Make it private.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:60: // and is deleted whenever the number of
references for listening reachers
nit: "reaches"

Suggest: "whenever the number of listeners reaches zero."

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:62: // tasks scheduled on it are not delivered after
it is destroyed.
The last sentence does not seem to be adding much.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:124: explicit MDnsClientImpl(MDnsConnectionFactory*
connection_factory);
I suggest always passing the factory into the constructor. MDnsClientImpl is not
the public interface. Pass the real factory when you construct it in
MDnsClient::GetInstance.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:157: MDnsConnectionFactory* connection_factory_;
I suggest it should always own it. Whenever a situation like this arises, just
keep a plain (unsafe) pointer in the test fixture.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:292: // Callback for handling a datagram being
recieved on either ipv4 or ipv6.
nit: "received"

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:293: // Responsible for ensuring we request another
packet from the network.
This sentence is obsolete.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:231: class MockMDnsConnection : public
MDnsConnection {
I think this could be equivalently built from MDnsConnectionImpl with mock
sockets. It's possible you could accomplish that with existing
MockDatagramServerSocket. Alternatively, a "SimpleMockSocket" would be:

int RecvFrom(buffer, callback) {
  recv_buffer_ = buffer;
  recv_callback_ = callback;
  return ERR_IO_PENDING;
}

int SendTo(buffer, callback) {
  send_buffer_ = buffer;
  send_callback_ = callback;
  return ERR_IO_PENDING;
}

bool SimulateReceive(string data) {
  if(recv_buffer_ == NULL) return false;
  memcpy(recv_buffer_, data);
  recv_buffer_ = NULL;
  recv_callback_.Run(data.size());
  return true;
}

bool ExpectSendData(string data) {
  if (send_buffer_ == NULL) return false;
  if (memcmp(send_buffer_, data)) return false;
  send_buffer_ = NULL;
  send_callback_.Run(data.size());
  return true;
}

Then inject this into MDnsClientImpl using DatagramServerSocketFactory (instead
of MDnsConnectionFactory).

MDnsTest() {
  MDnsTest(): factory_(new MockSocketFactory),
              test_client_(new MDnsClientImpl(factory_)) {}

  MockSocketFactory* factory_;  // owned by test_client_
  scoped_ptr<MDnsClientImpl> test_client_;
};

where

MockSocketFactory : DatagramServerSocketFactory {
  scoped_ptr<DatagramServerSocket> CreateSocket() {
    socket = new MockDatagramServerSocket();
    sockets_.push(socket);
    return new MockDatagramServerSocket();
  }
  bool ExpectSendData(string data) {
    for (socket in sockets_)
      if (!socket->ExpectSendData(data))
        return false;
  }

  vector<DatagramServerSocket*> sockets_; // sockets owned by the caller.
}

This approach would completely avoid the need for the pure virtual
MDnsConnection interface. The price is only a little bit of awkwardness because
you need to go through all the issued sockets by the factory.

======
Why it makes sense? DatagramServerSocket is already an interface which can be
mocked out for testing. There is no need to add yet another interface unless it
significantly simplifies the mocks. I don't think that's the case here.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:298: class RecordParsedCopyContainer {
It seems you could avoid the clone by following the pattern of ExpectPacket,
i.e., store the expectations before the callback is called. (Maybe call it
ExpectedNptrRecord.)

If you prefer this, you should copy only the interesting parts out of the
record: |name| and |ptrdomain| and verify that it is a PTR record. (Maybe call
it NptrRecordContainer.)

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:336: void SendPacket(const char* packet,
unsigned size);
This name is a bit misleading. You are simulating a packet arriving on the
socket. Needs a better name and a comment.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:337: void ExpectPtrRecord(
This name is misleading. Should be ExpectContainsPtrRecord, but bool
ContainsPtrRecord would be better. If you fail within a method, you will need to
rely on stack trace to diagnose the bug. If possible, it's generally better to
return bool and wrap it in EXPECT_TRUE(...).

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:909: return
JoinGroupInternal(IPAddressToString(group_address));
Shouldn't this be LeaveGroupInternal?

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:957: TEST_F(MDnsConnectionTest,
RecieveSynchronous) {
nit: Receive

https://codereview.chromium.org/15733008/diff/93001/net/dns/record_parsed.h
File net/dns/record_parsed.h (right):

https://codereview.chromium.org/15733008/diff/93001/net/dns/record_parsed.h#n...
net/dns/record_parsed.h:28: scoped_ptr<const RecordParsed> Clone() const;
How did this make its way back into this CL? It seems to be used in tests only.
Does the test really need to Clone?

Noam Samuel

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unittest.cc File net/dns/mdns_client_unittest.cc (right): https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unittest.cc#newcode231 net/dns/mdns_client_unittest.cc:231: class MockMDnsConnection : public MDnsConnection { Should I call ...

7 years, 6 months ago (2013-06-11 18:31:14 UTC) #21

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:231: class MockMDnsConnection : public
MDnsConnection {
Should I call the factory interface DatagramServerSocketFactory, or should the
name be more specific? Right now I'm using MDnsDatagramServerSocketFactory as a
placeholder name, but it is a bit unwieldy.

On 2013/06/10 21:58:29, szym wrote:
> I think this could be equivalently built from MDnsConnectionImpl with mock
> sockets. It's possible you could accomplish that with existing
> MockDatagramServerSocket. Alternatively, a "SimpleMockSocket" would be:
> 
> int RecvFrom(buffer, callback) {
>   recv_buffer_ = buffer;
>   recv_callback_ = callback;
>   return ERR_IO_PENDING;
> }
> 
> int SendTo(buffer, callback) {
>   send_buffer_ = buffer;
>   send_callback_ = callback;
>   return ERR_IO_PENDING;
> }
> 
> bool SimulateReceive(string data) {
>   if(recv_buffer_ == NULL) return false;
>   memcpy(recv_buffer_, data);
>   recv_buffer_ = NULL;
>   recv_callback_.Run(data.size());
>   return true;
> }
> 
> bool ExpectSendData(string data) {
>   if (send_buffer_ == NULL) return false;
>   if (memcmp(send_buffer_, data)) return false;
>   send_buffer_ = NULL;
>   send_callback_.Run(data.size());
>   return true;
> }
> 
> Then inject this into MDnsClientImpl using DatagramServerSocketFactory
(instead
> of MDnsConnectionFactory).
> 
> MDnsTest() {
>   MDnsTest(): factory_(new MockSocketFactory),
>               test_client_(new MDnsClientImpl(factory_)) {}
> 
>   MockSocketFactory* factory_;  // owned by test_client_
>   scoped_ptr<MDnsClientImpl> test_client_;
> };
> 
> where
> 
> MockSocketFactory : DatagramServerSocketFactory {
>   scoped_ptr<DatagramServerSocket> CreateSocket() {
>     socket = new MockDatagramServerSocket();
>     sockets_.push(socket);
>     return new MockDatagramServerSocket();
>   }
>   bool ExpectSendData(string data) {
>     for (socket in sockets_)
>       if (!socket->ExpectSendData(data))
>         return false;
>   }
> 
>   vector<DatagramServerSocket*> sockets_; // sockets owned by the caller.
> }
> 
> This approach would completely avoid the need for the pure virtual
> MDnsConnection interface. The price is only a little bit of awkwardness
because
> you need to go through all the issued sockets by the factory.
> 
> ======
> Why it makes sense? DatagramServerSocket is already an interface which can be
> mocked out for testing. There is no need to add yet another interface unless
it
> significantly simplifies the mocks. I don't think that's the case here.

Noam Samuel

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client.h File net/dns/mdns_client.h (right): https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client.h#newcode141 net/dns/mdns_client.h:141: int flags, // See MDnsTransactionFlags On 2013/06/10 21:58:28, szym ...

7 years, 6 months ago (2013-06-11 20:35:03 UTC) #22

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client.h
File net/dns/mdns_client.h (right):

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client.h#new...
net/dns/mdns_client.h:141: int flags,  // See MDnsTransactionFlags
On 2013/06/10 21:58:28, szym wrote:
> Move this to the comment above.

Done.

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:530: : socket_handler_ipv4_(this,
GetIPEndPoint(kMDnsMulticastGroupIPv4,
On 2013/06/10 21:58:28, szym wrote:
> If you always use this with the same port, why not make it
> GetMDnsIPEndPoint(const string& multicast_group)?

Done.

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/15733008/diff/82002/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:220: void Reset();
On 2013/06/10 21:58:28, szym wrote:
> Make it private.

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:60: // and is deleted whenever the number of
references for listening reachers
On 2013/06/10 21:58:29, szym wrote:
> nit: "reaches"
> 
> Suggest: "whenever the number of listeners reaches zero."

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:62: // tasks scheduled on it are not delivered after
it is destroyed.
On 2013/06/10 21:58:29, szym wrote:
> The last sentence does not seem to be adding much.

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:124: explicit MDnsClientImpl(MDnsConnectionFactory*
connection_factory);
On 2013/06/10 21:58:29, szym wrote:
> I suggest always passing the factory into the constructor. MDnsClientImpl is
not
> the public interface. Pass the real factory when you construct it in
> MDnsClient::GetInstance.

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:157: MDnsConnectionFactory* connection_factory_;
On 2013/06/10 21:58:29, szym wrote:
> I suggest it should always own it. Whenever a situation like this arises, just
> keep a plain (unsafe) pointer in the test fixture.

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:292: // Callback for handling a datagram being
recieved on either ipv4 or ipv6.
On 2013/06/10 21:58:29, szym wrote:
> nit: "received"

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:293: // Responsible for ensuring we request another
packet from the network.
On 2013/06/10 21:58:29, szym wrote:
> This sentence is obsolete.

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:231: class MockMDnsConnection : public
MDnsConnection {
Done with name MDnsDatagramServerSocketFactory.

On 2013/06/11 18:31:14, Noam Samuel wrote:
> Should I call the factory interface DatagramServerSocketFactory, or should the
> name be more specific? Right now I'm using MDnsDatagramServerSocketFactory as
a
> placeholder name, but it is a bit unwieldy.
> 
> On 2013/06/10 21:58:29, szym wrote:
> > I think this could be equivalently built from MDnsConnectionImpl with mock
> > sockets. It's possible you could accomplish that with existing
> > MockDatagramServerSocket. Alternatively, a "SimpleMockSocket" would be:
> > 
> > int RecvFrom(buffer, callback) {
> >   recv_buffer_ = buffer;
> >   recv_callback_ = callback;
> >   return ERR_IO_PENDING;
> > }
> > 
> > int SendTo(buffer, callback) {
> >   send_buffer_ = buffer;
> >   send_callback_ = callback;
> >   return ERR_IO_PENDING;
> > }
> > 
> > bool SimulateReceive(string data) {
> >   if(recv_buffer_ == NULL) return false;
> >   memcpy(recv_buffer_, data);
> >   recv_buffer_ = NULL;
> >   recv_callback_.Run(data.size());
> >   return true;
> > }
> > 
> > bool ExpectSendData(string data) {
> >   if (send_buffer_ == NULL) return false;
> >   if (memcmp(send_buffer_, data)) return false;
> >   send_buffer_ = NULL;
> >   send_callback_.Run(data.size());
> >   return true;
> > }
> > 
> > Then inject this into MDnsClientImpl using DatagramServerSocketFactory
> (instead
> > of MDnsConnectionFactory).
> > 
> > MDnsTest() {
> >   MDnsTest(): factory_(new MockSocketFactory),
> >               test_client_(new MDnsClientImpl(factory_)) {}
> > 
> >   MockSocketFactory* factory_;  // owned by test_client_
> >   scoped_ptr<MDnsClientImpl> test_client_;
> > };
> > 
> > where
> > 
> > MockSocketFactory : DatagramServerSocketFactory {
> >   scoped_ptr<DatagramServerSocket> CreateSocket() {
> >     socket = new MockDatagramServerSocket();
> >     sockets_.push(socket);
> >     return new MockDatagramServerSocket();
> >   }
> >   bool ExpectSendData(string data) {
> >     for (socket in sockets_)
> >       if (!socket->ExpectSendData(data))
> >         return false;
> >   }
> > 
> >   vector<DatagramServerSocket*> sockets_; // sockets owned by the caller.
> > }
> > 
> > This approach would completely avoid the need for the pure virtual
> > MDnsConnection interface. The price is only a little bit of awkwardness
> because
> > you need to go through all the issued sockets by the factory.
> > 
> > ======
> > Why it makes sense? DatagramServerSocket is already an interface which can
be
> > mocked out for testing. There is no need to add yet another interface unless
> it
> > significantly simplifies the mocks. I don't think that's the case here.
>

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:298: class RecordParsedCopyContainer {
On 2013/06/10 21:58:29, szym wrote:
> It seems you could avoid the clone by following the pattern of ExpectPacket,
> i.e., store the expectations before the callback is called. (Maybe call it
> ExpectedNptrRecord.)
> 
> If you prefer this, you should copy only the interesting parts out of the
> record: |name| and |ptrdomain| and verify that it is a PTR record. (Maybe call
> it NptrRecordContainer.)

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:336: void SendPacket(const char* packet,
unsigned size);
On 2013/06/10 21:58:29, szym wrote:
> This name is a bit misleading. You are simulating a packet arriving on the
> socket. Needs a better name and a comment.

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:337: void ExpectPtrRecord(
On 2013/06/10 21:58:29, szym wrote:
> This name is misleading. Should be ExpectContainsPtrRecord, but bool
> ContainsPtrRecord would be better. If you fail within a method, you will need
to
> rely on stack trace to diagnose the bug. If possible, it's generally better to
> return bool and wrap it in EXPECT_TRUE(...).

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:909: return
JoinGroupInternal(IPAddressToString(group_address));
On 2013/06/10 21:58:29, szym wrote:
> Shouldn't this be LeaveGroupInternal?

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:957: TEST_F(MDnsConnectionTest,
RecieveSynchronous) {
On 2013/06/10 21:58:29, szym wrote:
> nit: Receive

Done.

https://codereview.chromium.org/15733008/diff/93001/net/dns/record_parsed.h
File net/dns/record_parsed.h (right):

https://codereview.chromium.org/15733008/diff/93001/net/dns/record_parsed.h#n...
net/dns/record_parsed.h:28: scoped_ptr<const RecordParsed> Clone() const;
On 2013/06/10 21:58:29, szym wrote:
> How did this make its way back into this CL? It seems to be used in tests
only.
> Does the test really need to Clone?

Removed.

szym

I appreciate your patience with the slow review. Still a bunch of nits, but LGTM ...

7 years, 6 months ago (2013-06-12 21:35:41 UTC) #23

Noam Samuel

https://codereview.chromium.org/15733008/diff/128001/net/dns/mdns_client.cc File net/dns/mdns_client.cc (right): https://codereview.chromium.org/15733008/diff/128001/net/dns/mdns_client.cc#newcode6 net/dns/mdns_client.cc:6: #include "net/dns/mdns_client_impl.h" On 2013/06/12 21:35:41, szym wrote: > nit: ...

7 years, 6 months ago (2013-06-13 01:08:40 UTC) #24

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/15733008/158002

7 years, 6 months ago (2013-06-13 01:09:11 UTC) #25

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/15733008/158002

7 years, 6 months ago (2013-06-13 02:29:07 UTC) #26

commit-bot: I haz the power

Sorry for I got bad news for ya. Compile failed with a clobber build on ...

7 years, 6 months ago (2013-06-13 06:47:46 UTC) #27

Noam Samuel

On 2013/06/13 06:47:46, I haz the power (commit-bot) wrote: > Sorry for I got bad ...

7 years, 6 months ago (2013-06-13 17:36:52 UTC) #28

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/15733008/158002

7 years, 6 months ago (2013-06-13 17:37:31 UTC) #29

szym

https://codereview.chromium.org/15733008/diff/158002/net/dns/mdns_client_impl.cc File net/dns/mdns_client_impl.cc (right): https://codereview.chromium.org/15733008/diff/158002/net/dns/mdns_client_impl.cc#newcode481 net/dns/mdns_client_impl.cc:481: DCHECK((flags_ & MDnsTransaction::FLAG_MASK) == flags_); For future reference, you ...

7 years, 6 months ago (2013-06-13 19:20:00 UTC) #30

Message was sent while issue was closed.

Change committed as 206170

Issue 15733008: Multicast DNS implementation (initial) (Closed)

Description

Patch Set 1 #

Patch Set 2 : Uploading to check comments against diff (next upload will have files renamed) #

Patch Set 3 : Renamed files from "listener" to "client" #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Messages