Skip to content

Conversation

@asolove
Copy link
Contributor

@asolove asolove commented Nov 22, 2013

+100 points to @aadsm for this discovery: a bug caused by a strange loop!

drawinghands

The problem

MontageReviver is a subclass of Reviver. When you call MontageReviver#reviveRootObject it calls super, which resolves to Reviver#reviveRootObject. That method then calls other methods on the object, which eventually wind their way around to calling MontageReviver#reviveRootObject again. When we reach the super call the second time, which is still nested inside the call stack for the first time, the super context thinks we're trying to call the parent of Reviver, rather than of MontageReviver, and is thus a no-op.

Possible fixes

  1. Laugh at this strange problem and content ourselves with manually calling Reviver.prototype.reviveRootObject instead of having the convenience of super in this specific case.
  2. Fix super to work in cases like this. When we call super() from inside Class#method, we could wrap Class#method in a decorator. Then if it is called again within itself, the wrapper resets the super context just for the duration of the inner call.

…od recursively calls the same method on a child.
@francoisfrisch
Copy link
Contributor

Fix super to work in cases like this. When we call super() from inside Class#method, we could wrap Class#method in a decorator. Then if it is called again within itself, the wrapper resets the super context just for the duration of the inner call.

Would the decorator be left in place? If it isn't this could be expensive... if it is this could mess up some references.
A could problems that would need to be solved to leave it in place

  1. this.super() uses the reference to the function itself I believe, this will need to be handled.
  2. If the function is assigned to another object's property it will need to be able to detect that and recover.

@asolove
Copy link
Contributor Author

asolove commented Nov 25, 2013

Yeah, I'm not sure if the decorator method is possible. I've been hacking at it for a bit and changing to a wrapped method breaks a lot of other things even before we get to your objections.

In addition, since we maintain the super chain inline with the call stack, I've noticed that super doesn't work with anything async...

kriskowal added a commit to kriskowal/montage that referenced this pull request Jan 3, 2014
These changes fail to fix MON-234 montagejsgh-1348, but in the process add
comments, massage style, and fix theoretical internal consistency
problems introduced by thrown exceptions and nested and repeated super
calls, for which I have provided no evidence of their necessity.
@rayshan rayshan assigned rayshan and unassigned aadsm Feb 25, 2015
@hthetiot hthetiot modified the milestones: v18.x, Future May 1, 2017
@hthetiot hthetiot assigned hthetiot and unassigned rayshan Jul 13, 2017
@hthetiot hthetiot requested a review from cdebost July 13, 2017 02:19
@hthetiot hthetiot modified the milestones: v17.0.x Support, Future Jul 13, 2017
@cdebost cdebost assigned cdebost and unassigned hthetiot Aug 31, 2017
@hthetiot hthetiot modified the milestones: v17.0.x Support, v17.1.x API Enhancements, v17.1.1 Data and webcomponent Sep 8, 2017
@hthetiot hthetiot modified the milestones: v17.1.1 Data and Serialization, v17.1.x API Enhancements Nov 16, 2017
@hthetiot hthetiot removed this from the v18.1.x API Enhancements milestone Dec 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants