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: devtools Profiler causes unexpected errors #19911

Closed
henryqdineen opened this issue Sep 25, 2020 · 12 comments · Fixed by #20011
Closed

Bug: devtools Profiler causes unexpected errors #19911

henryqdineen opened this issue Sep 25, 2020 · 12 comments · Fixed by #20011

Comments

@henryqdineen
Copy link
Contributor

henryqdineen commented Sep 25, 2020

We noticed that our app would behave differently during profiling runs and trigger errors. I'm not totally sure what the underlying issue is but I was able to put together a example app to reproduce. As far as I can tell it has to do with how devtools is overriding console.warn and console.error. In that case describeNativeComponentFrame() will call a function component with no args. This works fine as the error is caught in describeNativeComponentFrame() but in it looks like a useEffect() that accesses those props is still triggered and it does not expect props to be undefined.

I realize that having props in the dependencies array of the useEffect doesn't really make sense but I still think it probably shouldn't error.

React version: 16.13.1
React devtools version: 4.8.2

Steps To Reproduce

  1. Open link to code example below
  2. Click "Open In New Window" from the "Browser" tab
  3. Observe a simple app with only <h1>Hello World</h1>
  4. Open the React devtools Profiler tab
  5. Click "Reload and start profiling"
  6. Observe an error Uncaught TypeError: Cannot read property 'foo' of undefined

Link to code example:

https://codesandbox.io/s/cool-sun-wdrry

The current behavior

wdrry csb app_

The expected behavior

The app should work as it while not profile. It should render a <h1>Hello World</h1>

@henryqdineen henryqdineen added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Sep 25, 2020
@bvaughn bvaughn self-assigned this Sep 25, 2020
@bvaughn bvaughn added Component: Developer Tools Type: Bug and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Sep 25, 2020
@bvaughn
Copy link
Contributor

bvaughn commented Sep 25, 2020

Thanks for the report. I'll look at it sometime soon. :)

@ashr81
Copy link

ashr81 commented Sep 28, 2020

@bvaughn Shall I work on a pull request for this issue?

@bvaughn
Copy link
Contributor

bvaughn commented Sep 28, 2020

@ashr81 If you'd like!

@ashr81
Copy link

ashr81 commented Sep 28, 2020

@bvaughn Yes, I like to contribute.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

I'm going to look into this this afternoon! :)

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

I'm not sure I understand yet what Profiling or DevTools has to do with this bug. I've exported the attached Code Sandbox and I can't repro this bug with DevTools or while profiling.

Edit I can reproduce this on Code Sandbox itself, just not locally. It's possible that this Code Sandbox is pinned to using react-devtools-inline v4.4.0 from January, so it's missing several bug fixes since then. I'm not sure which one would have fixed this though.

Edit 2 I have a bit more context now. Still looking though.

@ashr81
Copy link

ashr81 commented Oct 13, 2020

You should see the bug when you open codesandbox in seperate tab in browser and start profiling.props are not passing into useEffect while profiling on only codesandbox. I tried it outside codesandbox, It is working fine.

@ashr81
Copy link

ashr81 commented Oct 13, 2020

Profiling using same react-dev-tools works codesandbox fine with the latest React 17.0.0-rc.3

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

I see the problem here.

Logging a warning during render causes DevTools (if configured to "append component stacks" to warnings) to do its own shallow render of a component. The problem is that the useEffect from this shallow render is getting scheduled to be called later when we flush passive effects. This is not supposed to happen.

DevTools overrides the dispatcher before shallow rendering to prevent this from happening:

let previousDispatcher;
if (__DEV__) {
previousDispatcher = currentDispatcherRef.current;
// Set the dispatcher in DEV because this might be call in the render function
// for warnings.
currentDispatcherRef.current = null;
disableLogs();
}

That being said, looking in the Chrome debugger– the source I'm seeing does not do that:

function describeNativeComponentFrame(fn, construct, currentDispatcherRef) {
  // If something asked for a stack inside a fake render, it should get ignored.
  if (!fn || reentry) {
    return '';
  }

  if (false) {}

  let control;
  const previousPrepareStackTrace = Error.prepareStackTrace; // $FlowFixMe It does accept undefined.

  Error.prepareStackTrace = undefined;
  reentry = true;
  let previousDispatcher;

  // ...

This reason for this is that I'm kind of clowny and didn't take into account the fact that the DevTools extension itself would be running in production mode, even if the application code (that logs warnings) is in DEV mode.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

Feel like testing this (local) build of DevTools and seeing if it fixes the problem for you?
ReactDevTools.zip

@ashr81
Copy link

ashr81 commented Oct 13, 2020

It's working fine now. tested it using the generated link on pull request you raised.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 13, 2020

Thank you for confirming, and for the repro case. I'll try go get an update (with the fix) pushed to the browser stores shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants