There's no new CDM on Android but does seem a recent regression.

What device do you use? Does this repro on multiple devices?

Assign to jrummell@ to take a look. Thank you!

I used pixel C with Android 6.0.1. I will try other Android device and report back.

I can repro this on ToT, it seems there's no key to decrypt the content: NO_KEY returned for both audio and video.

I also tried Youtube purchased video and it's playing fine.

I suspect this is a player/server issue given Youtube is playing. But it doesn't make sense that M56 doesn't have this issue. I'll try M56 when I have time.

Assign back to me since jrummell@ is busy :)

This is broken by https://codereview.chromium.org/2593073002/

Previously, null base::Time (internal value being 0) is converted to NaN. For license expiration, NaN means "never expire".

Now with this change, null base::Time is converted to |1601-01-01 00:00:00 UTC|. When the player check the expiration time, since this license has expired 400 years ago, it closed the session immediately. Then when we try to decode, both audio and video decoder hit NO_KEY and playback stalls.

I have commented in that CL and we'll see what we can do to fix this.

Hmm...This isn't the first time code has broken because of the special "zero == is_null()" interpretation of base::Time. It was a bad design idea because lots of valid time math can result in zero (not meaning is_null()). So, I think it makes  sense to fix base::Time for everyone, and remove the is_null() concept.

As for this issue, why not just handle NaN as a special case? Other code modules may not interpret NaN the same way the EME API does, and so it doesn't make sense to bake this assumption in the base library. The EME impl could do this, for example:

  base::Optional<base::Time> expires_at;
  if (!std::isnan(jsTimeValue)) {
    base::Time converted;
    if (!base::Time::FromJSValue(jsTimeValue)) {
      // handle out-of-range error
    } else {
      expires_at = converted;
    ]
  } else {
    // expires_at is base::nullopt, which later code interprets as "never expires"
  }

...

  if (expires_at && *expires_at < base::Time::Now()) {
    // license expired
  } else {
    // license still valid (either because it never expires or it will in the future)
  }

The CL that broke this was landed because otherwise converting from -epoch time value (valid value in JS)  to base::Time and converting back would lead to information loss and be very surprising. I don't think it is acceptable to have this 
very broken behavior in our time library.

> Also, it seems inconsistent that Time::ToJavaTime() still checks is_null().
I agree. We should fix that too. I was only concerned with JS time conversion which is what Blink code may start using. I totally agree with miu@ that having "zero == is_null()" was a bad design and we should just remove is_null() altogether from base::Time and any code that needs a invalid time value can use base::Optional<base::Time> or something like that.

So I don't think we should go back to legacy behavior however if this is difficult thing to fix in your client code quickly then I am fine reverting that patch until that is corrected.

Re #7 #8: Agreed that NaN can be handled separately, maybe after  base::Time API is updated.

For now, looking at M57, media EME code still looks reasonable to me given the current time.h API, where "null" time is allowed and clearly documented. Also have you searched in the code base to make sure no other code is also relying on this behavior?

I feel the right way to make a base/ API change is that:
1. Update the interface in .h file, along with all the documentation.
2. Update the implementation.
3. Fix all code that depends on the API.

It seems to me we only did (2) in the change. And it doesn't make sense to me to do media/EME change before (1) happens.

Was the previous behavior breaking anything? If not, how about we revert the CL in M57 and land a proper fix with API change in M58? If "null" time is not supported in base::Time, then I can definitely do the changes suggested by miu@. Note that when you drop "null" support in base::Time, you also need to make sure Time::ToJavaTime() is not broken.

miu / majidvp: Kindly ping on comments :)

I plan to change our implementation to use std::numeric_limits<double>::quiet_NaN() as a special signal for null time.

But the current base::Time API doesn't differentiate these two cases:
https://cs.chromium.org/chromium/src/base/time/time.cc?rcl=1485274695&l=160

I feel it would be better that we support |1601-01-01 00:00:00 UTC|, but at the same time, also support a "null" base::Time. We can add a bool flag internally, or use base::Optional() within base::Time.

WDYT?

re #9:
I did originally search in the codebase but nothing obvious jumped to me as potentially breaking. But the change is subtle and clearly I have missed this one :(. I suggest you add a test in your module to ensure the behavior your want w.r.t the EME license expiration logic.

I am fine reverting my the CL for now. It was done to prevent a data loss issue but there is no concrete case of it which I can point out. Blink is not using From/ToJsTime yet so it is not an issue there. I am going to remove isNull() method the API surfaced exposed in Blink to avoid depending on this problematic API.

/cc tkent@ and esprehn@ for awareness.

re #11: 
I don't think having a "null" time value is a good idea. As you pointed out it is possible to treat validity/nullness at a higher level e.g., using base::Optional and similar constructs. In fact this is how internal google equivalent of base::Time seems to work. 

What is the value of having base::Optional inside base::Time? Most usecases for base::Time don't need this nullness feature so paying the extra cost (readability and added storage) seems unnecessary and harmful. 

Why do you need to feed std::numeric_limits<double>::quiet_NaN() to base::Time::FromDoubleT? See miu@ example in #7.

Ok, I'm also fine with reverting for now, but the logic flaw still exists; and we should fix that for M58. It looks like there are a number of variants of "convert to/from JS double" in our base::Time library; and none of them map all possible inputs to a reasonable output. (e.g., NaN or +/- inf or anything outside of the approx. range [-2^46,+2^46]).

So, it looks like we need a more long-term approach to fixing these converters. First of all, we really need to get rid of the "is_null" concept in the base::Time* classes. I myself have come across bugs in my own projects where valid time math led to a zero value being later interpreted as a null. Once we do that, and client code is handling its own "null" concept, then we can focus the converters on the simpler problem of mapping the entire possible range of JS time to that of base::Time (or returning an out-of-range error).

I'll take a closer look at everything and write up a proposal for discussion on chromium-dev@.

miu: Thanks for looking into this!!! The plan sgtm!

Meanwhile, I have a CL from EME side to fix this:
https://chromiumcodereview.appspot.com/2650033007/

IMHO the CL is valid regardless whether we revert the other CL. It actually fixes a FIXME in Blink which was a bit hacky in it's own.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d81adccb0dd24dd7c47ad0d3400dd745150e76db

commit d81adccb0dd24dd7c47ad0d3400dd745150e76db
Author: xhwang <xhwang@chromium.org>
Date: Thu Jan 26 06:04:30 2017

media: Fix expiry time convertion from base::Time to JS time

This is broken by https://codereview.chromium.org/2593073002/ where the
behavior of converting a null base::Time to JS time (double) has been
changed. We are discussing how to fix the base::Time API more properly.
For now, this check should work.

BUG= 679079 
TEST=Manually tested. I'll add auto tests later.

Review-Url: https://codereview.chromium.org/2650033007
Cr-Commit-Position: refs/heads/master@{#446246}

[modify] https://crrev.com/d81adccb0dd24dd7c47ad0d3400dd745150e76db/media/blink/webcontentdecryptionmodulesession_impl.cc
[modify] https://crrev.com/d81adccb0dd24dd7c47ad0d3400dd745150e76db/third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

#13 sgtm. I went ahead and reverted [1] the original CL as well in favor this long-term plan which is going to fix conversion issues in a consistent way.

Another area which I think we can improve on is to try to reduce number of conversions that are required. For example Mojo now supports base::Time* types so it should be possible to avoid some of existing conversions at Chrome/Blink boundary. Ideally we only need to convert when interfacing with an underlying platform API or a Web API.


[1] https://codereview.chromium.org/2655233003/

Re #19: Do you want to merge the revert in M57 as well? Then I don't need to merge mine. I feel it's better to merge your revert since there might be something else broken silently...

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ccfd6f67407374d5a23fb98eab6b39d887a0291

commit 2ccfd6f67407374d5a23fb98eab6b39d887a0291
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Fri Jan 27 08:30:15 2017

Revert of base::Time should correctly handle -epoch time values from Javascript (patchset #1 id:1 of https://codereview.chromium.org/2593073002/ )

Reason for revert:
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

Original issue's description:
> 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}

TBR=miu@chromium.org,majidvp@chromium.org
BUG= 625680 , 679079 

Review-Url: https://codereview.chromium.org/2655233003
Cr-Commit-Position: refs/heads/master@{#446340}
(cherry picked from commit 7ff9ceb9be03477632e2808268882889ff551185)

Review-Url: https://codereview.chromium.org/2663493002 .
Cr-Commit-Position: refs/branch-heads/2987@{#138}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/2ccfd6f67407374d5a23fb98eab6b39d887a0291/base/time/time.cc
[modify] https://crrev.com/2ccfd6f67407374d5a23fb98eab6b39d887a0291/base/time/time_unittest.cc

It turns out https://codereview.chromium.org/2650033007 is not a complete fix. I'll have a followup CL with more fix, and as promised, tests.

With that, I feel the best option for M57 is to revert https://codereview.chromium.org/2655233003/ in M57, which I did in #21.

Updated the summary to better reflect the root cause of this bug.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c54be1be4832bb10b83d43078ccb804cdddbbd14

commit c54be1be4832bb10b83d43078ccb804cdddbbd14
Author: xhwang <xhwang@chromium.org>
Date: Tue Jan 31 02:10:57 2017

Fix expiry time conversion in ContentDecryptorDelegate

Currently we use PPTimeToTime to convert PP_Time (double) to base::Time.
In PPTimeToTime, 0.0 is converted to base::Time::UnixEpoch() instead of
base::Time(), which is a "null" Time.

Now with https://chromiumcodereview.appspot.com/2650033007/, only null
base::Time will be converted to NaN in JavaScript. Therefore, a PP_Time
of 0.0 will be converted to Epoch in JS, which is wrong.

This CL update the code in ContentDecryptorDelegate such that PP_Time
0.0 will be converted to null base::Time.

BUG= 679079 
TEST=Updated browser and unit test to cover this.

Review-Url: https://codereview.chromium.org/2655413003
Cr-Commit-Position: refs/heads/master@{#447162}

[modify] https://crrev.com/c54be1be4832bb10b83d43078ccb804cdddbbd14/content/renderer/pepper/content_decryptor_delegate.cc
[modify] https://crrev.com/c54be1be4832bb10b83d43078ccb804cdddbbd14/media/cdm/aes_decryptor_unittest.cc
[modify] https://crrev.com/c54be1be4832bb10b83d43078ccb804cdddbbd14/media/cdm/cdm_adapter_unittest.cc
[modify] https://crrev.com/c54be1be4832bb10b83d43078ccb804cdddbbd14/media/cdm/ppapi/external_clear_key/clear_key_cdm.cc
[modify] https://crrev.com/c54be1be4832bb10b83d43078ccb804cdddbbd14/media/test/data/eme_player_js/clearkey_player.js