Right now, blocking inline scripts with CSP results in an unhelpful "line 1". We can do better. pfeldman suggests: - Introduce WebCore::InspectorInstrumentation::scriptBlockedByCSP() and call it from where appropriate - Wire it to InspectorDebuggerAgent::scriptBlockedByCSP() - Latter should call InspectorDebuggerAgent::breakProgram() in case "Break on exceptions" is engaged. It will look like it stopped on exception (with JS stack trace and such). - If there are call sites in V8, we would need to go through the bindings
Hey Pavel, I'll have a patch for you in just a bit. Thanks for the detailed description. :) Would you prefer that the tests for this feature live with the other debugger tests, or with the other CSP tests?
> Would you prefer that the tests for this feature live with the other debugger tests, or with the other CSP tests? I'd guess with the other debugger test since there's a bunch of infrastructure along with those tests.
(In reply to comment #2) > > Would you prefer that the tests for this feature live with the other debugger tests, or with the other CSP tests? > > I'd guess with the other debugger test since there's a bunch of infrastructure along with those tests. Thanks! How are you possibly awake? :) I'll throw it in LayoutTests/inspector/debugger for now; can always move it later if it becomes unwieldy.
Created attachment 158271 [details] Patch
Comment on attachment 158271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158271&action=review Overall looks good. > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:582 > + breakProgram(InspectorFrontend::Debugger::Reason::Exception, desc.release()); Reason::Exception should have exception in the aux data, while you only pass plain text (see ScopeChainSidebarPane.js:83). I think you should create a new reason "CSPViolation" and pass 0 as the aux data string (it does not add much info). You would need to change ScriptsPanel around line 288 to format it properly as well. > Source/WebCore/inspector/InspectorDebuggerAgent.h:117 > + virtual void scriptExecutionBlockedByCSP(); While you are here, could you move schedulePauseOnNextStatement, cancelPauseOnNextStatement, breakProgram to above this line as well? They are a part of the instrumentation API, but squeezed into the middle of the protocol methods. > Source/WebCore/page/ContentSecurityPolicy.cpp:598 > + bool checkInlineAndReportViolation(CSPDirective*, const String& consoleMessage, const String& contextURL, const WTF::OrdinalNumber& contextLine, bool isInlineScript = true) const; checkInline receiving isInlineScript confuses me. Leaving this up to abarth@.
Comment on attachment 158271 [details] Patch Thanks Pavel! I'll probably have a new patch up for you tomorrow. View in context: https://bugs.webkit.org/attachment.cgi?id=158271&action=review >> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:582 >> + breakProgram(InspectorFrontend::Debugger::Reason::Exception, desc.release()); > > Reason::Exception should have exception in the aux data, while you only pass plain text (see ScopeChainSidebarPane.js:83). I think you should create a new reason "CSPViolation" and pass 0 as the aux data string (it does not add much info). You would need to change ScriptsPanel around line 288 to format it properly as well. How about passing in the directive text as the auxData string, which would enable a more informative status message: "Paused on a script blocked by Content Security Policy directive 'XXX'"? >> Source/WebCore/page/ContentSecurityPolicy.cpp:598 >> + bool checkInlineAndReportViolation(CSPDirective*, const String& consoleMessage, const String& contextURL, const WTF::OrdinalNumber& contextLine, bool isInlineScript = true) const; > > checkInline receiving isInlineScript confuses me. Leaving this up to abarth@. This was poorly named. Changing it to 'isScript'. We use 'checkInlineAndReportViolation' for both inline script and inline style; I need a mechanism to distinguish between the two, and it seemed cleaner to do it with (yet another) parameter rather than adding the inspector notification to each of the higher-level methods ('CSPDirectiveList::allowInlineScript', for instance).
> How about passing in the directive text as the auxData string, which would enable a more informative status message: "Paused on a script blocked by Content Security Policy directive 'XXX'"? Sounds good! You've seen the place you can format it in the front-end, right? > This was poorly named. Changing it to 'isScript'. We use 'checkInlineAndReportViolation' for both inline script and inline style; I need a mechanism to distinguish between the two, and it seemed cleaner to do it with (yet another) parameter rather than adding the inspector notification to each of the higher-level methods ('CSPDirectiveList::allowInlineScript', for instance). Sounds good.
Created attachment 158773 [details] Patch
(In reply to comment #8) > Created an attachment (id=158773) [details] > Patch This should address your feedback, Pavel. I'm pretty sure I added the new CSPViolation to all the right places, but I'd appreciate you double-checking that. :) WDYT?
Created attachment 158774 [details] resetting tests.
Comment on attachment 158774 [details] resetting tests. View in context: https://bugs.webkit.org/attachment.cgi?id=158774&action=review > Source/WebCore/inspector/Inspector.json:2817 > + { "name": "reason", "type": "string", "enum": [ "XHR", "DOM", "EventListener", "exception", "CSPViolation", "other" ], "description": "Pause reason." }, Oh, so that string was a part of the enum. Now we are violating the protocol a tiny bit. I'll think of removing this enum declration. > Source/WebCore/inspector/front-end/ScriptsPanel.js:306 > + this.sidebarPanes.callstack.setStatus(WebInspector.UIString("Paused on a script blocked due to Content Security Policy directive: \"%s\".", details.auxData.directiveText)); Please add this to the English.lproj/localizedStrings.js and you are good to go.
Created attachment 158781 [details] Patch
Comment on attachment 158781 [details] Patch Attachment 158781 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13516365 New failing tests: inspector/debugger/debugger-pause-on-blocked-script-url.html inspector/debugger/debugger-pause-on-blocked-script-injection.html inspector/debugger/debugger-pause-on-blocked-event-handler.html
Created attachment 158791 [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
These tests pass for me locally (chromium port, linux). The only difference I see between "resetting tests." patch (which passed) and the "Patch" test (which failed) is the localized text. Did I do something wrong adding it to localizedStrings.js? Does it need to be registered somewhere?
(In reply to comment #15) > These tests pass for me locally (chromium port, linux). The only difference I see between "resetting tests." patch (which passed) and the "Patch" test (which failed) is the localized text. > > Did I do something wrong adding it to localizedStrings.js? Does it need to be registered somewhere? It looks more like your tests are flaky - entire line is missing, not just the localized piece.
True. In that case, I'm a bit stuck. I'm running them in a loop locally, and they seem stable (no failures yet, about halfway through running them 100 times each). Do you know of any case in which setting a sniffer on `setStatus` would flake? I'll rework the tests so that `setStatus` callback has to be called in order to finish... Perhaps they're just getting called after the test has exited due to interesting async behaviors?
(In reply to comment #17) > True. In that case, I'm a bit stuck. I'm running them in a loop locally, and they seem stable (no failures yet, about halfway through running them 100 times each). > > Do you know of any case in which setting a sniffer on `setStatus` would flake? > > I'll rework the tests so that `setStatus` callback has to be called in order to finish... Perhaps they're just getting called after the test has exited due to interesting async behaviors? Yeah, it is a layered async cake as you probably noticed...
Created attachment 158809 [details] Let's try this.
(In reply to comment #18) > (In reply to comment #17) > > True. In that case, I'm a bit stuck. I'm running them in a loop locally, and they seem stable (no failures yet, about halfway through running them 100 times each). > > > > Do you know of any case in which setting a sniffer on `setStatus` would flake? > > > > I'll rework the tests so that `setStatus` callback has to be called in order to finish... Perhaps they're just getting called after the test has exited due to interesting async behaviors? > > Yeah, it is a layered async cake as you probably noticed... Hrm. The current patch ensures that `setStatus` must be called in order to complete. Since it's timing out, I think something else is going wrong... it's not just an async issue. I'm sure there's something I'm doing wrong, but I don't really know where to start looking to figure out what the bot is doing. :(
> Since it's timing out, I think something else is going wrong... it's not just an async issue. I'm sure there's something I'm doing wrong, but I don't really know where to start looking to figure out what the bot is doing. :( I wfh today, so am only hacking the front-end. I can apply it tomorrow and see what is wrong.
(In reply to comment #21) > > Since it's timing out, I think something else is going wrong... it's not just an async issue. I'm sure there's something I'm doing wrong, but I don't really know where to start looking to figure out what the bot is doing. :( > > I wfh today, so am only hacking the front-end. I can apply it tomorrow and see what is wrong. No worries, and no rush. No one's waiting on this but me. :) Thanks!
Comment on attachment 158809 [details] Let's try this. Attachment 158809 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13514399 New failing tests: inspector/debugger/debugger-pause-on-blocked-script-url.html inspector/debugger/debugger-pause-on-blocked-script-injection.html inspector/debugger/debugger-pause-on-blocked-event-handler.html
Created attachment 158820 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 159404 [details] Patch
Comment on attachment 159404 [details] Patch Rejecting attachment 159404 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 50>At revision 134927. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13538681
Created attachment 159429 [details] Patch
(In reply to comment #26) > (From update of attachment 159404 [details]) > Rejecting attachment 159404 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 > > Last 500 characters of output: > Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > 50>At revision 134927. > > ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > > ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > Updating webkit projects from gyp files... > > Full output: http://queues.webkit.org/results/13538681 "Merge conflict during commit: Conflict at '/trunk/LayoutTests/ChangeLog' at /usr/lib/git-core/git-svn line 570" Hilarious. Rebased onto an even tippier ToT. Want to try again? :)
Comment on attachment 159429 [details] Patch Attachment 159429 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13543372 New failing tests: inspector/debugger/debugger-pause-on-blocked-script-url.html inspector/debugger/debugger-pause-on-blocked-script-injection.html inspector/debugger/debugger-pause-on-blocked-event-handler.html
Created attachment 159440 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 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 #29) > (From update of attachment 159429 [details]) > Attachment 159429 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13543372 > > New failing tests: > inspector/debugger/debugger-pause-on-blocked-script-url.html > inspector/debugger/debugger-pause-on-blocked-script-injection.html > inspector/debugger/debugger-pause-on-blocked-event-handler.html I didn't touch anything other than the ChangeLog. :( Bah. I'll look at the diff between the diffs in a few minutes.
Heh. I've seen it somewhere :) So it did not work. It sounds like setStatus is not consistently called. I'll need to investigate :(
(In reply to comment #32) > Heh. I've seen it somewhere :) So it did not work. It sounds like setStatus is not consistently called. I'll need to investigate :( I haven't had time to dig into this flake, and it's certainly not high-prio enough for you to spend time on it, Pavel. Would you mind if I just dropped the setStatus check in favor of a FIXME comment in the test, and landed the core feature without it? Users will still get a nice experience if "break on exceptions" is on. If that sounds alright to you, I'll rebase the patch onto trunk.
Sure, lets do that. Please file a bug on that and mention it in the FIXME.
Created attachment 164293 [details] Patch
(In reply to comment #35) > Created an attachment (id=164293) [details] > Patch Mostly a clean rebase. Let's see what the bots say.
Comment on attachment 164293 [details] Patch Bots are happy. CQ?
Comment on attachment 164293 [details] Patch Clearing flags on attachment: 164293 Committed r128703: <http://trac.webkit.org/changeset/128703>
All reviewed patches have been landed. Closing bug.