Fixed an off-by-one error for JavaScript processing

.eslintignore

@@ -1,4 +1,5 @@
 **/dist/**/*
 **/vendor/**/*
 **/tests/sentry/lang/javascript/fixtures/**/*
+**/tests/sentry/lang/javascript/example-project/**/*
 /examples/

src/sentry/lang/javascript/processor.py

@@ -505,9 +505,15 @@ def handles_frame(self, frame, stacktrace_info):
         platform = frame.get('platform') or self.data.get('platform')
         return (settings.SENTRY_SCRAPE_JAVASCRIPT_CONTEXT and platform == 'javascript')
 
+    def preprocess_frame(self, processable_frame):
+        # Stores the resolved token.  This is used to cross refer to other
+        # frames for function name resolution by call site.
+        processable_frame.data = {
+            'token': None,
+        }
+
     def process_frame(self, processable_frame, processing_task):
         frame = processable_frame.frame
-        last_token = None
         token = None
 
         cache = self.cache
@@ -541,8 +547,6 @@ def process_frame(self, processable_frame, processing_task):
                 }
             )
         elif sourcemap_view:
-            last_token = token
-
             if is_data_uri(sourcemap_url):
                 sourcemap_label = frame['abs_path']
             else:
@@ -554,7 +558,7 @@ def process_frame(self, processable_frame, processing_task):
                 # Errors are 1-indexed in the frames, so we need to -1 to get
                 # zero-indexed value from tokens.
                 assert frame['lineno'] > 0, "line numbers are 1-indexed"
-                token = sourcemap_view.lookup_token(frame['lineno'] - 1, frame['colno'])
+                token = sourcemap_view.lookup_token(frame['lineno'] - 1, frame['colno'] - 1)
             except Exception:
                 token = None
                 all_errors.append(
@@ -567,6 +571,9 @@ def process_frame(self, processable_frame, processing_task):
                     }
                 )
 
+            # persist the token so that we can find it later
+            processable_frame.data['token'] = token
+
             # Store original data in annotation
             new_frame['data'] = dict(frame.get('data') or {}, sourcemap=sourcemap_label)
 
@@ -596,12 +603,18 @@ def process_frame(self, processable_frame, processing_task):
                 # Token's return zero-indexed lineno's
                 new_frame['lineno'] = token.src_line + 1
                 new_frame['colno'] = token.src_col
+                last_token = None
+                if processable_frame.previous_frame:
+                    last_token = processable_frame.previous_frame.data['token']
+
                 # The offending function is always the previous function in the stack
-                # Honestly, no idea what the bottom most frame is, so we're ignoring that atm
+                # Honestly, no idea what the bottom most frame is, so
+                # we're ignoring that atm.
+                #
+                # XXX: we should actually be parsing the source code here
+                # and not use the last token
                 if last_token:
                     new_frame['function'] = last_token.name or frame.get('function')
-                else:
-                    new_frame['function'] = token.name or frame.get('function')
 
                 filename = token.src
                 # special case webpack support

src/sentry/lint/engine.py

@@ -45,8 +45,8 @@ def get_files(path):
 def get_modified_files(path):
     return [
         s
-        for s in check_output(['git', 'diff-index', '--cached', '--name-only', 'HEAD'])
-        .split('\n') if s
+        for s in check_output(['git', 'diff-index', '--cached', '--name-only', 'HEAD']).split('\n')
+        if s
     ]
 
 
@@ -68,7 +68,6 @@ def get_js_files(file_list=None):
     if file_list is None:
         file_list = ['tests/js', 'src/sentry/static/sentry/app']
     return [x for x in get_files_for_list(file_list) if x.endswith(('.js', '.jsx'))]
-    return file_list
 
 
 def get_python_files(file_list=None):
@@ -178,6 +177,10 @@ def js_format(file_list=None):
         return False
 
     js_file_list = get_js_files(file_list)
+
+    # manually exclude some bad files
+    js_file_list = [x for x in js_file_list if '/javascript/example-project/' not in x]
+
     return run_formatter(
         [
             prettier_path, '--write', '--single-quote', '--bracket-spacing=false',

src/sentry/stacktraces.py

@@ -22,14 +22,18 @@
 
 
 class ProcessableFrame(object):
-    def __init__(self, frame, idx, processor, stacktrace_info):
+    def __init__(self, frame, idx, processor, stacktrace_info, processable_frames):
         self.frame = frame
         self.idx = idx
         self.processor = processor
         self.stacktrace_info = stacktrace_info
         self.data = None
         self.cache_key = None
         self.cache_value = None
+        self.processable_frames = processable_frames
+
+    def __repr__(self):
+        return '<ProcessableFrame %r #%r>' % (self.frame.get('function') or 'unknown', self.idx, )
 
     def __contains__(self, key):
         return key in self.frame
@@ -40,6 +44,13 @@ def __getitem__(self, key):
     def get(self, key, default=None):
         return self.frame.get(key, default)
 
+    @property
+    def previous_frame(self):
+        last_idx = len(self.processable_frames) - self.idx - 1 - 1
+        if last_idx < 0:
+            return
+        return self.processable_frames[last_idx]
+
     def set_cache_value(self, value):
         if self.cache_key is not None:
             cache.set(self.cache_key, value, 3600)
@@ -282,7 +293,9 @@ def get_processable_frames(stacktrace_info, processors):
     for idx, frame in enumerate(stacktrace_info.stacktrace['frames']):
         processor = next((p for p in processors if p.handles_frame(frame, stacktrace_info)), None)
         if processor is not None:
-            rv.append(ProcessableFrame(frame, frame_count - idx - 1, processor, stacktrace_info))
+            rv.append(
+                ProcessableFrame(frame, frame_count - idx - 1, processor, stacktrace_info, rv)
+            )
     return rv
 
 

tests/sentry/lang/javascript/example-project/.gitignore

@@ -0,0 +1 @@
+node_modules

tests/sentry/lang/javascript/example-project/Makefile

@@ -0,0 +1,12 @@
+all: update
+
+install:
+	yarn install
+
+minify: install
+	./node_modules/.bin/uglifyjs --mangle --source-map test.map --source-map-include-sources --prefix relative --output test.min.js test.js
+
+update: minify
+	node launch.js
+
+.PHONY: all install minify update

tests/sentry/lang/javascript/example-project/index.html

@@ -0,0 +1,11 @@
+<!doctype html>
+<script type=text/javascript src=test.min.js></script>
+<script>
+  function produceStack() {
+    try {
+      makeAFailure();
+    } catch (e) {
+      return e.stack;
+    }
+  }
+</script>

tests/sentry/lang/javascript/example-project/launch.js

@@ -0,0 +1,36 @@
+const fs = require('fs');
+const puppeteer = require('puppeteer');
+const stacktraceParser = require('stacktrace-parser');
+
+function convertStack(frames) {
+  return frames.map((item) => {
+    var match = item.file.match(/^.*\/(.*?)$/);
+    var fileName, absPath;
+    if (match && match[1]) {
+      fileName = match[1];
+      absPath = 'http://example.com/' + fileName;
+    } else {
+      fileName = absPath = item.file;
+    }
+    return {
+      abs_path: absPath,
+      filename: fileName,
+      lineno: item.lineNumber,
+      colno: item.column,
+      'function': item.methodName,
+    };
+  });
+}
+
+(async() => {
+  const browser = await puppeteer.launch();
+  const page = await browser.newPage();
+  await page.goto('file://' + __dirname + '/index.html');
+  const stack = await page.evaluate('produceStack()');
+  const frames = convertStack(stacktraceParser.parse(stack));
+  frames.pop();
+  const stackDump = JSON.stringify(frames, null, 2) + '\n';
+  fs.writeFile('minifiedError.json', stackDump, function() {
+    browser.close();
+  });
+})();

tests/sentry/lang/javascript/example-project/minifiedError.json

@@ -0,0 +1,30 @@
+[
+  {
+    "abs_path": "http://example.com/test.min.js",
+    "filename": "test.min.js",
+    "lineno": 1,
+    "colno": 64,
+    "function": "e"
+  },
+  {
+    "abs_path": "http://example.com/test.min.js",
+    "filename": "test.min.js",
+    "lineno": 1,
+    "colno": 136,
+    "function": "r"
+  },
+  {
+    "abs_path": "http://example.com/test.min.js",
+    "filename": "test.min.js",
+    "lineno": 1,
+    "colno": 183,
+    "function": "i"
+  },
+  {
+    "abs_path": "http://example.com/index.html",
+    "filename": "index.html",
+    "lineno": 6,
+    "colno": 7,
+    "function": "produceStack"
+  }
+]

tests/sentry/lang/javascript/example-project/package.json

@@ -0,0 +1,8 @@
+{
+  "dependencies": {
+    "chrome-launcher": "^0.6.0",
+    "puppeteer": "^0.9.0",
+    "stacktrace-parser": "^0.1.4",
+    "uglify": "^0.1.5"
+  }
+}

tests/sentry/lang/javascript/example-project/test.js

@@ -0,0 +1,24 @@
+var makeAFailure = (function() {
+  function onSuccess(data) {}
+
+  function onFailure(data) {
+    throw new Error('failed!');
+  }
+
+  function invoke(data) {
+    var cb = null;
+    if (data.failed) {
+      cb = onFailure;
+    } else {
+      cb = onSuccess;
+    }
+    cb(data);
+  }
+
+  function test() {
+    var data = {failed: true, value: 42};
+    invoke(data);
+  }
+
+  return test;
+})();