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 "ResizeObserver loop limit exceeded" error on Chrome #4598

Merged

Conversation

kazarmy
Copy link
Contributor

@kazarmy kazarmy commented Apr 29, 2023

This pr fixes the following error on current Windows Chrome (112.0.5615.138) when using server.js:

ResizeObserver-loop-limit-exceeded

This appears to have a good write-up of the error. In short, despite the word "Compiled", this actually appears to be a runtime error caught by a webpack-supplied error handler, it is benign, Chrome doesn't report this error by default, and it is caused by multiple resize calls within 1 animation frame.

Hunting for a fix led me to here and then here. Apparently, a workaround is to explicitly put the resize calls on separate animation frames and afaik that's what I've done here.

I don't see performance problems while testing but I don't have large profiles to test. The test change below appears benign since the snapshot appears to be canvas calls for a single animation frame.

If problems arise, an alternative solution is to disable completely the webpack overlay using:

const serverConfig = {
    client: {
      overlay: false
    },

in server.js.

@mstange
Copy link
Contributor

mstange commented Apr 29, 2023

I've also seen this error in Firefox, when running the profiler locally in development mode. In Firefox, the error message is "ResizeObserver loop completed with undelivered notifications."

@mstange
Copy link
Contributor

mstange commented May 1, 2023

The resize observer reports a resize of the TimelineTrackThreadImpl. It calls WithSize's listener, which calls setState on itself. This causes TimelineTrackThreadImpl's componentDidUpdate method to be called, which calls reportTrackThreadHeight. This notices a height change and dispatches an UPDATE_TRACK_THREAD_HEIGHT action. This changes the value stored in trackThreadHeights which during the render gets read by getTimelineHeight and causes the the timeline height to contract.

So basically, when we don't have enough tracks to fill the entire default timeline height, the shrink-to-fit render happens synchronously during a ResizeObserver notification, and browsers don't like that. It's not documented very well, but I think the expectation for ResizeObserver users is the following: During the callback for element A, you may make changes which affect the sizes of the contents of A, but don't make changes that affect the sizes of element A itself or of anything outside element A. Here we are making a change to the timeline container, which is an ancestor of A.

@kazarmy
Copy link
Contributor Author

kazarmy commented May 1, 2023

In Firefox, the error message is "ResizeObserver loop completed with undelivered notifications."

This error message (and the Chrome one as well) suggests that calls to the resize observer are being dropped which should result in some visual glitching, but I didn't notice any, perhaps because the glitching isn't perceptible.

@mstange
Copy link
Contributor

mstange commented May 1, 2023

I think the notifications will still be delivered, they'll just be a frame late.

I wonder if we can just find a different solution for the timeline height. Maybe we can make the resizer adjust max-height instead of height.

@julienw
Copy link
Contributor

julienw commented May 2, 2023

I'm just a little bit concerned about waiting for a frame to react to the resize, especially for the width (I don't really mind for the height). I'm not super happy about changing this... especially that in React 18 with the new API the rendering happens in an additional rAF too and may very well make this error disappear magically once we switch to it!

I'm not super happy about our current way of reporting the track height either. This may be the source of some of our performance issues at load time (that may disappear with the new API too BTW, but not sure).

But I'm very tempted to just remove the webpack overlay at this point. What do you think?

@kazarmy
Copy link
Contributor Author

kazarmy commented May 3, 2023

As per this, putting

  window.addEventListener('error', (e) => {
    if(e.message === 'ResizeObserver loop completed with undelivered notifications.' || 
        e.message === 'ResizeObserver loop limit exceeded') {
        e.stopImmediatePropagation();
    }
  });

at the start of res/before-load.js does fix the problem and is less of a blanket cover-up, but this solution is somewhat icky and might not work with a translated browser.

I agree with removing the webpack overlay. It would have been my first choice if not for the fact that it's basically about ignoring a possible problem. I'll redo the pr as soon as get the time to figure out where to put the overlay config settings.

@kazarmy
Copy link
Contributor Author

kazarmy commented May 3, 2023

The overlay settings are part of webpack-dev-server so they are in server.js (8b7e5a7). I don't think they need to be in res/photon/server.js.

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

Thanks, let's do that.
I just added a comment explaining why we disable it when when you might want to revisit.

@julienw julienw merged commit c7c6e3f into firefox-devtools:main May 4, 2023
@canova canova mentioned this pull request May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants