WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
114319
PageConsole::addMessage should automatically determine column number alongside line number
https://bugs.webkit.org/show_bug.cgi?id=114319
Summary
PageConsole::addMessage should automatically determine column number alongsid...
Joseph Pecoraro
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2013-04-09 16:23:17 PDT
<
rdar://problem/13614729
>
László Langó
Comment 2
2013-11-13 00:29:24 PST
Created
attachment 216775
[details]
Patch
Joseph Pecoraro
Comment 3
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.
László Langó
Comment 4
2013-12-04 06:42:18 PST
Created
attachment 218405
[details]
draft
László Langó
Comment 5
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?
Joseph Pecoraro
Comment 6
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.
Joseph Pecoraro
Comment 7
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?
László Langó
Comment 8
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.
László Langó
Comment 9
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?
Joseph Pecoraro
Comment 10
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.
László Langó
Comment 11
2013-12-06 02:03:12 PST
Created
attachment 218580
[details]
Patch
Joseph Pecoraro
Comment 12
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
László Langó
Comment 13
2013-12-10 06:27:32 PST
Created
attachment 218865
[details]
patch for landing
Joseph Pecoraro
Comment 14
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? =(
WebKit Commit Bot
Comment 15
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
>
WebKit Commit Bot
Comment 16
2013-12-10 10:50:52 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug