Fix MDnsClient's cache entry cleanup logic.

Fix MDnsClient's cache entry cleanup logic.
The current implementation has an error in its time delay computation.
It computes negative timedeltas for cache entries that are
> Now(). This causes a CHECK failure in
IncomingTaskQueue::CalculateDelayedRuntime.

BUG=459443
CC=mfoltz@chromium.org

Committed: https://crrev.com/3e740be5fa1026ae8b39d99f1180b7c39b9d4b12
Cr-Commit-Position: refs/heads/master@{#318766}

[Kevin M, 2015-02-18 19:49:37]

kmarshall@chromium.org changed reviewers:
+ cbentze@chromium.org, mfoltz@chromium.org

[Kevin M, 2015-02-18 19:49:50]

kmarshall@chromium.org changed reviewers:
+ cbentzel@chromium.org
- cbentze@chromium.org

[cbentzel, 2015-02-18 21:19:12]

cbentzel@chromium.org changed reviewers:
+ gene@chromium.org

[cbentzel, 2015-02-18 21:19:12]

cbentzel@chromium.org changed reviewers:
+ gene@chromium.org

[cbentzel, 2015-02-18 21:19:13]

gene: I'll take a look but wanted you to see as well.

[cbentzel, 2015-02-18 21:19:13]

gene: I'll take a look but wanted you to see as well.

[cbentzel, 2015-02-18 21:19:14]

gene: I'll take a look but wanted you to see as well.

[cbentzel, 2015-02-18 21:19:15]

gene: I'll take a look but wanted you to see as well.

[cbentzel, 2015-02-18 21:21:08]

Can this get a test?

[gene, 2015-02-18 21:56:20]

Thanks Chris. This looks good to me.

[mark a. foltz, 2015-02-18 23:11:11]

lgtm

[cbentzel, 2015-02-22 13:39:24]

On 2015/02/18 23:11:11, mark a. foltz wrote:
> lgtm

Any chance for a test for this?

[Kevin M, 2015-02-23 17:36:19]

Hi Chris,

I'm writing the tests now. My original testing efforts involved moving the code
to Timers for scheduling, but that ended up being a nontrivial change that
affected large portions of the code - I'll send that out as a separate CL.

[Kevin M, 2015-02-24 23:43:13]

OK, added a targeted test. Please take a look.

[mark a. foltz, 2015-02-25 21:44:07]

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

https://codereview.chromium.org/937743003/diff/20001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:415:
base::Bind(&MDnsClientImpl::Core::OnRecordRemoved,
What cancels this callback when MDnsClientImpl is deleted?

https://codereview.chromium.org/937743003/diff/20001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:591: next_refresh1 - base::Time::Now());
If we are converting this class to use base::Clock, we should do it everywhere.

https://codereview.chromium.org/937743003/diff/20001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:596: next_refresh2 - base::Time::Now());
Ditto.

[Kevin M, 2015-02-26 01:04:41]

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

https://codereview.chromium.org/937743003/diff/20001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:415:
base::Bind(&MDnsClientImpl::Core::OnRecordRemoved,
On 2015/02/25 21:44:07, mark a. foltz wrote:
> What cancels this callback when MDnsClientImpl is deleted?

DoCleanup() can't be called if the MdnsClientImpl is deleted, because
MDnsCleanup::cleanup_timer_ will be deleted and unable to execute the DoCleanup
callback.

https://codereview.chromium.org/937743003/diff/20001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:591: next_refresh1 - base::Time::Now());
On 2015/02/25 21:44:06, mark a. foltz wrote:
> If we are converting this class to use base::Clock, we should do it
everywhere.

This required a bit of plumbing to route the Clock objects through to child
objects, but done.

https://codereview.chromium.org/937743003/diff/20001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:596: next_refresh2 - base::Time::Now());
On 2015/02/25 21:44:07, mark a. foltz wrote:
> Ditto.

Done.

[mark a. foltz, 2015-02-26 01:18:03]

lgtm

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

https://codereview.chromium.org/937743003/diff/20001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:415:
base::Bind(&MDnsClientImpl::Core::OnRecordRemoved,
On 2015/02/26 01:04:40, Kevin M wrote:
> On 2015/02/25 21:44:07, mark a. foltz wrote:
> > What cancels this callback when MDnsClientImpl is deleted?
> 
> DoCleanup() can't be called if the MdnsClientImpl is deleted, because
> MDnsCleanup::cleanup_timer_ will be deleted and unable to execute the
DoCleanup
> callback.

Acknowledged.

[Kevin M, 2015-02-26 18:00:33]

Thanks Mark. cbentzel, gene, PTAL.

[Kevin M, 2015-02-26 19:46:33]

I just had a watercooler conversation with miu@ about this and he advised me to
switch to TimeTicks wherever possible in this code. base::Time is prone to clock
skew due to NTP adjustments etc. TimeTicks, on the other hand, is monotonically
increasing, so it's immune to skew.

I'll look into making the change in this CL.

[mmenke, 2015-02-26 20:13:00]

On 2015/02/26 19:46:33, Kevin M wrote:
> I just had a watercooler conversation with miu@ about this and he advised me
to
> switch to TimeTicks wherever possible in this code. base::Time is prone to
clock
> skew due to NTP adjustments etc. TimeTicks, on the other hand, is
monotonically
> increasing, so it's immune to skew.
> 
> I'll look into making the change in this CL.

Just a drive by comment:  Time doesn't increase monotonically, because of NTP
and manual adjustement and the like.  TimeTicks, on the other hand, stops
incrementing when the system is in suspend mode.

[miu, 2015-02-26 20:22:18]

On 2015/02/26 20:13:00, mmenke wrote:
> Just a drive by comment:  Time doesn't increase monotonically, because of NTP
> and manual adjustement and the like.  TimeTicks, on the other hand, stops
> incrementing when the system is in suspend mode.

Good point.  For cache expiration, base::Time probably should be used, with the
code carefully accounting for "fuzziness" due to skew.  Kevin, be sure to
document this special-handling need in the code.

[Kevin M, 2015-02-27 00:18:30]

The fix in this CL makes the code handle skew more gracefully.

[gene, 2015-03-02 18:43:10]

lgtm. two small nits

[Kevin M, 2015-03-02 18:47:47]

Thanks Gene. I don't see your nits, though?

[gene, 2015-03-02 19:37:16]

https://codereview.chromium.org/937743003/diff/40001/net/dns/mdns_client_unit...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/937743003/diff/40001/net/dns/mdns_client_unit...
net/dns/mdns_client_unittest.cc:242: 0x00, 0x00,                          // TTL
(4 bytes) is 1 second;
nit: can you move 0x00, 0x01 here from the line below, so comment will be more
accurate?

https://codereview.chromium.org/937743003/diff/40001/net/dns/mdns_client_unit...
net/dns/mdns_client_unittest.cc:251: 0x00, 0x00,              // TTL (4 bytes)
is 3 seconds.
same here

[Kevin M, 2015-03-02 20:23:46]

https://codereview.chromium.org/937743003/diff/40001/net/dns/mdns_client_unit...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/937743003/diff/40001/net/dns/mdns_client_unit...
net/dns/mdns_client_unittest.cc:242: 0x00, 0x00,                          // TTL
(4 bytes) is 1 second;
On 2015/03/02 19:37:16, gene wrote:
> nit: can you move 0x00, 0x01 here from the line below, so comment will be more
> accurate?

Hmm, "git cl format" screwed this up.
I'll put in an empty comment to force the formatting.

https://codereview.chromium.org/937743003/diff/40001/net/dns/mdns_client_unit...
net/dns/mdns_client_unittest.cc:251: 0x00, 0x00,              // TTL (4 bytes)
is 3 seconds.
On 2015/03/02 19:37:16, gene wrote:
> same here

Done.

[Kevin M, 2015-03-02 20:24:00]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2015-03-02 20:24:00]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
gene@chromium.org
Link to the patchset: https://codereview.chromium.org/937743003/#ps80001 (title:
"Formatting")

[commit-bot: I haz the power, 2015-03-02 20:25:44]

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

[commit-bot: I haz the power, 2015-03-02 20:42:25]

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

[commit-bot: I haz the power, 2015-03-02 20:42:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[cbentzel, 2015-03-02 21:17:08]

On 2015/03/02 20:42:26, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

LGTM

Thanks for writing the tests (and harnesses)!

[cbentzel, 2015-03-02 21:17:10]

On 2015/03/02 20:42:26, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

LGTM

Thanks for writing the tests (and harnesses)!

[Kevin M, 2015-03-02 21:25:38]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2015-03-02 21:26:38]

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

[commit-bot: I haz the power, 2015-03-02 21:30:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-02 21:31:49]

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