Skip to content
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

fix(scheduler): getNow detection can randomly fail (fix #9632) #9647

Merged

Conversation

felixbuenemann
Copy link
Contributor

@felixbuenemann felixbuenemann commented Mar 7, 2019

The previous detection code compared time stamps based on Date.now()
which are not monotonic, so the check could fail due to clock skew or
adjustments.

This fix changes the check to compare against performance.now() if it is
supported, because it is monotonic (strictly increasing).

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

See the discussion in #9632 for additional info.

This needs to be backported to the 2.6 branch.

The previous detection code compared time stamps based on Date.now()
which are not monotonic, so the check could fail due to clock skew or
adjustments.

This fix changes the check to compare against performance.now() if it is
supported, because it is monotonic (strictly increasing).
@yyx990803
Copy link
Member

The check would lose the point if we compare the event timeStamp against a hi-res timeStamp. I think it should be good enough to only use performance.now when it's available since this only affects IE9.

@yyx990803 yyx990803 merged commit da77d6a into vuejs:dev Mar 8, 2019
@felixbuenemann
Copy link
Contributor Author

The check would lose the point if we compare the event timeStamp against a hi-res timeStamp.

No, the point of the check is to see if the event timeStamp uses hi-res (like performance.now()) or lo-res (like Date.now()). So comparing if the event timeStamp is smaller or equal to performance.now() will work, because if it is lo-res it will be a lot bigger than performance.now().

Comparing to performance.now() fixes the problem, because it is not affected clock inaccuracies.

I think it should be good enough to only use performance.now when it's available since this only affects IE9.

No, it also affects IE 10/11 and any other browser that uses Unix timestamps (lo-res) for events.

@yyx990803
Copy link
Member

You are right. Reviewing PRs with a jetlag is a bad idea.

kiku-jw pushed a commit to kiku-jw/vue that referenced this pull request Jun 18, 2019
Lostlover pushed a commit to Lostlover/vue that referenced this pull request Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants