Error during unsubscribing with null callbacks
Closed this issue · 11 comments
I am seeing an error coming up on occasion which is difficult to determine the exact sequence the causes it, but the symptom is pretty straight forward
On this line of OoObjectObserver in the unsubscribe...
binding/dist/aurelia-binding.js
Line 3165 in abb6ac4
It initializes callbacks to the "callbacks" of the property. However if this is unsubscribed the callbacks variable is set to null vs. an empty array. This if the unsubscribe happens again it will cause an invalid access of .indexOf since callbacks is null.
Ideally object observers should only be unsubscribed once (I agree with this). However I am seeing it called multiple times (triggering this error) on "occasion". Often is seems to be when I have an edit panel that is using a "model" and that "model" is being set based on selection in a grid/table on the page (master/detail sort of thing) and I select a new model. In my case, model is a binding variable, and it is on subscribing to the new value (I assume to update the model that is being watched) the updateRight( ) is calling the disposeRight variable which is the unsubscribe(). however at times I am seeing this as already unsubscribed, where all the internal callbacks are null.
(I had made a comment about data-dismiss. That can be ignored. Changing my button for close from a trigger to a delegate eliminated that error, but this problem above still occurs)
Changing the unsubscribe to this drastically reduced the "break on error" conditions...
OoObjectObserver.prototype.unsubscribe = function unsubscribe(propertyName, callback) {
var callbacks = this.callbacks[propertyName];
if( !callbacks || callbacks.length === 0) {
console.log("property " + propertyName + " had no callbacks, count is " + this.callbackCount);
return;
}
var index = callbacks.indexOf(callback);
...
But I do get a few errors/issues - one that is strange is in the same class where OoObjectObserver handleChanges the variable ii is one larger than the length (which makes no sense unless it is changing via calls to the callback
Here is the handleChanges issue... took a while to catch it. The code on the left in the grey box I added so I could catch the condition where c.length !== callbacks.length
So somehow the callbacks array is being mutated while it is iterating...
I was able to resolve the handleChanges issue by fully initializing my "empty" objects with defaults. It seems if my editor had a visual element for "condition" that was bound to model.condition but it was a new object and model.condition was not initialized to a default value I would see the UndefinedPropertyObserver. Providing default values and the updating them to the live model state "seems" to avoid the handleChanges issue (I have not had it come up in the past hour of clicking away). I also had an issue in handling the subscribers on detach() in which they were not properly getting called. So this was all on my side up to this point (although I do find the UndefinedPropertyObserver case odd)... this is where I do not have the property in model, but my UX is bound to it, and then the model updates to include it (upon selecting a row to populate for example)
I am still seeing the unsubscribe issue denoted at the start of this issue where the callbacks are empty. I am also seeing callback counts in some cases > 0 which is leading me to suspect there could be another subscriber leak on my side again. I can certainly say I have not had a single break on error issue in well over an hour, but as I said I do have cases where the callbackCount > 0 which is problematic.
I am still getting console statements, however I believe I have it running without blowing up.... but hacking around in aurelia/binding is feeling wrong.
I can describe a little more on the conditions that cause the problem....
so I have an edit panel made up of N sub-panels. 1 of these sub-panels is controlled by a if.bind on a boolean property of the model of the editing object. The content on this sub-panel is the form whose bindings are causing the issue above (I have verified all of the properties belong to it). The sequence that triggers it is a follows: select a model where the if.bind condition is false (everything ok). Now select a new object where the if.bind condition is true (everything ok). Now select another new object where the if.bind will re-evaluate to false (BOOM).
The view-model for the component that is being removed has no subscribers, and it only has a @bindable tag to "model". However there are 15 or so form fields (mixture of custom selects, input fields, checkboxes.... all are showing the issue).
I did try wrapping my custom control (that is the sub-panel) in a span and moving the if.bind from the custom element to the span but that did not make a difference. So the issue is basically when the unsubscribers are called (because the sub-panel is being removed via a false if.bind condition ) callbacks and not valid.
I have created a simple project (I suppose I could have done a gist or something now) but it clearly shows the issue......
Simply click a row in the table with a child (true) and then click a row where the child is false. This will cause the if to unload the panel-2 and this will break the binding
Maybe I didn't understand the workaround correctly.... I also tried forking and building the latest in place of binding but I don't think I was successful with that. Here is what I have in my main.js
import {LogManager/* hack fix #178*/, ObserverLocator} from 'aurelia-framework';
import {ConsoleAppender} from 'aurelia-logging-console';
import {TWBootstrapViewStrategy} from 'aurelia-validation';
LogManager.addAppender(new ConsoleAppender());
LogManager.setLevel(LogManager.logLevel.info);
if (window.location.href.indexOf('debug=true') >= 0) {
LogManager.setLevel(LogManager.logLevel.debug);
}
export function configure(aurelia) {
aurelia.use
.standardConfiguration()
//.developmentLogging()
.feature('global-resources')
.plugin('aurelia-i18n', (instance) => {
instance.setup({
resGetPath: 'locale/__lng__/__ns__.json',
lng: 'en',
attributes: ['t', 'i18n'],
getAsync: true,
sendMissing: false,
fallbackLng: 'en',
debug: true
});
})
.plugin('aurelia-dialog')
.plugin('aurelia-validation', (config) => {
config.useDebounceTimeout(250);
config.useViewStrategy(TWBootstrapViewStrategy.AppendToInput);
});
/* hack fix #178 */
if (Object.observe) {
let obj = {
albumRef: -1,
defects: 0,
deception: 0,
sellerRef: -1,
code: 'USD',
condition: 0,
grade: 0,
img: '',
cert: false,
certImg: '',
notes: '',
purchased: '',
pricePaid: 0
};
aurelia.container.get(ObserverLocator).getObserver(obj, 'albumRef');
console.log(obj.__observer__.__proto__.handleChanges = handleChanges);
}
aurelia.start().then(a => a.setRoot('app', document.body));
}
/* hack fix #178 */
var temp = [null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null];
export function invokeCallbacks(callbacks, newValue, oldValue) {
var length, i = length = callbacks.length
while (i--) {
temp[i] = callbacks[i];
}
for (i = 0; i < length; i++) {
temp[i](newValue, oldValue);
temp[i] = null;
}
}
function handleChanges(changes) {
var properties = {}, i, ii, change, propertyName, oldValue, newValue, callbacks;
console.log(">>> handleChanges is getting called");
for (i = 0, ii = changes.length; i < ii; i++) {
change = changes[i];
properties[change.name] = change;
}
for (name in properties) {
callbacks = this.callbacks[name];
if (!callbacks) {
continue;
}
change = properties[name];
newValue = change.object[name];
oldValue = change.oldValue;
invokeCallbacks(callbacks, newValue, oldValue);
}
}
I am still getting the exception when the if.bind removes the panel.... however I am getting the console.log message that handleChanges is getting called.
I can also confirm that the project I submitted above, also fails the same way with this approach. The difference in that is I used
let obj = {
value: '',
on: false
};
aurelia.container.get(ObserverLocator).getObserver(obj, 'value');
@jadrake75 I had the same issue as you. Check out my comment, per @jdanyow's suggestion, in #173.