Fix timestamp of touch events should use system startup time

[wangying3426]

Fixed flutter/flutter#95570

as binding.dart shown, when the frameTime is equal to currentSystemFrameTimeStamp, "[[NSProcessInfo processInfo] systemUptime]" should be used to replace "[[NSDate date] timeIntervalSince1970]".

Otherwise, the time_stamp of fake event will always larger than endTime as resampler.dart shown, and the fake event will be invalid, then the touch down event will never be released.

See UITouch timestamp for more information.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
  writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

This pull request was opened against a branch other than master. Since Flutter pull requests should not normally be opened against branches other than master, I have changed the base to master. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than master, unless this is an intentional hotfix/cherrypick.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[wangying3426]

It seems should be covered by existing tests? cc @Hixie for review, thanks.

[Hixie]

If it's covered by existing tests, what existing test failed before that this is fixing?

[wangying3426]

If it's covered by existing tests, what existing test failed before that this is fixing?

I mean it’s a bugfix clearly and seems hard to write tests, maybe it's ok to pass the existing tests only? Could you have better suggestions？

[Hixie]

Ah, I see. It being hard to write tests isn't a reason to not write tests. :-) Usually the trick is to find a way to refactor the code to be easier to test. For example, can we make it possible to inject the pointer events before this code, and see how it looks once it gets to the Dart side?

cc @gaaclarke who may have opinions, looks like they've looked at this code before. @dkwingsmt and @gspencergoog may also have opinions?

[dkwingsmt]

@wangying3426 The reason to enforce tests for changes is to prevent future changes to accidentally revert your fix. Just like how you feel safe not breaking any existing tests, future contributors might think they are safe for not breaking any tests, while actually breaking yours because you didn't write any.

[dkwingsmt]

Although I'm not familiar with this part, I wonder:

• If the entire purpose is to make the timestamp lower, can we just use 0, or use "the time of the most recent real event"?
• Should we also fix the same timestamp scheme in flushOngoingTouches?

As for tests, is it possible to simulate normal events, then the fake events, and test if the fake events have a lower time than the normal events (instead of testing their absolute values)?

[wangying3426]

Although I'm not familiar with this part, I wonder:

• If the entire purpose is to make the timestamp lower, can we just use 0, or use "the time of the most recent real event"?
• Should we also fix the same timestamp scheme in flushOngoingTouches?

As for tests, is it possible to simulate normal events, then the fake events, and test if the fake events have a lower time than the normal events (instead of testing their absolute values)?

• If the entire purpose is to make the timestamp lower, can we just use 0, or use "the time of the most recent real event"?
  • Indeed, the purpose of using systemUptime is to fake "the time of the most recent real event", all real events will use systemUptime as UITouch. But 0 or [[NSDate date] timeIntervalSince1970] does not conform to "the most recent real event", and will lead to abnormal behavior sometimes.
• Should we also fix the same timestamp scheme in flushOngoingTouches?
  • Yes, i have also fixed it in flushOngoingTouches.

Besides，I find that the stable channel also uses systemUptime in generatePointerDataForMouse.

[wangying3426]

flushOngoingTouches code was submitted by xster, also cc @xster for review? thanks.

[chinmaygarde]

The implication here is that an issue is caused by comparing the scheduler timebase with the touch timebase. I can totally believe such issues to occur is code is assuming identical timebases from disparate subsystems. This guarantee was never present however and this patch seems is trying to match timebases. I agree with the insistence on the test. The fix is not to make this assumption on all subsystems using identical timebases. As written, I am not sure this fixes the issue on all platform versions.

[wangying3426]

Hi，@chinmaygarde @dkwingsmt , I searched all time_stamp fields on iOS, found that except for these two events and some tests, all the other time_stamp depends on systemUptime, as dispatchTouches and generatePointerDataForMouse shown, maybe we just need to ensure them using identical timebases? And could you have any other suggestions for how to write those test cases?

[wangying3426]

hello, @Hixie @chinmaygarde @dkwingsmt To further confirm the problem, some additional informations were added, Could you please review whether these informations are sufficient?

1. What problems will inconsistent ​timebases cause?
   If there are on going events when FlutterViewController disappear, native will send fake cancel events to flush the on going events. When resamplingEnabled is set to true, a periodic timer will started until the fake event's timestamp is smaller than sampling time. While in the original code the fake event's timestamp is so much bigger than sampling time (one depends on systemUpTime, while another timeIntervalSince1970), the timer will looping execution for a very very long time.

2. Added an associated PR: Fix timer keeps active when resampling disabled in some cases flutter#97197.

3. Here is a code sample that can reproduce the problem. Follow the steps below:

• Run the demo on iOS real device.
• One finger holds down the flutter list，at the same time another finger clicks the go to native button to push a new viewController. At this moment, flushOngoingTouches will be executed and a native fake event will be sent to flush the ongoing down event.
• Then, we'll find that ScrollUpdateNotification will be sent continuously even though the FlutterViewController is in the paused state, as the console shown:

flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
flutter: AppLifecycleState.paused scrolling.....
......

I also try to verify the timestamp of UITouch on scenario_app, but apple developer documentation has explain it clearly and we can also debug source code to watch this value.

[chinmaygarde]

cc @dreveman I see you as the author of the resampler. I think I understand the issue @wangying3426 is running into but cannot determine if this is the right fix.

[wangying3426]

cc @dreveman I see you as the author of the resampler. I think I understand the issue @wangying3426 is running into but cannot determine if this is the right fix.

It's been a while, @dreveman Could you review this PR, please?

[dreveman]

I'm not familiar with ios code changed in this PR but the resampler code rely on event timestamps with the same clock as frame presentation timestamps. If this is the correct way to accomplish that on ios and it's not breaking some other assumptions then it's likely the correct fix.

[dkwingsmt]

Fine...

@wangying3426 Do you think you can write a test, either in the way I described or any other proper ways?

[wangying3426]

Fine...

@wangying3426 Do you think you can write a test, either in the way I described or any other proper ways?

ok. @dkwingsmt @christopherfujino could we use this demo to verify whether it is the correct change? As mentioned earlier, if not fix, ScrollUpdateNotification will be sent continuously even though the FlutterViewController is in the paused state, while the behavior is correct if fixed. Maybe we can also write many code to mock a scene like the demo, and included it in https://github.com/flutter/engine/tree/main/testing/scenario_app or others, but it seems overly complicated, we don't need to constantly test this fix.

If just validated the behavior of this change, do you think
https://github.com/wangying3426/engine/pull/1/files is a right way to meet the test requirements? I can merge it if you think it's ok.

I personally feel that this issue has been discussed quite clearly, and adding explanation comments maybe a proper ways.

[christopherfujino]

If just validated the behavior of this change, do you think https://github.com/wangying3426/engine/pull/1/files is a right way to meet the test requirements? I can merge it if you think it's ok.

This looks reasonable

[dkwingsmt]

LGTM except for that one change.

[dkwingsmt]

@gaaclarke @chinmaygarde Can you take a look and give a second review?

[ColdPaleLight]

LGTM with nit

[fluttergithubbot]

This pull request is not suitable for automatic merging in its current state.

• The status or check suite Mac clang-tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.