Using observes in child class of Ember.Object breaks Ember Data, eachComputedProperty(), proto() in Firefox

[matchwood]

JS BIN: http://emberjs.jsbin.com/qiweyu/edit?html,js,output

Breaks in eg Firefox 40 on Ubuntu 14.04, Windows 7, Windows 8.1
Works in eg Chrome on Ubuntu, IE9 on Windows 7

Setup is simple: create an Ember Object with some properties.

var Foo = Ember.Object.extend({
  firstName: null,
  lastName: null
});

Create a child class that extends this object and that defines an observer

var Bar = Foo.extend({
  firstNameObserver: function() {
  }.observes('firstName')
});

Try to iterate over the properties in Bar in the same way as eachComputedProperty does (via _computedProperties)

var props = Bar.proto();
for (var name in proto) {
  console.log(name);
}

and firstName has disappeared!

Bizarrely, console.log(props) still shows firstName as being there, but the iterator seems to be skipping it.

Why this is a massively breaking issue

Ember Data uses eachComputedProperty as part of pushing data into models. This method does not fire for any properties with the bug above. This means Ember Data just silently ignores whatever is in the payload and produces a model without any data for, eg., firstName.

In turn this effectively means that the entire user interface of my application is broken wherever this pattern above occurs.

Options

Docs should state that .observes can only be used for properties in the same class, or some fix for Firefox needs to be found. I have no ideas at all why Firefox is breaking here - I have dug enough into Ember to know that if you block replaceObserversAndListeners

ember.js/packages/ember-metal/lib/mixin.js

Line 351 in 0f5c7f5

function replaceObserversAndListeners(obj, key, observerOrListener) {

(on 1.13 at least) the bug does not appear, so it must be something going on deep in the addObserve/Listener code but I don't have the time to dig any further at the moment.

[Emrvb]

Could you supply a test/demo where this is actually broken?

The behaviour you describe for Firefox is consistent for several Ember versions and I've been extending models in the past and never hit anything like this with Firefox (could have missed it though).

As far as I can tell, the way ember-data works with eachComputedProperty, should work as intended.

[matchwood]

Firstly, thanks for reading and replying!

Here is another jsbin: http://emberjs.jsbin.com/sunasa/edit?html,js,console,output

In that jsbin I am getting the same issue (in Firefox only). Iterating over the attributes of a model created from the store skips over the property with an observer. Is that not happening for you?

Putting together a demo that shows the specific context I ran across this is a bit more timeconsuming as I may have to configure adapters and so forth, but I think this demonstrates there is an issue pretty well - there is no way eachAttribute should suddenly start skipping attributes because you happen to observe them in a child class. Any code at all that relies on eachComputedProperty can be broken by this - at least that is how it seems to me...

[Emrvb]

Oh my... I think I'm confirming this one. Your second jsbin gives a much clearer example of the issue.

In this example I can confirm the following in Firefox:

• While using the Ember Inspector on the instance of the bar model, lastName is not recognized as attribute (it does show up when you look at its parent class Foo).
• When persisting the model using save(), the payload does not contain the lastName.
• Same behavior in Ember 1.13.x

While potentially breaking ember-data implementations, you are right the problem goes much deeper. To me it seems it definitely breaks eachComputedProperty but since it also affects regular attributes on some level (as shown in your first jsbin), who knows what else might "not work as expected".

I'm not sure I'm experienced enough with Ember to debug this, although I'm very curious so I might even try. In the mean time, I'd suggest rewording your issue. I think removing the "Nasty deep bug" part would help to be taken more seriously.

[matchwood]

Glad that it wasn't just me going crazy!

It is quite worrying - I was staggered when I first switched to Firefox and realised that half my models no longer existed!

Haha, thanks for the advice on title - any suggestions on making it look a bit more serious? I'm not very experienced in issue reporting...

[matchwood]

Oh, and as I mentioned in the first post, it seems to be caused by the observer setup that happens in the replaceObserversAndListeners call. I suspect that it will have something to do with the way properties are inherited and added to the object produced by proto() (produced here https://github.com/emberjs/ember.js/blob/v2.1.0/packages/ember-runtime/lib/system/core_object.js#L201) - possibly something that relates to some kind of strange hasOwnProperty check that Firefox is doing automatically when you use for to loop through an object. I imagine that Firefox is viewing that observed property as lower down the prototype chain and therefore ignores it. But that is just speculation really! This might be helpful: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties

[Emrvb]

Using the docs you provided, here is a VERY DIRTY workaround if you require an immediate fix for Firefox: http://emberjs.jsbin.com/qatanud/edit?html,js,console,output

I believe the bug itself is the creation of the (mandatory) getters and setters during the extend of the class, where the computed property is not correctly recognized as a descriptor.

[matchwood]

Ah good work, thanks. I have already had to refactor my code in the more obvious breaking places to copy definitions into the child-most class but I could well have missed some so it is good to have a temporary hack to cover those situations.

Just by the way, _computedProperties is not the only place proto is used so even with this workaround there may be subtle breakages throughout Ember. No time today but I might look tomorrow at hacking something into proto that fixes this.

[Emrvb]

Today I found some time to look into this issue. Checked out master and wrote a test. I was very confused when my test succeeded with Firefox.

It's already fixed: #12314

[matchwood]

Ah well good! Thanks for looking into it - I should have tried the canary build in the jsbin. I am still a bit uncertain though, because the first jsbin I posted still fails so there is clearly still something weird going on.

However, I'm not sure at this point if that affects any public facing apis, but if I find anything I'll open up another issue for it with.