File constructor understands lastModified.

File constructor understands lastModified.

This re-lands https://codereview.chromium.org/74213009/ with a
modification to the File constructor layout tests that should make it
not flake on XP.

BUG=164933
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163221

[pwnall-personal, 2013-11-21 22:40:45]

I would like to re-land the CL that brings the File constructor in line with the
spec.

The CL was reverted because it caused test flakiness on XP. The source of
flakiness was comparing lastModified against the current time. I made that
comparison significantly more robust.

Please take a look?

[kinuko, 2013-11-22 06:59:25]

Would you be able to upload the original patch first, and then the new one, so
that reviewers can easily see how you fixed it?

Also I think js-test.js change should be moved into a separate patch, that's not
directly related to File constructor.

[pwnall-personal, 2013-11-22 13:19:33]

On 2013/11/22 06:59:25, kinuko wrote:
> Would you be able to upload the original patch first, and then the new one, so
> that reviewers can easily see how you fixed it?

Done.
Sorry, I haven't had a CL reverted before, so I didn't know how it's done :)

> Also I think js-test.js change should be moved into a separate patch, that's
not
> directly related to File constructor.

My initial plan was to have shouldBeNow() in a separate CL, but I'm having a
hard time getting that reviewed on its own.
https://codereview.chromium.org/69813015/

Can you please advise me on how to proceed?

[kinuko, 2013-11-25 05:08:36]

Thanks for re-uploading the patch.

On 2013/11/22 13:19:33, pwnall wrote:
> > Also I think js-test.js change should be moved into a separate patch, that's
> not
> > directly related to File constructor.
> 
> My initial plan was to have shouldBeNow() in a separate CL, but I'm having a
> hard time getting that reviewed on its own.
> https://codereview.chromium.org/69813015/
> 
> Can you please advise me on how to proceed?

As for the shouldBeNow() part, currently it's only used in this test, I think
we'd better have this in fast/files.
I also honestly feel that the current code is a bit overkill for adding one
test.  Can we test this simple feature with a simple tests?
We can do more generalization and move it under common place when we find we
need it (YAGNI).

As for this patch I have one questions: did you find out what was the root cause
of the flakiness on XP?
You say 'you made it robust' but to me it's not very clear what part was causing
the flakiness.

[pwnall-personal, 2013-11-25 07:50:29]

On 2013/11/25 05:08:36, kinuko wrote:

*snip*

> As for the shouldBeNow() part, currently it's only used in this test, I think
> we'd better have this in fast/files.
> I also honestly feel that the current code is a bit overkill for adding one
> test.  Can we test this simple feature with a simple tests?
> We can do more generalization and move it under common place when we find we
> need it (YAGNI).

