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 release] Fix cyclic references on Array.prototype #17374

Merged

Conversation

jacobq
Copy link
Contributor

@jacobq jacobq commented Dec 14, 2018

My first non-doc ember.js PR so please be kind :)

  • Should detect the regression that causes v3.6.0-beta.3 JQuery.extend (recursively) is broken #17190
  • Test currently fails (correctly) on master but passes on v3.5.1
  • Not sure if the test is located in the right place or applied to the right object(s)
  • Note: this only checks the first level (e.g. won't catch a nested reference)

@jacobq jacobq force-pushed the test-for-prototype-extension-regression branch from 36fce9e to 3029e9d Compare December 14, 2018 22:10
@jacobq
Copy link
Contributor Author

jacobq commented Dec 14, 2018

I am looking at this again as it seems to have failed to correctly identify the point in git commit history was introduced. (It appears to pass against 246435e although that seems to be where the regression happened)

@jacobq jacobq changed the title test(NativeArray): Check for cyclic Array.prototype (ref #17190) [WIP] test(NativeArray): Check for cyclic Array.prototype (ref #17190) Dec 14, 2018
@rwjblue rwjblue self-assigned this Dec 14, 2018
@jacobq
Copy link
Contributor Author

jacobq commented Dec 14, 2018

@rwjblue Thanks for taking this on. Could you help me understand something here? How come when I checkout 246435e and build it, running the script below in node shows the problem, but running the test that import NativeArray passes (seems that the for...in loop iterates zero times)?

Test by loading Ember in NodeJS

Script

global.EmberENV = {
  EXTEND_PROTOTYPES: false,
};

const Ember = require('./dist/ember.debug.js');

const isOK = (a, original) => {
  original = original || a;
  for (let p in a) {
    if (a[p] === a) {
      return false;
    }
    else if (typeof a === 'object') {
      try {
        if (!isOK(a[p], a)) {
          return false;
        }
      } catch(e) {
        console.warn(`Caught exception while attempting recursive call: ${e.message}`);
        return false;
      }
    }
  }
  return true;
}

const check = (klass) => {
  const a = new klass();
  const result = isOK(a);
  console.log(`  * ${klass.name} ${result ? 'is ok' : 'is NOT OK -- cycle detected!'}`); 
}

// Extend this instead of built-in Array to avoid pollution
class ExtendedArray extends Array {}
Ember.NativeArray.apply(ExtendedArray.prototype);

// Just for fun, check nested case:
class CyclicThing {
  constructor() {
    this.sneaky = {
      firstLevelProp: 'foo',
      bar: {
        baz: 'quux',
        cycleWillBeHere: null
      }
    }
    this.sneaky.bar.cycleWillBeHere = this.sneaky;
  }
}

[Array, ExtendedArray, CyclicThing].forEach(check);

Output

  * Array is ok
  * ExtendedArray is NOT OK -- cycle detected!
Caught exception while attempting recursive call: Maximum call stack size exceeded
  * CyclicThing is NOT OK -- cycle detected!

@jacobq
Copy link
Contributor Author

jacobq commented Dec 15, 2018

Also, FWIW, there's an is-circular module that could be used instead of my home-made function.

…ble.

This fixes the following example case:

```js
$.extend(true, {}, {a:['a']})
```
Prior to this change, the above would trigger maximum call stack error.
This is because the `[]` computed property added to the array prototype
references itself, which ultimately makes `$.extend` (and other deep
equality style comparisons) fail.
@rwjblue rwjblue changed the title [WIP] test(NativeArray): Check for cyclic Array.prototype (ref #17190) [BUGFIX release] Fix cyclic references on Array.prototype Dec 15, 2018
@rwjblue
Copy link
Member

rwjblue commented Dec 15, 2018

Just pushed a commit fixing the issue (details in the comments and commit message).

Unfortunately, `class Foo extends Array{}` doesn't work well with older
browsers (and would even require changes in our transpilation for
production builds for modern ones).

The actual test doesn't care if its running on an Array (without the
fixes in this PR this modified test still fails the same way).
@rwjblue rwjblue merged commit 98bdc39 into emberjs:master Dec 15, 2018
@jacobq jacobq deleted the test-for-prototype-extension-regression branch December 15, 2018 04:47
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

2 participants