fix(*): do not rely on an object's hasOwnProperty

[mernen]

As noted by #1944, AngularJS currently relies in several places on an object's own .hasOwnProperty() method. This will break when said object does not inherit from Object.prototype, or when this key is redefined.

This patch is inspired by jQuery's approach to the problem.

Caveats

• No tests for Object.create(null) were included, as they would break on old versions of IE; however, their practical effect is the same as redefining hasOwnProperty to undefined.
• I only included tests for the main functions, as I felt that writing a test for every single instance where hasOwnProperty might be called would needlessly dilute the tests.

[IgorMinar]

PR Checklist (Minor Bugfix)

• Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name)
• PR doesn't introduce new api
• PR doesn't contain a breaking change
• PR contains unit tests
• [-] PR contains e2e tests (if suitable)
• [-] PR contains documentation update (if suitable)
• PR passes all tests on Travis (sanity)
• PR passes all tests on ci.angularjs.org (cross-browser compatibility)
• PR is rebased against recent master
• [-] PR is squashed into one commit per logical change
• PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
• All changes requested in review have been implemented

[mernen]

Not sure what happened to Travis – I have Firefox 19 on Ubuntu 12.04 here, and it passed test:e2e just fine, same for Chrome.

@IgorMinar I can certainly see your concerns about performance. While most of these places could hypothetically see a conflict, they are constrained to developer actions, and as such are extremely unlikely to happen and very easy to avoid. I will clean up this PR and only patch places that could conceivably receive keys out of the developer's control.

Regarding the CLA: I did sign it before, as Daniel Luz, and I have a few patches merged in already.

[IgorMinar]

thnx.

btw travis has been a bit flaky lately - don't worry about it much as long as tests are passing locally.

[petebacondarwin]

@mernen - did you make the changes suggested by @IgorMinar yet?

[barries]

FYI: to verify that this occurs easily in the wild, doing something like this:

<span ng-switch="foo.bar.hasOwnProperty('_value')">...</span>

caused getterFnCache["hasOwnProperty"] to be set, breaking future cache look ups.

[petebacondarwin]

@mernen - are you able to make these changes to this PR?

[IgorMinar]

@barries nice bug find: I can imagine $parse breaking if foo.bar.hasOwnProperty('_value') was provided as an expression to parse. we should add this as a test case.

I see that @petebacondarwin is making the changes already.

[petebacondarwin]

@barries - I can't actually get your suggested "in the wild" bug to fail. Any chance you could create a failing spec for it? I tried:

      it('should not break if hasOwnProperty is overridden inadvertently by $parse', function() {
        scope.foo = { value: 1 };
        expect(scope.$eval("foo.hasOwnProperty('value')")).toBeTruthy();
        expect(scope.$eval("foo.hasOwnProperty('value')")).toBeTruthy();
      });

[IgorMinar]

is 1865ea9 ready to be merged?

[petebacondarwin]

Sadly it breaks on IE8. More work needed.

On 24 July 2013 17:38, Igor Minar [email protected] wrote:

is 1865ea9 1865ea9 ready
to be merged?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2141#issuecomment-21498049
.

[petebacondarwin]

See http://ci.angularjs.org/job/angular.js-pete/264/

[petebacondarwin]

Actually, considering the reasoning from this commit: 7829c50#L1R275
We probably can simply drop the tests on IE8. What do you think @IgorMinar?

[petebacondarwin]

I have a new pull request for all of this, see #3331, so closing.