I think that comparing against Date.now() is bound to be tricky :(

We could choose not to test those cases. I could remove the current checks and
just check that a Date instance is returned. The second/millisecond conversions
that need to happen make me a bit uneasy.

Alternatively, we could land the current tests and I could take on a bug to
simplify them later, if they doesn't cause flakiness?

> As for this patch I have one questions: did you find out what was the root
cause
> of the flakiness on XP?
> You say 'you made it robust' but to me it's not very clear what part was
causing
> the flakiness.

This is what the flakiness dashboard shows.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

The failures are in the comparison against Date.now().
https://codereview.chromium.org/74213009/patch/1080001/1090004

The tests are flaky and not failing outright, so the comparisons will succeed if
they are tried a few times in a row. This causes me to think that the loop in
shouldBeNow() will fix the flakiness.

How would you like me to proceed?

[jsbell, 2013-11-25 19:56:23]

Another drive-by, sorry.

On 2013/11/25 07:50:29, pwnall wrote:
> This is what the flakiness dashboard shows.
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...

The failures have expired off the dashboard so it's hard to tell "how flaky" it
was.

I took a peek at v8/src/time.cc and on Win32 specifically (where the flakiness
was?)
the time calculation for Date.now() is... nontrivial, compared to what
currentTimeMS() is yielding. Ugh. Without looking too closely, my guess (hope?)
is 
that you're comparing two slightly different clocks rather than seeing actual
NTP
resets. If that's the case, shouldBeNow() would only be a general purpose
function
if we're frequently comparing different clocks, and I don't think we are.

> The tests are flaky and not failing outright, so the comparisons will succeed
if
> they are tried a few times in a row. This causes me to think that the loop in
> shouldBeNow() will fix the flakiness.

One option: remove the loop entirely and land the test with (1) more logging so
you can see the actual before/lastModified/after times and verify that the
failure
is as expected, and (2) land with a [Pass Failure] expectation. Watch after
landing
and consider if the loop is better, or if just an approximate comparison is good
enough (e.g. within 1 second) for this case.

[pwnall-personal, 2013-11-26 02:04:34]

kinuko: Seems like ojan solved the complexity by accepting the my CL that
introduces shouldBeNow(). This CL doesn't introduce complex testing log any more
:D

jsbell: Thank you very much for pointing that out!! I did not think to look
there. I'll read up on the code, I'm really curious why Date.now is implemented
differently on Windows.

[pwnall-personal, 2013-11-26 02:05:29]

On 2013/11/26 02:04:34, pwnall wrote:
> kinuko: Seems like ojan solved the complexity by accepting the my CL that
> introduces shouldBeNow(). This CL doesn't introduce complex testing log any
more
> :D

I meant "complex testing logic". Sorry for spamming your inbox!

[kinuko, 2013-11-26 04:50:12]

On 2013/11/26 02:05:29, pwnall wrote:
> On 2013/11/26 02:04:34, pwnall wrote:
> > kinuko: Seems like ojan solved the complexity by accepting the my CL that
> > introduces shouldBeNow(). This CL doesn't introduce complex testing log any
> more
> > :D
> 
> I meant "complex testing logic". Sorry for spamming your inbox!

I see thanks for the update.

For shouldBeNow part I think per Josh's investigation (thanks very much, I was
going to run the test on my win box) it sounds allowing some jitter would
probably make more sense if we're really comparing different clocks. Wdyt?

I'll also take a look at this patch itself.

[kinuko, 2013-11-26 05:02:00]

On 2013/11/26 04:50:12, kinuko wrote:
> On 2013/11/26 02:05:29, pwnall wrote:
> > On 2013/11/26 02:04:34, pwnall wrote:
> > > kinuko: Seems like ojan solved the complexity by accepting the my CL that
> > > introduces shouldBeNow(). This CL doesn't introduce complex testing log
any
> > more
> > > :D
> > 
> > I meant "complex testing logic". Sorry for spamming your inbox!
> 
> I see thanks for the update.
> 
> For shouldBeNow part I think per Josh's investigation (thanks very much, I was
> going to run the test on my win box) it sounds allowing some jitter would
> probably make more sense if we're really comparing different clocks. Wdyt?
> 
> I'll also take a look at this patch itself.

lgtm for this patch.  Please consider polishing shouldBeNow, or at least I think
the clock skew thing should be documented somewhere so that other people don't
need to stumble upon the same problem.

[pwnall-personal, 2013-11-27 03:17:27]

On 2013/11/26 05:02:00, kinuko wrote:
> On 2013/11/26 04:50:12, kinuko wrote:
> > On 2013/11/26 02:05:29, pwnall wrote:
> > > On 2013/11/26 02:04:34, pwnall wrote:
> > > > kinuko: Seems like ojan solved the complexity by accepting the my CL
that
> > > > introduces shouldBeNow(). This CL doesn't introduce complex testing log
> any
> > > more
> > > > :D
> > > 
> > > I meant "complex testing logic". Sorry for spamming your inbox!
> > 
> > I see thanks for the update.
> > 
> > For shouldBeNow part I think per Josh's investigation (thanks very much, I
was
> > going to run the test on my win box) it sounds allowing some jitter would
> > probably make more sense if we're really comparing different clocks. Wdyt?
> > 
> > I'll also take a look at this patch itself.
> 
> lgtm for this patch.  Please consider polishing shouldBeNow, or at least I
think
> the clock skew thing should be documented somewhere so that other people don't
> need to stumble upon the same problem.

file-constructor.html failed on the Windows buildbots, so I didn't submit the
patch to CQ.

I looked at how Date.now() works and compared with currentTimeMS(). This was
quite a curveball, I did not expect different notions of time in Blink and v8.

I would like to look into using v8's notion of time in the Blob and File custom
bindings.


