Fix window.requestIdleCallback not firing on iOS

[matt-oakes]

Summary

Fixes #28602

When creating a RCTFrameUpdate, ensure it is created with the correct unix timestamp in seconds. This is needed to match the NSTimeInterval type defined in the header.

Previously, it was using the CADisplayLink's timestamp property, which is not an NSTimeInterval but is instead a CFTimeInterval (note the different class prefix). This is the host time converted to seconds, which is the number of seconds since the device was turned on and therefore not a unix timestamp as expected.

This was causing issues with the window.requestIdleCallback timers as the timer code was expecting this timestamp property to be a unix timestamp in seconds which was causing the calculations to be done incorrectly and for the callbacks to never be called. The code does this calculation is here:

react-native/React/CoreModules/RCTTiming.mm

Line 262 in 4d920fe

NSTimeInterval frameElapsed = currentTimestamp - update.timestamp;

As one of these is a valid unix timestamp and the other is a much smaller number (number of seconds since device turn on), the if statement following this calculation never passes and the callbacks are never called.

This regression seems to have been introduced with this pull request: #26114

Changelog

[iOS] [Fixed] - Fixed window.requestIdleCallback not firing on iOS

Test Plan

I have tested this by patching my React Native code and testing that idle callbacks are correctly called. There is a reproduction case of the issue in the linked issue.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,292,190	+0
android	hermes	armeabi-v7a	7,625,378	+0
android	hermes	x86	8,764,609	+0
android	hermes	x86_64	8,701,169	+0
android	jsc	arm64-v8a	9,680,522	+0
android	jsc	armeabi-v7a	8,670,396	+0
android	jsc	x86	9,638,441	+0
android	jsc	x86_64	10,232,612	+0

Base commit: c68c47d
Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: c68c47d
Branch: main

[janicduplessis]

Wouldn't it be better to just revert #26114? I don't see the issue it tries to fix.

It previously used

NSTimeInterval frameElapsed = (CACurrentMediaTime() - update.timestamp);

to calculate the frame time difference using the system clock and:

NSTimeInterval currentTimestamp = [[NSDate date] timeIntervalSince1970];
NSNumber *absoluteFrameStartMS = @((currentTimestamp - frameElapsed) * 1000);

to convert it to unix time.

NSTimeInterval and CFTimeInterval are both double in seconds so they are compatible with each other.

cc @aleclarson

[aleclarson]

Reverting #26114 sounds good. I must have neglected due diligence on that one. 😥

[matt-oakes]

I couldn’t tell what the previous change was trying to fix and just assumed that this bug was an unintended side effect. If that’s not the case then I agree that reverting it is a better course of action.

[janicduplessis]

Yea if reverting works I think it is better since I’d rather keep using the timestamp on the CADisplayLink

[matt-oakes]

That makes sense. Are one of you ok to open the revert PR or do you need me to do it?

[janicduplessis]

If you can do it and test that it actually fixes the issue that would be great. I can do it too, but don't have a test case for idle callback.

[matt-oakes]

@janicduplessis I've tested that reverting that fixes my issue by patching my React Native files after install. I've updated this PR to remove the changes I previously made and instead revert the changes made in #26114. This should be ready for review and merging.

[janicduplessis]

Thanks! LGTM

[matt-oakes]

@shergin @cpojer Is there anything more needed to get this merged in?

[jenskuhrjorgensen]

Great job @matt-oakes 🎉

Hi @shergin @cpojer :)

Isn't this a pretty critical bug that should be prioritized asap? What is the status of getting this merged and released?

Best regards
Jens

[AlexanderKaran]

Is there any plan to merge this fix at all?

[matt-oakes]

@shergin @cpojer I’ve updated the branch and this is approved and ready to merge in 👍 Let me know if anything else is needed

[craftzdog]

Could you merge this?
window.requestIdleCallback always fires on timeout just like setTimeout on RN 0.66.3.

[jenskuhrjorgensen]

Hi @matt-oakes

Do you think this MR is being downprioritized due to the failing CI test? Would it be possible for you to fix it? I can't really see what you've changed that can cause the test to fail, so maybe a rerun would fix it (I cannot rerun it, maybe you can?).

[matt-oakes]

@jenskuhrjorgensen I'm not sure why it hasn't been looked at, but I've rerun te tests just to make sure it's not that. Like you said, the tests shouldn't fail as nothing has been changed to break that, but if t fails again I will take another look.

[janicduplessis]

cc @sammy-SC

[github-actions]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[jenskuhrjorgensen]

Hi @mdvacca

Sorry for dragging you in to this ancient PR, but I can see that you are very active on this repo, and maybe you would take the time to go through this fairly small PR that fixes a feature that hasn't worked for almost 3 years now and is still present in the newest version of RN.

Accordingly, we have a patch on RN that is also 3 years old now, containing the changes in this PR - and as with all other patches we would very much like to get rid of it.

I hope you can help out by approving this PR or at least give all the interested parties here an explanation of why this feature is not meant to work.

Best regards
Jens

[facebook-github-bot]

@sammy-SC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[matt-oakes]

Thanks for prompting @jenskuhrjorgensen and thanks for importing @sammy-SC 👍 I'd given up on this ever getting merged and just accepted I was always going to need to patch this.

[github-actions]

This pull request was successfully merged by @matt-oakes in 72abed2.

^{When will my fix make it into a release? | Upcoming Releases}

[sammy-SC]

thank you for the PR @matt-oakes!