1158399 - "Assertion failure: TimeClip(date) == date" with invalid date from File.lastModifiedDate

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: TimeClip(date) == date, at js/src/jsdate.cpp:2545

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

So in this case TimeClip(date) comes back NaN.  date is 18446744073709552 which is presumably out of whatever range Date thinks it can represent.

I checked, and Gecko is calling JS_NewDateObjectMs with that value.  Presumably that should be checking validity of the value...

That said, why is Gecko doing that?  Because it's storing modification dates unsigned for some reason.  Filed bug 1158437 on that, but we should fix JS_NewDateObjectMs anyway.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Ensure/assert that DateObject::setUTCTime never stores a non-TimeClip'd value

So this is slightly big, but by throwing the C++ type system at this, we get actual guarantees (modulo absence of casting, unions, etc.) that the only thing going into this reserved slot is a clipped time.  By the end, it actually feels quite pleasant to me.  (Then again, I'm comparing against the patch state a few hours ago when I hadn't introduced a type, and it was all based on spot-checking all users for correctness.)

There seems a good argument to me that JS_NewDateObjectMsec should take a ClippedTime and people should only provide those, never doubles.  Also JS::MakeDate could return a ClippedTime and not a double.  But 1) keep patch size down, and 2) I didn't get feedback from bz indicating those changes were good or bad just yet.

This builds on top of bug 1160356 patchwork.

At the far peripheries this patch might fix some NaN/overflow issues in places, but frankly I'm not sure it's worth the effort to write tests for those exact boundary conditions.  I guess I can if you insist on it, but the obvious correctness of this seems Good Enough to me.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0055add82982

Comment 5

[Jeff Walden [:Waldo]]

I want to propagate ClippedTime a little further out into APIs, leaving open for that second part.  I have it "finished", but it touches a bunch of stuff in Gecko, and it'll probably need review from a bunch of different people to confirm the semantics are exactly as desired.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0055add82982

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Propagate JS::ClippedTime and JS::TimeClip into APIs more

I'm only looking for review of the js/ parts of this right now, but feel free to skim the rest if you want.  (If you do, I have various notes I could dump at you, but I plan to direct most of them at the people doing component-specific reviews.)

I considered adding JS::Value::setTime(ClippedTime), but 1) it's somewhat esoteric, and 2) the dependency cycle issues look pretty bad.

Arguably js::DateGetMsecSinceEpoch should be in js/public/Date.h, returning a ClippedTime.  I couldn't think of a good name for it, so I left it as-is.  If you have name ideas, I'd gladly add something.

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8641250 [details] [diff] [review]
Propagate JS::ClippedTime and JS::TimeClip into APIs more

Review of attachment 8641250 [details] [diff] [review]:
-----------------------------------------------------------------

Nice improvement. Seems to me that the most callers of DateGetMsecSinceEpoch actually want a double, but maybe that can be fixed. Would it help to change MakeDate to return a ClippedTime?

::: js/public/Date.h
@@ +98,5 @@
>  
> +// Produce a double Value from the given time.  Because times may be NaN,
> +// prefer using this to manual canonicalization.
> +inline Value
> +TimeValue(ClippedTime time)

Can't we just assert that the NaN is canonical? There is only the default constructor and TimeClip. This means we don't need this method and can use value() again. We might have to use GenericNaN above?

::: js/src/vm/SelfHosting.cpp
@@ +1745,5 @@
>          RootedAtom source(cx, reobj.getSource());
>          MOZ_ASSERT(source->isPermanentAtom());
>          clone = RegExpObject::createNoStatics(cx, source, reobj.getFlags(), nullptr, cx->tempLifoAlloc());
>      } else if (selfHostedObject->is<DateObject>()) {
> +        clone = JS::NewDateObject(cx, selfHostedObject->as<DateObject>().clippedTime());

This looks so nice :)

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Tom Schuster [:evilpie] from comment #8)
> Seems to me that the most callers of DateGetMsecSinceEpoch
> actually want a double, but maybe that can be fixed.

It could...but it's no skin off their noses to throw in a toDouble(), and for callers that want to have a known-clipped time (for example a few of the DOM APIs in this patch, as I recall), this communicates it to them.  If the external interface exposes *only* ClippedTime, that seems simplest.

> Would it help to change MakeDate to return a ClippedTime?

The MakeDate algorithm in the spec very specifically *doesn't* return a ClippedTime, so this is consistent with that.  Note the current code incorporates clipping, which likely led to some of the confusion here about when to clip and when not.

> > +// Produce a double Value from the given time.  Because times may be NaN,
> > +// prefer using this to manual canonicalization.
> > +inline Value
> > +TimeValue(ClippedTime time)
> 
> Can't we just assert that the NaN is canonical? There is only the default
> constructor and TimeClip. This means we don't need this method and can use
> value() again. We might have to use GenericNaN above?

