Uncaught TypeError: Cannot call method 'unchain' of undefined #2031

Closed
KasperTidemann opened this Issue Feb 10, 2013 · 44 comments
@KasperTidemann

Explanation:

Once again, master generates an uncaught type error, this time the following:

Uncaught TypeError: Cannot call method 'unchain' of undefined

Bisecting:

Bisecting from the last commit I know worked, I've found the commit by @kselden, lazier computed properties – don't bother watching (cc4af51) to be the cause of this issue.

Stack trace:

Here is the stack trace from Chrome:

Uncaught TypeError: Cannot call method 'unchain' of undefined (ember.js:2808)
ChainNodePrototype.unchain (ember.js:2808)
ChainNodePrototype.remove (ember.js:2780)
Ember.unwatch (ember.js:3013)
removeDependentKeys (ember.js:3273)
ComputedPropertyPrototype.didChange (ember.js:3415)
propertyDidChange (ember.js:3119)
iterDeps (ember.js:2579)
dependentKeysDidChange (ember.js:2603)
propertyDidChange (ember.js:3122)
set (ember.js:1951)
Ember.Observable.Ember.Mixin.create.set (ember.js:9290)
Em.ArrayController.extend.dataObserver (app.js:2715)
sendEvent (ember.js:3933)
ObserverSet.flush (ember.js:2340)
Ember.endPropertyChanges (ember.js:2366)
Ember.EachProxy.Ember.Object.extend.arrayDidChange (ember.js:11609)
sendEvent (ember.js:3933)
Ember.Array.Ember.Mixin.create.arrayContentDidChange (ember.js:8411)
Ember.ArrayProxy.Ember.Object.extend.arrangedContentArrayDidChange (ember.js:11299)
Ember.ArrayProxy.Ember.Object.extend._arrangedContentDidChange (ember.js:11250)
sendEvent (ember.js:3933)
ObserverSet.flush (ember.js:2340)
Ember.endPropertyChanges (ember.js:2366)
Ember.Observable.Ember.Mixin.create.endPropertyChanges (ember.js:9343)
DS.AdapterPopulatedRecordArray.DS.RecordArray.extend.load ember-data.js:152)
loader.populateArray ember-data.js:6941)
DS.JSONSerializer.DS.Serializer.extend.extractMany ember-data.js:6447)
superWrapper (ember.js:884)
DS.Adapter.Ember.Object.extend.didFindQuery ember-data.js:6944)
(anonymous function) ember-data.js:7761)
(anonymous function) (ember.js:4026)
Ember.handleErrors (ember.js:401)
invoke (ember.js:4024)
tryable (ember.js:4214)
Ember.tryFinally (ember.js:1039)
Ember.run (ember.js:4218)
ajax.success ember-data.js:7760)
(anonymous function) (app.js:2313)
c.emit (socket.io.js:2)
c.onPacket (socket.io.js:2)
d.onPacket (socket.io.js:2)
c.onPacket (socket.io.js:2)
c.onData (socket.io.js:2)
websocket.onmessage

Questions:

@kselden, in my code, I fetch some data that's stored in a DS.AdapterPopulatedRecordArray. I observe the isLoaded event and once it fires, I convert the data to a plain array using data.toArray().

In terms of your refactorizations, do you have any idea what's failing here?

@krisselden
Ember.js member

Yes, removing the dependent keys twice. I would be extremely helpful if you could reduce it to a test or a example on jsfiddle or jsbin.

@krisselden krisselden was assigned Feb 10, 2013
@krisselden
Ember.js member

I suggest you break on error inspect the variables to look at what the property is and the dependency property path.

@KasperTidemann

The line node.unchain(key, path); results in a TypeError: node is undefined that subsequently generates the unchain error. The path is empty:

img

@KasperTidemann

I know this was an issue to others yesterday on IRC as well. Will investigate further today.

@krisselden
Ember.js member

@KasperTidemann so what does the computed property that depends on '@each' look like?

@krisselden
Ember.js member

I need some reproduction steps

@KasperTidemann

Yeah, sorry about the missing fiddle, I'm trying to reproduce in a more simple setup right now.

@KasperTidemann

Still no certain pinpointing of the bug, but here are some findings that @mspisars is also seeing:

  • In the first case, the TypeError: node is undefined error appears because of an attempt to observe data.isLoaded. Here, data equals the aforementioned DS.AdapterPopulatedRecordArray.

  • In the second case, the TypeError: node is undefined error appears because of an attempt to observe arrangedContent.@each.<someProperty>. Observing arrangedContent itself works, but throwing in the @each part causes trouble.

Here is an example of code that creates the first case:

