Drupal Ajax objects and settings grows endlessly

Suppose code that executes commands should remove the item from that array after processing

Suppose code that executes commands should remove the item from that array after processing

+1

… but only if the element is removed from the DOM. In other words: we have Drupal.behaviors.AJAX.attach(), but not Drupal.behaviors.AJAX.detach(). If we'd add a detach() implementation, this problem would be solved automatically.

First attempt...

Patch #9 tested:

1. Afer ajax call repeatedly DOW crowning.
2. According to this issue https://www.drupal.org/node/1090592 we must have data-attribute instead of ID. But we have data-atribute & ID.

You need to review the issue at hand. The fact that AJAX inserts divs inside divs or the lack of data attributes is not in the scope of this issue.

Second attempt! Tested manually and it seems to work...

I wasn't sure how to use detach() for this, since the code I wrote needs to run after the AJAX stuff has completed. So instead I added a clean() method to Drupal.behaviors.AJAX, which is called in the attach() method.

+++ b/core/misc/ajax.js
@@ -81,6 +84,19 @@
+      $.each(expired, function (key) {
+        delete ajax[key];
+      });

Tested on my machine. The keys were not being deleted. I noticed that the key variable is a index, but the keys in the ajax object are the selector names. So maybe it should be delete ajax[value];

Other than that, the patch in #13 works. I fixed the problem mentioned in #14. I just don't know if we should use detach() instead of calling clean in attach();

IMO it should be in detach, that's the whole reason of its existence.

In detach it will only run when content is removed from the page and that's when you need it to run.

If it's in the attach it will run every time content is added, even if nothing was removed and maybe nothing will be removed.

I uploaded the wrong patch in #15.

@tic2000: If you look at the code, you'll see that it cleans up by determining if the AJAX element actually exists on the page. So the question is -- does detach() run before the element has been removed, or after?

I tested this patch. If it's in the attach it gives false positives because the element may be added to the page later than ajax.js attach.

I also tested with the code moved into detach.

I clicked on the links for "Field Type" for body, comment and dummy text. Then on Edit and Delete for dummy text. All of them are ajax links and add a form to the page. Which means 5 in total as in first image.

This is the delete form with the submit also using AJAX. It removes the field, but more importantly for this issue, the 3 forms related to dummy field.

After the click we see that only 2 objects remain in drupalSettings.ajax which is what I expected.

It can be done with this way. (or after AjaxCommands is executed)

Not existing in DOM === Safe to remove drupalSettings.ajax

Is it true all time ?

One problem with the patch in #21 is that it will only be executed if a settings command comes back from the server. If one doesn't, drupalSettings.ajax will continue to grow and grow. I feel like the cleanup should be done as often as possible, but please correct me if I'm (probably) wrong about that :)

Drupal has detach for a reason. If you put the code in detach you know it will be run when something is removed and not every time an AJAX call is made, even if it removes nothing from the page.

Is it ok that each element is looping through AjaxSettings? That will guarantee no leaks, but couldn't each element clean up after themselves? It probably doesn't matter performance wise, but it could. I don't think I've used enough AJAX that it would matter.

This are added by Form API every time it renders an element with ajax attached. Detaching after themselves is possible in theory, but not that easy and would mean a lot of duplicated code in contrib and in core. For example file module in core would need to check the id's added by uploads and remove them when a user decides he want to remove. The contrib media module would need to do the same. field_ui in core for the widgets on the manage display pages, etc.

Moved into detach().

Added a small protection against the possibility that drupalSettings.ajax will not exist in detach().

I tested this as per #2561619-19: Drupal Ajax objects and settings grows endlessly.

To me this looks like a bug that should be fixed in 8.0.x as well as 8.1.x - @andypost was there a reason to move this to 8.1.x?

Assigning to nod_ for a javascript sub-system maintainer review.

EDITED

Drupal.behaviors.detach is a standard way to detach things from Drupal.behaviors.attach. I don't think it's suitable in this case.

I'd expect Drupal.behaviors.Ajax.detach will be detached all Drupal.Ajax stuff. eg:

drupalSetting.ajax = {}
// and furthermore clean up actions.

Drupal.Ajax things binded from loadAjaxBehavior(). Instead of calling it as `Detach`, personally I'd say this is`Swapping over the setting (due to bad design of Drupal Core)` (although Drupal Core does not sync it)

We needed another feedback from Ajax & JS Maintainers.

Furthermore, no matter put the code in where, I think the cleanSettings function in #21 is more effectively and readability. and in #21, it skipped all new attach drupalSettings. I think that could be in some way `detach` is called before the elements insert into DOM.

+++ b/core/misc/ajax.js
@@ -81,6 +81,19 @@
+      var ajax = drupalSettings.ajax || {};

for example, this line should not exist. If nothing, we do not execute it.

Doing things on attach means you run them all the time. On top of that as I said in my previous review you will get false positives cause the settings is the first of the commands. Only then new content is added, so you end up with id's removed that shouldn't be removed and I don't thing doing that in a timeout helps, but even if it did, why do it when we have detach?.

.detach() is the cleanup for attach. Yes, detach should detach all Ajax stuff for the piece of content removed. It doesn't cause it didn't exist. Now we want to add it and start with settings. So I don't think the point that it doesn't do all it should do, it should do nothing.

.detach() is called when content on the page are remove. Content that might have IDs we need to remove. That's why it should be there and not on a piece of code that runs on every AJAX call, no matter if you remove nothing from the page.

+++ b/core/misc/ajax.js
@@ -81,6 +81,19 @@
+      var ajax = drupalSettings.ajax || {};

This exists cause drupalSettings.ajax exist only when you have IDs added. If you don't have a form with #ajax attached to them it will not exist and it will throw a JavaScript error. Of course it could wrap everything in an if. Either works.

Patch #21 may not work in some conditions, it needed tests for sure. However if we put it after AjaxCommands, it's similar to `.detach()`.

The difference is:
ONE is called by Drupal AJAX system. At least you know you're doing some Ajax works now.
Another one can be executed by any another scripts at anytime. If we work with 3rd party scripts to detach DOM temporarily, any Drupal.detachBehaviors executions would remove all settings. I don't think it's what we expected. (also, running in non-scoped env)

IMO, both are not safe. that's why I raised the question in #21

Here're 2 problems:

1. We didn't merge the new settings to old settings correctly when doing Drupal Ajax (Insert & Remove). It's main reason caused settings growing up.
2. Zombie settings when the DOM is detached. ( Should we do auto-gc globally ? )

Both #21 & #27 fixing #2 in loose way.

We're talking about drupalSettings.ajax getting out of hand but it's nothing next to how Drupal.ajax.instances get out of hand. sure it doesn't slow down attachbehaviors but it does eat up memory. Opening a few modals on the Views ui makes Drupal.ajax.instances go to 500+ objects very easily.

Turned the problem around and when a detach:unload happens, script checks all the Ajax object created and prune all objects for which the instance.element DOM element is not attached in the DOM anymore. Same things for settings, if one object is marked as pruned and it references settings in drupalSettings.ajax, remove the settings from there.

The graphs don't show it very clearly but the memory usage tends to be lower.

There was an issue with views preview (at least) removal of settings was too agressive.

Now settings are cleaned up in the settings command and objects are removed in the detach behavior.

interdiff is not very useful on two quick patches (just ignore #33) but here it is.

I tested this code. It does set to null all the correct instances, but I don't know how to check if gc gets them or not.

Memory and GC are secondary concerns for this issue I'd say. having null is at least better than before.

Patch #35 clean up the zombie furthermore. I only wonder why @nod_ chose a loose (less aggressive) way to clean up setting. (compare to #21)

Mainly because it relies on setTimeout to work, I prefer not messing with the execution queue.

Looks good to me. I pinged cottser in case he wants to take a look before it goes in but otherwise will try to get it in this week.

+++ b/core/misc/ajax.js
@@ -81,6 +81,15 @@
+          // Let the GC take it.

I'm not super familiar with all the Ajax guts but I think we can spell out GC here or just tweak this comment a bit. Leaving at RTBC.

1. +++ b/core/misc/ajax.js
   @@ -81,6 +81,15 @@
   +    detach: function (context, settings, trigger) {
   

   Just realised this should really have jsdoc.

2. +++ b/core/misc/ajax.js
   @@ -81,6 +81,15 @@
   +          // Let the GC take it.
   

   Something like 'Set this to null and allow garbage collection to reclaim the memory'.

Interdiff failed me, only made doc changes.

The behavior doc might look strange, but it's been agreed upon, see http://read.theodoreb.net/drupal-jsapi/Drupal.behaviors.html for what it looks like.

+++ b/core/misc/ajax.js
@@ -19,6 +19,14 @@
+   *
+   * @prop {Drupal~behaviorAttach} attach
+   *   Initialize all {@link Drupal.Ajax} objects declared in
+   *   `drupalSettings.ajax` or initialize {@link Drupal.Ajax} objects from
+   *   DOM elements having the `use-ajax-submit` or `use-ajax` css class.
+   * @prop {Drupal~behaviorDetach} detach
+   *   During `unload` remove all {@link Drupal.Ajax} objects related to
+   *   the content removed.

@@ -1030,6 +1063,8 @@
+     * This method will also remove expired `drupalSettings.ajax` settings.
+     *

These are the new bits. i.e. this is the interdiff.

Only nit: the content removed -> the removed content. Can be fixed on commit.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!

+++ b/core/misc/ajax.js
@@ -237,11 +255,26 @@
+  Drupal.ajax.expired = function () {
+    return Drupal.ajax.instances.filter(function (instance) {
+      return instance && instance.element !== false && !document.body.contains(instance.element);
+    });
+  };

I get an error in TMGMT due to this change.

Here 2 screenshots of the error, not sure why this happens, I'll provide a simplytest link.

We checked Views UI but not the ajax behavior of a view itself.

Patch should fix it. Our docs do say element should be an HTMLElement, not a jquery object http://read.theodoreb.net/drupal-jsapi/Drupal.html#.ajax

Tested manually, it works fine now.

So setting it to RTBC.

Let's fix it in a followup #2673824: Views JS passing wrong type of object to Drupal.ajax.

Rolled back in 8.0.x - but committed the follow-up to 8.1.x

For this to go into 8.0.x I think we'd need more manual testing and/or scanning contrib code bases for similar errors.

This also caused this issue: #2705327: Failed to execute 'contains' on 'Node'

Not sure if this is a duplicate of the above, but this also caused #2707631: Views AJAX pagination broken after the first page..

Are there any plans for a port for D7?