Fixes #1540

[Nthalk]

As said in #1540, things like hitting enter on a form that cause a submit that destroy a view remove the handler for the actionId of things like "focusIn".

When the action is triggered and the handler has already been removed it should just be ignored since it's been intentionally removed.

I tested it with rake test[all], and everything was peachy.

I hope you guys pull this, because the error messages are filling up my logs, Thanks!

[trek]

This looks good to me. Probably @tomdale should review quickly.

[lukemelia]

I believe @kselden mentioned this bug this weekend. Perhaps he can weigh in on this solution.

[krisselden]

Need a failing test demonstrating the problem

[tomdale]

This seems like a fine fix, but @kselden is right: we need a unit test. I believe there are plans to revamp the action helper in the future and we should make sure we don't regress.

[krisselden]

I know this is more complicated than described, action helpers are unregistered on willClearRender when a view is destroyed, this can only happen if you get an event to be dispatched during the window between willClearRender and the actual DOM removal.

[krisselden]

I'll take a jsfiddle or jsbin as well.

[sly7-7]

@kselden There is a jsFiddle in #1540. http://jsfiddle.net/teyY5/1/
I wonder if this is also related to #1045 and #1279 ?

[wagenet]

@kselden Are you up for writing a test based on the mentioned fiddle?

[krisselden]

@sly7-7 what do I do to reproduce the issue with that jsfiddle?

[sly7-7]

@kselden if you fill the input the click on submit, there is no error, but if you press enter, the following error is thrown: Uncaught TypeError: Cannot read property 'handler' of undefined

[mikegrassotti]

+1, i was having same issue - app worked fine in chrome but poltergeist integration tests were failing. This patch solved it for me, would be happy to write a unit test if that would help

[wagenet]

Unit tests are almost always useful. It sounded like @kselden was going to
take a shot but it seems like he probably hasn't started yet.

[sly7-7]

@kselden Did you find the time to work about it ? A SO question is about it: http://stackoverflow.com/questions/14048030/emberjs-breaks-with-action-is-undefined to here

[kalien]

Hi,

I am the author of the SO question mentioned above.
FYI I tried patching Ember with the fix provided here but it didn't work for me. It hid the javascript error I had but it didn't solve the underlying issue of having 'registeredActions' empty.
As mentioned in the SO question, an example which demonstrates the problem is published at http://www.follow-nature.com/jsfiddle/sn.html (I didn't manage to run this example in jsfiddle). I hope this example could be used instead of unit test.
The steps to recreate the problem are:

• Open the URL. A login popup appears.
• Click on 'Login' (no need to enter any username/password). Tabs widget appears.
• Click on 'Add New Document'. Add New Document popup appears.
• Click on 'Cancel'. The Add New Document popup disappears.
• Hover over 'Add New Document' button.

PROBLEM: Ember breaks with 'action is undefined' error.

[kalien]

Additional info after examining how Ember behaves...
Looks like the underlying issue is that when one specific outlet gets connected or disconnected the framework unregisters actions associated with all outlets even though only one outlet gets changed. This way we are losing pefrectly valid actions.
Based on my still limited understanding of Ember when connect/disconnect happens for an outlet then only actions within this specific outlet has to be removed

[kalien]

I managed to spend some time on this issue and I realized that my previous comment could be a bit misleading. I debugged the issue up to the point where I realized that Views sometimes have wrong children listed in their '_childViews' attribute. In my case when the "Add New Document" popup appears its forEachChildView method gets called. At this point in time its '_childViews' attribute contains wrong views and Ember unregisters all actions within these views (including the "Add New Document" button action). Therefore "Add New Document" button doesn't have an action associated with it any more.

[Nthalk]

Kalien, this isn't a childView issue, it's a runloop issue. Since ember runs willClearRenderer which clears the ActionHandler before removing the element, it's possible to trigger a dom event that references a removed handler.

In a case like that, it should safely be ignored

[krisselden]

@Nthalk that test is too artificial, during a loop, an action shouldn't trigger between willClearRender and the element actually being removed. The test should show how this issue actually happens in an app, not by calling internal methods.

[krisselden]

https://www.evernote.com/shard/s121/sh/600772dd-e7c0-4996-85f3-aee1a786d50c/ceed60027e5a9c732fc6834dbe40ce9d

[Nthalk]

@kselden I understand that, but I fear that this is exactly what is happening -- And I would need a full ember app with a router to properly demonstrate it like in the jsfiddles shown here -- That's not very unity, is it?

If you think you can help me out with this, that would be greatly appreciated.

[Nthalk]

In this particular instance, it's as if some UI loop is queuing events to fire, but doing it in js land prevents the actual path from being traversed.

[kalien]

Hi guys, I am a bit confused here. Probably we talk about two different issues here. If this is the case then let me know if you prefer to have my scenario tracked in a separate report.

In the example shown at http://www.follow-nature.com/jsfiddle/sn.html I have a complete Ember app which doesn't use any internal APIs. After following the steps mentioned above the app becomes unusable because actions get removed incorrectly. In my case the "Add New Document" button cannot be used anymore. I cannot simply ignore the errors. The app is broken after the last step.

[krisselden]

The submitted test in this pull doesn't actually reflect the fiddle, the fix here is covering up the actual issue in that jquery actually synchronously synthesizes events. This isn't a run loop issue in that we unregister and remove from the DOM later, it is that we are trying to unregister and remove together but jquery is reentering the event dispatcher in the DOM removal.

[Nthalk]

@kselden Since we know that jQuery is doing this, and that the best way to emulate it is to remove the dom element and fire a stale handler, why is the test bad?

Is it wrong to defensively code against a (non-)issue with a library that we use to do something?

What jQuery is doing isn't inherently wrong, but it breaks our case. Can't we just throw a comment in there stating this?

Just what about the patch is so controversial?

[krisselden]

I just want to guard against reentry in the event dispatcher and was hoping for a test that reproduced that is all. This defense is just further down than I would like.

[wagenet]

I forgot about this ticket and ended up committing the same fix here: aafb5eb. Nevertheless, I'm leaving this ticket open since I do think there's a need for a more thorough fix.

[wagenet]

@kselden, what are your current thoughts on this?

[krisselden]

@wagenet my current thought is that the run loop should have a join() method that joins an existing loop or creates one and that the event triggering should be scheduled as an action, so that if you somehow reenter the event dispatcher, the event will still be async.

[stefanpenner]

@kselden now that backburner is in, I can role the join this evening.