Bug 149405 – Web Inspector: Clicking on a stack trace link does not navigate to the corresponding line

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 261654 [details]
[HTML] Reduction

Steps:
1. Open attached stacktrace2.html
2. Open the console
3. Click on the first link, ".../stacktrace2.html:12:13"

Expected:
Debugger opened with the right line scrolled to the viewport and highlighted

Actual:
No lines are highlighted

Notes:
Possibly a regression.

[Radar WebKit Bug Importer]

<rdar://problem/22781191>

[Nikita Vasilyev]

Created attachment 262412 [details]
[Animated GIF] Bug

[Nikita Vasilyev]

Created attachment 262413 [details]
Patch

[Devin Rousso]

Comment on attachment 262413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262413&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:2042
> +        var lineNumber;

I think that for clarity's sake you may want to add a comment here as to why you don't have a default value on lineNumber.  I was originally confused, but once I did some digging I understood that you wanted lineNumber to be undefined if lineColumnMatch was false (hopefully).

[Nikita Vasilyev]

Created attachment 262427 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 262427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262427&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:672
> +    if (typeof lineNumber !== "number")
> +        lineNumber = parseInt(lineNumber);
> +
> +    console.assert(!isNaN(lineNumber), "lineNumber isn't a number.");

How asserting:

    console.assert(typeof lineNumber === "undefined" || typeof lineNumber === "number", "lineNumber should be a number.");

Or just asserting a number and giving lineNumber a default argument value of `0`.

I don't think it makes sense to call openURL with a string line number, we should find and fix that instance, not cover it up.

[Joseph Pecoraro]

> How asserting:

How *about* asserting.

[Nikita Vasilyev]

(In reply to comment #6)
> Comment on attachment 262427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262427&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:672
> > +    if (typeof lineNumber !== "number")
> > +        lineNumber = parseInt(lineNumber);
> > +
> > +    console.assert(!isNaN(lineNumber), "lineNumber isn't a number.");
> 
> How asserting:
> 
>     console.assert(typeof lineNumber === "undefined" || typeof lineNumber
> === "number", "lineNumber should be a number.");

Nit: typeof NaN == "number", so this assert wouldn't catch NaN.

[Nikita Vasilyev]

(In reply to comment #8)
> (In reply to comment #6)
> > Comment on attachment 262427 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=262427&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Base/Main.js:672
> > > +    if (typeof lineNumber !== "number")
> > > +        lineNumber = parseInt(lineNumber);
> > > +
> > > +    console.assert(!isNaN(lineNumber), "lineNumber isn't a number.");
> > 
> > How asserting:
> > 
> >     console.assert(typeof lineNumber === "undefined" || typeof lineNumber
> > === "number", "lineNumber should be a number.");
> 
> Nit: typeof NaN == "number", so this assert wouldn't catch NaN.

How about:

    if (typeof lineNumber !== "undefined")
        console.assert(typeof lineNumber === "number" && !isNaN(lineNumber), "lineNumber should be a number.");

[Joseph Pecoraro]

> Nit: typeof NaN == "number", so this assert wouldn't catch NaN.

I don't think we need to be asserting !NaN everywhere we expect a number, but it seems NaN would end up behaving like 0 here when passed down to SourceCodePosition's constructor.


>     if (typeof lineNumber !== "undefined")
>         console.assert(typeof lineNumber === "number" && !isNaN(lineNumber), "lineNumber should be a number.");

We strip console.assert lines in Production builds. This style of code (assert inside non-brace condition block) would present problems when the assert statement is stripped an suddenly the next statement is inside the if instead of running always.

This:

    if (A)
        assert(...);
    B;
    C;

Suddenly becomes:

    if (A)
        B;
    C;

So while the code is logically correct, our processing of the scripts would change the logic. So we tend to avoid this style of code (asserts inside conditionals).

[Nikita Vasilyev]

Created attachment 262454 [details]
Patch

[Joseph Pecoraro]

> We strip console.assert lines in Production builds. This style of code
> (assert inside non-brace condition block) would present problems when the
> assert statement is stripped an suddenly the next statement is inside the if
> instead of running always.

In fact, Source/WebInspectorUI/Scripts/remove-console-asserts.pl would warn:

>    # Warn about console.assert in control flow statement without braces. Can change logic when stripped.
>    if (/console\.assert/) {
>        if ($previousLine =~ /^\s*(for|if|else|while|do)\b/ && $previousLine !~ /\{\s*$/) {
>            print "WARNING: console.assert inside control flow statement without braces on line: $.: $_";
>        }
>    }

[Joseph Pecoraro]

Comment on attachment 262454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262454&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:2041
> +        var lineNumber;
> +        if (lineColumnMatch)
> +            lineNumber = lineColumnMatch[1] - 1;

Shouldn't this be using parseInt?

[Nikita Vasilyev]

(In reply to comment #13)
> Comment on attachment 262454 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262454&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:2041
> > +        var lineNumber;
> > +        if (lineColumnMatch)
> > +            lineNumber = lineColumnMatch[1] - 1;
> 
> Shouldn't this be using parseInt?

`- 1` implicitly causes parseInt. Should I use parseInt for consistency?

[Nikita Vasilyev]

Created attachment 262456 [details]
Patch

[Blaze Burg]

(In reply to comment #14)
> (In reply to comment #13)
> > Comment on attachment 262454 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=262454&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Base/Main.js:2041
> > > +        var lineNumber;
> > > +        if (lineColumnMatch)
> > > +            lineNumber = lineColumnMatch[1] - 1;
> > 
> > Shouldn't this be using parseInt?
> 
> `- 1` implicitly causes parseInt. Should I use parseInt for consistency?

Please do.

[Nikita Vasilyev]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Comment on attachment 262454 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=262454&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Base/Main.js:2041
> > > > +        var lineNumber;
> > > > +        if (lineColumnMatch)
> > > > +            lineNumber = lineColumnMatch[1] - 1;
> > > 
> > > Shouldn't this be using parseInt?
> > 
> > `- 1` implicitly causes parseInt. Should I use parseInt for consistency?
> 
> Please do.

Fixed in the last patch.

[WebKit Commit Bot]

Comment on attachment 262456 [details]
Patch

Clearing flags on attachment: 262456

Committed r190651: <http://trac.webkit.org/changeset/190651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.