revert + fix($rootScope): fix potential memory leak when removing scope listeners #16358
Conversation
| })); | ||
|
|
||
|
|
||
| it('should allow recursive $emit/$broadcast between parent/child', inject(function($rootScope) { |
There was a problem hiding this comment.
I don't think this test demonstrates recursive emitting/broadcasting.
There was a problem hiding this comment.
How so? It does at least one recursive $broadcast + one $emit within $broadcast...
I'm sure we could think of better or more interesting tests though. Let me know if you have any specific ones you think should be added.
There was a problem hiding this comment.
Yeah, it does root.$broadcast inside root.$broadcast (which we don't care about in this test) and child.$emit inside root.$broadcast, so in a way it is recursive. But it doesn't test for root.$broadcast inside child.$emit, for example.
In order to test both root.$broadcast --> child.$emit and child.$emit --> root.$broadcast, I would change it to something like:
var child = $rootScope.$new();
var calls = '';
$rootScope.$on('evt', function($event, arg0) {
calls += 'r'; // For "root".
if (arg0 === 'fromChild') {
$rootScope.$broadcast('evt', 'fromRoot2');
}
});
child.$on('evt', function($event, arg0) {
calls += 'c'; // For "child".
if (arg0 === 'fromRoot1') {
child.$emit('evt', 'fromChild');
}
});
$rootScope.$broadcast('evt', 'fromRoot1');
expect(calls).toBe('rccrrc');There was a problem hiding this comment.
I updated to use your test
jbedard
force-pushed
the
16135-17-delete
branch
2 times, most recently
from
26a8f97 to
168a641
Compare
|
Acording to the guidelines, the commit message should start with "revert: ..." but I don't think that's consequential. Imo, we should support the default git Revert, too. Doesn't matter though. What I find a bit strange is that the Revert commit comes after the commit with the different fix. I think it's easier to understand if their position is reversed. |
|
I think github is showing them based on the commit date, and the cherry-picked commit has a date in august while the revert is more recent? I'm sure I reverted first, otherwise I would have had a lot of conflicts... |
|
Also, the |
|
What's the angular format for reverts though? |
|
For |
|
Yeah, the commits are correctly ordered when viewed locally. Strange. The commit message header of the revert commit should look like this:
|
…e listeners This reverts commit 817ac56.
When removing listeners they are removed from the array but the array size is not changed until the event is fired again. If the event is never fired but listeners are added/removed then the array will continue growing. By changing the listener removal to `delete` the array entry instead of setting it to `null` browsers can potentially deallocate the memory for the entry. Fixes angular#16135 Closes angular#16161
jbedard
force-pushed
the
16135-17-delete
branch
from
168a641 to
916f3cb
Compare
|
Merged! Thanks @jbedard |
This reverts the 1.7 change, cherry-picks the 1.6 version that had no breaking changes, and adds some tests to prevent such a breaking change again in the future.
See #16293 (comment) for the reason to revert the breaking change.