Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] AppInsights stub methods captured incorrect method names in the closure #1283

Closed
chenxinyanc opened this issue Jun 5, 2020 · 4 comments · Fixed by #1284
Closed

[BUG] AppInsights stub methods captured incorrect method names in the closure #1283

chenxinyanc opened this issue Jun 5, 2020 · 4 comments · Fixed by #1284
Labels
Milestone

Comments

@chenxinyanc
Copy link
Contributor

chenxinyanc commented Jun 5, 2020

Description/Screenshot

function _createMethods(methods) {
while (methods.length) {
var name = methods.pop();
// Define a temporary method that queues-up a the real method call
appInsights[name] = function () {
// Capture the original arguments passed to the method
var originalArguments = arguments;
if (!loadFailed) { // If we have detected that the main script failed to load then stop queuing events that will never be processed
// Queue-up a call to the real method
appInsights.queue.push(function () {
// Invoke the real method with the captured original arguments
appInsights[name].apply(appInsights, originalArguments);
});
}
};
}

Note the statement on L227:

var name = methods.pop();

Since var spans the variable scope to the containing function (i.e. _createMethods), the value of this variable gets replaced among each iteration of the while loops.

This causes all the appInsights.[method] calls before ai.2.js has loaded incorrectly forwarded as appInsights.trackEvent calls.

Steps to Reproduce
Copy-paste all the code in snippet.js, or its minimized version on README.md. Then follows the code. Ensure the following code is executed before ai.2.js has been downloaded.

appInsights.addTelemetryInitializer(function (e) { /* dummy */ } );

When running the code, you will find there are 2 trackEvent calls to the actual appInsights object, making 2 extra (invalid) event traces.

image

  • OS/Browser:
  • SDK Version [e.g. 22]: v2
  • How you initialized the SDK: with snippet (snippet.js)

Expected behavior
I should have telemetry initializer set up. And there should be trackPageView called so I should have its telemetry.

Additional context
Add any other context about the problem here.

@chenxinyanc chenxinyanc changed the title [BUG] AppInsights stub methods fails to capture method name in the closure [BUG] AppInsights stub methods captured incorrect method names in the closure Jun 5, 2020
@MSNev
Copy link
Collaborator

MSNev commented Jun 5, 2020

NICE CATCH! - the issue is on line 234 as you identified where it's using effectively the last popped name!

@MSNev MSNev added the bug label Jun 5, 2020
@MSNev MSNev added the fixed - waiting release PR Committed and waiting deployment label Jun 5, 2020
@MSNev
Copy link
Collaborator

MSNev commented Jun 5, 2020

Reopening for release tracking

@MSNev MSNev reopened this Jun 5, 2020
@MSNev MSNev added this to the 2.5.6 milestone Jul 2, 2020
@MSNev MSNev removed the fixed - waiting release PR Committed and waiting deployment label Jul 8, 2020
@MSNev
Copy link
Collaborator

MSNev commented Jul 8, 2020

Release is now fully deployed

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants