Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Infinite loop in Controller method: remove() #34

Open
mohsentaleb opened this issue Nov 11, 2018 · 3 comments
Open

Infinite loop in Controller method: remove() #34

mohsentaleb opened this issue Nov 11, 2018 · 3 comments

Comments

@mohsentaleb
Copy link
Contributor

mohsentaleb commented Nov 11, 2018

Consider I'm removing a sub-controller (created with sub), it goes through this function:
https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L261
which then releases the controller: https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L264
and then triggers a release event:
https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L324
which ultimately runs this line because it's listening to a release event:
https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L253
And again goes through remove function. But since it's got a {skip_release: true} option it wont make an infinite loop.
I've re-created a scenario here: https://codepen.io/anon/pen/qQaPpL?editors=1011

If you run myDashboard.sub('users').remove('inner') in console everything goes perfectly and removes the inner sub-controller.

The problem is, in my real world application this causes an error Uncaught RangeError: Maximum call stack size exceeded.

If I change this line

instance.bind('release', this.remove.bind(this, name, {skip_release: true}));

to this block, it's fixed:

instance.bind('release', function() {
	this.remove(name, {skip_release: true});
}.bind(this));

Do you have any idea what could be the problem here?

@orthecreedence
Copy link
Member

The only diffrence I can see:

In the first version (instance.bind('release', this.remove.bind(this, name, {skip_release: true}))) the event listener uses this.remove at the time of binding the event, not at the time the event is called.

In the second version, the most recent version of this.remove is called, not the version that existed at the time of binding the event.

So, the following would play out like this:

this.remove = function() { console.log('release v1'); };
this.bind('release', this.remove.bind(this, name, {skip_release: true}));
this.bind('release', function() {
	this.remove(name, {skip_release: true});
}.bind(this));

this.trigger('release');
// release v1
// release v1
this.remove = function() { console.log('release v2'); };
this.trigger('release');
// release v1
// release v2
this.remove = function() { console.log('release v3'); };
this.trigger('release');
// release v1
// release v3

My first idea is that this.remove is being overwritten somewhere with a version that doesn't loop endlessly. Are there any references to this.remove in the controller itself?

@mohsentaleb
Copy link
Contributor Author

Thanks for the explanation, but I'm afraid that's not the case. I put a console.log(name); in composer's remove function and it correctly prints the name of my controller by which I initialized with sub(). That means the correct remove function is called.

remove: function(name, options) {
	options || (options = {});
	if(!this._subcontrollers[name]) return;
	console.log(name);
	if(!options.skip_release) this._subcontrollers[name].release();
	delete this._subcontrollers[name];
}

output:

> App.controllers.pages.currentController.sub('appBarController').remove('billingController')
billingController       composer.js?nocache=872:2299
billingController       composer.js?nocache=872:2299
undefined

@orthecreedence
Copy link
Member

Hi, Mohsen. Sorry for the delay in getting back to you.

I've tried quite a bit to reproduce this and I'm not able to. I wonder if you made a simple test case and then bundled it using the same minification process you use in production if it could be reproduced.

Would that be feasible?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants