Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maximum Call Stack Exceeded When Destroying a nested model #476

Merged
merged 3 commits into from
Dec 11, 2013

Conversation

daffl
Copy link
Contributor

@daffl daffl commented Dec 11, 2013

@ghost ghost assigned andykant Sep 12, 2013
@andykant
Copy link
Contributor

This seems to be an issue with the can.Observe attributes serialize method conflicting with can.Observe.List's serialize method. When attributes get serialized, they're checked against a stack of already serialized observes to prevent infinite loops, but can.Observe.List.prototype.serialize seems to bypass this.

@daffl
Copy link
Contributor Author

daffl commented Sep 12, 2013

Wouldn't this just be solvable by using the same stack?

@andykant
Copy link
Contributor

Yeah, I think this could be easily fixed simply by having can/observe/attributes also override can.Observe.List.prototype.serialize in addition to can.Observe.prototype.serialize. I don't have time at the moment to take care of this but I'll fix it soon.

@daffl
Copy link
Contributor Author

daffl commented Sep 12, 2013

I guess I can give it a shot, too if I have time.

@daffl
Copy link
Contributor Author

daffl commented Sep 13, 2013

Odd, now I'm not getting an error in the Fiddles. Could possibly be related with the fact that I forgot to update the release folder with 1.1.7 and only did it a week or two ago. Or am I missing something @thecountofzero?

@thecountofzero
Copy link
Contributor

@daffl, Which fiddle?

When I use the following fiddle:

http://jsfiddle.net/thecountofzero/a2b3S/

And then enter v.destroy() in the console I get the "RangeError: Maximum call stack size exceeded"

@andykant
Copy link
Contributor

It's reproducible with this fiddle: http://jsfiddle.net/a2b3S/5/

@thecountofzero
Copy link
Contributor

Are you saying doing something like this in attributes.js ?

can.Observe.prototype.serialize = can.Observe.List.prototype.serialize = function(attrName, stack) {

@thecountofzero
Copy link
Contributor

If I enter "can.Observe.List.prototype.serialize = can.Observe.prototype.serialize" in the console of the fiddle, it no longer throws the error.

@daffl
Copy link
Contributor Author

daffl commented Sep 14, 2013

I just tried that, too and it seems to work: http://jsfiddle.net/a2b3S/6/
Could you maybe verify that it also destroys properly?

@andykant
Copy link
Contributor

Yeah, that should fix it, but like you mentioned, need to verify that it destroys properly as well as serializes lists properly.

@justinbmeyer
Copy link
Contributor

Anyway someone can reduce this to something I can add as a test?

@thecountofzero
Copy link
Contributor

Try typing "v.attr()" in the console. This will generate the maximum callstack error even with the above proposed fix

@daffl
Copy link
Contributor Author

daffl commented Oct 16, 2013

Is this fixed in 2.0.0-pre?

@daffl
Copy link
Contributor Author

daffl commented Nov 15, 2013

Closing this. Please reopen if it is still happening in the current release (2.0.2).

@daffl daffl closed this Nov 15, 2013
@thecountofzero
Copy link
Contributor

This issue has resurfaced in 2.0.3 (or earlier).

Help me Obi-bitovi...

@thecountofzero
Copy link
Contributor

This should be re-opened

@daffl daffl reopened this Dec 3, 2013
@justinbmeyer
Copy link
Contributor

@thecountofzero if you can reduce those fiddles into something testable by Monday of next week, I'll fix it. Thanks!

@thecountofzero
Copy link
Contributor

See my comments on the forum. I reduced it a bit, but not much. Globals are needed for associations (attributes plugin) and the rest is the bare minimum to exhibit the issue.

@thecountofzero
Copy link
Contributor

@justinbmeyer here is a reduced fiddle.

http://jsfiddle.net/thecountofzero/JTa2M/

@scorphus
Copy link
Contributor

scorphus commented Dec 9, 2013

I'm digging this one too, and in the meanwhile I found out something at least interesting. I'm not sure as to how this adds for the discussion, but anyway, If one deferred FOO is resolved after another BAR, FOO just gets serialized and BAR makes the exception to be thrown. See http://jsfiddle.net/scorphus/ARTQ5/ for a brief demo. Hope it helps.

daffl added a commit that referenced this pull request Dec 11, 2013
Maximum Call Stack Exceeded When Destroying a nested model
@daffl daffl merged commit 7f83be6 into master Dec 11, 2013
@daffl daffl deleted the serialize-global-476 branch December 11, 2013 23:05
test('Maximum call stack size exceeded with global models (#476)', function() {
stop();

window.Character = can.Model.extend({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to use attributes / converters without adding to the window.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interesting thing is that it worked when just passing the instances but the stack overflow happens when it is on the window using the string notation (Game.model, Game.models).

@daffl daffl mentioned this pull request Dec 22, 2013
7 tasks
@justinbmeyer
Copy link
Contributor

@thecountofzero checkout that last commit that shows how to avoid globals and use attributes.

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

Successfully merging this pull request may close these issues.

5 participants