-
Notifications
You must be signed in to change notification settings - Fork 92
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
ResizeObserver loop limit exceeded #45
Comments
Thanks for such detailed issue and links how to fix this. Fixed in this commit 9625f67#diff-1fdf421c05c1140f6d71444ea2b27638R36 |
@maslianok Is this something that should be fixed? |
@ksocha @maslianok We have seen this issue for quite some time (at least since 6.1). It's visible in our Sentry logs and also Cypress fails on this error (unless we ignore it). |
Also should note that changing refresh mode to debounce seems to resolve the problem. |
To provide some context: i've removed
To fix this bug we have to reproduce it. Can you provide a reproducible example? Actually any info would help: browser version, device type (desktop or mobile), maybe some specific platform (Android / iOS / Windows / Linux / Mac) etc. For now, the best workaround I can think about is to use debounce with 0 delay |
Here's a list of user agent strings producing this error over the last week or so: https://docs.google.com/spreadsheets/d/177wudO9omK6tbzyWmXKLFQpW1idKaceX4rzvb6MFxBE/edit?usp=sharing |
+1 |
We're seeing this on the following Browser / OS: Edge 90.0.818 / Windows 10 |
+1 |
Sorry, guys, I have no quick fix for this problem. Earlier we blindly added For now we can try to workaround the problem by using { refreshMode: 'debounce', refreshRate: 0 } Any help would be greatly appreciated. Even if someone could provide a code snippet where the bug appears. |
I had to do some research on this for work, and found a bit of background information that may be helpful. If I get a bit of time I'll try to make a reproducible code sample (although I can't promise that right now). Background Info:I believe this error is the same as the one listed here as part of the ResizeObserver spec. Chromium based browsers seem to use a different error message than that exact one, but in Firefox the error message is the same. This error is emitted if a resize event happens and the browser is unable to notify all the listeners within that frame. In our app this happens because we update some fairly global React state directly from the I'm not sure if the browser eventually does notify all the listeners and this warning just means there was a dropped frame, or if some of them just get skipped for that event? If it's the former then this is potentially an error that is safe to ignore, if it's the latter than probably not. Why this error is hard to replicate:Because ResizeObserver emits its errors as events on the You can manually log them by running The solution that worked for us:The root of this problem seems to be React re-rendering (or any computationally intensive work, really) happening as a direct result of the resize event. If you defer that work to after the resize event has been handled, the browser is totally fine with it because it's not being prevented from delivering all of its event notifications in time. The simplest method of doing this for us was to replace:
with:
in our app. This defers the React update until the next pass of the event loop, gets it out of the event context, and avoids the issue. Obviously the code your app is running to trigger re-renders or other work will be different, but I believe that idea of running this work in a delayed callback should resolve the issue for any application. I believe this also explains why I'm not sure if adding a setTimeout to the library would be an appropriate fix, as it may introduce similar bugs - but at least as a user of the library my team has found that this prevents the issue in our app and we felt it would be helpful to share. |
@pawnstar oh wow! Thank you for such a detailed description 👍 Great job 💪 |
Yes, using I think you can use |
@pawnstar @maslianok just to be clear; using any value as refreshRate (or any duration in So values above 0, and both |
yes, correct |
Still having this issue despite using the suggested workaround. Will feedback if I find anything useful.
It should be the former if implemented to spec: Elements with undelivered notifications will be considered for delivery in the next loop. https://www.w3.org/TR/resize-observer/#html-event-loop |
@aderickson Wow I was pulling my hair out trying to replicate it. Funny enough, the chrome instance of Cypress did log this error in the console as well, I guess they attach a similar event handler to log errors. Out of curiosity, do you have some more info on this statement:
I've been in WebDev for a while now and was completely unaware of this. Can't even seem to find more insights on this online, MDN does not mention anything about it either |
@maslianok I'm sure you have a reproduction case by now. But still, here's a very very basic example where the error already appears, simply by unmounting a resize-detector component using useState: Screen.Recording.2022-01-20.at.11.54.21.mov
|
So the ResizeObserver spec section 3.4.6 says that if there are undelivered resize notifications it should:
Reporting the exception is a bit confusing to follow, but I believe the relevant behavior is defined in the HTML spec section 8.1.4.6, where it says:
Error reporting includes:
This appears to be why an event is emitted from the There's also the possibility I've found the wrong exception reporting spec, especially because the ResizeObserver spec indicates that the resize notification delivery (and undelivered notification exception) all happen as part of the browser's HTML processing loop rather than in any direct script context. I haven't managed to find anywhere else that talks about reporting an exception though, so at the moment I think this is accurate. |
@aderickson Thanks a lot! I rarely have to go this deep into specs, so this is very interesting to me 😄 |
If this is still the recommended solution for a 2018 issue, should these two props be default arguments? My solution right now is using a wrapper component for my project's 30+ usages of
|
I created a browser extension that runs the following code which blocks adding an error event handler before there's anything else in the DOM (so no other scripts could have executed): window.addEventListener = new Proxy(window.addEventListener, {
apply(target, this_arg, arg_list) {
if (arg_list[0] === "error") {
console.log("blocked aev", arg_list, this_arg);
console.trace();
return;
}
return Reflect.apply(target, this_arg, arg_list);
},
});
Object.defineProperty(window, "onerror", {
get() {},
set() {
console.log("prevented setting onerror");
console.trace();
},
configurable: false,
}); I did catch some libraries trying to add an error event handler but I don't see any error in both firefox's and chrome's consoles with no "error" event handlers. So I am confident to say that the browsers don't report this properly to the console and the only way to get the error is by having an "error" event handler. The incredibly frustrating thing is that the error doesn't have any stack trace so I don't know which ResizeObserver is causing the issue for me (I'm not using this library, just came across this github issue since it contained good info about the ResizeObserver issue) |
Many things changed in my life in the past five years... Except for the one - this bug is still open. Today I'm going to change this too :) I spent a day reading the comments and playing with the code. Honestly, I'm not sure I fully understand the root of the problem, so I had to almost blindly change this and that... I believe the issue is fixed in React@18. To be more specific, the error goes when you update This is the fork of the sandbox provided in the message above. As you can see, there is no error https://codesandbox.io/s/useresizedetector-resizeobserver-loop-limit-exceeded-reprod-forked-v90h07 I hope all collaborators of this issue doing well ❤️ Letting you know that I will remember your github logins for a long time 🤣 And no, I won't reopen it! Sorry :) |
I am getting a lot of "ResizeObserver loop limit exceeded" exceptions reported from my users (through Sentry).
It seems to only come from Chrome 64+ on mobile (so no polyfill).
WICG/resize-observer#38 gives a bit of context, and using
requestAnimationFrame
seems to fix the bug (see cerner/terra-core#1647).Would you be open to implement a fix for this?
The text was updated successfully, but these errors were encountered: