fix($parse): Preserve expensive checks when runnning $eval inside an expression #13850

lgalfaso · 2016-01-26T14:13:43Z

When running an expression with expensive checks, there is a call to $eval or $evalAsync
then that expression is also evaluated using expensive checks

gkalpak · 2016-01-26T14:46:56Z

Aren't $watch and $watchCollection also affected ?
LGTM otherwise.

gkalpak · 2016-01-26T17:40:13Z

So, this works for $$postDigest(), $apply(), $watch() and $watchCollection(), but it doesn't seem to work for $applyAsync().

I would still add tests for all of those (better safe than sorry 😃). E.g.:

scope.foo = {w: $window};

expect(function() {
  scope.$eval($parse('$$postDigest($eval("foo.w && true"))', null, true));
}).toThrow();
expect(function() {
  scope.$eval($parse('$apply("foo.w && true")', null, true));
}).toThrow();

expect(function() {
  scope.$eval($parse('$applyAsync("foo.w && true")', null, true));
  scope.$digest();
}).toThrow();
expect(function() {
  scope.$eval($parse('$watch("foo.w && true")', null, true));
  scope.$digest();
}).toThrow();
expect(function() {
  scope.$eval($parse('$watchCollection("foo.w && true", $eval)', null, true));
  scope.$digest();
}).toThrow();

jbedard · 2016-01-26T18:24:22Z

src/ng/parse.js

+
+    function expensiveChecksInterceptor(fn, expensiveChecks) {
+      if (!expensiveChecks) return fn;
+      return function expensiveCheckFn(scope, locals, assign, inputs) {


Won't this break $$watchDelegate and inputs on fn?

@jbedard you are right, this needs more work

jbedard · 2016-01-26T22:05:31Z

How about disallowing execution of nested $parseed statements. Would that be an option?

lgalfaso · 2016-01-26T22:06:41Z

How about disallowing execution of nested $parseed statements. Would that be an option?

That would be too big of a breaking change

jbedard · 2016-01-26T22:10:50Z

People actually do that? I've never even thought of it. Unless there is a less obvious way that I haven't thought of...

lgalfaso · 2016-01-27T10:06:07Z

Updated the PR and fixed the comments. PTAL

gkalpak · 2016-01-27T11:25:17Z

src/ng/parse.js

@@ -1788,6 +1797,7 @@ function $ParseProvider() {
            }
            cache[cacheKey] = parsedExpression;
          }
+          parsedExpression = expensiveChecksInterceptor(parsedExpression, expensiveChecks);


Couldn't we move this inside the if-block ?

Also, this would hide properties like assign, constant, inputs and literal.

Also, wouldn't it make sense to cache the parsedExpression that has already had the expensive checks interceptor added, if the cache is cacheExpensive?

Actually, that's what I meant by "move this inside the if-block". (Inside and before cache[cacheKey] = parsedExpression.)
Thx for making it clear, @petebacondarwin 😃

Also, this would hide properties like assign, constant, inputs and literal

Given that the expensive checks is a private API, I think this is not an issue. Anyhow, made the change.

Couldn't we move this inside the if-block ?

Yep :)

petebacondarwin · 2016-01-27T13:54:38Z

LGTM

…expression When running an expression with expensive checks, there is a call to `$eval` or `$evalAsync` then that expression is also evaluated using expensive checks

arashkbanan · 2016-01-27T14:36:50Z

test/ng/rootScopeSpec.js

@@ -1387,7 +1387,7 @@ describe('Scope', function() {
      expect(child.log).toBe('child context');
    }));

-    it('should operate only with a single queue across all child and isolate scopes', inject(function($rootScope) {
+    it('should operate only with a single queue across all child and isolate scopes', inject(function($rootScope, $parse) {
      var childScope = $rootScope.$new();


gkalpak · 2016-01-28T17:22:23Z

@petebacondarwin, should this (and #13871) be backported to v1.4.x ?

petebacondarwin · 2016-01-28T18:49:09Z

@gkalpak - yes I would say so. Please do make it happen :-)

…expression When running an expression with expensive checks, there is a call to `$eval` or `$evalAsync` then that expression is also evaluated using expensive checks Closes: #13850

gkalpak · 2016-01-28T19:13:26Z

Backported to v1.4.x as 96d62cc.

googlebot added the cla: yes label Jan 26, 2016

lgalfaso force-pushed the parse-expensive-eval branch from c900f6f to b8a785b Compare January 26, 2016 14:15

lgalfaso force-pushed the parse-expensive-eval branch from b8a785b to 157e9e7 Compare January 26, 2016 14:48

jbedard reviewed Jan 26, 2016
View reviewed changes

lgalfaso force-pushed the parse-expensive-eval branch 2 times, most recently from 2add0ce to 39ecc9a Compare January 27, 2016 10:05

lgalfaso force-pushed the parse-expensive-eval branch 2 times, most recently from d529115 to 0e7bdaa Compare January 27, 2016 10:50

gkalpak reviewed Jan 27, 2016
View reviewed changes

lgalfaso force-pushed the parse-expensive-eval branch 2 times, most recently from 997a7fd to 58e4014 Compare January 27, 2016 13:21

fix($parse): Preserve expensive checks when runnning $eval inside an …

5dbd8de

…expression When running an expression with expensive checks, there is a call to `$eval` or `$evalAsync` then that expression is also evaluated using expensive checks

lgalfaso force-pushed the parse-expensive-eval branch from 58e4014 to 5dbd8de Compare January 27, 2016 13:56

lgalfaso closed this in acfda10 Jan 27, 2016

arashkbanan reviewed Jan 27, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($parse): Preserve expensive checks when runnning $eval inside an expression #13850

fix($parse): Preserve expensive checks when runnning $eval inside an expression #13850

lgalfaso commented Jan 26, 2016

gkalpak commented Jan 26, 2016

gkalpak commented Jan 26, 2016

jbedard Jan 26, 2016

lgalfaso Jan 26, 2016

jbedard commented Jan 26, 2016

lgalfaso commented Jan 26, 2016

jbedard commented Jan 26, 2016

lgalfaso commented Jan 27, 2016

gkalpak Jan 27, 2016

gkalpak Jan 27, 2016

petebacondarwin Jan 27, 2016

gkalpak Jan 27, 2016

lgalfaso Jan 27, 2016

petebacondarwin commented Jan 27, 2016

arashkbanan Jan 27, 2016

gkalpak commented Jan 28, 2016

petebacondarwin commented Jan 28, 2016

gkalpak commented Jan 28, 2016

fix($parse): Preserve expensive checks when runnning $eval inside an expression #13850

fix($parse): Preserve expensive checks when runnning $eval inside an expression #13850

Conversation

lgalfaso commented Jan 26, 2016

gkalpak commented Jan 26, 2016

gkalpak commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Jan 26, 2016

lgalfaso commented Jan 26, 2016

jbedard commented Jan 26, 2016

lgalfaso commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Jan 27, 2016

Choose a reason for hiding this comment

gkalpak commented Jan 28, 2016

petebacondarwin commented Jan 28, 2016

gkalpak commented Jan 28, 2016