Bug 114319 - PageConsole::addMessage should automatically determine column number alongside line number
Summary: PageConsole::addMessage should automatically determine column number alongsid...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-04-09 16:23 PDT by Joseph Pecoraro
Modified: 2013-12-10 10:50 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.14 KB, patch)
2013-11-13 00:29 PST, László Langó
no flags Details | Formatted Diff | Diff
draft (14.32 KB, patch)
2013-12-04 06:42 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (17.90 KB, patch)
2013-12-06 02:03 PST, László Langó
no flags Details | Formatted Diff | Diff
patch for landing (17.88 KB, patch)
2013-12-10 06:27 PST, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2013-04-09 16:23:02 PDT
Line numbers and stack traces are auto generated when possible for ConsoleMessages created through PageConsole::addMessage. In these cases we should also determine the column number on the line. This is important so that the Web Inspector can accurately jump to the right location to show where the ConsoleMessage originated from.
Comment 1 Radar WebKit Bug Importer 2013-04-09 16:23:17 PDT
<rdar://problem/13614729>
Comment 2 László Langó 2013-11-13 00:29:24 PST
Created attachment 216775 [details]
Patch
Comment 3 Joseph Pecoraro 2013-11-13 10:34:28 PST
Comment on attachment 216775 [details]
Patch

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

> Source/WebCore/page/PageConsole.cpp:156
>              line = parser->lineNumber().oneBasedInt();
> +            column = parser->textPosition().m_column.oneBasedInt();

Nice! You should remove the FIXME on line 145 that points to this bugzilla bug, since you're fixing that now.

Nit: You might as well use textPosition() for both line / column. Under the hood they are doing the same thing, this will ensure less work:

    TextPosition textPosition = parser->textPosition();
    line = textPosition.m_line.oneBasedInt();
    column = textPosition.m_column.oneBasedInt();

