From efe146c24b408e0cd56faaf20c8f0b679a2210f7 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 5 May 2015 18:58:51 -0400 Subject: [PATCH] fix(ngClass): add/remove classes which are properties of Object.prototype Previously, ngClass and ngAnimate would track the status of classes using an ordinary object. This causes problems when class names match names of properties in Object.prototype, including non-standard Object.prototype properties such as 'watch' and 'unwatch' in Firefox. Because of this shadowing, ngClass and ngAnimate were unable to correctly determine the changed status of these classes. In orderto accomodate this patch, some changes have been necessary elsewhere in the codebase, in order to facilitate iterating, comparingand copying objects with a null prototype, or which shadow the `hasOwnProperty` method Summary: - fast paths for various internal functions when createMap() is used - Make createMap() safe for internal functions like copy/equals/forEach - Use createMap() in more places to avoid needing hasOwnProperty() Closes #11813 --- src/Angular.js | 70 ++++++++++++++++++++++----- src/ng/animate.js | 4 +- src/ng/directive/ngClass.js | 4 +- test/AngularSpec.js | 83 ++++++++++++++++++++++++++++++++ test/ng/directive/ngClassSpec.js | 26 ++++++++++ 5 files changed, 172 insertions(+), 15 deletions(-) diff --git a/src/Angular.js b/src/Angular.js index 5ba8b6c88eb2..824c0a9d70c8 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -36,6 +36,7 @@ isUndefined: true, isDefined: true, isObject: true, + isBlankObject: true, isString: true, isNumber: true, isDate: true, @@ -175,6 +176,7 @@ var splice = [].splice, push = [].push, toString = Object.prototype.toString, + getPrototypeOf = Object.getPrototypeOf, ngMinErr = minErr('ng'), /** @name angular */ @@ -267,12 +269,25 @@ function forEach(obj, iterator, context) { } } else if (obj.forEach && obj.forEach !== forEach) { obj.forEach(iterator, context, obj); - } else { + } else if (isBlankObject(obj)) { + // createMap() fast path --- Safe to avoid hasOwnProperty check because prototype chain is empty + for (key in obj) { + iterator.call(context, obj[key], key, obj); + } + } else if (typeof obj.hasOwnProperty === 'function') { + // Slow path for objects inheriting Object.prototype, hasOwnProperty check needed for (key in obj) { if (obj.hasOwnProperty(key)) { iterator.call(context, obj[key], key, obj); } } + } else { + // Slow path for objects which do not have a method `hasOwnProperty` + for (key in obj) { + if (hasOwnProperty.call(obj, key)) { + iterator.call(context, obj[key], key, obj); + } + } } } return obj; @@ -498,6 +513,16 @@ function isObject(value) { } +/** + * Determine if a value is an object with a null prototype + * + * @returns {boolean} True if `value` is an `Object` with a null prototype + */ +function isBlankObject(value) { + return value !== null && typeof value === 'object' && !getPrototypeOf(value); +} + + /** * @ngdoc function * @name angular.isString @@ -781,7 +806,7 @@ function copy(source, destination, stackSource, stackDest) { destination = new RegExp(source.source, source.toString().match(/[^\/]*$/)[0]); destination.lastIndex = source.lastIndex; } else if (isObject(source)) { - var emptyObject = Object.create(Object.getPrototypeOf(source)); + var emptyObject = Object.create(getPrototypeOf(source)); destination = copy(source, emptyObject, stackSource, stackDest); } } @@ -800,7 +825,7 @@ function copy(source, destination, stackSource, stackDest) { stackDest.push(destination); } - var result; + var result, key; if (isArray(source)) { destination.length = 0; for (var i = 0; i < source.length; i++) { @@ -820,21 +845,40 @@ function copy(source, destination, stackSource, stackDest) { delete destination[key]; }); } - for (var key in source) { - if (source.hasOwnProperty(key)) { - result = copy(source[key], null, stackSource, stackDest); - if (isObject(source[key])) { - stackSource.push(source[key]); - stackDest.push(result); + if (isBlankObject(source)) { + // createMap() fast path --- Safe to avoid hasOwnProperty check because prototype chain is empty + for (key in source) { + putValue(key, source[key], destination, stackSource, stackDest); + } + } else if (source && typeof source.hasOwnProperty === 'function') { + // Slow path, which must rely on hasOwnProperty + for (key in source) { + if (source.hasOwnProperty(key)) { + putValue(key, source[key], destination, stackSource, stackDest); + } + } + } else { + // Slowest path --- hasOwnProperty can't be called as a method + for (key in source) { + if (hasOwnProperty.call(source, key)) { + putValue(key, source[key], destination, stackSource, stackDest); } - destination[key] = result; } } setHashKey(destination,h); } - } return destination; + + function putValue(key, val, destination, stackSource, stackDest) { + // No context allocation, trivial outer scope, easily inlined + var result = copy(val, null, stackSource, stackDest); + if (isObject(val)) { + stackSource.push(val); + stackDest.push(result); + } + destination[key] = result; + } } /** @@ -915,14 +959,14 @@ function equals(o1, o2) { } else { if (isScope(o1) || isScope(o2) || isWindow(o1) || isWindow(o2) || isArray(o2) || isDate(o2) || isRegExp(o2)) return false; - keySet = {}; + keySet = createMap(); for (key in o1) { if (key.charAt(0) === '$' || isFunction(o1[key])) continue; if (!equals(o1[key], o2[key])) return false; keySet[key] = true; } for (key in o2) { - if (!keySet.hasOwnProperty(key) && + if (!(key in keySet) && key.charAt(0) !== '$' && o2[key] !== undefined && !isFunction(o2[key])) return false; diff --git a/src/ng/animate.js b/src/ng/animate.js index e105de1ab28f..7905f062ce05 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -26,7 +26,9 @@ function splitClasses(classes) { classes = classes.split(' '); } - var obj = {}; + // Use createMap() to prevent class assumptions involving property names in + // Object.prototype + var obj = createMap(); forEach(classes, function(klass) { // sometimes the split leaves empty string values // incase extra spaces were applied to the options diff --git a/src/ng/directive/ngClass.js b/src/ng/directive/ngClass.js index cd1f6241efd3..dd06bd0c4635 100644 --- a/src/ng/directive/ngClass.js +++ b/src/ng/directive/ngClass.js @@ -39,7 +39,9 @@ function classDirective(name, selector) { } function digestClassCounts(classes, count) { - var classCounts = element.data('$classCounts') || {}; + // Use createMap() to prevent class assumptions involving property + // names in Object.prototype + var classCounts = element.data('$classCounts') || createMap(); var classesToUpdate = []; forEach(classes, function(className) { if (count > 0 || classCounts[className]) { diff --git a/test/AngularSpec.js b/test/AngularSpec.js index e70e49b49d3b..36101bbe707b 100644 --- a/test/AngularSpec.js +++ b/test/AngularSpec.js @@ -370,6 +370,21 @@ describe('angular', function() { expect(copy(undefined, [1,2,3])).toEqual([]); expect(copy({0: 1, 1: 2}, [1,2,3])).toEqual([1,2]); }); + + it('should copy objects with no prototype parent', function() { + var obj = extend(Object.create(null), { + a: 1, + b: 2, + c: 3 + }); + var dest = copy(obj); + + expect(Object.getPrototypeOf(dest)).toBe(null); + expect(dest.a).toBe(1); + expect(dest.b).toBe(2); + expect(dest.c).toBe(3); + expect(Object.keys(dest)).toEqual(['a', 'b', 'c']); + }); }); describe("extend", function() { @@ -651,6 +666,38 @@ describe('angular', function() { it('should return false when comparing an object and a Date', function() { expect(equals({}, new Date())).toBe(false); }); + + it('should safely compare objects with no prototype parent', function() { + var o1 = extend(Object.create(null), { + a: 1, b: 2, c: 3 + }); + var o2 = extend(Object.create(null), { + a: 1, b: 2, c: 3 + }); + expect(equals(o1, o2)).toBe(true); + o2.c = 2; + expect(equals(o1, o2)).toBe(false); + }); + + + it('should safely compare objects which shadow Object.prototype.hasOwnProperty', function() { + /* jshint -W001 */ + var o1 = { + hasOwnProperty: true, + a: 1, + b: 2, + c: 3 + }; + var o2 = { + hasOwnProperty: true, + a: 1, + b: 2, + c: 3 + }; + expect(equals(o1, o2)).toBe(true); + o1.hasOwnProperty = function() {}; + expect(equals(o1, o2)).toBe(false); + }); }); @@ -980,6 +1027,42 @@ describe('angular', function() { }); + it('should safely iterate through objects with no prototype parent', function() { + var obj = extend(Object.create(null), { + a: 1, b: 2, c: 3 + }); + var log = []; + var self = {}; + forEach(obj, function(val, key, collection) { + expect(this).toBe(self); + expect(collection).toBe(obj); + log.push(key + '=' + val); + }, self); + expect(log.length).toBe(3); + expect(log).toEqual(['a=1', 'b=2', 'c=3']); + }); + + + it('should safely iterate through objects which shadow Object.prototype.hasOwnProperty', function() { + /* jshint -W001 */ + var obj = { + hasOwnProperty: true, + a: 1, + b: 2, + c: 3 + }; + var log = []; + var self = {}; + forEach(obj, function(val, key, collection) { + expect(this).toBe(self); + expect(collection).toBe(obj); + log.push(key + '=' + val); + }, self); + expect(log.length).toBe(4); + expect(log).toEqual(['hasOwnProperty=true', 'a=1', 'b=2', 'c=3']); + }); + + describe('ES spec api compliance', function() { function testForEachSpec(expectedSize, collection) { diff --git a/test/ng/directive/ngClassSpec.js b/test/ng/directive/ngClassSpec.js index f9f8f044ad89..e230c1a6042d 100644 --- a/test/ng/directive/ngClassSpec.js +++ b/test/ng/directive/ngClassSpec.js @@ -33,6 +33,32 @@ describe('ngClass', function() { })); + it('should add new and remove old classes with same names as Object.prototype properties dynamically', inject(function($rootScope, $compile) { + /* jshint -W001 */ + element = $compile('
')($rootScope); + $rootScope.dynClass = { watch: true, hasOwnProperty: true, isPrototypeOf: true }; + $rootScope.$digest(); + expect(element.hasClass('existing')).toBe(true); + expect(element.hasClass('watch')).toBe(true); + expect(element.hasClass('hasOwnProperty')).toBe(true); + expect(element.hasClass('isPrototypeOf')).toBe(true); + + $rootScope.dynClass.watch = false; + $rootScope.$digest(); + expect(element.hasClass('existing')).toBe(true); + expect(element.hasClass('watch')).toBe(false); + expect(element.hasClass('hasOwnProperty')).toBe(true); + expect(element.hasClass('isPrototypeOf')).toBe(true); + + delete $rootScope.dynClass; + $rootScope.$digest(); + expect(element.hasClass('existing')).toBe(true); + expect(element.hasClass('watch')).toBe(false); + expect(element.hasClass('hasOwnProperty')).toBe(false); + expect(element.hasClass('isPrototypeOf')).toBe(false); + })); + + it('should support adding multiple classes via an array', inject(function($rootScope, $compile) { element = $compile('
')($rootScope); $rootScope.$digest();