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 beta] Ensure params and hash are frozen in debug builds. #14244

Merged
merged 2 commits into from
Sep 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ember-cli-build.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,8 @@ function babelConfigFor(environment) {
if (isProduction) {
includeDevHelpers = false;
plugins.push(filterImports({
'ember-metal/debug': ['assert', 'debug', 'deprecate', 'info', 'runInDebug', 'warn', 'debugSeal'],
'ember-metal': ['assert', 'debug', 'deprecate', 'info', 'runInDebug', 'warn', 'debugSeal']
'ember-metal/debug': ['assert', 'debug', 'deprecate', 'info', 'runInDebug', 'warn', 'debugSeal', 'debugFreeze'],
'ember-metal': ['assert', 'debug', 'deprecate', 'info', 'runInDebug', 'warn', 'debugSeal', 'debugFreeze']
}));
}

Expand Down
4 changes: 4 additions & 0 deletions packages/ember-debug/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ setDebugFunction('debugSeal', function debugSeal(obj) {
Object.seal(obj);
});

setDebugFunction('debugFreeze', function debugFreeze(obj) {
Object.freeze(obj);
});

setDebugFunction('deprecate', _deprecate);

setDebugFunction('warn', _warn);
Expand Down
36 changes: 32 additions & 4 deletions packages/ember-glimmer/lib/utils/references.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import {
EmptyObject,
meta as metaFor,
watchKey,
isFeatureEnabled
isFeatureEnabled,
runInDebug
} from 'ember-metal';
import { CONSTANT_TAG, ConstReference, DirtyableTag, UpdatableTag, combine, isConst } from 'glimmer-reference';
import { ConditionalReference as GlimmerConditionalReference, NULL_REFERENCE, UNDEFINED_REFERENCE } from 'glimmer-runtime';
Expand Down Expand Up @@ -276,7 +277,16 @@ export class SimpleHelperReference extends CachedReference {
static create(helper, args) {
if (isConst(args)) {
let { positional, named } = args;
let result = helper(positional.value(), named.value());

let positionalValue = positional.value();
let namedValue = named.value();

runInDebug(() => {
Object.freeze(positionalValue);
Object.freeze(namedValue);
});

let result = helper(positionalValue, namedValue);

if (result === null) {
return NULL_REFERENCE;
Expand All @@ -302,7 +312,16 @@ export class SimpleHelperReference extends CachedReference {

compute() {
let { helper, args: { positional, named } } = this;
return helper(positional.value(), named.value());

let positionalValue = positional.value();
let namedValue = named.value();

runInDebug(() => {
Object.freeze(positionalValue);
Object.freeze(namedValue);
});

return helper(positionalValue, namedValue);
}
}

Expand All @@ -323,7 +342,16 @@ export class ClassBasedHelperReference extends CachedReference {

compute() {
let { instance, args: { positional, named } } = this;
return instance.compute(positional.value(), named.value());

let positionalValue = positional.value();
let namedValue = named.value();

runInDebug(() => {
Object.freeze(positionalValue);
Object.freeze(namedValue);
});

return instance.compute(positionalValue, namedValue);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* globals EmberDev */
import { RenderingTest, moduleFor } from '../../utils/test-case';
import { makeBoundHelper } from '../../utils/helpers';
import { runDestroy } from 'internal-test-helpers';
Expand Down Expand Up @@ -548,5 +549,120 @@ moduleFor('Helpers test: custom helpers', class extends RenderingTest {

equal(destroyCount, 1, 'destroy is called after a view is destroyed');
}

});

// these feature detects prevent errors in these tests
// on platforms (*cough* IE9 *cough*) that do not
// property support `Object.freeze`
let pushingIntoFrozenArrayThrows = (() => {
let array = [];
Object.freeze(array);

try {
array.push('foo');

return false;
} catch(e) {
return true;
}
})();

let assigningExistingFrozenPropertyThrows = (() => {
let obj = { foo: 'asdf' };
Object.freeze(obj);

try {
obj.foo = 'derp';

return false;
} catch(e) {
return true;
}
})();

let addingPropertyToFrozenObjectThrows = (() => {
let obj = { foo: 'asdf' };
Object.freeze(obj);

try {
obj.bar = 'derp';

return false;
} catch(e) {
return true;
}
})();

if (!EmberDev.runningProdBuild && (
pushingIntoFrozenArrayThrows ||
assigningExistingFrozenPropertyThrows ||
addingPropertyToFrozenObjectThrows
)) {
class HelperMutatingArgsTests extends RenderingTest {
buildCompute() {
return (params, hash) => {
if (pushingIntoFrozenArrayThrows) {
this.assert.throws(() => {
params.push('foo');

// cannot assert error message as it varies by platform
});
}

if (assigningExistingFrozenPropertyThrows) {
this.assert.throws(() => {
hash.foo = 'bar';

// cannot assert error message as it varies by platform
});
}

if (addingPropertyToFrozenObjectThrows) {
this.assert.throws(() => {
hash.someUnusedHashProperty = 'bar';

// cannot assert error message as it varies by platform
});
}
};
}

['@test cannot mutate params - no positional specified / named specified']() {
this.render('{{test-helper foo=bar}}', { bar: 'derp' });
}

['@test cannot mutate params - positional specified / no named specified']() {
this.render('{{test-helper bar}}', { bar: 'derp' });
}

['@test cannot mutate params - positional specified / named specified']() {
this.render('{{test-helper bar foo=qux}}', { bar: 'derp', qux: 'baz' });
}

['@test cannot mutate params - no positional specified / no named specified']() {
this.render('{{test-helper}}', { bar: 'derp', qux: 'baz' });
}
}

moduleFor('Helpers test: mutation triggers errors - class based helper', class extends HelperMutatingArgsTests {
constructor() {
super();

let compute = this.buildCompute();

this.registerHelper('test-helper', {
compute
});
}
});

moduleFor('Helpers test: mutation triggers errors - simple helper', class extends HelperMutatingArgsTests {
constructor() {
super();

let compute = this.buildCompute();

this.registerHelper('test-helper', compute);
}
});
}
7 changes: 6 additions & 1 deletion packages/ember-metal/lib/debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ export let debugFunctions = {
deprecate() {},
deprecateFunc(...args) { return args[args.length - 1]; },
runInDebug() {},
debugSeal() {}
debugSeal() {},
debugFreeze() {}
};

export function getDebugFunction(name) {
Expand Down Expand Up @@ -48,3 +49,7 @@ export function runInDebug() {
export function debugSeal() {
return debugFunctions.debugSeal.apply(undefined, arguments);
}

export function debugFreeze() {
return debugFunctions.debugFreeze.apply(undefined, arguments);
}
4 changes: 3 additions & 1 deletion packages/ember-metal/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ export {
deprecateFunc,
runInDebug,
setDebugFunction,
getDebugFunction
getDebugFunction,
debugSeal,
debugFreeze
} from './debug';
export {
instrument,
Expand Down