ReactMount.nodeCache is repopulated with purged nodes that are focused at unmount

[syranide]

If you unmount a currently focused node, nodeCache is repopulated after being purged and unless that specific ID is revisited later, it will remain there forever. This is mostly likely due to the focus/selection restoration phase after reconciliation.

Repro: http://dev.cetrez.com/jsx/nodecache.html (nodeCache is output into the console)

[syranide]

I'll go ahead and put good first bug on this one, it may be a bit tricky to find the root cause but it should be quite easy to narrow down and the fix should be rather simple too I think. So perhaps not the easiest bug if you're entirely new.

[javpaw]

Hi syranide, I'd like to take a look but the link is not working, can you check it please?

[syranide]

@javpaw I can have it fixed in an hour or so, but really, just change var nodeCache = {} to var nodeCache = window.nodeCache = {} and run that code against your own React build and you'll see it.

[syranide]

Fixed.

[pletcher]

After a bunch of experimentation related to the fix in #3144, I was able to trace this issue to EventListener, and specifically to the fact that event the default event handlers (added as part of ReactBrowserEventEmitter.listenTo) aren't removed when unmounting.

It looks like when an <input> is mounted, it attaches listeners for focus, blur, etc.

https://github.com/facebook/react/blob/master/src/renderers/dom/client/ReactBrowserEventEmitter.js#L271-L275

ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
  topLevelTypes.topBlur,
  'blur',
  mountAt
);

Which leads us to https://github.com/facebook/react/blob/master/src/renderers/dom/client/ReactEventListener.js#L145-L155

trapCapturedEvent: function(topLevelType, handlerBaseName, handle) {
  var element = handle;
  if (!element) {
    return null;
  }  
  return EventListener.capture(
    element,
    handlerBaseName,
    ReactEventListener.dispatchEvent.bind(null, topLevelType)
  );
},

and finally to https://github.com/facebook/react/blob/master/src/shared/vendor/stubs/EventListener.js#L74-L79

target.addEventListener(eventType, callback, true);
return {
  remove: function() {
    target.removeEventListener(eventType, callback, true);
   }
};

So a remove function is being returned, it's just not getting used. I think if we exposed remove as callable (and called it before the component is removed from the DOM), we'd be in the clear.

I'm not sure where best to handle this, given that I'm fairly new to React's source and it's sort of massive. Are there any conventions in use or places that I should look?

[sophiebits]

@pletcher You're right that those aren't removed, but they're added only once per document at the top level so that shouldn't really be a problem.

[pletcher]

Right right, but it seems like there's a handler getting attched that's firing the blur event that causes the node to get recached when it's unmounted (or at least, that's what the stack points to when I climb through it) -- the browser's native handling of removing an input component is triggering this handler.

[sophiebits]

Yes; the fix is probably to change React's event handling to not fire events on removed nodes. (If another root is still mounted, we can't remove the top-level handler anyway.) cc #3298, #3790

[jimfb]

@spicyj I think we intentionally fire events on removed nodes. I'm pretty sure there was a unit test for that, ensuring that we fire an event on nodes even if they are removed as part of an event handler as the event bubbles up.

[jimfb]

Yeah, https://github.com/facebook/react/blob/master/src/renderers/dom/client/__tests__/ReactEventListener-test.js#L93

Spec says it should not get confused by disappearing elements, which I suppose is sufficiently vague as to imply any non-fatal but still undefined behavior :).

[sophiebits]

Yeah, goes back to #1105.

[nhunzaker]

Looks like nodeCache was killed in 4ba0e95. Is this still an issue?

[sophiebits]

Nope, thank you.