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 ignored setState in Safari when iframe is touched #24459

Merged
merged 1 commit into from
May 12, 2022

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 28, 2022

Fixes #24458.

Just like #22459, this is caused by a Safari bug (https://bugs.webkit.org/show_bug.cgi?id=235322).

Our original solution for #22459 was #23111: if the microtask callback runs prematurely, we ignore it. This relied on the assumption that we always check for more work at the end of the task anyway. However, we don't check for more work if we aren't either rendering or committing — notably, we don't check for more work at the end of the event handler.

This fix is to allow the update to flush early. This doesn't respect batching but matches what Safari is trying to do (which is to flush everything early).

@gaearon gaearon requested review from sebmarkbage and acdlite April 28, 2022 16:41
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Apr 28, 2022
@sizebot
Copy link

sizebot commented Apr 28, 2022

Comparing: 340060c...b7b40cd

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.58 kB 131.58 kB = 42.11 kB 42.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.82 kB 136.82 kB = 43.69 kB 43.69 kB
facebook-www/ReactDOM-prod.classic.js = 441.17 kB 441.18 kB = 80.49 kB 80.49 kB
facebook-www/ReactDOM-prod.modern.js = 426.38 kB 426.39 kB = 78.29 kB 78.29 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.17 kB 441.18 kB = 80.49 kB 80.49 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against b7b40cd

queue.forEach(cb => cb());
queue = [];
flushMicrotasksPrematurely = function() {
while (queue.length > 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This simulates the Safari behavior of trying to drain the queue until it's empty. So that we don't "fix" this by adding an infinite loop.

Proof:

<body>
<script>
let busy = true
const callback = () => {
  if (busy) {
    console.error('noo im busy')
    queueMicrotask(callback)
  } else {
    console.log('im ok')
  }
}
queueMicrotask(callback);
console.log("will add iframe");
const iframe = document.createElement("iframe");
iframe.src = "localhost";
document.body.appendChild(iframe);
console.log("did add iframe");
busy = false
</script>
</body>

gets Safari infinitely stuck

@gaearon
Copy link
Collaborator Author

gaearon commented May 6, 2022

ping @acdlite

@gaearon gaearon merged commit 2c8a145 into facebook:main May 12, 2022
@gaearon gaearon deleted the safari-flush-early branch May 12, 2022 16:58
lunaruan pushed a commit to lunaruan/react that referenced this pull request May 13, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 6, 2022
Summary:
This sync includes the following changes:
- **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>//
- **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>//
- **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>//
- **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>//
- **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>//
- **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>//
- **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>//
- **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>//
- **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>//
- **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>//
- **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>//
- **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>//
- **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>//
- **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>//
- **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>//
- **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>//
- **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>//
- **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>//
- **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>//
- **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>//

Changelog:
[General][Changed] - React Native sync for revisions bd4784c...d300ceb

jest_e2e[run_all_tests]

Reviewed By: cortinico, kacieb

Differential Revision: D36874368

fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: setState is not flushed if an iframe is added in the same tick in Safari
4 participants