For posterity, here's what I found and the relevant bits of code. I'm really
grateful to jsbell for leading me to this information.

On Windows, both Chromium/Blink and v8 combine a low-resolution system clock
with a high resolution timer, and periodically update the baseline. This means
that on Windows it is entirely possible for time to go backwards by small
amounts quite often. 

Furthermore, Chromium and v8 have separate implementations of this concept which
resync independently, so the two clocks can go out of sync.

Date.now() traces to v8::internal::Time::Now, which uses
v8::internal::Clock::Now on Windows.
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/platform/ti...

currentTimeMS() traces to base::Time::Now in Chromium, going through Blink's
platform layer.
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time_win...

[kinuko, 2013-11-27 04:03:22]

Thanks for investigation.

1. How much time diff could they yield?
2. If they're small enough I feel allowing jitter is probably fine at least
in our usage
3. Regardless of whether it's acceptable diff or not (and if we can work
around here or not) I think it's probably worth posting a note/question on
blink-dev@ about the clock difference on Windows, so that everyone can be
aware of the potential problems. and also someone may have a better
solution (especially I'd like to see the possibility to use the same timer
impl in V8 and Chromium).



On Wed, Nov 27, 2013 at 12:17 PM, <costan@gmail.com> wrote:

> On 2013/11/26 05:02:00, kinuko wrote:
>
>> On 2013/11/26 04:50:12, kinuko wrote:
>> > On 2013/11/26 02:05:29, pwnall wrote:
>> > > On 2013/11/26 02:04:34, pwnall wrote:
>> > > > kinuko: Seems like ojan solved the complexity by accepting the my CL
>>
> that
>
>> > > > introduces shouldBeNow(). This CL doesn't introduce complex testing
>> log
>> any
>> > > more
>> > > > :D
>> > >
>> > > I meant "complex testing logic". Sorry for spamming your inbox!
>> >
>> > I see thanks for the update.
>> >
>> > For shouldBeNow part I think per Josh's investigation (thanks very
>> much, I
>>
> was
>
>> > going to run the test on my win box) it sounds allowing some jitter
>> would
>> > probably make more sense if we're really comparing different clocks.
>> Wdyt?
>> >
>> > I'll also take a look at this patch itself.
>>
>
>  lgtm for this patch.  Please consider polishing shouldBeNow, or at least I
>>
> think
>
>> the clock skew thing should be documented somewhere so that other people
>> don't
>> need to stumble upon the same problem.
>>
>
> file-constructor.html failed on the Windows buildbots, so I didn't submit
> the
> patch to CQ.
>
> I looked at how Date.now() works and compared with currentTimeMS(). This
> was
> quite a curveball, I did not expect different notions of time in Blink and
> v8.
>
> I would like to look into using v8's notion of time in the Blob and File
> custom
> bindings.
>
>
> For posterity, here's what I found and the relevant bits of code. I'm
> really
> grateful to jsbell for leading me to this information.
>
> On Windows, both Chromium/Blink and v8 combine a low-resolution system
> clock
> with a high resolution timer, and periodically update the baseline. This
> means
> that on Windows it is entirely possible for time to go backwards by small
> amounts quite often.
>
> Furthermore, Chromium and v8 have separate implementations of this concept
> which
> resync independently, so the two clocks can go out of sync.
>
> Date.now() traces to v8::internal::Time::Now, which uses
> v8::internal::Clock::Now on Windows.
> https://code.google.com/p/chromium/codesearch#chromium/
> src/v8/src/platform/time.cc&l=173
>
> currentTimeMS() traces to base::Time::Now in Chromium, going through
> Blink's
> platform layer.
> https://code.google.com/p/chromium/codesearch#chromium/
> src/base/time/time_win.cc&l=105
>
> https://codereview.chromium.org/80983002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pwnall-personal, 2013-11-28 03:13:30]

On 2013/11/27 04:03:22, kinuko wrote:
> 1. How much time diff could they yield?

In my Windows 7 VM, the diff went up to 2 seconds. Also, Date.now() can jump
back by 200-1000ms, and will always jump back if it is observed for 5-10
minutes.

Details on my experiments are here:
https://code.google.com/p/chromium/issues/detail?id=324110#c3

