base::Time should correctly handle -epoch time values from Javascript

Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=TimeTest.JsTime

BUG=625680
Committed: https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
Cr-Commit-Position: refs/heads/master@{#440304}

[majidvp, 2016-12-21 16:58:29]

Description was changed from

==========
base::Time should correctly handle -epoch time values from Javascript

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=Time.JsTime

BUG=625680
==========

to

==========
Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=Time.JsTime

BUG=625680
==========

[majidvp, 2016-12-21 16:58:29]

majidvp@chromium.org changed reviewers:
+ miu@chromium.org

[majidvp, 2016-12-21 16:59:47]

miu@ PTAL

This was brought to my attention by tkent@ in blink-dev:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/2c3sDVonZ3A/itnzRQl1...

[majidvp, 2016-12-21 17:12:49]

The CQ bit was checked by majidvp@chromium.org to run a CQ dry run

[majidvp, 2016-12-21 17:13:07]

Description was changed from

==========
Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=Time.JsTime

BUG=625680
==========

to

==========
Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=TimeTest.JsTime

BUG=625680
==========

[commit-bot: I haz the power, 2016-12-21 17:13:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 18:51:59]

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

[commit-bot: I haz the power, 2016-12-21 18:52:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[miu, 2016-12-22 01:01:44]

The CQ bit was checked by miu@chromium.org

[miu, 2016-12-22 01:01:45]

lgtm

[commit-bot: I haz the power, 2016-12-22 01:02:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 01:36:37]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482368504861270, "parent_rev":
"75d6e604fbdc9eaeeccf355d1759bf4294f918a1", "commit_rev":
"5b2b32bcfe0e9618c1350fe6c8e42d139e771cc0"}

[commit-bot: I haz the power, 2016-12-22 01:37:07]

Description was changed from

==========
Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=TimeTest.JsTime

BUG=625680
==========

to

==========
Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=TimeTest.JsTime

BUG=625680

Review-Url: https://codereview.chromium.org/2593073002
==========

[commit-bot: I haz the power, 2016-12-22 01:37:08]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-22 01:39:34]

Description was changed from

==========
Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=TimeTest.JsTime

BUG=625680

Review-Url: https://codereview.chromium.org/2593073002
==========

to

==========
Correctly handle -epoch time values when converting from JS time to base::Time

-11644473600 seconds (which represents windows epoch time of
|1601-01-01 00:00:00 UTC|) is a valid time value in Javascript. Incidentally
this value is internally represented by 0 which is mistakenly confused with a
null time value.

FromJsTime is meant to be used to convert time values coming from Javascript
for which 0 or -epoch do not represent null values. So the extra check was
incorrect.

* In fact there is a comment in FromJsTime method making it clear that 0 is a
valid value but this was missed in ToJsTime method.

TEST=./base_unittests --gtest_filter=TimeTest.JsTime

BUG=625680

Committed: https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
Cr-Commit-Position: refs/heads/master@{#440304}
==========

[commit-bot: I haz the power, 2016-12-22 01:39:35]

Patchset 1 (id:??) landed as
https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
Cr-Commit-Position: refs/heads/master@{#440304}

[xhwang, 2017-01-21 05:40:22]

On 2016/12/22 01:39:35, commit-bot: I haz the power wrote:
> Patchset 1 (id:??) landed as
> https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
> Cr-Commit-Position: refs/heads/master@{#440304}

I don't feel this is a complete/proper fix. In time.h there are multiple places
noting that 0 represents "not initialized" or "null" time, e.g.:

// Because WebKit initializes double time value to 0 to indicate "not
// initialized", we map it to empty Time object that also means "not
// initialized".
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=479

// Contains the NULL time...
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=454

// Returns true if this object has not been initialized.
//
// Warning: Be careful when writing code that performs math on time values,
// since it's possible to produce a valid "zero" result that should not be
// interpreted as a "null" value.
bool is_null() const {
  return us_ == 0;
}
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=321

When people look at this API, they will assume 0 means a null value, which
should be converted to NaN in JS, not |1601-01-01 00:00:00 UTC|.

Currently in media code, we rely on this API to populate license expiration time
to JS. According to the EME spec, NaN expiration time means "never expire".
Based on time.h API, in Chromium code we use null Time (Time(0)) to indicate
this. With this change, now the expiration time becomes year 1601, and playback
fails in M57.

Is the old behavior breaking anything? If not, can we revert to previous
behavior and look for a more complete fix? Maybe we can use std::nan to indicate
null time instead of using 0.

[xhwang, 2017-01-21 05:47:53]

On 2017/01/21 05:40:22, xhwang_slow wrote:
> On 2016/12/22 01:39:35, commit-bot: I haz the power wrote:
> > Patchset 1 (id:??) landed as
> > https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
> > Cr-Commit-Position: refs/heads/master@{#440304}
> 
> I don't feel this is a complete/proper fix. In time.h there are multiple
places
> noting that 0 represents "not initialized" or "null" time, e.g.:
> 
> // Because WebKit initializes double time value to 0 to indicate "not
> // initialized", we map it to empty Time object that also means "not
> // initialized".
> https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=479
> 
> // Contains the NULL time...
> https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=454
> 
> // Returns true if this object has not been initialized.
> //
> // Warning: Be careful when writing code that performs math on time values,
> // since it's possible to produce a valid "zero" result that should not be
> // interpreted as a "null" value.
> bool is_null() const {
>   return us_ == 0;
> }
> https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=321
> 
> When people look at this API, they will assume 0 means a null value, which
> should be converted to NaN in JS, not |1601-01-01 00:00:00 UTC|.
> 
> Currently in media code, we rely on this API to populate license expiration
time
> to JS. According to the EME spec, NaN expiration time means "never expire".
> Based on time.h API, in Chromium code we use null Time (Time(0)) to indicate
> this. With this change, now the expiration time becomes year 1601, and
playback
> fails in M57.
> 
> Is the old behavior breaking anything? If not, can we revert to previous
> behavior and look for a more complete fix? Maybe we can use std::nan to
indicate
> null time instead of using 0.

FWIW, this is the bug tracking the regression:
https://bugs.chromium.org/p/chromium/issues/detail?id=679079#c6

[xhwang, 2017-01-21 05:49:18]

On 2017/01/21 05:47:53, xhwang_slow wrote:
> On 2017/01/21 05:40:22, xhwang_slow wrote:
> > On 2016/12/22 01:39:35, commit-bot: I haz the power wrote:
> > > Patchset 1 (id:??) landed as
> > > https://crrev.com/6650abf280f2987af809b0111ad00e5dba5723e4
> > > Cr-Commit-Position: refs/heads/master@{#440304}
> > 
> > I don't feel this is a complete/proper fix. In time.h there are multiple
> places
> > noting that 0 represents "not initialized" or "null" time, e.g.:
> > 
> > // Because WebKit initializes double time value to 0 to indicate "not
> > // initialized", we map it to empty Time object that also means "not
> > // initialized".
> > https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=479
> > 
> > // Contains the NULL time...
> > https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=454
> > 
> > // Returns true if this object has not been initialized.
> > //
> > // Warning: Be careful when writing code that performs math on time values,
> > // since it's possible to produce a valid "zero" result that should not be
> > // interpreted as a "null" value.
> > bool is_null() const {
> >   return us_ == 0;
> > }
> > https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=321
> > 
> > When people look at this API, they will assume 0 means a null value, which
> > should be converted to NaN in JS, not |1601-01-01 00:00:00 UTC|.
> > 
> > Currently in media code, we rely on this API to populate license expiration
> time
> > to JS. According to the EME spec, NaN expiration time means "never expire".
> > Based on time.h API, in Chromium code we use null Time (Time(0)) to indicate
> > this. With this change, now the expiration time becomes year 1601, and
> playback
> > fails in M57.
> > 
> > Is the old behavior breaking anything? If not, can we revert to previous
> > behavior and look for a more complete fix? Maybe we can use std::nan to
> indicate
> > null time instead of using 0.
> 
> FWIW, this is the bug tracking the regression:
> https://bugs.chromium.org/p/chromium/issues/detail?id=679079#c6

Also, it seems inconsistent that Time::ToJavaTime() still checks is_null().

[majidvp, 2017-01-26 15:07:42]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2655233003/ by majidvp@chromium.org.

The reason for reverting is: The patched broke EME player which was relying on
null time being converted to 0 in FromJsTime.

After discussing this, we agreed to try to change all conversion methods
together to ensure consistency. A precursor to doing this will be to get stop
treating zero as null.


For more info see: 
https://bugs.chromium.org/p/chromium/issues/detail?id=679079#c13.