Skip to content

|expiration| should be Unix time like Date() #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ddorwin opened this issue May 21, 2015 · 17 comments
Closed

|expiration| should be Unix time like Date() #59

ddorwin opened this issue May 21, 2015 · 17 comments

Comments

@ddorwin
Copy link
Contributor

ddorwin commented May 21, 2015

The expiration attribute is currently defined as

The time, in milliseconds since 01 January, 1970 UTC, ...

This could be interpreted as including leap seconds, which would appear to be inconsistent with ECMAScript and Date(), which appear to expect Unix time.

Ideally, there would be a definition of time that all specs could point to. I'll follow up with Web IDL.

@ddorwin ddorwin self-assigned this May 21, 2015
ddorwin referenced this issue in mwatson2/encrypted-media Oct 14, 2015
@ddorwin
Copy link
Contributor Author

ddorwin commented Oct 14, 2015

We now have several instances of "time" in the spec. Even if we can't get a common definition in Web IDL, we should have a local definition of time in the Definitions section that those instances can refer to.

Once I follow up with Web IDL, we can mark this "to be implemented". The proposal is:

@ddorwin ddorwin modified the milestone: V1 Oct 17, 2015
@ddorwin
Copy link
Contributor Author

ddorwin commented Oct 22, 2015

Also, define what an invalid time is. In issue #58, we decided on NaN. We also use NaN as the default value in algorithms.

@ddorwin
Copy link
Contributor Author

ddorwin commented Oct 26, 2015

Date is still in the Web IDL spec and the bug to remove it Web IDL bug 22824 has not been updated in two years. I requested an update there.

For future reference, we removed the use of Date from EME for bug 25902.

@ddorwin
Copy link
Contributor Author

ddorwin commented Oct 26, 2015

Note that https://heycam.github.io/webidl/#idl-Date already has the text about ECMAScript consistency.

If we do need a definition of time, perhaps the Common definitions section of the Web IDL spec would be an appropriate place.

@bzbarsky
Copy link

Yes, a type like DOMTimeStamp but explicitly consistent with ES Date timestamps would be a good idea. And then auditing existing DOMTimeStamp users....

@travisleithead
Copy link
Member

Looks like the DOM standard is also expecting a DOMTimeStamp to be defined (Event.timestamp). Right now it's a broken link :( Looks like issue 2 covers it.

@paulbrucecotton
Copy link

@travisleithead or @plehegar - Can you provide an update on the WebIDL issue 2 and recommend how we should proceed here? Do we wait for Issue-2 to be processed?

@mwatson2
Copy link
Contributor

mwatson2 commented Feb 9, 2016

In the absence of a response on the WebIDL status, I suggest we address this as follows:
(1) add the words "not including leap seconds" to our existing definition
(2) move this issue to V2 to track alignment with a future common timestamp definition in WebIDL

@paulbrucecotton
Copy link

Can @travisleithead and/or @plehegar please comment on @mwatson2's proposal?

@plehegar
Copy link
Member

plehegar commented Feb 9, 2016

DOMTimeStamp is part of https://www.w3.org/TR/WebIDL-1/#common-DOMTimeStamp
but it's using "unsigned long long" while the spec uses "double" (similar to DOMHighResTimeStamp). My suggestion would be to follow @mwatson2 recommendation for now. This can be revisited in the future once there is a better understanding of time.

@ddorwin
Copy link
Contributor Author

ddorwin commented Feb 17, 2016

Web IDL bug 22824 has been fixed, removing Date. Thus, we can be sure that we should not use that type.

Therefore, I propose we resolve this similar to my proposal on October 14th but address the last two bullets by borrowing the relevant text deleted from Web IDL.

I'm not sure we need to track this for VNext. If/when Web IDL gets a definition, we can choose to update the spec to refer to it.

@ddorwin ddorwin removed their assignment Feb 17, 2016
@jdsmith3000 jdsmith3000 self-assigned this Mar 14, 2016
@jdsmith3000
Copy link
Contributor

I intend to work this soon, implementing the plan proposed by David:

  • Create a common Time definition and link to it in expiration
  • Base the definition on the removed text for WebIDL Date
  • Move the current expiration text into the definition
  • Specify NaN use if no such time exists
  • State that it is intended to be consistent with UNIX time

jdsmith3000 added a commit to jdsmith3000/encrypted-media that referenced this issue Mar 29, 2016
@jdsmith3000 jdsmith3000 removed this from the V1 milestone Jul 19, 2016
@jdsmith3000 jdsmith3000 modified the milestones: V1Editorial, V1 Jul 19, 2016
@joeyparrish
Copy link
Member

There is some debate in Mozilla's bug tracker about the correct interpretation of this change. https://bugzilla.mozilla.org/show_bug.cgi?id=1324925

The current spec text says: "Time is intended to be consistent with Unix time". The definition of Unix Time on Wikipedia says: "Unix 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."

This would lead me to believe that session expirations were meant to be in seconds. Mozilla thought the same and changed the implementation to seconds. Chrome still reports milliseconds.

Is Chrome behind the spec? Or was Mozilla misled by spec text which needs to be clarified?

If the intent was to have the value in milliseconds, it might be helpful for the spec to state a unit explicitly. A double with "millisecond accuracy" could still conceivably be in units of seconds.

If there is a consensus that the intent was to use milliseconds, I would be happy to send a PR to make it explicit.

@ddorwin
Copy link
Contributor Author

ddorwin commented Jan 26, 2017

Yes, it is intended to be in milliseconds.

Not because that accuracy is important but because it is consistent with ECMAScript Date objects ("A Date object contains a Number indicating a particular instant in time to within a millisecond.") and it should be easy to create a Date object from values exposed by this API. Presumably, this should be true for all web APIs dealing with absolute times, though I'm not aware of such a definition (as we would have preferred). See also whatwg/webidl#2.

The first paragraph of the Time definition seems clear to me:

"millisecond accuracy... the same that can be represented with ECMAScript Date objects (ECMA-262, section 20.3 [ECMA-262]) – namely, every millisecond..."

The Unix time reference could be misleading and unnecessary. The linked reference in the original report has changed, so I don't know what was there before. I think this reference may have been related to the epoch and whether leap seconds are included.

Proposals to clarify this section are welcome. It may be sufficient to just reference https://tc39.github.io/ecma262/#sec-time-values-and-time-range, which appears to address all the related issues. We should also replace the unofficial and now redirected URL in the EME spec with this URL.

@bzbarsky
Copy link

The Unix time reference could be misleading and unnecessary.

Well, it just makes the spec self-conttradictory, because ES time and Unix time aren't consistent with each other.

It may be sufficient to just reference https://tc39.github.io/ecma262/#sec-time-values-and-time-range

If that's what you meant, that's what you should have done, yes.

@cpearce
Copy link

cpearce commented Jan 26, 2017

I'll update Firefox's implementation to report the expiration in milliseconds not seconds.

@ddorwin
Copy link
Contributor Author

ddorwin commented Jan 27, 2017

I opened #370 to track the issues above and others related to time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants