From 36dc9da9a44040c5d639238e7e84a9447c8c70ed Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 23 Apr 2019 16:03:58 -0700 Subject: [PATCH] Measure callback timeout relative to current time Fixes a bug where the timeout passed to `scheduleCallback` represented an absolute timestamp, instead of the amount of time until that timestamp is reached. The solution is to subtract the current time from the expiration. The bug wasn't caught by other tests because we use virtual times that default to 0, and most tests don't advance time. I also moved the `initialTimeMs` offset to the `SchedulerWithReactIntegration` module so that we don't have to remember to subtract the offset every time. (We should consider upstreaming this to the Scheduler package.) --- .../src/ReactFiberScheduler.js | 25 ++++++++++++------- .../src/SchedulerWithReactIntegration.js | 12 ++++++++- .../ReactExpiration-test.internal.js | 21 ++++++++++++++++ 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 9c1065a568781..edb3304ce244e 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -234,13 +234,12 @@ let interruptedBy: Fiber | null = null; // In other words, because expiration times determine how updates are batched, // we want all updates of like priority that occur within the same event to // receive the same expiration time. Otherwise we get tearing. -let initialTimeMs: number = now(); let currentEventTime: ExpirationTime = NoWork; export function requestCurrentTime() { if (workPhase === RenderPhase || workPhase === CommitPhase) { // We're inside React, so it's fine to read the actual time. - return msToExpirationTime(now() - initialTimeMs); + return msToExpirationTime(now()); } // We're not inside React, so we may be in the middle of a browser event. if (currentEventTime !== NoWork) { @@ -248,7 +247,7 @@ export function requestCurrentTime() { return currentEventTime; } // This is the first update since React yielded. Compute a new start time. - currentEventTime = msToExpirationTime(now() - initialTimeMs); + currentEventTime = msToExpirationTime(now()); return currentEventTime; } @@ -453,10 +452,18 @@ function scheduleCallbackForRoot( cancelCallback(existingCallbackNode); } root.callbackExpirationTime = expirationTime; - const options = - expirationTime === Sync - ? null - : {timeout: expirationTimeToMs(expirationTime)}; + + let options = null; + if (expirationTime !== Sync && expirationTime !== Never) { + let timeout = expirationTimeToMs(expirationTime) - now(); + if (timeout > 5000) { + // Sanity check. Should never take longer than 5 seconds. + // TODO: Add internal warning? + timeout = 5000; + } + options = {timeout}; + } + root.callbackNode = scheduleCallback( priorityLevel, runRootCallback.bind( @@ -950,7 +957,7 @@ function inferTimeFromExpirationTime(expirationTime: ExpirationTime): number { // We don't know exactly when the update was scheduled, but we can infer an // approximate start time from the expiration time. const earliestExpirationTimeMs = expirationTimeToMs(expirationTime); - return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION + initialTimeMs; + return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION; } function workLoopSync() { @@ -1861,7 +1868,7 @@ function computeMsUntilTimeout( // Compute the time until this render pass would expire. const timeUntilExpirationMs = - expirationTimeToMs(committedExpirationTime) + initialTimeMs - currentTimeMs; + expirationTimeToMs(committedExpirationTime) - currentTimeMs; // Clamp the timeout to the expiration time. // TODO: Once the event time is exact instead of inferred from expiration time diff --git a/packages/react-reconciler/src/SchedulerWithReactIntegration.js b/packages/react-reconciler/src/SchedulerWithReactIntegration.js index 3a30a2ea2e48a..0463ab03b8ec3 100644 --- a/packages/react-reconciler/src/SchedulerWithReactIntegration.js +++ b/packages/react-reconciler/src/SchedulerWithReactIntegration.js @@ -65,7 +65,6 @@ export const IdlePriority: ReactPriorityLevel = 95; // NoPriority is the absence of priority. Also React-only. export const NoPriority: ReactPriorityLevel = 90; -export const now = Scheduler_now; export const shouldYield = disableYielding ? () => false // Never yield when `disableYielding` is on : Scheduler_shouldYield; @@ -73,6 +72,17 @@ export const shouldYield = disableYielding let immediateQueue: Array | null = null; let immediateQueueCallbackNode: mixed | null = null; let isFlushingImmediate: boolean = false; +let initialTimeMs: number = Scheduler_now(); + +// If the initial timestamp is reasonably small, use Scheduler's `now` directly. +// This will be the case for modern browsers that support `performance.now`. In +// older browsers, Scheduler falls back to `Date.now`, which returns a Unix +// timestamp. In that case, subtract the module initialization time to simulate +// the behavior of performance.now and keep our times small enough to fit +// within 32 bits. +// TODO: Consider lifting this into Scheduler. +export const now = + initialTimeMs < 10000 ? Scheduler_now : () => Scheduler_now() - initialTimeMs; export function getCurrentPriorityLevel(): ReactPriorityLevel { switch (Scheduler_getCurrentPriorityLevel()) { diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js index 9a9402ecb90d2..4569250c48e06 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js @@ -272,4 +272,25 @@ describe('ReactExpiration', () => { expect(Scheduler).toFlushExpired([]); expect(ReactNoop).toMatchRenderedOutput('Hi'); }); + + it('should measure callback timeout relative to current time, not start-up time', () => { + // Corresponds to a bugfix: https://github.com/facebook/react/pull/15479 + // The bug wasn't caught by other tests because we use virtual times that + // default to 0, and most tests don't advance time. + + // Before scheduling an update, advance the current time. + Scheduler.advanceTime(10000); + + ReactNoop.render('Hi'); + expect(Scheduler).toFlushExpired([]); + expect(ReactNoop).toMatchRenderedOutput(null); + + // Advancing by ~5 seconds should be sufficient to expire the update. (I + // used a slightly larger number to allow for possible rounding.) + Scheduler.advanceTime(6000); + + ReactNoop.render('Hi'); + expect(Scheduler).toFlushExpired([]); + expect(ReactNoop).toMatchRenderedOutput('Hi'); + }); });