DevTools: linkify relative links in console.log(new Error().stack)

DevTools: linkify relative links in console.log(new Error().stack)

BUG=424001, 357958
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184915

[sergeyv, 2014-10-22 13:35:53]

sergeyv@chromium.org changed reviewers:
+ vsevik@chromium.org

[vsevik, 2014-10-23 08:40:57]

Please make sure this is well tested and does not regress performance

https://codereview.chromium.org/674513002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/console/ConsoleViewMessage.js (right):

https://codereview.chromium.org/674513002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleViewMessage.js:1283: for (var i = 0; i
< lines.length; ++i) {
var i = 0;
for (; i < lines.length && !lines[i].trim().startsWith("at"); ++i);
var firstAtLineIndex = i;
for (; i < lines.length && lines[i].trim().startsWith("at"); ++i);
if (i !== lines.length)
    return null;

https://codereview.chromium.org/674513002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleViewMessage.js:1297: for (var i = 0; i
< lines.length; ++i) {
for (var i = 0; i < firstAtLineIndex; ++i) {
  ....
}
for (var i = firstAtLineIndex; i < lines.length; ++i) {
  ....
}

https://codereview.chromium.org/674513002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleViewMessage.js:1307: var toAppend =
this._parseLink(lines[i].substring(left, right));
We need to check that this line matches some very basic url filter, this is only
an heuristic after all.

[vsevik, 2014-10-23 10:47:27]

https://codereview.chromium.org/674513002/diff/1/Source/devtools/front_end/co...
File Source/devtools/front_end/console/ConsoleViewMessage.js (right):

https://codereview.chromium.org/674513002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleViewMessage.js:1323: if (!splitResult)
{
extra {

https://codereview.chromium.org/674513002/diff/1/Source/devtools/front_end/co...
Source/devtools/front_end/console/ConsoleViewMessage.js:1331: return
parsed.isValid ? this._linkifier.linkifyScriptLocation(this._target(), null,
splitResult.url, splitResult.lineNumber, splitResult.columnNumber) :
createTextNode(link);
Shouldn't we use parsed here?

[aandrey, 2014-10-23 11:52:03]

aandrey@chromium.org changed reviewers:
+ aandrey@chromium.org

[aandrey, 2014-10-23 11:52:04]

can you add a test?

[sergeyv, 2014-10-23 16:49:16]

Please, take a look.
I completely rewrote code that linkifies a stacktrace, so it checks format of
the stack more aggressively. 

Test was added too.

[aandrey, 2014-10-23 17:51:31]

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/bindings/ResourceUtils.js (right):

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/bindings/ResourceUtils.js:123: } else
{ .. }

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/console/ConsoleViewMessage.js (right):

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1285: if
(!string.startsWith("Error"))
what about ReferenceError? RangeError, SyntaxError, etc.
what about DOMException?

plz add such test cases.

1) return foo.ref.error;

2)
  Error: Failed to execute 'removeChild' on 'Node': The node to be removed is
not a child of this node.
    at Error (native)
    at <anonymous>:4:3
    at Object.InjectedScript._evaluateOn (<anonymous>:732:137)
    at Object.InjectedScript._evaluateAndWrap (<anonymous>:665:34)
    at Object.InjectedScript.evaluate (<anonymous>:579:21)

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1294: var hasAt =
lines[i].trim().startsWith("at ");
var isCallFrameLine = /^\s*at\s/.test(lines[i]);

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1295: if (!hasAt &&
!beforeStack)
beforeStack -> links.length

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1327: links.push({url:
parsed.url, positionLeft: position + left, positionRight: position + right,
lineNumber: splitResult.lineNumber, columnNumber: splitResult.columnNumber});
too complex...

[sergeyv, 2014-10-27 08:31:00]

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/console/ConsoleViewMessage.js (right):

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1285: if
(!string.startsWith("Error"))
On 2014/10/23 17:51:30, aandrey wrote:
> what about ReferenceError? RangeError, SyntaxError, etc.
> what about DOMException?
> 
> plz add such test cases.
> 
> 1) return foo.ref.error;
> 
> 2)
>   Error: Failed to execute 'removeChild' on 'Node': The node to be removed is
> not a child of this node.
>     at Error (native)
>     at <anonymous>:4:3
>     at Object.InjectedScript._evaluateOn (<anonymous>:732:137)
>     at Object.InjectedScript._evaluateAndWrap (<anonymous>:665:34)
>     at Object.InjectedScript.evaluate (<anonymous>:579:21)

