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

[bugfix] Prevents the recursive redefinition of root chains #16857

Merged

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Aug 2, 2018

This fix prevents an edge case where recursive call to writableChains
causes the root _chains ChainNode on a meta to be overwritten,
resulting in a chains node that does not match up with the watching
state of the meta.

This can occur when:

  1. A computed property has been setup on a class, which lazily delays
    finalizing chains.
  2. An observer is then added to the class, which eagerly forces its
    dependencies to setup their chains.

In the case observed, it was only triggered because there were some
number of intermediate aliases between the computed and the
observer. Aliases are not volatile, but they also don't use the
computed cache for values so they cannot rely on getCachedValueFor
when setting up chains. This results in eager fetching of computed
values, which leads to the nested recursive call.

This PR solves the problem by ensuring that the root chain node is set
on the meta before we attempt to add any values to it (e.g. with
copy). The copy method has been renamed to copyTo to indicate that
it is not actually creating or returning a copy (open to suggestions
for a better name).

This fix prevents an edge case where recursive call to `writableChains`
causes the root `_chains` ChainNode on a meta to be overwritten,
resulting in a chains node that does not match up with the `watching`
state of the meta.

This can occur when:

1. A computed property has been setup on a class, which lazily delays
finalizing chains.
2. An observer is then added to the class, which eagerly forces its
dependencies to setup their chains.

In the case observed, it was only triggered because there were some
number of intermediate aliases between the `computed` and the
`observer`. Aliases are not volatile, but they also don't use the
computed cache for values so they cannot rely on `getCachedValueFor`
when setting up chains. This results in eager fetching of computed
values, which leads to the nested recursive call.

This PR solves the problem by ensuring that the root chain node is set
on the meta before we attempt to add any values to it (e.g. with
`copy`). The copy method has been renamed to `copyTo` to indicate that
it is not actually creating or returning a copy (open to suggestions
for a better name).
@pzuraq pzuraq force-pushed the bugfix/prevent-redefinition-of-root-chains branch from 9c31649 to b4026e0 Compare August 2, 2018 01:26
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Seems good to me, though I couldn’t imagine being able to figure this out on my own 😜

Adding @krisselden for additional review...

@rwjblue rwjblue requested a review from krisselden August 2, 2018 12:18
assert.expect(0);
function didChange() {}

let obj = {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer using class syntax, and defining the CPs on prototype then calling finish chains in the constructor

Copy link
Member

Choose a reason for hiding this comment

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

Mind sending a follow up PR for that?

@rwjblue rwjblue merged commit cbd37a2 into emberjs:master Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants