Bug 58646

Summary: CSP policy violations should log to the console
Product: WebKit Reporter: Adam Barth <abarth>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 53572    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Archive of layout-test-results from eseidel-cq-sf
none
Patch for landing none

Description Adam Barth 2011-04-15 00:29:08 PDT
CSP policy violations should log to the console
Comment 1 Adam Barth 2011-04-19 23:37:37 PDT
Created attachment 90312 [details]
Patch
Comment 2 Adam Barth 2011-04-19 23:44:20 PDT
Comment on attachment 90312 [details]
Patch

These tests are flaky as written (order dependent).  Will fix.
Comment 3 WebKit Review Bot 2011-04-19 23:55:38 PDT
Attachment 90312 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8473567
Comment 4 Adam Barth 2011-04-20 00:09:06 PDT
Created attachment 90318 [details]
Patch
Comment 5 WebKit Review Bot 2011-04-20 00:18:28 PDT
Attachment 90312 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8473573
Comment 6 Eric Seidel (no email) 2011-04-20 12:09:16 PDT
Comment on attachment 90318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90318&action=review

Looks fine.  The only nit which really matters is the repeating of all that logging code.

> Source/WebCore/page/ContentSecurityPolicy.cpp:492
> +    if (!allowed) {

I might have reversed these ifs to flatten these blocks:
if (allowed)
    return allowed; (or simply true;)

But this is also OK.

> Source/WebCore/page/ContentSecurityPolicy.cpp:495
> +        if (Frame* frame = m_document->frame())
> +            frame->domWindow()->console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, consoleMessage, 1, String());

Seems like these two lines want to be a helper.  We repeat them 7 times. :)

> Source/WebCore/page/ContentSecurityPolicy.cpp:702
> +        m_scriptSrc = adoptPtr(new CSPDirective(value, m_document->securityOrigin()));

I would have added an origin() or securityOrigin() private method.  BUt this is OK.
Comment 7 Adam Barth 2011-04-20 13:24:33 PDT
Created attachment 90397 [details]
Patch for landing
Comment 8 Adam Barth 2011-04-20 13:26:20 PDT
> I would have added an origin() or securityOrigin() private method.  BUt this is OK.

I did everything except this last one.
Comment 9 WebKit Commit Bot 2011-04-20 15:12:12 PDT
Comment on attachment 90397 [details]
Patch for landing

Rejecting attachment 90397 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
mlmp .
http/tests/xmlhttprequest ................................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ............
747.48s total testing time

23330 test cases (99%) succeeded
1 test case (<1%) was new
15 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8468863
Comment 10 WebKit Commit Bot 2011-04-20 15:12:15 PDT
Created attachment 90425 [details]
Archive of layout-test-results from eseidel-cq-sf

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: eseidel-cq-sf  Port: Mac  Platform: Mac OS X 10.6.4
Comment 11 Adam Barth 2011-04-20 18:52:50 PDT
Created attachment 90473 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2011-04-20 19:34:12 PDT
Comment on attachment 90473 [details]
Patch for landing

Clearing flags on attachment: 90473

Committed r84457: <http://trac.webkit.org/changeset/84457>
Comment 13 WebKit Commit Bot 2011-04-20 19:34:19 PDT
All reviewed patches have been landed.  Closing bug.