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: didPutListener tries to findDOMNode on unmounted component #7440

Closed
joshma opened this issue Aug 8, 2016 · 8 comments
Closed

Bug: didPutListener tries to findDOMNode on unmounted component #7440

joshma opened this issue Aug 8, 2016 · 8 comments

Comments

@joshma
Copy link
Contributor

joshma commented Aug 8, 2016

I believe this is a bug and potentially related to #3298, but the specific details are different enough that I wanted to file a separate issue. Feel free to close if it's the same root cause.

I have a repro here: https://jsfiddle.net/7s6mwccu/2/

  • ClickMe is a component that runs causeError on click
  • causeError first renders components of type A and B (let's call them A0 and B0)
  • B0 has a componentDidMount function that calls setState (comes into play later)
  • causeError then updates A0 with a new prop
  • A.componentDidUpdate unmounts B0 and mounts a new B, B1
  • After causeError is all done, React's runtime performs some ops (presumably due to B0's setState) that leads to:

Warning: React can't find the root component node for data-reactid value .2. If you're seeing this message, it probably means that you've loaded two copies of React on the page. At this time, only a single copy of React can be loaded at a time.
Uncaught TypeError: Cannot read property 'firstChild' of undefined

Peeking at the stacktrace, it looks like didPutListener is trying to findDOMNode on an already-unmounted component.

Some weird details:

  • Leaving off onClick removes this error
  • Not calling setState in B.componentDidMount also removes this error

So it's a bit different from #3298 because it's not erroring if we only unmount B during a click; it's erroring if we have onClick and setState.

This is on React 0.14 - I tested it briefly on React 15 and got

Invariant Violation: React DOM tree root should always have a node reference.

which presumably is a better formed version of the same error.

@gaearon
Copy link
Collaborator

gaearon commented Aug 8, 2016

For the record, here is the version with React 15: https://jsfiddle.net/7s6mwccu/3/

It is better to describe the error from 15’s point of view because @spicyj fixed a very similar bug in #6650 so exact cause might have changed since then.

@gaearon
Copy link
Collaborator

gaearon commented Aug 8, 2016

Leaving off onClick removes this error

Likely because there is some special logic related to it that always puts it on the specific node (and not the document root).

Not calling setState in B.componentDidMount also removes this error

Likely because setState() in componentDidMount() is batched, and the bug appears related to batching, just like #6650.

@gaearon
Copy link
Collaborator

gaearon commented Aug 8, 2016

Do you want to dig into this?

@joshma
Copy link
Contributor Author

joshma commented Aug 8, 2016

@gaearon I'm not sure I have the context or bandwidth to look into this. :(

We have a temporary workaround in place (defer some work to the next stack frame), but I thought I'd report it in so that someone with more experience could help out.

@quicksnap
Copy link

I was hoping to get my feet wet with contributing to React, as I have some free time. All the "good first bug" issues seem to be scooped up; think this one is sane to take up?

I'll probably have time on Friday to spend some time with it. If anyone else picks it up beforehand, I understand!

@aweary
Copy link
Contributor

aweary commented Aug 12, 2016

@quicksnap I'm not sure I'd classify this as a "good first bug", but feel free to give it a shot! Never hurts to have more eyes on an issue 😄

@nhunzaker
Copy link
Contributor

nhunzaker commented Oct 23, 2016

It looks like this is fixed in 15.4.0-rc.4:
https://jsfiddle.net/7s6mwccu/5/

I think this is because of my work here (at least git bisect says so):
2fbe0cd

But I'm not sure specifically why. I only set up a conditional that won't attach click listeners for buttons. So I made a version that just uses divs:

https://jsfiddle.net/7s6mwccu/6/

Works too, though I'm not clear why. Stepping through the code (15.4.0-rc.4), it looks like there might be an exception that isn't getting raised: Edit: Sorry, I interpreted the compare wrong on Github (reverse order).

@nhunzaker
Copy link
Contributor

Updated my comment, interpreted the history wrong on the comparison feature. I think this one might be resolved.

@gaearon gaearon closed this as completed Oct 4, 2017
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

5 participants