dataObserver: (function() {
  if (this.get('data.isLoaded')) {
    return this.set('content', this.get('data').toArray());
  }
}).observes('data.isLoaded')

... the code doesn't work because of the observes('data.isLoaded') part. If I change it to observes('data') it will work, but then the observer won't react when all data from the findQuery call has been materialized.

(The second case seems to be derived from the first and I still need to dig deeper and isolate parts of the code to be able to say something more specific.)

@krisselden
Ember.js member

Looking forward to the fiddle. FYI, you shouldn't be observing arrangedContent.@each, you should be observing the proxy's @each

@krisselden
Ember.js member

@KasperTidemann any progress on a failing test or fiddle? Anxious to reproduce and fix before the RC

@workmanw

I too have seen this sporadically when using an observer that is dependent on isLoaded or isSaving. I've tried to reproduce a fiddle, and completely failed. I'll continue to try if I can find time later.

@KasperTidemann

@kselden I really wish I could tell you I've narrowed it down to a single case, but I just can't seem to consistently reproduce this error.

It originally happened at times - but not each time - when having a computed property inside an ArrayController that listened for changes to isLoaded in a DS.AdapterPopulatedRecordArray.

What fixed it initially was to use an ArrayProxy instead of the ArrayController. That somehow made this go away. But changing back to using an ArrayController doesn't seem to reproduce this problem anymore, at least not tonight nor yesterday. It seems like the problem resides somewhere else.

