Bug 114319 – PageConsole::addMessage should automatically determine column number alongside line number

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Radar WebKit Bug Importer]

<rdar://problem/13614729>

[László Langó]

Created attachment 216775 [details]
Patch

[Joseph Pecoraro]

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.

[László Langó]

Created attachment 218405 [details]
draft

[László Langó]

(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?

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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?

[László Langó]

(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.

[László Langó]

(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?

[Joseph Pecoraro]

> 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.

[László Langó]

Created attachment 218580 [details]
Patch

[Joseph Pecoraro]

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

[László Langó]

Created attachment 218865 [details]
patch for landing

[Joseph Pecoraro]

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? =(

[WebKit Commit Bot]

Comment on attachment 218865 [details]
patch for landing

Clearing flags on attachment: 218865

Committed r160374: <http://trac.webkit.org/changeset/160374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.