Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
mernen wants to merge 2 commits into
angular:masterfrom
mernen:hasOwnProperty
Closed

fix(*): do not rely on an object's hasOwnProperty #2141
mernen wants to merge 2 commits into
angular:masterfrom
mernen:hasOwnProperty

Conversation

@mernen
Copy link
Copy Markdown
Contributor

@mernen mernen commented Mar 12, 2013

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.

mernen added 2 commits March 12, 2013 00:51
@mernen
fix(*): do not rely on an object's hasOwnProperty
10694e9 
Closes angular#1944
@mernen
fix(angular.equals): stricter test for key presence
6c32811 
Merely testing for object[key] will give incorrect results on keys
defined in Object.prototype.
@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Mar 12, 2013

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
Copy link
Copy Markdown
Contributor Author

mernen commented Mar 13, 2013

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
Copy link
Copy Markdown
Contributor

IgorMinar commented Mar 13, 2013

thnx.

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

@ghost ghost assigned IgorMinar Mar 13, 2013
@petebacondarwin
Copy link
Copy Markdown
Contributor

petebacondarwin commented Apr 30, 2013

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

@barries
Copy link
Copy Markdown

barries commented May 17, 2013

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
Copy link
Copy Markdown
Contributor

petebacondarwin commented Jun 20, 2013

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

petebacondarwin pushed a commit that referenced this pull request Jul 8, 2013
@mernen @petebacondarwin
fix(angular.equals): do not match keys defined in the prototype chain
12d77c4 
Merely testing for object[key] will give incorrect results on keys
defined in Object.prototype.
Note: IE8 is generally broken in this regard since `for...in` never returns
certain property keys even if they are defined directly on the object.

See #2141 - partially merges this PR
petebacondarwin pushed a commit that referenced this pull request Jul 8, 2013
@mernen @petebacondarwin
fix(angular.equals): do not match keys defined in the prototype chain
7829c50 
Merely testing for object[key] will give incorrect results on keys
defined in Object.prototype.
Note: IE8 is generally broken in this regard since `for...in` never returns
certain property keys even if they are defined directly on the object.

See #2141 - partially merges this PR
@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Jul 10, 2013

@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.

ctrahey pushed a commit to ctrahey/angular.js that referenced this pull request Jul 22, 2013
@mernen @ctrahey
fix(angular.equals): do not match keys defined in the prototype chain
13abd8b 
Merely testing for object[key] will give incorrect results on keys
defined in Object.prototype.
Note: IE8 is generally broken in this regard since `for...in` never returns
certain property keys even if they are defined directly on the object.

See angular#2141 - partially merges this PR
@petebacondarwin
Copy link
Copy Markdown
Contributor

petebacondarwin commented Jul 23, 2013

@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
Copy link
Copy Markdown
Contributor

IgorMinar commented Jul 24, 2013

is 1865ea9 ready to be merged?

@petebacondarwin
Copy link
Copy Markdown
Contributor

petebacondarwin commented Jul 24, 2013

Sadly it breaks on IE8. More work needed.

On 24 July 2013 17:38, Igor Minar notifications@github.com 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
Copy link
Copy Markdown
Contributor

petebacondarwin commented Jul 24, 2013

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

@petebacondarwin
Copy link
Copy Markdown
Contributor

petebacondarwin commented Jul 24, 2013

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

@petebacondarwin petebacondarwin mentioned this pull request Jul 24, 2013
Closed
@petebacondarwin
Copy link
Copy Markdown
Contributor

petebacondarwin commented Jul 27, 2013

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

@mernen mernen deleted the hasOwnProperty branch September 2, 2013 04:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@IgorMinar IgorMinar

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mernen @IgorMinar @petebacondarwin @barries