All in all, completely useless feedback from me. :( I hope to be able to provide more concrete insight into this if/when the problem returns.

@krisselden
Ember.js member

@wagenet reproduced this in #2043

@KasperTidemann

@kselden and @wagenet, that's awesome. So far, so good.

@krisselden
Ember.js member

@KasperTidemann unfortunately the cause of that fiddle's error was a binding to a view with a null context and adding a 'property.path' to window (which should give an error). Maybe that is the same thing you are causing to happen to via some other mechanism, but I'd like to understand how you are trying to observe isLoading. Can you give some examples of your CP's or Observers?

@KasperTidemann

@kselden, absolutely. Here is the ArrayController code:

ExampleController = Em.ArrayController.extend({
    data: null,
    content: [],
    sortAscending: true,
    sortProperties: ['sort_order'],

    dataObserver: (function() {
        if (this.get('data.isLoaded')) {
            return this.set('content', this.get('data').toArray());
        }
    }).observes('data.isLoaded')

});

Since I re-use the above for different scenarios, an instance of ExampleController is created separately for each of these scenarios (or, in other words: it's not a controller that belongs to a route). I have no idea if that affects anything (automagically speaking), but whenever I need an instance of the above controller, I create it:

someThing.set('exampleController', ExampleController.create());

... and when I've created the controller, I load it with data of a certain model depending on the use case. I do this like so:

someThing.set('exampleController.data', App.store.findQuery(App.ExampleModel, { user_id: <some-user-id> }));

That's pretty much it. And the observes('data.isLoaded') part is what caused the error to happen sporadically.

@wagenet
Ember.js member

We temporarily rolled back this commit. However, I'm keeping this open since we do want to bring this commit back in the future.

@timols

Verifying this again...

@timols

@KasperTidemann can you still reproduce this error in RC5+?

@timols

Can't verify in RC5 and given this is 4 months old, @wycats we may want to close this issue.

@wycats wycats closed this Jun 21, 2013
@mastoj

I found this issue in rc6. The html and code for this problem is here: https://gist.github.com/mastoj/5933960.

I tried to narrow it down further but that removed the problem as a whole. I think it might be a problem that I call result.erPerson, it might be a concept I've missunderstood there. If I remove that line I'll get another problem saying that I can't read the property HarHistorikk, which also is wrong.

I think the ChainNodePrototype.unchain might be missing som null/undefined checks or some values are not set as they should.

@stefanpenner
Ember.js member

@mastoj it would be a massive help if u could assemble a working jsbin or jsfiddle of your gist.

@stefanpenner stefanpenner reopened this Jul 5, 2013
@mastoj

@stefanpenner here you go: http://jsfiddle.net/VzWY2/ (I think it work, jsfiddle is down for me so I'm no sure it got saved). Otherwise I can fix it later this weekend.

I think that it might be some error in my code as well, but that should be handled clearer from ember.

@orjantufte

Same problem here, though my problem is so big that I cant reproduce in a jsfiddle.

@wycats
Ember.js member

paging @kselden

@krisselden
Ember.js member

@wycats this cryptic error means there were more calls to unwatch than there were to watch and the reference counting is already at 0. I'm not sure why it is happening here, but I'm fairly certain it has to do with array proxies and observing length on a chain. @stefanpenner wanted to try his hand at debugging this

@mastoj

FYI, @kselden @stefanpenner, the way I did a work arround was to add a check in ChainNodePrototype.unchain to make sure that node is not undefined. I know this might be a wrong thing to do since that is probably not the root cause to the error, that is only where you see the effect of the error. However, I had to do so until there is final fix for the bug.

@cryptoquick

Hey guys, I've been running into this problem pretty consistently for weeks now. Is there anything I can do to help? This problem "breaks the back button."

I'm seeing this in Ember 1.0.0-rc.7.

@krisselden
Ember.js member

@cryptoquick submit a failing test?

@cryptoquick

Alright, I made a small demo with cdnjs assets, @kselden -- http://jsfiddle.net/cryptoquick/Nrtzq/1/

Click on the product, then, after it takes you to the product, press the back button. You should get an error:

Uncaught TypeError: Cannot call method 'unchain' of undefined

I get this error all the time, even in rc7!

@coladarci

So this error brought me to my knees - I spent all day reverting code to find when it was introduced. What I found was a bit shocking in it's simplicity! Templates driven by ObjectController or ArrayController can't have conditionals that start with a capital letter. There might be a more general rule, to this, but I think this explains my issue (I prefixed a conditional with REMOVE_ to remind my developer to get rid of it - my entire app was brought to it's knees).

Here's an extremely simplified Fiddle: http://jsfiddle.net/6Hsjk/2/

@wagenet
Ember.js member

@coladarci, capital letters denote constants which are handled differently.

@coladarci

As they should - @wagenet, is it reasonable to get expect more meaningful error at some point if the "constant" isn't found?

@cryptoquick - you naming your model attributes in all CAPS is likely your problem.

@cryptoquick

This sounds like legacy behavior. Nowhere is case mentioned in where I'd expect it to be in the Ember docs:
http://emberjs.com/guides/concepts/naming-conventions/

It is mentioned here, but this article is over a year old:
http://www.emberist.com/2012/04/09/naming-conventions.html

I can remove the caps of my model keys with a JSON preprocessor in my Ember Data store adapter code, but if that's the case, either constants should be mentioned in the docs, or this legacy behavior should be updated.

@cryptoquick

Welp, after going on an uppercase witch hunt through our codebase, and altering all the keys that I get back from the API we're using, I have confirmed coladarci's suspicions. This issue is related to capitalization. After changing the keys to lowercase values, no longer am I seeing unchain errors. I'm not sure how my teammates are going to respond to this commit... -sigh-

@stefanpenner
Ember.js member

seems like better assertions would be handy.

@wagenet
Ember.js member

@cryptoquick Can you tell me a bit about your naming conventions? I'd like to understand your situation a bit better.

@cryptoquick

@wagenet They're actually not my naming conventions, but those of an application developer who has since left the company. We have legacy products using the older API, including a mobile app whose codebase is considered sacrosanct, so we're trying to make new products match the older API. I've written some custom serializer methods to accomplish that. Things are working fine now, I just have to, as an additional step, lowercase all object keys as they come back from the server.

@coladarci

@cryptoquick - I'd recommend keeping all model attributes lowercase (much more standard) and then if you can't change your API, just adjust the mapping on your store to route "property" -> "PROPERTY"

@krisselden krisselden added a commit that closed this issue Aug 28, 2013
@krisselden krisselden Don't fire change events while traversing the
chain node graph, instead gather the events and
fire them after the traversal to avoid side effects
of the change events from modifying the chain graph.

fix #2031
fix #3197
9402e9c
@stefanpenner
Ember.js member
@mastoj

@coldarci, if you are a .net-dev it is much more common with capitalized properties which then are exposed through the api.

@artzte

Maybe this is a dumb question, but if there are attributes that are supposed to be constants, could there not be a way of declaring them as such specifically, rather than building in behaviors that affect how computed properties work, based on capitalization? This behavior was really hard for me to track down. It manifested in several bizarre behaviors around calculated properties dependent on capitalized attributes - it was just by luck that in my attempts to figure this out I ran into an exception in ChainNodePrototype.unchain that led me to this thread. I lost many hours circling on this - it smelled like some bizarre conventions-related behavior, but I discarded that theory because usually these problems turn out to be my own mistakes. Fortunately the solution was pretty easy, just aliasing the properties using lowercased names and then using those aliases as dependents. But super hard to track down.

@alexgorbatchev

I'm seeing this issue in 1.7.0. Unfortunately I can't isolate it because I'm upgrading a giant Ember app from 1.5. For me it's happening with nested properties.

@wagenet
Ember.js member

@artze I suspect we will change global behavior in the future. Maybe for 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment