Closed Bug 1324929 Opened 5 years ago Closed 4 years ago

Server side logs don't parse backtrace properly

Categories

(DevTools :: Console, defect)

defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: florian, Assigned: florian)

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161213211510

Steps to reproduce:

I followed the https://craig.is/writing/chrome-logger/techspecs spec and sent a backtrace like "somefile.py : 25"


Actual results:

The devtools only showed "somefile.py :" instead of properly displaying line & number. This at least seems to be due to a bug in http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/server-logger.js#325-339 an easier implementation could look like this: https://gist.github.com/apollo13/159529a5366ed2c0d8b66223a0541695


Expected results:

devtools should have shown "somefile.py : 25"
Assignee: nobody → florian
Attached patch properly parse backtrace (obsolete) — Splinter Review
Attachment #8820684 - Flags: feedback?
OS: Unspecified → All
Hardware: Unspecified → All
Version: 50 Branch → Trunk
(In reply to Florian Apolloner from comment #1)
> Created attachment 8820684 [details] [diff] [review]
> properly parse backtrace

Attached is a simple first patch which fixes the issue. I am wondering if we want to be lenient with the data we accept or if we want to force whitespace between the filename and the double colon. If we want to force a single whitespace we can drop the non-greedy flag on the regex and prevent backtracking.
As far as I know, we're following specs from https://craig.is/writing/chrome-logger/techspecs , in which it appears that we expect something like '/path/to/file.py : 25', i.e. with single whitespace before and after the double colon.
Component: Untriaged → Developer Tools
Attached patch bug1324929_2.patch (obsolete) — Splinter Review
Attachment #8820708 - Flags: review?(poirot.alex)
Attachment #8820684 - Attachment is obsolete: true
Attachment #8820684 - Flags: feedback?
Comment on attachment 8820708 [details] [diff] [review]
bug1324929_2.patch

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

Thanks Florian, I think we can better test that and ensure there is not some other edge cases.

::: devtools/shared/webconsole/test/unit/test_server_logger.js
@@ +9,5 @@
> +// bug1324929
> +function run_test() {
> +  deepEqual(parseBacktrace('somefile.py : 25'), {url: 'somefile.py', line: 25});
> +  equal(parseBacktrace('somefile.py'), 'somefile.py');
> +}

It would be better to test from end to end by contributing to this test instead:
http://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/browser_console_server_logging.js

waitForMessages also expects a "stacktrace" attributes that you can use to verify that the stack being displayed is correct:
http://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/browser_console.js#90

You may want to also tweak devtools/client/webconsole/test/test-console-server-logging.sjs
in order to test various scenarios where the regexp matches or not and assert the finale behavior in the UI.
Attachment #8820708 - Flags: review?(poirot.alex)
Attached patch bug1324929_3.patch (obsolete) — Splinter Review
Attachment #8820708 - Attachment is obsolete: true
Attachment #8820761 - Flags: review?(poirot.alex)
@Alexandre: I couldn't test for the stackTrace since it was always empty, I think source is the correct thing I should test against.
Comment on attachment 8820761 [details] [diff] [review]
bug1324929_3.patch

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

Thanks Florian, I'll submit your patch to try to run tests once you addressed the last comments.
Then, if green, we will ship your tweaks!

::: devtools/shared/webconsole/server-logger.js
@@ +491,5 @@
> +  if (!backtrace) {
> +    return null;
> +  }
> +
> +  let result = backtrace.match(/^(.+)\s:\s(\d+)$/);

It is not very explicit that spaces around ':' are mandatory,
so please use \s*:\s* and tweak tests accordingly.
(I'm afraid honza already provided libraries depending on this behavior)

@@ +493,5 @@
> +  }
> +
> +  let result = backtrace.match(/^(.+)\s:\s(\d+)$/);
> +  if (!result || result.length != 3) {
> +    return backtrace;

It looks like even if we return the string as-is, the UI ends up displaying "(unknown)". So what about returning { url: backtrace, line: -1 } or null when the regexp fails?

::: devtools/shared/webconsole/test/unit/test_server_logger.js
@@ +9,5 @@
> +// bug1324929
> +function run_test() {
> +  deepEqual(parseBacktrace('somefile.py : 25'), {url: 'somefile.py', line: 25});
> +  equal(parseBacktrace('somefile.py'), 'somefile.py');
> +}

I don't see much value in this test, so please rip it off in favor of your tweaks made to browser_console_server_logging.js.
Attachment #8820761 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> It is not very explicit that spaces around ':' are mandatory,
> so please use \s*:\s* and tweak tests accordingly.

Ok, this is what I had initially, will revert to that.

> It looks like even if we return the string as-is, the UI ends up displaying
> "(unknown)". So what about returning { url: backtrace, line: -1 } or null
> when the regexp fails?

Will test and then see what I like more (probably the null variant), I just assumed that you added that with a reason initially.

> I don't see much value in this test, so please rip it off in favor of your
> tweaks made to browser_console_server_logging.js.

Ok.
Attached patch bug1324929_4.patch (obsolete) — Splinter Review
Attachment #8820761 - Attachment is obsolete: true
Attachment #8821158 - Flags: review?(poirot.alex)
Component: Developer Tools → Developer Tools: Console
Comment on attachment 8821158 [details] [diff] [review]
bug1324929_4.patch

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

Sorry for the delay, I was off during the end of year.
Looks good now, just miss a single space.
I just launched the tests against your patch.

::: devtools/shared/webconsole/server-logger.js
@@ +493,5 @@
> +  }
> +
> +  let result = backtrace.match(/^(.+?)\s*:\s*(\d+)$/);
> +  if (!result || result.length != 3) {
> +    return { url: backtrace};

You miss a space before the closing brace: `backtrace };`
Attachment #8821158 - Flags: review?(poirot.alex) → review+
Ok, uploaded a new patch which also fixes the eslint warning -- tests/build seems to fail, I cannot figure out what though.
Attachment #8821158 - Attachment is obsolete: true
Attachment #8823248 - Flags: review?(poirot.alex)
I forgot added the test file while pushing to tests. Here is a valid one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=092ebceaae7aaccdf2034cf6d9d78256ac8193fe

It is still running, but so far, it only reports the parseInt() usage you already fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3535ad342933572ba2c93f056a9cdb44a085a6cf
Bug 1324929 - Accept ':' between url and line number in server side logs backtraces. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/3535ad342933
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Attachment #8823248 - Flags: review?(poirot.alex) → review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.