Bug 86323

Created attachment 141612 [details] Patch

Comment on attachment 141612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141612&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:661 > + const String verb = type == "connect" ? ("connect to") : ("load the"); Should just be String rather than const String. Also no need for those parentheses; it’s not a conventional style for something like this. Maybe there’s another way to do this without constructing this extra string that we have to destroy, but I can’t think of anything offhand. > Source/WebCore/page/ContentSecurityPolicy.cpp:662 > + reportViolation(directive->text(), "Refused to " + verb + " " + type + " '" + url.string() + "' as it violates the following Content Security Policy directive: \"" + directive->text() + "\".\n", url); I’m not too happy with the phrase “refused to x as it violates y”; the word “as” there does not seem right to me. I think the word “because” is far clearer. Or maybe “since”.

Comment on attachment 141612 [details] Patch Attachment 141612 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12682305 New failing tests: http/tests/security/contentSecurityPolicy/javascript-url-blocked.html http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html http/tests/security/sandboxed-iframe-origin-add.html http/tests/security/contentSecurityPolicy/inline-style-attribute-blocked.html http/tests/security/contentSecurityPolicy/image-blocked.html http/tests/security/contentSecurityPolicy/directive-parsing-03.html http/tests/security/contentSecurityPolicy/inline-script-blocked.html http/tests/security/contentSecurityPolicy/inline-script-blocked-goofy.html http/tests/security/contentSecurityPolicy/eval-scripts-setInterval-blocked.html fast/canvas/webgl/shader-precision-format.html http/tests/security/contentSecurityPolicy/inline-script-blocked-javascript-url.html http/tests/security/contentSecurityPolicy/directive-parsing-01.html http/tests/security/contentSecurityPolicy/block-mixed-content-hides-warning.html http/tests/security/contentSecurityPolicy/inline-style-blocked.html http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html http/tests/security/contentSecurityPolicy/frame-src-blocked.html http/tests/security/contentSecurityPolicy/combine-multiple-policies.html http/tests/security/contentSecurityPolicy/directive-parsing-02.html http/tests/security/contentSecurityPolicy/object-src-no-url-blocked.html http/tests/security/contentSecurityPolicy/default-src-inline-blocked.html http/tests/security/contentSecurityPolicy/media-src-blocked.html http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html http/tests/security/contentSecurityPolicy/eval-scripts-setTimeout-blocked.html http/tests/inspector/resource-har-pages.html

Created attachment 141635 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

@Mike: You'll probably need to update the expected results for many of the CSP tests because they contain the console message in the output. You can do that using the --reset-results flag for run-webkit-tests. (Note: You can also tell run-webkit-tests to just run the tests in the LayoutTests/http/tests/security/contentSecurityPolicy directory.)

Created attachment 141643 [details] Patch

Thanks Darin and Adam. The new patch changes "as" to "because", drops `const` and parens, and resets the tests (`--result-results` is a life-saver... I've been doing those edits by hand!).

Comment on attachment 141643 [details] Patch Attachment 141643 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12685237 New failing tests: http/tests/security/contentSecurityPolicy/source-list-parsing.html media/csp-blocks-video.html

Created attachment 141649 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 141655 [details] Patch

Missed a test: the latest patch updates `media/csp-blocks-video.html`. `http/tests/security/contentSecurityPolicy/source-list-parsing.html` looks like it's loading the iframes in a different order than expected. The results are all there, and correct so far as I can tell, but mixed up. Adam, any ideas about how I could address that?

Comment on attachment 141655 [details] Patch Attachment 141655 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12679432 New failing tests: http/tests/security/contentSecurityPolicy/source-list-parsing.html media/csp-blocks-video.html

Created attachment 141661 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 141668 [details] Patch

Comment on attachment 141668 [details] Patch Attachment 141668 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12682393 New failing tests: http/tests/security/contentSecurityPolicy/source-list-parsing.html

Created attachment 141696 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

> `http/tests/security/contentSecurityPolicy/source-list-parsing.html` looks like it's loading the iframes in a different order than expected. The results are all there, and correct so far as I can tell, but mixed up. Adam, any ideas about how I could address that? Is the test non-deterministic? If so, we might need to improve the test to eliminate any race conditions. It looks like the test is slightly flaky: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Fsource-list-parsing.html However, your patch seems to cause it to fail more consistently.

(In reply to comment #17) > Is the test non-deterministic? If so, we might need to improve the test to eliminate any race conditions. It looks like the test is slightly flaky: It's loading 18 iframes, and they don't all seem to load in the same order every time it's run. I'm not entirely sure how we can ensure that they load in the correct order... Chaining `document.write` calls via `onload` events seems like a bad solution. Splitting this up into 18 tests would certainly solve the problem, but seems like overkill. Are there other tests of this sort that I could take a look at for ideas?

> It's loading 18 iframes, and they don't all seem to load in the same order every time it's run. I'm not entirely sure how we can ensure that they load in the correct order... Chaining `document.write` calls via `onload` events seems like a bad solution. I bet these all used to produce the same output but now they produce different output. Previously, it didn't matter what order they ran in, but now it does. > Splitting this up into 18 tests would certainly solve the problem, but seems like overkill. Yeah, we either need to serialize the order using onload and appendChild or shard the test. We probably should do a little of both. Maybe four per test?

Created attachment 141783 [details] Patch

(In reply to comment #19) > Yeah, we either need to serialize the order using onload and appendChild or shard the test. We probably should do a little of both. Maybe four per test? I've taken a stab at this, splitting it into 4 tests: two with 5, two with 4. This seems to work locally, let's see if the bots are happy.

Comment on attachment 141783 [details] Patch Clearing flags on attachment: 141783 Committed r117006: <http://trac.webkit.org/changeset/117006>

All reviewed patches have been landed. Closing bug.

(In reply to comment #22) > (From update of attachment 141783 [details]) > Clearing flags on attachment: 141783 > > Committed r117006: <http://trac.webkit.org/changeset/117006> After this, script-src-redirect.html has been really flaky. I wonder if we should roll it out.

> After this, script-src-redirect.html has been really flaky. > I wonder if we should roll it out. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=script-src-redirect.html

Looks like `script-src-redirect.html` is another iframe ordering test. I'll rewrite it to use the same methodology as `source-list-parsing.html`: https://bugs.webkit.org/show_bug.cgi?id=86433

It looks like the the global expectation was not updated for the following test: http/tests/security/contentSecurityPolicy/media-src-blocked.html You only updated the Chromium expectation.

Similarly, the expectation for the following test needs to be updated: http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html

Created attachment 141858 [details] Additional patch to update expectations Update the expectations for: http/tests/security/contentSecurityPolicy/media-src-blocked.html http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html

Reopening the bug until all expectations are updated.

Comment on attachment 141858 [details] Additional patch to update expectations Rejecting attachment 141858 [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: ecurityPolicy/media-src-blocked-expected.txt patching file LayoutTests/http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked-expected.txt Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked-expected.txt.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12684697

Created attachment 141863 [details] Patch Could someone please cq+?

Comment on attachment 141863 [details] Patch Clearing flags on attachment: 141863 Committed r117035: <http://trac.webkit.org/changeset/117035>

Expectations were updated in May. Closing. :)