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

Bug: infinite loop when a set state hook is called in a global handler #17918

Closed
OrKoN opened this issue Jan 28, 2020 · 9 comments
Closed

Bug: infinite loop when a set state hook is called in a global handler #17918

OrKoN opened this issue Jan 28, 2020 · 9 comments

Comments

@OrKoN
Copy link

OrKoN commented Jan 28, 2020

To be honest, I am not 100% sure it's a bug. Perhaps my code is badly written but I don't know why it's not working or how to rewrite it in the proper way.

React version: 16.12.0

Steps To Reproduce

function Hello() {
    const [s, setS] = React.useState(1);
    const print = () => {
      setS(s + 1);
      setTimeout(() => {
      	window.print();
      }, 600);
      window.onafterprint = () => {
      	setS(s - 1);
      }
    }
    return <div>{s}<button onClick={print}>print</button></div>;
}

  1. Open this jsfiddle https://jsfiddle.net/z4ku39t2/2 or try the code above
  2. Click Print button and cancel the print dialog

Link to code example: https://jsfiddle.net/z4ku39t2/2

The current behavior

When the setS is called in the onafterprint handler, the app enters an infinite loop with 100% cpu usage so you won't be able to do anything on the page. The profiler shows that it happens inside React.

The expected behavior

The setS successfully modifies the state and the component re-renders.

Browser: Version 79.0.3945.130 (Official Build) (64-bit)

@OrKoN OrKoN added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jan 28, 2020
@thecodrr
Copy link

I can confirm and reproduce this bug.

@bvaughn
Copy link
Contributor

bvaughn commented Jan 28, 2020

Interesting. Looks like this has been reported before for people using the react-to-print package (but not in this repository):

Looks like that library just calls props.onAfterPrint synchronously right after calling print(). I think this is because print() used to be a blocking method for most browsers, but it no longer seems to be for Chrome nor Safari.

I'm not very familiar with our DOM event system so I don't know why this behavior would be happening, but as a temporary workaround for the issue, you can use a setTimeout in onAfterPrint:
https://codesandbox.io/s/goofy-cerf-yh84o

@bvaughn bvaughn added Component: DOM Type: Bug and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Jan 28, 2020
@OrKoN
Copy link
Author

OrKoN commented Jan 28, 2020

The print function is still sync in Chrome but not in Safari and that was our workaround for this (use case: modify some state for printing and revert after printing). It seems like the following mitigates the problem:

      window.onafterprint = () => {
        setTimeout(() => {
      		setS(s - 1);
      	}, 100);
      }

@bvaughn
Copy link
Contributor

bvaughn commented Jan 28, 2020

The print function is still sync in Chrome

This does not match my testing a few minutes ago.

Also, a duration of 100 shouldn't be necessary. 0 should work. I think you just need to move the state update outside of the context of the onafterprint function.

@lcelso
Copy link

lcelso commented Feb 8, 2020

I believe the problem is happening is an event loop, right?

In the first call of setTimeout it goes to the Call Stack which in turn calls the Web App and starts the timer and after the timer ends it throws it to a Callback Quee.

However the javascript reading continues and reads below and calling the onafterprint performs the same.

And after that the event looping detects that there is something in the queue and executes setTimeout.

Example:
https://miro.medium.com/max/1468/1*LvbUhFpMUeN9xoaazrp_gQ.jpeg

Here talking a little more about the topic.

https://www.youtube.com/watch?v=8aGhZQkoFbQ

@idmadj
Copy link

idmadj commented Aug 6, 2020

The behavior seems to be caused by the way react-dom's dev version handles errors in user-provided callbacks.

For context:

// To preserve the expected "Pause on exceptions" behavior, we don't use a
// try-catch in DEV. Instead, we synchronously dispatch a fake event to a fake
// DOM node, and call the user-provided callback from inside an event handler
// for that fake event. If the callback throws, the error is "captured" using
// a global event handler. But because the error happens in a different
// event loop context, it does not interrupt the normal program flow.
// Effectively, this gives us try-catch behavior without actually using
// try-catch. Neat!

Relevant to this issue:

const evt = document.createEvent('Event');
// Keeps track of whether the user-provided callback threw an error. We
// set this to true at the beginning, then set it to false right after
// calling the function. If the function errors, `didError` will never be
// set to false. This strategy works even if the browser is flaky and
// fails to call our global error handler, because it doesn't rely on
// the error event at all.
let didError = true;
// Keeps track of the value of window.event so that we can reset it
// during the callback to let user code access window.event in the
// browsers that support it.
let windowEvent = window.event;
// Keeps track of the descriptor of window.event to restore it after event
// dispatching: https://github.com/facebook/react/issues/13688
const windowEventDescriptor = Object.getOwnPropertyDescriptor(
window,
'event',
);
// Create an event handler for our fake event. We will synchronously
// dispatch our fake event using `dispatchEvent`. Inside the handler, we
// call the user-provided callback.
const funcArgs = Array.prototype.slice.call(arguments, 3);
function callCallback() {
// We immediately remove the callback from event listeners so that
// nested `invokeGuardedCallback` calls do not clash. Otherwise, a
// nested call would trigger the fake event handlers of any call higher
// in the stack.
fakeNode.removeEventListener(evtType, callCallback, false);
// We check for window.hasOwnProperty('event') to prevent the
// window.event assignment in both IE <= 10 as they throw an error
// "Member not found" in strict mode, and in Firefox which does not
// support window.event.
if (
typeof window.event !== 'undefined' &&
window.hasOwnProperty('event')
) {
window.event = windowEvent;
}
func.apply(context, funcArgs);
didError = false;
}
// Create a global error event handler. We use this to capture the value
// that was thrown. It's possible that this error handler will fire more
// than once; for example, if non-React code also calls `dispatchEvent`
// and a handler for that event throws. We should be resilient to most of
// those cases. Even if our error event handler fires more than once, the
// last error event is always used. If the callback actually does error,
// we know that the last error event is the correct one, because it's not
// possible for anything else to have happened in between our callback
// erroring and the code that follows the `dispatchEvent` call below. If
// the callback doesn't error, but the error event was fired, we know to
// ignore it because `didError` will be false, as described above.
let error;
// Use this to track whether the error event is ever called.
let didSetError = false;
let isCrossOriginError = false;
function handleWindowError(event) {
error = event.error;
didSetError = true;
if (error === null && event.colno === 0 && event.lineno === 0) {
isCrossOriginError = true;
}
if (event.defaultPrevented) {
// Some other error handler has prevented default.
// Browsers silence the error report if this happens.
// We'll remember this to later decide whether to log it or not.
if (error != null && typeof error === 'object') {
try {
error._suppressLogging = true;
} catch (inner) {
// Ignore.
}
}
}
}
// Create a fake event type.
const evtType = `react-${name ? name : 'invokeguardedcallback'}`;
// Attach our event handlers
window.addEventListener('error', handleWindowError);
fakeNode.addEventListener(evtType, callCallback, false);
// Synchronously dispatch our fake event. If the user-provided function
// errors, it will trigger our global error handler.
evt.initEvent(evtType, false, false);
fakeNode.dispatchEvent(evt);

Simply put, to play nicer with devtools, user-provided callbacks are not called directly. Instead a listener to a synthetic event is setup (L175), with its handler calling the user-provided callback (L130). Then the event is immediately dispatched (L180).

For some reason, when a callback is invoked from within a beforeprint or afterprint handler, the callCallback handler (L112) is never executed, despite both fakeNode.addEventListener(evtType, callCallback, false); and fakeNode.dispatchEvent(evt); being run. didError is never set to true, and the syncQueue keeps having the same callback being added to it, resulting in the infinite loop.

Replacing this part:

fakeNode.addEventListener(evtType, callCallback, false);
// Synchronously dispatch our fake event. If the user-provided function
// errors, it will trigger our global error handler.
evt.initEvent(evtType, false, false);
fakeNode.dispatchEvent(evt);

... with just:

callCallback();

... prevents the faulty behavior from occuring.

It's also worth nothing that this does not happen in production since callbacks are called directly.

Working around this proves to be difficult, especially when trying to change state in response to beforeprint. Any changes to the DOM need to happen synchronously within the beforeprint handler for them to show up on the printed page. For this reason the state change can't be delayed using setTimeout or rAF. Changing the state, then making a delayed call to window.print() only works if you are initiating the print process programmatically (Ctrl/Cmd+P or the browser's menu 'Print' function can only be intercepted using beforeprint).

@twavv
Copy link

twavv commented Aug 21, 2020

I'm also having this issue, in conjunction with printing, and can corroborate @idmadj's analysis. There's an infinite loop in the dev implementation for invokeGuardedCallback. The solutions above don't work since my app uses media query listeners in JS (which are attached to window) to change some aspects of the page before printing.

Interestingly, a workaround in my case seems to be changing window.print() to ReactDOM.unstable_batchedUpdates(() => window.print()).


Further investigating reveals that this only seems to be an issue when programmatically invoking window.print().

https://codesandbox.io/s/nervous-liskov-3mqby

Opening up the page outside of the sandbox, and pressing Ctrl+P reveals that the callback is invoked synchronously, and using window.print() does not invoke the event listener at all.

@twavv
Copy link

twavv commented Aug 23, 2020

Looks this this was actually fixed for React 17 in #19220.

@eps1lon
Copy link
Collaborator

eps1lon commented Aug 23, 2020

Looks this this was actually fixed for React 17 in #19220.

Last published build from master before #19220 was 0.0.0-33c3af284 which still hangs while it no longer freezes with ##19220.

Closing since it the description fits.

@eps1lon eps1lon closed this as completed Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants