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.

Sign up for GitHub

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

Jump to bottom

react-dev-overlay causes 100% CPU #19893

Closed
ocavue opened this issue Dec 6, 2020 · 4 comments · Fixed by #20647
Closed

react-dev-overlay causes 100% CPU #19893

ocavue opened this issue Dec 6, 2020 · 4 comments · Fixed by #20647
Assignees
@Timer
Milestone
iteration 15

Comments

@ocavue
Copy link

ocavue commented Dec 6, 2020 •
edited
Loading

Bug report

Describe the bug

My web page will have 100% CPU usage in development mode when an error causes.

To Reproduce

  1. Clone the repository and start the dev server
$ git clone https://github.com/ocavue/nextjs-issue-19893.git /tmp/next-overlay-isssue
$ cd /tmp/next-overlay-isssue
$ yarn install 
$ yarn dev
  1. Open http://localhost:3000 in the browser.
  2. Input 123456789 in the textarea.
  3. Open Chrome's Task Manager, and you will see that this tab use 100% CPU.

Expected behavior

The CPU usage is normal.

Screenshots

image

System information

  • OS: macOS 10.15
  • Browser (if applies) Chrome 87
  • Version of Next.js: 10.0.3
  • Version of Node.js: 15.2.1
  • Deployment: N/A

Additional context

I use ProseMirror in my application, which is a text editor that doesn't depend on React. ProseMirror has a lot of DOM manipulation, including MutationObserver.

react-dev-overlay uses ally.maintain.disabled to observe DOM manipulations and stop them, which also use MutationObserver.

After some debugging, I think the reason is: when a ProseMirror error occurs, react-dev-overlay and ProseMirror will observe each other's DOM manipulation and they manipulate the DOM as a response. This causes an infinite loop and the CPU usage reaches 100%.

According to this, the simplest solution should be removing ally.js from react-dev-overlay, also consider that ally.js hasn't been updated in nearly four years.
@ocavue ocavue added the bug Issue was opened via the bug report template. label Dec 6, 2020
@Timer Timer added kind: story and removed bug Issue was opened via the bug report template. labels Dec 7, 2020
@Timer Timer added this to the iteration 15 milestone Dec 7, 2020
@Timer Timer self-assigned this Dec 7, 2020
@Timer Timer added the point: 3 label Dec 7, 2020
@Timer Timer modified the milestones: iteration 15, iteration 14 Dec 7, 2020
@ocavue
Copy link
Author

ocavue commented Dec 8, 2020

@Timer

Just be curious, a question that is not related to this issue: I see that every next.js issue has a point:x label. What is the purpose of this label? The amount of work needed to fix an issue?

@timneutkens
Copy link
Member

The amount of work needed to fix an issue?

It's synced to our project management software, they're sprint points yeah.

Timer added a commit to Timer/next.js that referenced this issue Dec 31, 2020
@Timer
fix(overlay): skip disable & upgrade platform
5a45257 
This bundles ally.js into Next.js itself to upgrade a dependency they have pinned.

I tried every other major focus trap solution, even those used by some modal libraries, and they all failed.

`ally.js` is the only library that can do it correctly, so we're going to stick with it.

I also removed the `maintain/disabled` as we have a overlay that would effectively result in the same. This reduces CPU strain.

---

Fixes vercel#19893
Fixes vercel#14369
Closes vercel#14372
@Timer Timer mentioned this issue Dec 31, 2020
Merged
@kodiakhq kodiakhq bot closed this as completed in #20647 Dec 31, 2020
kodiakhq bot pushed a commit that referenced this issue Dec 31, 2020
@Timer
fix(overlay): skip disable & upgrade platform (#20647)
e4a7446 
This bundles ally.js into Next.js itself to upgrade a dependency they have pinned.

I tried every other major focus trap solution, even those used by some modal libraries, and they all failed.

`ally.js` is the only library that can do it correctly, so we're going to stick with it.

I also removed the `maintain/disabled` as we have a backdrop that would effectively result in the same. This reduces CPU strain.

---

Fixes #19893
Fixes #14369
Closes #14372
@ocavue
Copy link
Author

ocavue commented Jan 15, 2021

I can confirm that [email protected] doesn't have this issue anymore. Thank you very much for your help. @Timer

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Timer Timer

Labels
None yet
Projects
None yet
Milestone
iteration 15
Development

Successfully merging a pull request may close this issue.

fix(overlay): skip disable & upgrade platform Timer/next.js
4 participants
@Timer @timneutkens @balazsorban44 @ocavue