long chains without overflowing the stack fails on FF 10-23

[mbest]

The test Dependent Observable Should allow long chains without overflowing the stack fails on FF 10-23 with InternalError: too much recusion.

Noting: #358 & #359.

[brianmhunt]

@mbest Any thoughts on the best way to handle this? A few come to mind, and I had thought about putting a PR together but I noticed you had already done some work on this already and I'm sure would know better than I do where to start.

The basic options seem to be:

1. Lower the depth in the test;
2. Make an explicit exception in the test for FF 10-23;
3. Employ a technique like allow long subscriber chains #359 (at src/subscribables/subscribable.js notifySubscribers)

I noted that the depth in the current test is 200, but is 10,000 in the patch.

My personal inclination is towards a patch like #359, but I'd be gambling that there are no side-effects.

[mbest]

While investigating this, I decided to run the test against previous versions to figure out when this started failing. It was working with Knockout 3.2.0, but not with 3.3.0. I'll look into it some more.

[mbest]

Okay. So this is one way to fix this issue. This more-or-less mirrors what I did originally (688a567, #1088): reducing the function call depth for each iteration and eliminating the use of call and bind.

[brianmhunt]

This undoes the performance benefits of #1841; in most cases a closure will be more memory and slower than a function on the prototype chain, notably with newer browsers.

[mbest]

If we're okay with the test failing in older FF, then we can simply ignore these changes and possibly skip the test for those browsers. After all, the test already failed with version 3.3.0.

[brianmhunt]

I'm okay with that, and as you say, going forward the use of deferred updates makes this issue go away. @SteveSanderson ?

I think then the answer might be to just reduce the number of iterations to 100. Legacy FF should pass with that (I think it succeeds with 130 or so, but not positive– if we go this route, I'll fix it up).

[mbest]

I've made a further commit to undo some of my previous changes. This keeps the logic of moving the "change" notification to evaluatePossiblyAsync, reducing a couple layers of function calls from the recursion. This still requires reducing the depth value in the test to make it pass but allows for a greater depth than doing nothing.

[brianmhunt]

Thanks @mbest – The updated PR looks good to me –– bearing in mind the caveat that I haven't stared at this enough to really understand any potential side-effects or pitfalls.

[mbest]

Thanks. I'm happy with it too. I've merged it now.

[brianmhunt]

I merged the above into the 1360-gulp branch, but as you can see there is still an odd continuing failure:

Firefox 38.0.0 (Linux 0.0.0) Dependent Observable Should allow long chains without overflowing the stack FAILED
InternalError: too much recursion in /home/travis/build/knockout/knockout/src/subscribables/subscribable.js (line 72)
ko_subscribable_fn.notifySubscribers@/home/travis/build/knockout/knockout/src/subscribables/subscribable.js:72:25
computedFn.evaluatePossiblyAsync@/home/travis/build/knockout/knockout/src/subscribables/dependentObservable.js:211:17
ko_subscribable_fn.notifySubscribers@/home/travis/build/knockout/knockout/src/subscribables/subscribable.js:72:25
...
ko_subscribable_fn.notifySubscribers@/home/travis/build/knockout/knockout/src/subscribables/subscribable.js:72:25
observableFn.valueHasMutated@/home/travis/build/knockout/knockout/src/subscribables/observable.js:46:36
observable@/home/travis/build/knockout/knockout/src/subscribables/observable.js:12:17
@/home/travis/build/knockout/knockout/spec/dependentObservableBehaviors.js:516:9

Firefox 17 and 24 pass; all three failed prior builds without this patch. Not sure why FF 38 would have this regression.

[mbest]

I'll check that out. I had tested it with Firefox 16.

[brianmhunt]

Here's a jsFiddle to show stack depth

FF 42 has a call stack of 200–262,000; Chrome is 8–20,000. FF 38.3.0esr has a call stack of 200–284,000 (On Mac OS X). May be a problem only on Linux? That'd be strange.