Avoid lossy time stamp conversions when caching metadata.

Avoid lossy time stamp conversions when caching metadata.

base::Time uses int64, but blink time stamps use double. Blink Resource
response time stamps are converted from base::Time. When storing
Resource metadata to the HTTP cache, that double time stamp is converted
back to base::Time. This conversion is lossy and causes metadata to not
being cached due to time stamp mismatch.

The fix is not to convert the time stamp back to base::Time, and do the
comparison in double instead.

R=gavinp@chromium.org, jochen@chromium.org
BUG=chromium:453298
Committed: https://crrev.com/9222efb0a3410e4c8b2ab1072027b66262dfb594
Cr-Commit-Position: refs/heads/master@{#313922}

[jochen (gone - plz use gerrit), 2015-01-30 14:38:35]

lgtm

[gavinp, 2015-01-30 15:53:50]

lgtm.

I'm a bit anxious about comparing doubles for equality, but this seems to be
doing it right.

[Yang, 2015-01-30 16:14:32]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2015-01-30 16:15:45]

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

[commit-bot: I haz the power, 2015-01-30 16:19:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-30 16:22:17]

Patchset 2 (id:??) landed as
https://crrev.com/9222efb0a3410e4c8b2ab1072027b66262dfb594
Cr-Commit-Position: refs/heads/master@{#313922}

[rvargas (doing something else), 2015-02-05 20:23:44]

rvargas@chromium.org changed reviewers:
+ rvargas@chromium.org

[rvargas (doing something else), 2015-02-05 20:23:44]

https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h#ne...
net/http/http_cache.h:181: double expected_response_time,
Isn't there another way to fix this?

This code is not really aware of blink types... it is written with chromium base
types so having time expressed by a double is completely unnatural.

Besides, int64 has a higher resolution than double so you are choosing the
lossiest of the two types.

Doesn't seem right!

[Yang, 2015-02-05 21:12:40]

On 2015/02/05 20:23:44, rvargas wrote:
> https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h
> File net/http/http_cache.h (right):
> 
>
https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h#ne...
> net/http/http_cache.h:181: double expected_response_time,
> Isn't there another way to fix this?
> 
> This code is not really aware of blink types... it is written with chromium
base
> types so having time expressed by a double is completely unnatural.
> 
> Besides, int64 has a higher resolution than double so you are choosing the
> lossiest of the two types.
> 
> Doesn't seem right!

I agree that this is not optimal. However, this does fix a very real bug. The
higher resolution of int64 has already been lost when the blink resource was
created. Given that RenderMessageFilter::OnCacheableMetadataAvailable takes a
double time stamp as argument, I haven't made the issue much worse by allowing
double time stamp in Chromium. Converting that double time stamp back to int64
is definitely the wrong thing to do, so repeating the (deterministically) lossy
conversion before time stamp comparison seems the only reasonable fix. The other
clean alternative I see is to refactor all of blink to use int64 instead of
double for time stamps, and there would be a ton of places to change. I myself
am far from being familiar enough with blink's code base to attempt a big
refactoring like this.

[Yang, 2015-02-05 21:14:11]

On 2015/02/05 21:12:40, Yang wrote:
> On 2015/02/05 20:23:44, rvargas wrote:
> > https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h
> > File net/http/http_cache.h (right):
> > 
> >
>
https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h#ne...
> > net/http/http_cache.h:181: double expected_response_time,
> > Isn't there another way to fix this?
> > 
> > This code is not really aware of blink types... it is written with chromium
> base
> > types so having time expressed by a double is completely unnatural.
> > 
> > Besides, int64 has a higher resolution than double so you are choosing the
> > lossiest of the two types.
> > 
> > Doesn't seem right!
> 
> I agree that this is not optimal. However, this does fix a very real bug. The
> higher resolution of int64 has already been lost when the blink resource was
> created. Given that RenderMessageFilter::OnCacheableMetadataAvailable takes a
> double time stamp as argument, I haven't made the issue much worse by allowing
> double time stamp in Chromium. Converting that double time stamp back to int64
> is definitely the wrong thing to do, so repeating the (deterministically)
lossy
> conversion before time stamp comparison seems the only reasonable fix. The
other
> clean alternative I see is to refactor all of blink to use int64 instead of
> double for time stamps, and there would be a ton of places to change. I myself
> am far from being familiar enough with blink's code base to attempt a big
> refactoring like this.

Of course I could also convert the time stamp from int64 to double and back to
int64 to guarantee that potentially lossy conversion has been taken into
account, but that sounds even worse.

[rvargas (doing something else), 2015-02-05 22:32:29]

On 2015/02/05 21:14:11, Yang wrote:
> On 2015/02/05 21:12:40, Yang wrote:
> > On 2015/02/05 20:23:44, rvargas wrote:
> > > https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h
> > > File net/http/http_cache.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/887973003/diff/20001/net/http/http_cache.h#ne...
> > > net/http/http_cache.h:181: double expected_response_time,
> > > Isn't there another way to fix this?
> > > 
> > > This code is not really aware of blink types... it is written with
chromium
> > base
> > > types so having time expressed by a double is completely unnatural.
> > > 
> > > Besides, int64 has a higher resolution than double so you are choosing the
> > > lossiest of the two types.
> > > 
> > > Doesn't seem right!
> > 
> > I agree that this is not optimal. However, this does fix a very real bug.
The
> > higher resolution of int64 has already been lost when the blink resource was
> > created. Given that RenderMessageFilter::OnCacheableMetadataAvailable takes
a
> > double time stamp as argument, I haven't made the issue much worse by
allowing
> > double time stamp in Chromium. Converting that double time stamp back to
int64
> > is definitely the wrong thing to do, so repeating the (deterministically)
> lossy
> > conversion before time stamp comparison seems the only reasonable fix. The
> other
> > clean alternative I see is to refactor all of blink to use int64 instead of
> > double for time stamps, and there would be a ton of places to change. I
myself
> > am far from being familiar enough with blink's code base to attempt a big
> > refactoring like this.
> 
> Of course I could also convert the time stamp from int64 to double and back to
> int64 to guarantee that potentially lossy conversion has been taken into
> account, but that sounds even worse.

The problem is that this code is now inconsistent.
HttpResponseInfo::response_time is a base::Time, and that is the value that the 
API is using to identify a change in the resource. If the consumer decides to
store that value in a variable that doesn't provide enough resolution before
attempting to use WriteMetadata it is not really a problem with the API but with
the consumer (yes, I totally understand that it is a real bug). The correct fix
is not to have an API that in one place provides a base::Time and in another
expects a double for the "same" value. This may be the easiest fix, but not the
right one.

IMO, The IPC message should not be using a double.