Fix two memory leaks + subtle bugs in Edit's JS (discovered by working on the Backbone upgrade in the D7 backport) [#2159965]

Memory leaks

There's a memory leak in EntityModel (not unbinding from listening to its own events) and a much more important one in EntityToolbarView (if you in-place edit once, and then again (without reloading the page), then the first in-place editing session's EntityToolbarView still lingers and is reacting to things, even though it's invisible).

Subtle bugs

Details (this is copied verbatim from #2159953: Fix subtle bugs discovered while working on the Backbone upgrade, which is where I fixed these subtle bugs in the D7 backport):

While working on #2149237: Upgrade from Backbone 0.9.2 to >=1.0.0 and from Underscore 1.4.0 to >=1.5.0, I found several subtle bugs (most of which were not causing any problems for the end user):

There are still a few cases where a model attribute was being changed in response to another attribute change without waiting for the first change to be propagated first.

When the Entity model enters the 'committing' state, it transitions non-candidate-ish fields to the 'candidate' state. That's good. But it also triggers a 'change:state' event on those fields that are already in a candidate-ish state. That's unnecessary, because a user can only save an entity if changes have been made, and once that happens, it's impossible to not have an in-place editor open, which by definition prevents you from having *only* candidate-ish fields. So at least one field will be transitioned to the 'candidate' state, and hence we don't have to trigger the 'change:state' event on candidate-ish fields.

While the Entity model is in the 'committing' state, a field may be saved, but in that case, the 'inTempStore' attribute won't be set. While in the 'opened' state, that *does* happen. It should *also* happen while in the 'committing' state.

The Entity and Field models' validate() method tried to return early if another attribute than the state one was being changed. But that code was pointless: Backbone does not support this (it doesn't want to: it wants you to always validate the entire thing).

Only for one View, there was still Edit module-namespaced events in the event map. This is pointless: Backbone already assigns a dynamically generated, unique event namespace to ensure proper unbinding.

Comment	File	Size	Author
#11	leak-plugged-10.png	109.13 KB	jessebeach
#10	Home___Drupal_D8_Dev_and_d8_—_bash_—_bash_—_Ocean_—_154×41_—_⌘1-9.png	109.13 KB	jessebeach
#9	interdiff.txt	3.53 KB	wim leers
#9	edit_memleaks_subtle_bugs-2159965-9.patch	14.29 KB	wim leers
#7	Developer_Tools_-_http___d8_drupal_dev_node_-4.png	106.03 KB	jessebeach
#7	Developer_Tools_-_http___d8_drupal_dev_node__and_Drupal_Core__D8__-_Status__No_Local_Changes_-5.png	113.27 KB	jessebeach
#7	Developer_Tools_-_http___d8_drupal_dev_node_.png	162.5 KB	jessebeach
#7	Developer_Tools_-_http___d8_drupal_dev_node__and_Drupal_Core__D8__-_Status__No_Local_Changes_-4.png	113.92 KB	jessebeach
#6	edit-memory-leak-2159965-6.patch	15.18 KB	jessebeach
#4	edit_memleaks_subtle_bugs-2159965-4.patch	14.06 KB	wim leers

Comments

Comment #1

wim leers

Ghent 🇧🇪🇪🇺

commented 19 December 2013 at 16:12

Title:	Fix subtle Edit bugs (discovered by working on the Backbone upgrade in the D7 backport)	» Fix two memory leaks + subtle bugs in Edit's JS (discovered by working on the Backbone upgrade in the D7 backport)
Category:	Task	» Bug report
Issue summary:	View changes
Priority:	Normal	» Major

Comment #2

wim leers

Ghent 🇧🇪🇪🇺

commented 19 December 2013 at 16:13

Issue tags:

+JavaScript

Comment #3

wim leers

Ghent 🇧🇪🇪🇺

commented 19 December 2013 at 16:13

Issue summary:

View changes

Comment #4

wim leers

Ghent 🇧🇪🇪🇺

commented 19 December 2013 at 16:14

Status:

Active

» Needs review

Status	File	Size
new	edit_memleaks_subtle_bugs-2159965-4.patch	14.06 KB

Comment #5

19 December 2013 at 16:16

Status:

Needs review

» Needs work

The last submitted patch, 4: edit_memleaks_subtle_bugs-2159965-4.patch, failed testing.

Comment #6

jessebeach commented 22 December 2013 at 21:53

Status:

Needs work

» Needs review

Status	File	Size
new	edit-memory-leak-2159965-6.patch	15.18 KB

Rerolled. Wim, please check AppView::renderUpdatedField to make sure I integrated the changes correctly with the changes from #2075185: When an entity is in-place edited (i.e. saved), other instances of that entity on the same page are not updated (no propagation).

Comment #7

jessebeach commented 23 December 2013 at 18:57

Status	File	Size
new	Developer_Tools_-_http___d8_drupal_dev_node__and_Drupal_Core__D8__-_Status__No_Local_Changes_-4.png	113.92 KB
new	Developer_Tools_-_http___d8_drupal_dev_node_.png	162.5 KB
new	Developer_Tools_-_http___d8_drupal_dev_node__and_Drupal_Core__D8__-_Status__No_Local_Changes_-5.png	113.27 KB
new	Developer_Tools_-_http___d8_drupal_dev_node_-4.png	106.03 KB

Ok, let's first prove the problem of zombie DOM elements exists. We'll do this by watching DOM node activity on the Timeline in Chrome. This is the procedure:

Clear the timeline and force a garbage collection cycle (baseline)
Open a node to quick edit
Close quick editing
Force a garbage collection cycle
Open a node to quick edit
Close quick edit
Force a garbage collection cycle
Count the number of nodes present at the start and at the end; take the difference.

The following screenshot shows the sawtooth Timeline graphs of the steps (noted above) annotated on the graph.

Without the patch applied, we see a 32 node increase between editing activity.

With the patch, we see an 8 node increase. This, I presumed, was the entity toolbar that gets reused across editing instances.

We can prove that the increase is nodes is due to Edit module comparing Heap snapshots from before and after quick edit sessions. The following four screenshots of the Chrome developer tools Heap Profiler show:

Editing one node, before and after quick edit, without the patch applied
Editing one node, before and after quick edit, with the patch applied
Editing two nodes in succession, before and after quick edit, without the patch applied
Editing two nodes in succession, before and after quick edit, with the patch applied

The conclusion is that the patch eliminates the accumulation of detached DOM nodes. The one DOM node that is added after a quick edit is launched is the element associated with the EntityToolbarView which is detached from the DOM when quick edit is cancelled, but it is not removed from memory so that the DOM can be reused.

Since I can't open Dreditor from the issue edit screen, I'll post this comment and then do a quick code review in a subsequent comment.

Comment #8

jessebeach commented 22 December 2013 at 22:43

+++ b/core/modules/edit/js/models/EntityModel.js
@@ -245,43 +276,22 @@ Drupal.edit.EntityModel = Backbone.Model.extend({
-        // If the fieldModel changed to the 'saved' state: remember that this

+++ b/core/modules/edit/js/models/FieldModel.js
@@ -82,7 +82,7 @@ Drupal.edit.FieldModel = Backbone.Model.extend({
-    Backbone.Model.prototype.destroy.apply(this, options);
+    Backbone.Model.prototype.destroy.call(this, options);

How did this ever work if options is an object? You'd think it would have just errorred out. Bizarre.

Code changes look good. Manual testing shows no regressions. jsHint comes up clean.

Wim, can you just verify my reroll, the code I mentioned in comment #6?

Comment #9

wim leers

Ghent 🇧🇪🇪🇺

commented 23 December 2013 at 13:37

Status	File	Size
new	edit_memleaks_subtle_bugs-2159965-9.patch	14.29 KB
new	interdiff.txt	3.53 KB

6 files were hidden/shown/deleted

Status	File	Size
hidden	edit_memleaks_subtle_bugs-2159965-4.patch	14.06 KB
hidden	edit-memory-leak-2159965-6.patch	15.18 KB
hidden	Developer_Tools_-_http___d8_drupal_dev_node__and_Drupal_Core__D8__-_Status__No_Local_Changes_-4.png	113.92 KB
hidden	Developer_Tools_-_http___d8_drupal_dev_node_.png	162.5 KB
hidden	Developer_Tools_-_http___d8_drupal_dev_node__and_Drupal_Core__D8__-_Status__No_Local_Changes_-5.png	113.27 KB
hidden	Developer_Tools_-_http___d8_drupal_dev_node_-4.png	106.03 KB

Thanks for all the work you put into this thorough review, Jesse! :)

#8: it worked fine, as long as options was empty. And that was almost always the case.

#6: it's very close, but a logic error has unfortunately slipped in: in the current code, the field model is always destroyed. After your changes, it's only being applied when propagation is enabled, which is incorrect — you've ironically introduced a new memory leak :D
However, I don't like my fix: the code is now pretty hard to follow. Do you see how you can make it simpler?

Comment #10

jessebeach commented 23 December 2013 at 17:46

Status:

Needs review

» Reviewed & tested by the community

Status	File	Size
new	Home___Drupal_D8_Dev_and_d8_—_bash_—_bash_—_Ocean_—_154×41_—_⌘1-9.png	109.13 KB

You were right, Wim, my reroll did introduce another leak. The +8 nodes I saw in my original testing. I've retested your correction in the reroll and now we see a return to the baseline established after the first node is quick edited.

We start with 783 nodes, quick edit a node, end up with 795 nodes (the Entity Toolbar DOM fragment still in memory for re-use) and after editing another node, the node count drops back down to 795 again. Leak plugged.

The code looks great. Let's get this leak fix in.