> 2. If they're small enough I feel allowing jitter is probably fine at least
> in our usage

I updated shouldBeNow() to take in a tolerance, and I updated the test here to
use 10 seconds as the tolerance value. I think this is acceptable, because what
I'm really worried about is forgetting to multiply or divide the timestamp by
1000, so a 10-second slop still allows the test to work the right way.

I resubmitted this change to the trybots, and it worked on Windows. win-blink
and win_layout failed, but fast/files/file-constructor.html is not in the list
of failures, and all failures appear unrelated.

Based on the above, I'd like to land this and see how it fares. I've also
started the work on getting V8 to use Chromium's timer, but that will require
CLs on both sides, so I estimate that it'll take 2-3 weeks, if everything goes
incredibly well. I don't think this CL needs to wait for that long.

What do you think?

[kinuko, 2013-11-28 04:46:38]

On 2013/11/28 03:13:30, pwnall wrote:
> On 2013/11/27 04:03:22, kinuko wrote:
> > 1. How much time diff could they yield?
> 
> In my Windows 7 VM, the diff went up to 2 seconds. Also, Date.now() can jump
> back by 200-1000ms, and will always jump back if it is observed for 5-10
> minutes.

Oh, that's bigger than I thought... thanks for measuing it.

> Details on my experiments are here:
> https://code.google.com/p/chromium/issues/detail?id=324110#c3
> 
> > 2. If they're small enough I feel allowing jitter is probably fine at least
> > in our usage
> 
> I updated shouldBeNow() to take in a tolerance, and I updated the test here to
> use 10 seconds as the tolerance value. I think this is acceptable, because
what
> I'm really worried about is forgetting to multiply or divide the timestamp by
> 1000, so a 10-second slop still allows the test to work the right way.
> 
> I resubmitted this change to the trybots, and it worked on Windows. win-blink
> and win_layout failed, but fast/files/file-constructor.html is not in the list
> of failures, and all failures appear unrelated.
> 
> Based on the above, I'd like to land this and see how it fares. I've also
> started the work on getting V8 to use Chromium's timer, but that will require
> CLs on both sides, so I estimate that it'll take 2-3 weeks, if everything goes
> incredibly well. I don't think this CL needs to wait for that long.
> 
> What do you think?

Sounds good to me.  Can you add a FIXME line with the issue link
(crbug.com/324110) around delta handling in shouldBeNow so that we can fix it
later / readers can find the info?  Then I think we can land this for our use.

[pwnall-personal, 2013-11-28 16:46:28]

On 2013/11/28 04:46:38, kinuko wrote:
> Sounds good to me.  Can you add a FIXME line with the issue link
> (crbug.com/324110) around delta handling in shouldBeNow so that we can fix it
> later / readers can find the info?  Then I think we can land this for our use.

Done.

Chris / haraken: Can I get an LGTM for bindings, please?
The changes are the same as in https://codereview.chromium.org/74213009/

Thank you!

[jsbell, 2013-12-02 18:11:02]

On 2013/11/27 03:17:27, pwnall wrote:
> I looked at how Date.now() works and compared with currentTimeMS(). This was
> quite a curveball, I did not expect different notions of time in Blink and v8.
...
> For posterity, here's what I found and the relevant bits of code. I'm really
> grateful to jsbell for leading me to this information.

Thank *YOU* for diligently pursuing this!

Hopefully once everything script-facing is using the same clock then
shouldBeNow() can be simplified to just a before <= eval <= after check again.

[pwnall-personal, 2013-12-04 06:21:48]

On 2013/12/02 18:11:02, jsbell wrote:
> On 2013/11/27 03:17:27, pwnall wrote:
> > I looked at how Date.now() works and compared with currentTimeMS(). This was
> > quite a curveball, I did not expect different notions of time in Blink and
v8.
> ...
> > For posterity, here's what I found and the relevant bits of code. I'm really
> > grateful to jsbell for leading me to this information.
> 
> Thank *YOU* for diligently pursuing this!
> 
> Hopefully once everything script-facing is using the same clock then
> shouldBeNow() can be simplified to just a before <= eval <= after check again.

jsbell: Happy to help, it's been an experience, and it makes for quite a story!

haraken: Can you please take a look at the bindings?
The changes are the same as in https://codereview.chromium.org/74213009/

[kinuko, 2013-12-05 03:05:54]

On 2013/12/04 06:21:48, pwnall wrote:
> On 2013/12/02 18:11:02, jsbell wrote:
> > On 2013/11/27 03:17:27, pwnall wrote:
> > > I looked at how Date.now() works and compared with currentTimeMS(). This
was
> > > quite a curveball, I did not expect different notions of time in Blink and
> v8.
> > ...
> > > For posterity, here's what I found and the relevant bits of code. I'm
really
> > > grateful to jsbell for leading me to this information.
> > 
> > Thank *YOU* for diligently pursuing this!
> > 
> > Hopefully once everything script-facing is using the same clock then
> > shouldBeNow() can be simplified to just a before <= eval <= after check
again.
> 
> jsbell: Happy to help, it's been an experience, and it makes for quite a
story!
> 
> haraken: Can you please take a look at the bindings?
> The changes are the same as in https://codereview.chromium.org/74213009/

If the binding code hasn't been changed since the previous one that was LGTM'ed
I think it's probably ok to submit this with TBR (maybe after sending one more
note to make sure).

[haraken, 2013-12-05 03:14:09]

Sorry for being late. LGTM for bindings/.

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right):

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:58: double lastModified() const
{ ASSERT(m_hasFileProperties); ASSERT(m_hasLastModified); return m_lastModified;
}

Nit: Write one sentence in one line.

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:98: v8SetReturnValue(info,
v8::Date::New(lastModified));

You're calling v8::Date::New() here...

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:107: v8SetReturnValue(info,
floor(lastModified));

but not calling here. Is this intentional?

[pwnall-personal, 2013-12-05 04:24:53]

Thank you for your feedback, haraken! Sorry to pester you with this CL!

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8BlobCustomHelpers.h (right):

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8BlobCustomHelpers.h:58: double lastModified() const
{ ASSERT(m_hasFileProperties); ASSERT(m_hasLastModified); return m_lastModified;
}
On 2013/12/05 03:14:10, haraken wrote:
> 
> Nit: Write one sentence in one line.

I expanded this to multiple lines. Thank you!

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8FileCustom.cpp (right):

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:98: v8SetReturnValue(info,
v8::Date::New(lastModified));
On 2013/12/05 03:14:10, haraken wrote:
> 
> You're calling v8::Date::New() here...

Yup. This is "lastModifiedDate" from the current / old File API spec.

I added a comment to explain the mismatch.

https://codereview.chromium.org/80983002/diff/460001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8FileCustom.cpp:107: v8SetReturnValue(info,
floor(lastModified));
On 2013/12/05 03:14:10, haraken wrote:
> 
> but not calling here. Is this intentional?

Yes. In the last File API ED, "lastModified" replaces "lastModifiedDate" and
returns a number, to avoid the confusion caused by "file.lastModifiedDate !=
file.lastModifiedDate".

I think we'll deprecate and remove the old property in due course. I added a
UseCounter on it in https://codereview.chromium.org/105763002/

For now, I added a comment so readers will know right away Date::New isn't
called on purpose.

[haraken, 2013-12-05 04:26:06]

> Yes. In the last File API ED, "lastModified" replaces "lastModifiedDate" and
> returns a number, to avoid the confusion caused by "file.lastModifiedDate !=
> file.lastModifiedDate".
> 
> I think we'll deprecate and remove the old property in due course. I added a
> UseCounter on it in https://codereview.chromium.org/105763002/
> 
> For now, I added a comment so readers will know right away Date::New isn't
> called on purpose.

Thanks for the clarification. Makes sense.

[pwnall-personal, 2013-12-05 04:27:31]

On 2013/12/05 03:05:54, kinuko wrote:
> If the binding code hasn't been changed since the previous one that was
LGTM'ed
> I think it's probably ok to submit this with TBR (maybe after sending one more
> note to make sure).

Thank you for explaining, kinuko!

Can a lowly contributor use TBR, or do I need to wait to get my committer star
first? :)

[commit-bot: I haz the power, 2013-12-05 06:47:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/costan@gmail.com/80983002/540001

[commit-bot: I haz the power, 2013-12-05 08:08:31]

Change committed as 163221