Currently, we ask callsites to generate stack traces or pass in line numbers when generating console messages. Predictably, dozens (hundreds?) of callsites don't. I'd like to automatically generate stack traces or line numbers when possible. This would basically involve taking the logic that currently lives in 'InspectorResourceAgent::buildInitiatorObject', and moving it somewhere that we could call internally when given a console message that doesn't have information about the context in which it executed. At the end of the day, I think it makes sense to have three 'addMessage' methods on Document, two explicit, one implicit. * addMessage(MessageSource source, MessageType type, MessageLevel level, const String& message, PassRefPtr<ScriptCallStack> callStack, unsigned long requestIdentifier = 0) * addMessage(MessageSource source, MessageType type, MessageLevel level, const String& message, const String& sourceURL, unsigned lineNumber, unsigned long requestIdentifier = 0) * addMessage(MessageSource source, MessageType type, MessageLevel level, const String& message) If the last one is called (and the inspector is open and the console is enabled), we'd generate a stack trace. If it's empty, we'd look for a scriptableDocumentParser to get a URL and line number. I'm very open to suggestion about where that generation should live. I'd agreed with Pavel that putting it directly in Document::addMessage seemed like the wrong place, but now I'm not so sure... that certainly seems like the simplest solution. :)
Er. I was confused. Replace "three 'addMessage' methods on Document" with "three 'addConsoleMessage' methods on ScriptExecutionContext and its subclasses". And maybe on Console too, now that I'm looking at callsites. :)
How about a public static method on ConsoleMessage for non-console clients + its usage in ConsoleMessage's constructor?
(In reply to comment #2) > How about a public static method on ConsoleMessage for non-console clients + its usage in ConsoleMessage's constructor? In order to check the line number, I need a reference to a document to call 'document->scriptableDocumentParser()'. Putting a static method on ConsoleMessage might make sense, but doing the work inside ConsoleMessage's constructor doesn't because we don't have anything that would allow us to get back to the document. I think we need to grab the line number higher in the stack. How about in Console::addMessage? Console knows what frame it's associated with, which would give us the bits we need. Otherwise I think we'd end up piping the document through InspectorInstrumentation::addMessageToConsole into ConsoleMessage. That seems like more work than it's worth. WDYT?
> WDYT? Sounds good!
Created attachment 171670 [details] Patch
Hey Pavel, this is a patch so you can see what I'm thinking. Does it more or less line up with what you thought you were agreeing to? :) Not throwing to the bots yet, as it depends on 100850 landing first. Thanks!
Comment on attachment 171670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171670&action=review > Source/WebCore/page/Console.h:59 > + void addMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL, unsigned lineNumber, unsigned long requestIdentifier = 0); So what do these stand for now? Is it parseLine or arbitrary line number identifying the context? If it is the latter, I think I like the former API where message could have arbitrary url + line context and in addition to that, it could have an optional JavaScript stack.
Comment on attachment 171670 [details] Patch Attachment 171670 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14661278
(In reply to comment #7) > (From update of attachment 171670 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171670&action=review > > > Source/WebCore/page/Console.h:59 > > + void addMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL, unsigned lineNumber, unsigned long requestIdentifier = 0); > > So what do these stand for now? Is it parseLine or arbitrary line number identifying the context? I think it's the latter. Otherwise we would expect a caller to use the callstack version or the completely automatic version. > If it is the latter, I think I like the former API where message could have arbitrary url + line context and in addition to that, it could have an optional JavaScript stack. Currently, we're throwing the line number away if we have a stack trace: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/Console.cpp&type=cs&l=152 It makes sense to me to reflect that in the API. That said, I'm not entirely sure what the current usage looks like. I'm very much open to suggestion about what makes sense.
Comment on attachment 171670 [details] Patch Attachment 171670 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14684237 New failing tests: http/tests/security/canvas-remote-read-remote-image-redirect.html http/tests/security/local-image-from-remote.html http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html http/tests/security/script-with-failed-cors-check-fails-to-load.html http/tests/misc/drag-over-iframe-invalid-source-crash.html http/tests/security/xss-DENIED-xsl-external-entity.xml http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html http/tests/misc/image-blocked-src-change.html http/tests/misc/iframe-invalid-source-crash.html http/tests/misc/bubble-drag-events.html http/tests/misc/image-blocked-src-no-change.html http/tests/misc/unloadable-script.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html http/tests/security/cross-origin-xsl-BLOCKED.html http/tests/security/video-poster-cross-origin-crash.html http/tests/security/img-with-failed-cors-check-fails-to-load.html http/tests/misc/last-modified-parsing.html http/tests/security/video-poster-cross-origin-crash2.html http/tests/inspector/console-xhr-logging.html http/tests/security/contentSecurityPolicy/default-src-inline-blocked.html http/tests/security/frame-loading-via-document-write.html http/tests/security/cross-origin-xsl-redirect-BLOCKED.html http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html http/tests/security/xss-DENIED-xsl-document.xml http/tests/security/contentSecurityPolicy/block-mixed-content-hides-warning.html http/tests/security/xss-DENIED-xml-external-entity.xhtml
Created attachment 172324 [details] WIP #1
Comment on attachment 172324 [details] WIP #1 View in context: https://bugs.webkit.org/attachment.cgi?id=172324&action=review Hey Pavel, this is a stab at #1 that we discussed. Dealing with the inspector clients turned out not to be particularly important because they don't currently accept stack traces (we might want to change that). > Source/WebCore/inspector/InspectorConsoleAgent.cpp:174 > + ASSERT_NOT_REACHED(); Er. I should have removed this line and the next before uploading. Crashing is a nice way to debug. ;)
(In reply to comment #12) > (From update of attachment 172324 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172324&action=review > > Hey Pavel, this is a stab at #1 that we discussed. Dealing with the inspector clients turned out not to be particularly important because they don't currently accept stack traces (we might want to change that). > > > Source/WebCore/inspector/InspectorConsoleAgent.cpp:174 > > + ASSERT_NOT_REACHED(); > > Er. I should have removed this line and the next before uploading. Crashing is a nice way to debug. ;) Actually, ignore this patch. I'm thinking about it some more, and it might make sense to go another route.
Created attachment 174985 [details] Patch
(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 172324 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=172324&action=review > > > > Hey Pavel, this is a stab at #1 that we discussed. Dealing with the inspector clients turned out not to be particularly important because they don't currently accept stack traces (we might want to change that). > > > > > Source/WebCore/inspector/InspectorConsoleAgent.cpp:174 > > > + ASSERT_NOT_REACHED(); > > > > Er. I should have removed this line and the next before uploading. Crashing is a nice way to debug. ;) > > Actually, ignore this patch. I'm thinking about it some more, and it might make sense to go another route. Ok, please don't ignore the new patch. :) I need to regenerate test results for Chromium (since something will likely break there), but before I go through the trouble of migrating things over to that linux machine over there, I'd like your take on the approach. The next step will be to get rid of the externally-visible CallStack endpoints, by replacing them with an optional ScriptState*. That will allow me to get rid of the callstack generation in ContentSecurityPolicy, but it's distinct enough from this patch that I'd like to do it separately if that's alright with you. WDYT?
Comment on attachment 174985 [details] Patch Attachment 174985 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14894247 New failing tests: http/tests/eventsource/eventsource-bad-mime-type.html accessibility/presentational-elements-with-focus.html http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html canvas/philip/tests/2d.fillStyle.parse.rgba-solid-3.html http/tests/canvas/philip/tests/security.pattern.cross.html http/tests/appcache/deferred-events-delete-while-raising.html http/tests/appcache/deferred-events-delete-while-raising-timer.html http/tests/canvas/philip/tests/security.pattern.canvas.timing.html http/tests/canvas/philip/tests/security.pattern.image.strokeStyle.html http/tests/canvas/philip/tests/security.reset.html canvas/philip/tests/2d.gradient.object.current.html http/tests/canvas/philip/tests/security.drawImage.image.html accessibility/aria-toggle-button-with-title.html http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html canvas/philip/tests/2d.drawImage.incomplete.html http/tests/cache/cancel-during-failure-crash.html canvas/philip/tests/2d.fillStyle.parse.hsl-6.html http/tests/canvas/philip/tests/security.pattern.image.fillStyle.html http/tests/appcache/deferred-events.html http/tests/canvas/philip/tests/security.drawImage.canvas.html http/tests/appcache/online-fallback-layering.html canvas/philip/tests/2d.fillStyle.parse.rgba-solid-4.html accessibility/removed-continuation-element-causes-crash.html http/tests/appcache/video.html http/tests/appcache/foreign-fallback.html accessibility/aria-fallback-roles.html canvas/philip/tests/2d.pattern.image.incomplete.html http/tests/canvas/philip/tests/security.pattern.create.html http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html http/tests/eventsource/eventsource-content-type-charset.html
Created attachment 175171 [details] Patch
(In reply to comment #16) > (From update of attachment 174985 [details]) > Attachment 174985 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/14894247 > > New failing tests: > http/tests/eventsource/eventsource-bad-mime-type.html > accessibility/presentational-elements-with-focus.html > http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html > canvas/philip/tests/2d.fillStyle.parse.rgba-solid-3.html > http/tests/canvas/philip/tests/security.pattern.cross.html > http/tests/appcache/deferred-events-delete-while-raising.html > http/tests/appcache/deferred-events-delete-while-raising-timer.html > http/tests/canvas/philip/tests/security.pattern.canvas.timing.html > http/tests/canvas/philip/tests/security.pattern.image.strokeStyle.html > http/tests/canvas/philip/tests/security.reset.html > canvas/philip/tests/2d.gradient.object.current.html > http/tests/canvas/philip/tests/security.drawImage.image.html > accessibility/aria-toggle-button-with-title.html > http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html > canvas/philip/tests/2d.drawImage.incomplete.html > http/tests/cache/cancel-during-failure-crash.html > canvas/philip/tests/2d.fillStyle.parse.hsl-6.html > http/tests/canvas/philip/tests/security.pattern.image.fillStyle.html > http/tests/appcache/deferred-events.html > http/tests/canvas/philip/tests/security.drawImage.canvas.html > http/tests/appcache/online-fallback-layering.html > canvas/philip/tests/2d.fillStyle.parse.rgba-solid-4.html > accessibility/removed-continuation-element-causes-crash.html > http/tests/appcache/video.html > http/tests/appcache/foreign-fallback.html > accessibility/aria-fallback-roles.html > canvas/philip/tests/2d.pattern.image.incomplete.html > http/tests/canvas/philip/tests/security.pattern.create.html > http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html > http/tests/eventsource/eventsource-content-type-charset.html A new patch, now with less crashing on Chromium! :)
Friendly ping. I'd like to decide how this bit is going to look before I get too far along with the next bit. :)
Comment on attachment 175171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175171&action=review > Source/WebCore/inspector/ConsoleMessage.h:55 > + ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, const String& u, unsigned li, unsigned long requestIdentifier = 0, Document* = 0); Why do we still need u and li parameters here?
Comment on attachment 175171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175171&action=review >> Source/WebCore/inspector/ConsoleMessage.h:55 >> + ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, const String& u, unsigned li, unsigned long requestIdentifier = 0, Document* = 0); > > Why do we still need u and li parameters here? I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance.
> I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance. I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one...
(In reply to comment #22) > I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one... I'll take a look. My intention was to keep the patches small. Dropping URLs and line numbers will touch a few additional files. :)
Created attachment 176247 [details] Patch
(In reply to comment #22) > > I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance. > > I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one... Ok. The new patch removes line numbers for most callers of ScriptExecutionContext::addConsoleMessage, and creates a clean pipe down to ConsoleMessage by adding a new variant of the intermediate methods that doesn't accept a line number/url or stack trace. The goal will be to get rid of the other paths. I'm putting together a doc that shows the remaining call sites that need work; we should talk about them at some point. :)
Created attachment 176252 [details] Patch
(In reply to comment #25) > (In reply to comment #22) > > > I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance. > > > > I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one... > > Ok. The new patch removes line numbers for most callers of ScriptExecutionContext::addConsoleMessage, and creates a clean pipe down to ConsoleMessage by adding a new variant of the intermediate methods that doesn't accept a line number/url or stack trace. The goal will be to get rid of the other paths. > > I'm putting together a doc that shows the remaining call sites that need work; we should talk about them at some point. :) Uploaded too quickly. The latest patch strips line numbers from a few more call sites.
Comment on attachment 176252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176252&action=review > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-197 > - m_document->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, reason, m_handshake->clientOrigin()); Is m_handshake->clientOrigin() always the same as m_document->securityOrigin()? > Source/WebCore/inspector/ConsoleMessage.cpp:57 > + : m_source(s) > + , m_type(t) > + , m_level(l) > + , m_message(m) Please use complete words in variable names. For example, s -> source; t -> type > Source/WebCore/inspector/ConsoleMessage.cpp:58 > + , m_url() No need to initialize this variable explicitly. The compiler will do it for you. > Source/WebCore/inspector/ConsoleMessage.h:58 > + ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, unsigned long requestIdentifier = 0, Document* = 0); > ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, const String& u, unsigned li, unsigned long requestIdentifier = 0); > - ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, PassRefPtr<ScriptArguments>, PassRefPtr<ScriptCallStack>, unsigned long requestIdentifier = 0); > + ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, PassRefPtr<ScriptCallStack>, unsigned long requestIdentifier = 0); > + ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, PassRefPtr<ScriptArguments>, ScriptState*, unsigned long requestIdentifier = 0); m -> message
This looks like an improvement to me, but I'll let someone from inspector-land do the actual review.
(In reply to comment #28) > > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-197 > > - m_document->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, reason, m_handshake->clientOrigin()); > > Is m_handshake->clientOrigin() always the same as m_document->securityOrigin()? I'm not sure. But I'm also not sure it should be passed in as the console message's URL. I suspect it would be better to have the failing URL as part of the console message. Who would be a good person to talk to about these errors in particular? > > Source/WebCore/inspector/ConsoleMessage.cpp:57 > > + : m_source(s) > > + , m_type(t) > > + , m_level(l) > > + , m_message(m) > > Please use complete words in variable names. For example, s -> source; t -> type I was thinking about that while copy/pasting... :) I'll do a quick style patch in a separate bug and rebase this one on top of it.
> Who would be a good person to talk to about these errors in particular? I would use svn blame to see who added that line of code and ask them.
(In reply to comment #31) > > Who would be a good person to talk to about these errors in particular? > > I would use svn blame to see who added that line of code and ask them. Ha! I'll do that. :)
(In reply to comment #28) > (From update of attachment 176252 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176252&action=review > > > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-197 > > - m_document->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, reason, m_handshake->clientOrigin()); > > Is m_handshake->clientOrigin() always the same as m_document->securityOrigin()? The console message is at least two years old. I stopped digging through blame logs after that. :) Instead, I dug through the code a bit: it looks like WebSocketHandshake::clientOrigin returns the origin of the ScriptExecutionContext in which the WebSocket was created. If the failure is due to a JavaScript error, the resulting console message will have a stack trace, and will set the URL appropriately. If not, I don't think we're actually adding any value to the console message by noting that it happened in the context of the currently active document: leaving the URL empty doesn't really hurt. I'll ping some random websockets folks to see if anyone has a strong opinion about it.
(In reply to comment #33) > I'll ping some random websockets folks to see if anyone has a strong opinion about it. After looking a bit more, I think we should just change the message here in a separate patch. I've thrown something up at https://bugs.webkit.org/show_bug.cgi?id=103448 that should obviate this portion of the patch we're discussing here, and poked relevant folks for a review. Thanks for asking the question, Adam. :)
Created attachment 176424 [details] Patch
Comment on attachment 176424 [details] Patch Attachment 176424 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15027327
Comment on attachment 176424 [details] Patch Attachment 176424 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15018370
Created attachment 176454 [details] Patch
(In reply to comment #38) > Created an attachment (id=176454) [details] > Patch Moar test expectation updates. Lots of line number dropping out of the expectations, as they're now only generated if the inspector is open. Not sure if I like that.
Comment on attachment 176454 [details] Patch Attachment 176454 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15013455
Comment on attachment 176454 [details] Patch Attachment 176454 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15029345
(In reply to comment #41) > (From update of attachment 176454 [details]) > Attachment 176454 [details] did not pass cr-android-ews (chromium-android): > Output: http://queues.webkit.org/results/15029345 I don't understand why this doesn't build on the bots. The build error is Source/WebCore/Modules/websockets/WebSocketChannel.cpp:197:126: error: no matching function for call to 'WebCore::Document::addConsoleMessage(WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, const WTF::String&, WTF::String)' but that line was killed this morning in http://trac.webkit.org/changeset/135981 I guess the bots haven't caught up yet? I'll upload another patch in a few hours. :(
Created attachment 176762 [details] Rebased.
Comment on attachment 176762 [details] Rebased. View in context: https://bugs.webkit.org/attachment.cgi?id=176762&action=review > Source/WebCore/inspector/InspectorConsoleAgent.cpp:169 > + document = m_instrumentingAgents->inspectorAgent()->inspectedDocument(); m_instrumentingAgents are supposed to be read only in InspectorInstrumentation.* If you need to access inspectorAgent you can pass it in the constructor of InspectorConsoleAgent. But in this case you seem to need access to the document where the message is being logged, you should pass it directly as it main differ from the document of main frame that inspectedDocument() returns. > LayoutTests/fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction-expected.txt:3 > +FAIL gFocusedDocument.activeElement.getAttribute("id") should be start. Was end. Hmm, the test is failing now, we should avoid such rebaseline.
(In reply to comment #39) > (In reply to comment #38) > > Created an attachment (id=176454) [details] [details] > > Patch > > Moar test expectation updates. Lots of line number dropping out of the expectations, as they're now only generated if the inspector is open. Not sure if I like that. Not providing line number where did so is a regression. We would like to see correct line numbers even if inspector wasn't open when the message was logged.
(In reply to comment #44) > (From update of attachment 176762 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176762&action=review > > > Source/WebCore/inspector/InspectorConsoleAgent.cpp:169 > > + document = m_instrumentingAgents->inspectorAgent()->inspectedDocument(); > > m_instrumentingAgents are supposed to be read only in InspectorInstrumentation.* If you need to access inspectorAgent you can pass it in the constructor of InspectorConsoleAgent. But in this case you seem to need access to the document where the message is being logged, you should pass it directly as it main differ from the document of main frame that inspectedDocument() returns. Pass it all the way through from Document::addMessage? Hrm. Too bad. This solution seemed clean, but if the inspectedDocument isn't what I thought it was, then I'll need to pipe it through. > > LayoutTests/fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction-expected.txt:3 > > +FAIL gFocusedDocument.activeElement.getAttribute("id") should be start. Was end. > > Hmm, the test is failing now, we should avoid such rebaseline. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fspatial-navigation :( Good eye. I'll just let it keep failing.
Created attachment 176857 [details] Patch
Comment on attachment 176857 [details] Patch Not fot landing; I need to rebase lots of tests and clean up the changelog. Still, I'd like to know whether this is the right direction before I do so. Would you mind taking another look? I've stripped out all the layout test changes so the actual code is easier to parse.
Comment on attachment 176857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176857&action=review > Source/WebCore/dom/ScriptExecutionContext.cpp:326 > + addMessage(source, type, level, message, requestIdentifier); I'd rather make this new addConsoleMessage pure virtual method and removed the new addMessage. > Source/WebCore/page/Console.cpp:145 > + url = document->url().string(); Only line number depends on the current parser state, you can unconditionally retrieve url from the document if one is provided. > Source/WebCore/page/Console.cpp:170 > + else if (!url.isNull() && lineNumber) Why do you need this branching? > Source/WebCore/workers/WorkerContext.cpp:289 > + addMessageToWorkerConsole(source, type, level, message, String(), 0, 0, requestIdentifier); In case of workers we should probably try to collect stack trace/get url+line here before posting addConsoleMessage to the main thread.
Comment on attachment 176857 [details] Patch Attachment 176857 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15064082 New failing tests: http/tests/misc/iframe-invalid-source-crash.html http/tests/security/xssAuditor/embed-tag-code-attribute-2.html http/tests/security/xssAuditor/embed-tag-null-char.html http/tests/security/xssAuditor/embed-tag-javascript-url.html http/tests/security/mixedContent/insecure-css-in-iframe.html http/tests/misc/drag-over-iframe-invalid-source-crash.html http/tests/security/xssAuditor/cookie-injection.html http/tests/security/xssAuditor/embed-tag-control-char.html http/tests/security/xssAuditor/full-block-javascript-link.html http/tests/security/cross-origin-xsl-BLOCKED.html http/tests/security/xssAuditor/full-block-iframe-no-inherit.php http/tests/security/xssAuditor/full-block-get-from-iframe.html http/tests/security/xssAuditor/base-href-scheme-relative.html http/tests/security/xssAuditor/full-block-base-href.html http/tests/security/mixedContent/insecure-css-in-main-frame.html http/tests/security/xssAuditor/base-href-null-char.html http/tests/misc/bubble-drag-events.html http/tests/security/mixedContent/insecure-image-in-main-frame.html http/tests/misc/image-blocked-src-no-change.html http/tests/security/xssAuditor/embed-tag.html http/tests/security/xssAuditor/form-action.html http/tests/misc/image-blocked-src-change.html http/tests/security/xssAuditor/base-href.html http/tests/security/xssAuditor/full-block-iframe-javascript-url.html http/tests/security/xssAuditor/full-block-link-onclick.html http/tests/security/frame-loading-via-document-write.html http/tests/inspector/console-xhr-logging.html http/tests/security/xssAuditor/embed-tag-code-attribute.html http/tests/security/xssAuditor/base-href-control-char.html http/tests/security/xss-DENIED-xml-external-entity.xhtml
Comment on attachment 176857 [details] Patch Attachment 176857 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15058172 New failing tests: http/tests/misc/iframe-invalid-source-crash.html http/tests/security/xssAuditor/embed-tag-code-attribute-2.html http/tests/security/xssAuditor/embed-tag-null-char.html http/tests/security/xssAuditor/embed-tag-javascript-url.html http/tests/security/mixedContent/insecure-css-in-iframe.html http/tests/misc/drag-over-iframe-invalid-source-crash.html http/tests/security/xssAuditor/cookie-injection.html http/tests/security/xssAuditor/embed-tag-control-char.html http/tests/security/xssAuditor/full-block-javascript-link.html http/tests/security/cross-origin-xsl-BLOCKED.html http/tests/security/xssAuditor/full-block-iframe-no-inherit.php http/tests/security/xssAuditor/full-block-get-from-iframe.html http/tests/security/xssAuditor/base-href-scheme-relative.html http/tests/security/xssAuditor/full-block-base-href.html http/tests/security/mixedContent/insecure-css-in-main-frame.html http/tests/security/xssAuditor/base-href-null-char.html http/tests/misc/bubble-drag-events.html http/tests/security/mixedContent/insecure-image-in-main-frame.html http/tests/misc/image-blocked-src-no-change.html http/tests/security/xssAuditor/embed-tag.html http/tests/security/xssAuditor/form-action.html http/tests/misc/image-blocked-src-change.html http/tests/security/xssAuditor/base-href.html http/tests/security/xssAuditor/full-block-iframe-javascript-url.html http/tests/security/xssAuditor/full-block-link-onclick.html http/tests/security/frame-loading-via-document-write.html http/tests/inspector/console-xhr-logging.html http/tests/security/xssAuditor/embed-tag-code-attribute.html http/tests/security/xssAuditor/base-href-control-char.html http/tests/security/xss-DENIED-xml-external-entity.xhtml
Created attachment 176963 [details] Patch
(In reply to comment #52) > Created an attachment (id=176963) [details] > Patch Patch is getting big. :) I think this addresses your concerns, Yury. I've rebaselined the tests I can run locally, but I expect more will break. You'll still see a lot of line number disappearing from the SVG tests. I checked those manually; they shouldn't have been generated in the first place, as they're all coming from JavaScript. They're just pointing to "</script>", where the parser is sitting while JS executes. Would you mind taking another look? Thanks!
Comment on attachment 176963 [details] Patch Attachment 176963 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15065236 New failing tests: svg/dom/SVGScriptElement/script-onerror-bubbling.svg
Comment on attachment 176963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176963&action=review > Source/WebCore/page/Console.cpp:140 > + String url = document ? document->url().string() : String(); String url; if (document) url = document->url().string(); > LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters-expected.txt:2 > +CONSOLE MESSAGE: Error while parsing the 'sandbox' attribute: 'allow-scriptsallow-forms' is an invalid sandbox flag. We are still losing console line numbers in many places where it may be useful. Can you avoid such regressions?
Comment on attachment 176963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176963&action=review >> LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters-expected.txt:2 >> +CONSOLE MESSAGE: Error while parsing the 'sandbox' attribute: 'allow-scriptsallow-forms' is an invalid sandbox flag. > > We are still losing console line numbers in many places where it may be useful. Can you avoid such regressions? Here, line 41 is the end of the file: http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters.html We shouldn't have generated it in the first place, as this is all coming from script. I've spot-checked several locations and come to the same conclusions each time. Looking at the call sites I've modified, it's clear why: most weren't properly testing to see if the line number ScriptableDocumentParser gave out was meaningful (e.g. we're parsing, and we're not in script).
(In reply to comment #56) > (From update of attachment 176963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176963&action=review > > >> LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters-expected.txt:2 > >> +CONSOLE MESSAGE: Error while parsing the 'sandbox' attribute: 'allow-scriptsallow-forms' is an invalid sandbox flag. > > > > We are still losing console line numbers in many places where it may be useful. Can you avoid such regressions? > > Here, line 41 is the end of the file: http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters.html We shouldn't have generated it in the first place, as this is all coming from script. > > I've spot-checked several locations and come to the same conclusions each time. Looking at the call sites I've modified, it's clear why: most weren't properly testing to see if the line number ScriptableDocumentParser gave out was meaningful (e.g. we're parsing, and we're not in script). That's not at all to say I'm totally 100% sure I'm right. If you see other suspicious locations, I'm really happy to check them out. :)
Created attachment 176994 [details] Patch
*** Bug 97979 has been marked as a duplicate of this bug. ***
Comment on attachment 176994 [details] Patch Attachment 176994 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15083752 New failing tests: http/tests/security/cross-origin-xsl-BLOCKED.html http/tests/security/xssAuditor/javascript-link-HTML-entities-null-char.html
Created attachment 177215 [details] Patch for landing
Comment on attachment 177215 [details] Patch for landing Clearing flags on attachment: 177215 Committed r136377: <http://trac.webkit.org/changeset/136377>
All reviewed patches have been landed. Closing bug.
I'll follow up on this with rebaselines for other ports as the bots catch up. Thanks for the review, Yury!
(In reply to comment #64) > I'll follow up on this with rebaselines for other ports as the bots catch up. The EFL bots are not green to start with so please ask if something seems fishy. Don't mind the image flakes, we have a snapshot-related problem at the moment, but these seem to have appeared after this patch (http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2): Unexpected flakiness: text-only failures http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html http/tests/xmlhttprequest/cross-site-denied-response.html http/tests/xmlhttprequest/origin-whitelisting-https.html Regressions: Unexpected text-only failures fast/media/mq-resolution-dpi-dpcm-warning.html fast/media/mq-resolution.html fast/media/w3c/test_media_queries.html Regressions: Unexpected crashes (8) fast/workers/shared-worker-exception.html [ Crash ] fast/workers/shared-worker-script-error.html [ Crash ] fast/workers/storage/open-database-set-empty-version-sync.html [ Crash ] http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html [ Crash ] http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html [ Crash ] http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html [ Crash ] http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html [ Crash ] http/tests/xmlhttprequest/workers/shared-worker-access-control-basic-get-fail-non-simple.html [ Crash ]
(In reply to comment #65) > (In reply to comment #64) > > I'll follow up on this with rebaselines for other ports as the bots catch up. > > The EFL bots are not green to start with so please ask if something seems fishy. Don't mind the image flakes, we have a snapshot-related problem at the moment, but these seem to have appeared after this patch (http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2): > > Unexpected flakiness: text-only failures > http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html > http/tests/xmlhttprequest/cross-site-denied-response.html > http/tests/xmlhttprequest/origin-whitelisting-https.html > > Regressions: Unexpected text-only failures > fast/media/mq-resolution-dpi-dpcm-warning.html > fast/media/mq-resolution.html > fast/media/w3c/test_media_queries.html > > Regressions: Unexpected crashes (8) > fast/workers/shared-worker-exception.html [ Crash ] > fast/workers/shared-worker-script-error.html [ Crash ] > fast/workers/storage/open-database-set-empty-version-sync.html [ Crash ] > http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html [ Crash ] > http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html [ Crash ] > http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html [ Crash ] > http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html [ Crash ] > http/tests/xmlhttprequest/workers/shared-worker-access-control-basic-get-fail-non-simple.html [ Crash ] *sigh* Yes. It's crashing on the Apple port as well in an ASSERT. I should have tested on a debug build :( I'll revert it and figure things out. Bleh. Thanks!
Re-opened since this is blocked by bug 103881
Created attachment 177254 [details] Now with less crashing.
Hey Yury, mind having another look at this? The only change in the most recent patch is a change to the way that we generate stacks for workers. Previously, I called out to createScriptCallStack directly, which ends up (in JSC) in an ASSERT that we're on the main thread. That doesn't work so well. This patch generates a ScriptState* from the WorkerContext, and pipes that through the instrumentation bits into ConsoleMessage, which generate the stack directly from the script state when available. I've tested this in debug builds locally; it works. I'd appreciate your feedback as to whether it's also correct. :)
Comment on attachment 177254 [details] Now with less crashing. View in context: https://bugs.webkit.org/attachment.cgi?id=177254&action=review > Source/WebCore/inspector/ConsoleMessage.cpp:118 > + m_callStack = state ? createScriptCallStackForConsole(state) : createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true) still can fail in case of workers if someone forgets to pass non-0 state? > Source/WebCore/workers/WorkerContext.cpp:309 > + InspectorInstrumentation::addMessageToConsole(this, source, type, level, message, sourceURL, lineNumber, scriptStateFromWorkerContext(this), requestIdentifier); Retrieving script state from worker context will not give you access to the top frame and so you will always have empty stack trace in such cases. You should use ScriptState* that is available where the message is logged just like you do in case of document. There is no 'console' binding in worker contexts so far but we should either provide correct implementation or postpone that until when we add 'console' binding to workers.
(In reply to comment #70) > createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true) still can fail in case of workers if someone forgets to pass non-0 state? Yes. At the moment there's only one call site (everything goes through WorkerContext::addMessage, so I'm not too worried about it. What would you suggest? > > Source/WebCore/workers/WorkerContext.cpp:309 > > + InspectorInstrumentation::addMessageToConsole(this, source, type, level, message, sourceURL, lineNumber, scriptStateFromWorkerContext(this), requestIdentifier); > > Retrieving script state from worker context will not give you access to the top frame and so you will always have empty stack trace in such cases. You should use ScriptState* that is available where the message is logged just like you do in case of document. I'm not sure I understand this point. The only place I'm grabbing ScriptState objects at the moment are in the bindings. I'm not actually sure how to get a ScriptState object from inside Document or Console where I'd need it. ScriptState::forContext only exists in V8 bindings, and ScriptState::scriptStateFromPage requires a DOMWrapperWorld that I also don't know how to get access to (and has a comment in the V8 implementation that says more or less that it should only be used for tests). This was the best I came up with today. I'd really appreciate pointers in the right direction, since it sounds like the solution I found isn't great. :) > There is no 'console' binding in worker contexts so far but we should either provide correct implementation or postpone that until when we add 'console' binding to workers. Given that it doesn't make the situation for workers any worse than it already is, I'd suggest splitting any further work on the API into a separate patch (though, of course I'd say that... ;)). Thanks!
(In reply to comment #71) > I'm not sure I understand this point. The only place I'm grabbing ScriptState objects at the moment are in the bindings. *the only place I'm grabbing ScriptState before this last patch. This patch, of course, grabs ScriptState inside WorkerContext::addMessageToConsole. ;)
(In reply to comment #71) > (In reply to comment #70) > > createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true) still can fail in case of workers if someone forgets to pass non-0 state? > > Yes. At the moment there's only one call site (everything goes through WorkerContext::addMessage, so I'm not too worried about it. > > What would you suggest? > We should not crash here. One way to avoid this would be to check if we're on the main thread in createScriptCallStack in case of JSC and bail out if not. > > > Source/WebCore/workers/WorkerContext.cpp:309 > > > + InspectorInstrumentation::addMessageToConsole(this, source, type, level, message, sourceURL, lineNumber, scriptStateFromWorkerContext(this), requestIdentifier); > > > > Retrieving script state from worker context will not give you access to the top frame and so you will always have empty stack trace in such cases. You should use ScriptState* that is available where the message is logged just like you do in case of document. > > I'm not sure I understand this point. The only place I'm grabbing ScriptState objects at the moment are in the bindings. I'm not actually sure how to get a ScriptState object from inside Document or Console where I'd need it. You need to pass it there explicitly if you have one. My point was that it doen't make much sense to get ScriptState for worker unless you are in the bindings as it won't let you generate a call stack anyway. Is it possible to change Source/WebCore/inspector/ConsoleMessage.cpp:118 so that it only generated stack trace if script state is not null? > ScriptState::forContext only exists in V8 bindings, and ScriptState::scriptStateFromPage requires a DOMWrapperWorld that I also don't know how to get access to (and has a comment in the V8 implementation that says more or less that it should only be used for tests). > scriptStateFromPage is not supposed to work for workers as it name suggests. > This was the best I came up with today. I'd really appreciate pointers in the right direction, since it sounds like the solution I found isn't great. :) > > > There is no 'console' binding in worker contexts so far but we should either provide correct implementation or postpone that until when we add 'console' binding to workers. > > Given that it doesn't make the situation for workers any worse than it already is, I'd suggest splitting any further work on the API into a separate patch (though, of course I'd say that... ;)). > > Thanks! I agree.
Created attachment 177556 [details] Patch
(In reply to comment #74) > Created an attachment (id=177556) [details] > Patch Thanks, Yury. The current patch is the result of our discussion this evening: it adds an 'isWorkerAgent' method to InspectorConsoleAgent, and uses it to determine whether a stack should be generated. This avoids the crashes we saw earlier, at least on my machine. :) WDYT?
Created attachment 177579 [details] Patch
Comment on attachment 177579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177579&action=review > Source/WebCore/inspector/ConsoleMessage.cpp:116 > + if (!canGenerateCallStack) We should check this only if state is null to avoid crash in createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); if state is presented then it is ok to collect it call stack even in case of workers or am I missing something?
Created attachment 177671 [details] Patch
(In reply to comment #77) > (From update of attachment 177579 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177579&action=review > > > Source/WebCore/inspector/ConsoleMessage.cpp:116 > > + if (!canGenerateCallStack) > > We should check this only if state is null to avoid crash in createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); if state is presented then it is ok to collect it call stack even in case of workers or am I missing something? Sure, I can do that. It's in the latest patch. WDYT?
Comment on attachment 177671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177671&action=review > Source/WebCore/inspector/ConsoleMessage.cpp:122 > + m_callStack = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); It is enough to capture only top frame if you need just url and line number. > Source/WebCore/inspector/ConsoleMessage.cpp:133 > + m_callStack.clear(); Why do you clear call stack here?
For clarity, this is what we discussed via chat: (In reply to comment #80) > (From update of attachment 177671 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177671&action=review > > > Source/WebCore/inspector/ConsoleMessage.cpp:122 > > + m_callStack = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); > > It is enough to capture only top frame if you need just url and line number. I think we want the whole stack to display to the developer. Grabbing only the top frame wouldn't be detailed enough. > > Source/WebCore/inspector/ConsoleMessage.cpp:133 > > + m_callStack.clear(); > > Why do you clear call stack here? Given the 'return' in the if block above, I only clear the stack if it's either already null or if it's non-null but empty. If we couldn't generate a usable stack, I wanted to ensure that we didn't try to display it to the user.
Comment on attachment 177671 [details] Patch Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :) I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :)
(In reply to comment #82) > (From update of attachment 177671 [details]) > Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :) > > I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :) EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>.
(In reply to comment #83) > (In reply to comment #82) > > (From update of attachment 177671 [details] [details]) > > Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :) > > > > I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :) > > EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>. QT in r136670: <http://trac.webkit.org/changeset/136670>.
(In reply to comment #84) > (In reply to comment #83) > > (In reply to comment #82) > > > (From update of attachment 177671 [details] [details] [details]) > > > Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :) > > > > > > I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :) > > > > EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>. > > QT in r136670: <http://trac.webkit.org/changeset/136670>. GTK in r136672: <http://trac.webkit.org/changeset/136672>.
(In reply to comment #85) > (In reply to comment #84) > > (In reply to comment #83) > > > (In reply to comment #82) > > > > (From update of attachment 177671 [details] [details] [details] [details]) > > > > Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :) > > > > > > > > I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :) > > > > > > EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>. > > > > QT in r136670: <http://trac.webkit.org/changeset/136670>. > > GTK in r136672: <http://trac.webkit.org/changeset/136672>. And minor Apple/Chromium cleanup in r136673: <http://trac.webkit.org/changeset/136673>. Should be the last of them. :) *whew*
This seems to have made a bunch of xhr tests flaky for at least EFL: -CONSOLE MESSAGE: XMLHttpRequest cannot load http://localhost:8000/xmlhttprequest/resources/get.txt. Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin. +CONSOLE MESSAGE: line 58: XMLHttpRequest cannot load http://localhost:8000/xmlhttprequest/resources/get.txt. Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin. This happens ~10% of the time. http/tests/xmlhttprequest/origin-whitelisting-https.html is an example.
(In reply to comment #87) > This seems to have made a bunch of xhr tests flaky for at least EFL: Bug 105280 filed.