fix($rootScope): avoid unstable reference in $broadcast event #7445

ncuillery · 2014-05-13T09:38:38Z

Request Type: bug

How to reproduce: Any case of asynchronous access to event.currentScope :

console.log(event) then expand the object in console (Chrome / Firefox)
Via $timeout
In Jasmine expectation

Plunker

Note : The tree of scopes must have a depth greater than 2 (see commit diff for rootScopeSpec.js).

Component(s): scope

Impact: small

Complexity: small

This issue is related to:

Detailed Description:

In the $broadcast method, the event.currentScope is initialized with current which is the iterator used by the scope traversal's loop.

It caused problems when using asynchonously the event object : the event.currentScope is not the scope which listen the event. It's the last scope crossed by the traversal which has nothing to do with the event (or coincidentally has).

Other Comments:

The event.currentScope was a reference to the iterator of the scope traversal (`current`). It caused problems when accessing the event.currentScope asynchronously : it was the last scope of the traversal instead of the scope which listen the event.

mary-poppins · 2014-05-13T09:38:40Z

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

Uses the issue template (#7445)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

lgalfaso · 2014-05-13T12:40:17Z

@ncuillery Thanks for the PR. This does look like a real bug, anyhow, I have a few comments

if we fix $broadcast, then the same should be done for $emit
it needs tests :)
it may have some performance implications

Thanks again

ncuillery · 2014-05-13T14:40:19Z

Right, I saw $emit and I didn't observe the bug in this method (there is no scope traversal), but I agree that having a different syntax in $emit and $broadcast is bad (I hesitated for a long time to refactor :) ).

About testing, I broke the listener should receive event object test case by simply adding a grand-child in the tree of scopes (See diff in rootScopeSpec.js). In my mind, the test case is relevant because we are immediately in a reproducing situation (jasmine expectations are asynchronous). If the test succeed again, it means that the bug is fixed.

I gonna play with jsPerf.

lgalfaso · 2014-05-13T21:21:10Z

@ncuillery if I apply only the changes to rootScopeSpec.js to the current master, then all tests pass. If this patch is meant to fix an issue, it will need a test that fail without the patch and passes with the patch

Fresheyeball · 2014-05-14T02:55:08Z

src/ng/rootScope.js

+        }
+        Event.prototype.preventDefault = function() {
+          this.defaultPrevented = true;
+        };


What is the purpose of this function?

It defines a javascript class which is instantiated for each listener among the tree of scopes.

By this way, each listener received a dedicated event with a stable currentScope property.

But why does this class need a preventDefault? There is not action being taken there other then altering this.defaultPrevented. This seems misleading to me.

I retained the existing preventDefault.

I asked myself for the purpose of this property. Perhaps to be consistent with DOM events.

I feel it should be removed, this may have been a mistake in the angular api. Events without a native default should not have a preventDefault method. @lgalfaso thoughts?

I think they should have preventDefault even when in the case of a $broadcast it does nothing

@lgalfaso why? I don't believe it does anything for $emit either. Its just basically a misleading noop.

@Fresheyeball the event used to have a function called preventDefault and that should not be removed, user may call it (even if it does nothing).
In the case of $emit, calling preventDefault does prevent the event from going up

@lgalfaso thats even worse. Why would preventDefault be stopPropagation? There is no browser default, so there should be no prevent default. That is exactly why this is bad.

@Fresheyeball There are two things involved there

This PR is to fix an existing issue with events current parameter (that affects $broadcast and $emit). As part of this fix, it is required that this does not break what people expect from the API, this includes that there is a function called preventDefault. How this function works is not part of this PR.

Now, preventDefault is part of the DOM standard http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event-preventDefault. This patch is not about adding/removing or being consistent on the fact that there is a preventDefault function and not stopPropagation nor on the fact that preventDefault works in one way when calling $broadcast and in another way when calling $emit. If you feel that this is a limitation you are welcome to put a PR in place

Travis build should be failed with this commit (meaning that the test case in failure is relevant for the bug seen in angular#7445). MUSTN'T BE MERGE

ncuillery · 2014-05-14T10:08:41Z

@lgalfaso Are you sure ? ;) I create a "trashable" PR with only the change on rootScopeSpec.js and it failed exactly as I expect :

Scope events $broadcast listener should receive event object FAILED
    Expected { ..., $id : '003', ... } to be { ..., $id : '002', ... }.

The currentScope is the grandChild (003, end of the scope traversal) instead of the child (002, listening to the event).

PR
Commit
Travis build

In current master, the currentScope is 002 because it's the scope listening to event and, by chance, the end of the scope traversal. But it's an "abstract" particular case (AngularJS apps rarely have only two scopes). That's why I'm a bit reluctant to add a new test case.

I can add a test case with both synchronous and asynchronous expectations but it will globally look like the existing one with 3 scopes instead of 2 and a different description in it function...

lgalfaso · 2014-05-14T14:05:01Z

@ncuillery you are right, the test fails with the new scope, my bad. There are two aspects to be talked about

There might be apps that make use of the fact that the same event object was used throughout the $broadcast
The patch creates a new array and a new Event per call, this may have some performance implications

On top, there is a real issue with $emit, if this patch moves forward, then $emit needs a similar fix

ncuillery · 2014-05-14T16:13:57Z

Yes indeed, I was wrong when I said there is no bug in $emit. The currentScope is unstable too :
Plunker

I'm working on a fix for $emit, in the same way as $broadcast for the moment (so potentially with performance impacts).

The event.currentScope in `$emit` method was a reference to an iterator. When accessing asynchronously, the event.currentScope was the $rootScope (end of the do/while loop) instead of the scope which listen the event. The `$emit` and `$broadcast` methods use the same syntax now. Both methods are tested with : - sync/async access to `event.currentScope` and `event.targetScope`. - a test case for `preventDefault()` and `defaultPrevented`.

They are modified version of $emit and $broadcast (see angular#7445)

ncuillery · 2014-05-19T11:59:56Z

Now $broadcast and $emit are both fixed and well-tested. Their syntax are consistent. @lgalfaso Do you approve ? (see bd131bb)

I investigated performances implications with jsperf:

The test cases call the new and the original $broadcast method many times on a complete B-tree of ~4000 scopes (5-B-tree with a deep of 6).

I change the number of listeners in different test cases (the tree is the same):

0 listener (only the scope traversal) --> 80/90% slower
1 listener --> 40% slower
40 listeners --> 10/20% slower
400 listeners --> 5/10% slower

I think the huge lack of perfs (when there is little or no listener) is not relevant because jsperf calibrate the number of execution from the average time of execution. In the 0-listener test case, Chrome still handle 130k calls to $broadcast per second. I suppose it doesn't reflect reality... (even the unique listener test case with 10k $broadcast per seconds...)

The results are greatly influenced by the content of the listeners because their execution time is included in the measured time. Tests above have a console.log in the listener.

The results are very unprofitable when listeners are empty:

40 empty listeners --> 50% slower instead of 10% with a console.log();

At the opposite, they become better when the listener changes a variable of the scope (a common use case):

1 listener which produces dirty checking --> 5/10% slower instead of 40%
40 listeners which produce dirty checking --> 0% slower with Chrome !

I objectively think that the updated $broadcast method is slower but the situations where it can be critical are consequences of a bad-form code. (mass calls to $broadcast, empty listeners, ...)

I hope this will help. I'm not really a JS perf tester, so feel free to analyze and criticize this post. I present only time-based tests, should I do memory-based tests ? with Chrome dev tools ?.

Others tests for $emit will soon be here.

caitp · 2014-05-20T15:40:53Z

why create Event twice? what does that gain?

caitp · 2014-05-20T15:43:29Z

test/ng/rootScopeSpec.js

            event = e;
          });
          scope.$broadcast('fooEvent');

-          expect(event.name).toBe('fooEvent');
+          // Asynchronous expectation : current & target should be the same as above


I just moved this expectation few lines above (in child.$on listener function L1725).

I made this to be consistent with the existing similar test case for $emit L1570.

it's not asynchronous, and you should not give the impression that it's asynchronous

I made this to be consistent with the existing similar test case for $emit

That's a test case you wrote yourself, it's not present in angular core! Just remove the "Asynchronous expectation..." bit

https://github.com/angular/angular.js/blob/master/test/ng/rootScopeSpec.js#L1570

You understand that I'm referring to the comment, right?

That scope event handler is not being called asynchronously, there is nothing asynchronous happening here, it's all within a single turn of the event loop

Oops. No, I didn't. Sorry for that.

caitp · 2014-05-20T15:49:41Z

If I understand this, you're creating a bunch of garbage (to be collected) for each scope an event bubbles over to, and this is not likely to have any kind of performance or usability benefit. What it's going to do is cause the GC to have a lot of work to do, which will cause pauses in the application

What are you trying to accomplish here?

It caused problems when using asynchonously the event object : the event.currentScope is not the scope which listen the event.

This is a non-issue, just save currentScope to a variable in your event handler. Event dispatch is synchronous, you should expect it to be treated as synchronous.

Further, currentScope should nearly always just refer to whatever $scope you happen to have on hand, so you have a stable reference to it there regardless

ncuillery · 2014-05-20T18:32:23Z

If I understand this, you're creating a bunch of garbage (to be collected) for each scope an event bubbles over to, and this is not likely to have any kind of performance or usability benefit. What it's going to do is cause the GC to have a lot of work to do, which will cause pauses in the application

During the traversal ($broadcast) or the bubbling ($emit), there is no memory implication for scopes which don't listen the event.

This is a non-issue, just save currentScope to a variable in your event handler.

I never said there is no work-around. I just find that it could be painful for a developer to understand what is happening when the currentScope is not the one you are looking for.

The way the currentScope works is misleading. Before my PR, the Scope events $broadcast listener should receive event object passed by sheer coincidence (with the tree of 2 scopes, the scope which listen the event is also the one referred by currentScope at the end of traversal/propagation). See #7454.

Event dispatch is synchronous, you should expect it to be treated as synchronous.

Maybe the docs should mention that.

caitp · 2014-05-20T19:35:56Z

Event dispatch is synchronous, you should expect it to be treated as synchronous.

Maybe the docs should mention that.

PRs welcome 😺

IgorMinar · 2014-05-20T21:05:33Z

Sorry guys, but I think that this is a wrong approach. If the developer cares about the currentScope being the right thing in an asynchronous context then the developer needs to grab the reference when the even is delivered to them.

The scope event system is modeled as a simplified DOM event system, so currentScope and targetScope are analogous for currentTarget and target. In case of DOM the currentTarget is simply set to null when the event is done propagating. And this is exactly what we should do as well.

When a event is finished propagating through Scope hierarchy the event's `currentScope` property should be reset to `null` to avoid accidental use of this property in asynchronous event handlers. In the previous code, the event's property would contain a reference to the last Scope instance that was visited during the traversal, which is unlikely what the code trying to grab scope reference expects. BREAKING CHANGE: $broadcast and $emit will now reset the `currentScope` property of the event to null once the event finished propagating. If any code depends on asynchronously accessing thei `currentScope` property, it should be migrated to use `targetScope` instead. All of these cases should be considered programming bugs. Closes angular#7445"

When a event is finished propagating through Scope hierarchy the event's `currentScope` property should be reset to `null` to avoid accidental use of this property in asynchronous event handlers. In the previous code, the event's property would contain a reference to the last Scope instance that was visited during the traversal, which is unlikely what the code trying to grab scope reference expects. BREAKING CHANGE: $broadcast and $emit will now reset the `currentScope` property of the event to null once the event finished propagating. If any code depends on asynchronously accessing thei `currentScope` property, it should be migrated to use `targetScope` instead. All of these cases should be considered programming bugs. Closes angular#7445

IgorMinar · 2014-05-20T21:30:31Z

I have a fix pr here: #7523 can someone please review?

ncuillery · 2014-05-21T07:01:47Z

@IgorMinar This explanation suits me well, and your fix solves the ambiguous situations I describe.

When a event is finished propagating through Scope hierarchy the event's `currentScope` property should be reset to `null` to avoid accidental use of this property in asynchronous event handlers. In the previous code, the event's property would contain a reference to the last Scope instance that was visited during the traversal, which is unlikely what the code trying to grab scope reference expects. BREAKING CHANGE: $broadcast and $emit will now reset the `currentScope` property of the event to null once the event finished propagating. If any code depends on asynchronously accessing thei `currentScope` property, it should be migrated to use `targetScope` instead. All of these cases should be considered programming bugs. Closes angular#7445 Closes angular#7523

style: add missing semi-colon

814f0e0

lgalfaso self-assigned this May 13, 2014

lgalfaso added component: scopes labels May 13, 2014

lgalfaso removed their assignment May 13, 2014

ncuillery added cla: yes and removed cla: no labels May 13, 2014

Fresheyeball reviewed May 14, 2014
View reviewed changes

ncuillery mentioned this pull request May 14, 2014

test($rootScope): voluntary failure for PR #7445. #7454

Closed

ncuillery added a commit to ncuillery/angular.js that referenced this pull request May 16, 2014

Add an angular.js file with $broadcastNew and $emitNew

2a596b5

They are modified version of $emit and $broadcast (see angular#7445)

caitp reviewed May 20, 2014
View reviewed changes

IgorMinar mentioned this pull request May 20, 2014

fix(Scope): $broadcast and $emit should set event.currentScope to null #7523

Closed

IgorMinar added this to the Purgatory milestone May 20, 2014

IgorMinar closed this in 82f45ae May 21, 2014

ncuillery mentioned this pull request May 23, 2014

Unable to access event.currentScope in $stateChangeSuccess angular-ui/ui-router#1005

Closed

ncuillery deleted the fix branch May 23, 2014 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($rootScope): avoid unstable reference in $broadcast event #7445

fix($rootScope): avoid unstable reference in $broadcast event #7445

ncuillery commented May 13, 2014

mary-poppins commented May 13, 2014

lgalfaso commented May 13, 2014

ncuillery commented May 13, 2014

lgalfaso commented May 13, 2014

Fresheyeball May 14, 2014

ncuillery May 14, 2014

Fresheyeball May 14, 2014

ncuillery May 14, 2014

Fresheyeball May 14, 2014

lgalfaso May 14, 2014

Fresheyeball May 15, 2014

lgalfaso May 19, 2014

Fresheyeball May 19, 2014

lgalfaso May 20, 2014

ncuillery commented May 14, 2014

lgalfaso commented May 14, 2014

ncuillery commented May 14, 2014

ncuillery commented May 19, 2014

caitp commented May 20, 2014

caitp May 20, 2014

ncuillery May 20, 2014

caitp May 20, 2014

ncuillery May 20, 2014

caitp May 20, 2014

ncuillery May 20, 2014

caitp commented May 20, 2014

ncuillery commented May 20, 2014

caitp commented May 20, 2014

IgorMinar commented May 20, 2014

IgorMinar commented May 20, 2014

ncuillery commented May 21, 2014

fix($rootScope): avoid unstable reference in $broadcast event #7445

fix($rootScope): avoid unstable reference in $broadcast event #7445

Conversation

ncuillery commented May 13, 2014

mary-poppins commented May 13, 2014

lgalfaso commented May 13, 2014

ncuillery commented May 13, 2014

lgalfaso commented May 13, 2014

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

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

ncuillery commented May 14, 2014

lgalfaso commented May 14, 2014

ncuillery commented May 14, 2014

ncuillery commented May 19, 2014

caitp commented May 20, 2014

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

Choose a reason for hiding this comment

caitp commented May 20, 2014

ncuillery commented May 20, 2014

caitp commented May 20, 2014

IgorMinar commented May 20, 2014

IgorMinar commented May 20, 2014

ncuillery commented May 21, 2014