Done.

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1294: var hasAt =
lines[i].trim().startsWith("at ");
On 2014/10/23 17:51:30, aandrey wrote:
> var isCallFrameLine = /^\s*at\s/.test(lines[i]);

Done.

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1295: if (!hasAt &&
!beforeStack)
On 2014/10/23 17:51:30, aandrey wrote:
> beforeStack -> links.length

Done.

https://codereview.chromium.org/674513002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1327: links.push({url:
parsed.url, positionLeft: position + left, positionRight: position + right,
lineNumber: splitResult.lineNumber, columnNumber: splitResult.columnNumber});
On 2014/10/23 17:51:30, aandrey wrote:
> too complex...

Acknowledged.

[vsevik, 2014-10-27 09:04:56]

https://codereview.chromium.org/674513002/diff/40001/LayoutTests/inspector/co...
File LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html
(right):

https://codereview.chromium.org/674513002/diff/40001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:38:
function test()
I would expect to see new Error().stack calls from scripts with sourceURL in
this test. 
I think all cases where the error message represents a correct stack trace
should be actually generated by v8.
Also I don't see how these URLs could actually be linkified to scripts if you
don't have this scripts at all.

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/console/ConsoleViewMessage.js (right):

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1291: var hasAt =
/^\s*at\s/.test(lines[i]);
s/hasAt/isCallFrameLine/

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1308: var linkPretender
= lines[i].substring(left, right);
linkCandidate

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1315: var completeURL =
WebInspector.ParsedURL.completeURL(WebInspector.targetManager.inspectedPageURL(),
splitResult.url);
This could be incorrect in case of worker. Is there anything we could do about
that?

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/Script.js (right):

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/Script.js:49: var completeURL =
WebInspector.ParsedURL.completeURL(WebInspector.targetManager.inspectedPageURL(),
sourceURL);
ditto

[sergeyv, 2014-10-30 14:38:39]

https://codereview.chromium.org/674513002/diff/40001/LayoutTests/inspector/co...
File LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html
(right):

https://codereview.chromium.org/674513002/diff/40001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:38:
function test()
On 2014/10/27 09:04:55, vsevik wrote:
> I would expect to see new Error().stack calls from scripts with sourceURL in
> this test. 
> I think all cases where the error message represents a correct stack trace
> should be actually generated by v8.
> Also I don't see how these URLs could actually be linkified to scripts if you
> don't have this scripts at all.

Done.

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/console/ConsoleViewMessage.js (right):

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1291: var hasAt =
/^\s*at\s/.test(lines[i]);
On 2014/10/27 09:04:56, vsevik wrote:
> s/hasAt/isCallFrameLine/

Done.

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1308: var linkPretender
= lines[i].substring(left, right);
On 2014/10/27 09:04:55, vsevik wrote:
> linkCandidate

Done.

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/console/ConsoleViewMessage.js:1315: var completeURL =
WebInspector.ParsedURL.completeURL(WebInspector.targetManager.inspectedPageURL(),
splitResult.url);
On 2014/10/27 09:04:55, vsevik wrote:
> This could be incorrect in case of worker. Is there anything we could do about
> that?

Done.

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/Script.js (right):

https://codereview.chromium.org/674513002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/Script.js:49: var completeURL =
WebInspector.ParsedURL.completeURL(WebInspector.targetManager.inspectedPageURL(),
sourceURL);
On 2014/10/27 09:04:56, vsevik wrote:
> ditto

Done.

[aandrey, 2014-10-30 14:50:43]

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
File LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html
(right):

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:19: }
can you also add a test for DOMException?

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:24:
console.log("Error valid stack #1\n    at function_name(foo.js:30:1)\n    at
boo.js:40:70");
nit: would be easier to read if it was not on one line

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:38: 
extra line br

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:46:
console.log("Error: Failed to execute 'removeChild' on 'Node': The node to be
removed is not a child of this node.\nat Object.InjectedScript._evaluateOn
(<anonymous>:732:137)\nat Object.InjectedScript._evaluateAndWrap
(<anonymous>:665:34)\nat Object.InjectedScript.evaluate (<anonymous>:579:21)");
maybe replace with generating actual DOMException?

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/Script.js (right):

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/sdk/Script.js:45: this.scriptId = scriptId;
duplicated line

[vsevik, 2014-10-31 09:05:34]

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/common/ParsedURL.js (right):

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/common/ParsedURL.js:52: var match =
url.match(/^([A-Za-z][A-Za-z0-9+.-]*):\/\/([^\s\/:]*)(?::([\d]+))?(?:(\/[^\s<>#]*)(?:#(.*))?)?$/i);
Please check whether <> are allowed in query param.

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/Script.js (right):

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/sdk/Script.js:46: if (hasSourceURL) {
Let's try to avoid doing this in this patch since this is unrelated.
A correct implementation of this url completing would depend on the frame url
for which this script was loaded which could be different from the main frame
url in general case. Hence relying on target.url seems not robust.

[sergeyv, 2014-11-05 10:48:33]

Patchset #5 (id:80001) has been deleted

[sergeyv, 2014-11-05 10:50:04]

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
File LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html
(right):

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:19: }
On 2014/10/30 14:50:43, aandrey wrote:
> can you also add a test for DOMException?

Done.

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:24:
console.log("Error valid stack #1\n    at function_name(foo.js:30:1)\n    at
boo.js:40:70");
On 2014/10/30 14:50:43, aandrey wrote:
> nit: would be easier to read if it was not on one line

Acknowledged.

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:38: 
On 2014/10/30 14:50:43, aandrey wrote:
> extra line br

Done.

https://codereview.chromium.org/674513002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/console-log-linkify-stack-in-errors.html:46:
console.log("Error: Failed to execute 'removeChild' on 'Node': The node to be
removed is not a child of this node.\nat Object.InjectedScript._evaluateOn
(<anonymous>:732:137)\nat Object.InjectedScript._evaluateAndWrap
(<anonymous>:665:34)\nat Object.InjectedScript.evaluate (<anonymous>:579:21)");
On 2014/10/30 14:50:43, aandrey wrote:
> maybe replace with generating actual DOMException?

Done.

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/common/ParsedURL.js (right):

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/common/ParsedURL.js:52: var match =
url.match(/^([A-Za-z][A-Za-z0-9+.-]*):\/\/([^\s\/:]*)(?::([\d]+))?(?:(\/[^\s<>#]*)(?:#(.*))?)?$/i);
On 2014/10/31 09:05:33, vsevik wrote:
> Please check whether <> are allowed in query param.

Done.

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/Script.js (right):

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/sdk/Script.js:45: this.scriptId = scriptId;
On 2014/10/30 14:50:43, aandrey wrote:
> duplicated line

Done.

https://codereview.chromium.org/674513002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/sdk/Script.js:46: if (hasSourceURL) {
On 2014/10/31 09:05:34, vsevik wrote:
> Let's try to avoid doing this in this patch since this is unrelated.
> A correct implementation of this url completing would depend on the frame url
> for which this script was loaded which could be different from the main frame
> url in general case. Hence relying on target.url seems not robust.

Done.

[vsevik, 2014-11-05 12:33:19]

lgtm

[sergeyv, 2014-11-05 12:55:25]

The CQ bit was checked by sergeyv@chromium.org

[commit-bot: I haz the power, 2014-11-05 12:56:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/674513002/100001

[commit-bot: I haz the power, 2014-11-05 13:50:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-05 13:50:51]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[sergeyv, 2014-11-06 10:37:28]

The CQ bit was checked by sergeyv@chromium.org

[commit-bot: I haz the power, 2014-11-06 10:38:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/674513002/120001

[commit-bot: I haz the power, 2014-11-06 13:19:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-06 13:19:58]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35158)

[sergeyv, 2014-11-06 14:28:25]

The CQ bit was checked by sergeyv@chromium.org

[commit-bot: I haz the power, 2014-11-06 14:28:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/674513002/120001

[commit-bot: I haz the power, 2014-11-06 15:05:36]

Committed patchset #6 (id:120001) as 184915