We could use GenericNaN in TimeClip.  I'm somewhat hesitant to do so, however, because it makes this one particular place more convenient, at cost of making this one more place people can let down their guard, "safely".  When everything requires care, people are more likely to take it.  When some things don't and some things do, people will turn lazy.

Comment 10

[Tom S [:evilpie]]

Comment on attachment 8641250 [details] [diff] [review]
Propagate JS::ClippedTime and JS::TimeClip into APIs more

Review of attachment 8641250 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I necessarily agree, but TimeValue is a nice API.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8641250 [details] [diff] [review]
Propagate JS::ClippedTime and JS::TimeClip into APIs more

Okay, let's parcel out the rest of the reviewing here -- each reviewer to specific changes noted below.  For context, I recommend reading what's happening in js/public/Date.h -- and if anything's unclear in the documentation I'm adding there, comment to say what you'd like to see explained better.

Feel free to skim other bits you think are relevant to your interests if you want, too -- I just picked people, possibly not always the ideal ones.

===============

bz: dom/bindings/Date.{cpp,h}, dom/html/HTMLInputElement.cpp

WebIDL bindings' Date.h has more inlining in it than it did before, but no more than is required to store JS::ClippedTime internally.  So why not?

Date::TimeStamp() returns a ClippedTime.  It seems best to me to force users to manually convert to double, because then they'll hopefully have to consider the invalid-date possibility themselves.

An IsUndefined() just above it guards the last HTMLInputElement.cpp change; the rest are simple.

===============

baku: dom/base/File.cpp, dom/filehandle/MetadataHelper.cpp

(File.cpp)  If we have a last-modified date that's out of range that we must expose as a Date object, there's literally nothing we can do but expose as an invalid Date.  Right?

(MetadataHelper.cpp)  As with File.cpp, it seems we have no choice but to return an invalid Date if the last-modified date is out of range.  Or should something else be done instead, if this at all matters?

===============

khuey: dom/indexedDB/IDBObjectStore.cpp, dom/indexedDB/Key.cpp

(IDBObjectStore.cpp)  This is the trickiest issue in the entire patch.  aData.lastModifiedDate can be INT64_MAX, for the case of files that don't have a last-modified time.  (I understand such file items are only in old databases, from the last time I asked.)  This change will convert it to the (safe) value of NaN, exposing a Date object containing a NaN value.  This is better than the current wrong behavior of possibly asserting, but is it the *right* standards-compliant behavior?  What even makes sense here, if this doesn't make sense?

(Key.cpp)  Key::DecodeJSValInternal pairs with Key::EncodeJSValInternal.  That method only encodes a Date object if it contained a valid date/time.  So this method can use TimeClip and assert it won't change anything.

===============

dhylands: dom/devicestorage/nsDeviceStorage.cpp

The toDouble() there is safe because the IsUndefined() above it guards it.

===============

smaug: dom/time/TimeManager.cpp

TimeManager::Set(double aTime), to which is passed a TimeStamp().toDouble() that might be NaN, contains no check for a valid time being requested.  This is a pre-existing problem; I have no idea what such validity testing might look like.  Tell me the test to do and I'll add it, or I can file the bug to fix this and let someone else do it separately.

===============

mt: dom/media/webrtc/RTCCertificate.h

This value is limited (in RTCCertificate.cpp) to be in the range 0-30 days from the epoch, or 0-30 days after today.  These are basically real-world times that won't clip.


===============

froydnj: toolkit/components/startup/nsAppStartup.cpp

These are real-world times that aren't going to clip, right?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8641250 [details] [diff] [review]
Propagate JS::ClippedTime and JS::TimeClip into APIs more

r=me on the dom/bindings/Date.{cpp,h}, dom/html/HTMLInputElement.cpp changes.

Comment 13

[Dave Hylands [:dhylands]]

That was r=me on nsDeviceStorage.cpp change

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8641250 [details] [diff] [review]
Propagate JS::ClippedTime and JS::TimeClip into APIs more

I'm not very happy to push more JS API usage to Gecko, though in TimeManager case it is just .toDouble() call.

Could we please add ToDouble() to 'class Date' so that code outside js and bindings could more often live without any JSAPI usage.
With that, and TimeManager just using aDate.ToDouble(), r+.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8641250 [details] [diff] [review]
Propagate JS::ClippedTime and JS::TimeClip into APIs more

Review of attachment 8641250 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the nsAppStartup.cpp change.  These should be times measured from the startup of the browser, so if we're clipping...somebody needs to update and/or submit records to the Guinness Book of World Records.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8641250 [details] [diff] [review]
Propagate JS::ClippedTime and JS::TimeClip into APIs more

Review of attachment 8641250 [details] [diff] [review]:
-----------------------------------------------------------------

NaN is fine, IMO.  This only applies to databases that have files that were stored before we implemented lastModifiedDate which was a long time ago, so I'm not that worried about standards.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/404e5944dc59

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/404e5944dc59