From 32d1e4b15c20516219de3f1e6a6c6e7beb8e2d7d Mon Sep 17 00:00:00 2001 From: Sebastian Sebbie Silbermann Date: Sun, 25 May 2025 10:02:40 +0200 Subject: [PATCH 1/3] Add affordance to trigger Chrome crash --- fixtures/flight/src/App.js | 2 ++ fixtures/flight/src/Navigate.js | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 fixtures/flight/src/Navigate.js diff --git a/fixtures/flight/src/App.js b/fixtures/flight/src/App.js index 2f2118580a93c..7b811dd7628e0 100644 --- a/fixtures/flight/src/App.js +++ b/fixtures/flight/src/App.js @@ -12,6 +12,7 @@ import Button from './Button.js'; import Form from './Form.js'; import {Dynamic} from './Dynamic.js'; import {Client} from './Client.js'; +import {Navigate} from './Navigate.js'; import {Note} from './cjs/Note.js'; @@ -89,6 +90,7 @@ export default async function App({prerender}) { {dedupedChild} {Promise.resolve([dedupedChild])} + diff --git a/fixtures/flight/src/Navigate.js b/fixtures/flight/src/Navigate.js new file mode 100644 index 0000000000000..f6e0dd3697800 --- /dev/null +++ b/fixtures/flight/src/Navigate.js @@ -0,0 +1,40 @@ +'use client'; + +import * as React from 'react'; +import Container from './Container.js'; + +export function Navigate() { + /** Repro for https://issues.chromium.org/u/1/issues/419746417 */ + function triggerChromeCrash() { + React.startTransition(async () => { + console.log('Default transition triggered'); + + await new Promise(resolve => { + setTimeout( + () => { + history.pushState( + {}, + '', + `?chrome-crash-419746417=${performance.now()}` + ); + }, + // This needs to happen before React's default transition indicator + // is displayed but after it's scheduled. + 100 + -50 + ); + + setTimeout(() => { + console.log('Default transition completed'); + resolve(); + }, 1000); + }); + }); + } + + return ( + +

Navigation fixture

+ + + ); +} From f87e7a4c5fd9b18a15c555ca1556a5f5ff34ef6a Mon Sep 17 00:00:00 2001 From: Sebastian Sebbie Silbermann Date: Sun, 25 May 2025 10:45:39 +0200 Subject: [PATCH 2/3] [react-dom] Enforce 100ms delay before default Transition indicator is triggered We delay the default Transition indicator by 100ms to reduce jank in case we're dealing with a fast Transition. However, this delay was previously interrupted if a navigation completed before the default Transition indicator is shown. Instead of interrupting, we now reschedule the default Transition indicator while ensuring we don't indefinitely defer. This also prevents a crash in Chrome (https://issues.chromium.org/u/1/issues/419746417). ## Test plan - [x] "Provoke Chrome crash" in Flight fixture no longer crashes the page --- fixtures/flight/src/Navigate.js | 4 ++-- .../ReactDOMDefaultTransitionIndicator.js | 23 +++++++++++++++---- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/fixtures/flight/src/Navigate.js b/fixtures/flight/src/Navigate.js index f6e0dd3697800..4436b9fdf7d2e 100644 --- a/fixtures/flight/src/Navigate.js +++ b/fixtures/flight/src/Navigate.js @@ -5,7 +5,7 @@ import Container from './Container.js'; export function Navigate() { /** Repro for https://issues.chromium.org/u/1/issues/419746417 */ - function triggerChromeCrash() { + function provokeChromeCrash() { React.startTransition(async () => { console.log('Default transition triggered'); @@ -34,7 +34,7 @@ export function Navigate() { return (

Navigation fixture

- + ); } diff --git a/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js b/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js index 8f1a32d826c1a..364bbd77c8eed 100644 --- a/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js +++ b/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js @@ -7,6 +7,8 @@ * @flow */ +const defaultTransitionIndicatorDelay = 100; + export function defaultOnDefaultTransitionIndicator(): void | (() => void) { if (typeof navigation !== 'object') { // If the Navigation API is not available, then this is a noop. @@ -16,6 +18,10 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { let isCancelled = false; let pendingResolve: null | (() => void) = null; + const scheduledAt = performance.now(); + // Delay the start a bit in case this is a fast Transition. + setTimeout(startFakeNavigation, defaultTransitionIndicatorDelay); + function handleNavigate(event: NavigateEvent) { if (event.canIntercept && event.info === 'react-transition') { event.intercept({ @@ -29,6 +35,7 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { } function handleNavigateComplete() { + const wasRunning = pendingResolve !== null; if (pendingResolve !== null) { // If this was not our navigation completing, we were probably cancelled. // We'll start a new one below. @@ -38,7 +45,18 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { if (!isCancelled) { // Some other navigation completed but we should still be running. // Start another fake one to keep the loading indicator going. - startFakeNavigation(); + if (wasRunning) { + // Restart sync to make it not janky if it was already running + startFakeNavigation(); + } else { + // Since it hasn't started yet, we want to delay it up to a bit. + // There needs to be an async gap to work around https://issues.chromium.org/u/1/issues/419746417. + const fakeNavigationDelay = Math.max( + 0, + defaultTransitionIndicatorDelay - (performance.now() - scheduledAt), + ); + setTimeout(startFakeNavigation, fakeNavigationDelay); + } } } @@ -70,9 +88,6 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { } } - // Delay the start a bit in case this is a fast navigation. - setTimeout(startFakeNavigation, 100); - return function () { isCancelled = true; // $FlowFixMe From f8ab64e1e482f4d8bc6355883ae0271067ff4ce7 Mon Sep 17 00:00:00 2001 From: Sebastian Sebbie Silbermann Date: Mon, 26 May 2025 10:37:16 +0200 Subject: [PATCH 3/3] Always delay by 20ms --- .../ReactDOMDefaultTransitionIndicator.js | 24 ++++--------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js b/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js index 364bbd77c8eed..37661849cad87 100644 --- a/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js +++ b/packages/react-dom/src/client/ReactDOMDefaultTransitionIndicator.js @@ -7,8 +7,6 @@ * @flow */ -const defaultTransitionIndicatorDelay = 100; - export function defaultOnDefaultTransitionIndicator(): void | (() => void) { if (typeof navigation !== 'object') { // If the Navigation API is not available, then this is a noop. @@ -18,10 +16,6 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { let isCancelled = false; let pendingResolve: null | (() => void) = null; - const scheduledAt = performance.now(); - // Delay the start a bit in case this is a fast Transition. - setTimeout(startFakeNavigation, defaultTransitionIndicatorDelay); - function handleNavigate(event: NavigateEvent) { if (event.canIntercept && event.info === 'react-transition') { event.intercept({ @@ -35,7 +29,6 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { } function handleNavigateComplete() { - const wasRunning = pendingResolve !== null; if (pendingResolve !== null) { // If this was not our navigation completing, we were probably cancelled. // We'll start a new one below. @@ -45,18 +38,8 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { if (!isCancelled) { // Some other navigation completed but we should still be running. // Start another fake one to keep the loading indicator going. - if (wasRunning) { - // Restart sync to make it not janky if it was already running - startFakeNavigation(); - } else { - // Since it hasn't started yet, we want to delay it up to a bit. - // There needs to be an async gap to work around https://issues.chromium.org/u/1/issues/419746417. - const fakeNavigationDelay = Math.max( - 0, - defaultTransitionIndicatorDelay - (performance.now() - scheduledAt), - ); - setTimeout(startFakeNavigation, fakeNavigationDelay); - } + // There needs to be an async gap to work around https://issues.chromium.org/u/1/issues/419746417. + setTimeout(startFakeNavigation, 20); } } @@ -88,6 +71,9 @@ export function defaultOnDefaultTransitionIndicator(): void | (() => void) { } } + // Delay the start a bit in case this is a fast Transition. + setTimeout(startFakeNavigation, 100); + return function () { isCancelled = true; // $FlowFixMe