Bug 1038238 - Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers,
authorNick Fitzgerald <fitzgen@gmail.com>
Fri, 27 Mar 2015 13:08:46 -0700
changeset 236192 fadf551562b882a3265ea0e10eee74bb028bfcd9
parent 236191 d56c2eb468f65272e20b3e9562ef05c47be75e57
child 236193 550a5c9e8800868198536792e43b872bde3576fe
push id57618
push usernfitzgerald@mozilla.com
push dateFri, 27 Mar 2015 20:09:43 +0000
treeherdermozilla-inbound@b336dc0af92c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1038238
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1038238 - Part 0: Make js/src/vm/SavedStacks.h use 1-based column numbers, like js::ComputeStackString and other browsers do; r=jorendorff https://bugzilla.mozilla.org/show_bug.cgi?id=1038238
browser/devtools/scratchpad/test/browser_scratchpad_wrong_window_focus.js
browser/devtools/webconsole/test/browser_webconsole_bug_585956_console_trace.js
browser/devtools/webconsole/test/browser_webconsole_column_numbers.js
dom/bindings/test/test_exception_options_from_jsimplemented.html
dom/bindings/test/test_promise_rejections_from_jsimplemented.html
dom/tests/browser/browser_ConsoleAPITests.js
js/src/jit-test/tests/parser/columnNumber.js
js/src/jit-test/tests/saved-stacks/asm-frames.js
js/src/vm/SavedStacks.cpp
--- a/browser/devtools/scratchpad/test/browser_scratchpad_wrong_window_focus.js
+++ b/browser/devtools/scratchpad/test/browser_scratchpad_wrong_window_focus.js
@@ -46,17 +46,17 @@ function testFocus(sw, hud) {
   let sp = sw.Scratchpad;
 
   function onMessage(event, messages) {
     let msg = [...messages][0];
     let node = msg.node;
 
     var loc = node.querySelector(".message-location");
     ok(loc, "location element exists");
-    is(loc.textContent.trim(), sw.Scratchpad.uniqueName + ":1:0",
+    is(loc.textContent.trim(), sw.Scratchpad.uniqueName + ":1:1",
         "location value is correct");
 
     sw.addEventListener("focus", function onFocus() {
       sw.removeEventListener("focus", onFocus, true);
 
       let win = Services.wm.getMostRecentWindow("devtools:scratchpad");
 
       ok(win, "there are active Scratchpad windows");
--- a/browser/devtools/webconsole/test/browser_webconsole_bug_585956_console_trace.js
+++ b/browser/devtools/webconsole/test/browser_webconsole_bug_585956_console_trace.js
@@ -30,19 +30,19 @@ function test() {
     let node = [...result.matched][0];
     ok(node, "found trace log node");
 
     let obj = node._messageObject;
     ok(obj, "console.trace message object");
 
     // The expected stack trace object.
     let stacktrace = [
-      { columnNumber: 2, filename: TEST_URI, functionName: "window.foobar585956c", language: 2, lineNumber: 9 },
-      { columnNumber: 9, filename: TEST_URI, functionName: "foobar585956b", language: 2, lineNumber: 14 },
-      { columnNumber: 9, filename: TEST_URI, functionName: "foobar585956a", language: 2, lineNumber: 18 },
-      { columnNumber: 0, filename: TEST_URI, functionName: "", language: 2, lineNumber: 21 }
+      { columnNumber: 3, filename: TEST_URI, functionName: "window.foobar585956c", language: 2, lineNumber: 9 },
+      { columnNumber: 10, filename: TEST_URI, functionName: "foobar585956b", language: 2, lineNumber: 14 },
+      { columnNumber: 10, filename: TEST_URI, functionName: "foobar585956a", language: 2, lineNumber: 18 },
+      { columnNumber: 1, filename: TEST_URI, functionName: "", language: 2, lineNumber: 21 }
     ];
 
     ok(obj._stacktrace, "found stacktrace object");
     is(obj._stacktrace.toSource(), stacktrace.toSource(), "stacktrace is correct");
     isnot(node.textContent.indexOf("bug-585956"), -1, "found file name");
   }
 }
--- a/browser/devtools/webconsole/test/browser_webconsole_column_numbers.js
+++ b/browser/devtools/webconsole/test/browser_webconsole_column_numbers.js
@@ -25,17 +25,17 @@ function consoleOpened(aHud) {
       category: CATEGORY_WEBDEV,
       severity: SEVERITY_ERROR
     }]
   }).then(testLocationColumn);
 }
 
 function testLocationColumn() {
   let messages = hud.outputNode.children;
-  let expected = ['10:6', '10:38', '11:8', '12:10', '13:8', '14:6'];
+  let expected = ['10:7', '10:39', '11:9', '12:11', '13:9', '14:7'];
 
   for(let i = 0, len = messages.length; i < len; i++) {
     let msg = messages[i].textContent;
 
     is(msg.contains(expected[i]), true, 'Found expected line:column of ' + expected[i]);
   }
 
   finishTest();
--- a/dom/bindings/test/test_exception_options_from_jsimplemented.html
+++ b/dom/bindings/test/test_exception_options_from_jsimplemented.html
@@ -40,23 +40,23 @@ https://bugzilla.mozilla.org/show_bug.cg
       ok(e instanceof Error, "Should also have an Error here");
       ok(e instanceof DOMException, "Should have DOMException here");
       ise(e.name, "NotSupportedError", "Should have the right name here");
       ise(e.message, "We are a DOMException",
           "Should also have the right message");
       ise(e.code, DOMException.NOT_SUPPORTED_ERR,
           "Should have the right 'code'");
       ise(e.stack,
-          "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_exception_options_from_jsimplemented.html:38:6\n",
+          "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_exception_options_from_jsimplemented.html:38:7\n",
           "Exception stack should still only show our code");
       ise(e.filename,
           "http://mochi.test:8888/tests/dom/bindings/test/test_exception_options_from_jsimplemented.html",
           "Should still have the right file name");
       ise(e.lineNumber, 38, "Should still have the right line number");
-      todo_is(e.columnNumber, 6,
+      todo_is(e.columnNumber, 7,
               "No column number support for DOMException yet");
     }
     SimpleTest.finish();
   }
 
   SpecialPowers.pushPrefEnv({set: [['dom.expose_test_interfaces', true]]},
                             doTest);
   </script>
