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

[DT] Fix #915. Adjust `_setColumns` to deep clone properly and only once on recursive nodes. #1008

Merged
merged 10 commits into from Aug 31, 2013

Conversation

Projects
None yet
2 participants
@apipkin
Copy link
Contributor

apipkin commented Jul 19, 2013

Adjust _setColumns to deep clone properly and only once on recursive nodes.

_setColumns has a local function called copyObj that is used to clone the columns config to prevent it from potentially busting the use of that object outside of DataTable. arrayIndex, a pointer to Y.Array.indexOf, is used to look up to prevent infinite recursion during the clone.

arrayIndex had the arguments in the wrong order. This pr is submitted to correct this.

Fix #915

@lsmith

This comment has been minimized.

Copy link
Contributor

lsmith commented Jul 19, 2013

👍

@lsmith

This comment has been minimized.

Copy link
Contributor

lsmith commented Jul 19, 2013

Can you add at least one test and a HISTORY.md entry for this?

Hmm, since this is fixing an infinite loop, I wonder if it would be possible to verify the failure with a test.

@apipkin

This comment has been minimized.

Copy link
Contributor

apipkin commented Jul 19, 2013

ping @lsmith

@@ -4,7 +4,8 @@ DataTable Change History
@VERSION@
------

* No changes.
* Fix issue where recurssive nesting of objects was clone infinitely

This comment has been minimized.

@lsmith

lsmith Jul 19, 2013

Contributor

recursive, cloned.

cloned = table._setColumns([data]);
Y.Assert.isTrue(true);
} catch (e) {
Y.Assert.isTrue(false, e);

This comment has been minimized.

@lsmith

lsmith Jul 19, 2013

Contributor

Y.Assert.fail(e.message), perhaps?

I didn't think try/catch caught infinite loop errors. I could be wrong.

This comment has been minimized.

@apipkin

apipkin Jul 20, 2013

Contributor

It's failing without the fix in place. Eric made a comment about this may not be the best thing to be testing for because a failing test would mean a stack overflow error would be bad on the VMs.

table = new this.Table(),
data = { key: 'abc', self: {} },
cloned,
timer;

This comment has been minimized.

@lsmith

lsmith Jul 19, 2013

Contributor

test and timer are declared, but not used.

data.self = data;

try {
cloned = table._setColumns([data]);

This comment has been minimized.

@lsmith

lsmith Jul 19, 2013

Contributor

Best not to test protected methods directly, but the public api method/s that trigger the potentially failing use case. Protected and private methods can be changed and removed, which would require changing the tests, which means you can't use them to test for regressions.

This comment has been minimized.

@apipkin

apipkin Jul 20, 2013

Contributor

Thanks for the tip :)

@ghost ghost assigned apipkin Jul 22, 2013

@apipkin

This comment has been minimized.

Copy link
Contributor

apipkin commented Aug 8, 2013

@lsmith care to evaluate the new changes?

table.set('columns', [data]);
Y.Assert.isTrue(true);
} catch (e) {
Y.Assert.isTrue(false, e.message);

This comment has been minimized.

@lsmith

lsmith Aug 8, 2013

Contributor

Y.Assert.fail(e.message)

Did you ever confirm that try/catch will catch infinite loops? If not, a regression could be bad for the VMs, or even local testing env.

@lsmith

This comment has been minimized.

Copy link
Contributor

lsmith commented Aug 8, 2013

The changes are fine. Feel free to ignore the Y.Assert.fail(...) nit. But You do need to confirm that try/catch protects against infinite recursion. Otherwise, the test is potentially harmful.

@apipkin

This comment has been minimized.

Copy link
Contributor

apipkin commented Aug 11, 2013

I cannot find anything about trapping infinite recursion. So I'm assuming this could be potentially harmful since the exception isn't thrown until the stack has been exceeded.

Should I just drop the test?

@lsmith

This comment has been minimized.

Copy link
Contributor

lsmith commented Aug 12, 2013

Leave it in the source, commented out with an additional explanatory comment. Better to leave evidence.

Ship it!

@apipkin apipkin merged commit c712e8c into yui:dev-3.x Aug 31, 2013

1 check was pending

default The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment