1315242 - Web Console doesn't show line/column for error thrown from console input.

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Tooru Fujisawa [:arai]]

Steps to reproduce:
  1. open web console
  2. evaluate the following code
       foo()

Actual result:
  get the following error message
    ReferenceError: foo is not defined [Learn More]

Expected result:
  get error message with location information like following
    ReferenceError: foo is not defined [Learn More]          typein:1:1

filename part isn't necessary, but line/column should be there.

Comment 1

[Tooru Fujisawa [:arai]]

sometimes column information is important for JavaScript error, like bug 1315242's case.
will work on this,  this will help bug 1283712 too, as location information is important for note too.
let me know if there's already a bug for this.

Comment 2

[Brian Grinstead [:bgrins]]

The location would be coming from the evalled debugger code in this case.  We wouldn't have anywhere to link to with the source, but the location info might be handy.  I'm not sure if the debugger API exposes this currently though.

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> I'm not sure if the debugger API exposes this currently though.

there wasn't, so I've added :)
will attach patch after I added some testcases.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add lineNumber and columnNumber property to debugger object to get line/column from JSErrorReport.

To get line/column number from devtools, added lineNumber/columnNumber getters, just like errorMessageName getter that extracts JSErrorReport information.

now we have 3 accessors for JSErrorReport, so separated the part that extracts JSErrorReport from `referent` object, to GetErrorReport function.
and used it from DebuggerObject::getErrorMessageName, DebuggerObject::getLineNumber, and DebuggerObject::getColumnNumber.

lineNumber and columnNumber getters return number value if there's JSErrorReport, otherwise returns undefined.
so that we can check if it has lineNumber/columnNumber by checking type.

will post other parts after this part, since they depends on interface.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 8807787 [details] [diff] [review]
Part 1: Add lineNumber and columnNumber property to debugger object to get line/column from JSErrorReport.

Review of attachment 8807787 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much for doing this!

Thank you for cleaning up DebuggerObject::getErrorMessageName. The new code better fits the SpiderMonkey coding style.

Changes to Debugger.cpp require corresponding changes to the documentation in js/src/doc/Debugger, and they require full unit tests in js/src/jit-test/tests/debug.

Please flag for review again when these changes have been made; I will try to respond as quickly as possible.

::: js/src/vm/Debugger.cpp
@@ +8683,5 @@
> +/* static */ bool
> +DebuggerObject::lineNumberGetter(JSContext *cx, unsigned argc, Value* vp)
> +{
> +    THIS_DEBUGOBJECT(cx, argc, vp, "get lineNumber", args, object)
> +fprintf(stderr, "lineNumberGetter\n");

These debugging printfs need to come out.

@@ +9300,5 @@
>      JS_PSG("isProxy", DebuggerObject::isProxyGetter, 0),
>      JS_PSG("proxyTarget", DebuggerObject::proxyTargetGetter, 0),
>      JS_PSG("proxyHandler", DebuggerObject::proxyHandlerGetter, 0),
> +    JS_PSG("lineNumber", DebuggerObject::lineNumberGetter, 0),
> +    JS_PSG("columnNumber", DebuggerObject::columnNumberGetter, 0),

I would rather see these called `errorLineNumber` and `errorColumnNumber`. (Debugger.Object needs to carry accessors for many kinds of referents.)

@@ +9618,5 @@
>      return true;
>  }
>  
> +static bool
> +GetErrorReport(JSContext* cx, HandleObject maybeError, JSErrorReport** report)

This should be a private static method of DebuggerObject, like `checkThis`.

It's SpiderMonkey style to use `&` for pointers that can't be null, even when they are non-const. The pointer to the JSErrorReport may be null, but the pointer to that pointer must not be. So `report` should be `JSObject*&`.

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> sometimes column information is important for JavaScript error, like bug
> 1315242's case.

bug 1310045's case, I meant :P

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add errorLineNumber and errorColumnNumber property to debugger object to get line/column from JSErrorReport.

Thank you for reviewing :)
renamed to errorLineNumber and errorColumnNumber, and added documents and testcases.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8808412 [details] [diff] [review]
Part 1: Add errorLineNumber and errorColumnNumber property to debugger object to get line/column from JSErrorReport.

Review of attachment 8808412 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the revisions! The tests look really good.

::: js/src/doc/Debugger/Debugger.Object.md
@@ +157,5 @@
>  :  If the referent is an error created with an engine internal message template
>     this is a string which is the name of the template; `undefined` otherwise.
>  
> +`errorLineNumber`
> +:  If the referent is an error this is a line number of the error;

I'd prefer:

"If the referent is an Error object, this is the line number at which the referent was created; ..."

and similarly for column number.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Display line number and column number in error message for console input.

Added code to get error's line number and column number from evaluation result,
and when they're available, set frame to "typein:{line}:{column}"

then when formatting it, if frame's url is "typein", it shows location even if it's not linkable.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add a test input for syntax error in Evaluation Result.

Added testcase for syntax error that will set column number to specific value. (column 4)

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Update stub.

Updated stub with
./mach test devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_evaluation_result.js

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Add a testcase for location in Evaluation Result.

and added a testcase for location

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: input.png

Hi, I have this series applied but am not seeing the typein location.  Should I be seeing the frame here?

Comment 14

[Tooru Fujisawa [:arai]]

yeah, the location should be there.
let me check again.
maybe I broke the patch just before attaching

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8808691 [details] [diff] [review]
Part 2: Display line number and column number in error message for console input.

Review of attachment 8808691 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still having issues applying locally, waiting for a clobber but here's initial feedback.  This looks good though

::: devtools/client/shared/components/frame.js
@@ +37,5 @@
>      showFullSourceUrl: PropTypes.bool,
>      // Service to enable the source map feature for console.
>      sourceMapService: PropTypes.object,
> +    // Frame is typein
> +    isTypein: PropTypes.bool,

Nit: I prefer a name like isConsoleInput throughout this patch

::: devtools/server/actors/webconsole.js
@@ +933,5 @@
> +        try {
> +          let line = error.errorLineNumber;
> +          let column = error.errorColumnNumber;
> +
> +          if (typeof line === "number" &&typeof column === "number") {

Nit: space after &&

@@ +936,5 @@
> +
> +          if (typeof line === "number" &&typeof column === "number") {
> +            // Set frame only if we have line/column numbers.
> +            frame = {
> +              source: "typein",

Instead of "typein" here, can we try "console-input"?

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Created attachment 8808705 [details]
> input.png
> 
> Hi, I have this series applied but am not seeing the typein location. 
> Should I be seeing the frame here?

I think this was an issue in my build, clearing needinfo

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Display line number and column number in error message for console input.

reused "debugger eval code" as frame.source.
and changed "typein" to "console-input" when displaying the frame.

also, fixed the comment for evalWithDebugger, that "url" is the property of |aOptions| parameter, not return object.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8808695 [details] [diff] [review]
Part 4: Update stub.

Review of attachment 8808695 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, now that Bug 1307905 has landed can you re-run the stub generation for this patch?  It should ignore properties like 'timestamp'

Comment 19

[Tooru Fujisawa [:arai]]

to make things easier, I'll land Part 1 first.
so that we can use artifact build.

Comment 20

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f28db33d51f15a0f1ee7b9916b1c3ae59641cd6e
Bug 1315242 - Part 1: Add errorLineNumber and errorColumnNumber property to debugger object to get line/column from JSErrorReport. r=jimb

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Update stub.

regenerated

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Add a testcase for location in Evaluation Result.

and updated to follow part 2

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: part2-review-changes.patch

Started doing a review on part 2 which led me to making local changes, which I'll just upload instead commenting on splinter.  This is meant to be squashed in with your patch but basically here's what it does:

* Set don't set "console-input" as the visible source but just allow "debugger eval input" to go through.  This has a benefit of showing the same source in both the stack trace and the console.trace() header when running this in console: `setTimeout(() => { console.trace() })`.  This will require another change on Part 5 to match (sorry about that)
* Remove isConsoleInput special case in frame component and instead display source information and line number even when it's not linkable.  AFAIK there aren't other cases of this - if it ends up being a problem we can reintroduce an option to force the location info to be shown, but let's do this for now since it's simpler

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8808754 [details] [diff] [review]
Part 2: Display line number and column number in error message for console input.

See comment 23 - can you please fold the review comments into this one and confirm they look right to you?  r=me on that patch if you are happy with the changes as well

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8809083 [details] [diff] [review]
Part 5: Add a testcase for location in Evaluation Result.

Review of attachment 8809083 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/webconsole/new-console-output/test/components/evaluation-result.test.js
@@ +78,5 @@
> +    const wrapper = render(EvaluationResult({ message }));
> +
> +    const locationLink = wrapper.find(`.message-location`);
> +    expect(locationLink.length).toBe(1);
> +    expect(locationLink.text()).toBe("console-input:1:4");

Will need to switch this back to "debugger eval code" after Comment 23

Comment 26

[Tooru Fujisawa [:arai]]

Thanks :)

We could use "console-input" as filename in debugger, so that trace shows that filename.
I think "console-input" is more intuitive than "debugger eval code", since it's a code entered in console, not debugger.
Will try changing that part.

Comment 27

[Tooru Fujisawa [:arai]]

looks like we should change many more code in devtools to change "debugger eval code" to "console-input", for console input filename.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=047ff6417fca977c25e66cba73a64c8ccf9bea78&selectedJob=30795366
it should be left to another dedicated bug if we go that way.

will use "debugger eval code" for now.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Add a testcase for location in Evaluation Result.

Updated the test to use "debugger eval code".

will file a bug to change internal and exposed filename to more intuitive one.

Comment 29

[Tooru Fujisawa [:arai]]

filed bug 1316517 for filename.

Comment 30

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1505a03d78c38a05fac455c28539ef05686bfe
Bug 1315242 - Part 2: Display line number and column number in error message for console input. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/07585bbe0c8fb528e6103ca46b0b243019bf61c0
Bug 1315242 - Part 3: Add a test input for syntax error in Evaluation Result. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/5778f05e78db4dfaaaba4a4c50c61e2e8969b83f
Bug 1315242 - Part 4: Update stub. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce34d3f8a3b728d60a7ecd35b70e096bf87b85f9
Bug 1315242 - Part 5: Add a testcase for location in Evaluation Result. r=bgrins

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f28db33d51f1

Comment 32

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/debae3394553dde5b5edcdf1e2afbed5d282db1d
Backed out changeset ce34d3f8a3b7 (bug 1315242)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f35567f72f1127cee6e6dda78b1ce38f74186a4c
Backed out changeset 5778f05e78db (bug 1315242)

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b1fd4bf3ac16abd6e718260050046f4dbe1db11
Backed out changeset 07585bbe0c8f (bug 1315242)

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f3b378f627f6fe3288bd0c4b193bec23badba2b
Backed out changeset fd1505a03d78 for mochitest chrome bustage (bug 1315242)

Comment 33

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/logviewer.html#?job_id=38970877&repo=mozilla-inbound#L3819

looks like this test needs fix:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/test/mochitest/test_frame_01.html

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Update test_frame_01.html to follow new behavior of location.

As we changed line/column to be shown for not-linkable location, the test for it should be updated.
Changed the expected value for not-linkable test to expect line/column, also added the case that it has column.

Comment 35

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66f8ba508c9851a48944894a3040738a8cbcd4b0
Bug 1315242 - Part 2: Display line number and column number in error message for console input. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/48214b1297023ed37de76d1421426c58af935532
Bug 1315242 - Part 3: Add a test input for syntax error in Evaluation Result. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6fa81a22987ecd04e0652c2241ed0aa3c5e1b0
Bug 1315242 - Part 4: Update stub. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa67f98c5ffd878b667226c589e819d3c1c91fea
Bug 1315242 - Part 5: Add a testcase for location in Evaluation Result. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1adcac2cfd09116a522c79a4baafc02d26fee3
Bug 1315242 - Part 6: Update test_frame_01.html to follow new behavior of location. r=bgrins

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/66f8ba508c98
https://hg.mozilla.org/mozilla-central/rev/48214b129702
https://hg.mozilla.org/mozilla-central/rev/ad6fa81a2298
https://hg.mozilla.org/mozilla-central/rev/fa67f98c5ffd
https://hg.mozilla.org/mozilla-central/rev/4e1adcac2cfd

Comment 37

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox nightly 52.0a1(build id:20161104030212)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox beta 52.0b5(build id:20170209120619)
User agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20170208]

Comment 38

[Jim Michaels]

problem is back in 55.0.1. I am doing a function call I made which has a bug in it and it's a large function.
TypeError: undefined has no properties [Learn More]
is the error message, without line:col on right hand side, in pink.
what this error is, is the index of a string accessed with [] is out of bounds. message should say that (that's a different bug), but mainly the biggest problem I have is that when running from an HTML file it shows line:col, but from console prompt no.

Comment 39

[Jim Michaels]

same with 55.