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

Prevent infinite recursion on unbindAndTeardown when Observe's _bindings is undefined and Observe is self-referential #461

Merged
merged 1 commit into from Sep 19, 2013

Conversation

Projects
None yet
4 participants
@bmomberger-reciprocity
Contributor

bmomberger-reciprocity commented Aug 16, 2013

Previously in util/bind/bind.js

                this._bindings--;
                // If there are no longer any bindings and
                // there is a bindteardown method, call it.
                if(!this._bindings){

If _bindings was undefined, this sets it to NaN and then does the teardown. If in the teardown this object is touched again, having NaN for bindings will decrement to NaN again, still be falsy, and run the teardown again (eventually leading to a stack fault).

The code in this pull request sets _bindings to 0 if it is undefined. Any subsequent visit to unbindAndTeardown will then decrement it to -1 or less, which is okay because we've already started the teardown process.

Preventing infinite recursion on unbindAndTeardown by ensuring that _…
…bindings is always a number when incremented/decremented
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Aug 16, 2013

Contributor

Thanks brad!

Justin Meyer
847-924-6039

On Fri, Aug 16, 2013 at 3:03 PM, Brad (Bradley) Momberger <
notifications@github.com> wrote:

Previously in util/bind/bind.js

            this._bindings--;
            // If there are no longer any bindings and
            // there is a bindteardown method, call it.
            if(!this._bindings){

If _bindings was undefined, this sets it to NaN and then does the
teardown. If in the teardown this object is touched again, having NaN for
bindings will decrement to NaN again, still be falsy, and run the teardown
again (eventually leading to a stack fault).

The code in this pull request sets _bindings to 0 if it is undefined. Any
subsequent visit to unbindAndTeardown will then decrement it to -1 or less,

which is okay because we've already started the teardown process.

You can merge this Pull Request by running

git pull https://github.com/bmomberger-reciprocity/canjs self-limiting-unbind

Or view, comment on, or merge it at:

#461
Commit Summary

  • Preventing infinite recursion on unbindAndTeardown by ensuring that
    _bindings is always a number when incremented/decremented

File Changes

  • M util/bind/bind.jshttps://github.com/bitovi/canjs/pull/461/files#diff-0(8)

Patch Links:

Contributor

justinbmeyer commented Aug 16, 2013

Thanks brad!

Justin Meyer
847-924-6039

On Fri, Aug 16, 2013 at 3:03 PM, Brad (Bradley) Momberger <
notifications@github.com> wrote:

Previously in util/bind/bind.js

            this._bindings--;
            // If there are no longer any bindings and
            // there is a bindteardown method, call it.
            if(!this._bindings){

If _bindings was undefined, this sets it to NaN and then does the
teardown. If in the teardown this object is touched again, having NaN for
bindings will decrement to NaN again, still be falsy, and run the teardown
again (eventually leading to a stack fault).

The code in this pull request sets _bindings to 0 if it is undefined. Any
subsequent visit to unbindAndTeardown will then decrement it to -1 or less,

which is okay because we've already started the teardown process.

You can merge this Pull Request by running

git pull https://github.com/bmomberger-reciprocity/canjs self-limiting-unbind

Or view, comment on, or merge it at:

#461
Commit Summary

  • Preventing infinite recursion on unbindAndTeardown by ensuring that
    _bindings is always a number when incremented/decremented

File Changes

  • M util/bind/bind.jshttps://github.com/bitovi/canjs/pull/461/files#diff-0(8)

Patch Links:

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Aug 30, 2013

Contributor

Looks good to me. What was the case when this is happening? Should we add a test for it?

Contributor

daffl commented Aug 30, 2013

Looks good to me. What was the case when this is happening? Should we add a test for it?

@airhadoken

This comment has been minimized.

Show comment
Hide comment
@airhadoken

airhadoken Sep 10, 2013

Contributor

It was happening in /reciprocity/ggrc-core until we applied the fix, but I am having trouble distilling the problem down into a simple fiddle. I think it might be in part due to us overriding can.Model.model.

Contributor

airhadoken commented Sep 10, 2013

It was happening in /reciprocity/ggrc-core until we applied the fix, but I am having trouble distilling the problem down into a simple fiddle. I think it might be in part due to us overriding can.Model.model.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Sep 19, 2013

Contributor

Ok. Merging this in since it doesn't seem to break any of the existing tests.

Contributor

daffl commented Sep 19, 2013

Ok. Merging this in since it doesn't seem to break any of the existing tests.

daffl added a commit that referenced this pull request Sep 19, 2013

Merge pull request #461 from bmomberger-reciprocity/self-limiting-unbind
Prevent infinite recursion on unbindAndTeardown when Observe's _bindings is undefined and Observe is self-referential

@daffl daffl merged commit 7725939 into canjs:master Sep 19, 2013

1 check passed

default The Travis CI build passed
Details
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 20, 2013

Contributor

I bet calling unbind without calling bind might do it.

Sent from my iPhone

On Sep 19, 2013, at 4:24 PM, David Luecke notifications@github.com wrote:

Ok. Merging this in since it doesn't seem to break any of the existing tests.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Sep 20, 2013

I bet calling unbind without calling bind might do it.

Sent from my iPhone

On Sep 19, 2013, at 4:24 PM, David Luecke notifications@github.com wrote:

Ok. Merging this in since it doesn't seem to break any of the existing tests.


Reply to this email directly or view it on GitHub.

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