I've been reliably informed by pfeldman that generating call stacks is a good way of ensuring that there's enough detail in the console message to figure out what's going on. This is particularly true for the various bits of inline code that CSP might block when they're injected via JavaScript. We still won't be able to point directly to them via a line number, but the call stack should give developers enough info to determine what they're doing that's causing an error. Whenever relevant, we should generate a call stack, and pipe it through the CSP check in order to get it into the log if it's relevant.
Created attachment 159304 [details] Patch
Hi Adam! This patch adds moar detail to CSP's console logs by generating call stacks, and passing them down to the CSP object. I think it's pretty straightforward from the CSP side, though I'd love to have tests. Speaking of tests, and the console, hello Pavel! Do you have a recommendation regarding testing for this patch? I've added call stacks to a variety of console warnings, but DRT doesn't currently output anything that would identify which logs have call stacks, and which don't. Is there a mechanism I could use to make that determination without modifying DRT (and then rebaselining every test that uses the console? :) ). Thanks to you both!
Actually adding Pavel to CC this time.
Comment on attachment 159304 [details] Patch Attachment 159304 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13527742
Comment on attachment 159304 [details] Patch Attachment 159304 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13531719
Comment on attachment 159304 [details] Patch Attachment 159304 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13538441
Comment on attachment 159304 [details] Patch Attachment 159304 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13531721
Comment on attachment 159304 [details] Patch Attachment 159304 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13539332
Comment on attachment 159304 [details] Patch Attachment 159304 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13528736 New failing tests: http/tests/inspector/appcache/appcache-iframe-manifests.html http/tests/inspector/injected-script-for-origin.html http/tests/inspector/extensions-network-redirect.html http/tests/inspector/change-iframe-src.html http/tests/inspector/modify-cross-domain-rule.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-resource-errors.html http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html http/tests/inspector/extensions-headers.html http/tests/inspector-enabled/injected-script-discard.html http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html http/tests/inspector/extensions-useragent.html http/tests/inspector/console-cd-completions.html http/tests/inspector/compiler-script-mapping.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html http/tests/inspector/compiler-source-mapping-debug.html http/tests/inspector/web-socket-frame-error.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/console-cd.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network-preflight-options.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/database-open.html
Created attachment 159308 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Looks like you've got some test failures to work through...
(In reply to comment #11) > Looks like you've got some test failures to work through... Hrm. Well, at least the style bot passed. :) Back to the drawing board!
Created attachment 160736 [details] Patch
(In reply to comment #12) > (In reply to comment #11) > > Looks like you've got some test failures to work through... > > Hrm. Well, at least the style bot passed. :) > > Back to the drawing board! The drawing board was a good place to be. While working through a separate bug, I realized that we can really generate stack traces from wherever we like. This means that we can actually just generate the trace directly before logging a message: there's no reason to generate a stack trace _every time_ we check 'allowEval'. This also enables us to get stack traces from any JavaScript-driven CSP violation. Resource injection now traces back up to the JS code that does the injection, which is nice indeed. Let's hope the bots like it as much as I do. WDYT, Adam/Pavel?
Comment on attachment 160736 [details] Patch Attachment 160736 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13609426
Comment on attachment 160736 [details] Patch Attachment 160736 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13609434
Comment on attachment 160736 [details] Patch Attachment 160736 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13615255
Created attachment 160749 [details] JSC fix. :(
Cool!
Comment on attachment 160749 [details] JSC fix. :( Attachment 160749 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13612330 New failing tests: http/tests/inspector/csp-inline-warning-contains-stacktrace.html http/tests/inspector/csp-injected-content-warning-contains-stacktrace.html
Created attachment 160766 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
(In reply to comment #20) > (From update of attachment 160749 [details]) > Attachment 160749 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13612330 > > New failing tests: > http/tests/inspector/csp-inline-warning-contains-stacktrace.html > http/tests/inspector/csp-injected-content-warning-contains-stacktrace.html Hey Pavel, these tests are giving different column numbers for the inspector test code on the EWS bots than they are on my local machine. Is that something you've seen before? If the exact lines of the inspector-test/debugger-test are somehow flakey, should I adjust the test to exclude them?
(In reply to comment #22) > If the exact lines of the inspector-test/debugger-test are somehow flakey, should I adjust the test to exclude them? Actually, it looks like 'debugger-test.js' does exactly that: internal scripts are labeled as "(internal script)" with line number "(line number)". I'll adjust my tests accordingly.
Created attachment 160774 [details] Happy bots?
Comment on attachment 160774 [details] Happy bots? Happy bots! CQ? :)
Comment on attachment 160774 [details] Happy bots? oh, please manually replace "Reviewed by NOBODY (OOPS)" with "Reviewed by Adam Barth".
(In reply to comment #26) > (From update of attachment 160774 [details]) > oh, please manually replace "Reviewed by NOBODY (OOPS)" with "Reviewed by Adam Barth". Hrm. I thought the CQ took care of that? I'll add it in right now.. thanks for hopping on the CQ request so quickly! :)
(In reply to comment #27) > Hrm. I thought the CQ took care of that? The CQ takes care of it if you keep the r+ you obtained. Now you've cleared r+ you obtained, you need to manually replace it.
Created attachment 160905 [details] Patch
Comment on attachment 160905 [details] Patch ok
(In reply to comment #28) > (In reply to comment #27) > > Hrm. I thought the CQ took care of that? > > The CQ takes care of it if you keep the r+ you obtained. Now you've cleared r+ you obtained, you need to manually replace it. Understood. Thanks!
Comment on attachment 160905 [details] Patch Clearing flags on attachment: 160905 Committed r126852: <http://trac.webkit.org/changeset/126852>
All reviewed patches have been landed. Closing bug.
(In reply to comment #32) > (From update of attachment 160905 [details]) > Clearing flags on attachment: 160905 > > Committed r126852: <http://trac.webkit.org/changeset/126852> It broke many tests - https://bugs.webkit.org/show_bug.cgi?id=95176 Could you check it, please?
(In reply to comment #34) > It broke many tests - https://bugs.webkit.org/show_bug.cgi?id=95176 > Could you check it, please? Looks like Mike is not around now. Let me roll out the patch.
Reverted r126852 for reason: broke qt and mac tests Committed r126854: <http://trac.webkit.org/changeset/126854>
(In reply to comment #36) > Reverted r126852 for reason: > > broke qt and mac tests > > Committed r126854: <http://trac.webkit.org/changeset/126854> It looks like this is simply the output that JSC is generating; the results I here here on WK2/Lion have the "native code" stack traces, while the results I get for Chromium are in line with what this patch contains. I'll reset the baseline test to these new values, and override it with a platform specific Chromium test containing the old. I'd like Pavel to take a look at it before trying again though...
Created attachment 161022 [details] Chromium vs Everything Else.
(In reply to comment #38) > Created an attachment (id=161022) [details] > Chromium vs Everything Else. Hey Pavel: not a stunningly high-priority bug, but when you have some time, I'd appreciate you evaluating the V8 vs JSC test expectations in the attached patch. Thanks!
(In reply to comment #39) > (In reply to comment #38) > > Created an attachment (id=161022) [details] [details] > > Chromium vs Everything Else. > > Hey Pavel: not a stunningly high-priority bug, but when you have some time, I'd appreciate you evaluating the V8 vs JSC test expectations in the attached patch. > > Thanks! Friendly ping, Pavel. I know I said low priority, but I'd love to see this land in time for M23 if possible. Do you think you'll have time this week? Thanks!
Comment on attachment 161022 [details] Chromium vs Everything Else. (resetting the r? flag, I don't want to land this again without someone signing off. :) )
Mike, are you waiting for Pavel here, or can I help?
(In reply to comment #42) > Mike, are you waiting for Pavel here, or can I help? Hey, sorry, I'm on vacations. Please ping yurys@chromium.org or vsevik@chromium.org for assistance.
Thanks for the ping Adam, but I think your work here is done! It's just web inspector stuff from here on out. :) Thanks Pavel, sorry, I didn't realize you were out of office. Hi Vsevolod and Yury! Would one of you mind taking a look a this patch? I landed it a week or two ago, and had to roll it back because of differences between JSC and V8's generated stack traces. I'd appreciate you reviewing the platform-specific tests to make sure that the differences are expected, and not a result of something weird I've done.
Comment on attachment 161022 [details] Chromium vs Everything Else. View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review r=me > Source/WebCore/ChangeLog:48 > + stack trace just before logging it, and only pass it to Nice. > LayoutTests/http/tests/inspector/resources/csp-test.js:4 > + var messageCount = 0; You don't need this counter. Just drop the second argument in the call to InspectorTest.addConsoleSniffer above.
Comment on attachment 161022 [details] Chromium vs Everything Else. View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review > LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7 > + [native code]:1 As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding JSMainThreadExecState currentState(exec); to ScheduledAction::create. r- for this
(In reply to comment #46) > (From update of attachment 161022 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review > > > LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7 > > + [native code]:1 > > As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding > JSMainThreadExecState currentState(exec); > to ScheduledAction::create. > > r- for this On second thought, we cannot just put JSMainThreadExecState currentState(exec); into ScheduledAction::create as it wouldn't work for Workers. They also can call setTimeout/Interval which would lead to modification of JSMainThreadExecState::s_mainThreadState from wrong thread.
(In reply to comment #47) > (In reply to comment #46) > > (From update of attachment 161022 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review > > > > > LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7 > > > + [native code]:1 > > > > As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding > > JSMainThreadExecState currentState(exec); > > to ScheduledAction::create. > > > > r- for this > > On second thought, we cannot just put JSMainThreadExecState currentState(exec); into ScheduledAction::create as it wouldn't work for Workers. They also can call setTimeout/Interval which would lead to modification of JSMainThreadExecState::s_mainThreadState from wrong thread. Ok. I don't know nearly enough about the inner workings of JSC to determine what I ought to do here. If you have a concrete suggestion, I'm more than happy to implement it. :)
Created attachment 162738 [details] Rebased onto ToT.
(In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #46) > > > (From update of attachment 161022 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review > > > > > > > LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7 > > > > + [native code]:1 > > > > > > As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding > > > JSMainThreadExecState currentState(exec); > > > to ScheduledAction::create. > > > > > > r- for this > > > > On second thought, we cannot just put JSMainThreadExecState currentState(exec); into ScheduledAction::create as it wouldn't work for Workers. They also can call setTimeout/Interval which would lead to modification of JSMainThreadExecState::s_mainThreadState from wrong thread. > > Ok. > > I don't know nearly enough about the inner workings of JSC to determine what I ought to do here. If you have a concrete suggestion, I'm more than happy to implement it. :) I see the following options: 1) Capture stack trace before the CSP check. The problem is that we spend resources collecting stack trace even if it is not needed(presumably common case). 2) Replace JSMainThreadExecState with JSCurrentThreadExecState which would keep current exec state pointer in a thread local variable. This might cause some performance penalty. 3) Introduce ScriptCallStackBuilder that would be allocated on the C++ call stack and in case of JSC keep a pointer to the current exec state. It can later call createScriptCallStackForInspector(exec) with the saved ExecState. In case of V8 it doesn't need to store anything. Reference to the builder could be passed down to CSPDirectiveList::checkEvalAndReportViolation.
Created attachment 165756 [details] Patch
Hi Yury (and Vsevolod (and Pavel))! I've been avoiding this ticket for a few weeks because I didn't want to deal with the threading issue. I took another look at it this morning, and realized that we only have the threading problem because I'm trying to be clever and elegant about not passing any state into ContentSecurityPolicy. If I give up on that, and pass in the ExecState* we already have inside JSC::ScheduledAction::create, then I think we can avoid the issue entirely. Can you take a quick look at this approach? In particular, it involves adding a shim to V8's ScriptCallStackFactory to match the JSC interface, and bringing a bit more context into ContentSecurityPolicy::logToConsole. If you're alright with the approach, I'll rebaseline the tests and see what the bots think. Thanks!
Comment on attachment 165756 [details] Patch Attachment 165756 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14032412 New failing tests: http/tests/inspector/csp-inline-warning-contains-stacktrace.html http/tests/inspector/csp-injected-content-warning-contains-stacktrace.html
Created attachment 165847 [details] Patch
Comment on attachment 165847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165847&action=review This is fine, but we don't what to have JSC types in WebCore headers. That's the problem that ScriptState.h is supposed to solve for us. > Source/WebCore/page/ContentSecurityPolicy.h:44 > +#if USE(JSC) > +namespace JSC { > +class ExecState; > +} > +#endif This shouldn't be needed. > Source/WebCore/page/ContentSecurityPolicy.h:57 > +#if USE(JSC) > +typedef JSC::ExecState ScriptState; > +#else > +class ScriptState; > +#endif Why not just include ScriptState.h ?
(In reply to comment #55) > (From update of attachment 165847 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165847&action=review > > This is fine, but we don't what to have JSC types in WebCore headers. That's the problem that ScriptState.h is supposed to solve for us. This solution was copy/pasted from InspectorInstrumentation.h. I'll update this patch, and I'm happy to change that other file to use ScriptState.h in a separate patch, if you like.
Created attachment 165935 [details] Patch
(In reply to comment #56) > This solution was copy/pasted from InspectorInstrumentation.h. I'll update this patch, and I'm happy to change that other file to use ScriptState.h in a separate patch, if you like. I've updated the patch on this bug, and uploaded https://bugs.webkit.org/show_bug.cgi?id=97759 to address InspectorInstrumentation.h. WDYT?
Comment on attachment 165935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165935&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:1630 > + if (InspectorInstrumentation::hasFrontends()) { I gather from https://bugs.webkit.org/show_bug.cgi?id=96730 (thanks Adam!) that `hasFrontends` isn't what I should be checking here. I'm actually not sure if I should be checking for frontends at all. I think we probably want the stacktrace to be generated even if the Inspector isn't open... Pavel, et al? WDYT?
Comment on attachment 165935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165935&action=review >> Source/WebCore/page/ContentSecurityPolicy.cpp:1630 >> + if (InspectorInstrumentation::hasFrontends()) { > > I gather from https://bugs.webkit.org/show_bug.cgi?id=96730 (thanks Adam!) that `hasFrontends` isn't what I should be checking here. I'm actually not sure if I should be checking for frontends at all. I think we probably want the stacktrace to be generated even if the Inspector isn't open... > > Pavel, et al? WDYT? Having the stack traces generated even when there is no front-end is better from the web developer perspective but what if there is some function call violating CSP in a loop? This is probably a rare case and is likely an error anyway, but if we don't have precise information about that I'd rather put some check to be on the safe side. InspectorInstrumentation::hasFrontends check could be replaced with InspectorInstrumentation::consoleAgentEnabled() as the stack trace will be saved in a console message.
(In reply to comment #60) > Having the stack traces generated even when there is no front-end is better from the web developer perspective but what if there is some function call violating CSP in a loop? This is probably a rare case and is likely an error anyway, but if we don't have precise information about that I'd rather put some check to be on the safe side. Someone could maliciously generate code that generated errors in a loop, but it would be pretty obviously an error. Still... > InspectorInstrumentation::hasFrontends check could be replaced with InspectorInstrumentation::consoleAgentEnabled() as the stack trace will be saved in a console message. I'll throw this in. Thanks for your help!
Created attachment 166048 [details] Patch
Comment on attachment 166048 [details] Patch Attachment 166048 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14034932 New failing tests: http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html
Comment on attachment 166048 [details] Patch Attachment 166048 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14036931 New failing tests: http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html
Created attachment 166068 [details] Patch
(In reply to comment #65) > Created an attachment (id=166068) [details] > Patch Uploading a patch without a consoleAgentEnabled check. I'd really like to get a stack trace even if the console isn't open; asking a developer to reload the page after opening devtools is something I'd like to avoid if possible. I'll ping you tomorrow to chat about it, Yury. If I can't change your mind, then maybe you can help me fiddle with the inspector-enabled tests to make them work with that check. :)
> Uploading a patch without a consoleAgentEnabled check. I'd really like to get a stack trace even if the console isn't open; asking a developer to reload the page after opening devtools is something I'd like to avoid if possible. At this moment, we only capture traces for console messages when console is enabled. Otherwise it can affect page runtime and footprint. I'd recommend doing it consistently.
(In reply to comment #67) > > Uploading a patch without a consoleAgentEnabled check. I'd really like to get a stack trace even if the console isn't open; asking a developer to reload the page after opening devtools is something I'd like to avoid if possible. > > At this moment, we only capture traces for console messages when console is enabled. Otherwise it can affect page runtime and footprint. I'd recommend doing it consistently. Good timing, I was just about to hop on IRC to ask about this. :) Ok. I'll throw the check back in, and rewrite the http/tests/inspector-enabled/contentSecurityPolicy-* tests to ensure that the console is open. Thanks for the feedback.
Created attachment 166201 [details] Patch
Comment on attachment 166201 [details] Patch Attachment 166201 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14060382 New failing tests: http/tests/inspector/csp-setInterval-warning-contains-stacktrace.html http/tests/workers/terminate-during-sync-operation.html http/tests/inspector/csp-setTimeout-warning-contains-stacktrace.html
Created attachment 166278 [details] Patch
96730 got rolled out, so my patch failed. :( Inspector folks, do you expect to reland that soonish? Or should I rebase this onto ToT without that patch?
(In reply to comment #72) > 96730 got rolled out, so my patch failed. :( > > Inspector folks, do you expect to reland that soonish? Or should I rebase this onto ToT without that patch? I think Pavel will quickly fix this crash (seems like nothing special there), so I would wait.
(In reply to comment #73) > (In reply to comment #72) > > 96730 got rolled out, so my patch failed. :( > > > > Inspector folks, do you expect to reland that soonish? Or should I rebase this onto ToT without that patch? > > I think Pavel will quickly fix this crash (seems like nothing special there), so I would wait. Easy. Thanks! :)
Created attachment 166440 [details] Rebased.
(In reply to comment #75) > Created an attachment (id=166440) [details] > Rebased. Pavel relanded his change, and the bots are happy with mine. I'll wait until tomorrow to make sure the change sticks, and then land this.
Comment on attachment 166440 [details] Rebased. Clearing flags on attachment: 166440 Committed r130150: <http://trac.webkit.org/changeset/130150>