Maximum Call Stack Exceeded When Destroying a nested model #476

Merged
merged 3 commits into from Dec 11, 2013

Conversation

Projects
None yet
5 participants
@daffl
Contributor

daffl commented Dec 11, 2013

@ghost ghost assigned andykant Sep 12, 2013

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Sep 12, 2013

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.

Contributor

andykant commented Sep 12, 2013

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Sep 12, 2013

Contributor

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

Contributor

daffl commented Sep 12, 2013

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

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Sep 12, 2013

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.

Contributor

andykant commented Sep 12, 2013

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Sep 12, 2013

Contributor

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

Contributor

daffl commented Sep 12, 2013

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

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Sep 13, 2013

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Sep 13, 2013

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"

Contributor

thecountofzero commented Sep 13, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Sep 14, 2013

Contributor

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

Contributor

andykant commented Sep 14, 2013

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

@thecountofzero

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Sep 14, 2013

Contributor

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

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

thecountofzero commented Sep 14, 2013

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

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

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Sep 14, 2013

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.

Contributor

thecountofzero commented Sep 14, 2013

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Sep 14, 2013

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Sep 14, 2013

Contributor

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

Contributor

andykant commented Sep 14, 2013

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 15, 2013

Contributor

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

Contributor

justinbmeyer commented Sep 15, 2013

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

@thecountofzero

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Sep 20, 2013

Contributor

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

Contributor

thecountofzero commented Sep 20, 2013

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

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Oct 16, 2013

Contributor

Is this fixed in 2.0.0-pre?

Contributor

daffl commented Oct 16, 2013

Is this fixed in 2.0.0-pre?

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 15, 2013

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Dec 1, 2013

Contributor

This issue has resurfaced in 2.0.3 (or earlier).

Help me Obi-bitovi...

Contributor

thecountofzero commented Dec 1, 2013

This issue has resurfaced in 2.0.3 (or earlier).

Help me Obi-bitovi...

@thecountofzero

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Dec 3, 2013

Contributor

This should be re-opened

Contributor

thecountofzero commented Dec 3, 2013

This should be re-opened

@daffl daffl reopened this Dec 3, 2013

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 3, 2013

Contributor

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

Contributor

justinbmeyer commented Dec 3, 2013

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

@thecountofzero

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Dec 3, 2013

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.

Contributor

thecountofzero commented Dec 3, 2013

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

This comment has been minimized.

Show comment
Hide comment
Contributor

thecountofzero commented Dec 5, 2013

@scorphus

This comment has been minimized.

Show comment
Hide comment
@scorphus

scorphus Dec 9, 2013

Contributor

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.

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

Merge pull request #476 from bitovi/serialize-global-476
Maximum Call Stack Exceeded When Destroying a nested model

@daffl daffl merged commit 7f83be6 into master Dec 11, 2013

1 check passed

default The Travis CI build passed
Details

@daffl daffl deleted the serialize-global-476 branch Dec 11, 2013

+test('Maximum call stack size exceeded with global models (#476)', function() {
+ stop();
+
+ window.Character = can.Model.extend({

This comment has been minimized.

@justinbmeyer

justinbmeyer Dec 12, 2013

Contributor

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

@justinbmeyer

justinbmeyer Dec 12, 2013

Contributor

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

This comment has been minimized.

@daffl

daffl Dec 12, 2013

Contributor

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 Dec 12, 2013

Contributor

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).

+ }
+ }, {});
+
+ window.Game.findOne({id: 'LOZ'}).then(function(g) {

This comment has been minimized.

@justinbmeyer

justinbmeyer Dec 12, 2013

Contributor

Is a findOne necessary? Could new be used instead?

@justinbmeyer

justinbmeyer Dec 12, 2013

Contributor

Is a findOne necessary? Could new be used instead?

@daffl daffl referenced this pull request Dec 22, 2013

Merged

2.0.4 test fixes #625

6 of 7 tasks complete
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 22, 2013

Contributor

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

Contributor

justinbmeyer commented Dec 22, 2013

@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