fix(ngEventDirs): check for $rootScope.$$phase in event handler and don't $apply if already in $digest #16629

Narretz · 2018-07-13T10:47:32Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

googlebot · 2018-07-13T10:47:35Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-13T10:47:35Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

petebacondarwin · 2018-07-13T17:40:44Z

test/ng/directive/ngEventDirsSpec.js

+        $rootScope.$apply(function() {
+          element.triggerHandler('click');
+        });
+      }).toThrowError('listener error');


This test surprises me. AFAICT the callback in the $apply should be called wrapped in a try-catch block. I see that errors in the $digest() call get re-thrown; so perhaps the click() method on the scope is being called in the digest?

I thought this error would be caught in the $apply phase try/catch 🤔

Also I think we could use another test which invokes the handler within a watcher, so it would be within the $digest phase

But note that in both of these cases it does not re-throw, so why does it in this test?

I haven't confirmed, but I suspect it is because $exceptionHandler throws by default in tests. In order to test the "real" behavior, you have to set $exceptionHandler in log mode.

Ah yes, that sounds right. I guess we should have tests for both cases?

petebacondarwin

Except for that surprising test, this LGTM

gkalpak · 2018-07-17T06:06:25Z

src/ng/directive/ngEventDirs.js

                scope.$evalAsync(callback);
              } else {
-                scope.$apply(callback);
+                callback();


So, we are not wrapping this in try { ... } catch (err) { $exceptionHandler(Err); }?

That should make the following more consistent...

dom.click(); otherStuff();

... when dom.click() invokes ng-click and does vs does-not throw. SGTM...

Narretz · 2018-07-17T19:58:09Z

I've updated the PR and added the additional try catch ... however, I think it's not actually necessary. See the two new tests: when we trigger an event handler in a watch expression fn, the error thrown gets logged twice because the watch loop in the digest never sees the error.

If we simply throw in the watch expression fn, the watch loop in the digest catches the error and logs it (only once).

At least it feels wrong that this leads to two different results.

jbedard · 2018-07-17T20:20:00Z

test/ng/directive/ngEventDirsSpec.js

        $rootScope.$apply(function() {
          element.triggerHandler('click');
        });
-      }).toThrowError('listener error');
-    }));
+        $rootScope.do();


I was thinking this would be one line up, within the $apply. Or maybe it would be more clear if we did:

$rootScope.do = function() { element.triggerHandler('click'); $log.log('done'); }

Then invoke $rootScope.do within vs outside $apply. Those 2 cases should be consistent, both with $exceptionHandler throwing (the done never runs) vs only logging (the done always runs). I think this commit is required for those to be consistent?

Ok, if we do

$rootScope.do = function() { element.triggerHandler('click'); $log.log('done'); }; $rootScope.$apply(function() { $rootScope.do(); });

then we need the try-catch.
I guess the question is now for which behavior it's more important to be consistent? Because the try catch changes the behavior when we throw inside a digest. I assume that behavior is not as important.

Can you show an example of that change?

I've updated the tests

Narretz · 2018-07-17T21:20:53Z

test/ng/directive/ngEventDirsSpec.js

+    });
+
+
+    it('should not stop the digest when the event is fired in a watch expression fn', function() {


@jbedard if you trigger a click event handler inside a watch expression fn, then the additional try catch changes how the error gets logged, and in this case, anything that comes after the handler is called, whereas when you call the handler directly and throw, what comes after the handler is not called. See the test above for the different outcome.

That is caused by the click() vs triggerHandler('click')? I didn't think that would change anything...?

It's caused by the new try catch. To clarify, ij the watcher expression fn tests I test direct call to scope fn against event handler call to scope fn. The first test doesn't use the event handler at all. It's a bit of a digression from the original testing but it's an example where the new try catch leads to different behavior.

Is the watch expression fn test set up like you intended it to be? Because you mentioned that you think such a test would make sense.

Oh... I understand these two tests now. Sorry I was misreading $rootScope.click() as $element.click() for some reason (my fault 😅). I think this difference is ok, definitely worth the two do() tests behaving the same imo.

I think this test is correct and good as-is 👍

jbedard · 2018-07-17T22:11:52Z

test/ng/directive/ngEventDirsSpec.js

@@ -1,6 +1,6 @@
 'use strict';

-describe('event directives', function() {
+fdescribe('event directives', function() {


This is fine for now, but making a note of it...

jbedard · 2018-07-18T17:54:04Z

test/ng/directive/ngEventDirsSpec.js

+      });
+    });
+
+    it('should not stop the digest when the handler is called in a watch expression fn', function() {


I don't think this one is necessary since it doesn't actually trigger a DOM event

jbedard · 2018-07-18T17:55:55Z

LGTM

Could potentially also test dom.click() (the native click method) in addition to the the jQuery version. I think they should behave the same.

gkalpak · 2018-07-18T18:08:14Z

test/ng/directive/ngEventDirsSpec.js

+    });
+
+
+    it('should not stop the digest when the handler throws an error', function() {


This description seems wrong.

So what are we actually testing? "it should pass an error in the handler to the $exceptionHandler if the ~~handler~~ event is triggered ~~called~~ during a digest"?

AFAICT, we are testing what happens when the event is triggered outside a digest. I would expected something along the lines:

should not stop normal execution when the handler is triggered outside a digest and throws an error

gkalpak · 2018-07-18T18:12:37Z

test/ng/directive/ngEventDirsSpec.js

+      });
+    });
+
+    it('should not stop the digest when the handler is called in a watch expression fn', function() {


How is this test (and the ones below) test that the digest is not stopped? I would expect a second watch expression.
(What am I missing?)

Yes, not correct. It tests if the second part of the watch expression fn gets called after the event handler throws

gkalpak · 2018-07-18T18:13:28Z

test/ng/directive/ngEventDirsSpec.js

+        };
+
+        $rootScope.$watch(function() {
+          $rootScope.click();


Did you mean element.triggerHandler('click')?
If yes, wouldn't this test be identical with the following one?
If no, then what does this test have to do with event handlers?

I think that's what the next test is for, which is why I don't think this one is necessary.

The test exists because I was looking for something that would behave differently when adding the try ... catch in the event directive.

gkalpak · 2018-07-18T18:20:24Z

test/ng/directive/ngEventDirsSpec.js

+    });
+  });
+
+  describe('click', function() {


Do we have similar test for forceAsyncEvents? If not, we should 😁

…n $digest Digest cycle already in progress error can inadvertently be caused when triggering an element's click event while within an active digest cycle. This is due to the ngEventsDirs event handler always calling $rootScope.$apply regardless of the status of $rootScope.$$phase. Checking the phase and calling the function immediately if within an active digest cycle will prevent the problem without reducing current functionality. Closes angular#14673 Closes angular#14674

…nt was triggered in a digest This ensures that the error handling is the same for events triggered inside and outside a digest.

googlebot added the cla: no label Jul 13, 2018

petebacondarwin reviewed Jul 16, 2018

View reviewed changes

petebacondarwin approved these changes Jul 16, 2018

View reviewed changes

gkalpak reviewed Jul 17, 2018

View reviewed changes

jbedard reviewed Jul 17, 2018

View reviewed changes

Narretz commented Jul 17, 2018

View reviewed changes

jbedard reviewed Jul 17, 2018

View reviewed changes

jbedard reviewed Jul 18, 2018

View reviewed changes

gkalpak reviewed Jul 18, 2018

View reviewed changes

Narretz force-pushed the fix-phase branch 2 times, most recently from a489fda to b389161 Compare July 23, 2018 10:06

markgardner and others added 2 commits July 23, 2018 19:31

fix(ngEventDirs): pass error in handler to $exceptionHandler when eve…

36a09ac

…nt was triggered in a digest This ensures that the error handling is the same for events triggered inside and outside a digest.

Narretz force-pushed the fix-phase branch from b389161 to 36a09ac Compare July 23, 2018 17:31

Narretz merged commit a42f8a0 into angular:master Jul 25, 2018

		});


		it('should not stop the digest when the event is fired in a watch expression fn', function() {

fix(ngEventDirs): check for $rootScope.$$phase in event handler and don't $apply if already in $digest #16629

fix(ngEventDirs): check for $rootScope.$$phase in event handler and don't $apply if already in $digest #16629

Uh oh!

Conversation

Narretz commented Jul 13, 2018

Uh oh!

googlebot commented Jul 13, 2018

Uh oh!

googlebot commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbedard commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Narretz commented Jul 17, 2018 •

edited

Loading

Narretz Jul 17, 2018 •

edited

Loading

Narretz Jul 20, 2018 •

edited

Loading