r- though, because we should really be adding some kind of a test for this to make sure we don't regress it in the future! That will also help when the other FIXME gets addressed to make sure we don't break this case.
Comment 4 László Langó 2013-12-04 06:42:18 PST
Created attachment 218405 [details]
draft
Comment 5 László Langó 2013-12-04 07:08:33 PST
(In reply to comment #3)
> (From update of attachment 216775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216775&action=review
> 
> > Source/WebCore/page/PageConsole.cpp:156
> >              line = parser->lineNumber().oneBasedInt();
> > +            column = parser->textPosition().m_column.oneBasedInt();
> 
> Nice! You should remove the FIXME on line 145 that points to this bugzilla bug, since you're fixing that now.
> 
> Nit: You might as well use textPosition() for both line / column. Under the hood they are doing the same thing, this will ensure less work:
> 
>     TextPosition textPosition = parser->textPosition();
>     line = textPosition.m_line.oneBasedInt();
>     column = textPosition.m_column.oneBasedInt();
> 
> r- though, because we should really be adding some kind of a test for this to make sure we don't regress it in the future! That will also help when the other FIXME gets addressed to make sure we don't break this case.

I tried to test it, but i could not figure out how to. This method that i had to change is called only from two place:
  Source/WebCore/dom/Document.cpp:2793
  Source/WebCore/loader/DocumentLoader.cpp:585

I don't know how to write a test case that provide the necessary error:
"Refused to display ... in a frame because it set 'X-Frame-Options' to ..."

Do you have any idea how to test this patch?
Comment 6 Joseph Pecoraro 2013-12-04 10:02:54 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 216775 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=216775&action=review
> >
> > r- though, because we should really be adding some kind of a test for this to make sure we don't regress it in the future! That will also help when the other FIXME gets addressed to make sure we don't break this case.
> 
> I tried to test it, but i could not figure out how to. This method that i had to change is called only from two place:
>   Source/WebCore/dom/Document.cpp:2793
>   Source/WebCore/loader/DocumentLoader.cpp:585
> 
> I don't know how to write a test case that provide the necessary error:
> "Refused to display ... in a frame because it set 'X-Frame-Options' to ..."
> 
> Do you have any idea how to test this patch?

There are some existings tests that exercise X-Frame-Options:

    fast/parser/x-frame-options-detached-document-crash.html
    1:This checks that writing an X-Frame-Options meta tag to a detached document does not crash.
    12:  fooDoc.write('<meta http-equiv="X-Frame-Options" content="deny"/>');

    http/tests/cache/x-frame-options-304.html
    2:Test that a 304 response for a resource with an X-Frame-Options header doesn't cause us to crash.

    http/tests/inspector/network/x-frame-options-deny.html
    46:<p>Tests that responseReceived is called on NetworkDispatcher for resource requests denied due to X-Frame-Options header.</p>

    http/tests/security/XFrameOptions/resources/nph-cached-xfo.pl
    2:# Script to generate a 304 HTTP status with (illegal) X-Frame-Options headers.
    12:    print "X-Frame-Options: deny\r\n";
    22:print "X-Frame-Options: allowall\r\n";

    http/tests/security/XFrameOptions/x-frame-options-cached.html
    6:        description('Check that an X-Frame-Options header added by a 304 response does not override one from the original request.');

Maybe viewing those tests can lead you to find a way to test this.
Comment 7 Joseph Pecoraro 2013-12-04 10:05:38 PST
Comment on attachment 218405 [details]
draft

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

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:976
> +        m_pendingCallbacks->appendErrorCallback(type, reinterpret_cast<const xmlChar*>(m), textPosition().m_line, textPosition().m_column);

Would be nice to only call textPosition() once here, and storing it into a local. Depending on the compiler, this may be doing double work?
Comment 8 László Langó 2013-12-05 06:35:47 PST
(In reply to comment #7)
> (From update of attachment 218405 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218405&action=review
> 
> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:976
> > +        m_pendingCallbacks->appendErrorCallback(type, reinterpret_cast<const xmlChar*>(m), textPosition().m_line, textPosition().m_column);
> 
> Would be nice to only call textPosition() once here, and storing it into a local. Depending on the compiler, this may be doing double work?

Yeah this isn't look so good. I'll try to do something with it.
Comment 9 László Langó 2013-12-05 06:45:46 PST
(In reply to comment #6)
> There are some existings tests that exercise X-Frame-Options:
> 
>     fast/parser/x-frame-options-detached-document-crash.html
>     1:This checks that writing an X-Frame-Options meta tag to a detached document does not crash.
>     12:  fooDoc.write('<meta http-equiv="X-Frame-Options" content="deny"/>');
> 
>     http/tests/cache/x-frame-options-304.html
>     2:Test that a 304 response for a resource with an X-Frame-Options header doesn't cause us to crash.
> 
>     http/tests/inspector/network/x-frame-options-deny.html
>     46:<p>Tests that responseReceived is called on NetworkDispatcher for resource requests denied due to X-Frame-Options header.</p>
> 
>     http/tests/security/XFrameOptions/resources/nph-cached-xfo.pl
>     2:# Script to generate a 304 HTTP status with (illegal) X-Frame-Options headers.
>     12:    print "X-Frame-Options: deny\r\n";
>     22:print "X-Frame-Options: allowall\r\n";
> 
>     http/tests/security/XFrameOptions/x-frame-options-cached.html
>     6:        description('Check that an X-Frame-Options header added by a 304 response does not override one from the original request.');
> 
> Maybe viewing those tests can lead you to find a way to test this.

Thanks a lot! Finally I can reach the method and catch the error, when write on the console. But there is another problem with this FIXME. The other one blocks this, because the line (and column) are always zeros in this case. http://trac.webkit.org/browser/trunk/Source/WebCore/page/PageConsole.cpp#L146
In this type of error message the condition in line 153 is always failing and the line never sets.

I think we should test this bug with the zero-s, and then fix the other FIXME. Then change the test when the other FIXME is solved. What do you think?
Comment 10 Joseph Pecoraro 2013-12-05 09:57:36 PST
> Thanks a lot! Finally I can reach the method and catch the error, when write on the console. But there is another problem with this FIXME. The other one blocks this, because the line (and column) are always zeros in this case. http://trac.webkit.org/browser/trunk/Source/WebCore/page/PageConsole.cpp#L146
> In this type of error message the condition in line 153 is always failing and the line never sets.
> 
> I think we should test this bug with the zero-s, and then fix the other FIXME. Then change the test when the other FIXME is solved. What do you think?

Sounds good. When you add the test, add a comment in the test pointing to the FIXME, and that a change in the expected results from 0:0 might be a progression.

Thanks.
Comment 11 László Langó 2013-12-06 02:03:12 PST
Created attachment 218580 [details]
Patch
Comment 12 Joseph Pecoraro 2013-12-09 15:20:10 PST
Comment on attachment 218580 [details]
Patch

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

r=me, though there are a few small nits that would be nice to cleanup.

> Source/WebCore/page/PageConsole.cpp:71
> +            printf("%s:%d:%d: ", sourceURL.utf8().data(), lineNumber, columnNumber);

Nit: This code, pre-existing your patch, is really weird. If you could clean that up at the same time that would be nice. Seems like we just need your new line and you could remove the other else blocks.

    * lineNumber and columnNumber are unsigned. So their values will always be greater than 0. These if checks don't make sense.
    * printf format specifier for unsigned should be %u.

> Source/WebCore/page/PageConsole.cpp:157
> +            line = parser->textPosition().m_line.oneBasedInt();
> +            column = parser->textPosition().m_column.oneBasedInt();

Nit: This still calls textPosition twice. Rather then think about what the compiler would do, I'd rather see a local variable for the result of textPosition().

> Source/WebCore/page/PageConsole.h:51
> +    static void printSourceURLAndPosition(const String& sourceURL, unsigned lineNumber, unsigned columnNumber = 0);

Nit: I've been bitten too many times by optional parameters. It looks like you've covered all of the call sites of this function. I'd like to see this parameter be made non-optional. We always want to push towards having column number everywhere anyways.

> LayoutTests/inspector-protocol/page/deny-X-FrameOption.html:16
> +        // FIXME The line and column values will be zeros until this fix:
> +        // https://bugs.webkit.org/show_bug.cgi?id=125340
> +        // After this probably we should update the expected.txt.
> +        InspectorTest.log(simplifiedMessage.location);

Excellent! Thanks
Comment 13 László Langó 2013-12-10 06:27:32 PST
Created attachment 218865 [details]
patch for landing
Comment 14 Joseph Pecoraro 2013-12-10 10:22:22 PST
Comment on attachment 218865 [details]
patch for landing

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

> Source/WebCore/page/PageConsole.h:51
> +    static void printSourceURLAndPosition(const String& sourceURL, unsigned lineNumber, unsigned columnNumber = 0);

Still optional? =(
Comment 15 WebKit Commit Bot 2013-12-10 10:50:49 PST
Comment on attachment 218865 [details]
patch for landing

Clearing flags on attachment: 218865

Committed r160374: <http://trac.webkit.org/changeset/160374>
Comment 16 WebKit Commit Bot 2013-12-10 10:50:52 PST
All reviewed patches have been landed.  Closing bug.