Skip to content

Conversation

@rajatd
Copy link
Contributor

@rajatd rajatd commented Dec 13, 2016

We need to keep property records for properties being enumerated live during enumeration. For dynamic objects, type handlers hold references to property records and for non-existent properties, we have code in ForInObjectEnumerator::MoveAndGetNext to keep the new records live.

Taking care of this case for Proxies which use IteratorObjectEnumerator


This change is Reviewable

@rajatd
Copy link
Contributor Author

rajatd commented Dec 13, 2016

@digitalinfinity could you please take a look?

Copy link
Contributor

@digitalinfinity digitalinfinity left a comment

Choose a reason for hiding this comment

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

LGTM, if you've verified that the property records do get cleaned up correctly

// Need to keep property records alive during enumeration to prevent collection
// and eventual reuse during the same enumeration. For DynamicObjects, property
// records are kept alive by type handlers.
this->propertyRecords.Prepend(iteratorObject->GetRecycler(), propertyRecord);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this cleared at some point?

@ThomsonTan
Copy link
Collaborator

    Assert(FALSE);

Reset this->propertyRecords when reset the enumerator? Also will this->propertyRecords be freed when destructing IteratorObjectEnumerator?


Refers to: lib/Runtime/Library/IteratorObjectEnumerator.cpp:63 in ba3244d. [](commit_id = ba3244d, deletion_comment = False)

@tcare
Copy link
Contributor

tcare commented Dec 22, 2016

Looks good, if they get cleaned up

@tcare
Copy link
Contributor

tcare commented Dec 29, 2016

I looked through this, and we generally follow the convention of throwing away the enumerator at the end unless it can be reset (e.g. CustomEnumerator.) We don't expect to reset IteratorObjectEnumerator, so it is fine to leave the references attached to the enumerator reference. OK to merge.

@rajatd
Copy link
Contributor Author

rajatd commented Dec 29, 2016

@tcare @digitalinfinity - thanks for the reviews.
@ThomsonTan The Assert in the Reset method implies that we should never call that method. Also, when the enumerator is thrown away (as @tcare mentioned), the propertyRecord list will become available for collection

@chakrabot chakrabot merged commit 4f58ae9 into chakra-core:release/1.4 Dec 29, 2016
chakrabot pushed a commit that referenced this pull request Dec 29, 2016
…merator

Merge pull request #2198 from rajatd:iteratorEnum

We need to keep property records for properties being enumerated live during enumeration. For dynamic objects, type handlers hold references to property records and for non-existent properties, we have code in ForInObjectEnumerator::MoveAndGetNext to keep the new records live.

Taking care of this case for Proxies which use IteratorObjectEnumerator
chakrabot pushed a commit that referenced this pull request Dec 29, 2016
…atorObjectEnumerator

Merge pull request #2198 from rajatd:iteratorEnum

We need to keep property records for properties being enumerated live during enumeration. For dynamic objects, type handlers hold references to property records and for non-existent properties, we have code in ForInObjectEnumerator::MoveAndGetNext to keep the new records live.

Taking care of this case for Proxies which use IteratorObjectEnumerator
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.

6 participants