Closed Bug 1324925 Opened 3 years ago Closed 3 years ago

[EME] MediaKeySession.expiration reported in milliseconds instead of seconds

Categories

(Core :: Audio/Video: GMP, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX
mozilla53
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I noticed that MediaKeySession.expiration is being reported in milliseconds, but it's actually specified to be seconds since the epoch.

https://w3c.github.io/encrypted-media/#dom-mediakeysession-expiration
https://w3c.github.io/encrypted-media/#time

This means we'll be exposing to JS the incorrect expiration time. The CDM will still be seeing the correct expiration time.
Comment on attachment 8820442 [details]
Bug 1324925 - Convert GMPTimestamp to epoch seconds in GMPDecryptorChild::ExpirationChange().

https://reviewboard.mozilla.org/r/99936/#review100414

Ideally we'd want different C++ types internally to distinguish the different timescales. For another bug...
Attachment #8820442 - Flags: review?(gsquelart) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/229e1278d976
Convert GMPTimestamp to epoch seconds in GMPDecryptorChild::ExpirationChange(). r=gerald
https://hg.mozilla.org/mozilla-central/rev/229e1278d976
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
I don't think this is correct.  It is supposed to be reported in milliseconds.  The spec says that it should have millisecond accuracy and should report the same as the Date object.  If you follow the ECMAScript Date object spec, it says in the "Time Values and Time Ranges" section that the number given should be represented in milliseconds.

Before, the spec explicitly said milliseconds since the epoch.  But there was confusion about whether to include leap seconds.  So the spec changed to explicitly list leap seconds (which it says consistent with Unix time).  I think the intent is to keep it with millisecond time.

See: https://github.com/w3c/encrypted-media/issues/59
Flags: needinfo?(joeyparrish)
When the spec says "Time is intended to be consistent with Unix time", to me, that would mean time in seconds.  This definition seems to agree with interpretation in seconds: https://en.wikipedia.org/wiki/Unix_time

"Unix time (also known as POSIX time or Epoch time) is a system for describing instants in time, defined as the number of seconds that have elapsed since 00:00:00 Coordinated Universal Time (UTC), Thursday, 1 January 1970, not counting leap seconds."

I will request clarification on the EME github issue where the change was discussed: https://github.com/w3c/encrypted-media/issues/59
Looks like the spec is just badly written and meant "ES time", not "Unix time" (not the same thing!).
Flags: needinfo?(cpearce)
Thanks for pointing this out modmaker.
(In reply to Chris Pearce (:cpearce) from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 627e9374ea4d9d8baeec9794bf415f6b9a8ca63a
> Bug 1324925 - Backed out changeset 229e1278d976. r=backout

Backed out from Firefox 54. Will request uplift to backout from 53.
Flags: needinfo?(cpearce)
Flags: needinfo?(joeyparrish)
Attached patch Patch: Backout from Aurora (obsolete) — Splinter Review
Comment on attachment 8830960 [details] [diff] [review]
Patch: Backout from Aurora

Requesting approval to back out this change from 53, i.e. Aurora.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1324925, i.e. this bug.
[User impact if declined]: Some EME videos will incorrectly report their expiration time to JavaScript. This could potentially cause EME video playback to fail.
[Is this code covered by automated tests?]: We don't have tests covering EME expiration as we don't have our own DRM server.
[Has the fix been verified in Nightly?]: No. I've verified in a local build that the behaviour is as expected.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's basically removing a divide by 1000.
[String changes made/needed]: None.
Attachment #8830960 - Flags: approval-mozilla-aurora?
I must remember to fixup the tracking flags once the backout completes.
Flags: needinfo?(cpearce)
Did this land already? We should file new bugs for backouts (the flags are in a confusing state for sheriffs/relman)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Did this land already? We should file new bugs for backouts (the flags are
> in a confusing state for sheriffs/relman)

Yes, the backout landed already.
Flags: needinfo?(cpearce)
Depends on: 1335166
I've filed bug 1335166 to backout this bug from Aurora.

Maybe we need a new bug status filed "backed-out"?
Comment on attachment 8830960 [details] [diff] [review]
Patch: Backout from Aurora

I'll do the backout of this bug in Aurora in bug 1335166.
Attachment #8830960 - Attachment is obsolete: true
Attachment #8830960 - Flags: approval-mozilla-aurora?
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.