Re-instrumenting an undefined function will Result in infinite recursion

[hernaneg350]

API Usage for StacktraceJS defines the following example for function instrumentation:

function logStackTrace(stack) {
    console.log(stack.join('\n'));
}

var p = new printStackTrace.implementation();
p.instrumentFunction(this, 'baz', logStackTrace);

function foo() {
    var a = 1;
    bar();
}

function bar() {
    baz();
}
foo(); //Will log a stacktrace when 'baz()' is called containing 'foo()'!

p.deinstrumentFunction(this, 'baz'); //Remove function instrumentation

Now let's suppose baz was never defined inside context this, and instrument again:

p.instrumentFunction(this, 'baz', logStackTrace);

This will result in an infinite recursion. To analyze deeper the cause of this, I took a look at the code:

instrumentFunction: function(context, functionName, callback) {
    context = context || window;
    var original = context[functionName];
    context[functionName] = function instrumented() {
        callback.call(this, printStackTrace().slice(4));
        return context[functionName]._instrumented.apply(this, arguments);
    };
    context[functionName]._instrumented = original;
}

See how after the first call to instrument function, our undefined this.carl will:

1. Call the callback we passed in
2. Call this.carl._instrumented which was set to the original value of this.carl (was 'undefined' at the time) immediately after function instrumented was defined and assigned to this.carl.

Now we are expecting deinstrument to reverse everything and leave our this.carl in its original value so we can reinstrumenting back, right?

Well let's look at deinstrument:

deinstrumentFunction: function(context, functionName) {
    if (context[functionName].constructor === Function &&
        context[functionName]._instrumented &&
        context[functionName]._instrumented.constructor === Function) {
        context[functionName] = context[functionName]._instrumented;
    }
}

See how we are checking the value of this.carl._instrumented to see it is not false or undefined and that it's constructor is a function. If these requirements are not met, then we won't reset this.carl to it's original value of 'undefined'.

Then this.carl will still contain function instrument, and looking at instrumentFunction one more time, you'll see that it will:

1. Save the original value of this.carl (currently the function instrument) into a value original
2. Define a function instrument that calls this.carl._instrument
3. Set this.carl._instrument to be this.carl.

In summary, we will have this.carl as a function that calls internally this.carl._instrument, which is the function instrument again, as it is the value this.carl had after the first instrumentation and even after deinstrumenting, as the value this.carl had before was not restored.

The thing here is, I don't exactly know how the API is supposed to work in this case, what the expectations are. It can be anything. Maybe we were never supposed to use instrumentFunction passing an undefined function. But then again the Usage examples should not leave this unreferenced and add a definition for 'baz', as the lack of definition for 'baz' suggests this function is undefined, something that given our premise is not supported. Or at least, throw an exception.

If the API supports undefined as the function parameter into instrumentFunction, then either instrumentFunction or deinstrumentFunction should be corrected addressing this issue.

[eriwen]

Fixed for v1.0 with commit 4c9683d59c7bdeb42d1bbd73ae1972d91388e622