Follow up to bug 984696 to continue to improve the source map + debugging experience.

I expect there are random tests that check column numbers that will fail due to this change so the try push is mostly about finding them, and I don't actually expect it to come back super green.

Comment on attachment 8441062 [details] [diff] [review] source-notes-for-calls.patch Review of attachment 8441062 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +208,5 @@ > + bce->current->lastColumn = columnIndex; > + } > + return true; > +} > + This code looks like it didn't change. So why did you move it? ::: js/src/jit-test/tests/debug/Script-getAllColumnOffsets-06.js @@ +16,5 @@ > +global.log = ""; > +global.eval("function f(){print(print());new Error();} debugger;"); > +global.f(); > + > +assertEq(global.log, "13 19 13 28 40 "); I'm a bit confused about these column offsets. If I understand the intent of your patch correctly, it should give us proper column offsets for the following: - The outer call to print - The inner call to print - The new expression with Error Now, I assume the offset 19 refers to the outer call to print. Since it points right before the opening ( of the call, I would expect a similar offset 25 for the inner call to print, and ditto for the new expression with Error. This probably reflects my lack of understanding of what we expect the column offsets to be, so can you please explain the expected behavior here?

By the way, the rest of the patch looks good to me, so I can r+ it as soon as you explain that test to me.

(In reply to Eddy Bruel [:ejpbruel] from comment #3) > Comment on attachment 8441062 [details] [diff] [review] > source-notes-for-calls.patch > > Review of attachment 8441062 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +208,5 @@ > > + bce->current->lastColumn = columnIndex; > > + } > > + return true; > > +} > > + > > This code looks like it didn't change. So why did you move it? I needed to call UpdateSourceCoordNotes in a function before it was defined and it seemed easier just to move it and everything it needed above. I can add forward declarations if you'd prefer that. > ::: js/src/jit-test/tests/debug/Script-getAllColumnOffsets-06.js > @@ +16,5 @@ > > +global.log = ""; > > +global.eval("function f(){print(print());new Error();} debugger;"); > > +global.f(); > > + > > +assertEq(global.log, "13 19 13 28 40 "); > > I'm a bit confused about these column offsets. If I understand the intent of > your patch correctly, it should give us proper column offsets for the > following: > - The outer call to print > - The inner call to print > - The new expression with Error > > Now, I assume the offset 19 refers to the outer call to print. Since it > points right before the opening ( of the call, I would expect a similar > offset 25 for the inner call to print, and ditto for the new expression with > Error. > > This probably reflects my lack of understanding of what we expect the column > offsets to be, so can you please explain the expected behavior here? Yeah it's pretty funky. Without this patch we get "13 28 40 ". My understanding is that they correspond to entering the function body, the second statement, and returning from the function body respectively. With the patch we get "13 19 13 28 40 ". This breaks down to: 13 - entering the function body 19 - calling the inner print 13 - calling the outer print 28 - second statement / new Error ; I think I should remove the ";" so we know for sure it comes from the new Error 40 - returning from the function body Note that the source notes point to the expression that is being called, not the open parenthesis. It just seemed a lot easier this way and which is more correct feels subjective to me. Thoughts?

Fixed tests in the browser that depend on column numbers. Forward declare UpdateSourceCoordNotes instead of moving it. Fix the new test to be much more clear (I hope!) This should be a much greener try push: https://tbpl.mozilla.org/?tree=Try&rev=ca7a1c557273

Ok, this should fix those last failures: https://tbpl.mozilla.org/?tree=Try&rev=86e229ce785d Sorry for bugmail spam. In the process of fixing the black boxing test, I ended up re-writing it to use Task.jsm because I couldn't understand it anymore (and I even originally wrote that test!). Should be a lot better now.

Comment on attachment 8443046 [details] [diff] [review] source-notes-for-calls.patch Review of attachment 8443046 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this is a great improvement of the previous version. Thanks for clearing stuff up! ::: dom/events/test/test_error_events.html @@ +36,5 @@ > [ "Event message", errorEvent.message, "Error: hello" ], > [ "Callback message", msg, "Error: hello" ], > [ "Event error-object", errorEvent.error, thrown], > [ "Callback error-object", error, thrown ], > + [ "Event column", errorEvent.colno, 15 ], Correct now! \O/ ::: js/src/jit-test/tests/debug/Script-getAllColumnOffsets-06.js @@ +12,5 @@ > + }); > + }); > +}; > + > +global.log = ""; This is much, much better. Thanks for clearing this up! ::: js/src/tests/ecma/extensions/errorcolumnblame.js @@ +39,5 @@ > test(function() { var tmp = null; tmp(); }, 34) > test(function() { var tmp = null; tmp.foo; }, 35) > /* Just a generic 'throw'. */ > test(function() { > +//234567890123456789 Perhaps put a comment at the end that says these are column numbers, not line noise? ;-) ::: toolkit/devtools/server/tests/unit/test_blackboxing-01.js @@ +24,5 @@ > } > const BLACK_BOXED_URL = "http://example.com/blackboxme.js"; > const SOURCE_URL = "http://example.com/source.js"; > +const testBlackBox = Task.async(function* () { > + yield executeOnNextTickAndWaitForPause(evalCode, gClient); This doesn't do exactly the same thing as the old version. Why do we need the executeOnNextTick part? @@ +114,5 @@ > ); > } > +const runTest = Task.async(function* (onSteppedLocation, onDebuggerStatementFrames) { > + let packet = yield executeOnNextTickAndWaitForPause(gDebuggee.runTest, > + gClient); Same question here as above. ::: toolkit/modules/tests/xpcshell/test_AsyncShutdown.js @@ -288,5 @@ > // Now that we have called `wait()`, the state contains interesting things > let state = barrier.state[0]; > do_print("State: " + JSON.stringify(barrier.state, null, "\t")); > Assert.equal(state.filename, filename); > - Assert.equal(state.lineNumber, lineNumber + 2); I don't understand this test well enough to see why your patch would cause lineNumber to change here, but I'll just roll with it.

(In reply to Eddy Bruel [:ejpbruel] from comment #8) > Comment on attachment 8443046 [details] [diff] [review] > source-notes-for-calls.patch > > Review of attachment 8443046 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall, this is a great improvement of the previous version. Thanks for > clearing stuff up! > > ::: dom/events/test/test_error_events.html > @@ +36,5 @@ > > [ "Event message", errorEvent.message, "Error: hello" ], > > [ "Callback message", msg, "Error: hello" ], > > [ "Event error-object", errorEvent.error, thrown], > > [ "Callback error-object", error, thrown ], > > + [ "Event column", errorEvent.colno, 15 ], > > Correct now! \O/ :D > ::: js/src/tests/ecma/extensions/errorcolumnblame.js > @@ +39,5 @@ > > test(function() { var tmp = null; tmp(); }, 34) > > test(function() { var tmp = null; tmp.foo; }, 35) > > /* Just a generic 'throw'. */ > > test(function() { > > +//234567890123456789 > > Perhaps put a comment at the end that says these are column numbers, not > line noise? ;-) Ok > ::: toolkit/devtools/server/tests/unit/test_blackboxing-01.js > @@ +24,5 @@ > > } > > const BLACK_BOXED_URL = "http://example.com/blackboxme.js"; > > const SOURCE_URL = "http://example.com/source.js"; > > +const testBlackBox = Task.async(function* () { > > + yield executeOnNextTickAndWaitForPause(evalCode, gClient); > > This doesn't do exactly the same thing as the old version. Why do we need > the executeOnNextTick part? Because otherwise the JS will pause before we can yield the wait-for-pause promise. It's an artifact of switching to Task.jsm and promises; we do it in other places as well. I'll add a comment above executeOnNextTickAndWaitForPause, since this is its only purpose. > ::: toolkit/modules/tests/xpcshell/test_AsyncShutdown.js > @@ -288,5 @@ > > // Now that we have called `wait()`, the state contains interesting things > > let state = barrier.state[0]; > > do_print("State: " + JSON.stringify(barrier.state, null, "\t")); > > Assert.equal(state.filename, filename); > > - Assert.equal(state.lineNumber, lineNumber + 2); > > I don't understand this test well enough to see why your patch would cause > lineNumber to change here, but I'll just roll with it. Before the nearest src note was the function on the next line, now there is a src note for the call on the previous line. Took me a while of staring at this test to figure it out, too.

Added comments, as discussed.