Release syntheticEvent.target on the destructor

[koba04]

Currently, SyntheticEvent isn't releasing the target property when it called destructor.
This PR makes the target property null.

Please refer to the following fiddle.

https://jsfiddle.net/koba04/9mdse5h9/

[syranide]

This seems unrelated?

[koba04]

@syranide
No. Some tests depend on this issue's behavior.

https://github.com/facebook/react/blob/master/src/test/__tests__/ReactTestUtils-test.js#L433-L465

I prefer that the SyntheticEvent in TestUtils.Simulate isn't released after the event callback has been invoked.
Should I separate a commit or create an another PR?

[syranide]

I don't know enough about testing and how this is used, but if it affects behavior for the event received by the callbacks then this seems wrong.

[zpao]

We shouldn't do this. It would make Simulate.* event handlers behave differently than they would in the browser where events aren't automatically persisted (eg a handler calls something with the event in a timeout would end up working in tests but fail in real life).

We should fix that and any other tests, something like this.

var foo = {
  onChange = function(e) {
    e.persist();
  }
}
spyOn(foo, 'onChange').andCallThrough();

[koba04]

Thanks. I removed this line and fixed the failed tests!

[quantizor]

This constitutes a potential memory leak, yeah?

[jimfb]

@yaycmyk I don't think it constitutes a memory leak. Persisting the event just means it won't be returned to the synthetic event pool, which should be fine. It will still get garbage collected when all the references are dropped.

[quantizor]

@jimfb I guess I mean if the event target is a node that has been deleted since the event was persisted, the node reference won't be able to be GC'd until it is nulled out or the event is reused as you said. Kind of a leak...

[zpao]

Persisting is generally an anti-pattern so I'm not too concerned about references there. Regardless, when you call persist(), that instance gets marked and it won't be returned to the pool. As a result, that SyntheticEvent will just get GCed like anything else and a good GC should release values as well if they aren't referenced anywhere else.

I think the current state of the world without this changes means we have a memory leak (albeit fixed size, so not an exponentially growing leaking)

[zpao]

We probably want to do this for currentTarget as well.

[koba04]

@zpao I think currentTarget is already nullifiled because it is in the EventInterface property.

• https://github.com/facebook/react/blob/master/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js#L27
• https://github.com/facebook/react/blob/master/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js#L157-L160

[zpao]

Ah yea, good call. Really target should be in that interface too but I get why it's not.

[koba04]

@zpao

Really target should be in that interface too

I think so. DOM3 Event interface has target attribute.
SyntheticEvent has all DOM3 Event interface attributes except target.

https://www.w3.org/TR/DOM-Level-3-Events/#event-interfaces

Why it's not.

Isn't this enough?

diff --git a/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js b/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
index f9d0206..48ba4e8 100644
--- a/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
+++ b/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
@@ -23,6 +23,7 @@ var warning = require('warning');
  */
 var EventInterface = {
   type: null,
+  target: null,
   // currentTarget is set when dispatching; no use in copying it here
   currentTarget: emptyFunction.thatReturnsNull,
   eventPhase: null,
@@ -55,16 +56,17 @@ var EventInterface = {
 function SyntheticEvent(dispatchConfig, targetInst, nativeEvent, nativeEventTarget) {
   this.dispatchConfig = dispatchConfig;
   this._targetInst = targetInst;
-
   this.nativeEvent = nativeEvent;
-  this.target = nativeEventTarget;
-  this.currentTarget = nativeEventTarget;

   var Interface = this.constructor.Interface;
   for (var propName in Interface) {
     if (!Interface.hasOwnProperty(propName)) {
       continue;
     }
+    if (propName === 'target') {
+      this.target = nativeEventTarget;
+      continue;
+    }
     var normalize = Interface[propName];
     if (normalize) {
       this[propName] = normalize(nativeEvent);
@@ -160,7 +162,6 @@ assign(SyntheticEvent.prototype, {
     this.dispatchConfig = null;
     this._targetInst = null;
     this.nativeEvent = null;
-    this.target = null;
   },

 });

[zpao]

Yea, that looks right. Seems like we shouldn't have been setting currentTarget in the c'tor anyway.

I get why it's not

I just meant that it feels awkward to have to check for === 'target' every time. We could probably move it into the else case below where you have it and then drop the continue.

[koba04]

@zpao Thanks! I updated this.

[facebook-github-bot]

@koba04 updated the pull request.

[facebook-github-bot]

@koba04 updated the pull request.

[zpao]

Looks great, thanks!

[koba04]

Thanks!