--- a/dom/bindings/test/test_promise_rejections_from_jsimplemented.html
+++ b/dom/bindings/test/test_promise_rejections_from_jsimplemented.html
@@ -40,17 +40,17 @@ https://bugzilla.mozilla.org/show_bug.cg
 
     var ourFile = "http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html";
 
     Promise.all([
       t.testPromiseWithThrowingChromePromiseInit().then(
           ensurePromiseFail.bind(null, 1),
           checkExn.bind(null, 44, "NS_ERROR_UNEXPECTED", "", undefined,
                         ourFile, 1,
-                        "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:44:6\n")),
+                        "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:44:7\n")),
       t.testPromiseWithThrowingContentPromiseInit(function() {
           thereIsNoSuchContentFunction1();
         }).then(
           ensurePromiseFail.bind(null, 2),
           checkExn.bind(null, 50, "ReferenceError",
                         "thereIsNoSuchContentFunction1 is not defined",
                         undefined, ourFile, 2,
                         "doTest/<@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:50:11\ndoTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:49:7\n")),
@@ -59,40 +59,40 @@ https://bugzilla.mozilla.org/show_bug.cg
           checkExn.bind(null, 0, "NS_ERROR_UNEXPECTED", "", undefined, "", 3, "")),
       t.testPromiseWithThrowingContentThenFunction(function() {
           thereIsNoSuchContentFunction2();
         }).then(
           ensurePromiseFail.bind(null, 4),
           checkExn.bind(null, 61, "ReferenceError",
                         "thereIsNoSuchContentFunction2 is not defined",
                         undefined, ourFile, 4,
-                        "doTest/<@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:61:11\n")),
+                        "doTest/<@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:61:11\nAsync*doTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:60:7\n")),
       t.testPromiseWithThrowingChromeThenable().then(
           ensurePromiseFail.bind(null, 5),
           checkExn.bind(null, 0, "NS_ERROR_UNEXPECTED", "", undefined, "", 5, "")),
       t.testPromiseWithThrowingContentThenable({
             then: function() { thereIsNoSuchContentFunction3(); }
         }).then(
           ensurePromiseFail.bind(null, 6),
           checkExn.bind(null, 72, "ReferenceError",
                         "thereIsNoSuchContentFunction3 is not defined",
                         undefined, ourFile, 6,
                         "doTest/<.then@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:72:32\n")),
       t.testPromiseWithDOMExceptionThrowingPromiseInit().then(
           ensurePromiseFail.bind(null, 7),
           checkExn.bind(null, 79, "NotFoundError",
                         "We are a second DOMException",
                         DOMException.NOT_FOUND_ERR, ourFile, 7,
-                        "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:79:6\n")),
+                        "doTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:79:7\n")),
       t.testPromiseWithDOMExceptionThrowingThenFunction().then(
           ensurePromiseFail.bind(null, 8),
           checkExn.bind(null, 85, "NetworkError",
                         "We are a third DOMException",
                         DOMException.NETWORK_ERR, ourFile, 8,
-                        "Async*doTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:85:6\n")),
+                        "Async*doTest@http://mochi.test:8888/tests/dom/bindings/test/test_promise_rejections_from_jsimplemented.html:85:7\n")),
       t.testPromiseWithDOMExceptionThrowingThenable().then(
           ensurePromiseFail.bind(null, 9),
           checkExn.bind(null, 0, "TypeMismatchError",
                         "We are a fourth DOMException",
                          DOMException.TYPE_MISMATCH_ERR, "", 9, "")),
     ]).then(SimpleTest.finish,
             function() {
               ok(false, "One of our catch statements totally failed");
--- a/dom/tests/browser/browser_ConsoleAPITests.js
+++ b/dom/tests/browser/browser_ConsoleAPITests.js
@@ -166,20 +166,20 @@ function testConsoleGroup(aMessageObject
   if (aMessageObject.level == "groupEnd") {
     startTimeTest();
   }
 }
 
 function startTraceTest() {
   gLevel = "trace";
   gArgs = [
-    {columnNumber: 8, filename: TEST_URI, functionName: "window.foobar585956c", language: 2, lineNumber: 6},
-    {columnNumber: 15, filename: TEST_URI, functionName: "foobar585956b", language: 2, lineNumber: 11},
-    {columnNumber: 15, filename: TEST_URI, functionName: "foobar585956a", language: 2, lineNumber: 15},
-    {columnNumber: 0, filename: TEST_URI, functionName: "onclick", language: 2, lineNumber: 1}
+    {columnNumber: 9, filename: TEST_URI, functionName: "window.foobar585956c", language: 2, lineNumber: 6},
+    {columnNumber: 16, filename: TEST_URI, functionName: "foobar585956b", language: 2, lineNumber: 11},
+    {columnNumber: 16, filename: TEST_URI, functionName: "foobar585956a", language: 2, lineNumber: 15},
+    {columnNumber: 1, filename: TEST_URI, functionName: "onclick", language: 2, lineNumber: 1}
   ];
 
   let button = gWindow.document.getElementById("test-trace");
   ok(button, "found #test-trace button");
   EventUtils.synthesizeMouseAtCenter(button, {}, gWindow);
 }
 
 function startLocationTest() {
--- a/js/src/jit-test/tests/parser/columnNumber.js
+++ b/js/src/jit-test/tests/parser/columnNumber.js
@@ -1,45 +1,45 @@
 // Simple tests for evaluate's "columnNumber" option.
 
 load(libdir + 'asserts.js');
 
-assertEq(evaluate("saveStack().column"), 0);
-assertEq(evaluate("saveStack().column", { columnNumber: 1729 }), 1729);
-assertEq(evaluate("\nsaveStack().column", { columnNumber: 1729 }), 0);
-assertEq(evaluate("saveStack().column", { columnNumber: "42" }), 42);
+assertEq(evaluate("saveStack().column"), 1);
+assertEq(evaluate("saveStack().column", { columnNumber: 1729 }), 1730);
+assertEq(evaluate("\nsaveStack().column", { columnNumber: 1729 }), 1);
+assertEq(evaluate("saveStack().column", { columnNumber: "42" }), 43);
 assertThrowsInstanceOf(() => evaluate("saveStack().column", { columnNumber: -10 }),
                        RangeError);
 assertThrowsInstanceOf(() => evaluate("saveStack().column", { columnNumber: Math.pow(2,30) }),
                        RangeError);
 
 if (helperThreadCount() > 0) {
   print("offThreadCompileScript 1");
   offThreadCompileScript("saveStack().column", { columnNumber: -10 });
   assertThrowsInstanceOf(runOffThreadScript, RangeError);
 
   print("offThreadCompileScript 2");
   offThreadCompileScript("saveStack().column", { columnNumber: Math.pow(2,30) });
   assertThrowsInstanceOf(runOffThreadScript, RangeError);
 
   print("offThreadCompileScript 3");
   offThreadCompileScript("saveStack().column", { columnNumber: 10000 });
-  assertEq(runOffThreadScript(), 10000);
+  assertEq(runOffThreadScript(), 10001);
 }
 
 // Check handling of columns near the limit of our ability to represent them.
 // (This is hardly thorough, but since web content can't set column numbers,
 // it's probably not worth it to be thorough.)
 const maxColumn = Math.pow(2, 30) - 1;
 assertEq(evaluate("saveStack().column", { columnNumber: maxColumn }),
-         maxColumn);
+         maxColumn + 1);
 
 // Check the 'silently zero' behavior when we reach the limit of the srcnotes
 // column encoding.
 assertEq(evaluate(" saveStack().column", { columnNumber: maxColumn }),
-         0);
+         1);
 
 // Gathering source text for inclusion in error messages should not try to reach
 // outside the buffer to find the start of the source line. The below should
 // report the error without crashing.
 assertThrowsInstanceOf(() => evaluate("function x(y,y) { 'use strict'; } x()",
                                       { columnNumber: 10 }),
                        SyntaxError);
--- a/js/src/jit-test/tests/saved-stacks/asm-frames.js
+++ b/js/src/jit-test/tests/saved-stacks/asm-frames.js
@@ -23,18 +23,18 @@ const buf = new ArrayBuffer(1024*8);
 const module = AsmModule(this, { t: tester }, buf);
 module.test();
 
 print(stack);
 assertEq(stack.functionDisplayName, "tester");
 
 assertEq(stack.parent.functionDisplayName, "doTest");
 assertEq(stack.parent.line, 6);
-assertEq(stack.parent.column, 4);
+assertEq(stack.parent.column, 5);
 
 assertEq(stack.parent.parent.functionDisplayName, "test");
 assertEq(stack.parent.parent.line, 10);
-assertEq(stack.parent.parent.column, 4);
+assertEq(stack.parent.parent.column, 5);
 
 assertEq(stack.parent.parent.parent.line, 24);
-assertEq(stack.parent.parent.parent.column, 0);
+assertEq(stack.parent.parent.parent.column, 1);
 
 assertEq(stack.parent.parent.parent.parent, null);
--- a/js/src/vm/SavedStacks.cpp
+++ b/js/src/vm/SavedStacks.cpp
@@ -1098,16 +1098,20 @@ SavedStacks::getLocation(JSContext *cx, 
         } else {
             const char *filename = iter.scriptFilename() ? iter.scriptFilename() : "";
             locationp->source = Atomize(cx, filename, strlen(filename));
         }
         if (!locationp->source)
             return false;
 
         locationp->line = iter.computeLine(&locationp->column);
+        // XXX: Make the column 1-based as in other browsers, instead of 0-based
+        // which is how SpiderMonkey stores it internally. This will be
+        // unnecessary once bug 1144340 is fixed.
+        locationp->column++;
         return true;
     }
 
     RootedScript script(cx, iter.script());
     jsbytecode *pc = iter.pc();
 
     PCKey key(script, pc);
     PCLocationMap::AddPtr p = pcLocationMap.lookupForAdd(key);
@@ -1121,17 +1125,18 @@ SavedStacks::getLocation(JSContext *cx, 
             source = Atomize(cx, filename, strlen(filename));
         }
         if (!source)
             return false;
 
         uint32_t column;
         uint32_t line = PCToLineNumber(script, pc, &column);
 
-        LocationValue value(source, line, column);
+        // Make the column 1-based. See comment above.
+        LocationValue value(source, line, column + 1);
         if (!pcLocationMap.add(p, key, value))
             return false;
     }
 
     locationp.set(p->value());
     return true;
 }