refactor($compile): remove skipDestroyOnNextJQueryCleanData, fix data leak on some replaced nodes

[jbedard]

This removes the skipDestroyOnNextJQueryCleanData global while also fixing an edge case where some elements removed by replaceWith did not have child elements cleaned and could leak jq data/handlers. It's a bit of an edge case but would be reproduced if jq data ever got onto the child element of a ng-if or ng-repeat before the transclude replaceWith call (or similar situations with replace:true). This is captured in the added tests (3 previously failed). Also added a test ensuring the firstElementToRemove does not trigger the element $destroy.

skipDestroyOnNextJQueryCleanData is removed by doing jqLite(firstElementToRemove).off('$destroy') before doing cleanData(fragment.qSA(*)).

Doing cleanData(fragment.qSA(*)) avoids jqLite(elements[1...n]).remove() for the multi element case while also fixing the bug mentioned above by also fetching child elements. This makes use of the fragment meaning #12041 that avoided creating it would no longer work.

JQLite.cleanData is added so this code doesn't need JQLite vs jQuery specific logic.

For the single node case this adds the .qSA(*) and may add .off('$destroy') but all the benchmarks I tried showed no effect. The replaceWith is just too small/infrequent when doing compile/link/digest.

[lgalfaso]

with the patch, is the issue outlined at the comment about jQuery UI an issue?

[jbedard]

I believe that's covered by this test added in b9389b2. As long as we always reference the current jqLite.cleanData it should be fine.

[jbedard]

Does anyone know if this is actually intentional? Why is this done? It seems like the replaced node was never .remove()ed so this was always the case. But when multi-element directives were added (e46100f) they used .remove() which does fire $destroy.

Should $destroy actually be fired on the replaced node as well?

[mgol]

Did you try to remove the jqLite(firstElementToRemove).off('$destroy'); line and run unit tests? From what I remember the whole skipDestroyOnNextJQueryCleanData was added because the scope is reused on the replaced elements so removing this logic made existing unit tests fail.

[jbedard]

Removing jqLite(firstElementToRemove).off('$destroy'); doesn't cause anything to fail. Even this test designed to fail in that case isn't failing! 😟

I thought of another case though which I think might show that not invoking the DOM $destory is (sometimes) wrong...

<div ng-form="myForm" lower-priority-replace-directive>

In this case the ng-form directive runs first which adds itself to the parent form then does formElement.on('$destroy', function() { parentFormCtrl.$removeControl(controller); ... }). Then the other lower priority directive replaces that element with something brand new, that replaced element is not removed from the ng-form because no $destroy was fired. The replaced ng-form controller left behind a) is leaked and b) might cause form validation issues?

However if that low priority directive was doing transclude instead of replace then maybe not invoking the DOM $destory is valid? Meaning replaceWith would need an invokeDestroy argument...

[mgol]

Interesting; just removing the skipDestroyOnNextJQueryCleanData logic from my commit definitely did fail unit tests.

This might be related, I totally forgot about this WIP PR: #8695

[jbedard]

Was that PR ready or was there more to look into still?

[jbedard]

I'm tempted to just drop this test for now. The cases when $destroy should/should-not fire are too unclear and I think the current implementation is wrong (or at least super weird) so I'm not sure if I want to enforce that with a test. I think all the other tests are good though (ensuring all elements get passed to cleanData).

[jbedard]

@mzgol, this is changing what you originally did for jQuery2. What do you think?

[mgol]

@jbedard I'll take a look on Thursday, I'm on vacation until then.

[mgol]

I wouldn't delete the whole above comment. The following:

        // Remove data of the replaced element. We cannot just call .remove()
        // on the element it since that would deallocate scope that is needed
        // for the new node. Instead, remove the data "manually".

seems still relevant to the .off('$destroy') code.

[jbedard]

I'm not sure if that comment is 100% correct. .remove() doesn't "deallocate [the] scope" it just fires a DOM $destroy event (not the scope $destroy event). It doesn't actually touch the scope...

[mgol]

A few questions/remarks, overall I like the direction of this PR. 👍

[petebacondarwin]

Moving to 1.5 - @mzgol & @jbedard can we try to combine this with #8695?

[mgol]

@petebacondarwin Sure, I'll have a look. It seems all my comments have been addressed so I'll take the last look and it'll most likely be ready to land.