Bug 1010953 indicates that there is no indicator in the console message that a CSP violation is just REPORT-ONLY. We should fix that for the new CSP implementation.

Bug 1010953 introduced different console messages for 'report-only' and 'regular' CSP violations. The attached patch introduces the same separation for the new CSP implementation. Keep in mind, that the test (introduced in bug 1013507) still does not pass, because the old implementation also prints the default port when reporting a violated url directive, whereas the new implementation only prints the port in case it's not the default port (which makes more sense in my opinion). I updated the test in the patch_queue, which we can take care of once the test splitting patch (bug 988616) landed.

Comment on attachment 8428395 [details] [diff] [review] bug_1011058.patch Review of attachment 8428395 [details] [diff] [review]: ----------------------------------------------------------------- As much as I don't like bloating the constructor with yet more arguments, I don't immediately see a better appropriate way without passing around references to the nsCSPPolicy instance (yuck). This needs test coverage before landing (which means, we'll want to update this patch with any required test changes once bug 988616 lands). While this all looks good (and r=me on these parts), I'm clearing the review flag because I want to see the patch updated to include test coverage before r+ing it. Please re-flag me once you incorporate test coverage. ::: content/base/src/nsCSPContext.cpp @@ +890,5 @@ > + > + const char16_t* msg = mReportOnlyFlag ? > + NS_LITERAL_STRING("CSPROViolationWithURI").get() : > + NS_LITERAL_STRING("CSPViolationWithURI").get(); > + CSP_LogLocalizedStr(msg, params, ArrayLength(params), since msg is only used once, I think you can just move the ternary operator statement into the args list below. @@ +947,5 @@ > const nsAString& aScriptSample, > uint32_t aLineNum) > { > + NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1); > + bool reportOnlyFlag = mPolicies[aViolatedPolicyIndex]->getReportOnlyFlag(); Do we need to declare reportOnlyFlag, or can we just put the call to getReportOnlyFlag into the args list of the constructor below? @@ +953,4 @@ > NS_DispatchToMainThread(new CSPReportSenderRunnable(aBlockedContentSource, > aOriginalURI, > aViolatedPolicyIndex, > + reportOnlyFlag, nit: prefix with "a".

Actually, it looks like the browser console tests will catch this: browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js I caught this in bug 949533 comment 6, so this might be enough coverage. Chris, if you think this is enough test coverage, fix my minors and flag me again for review.

Changes in bug 949533 includes updating the test which we are relying on for this patch to work. Adding dependcy so we can finish this bug as soon as 949533 lands.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > Changes in bug 949533 includes updating the test which we are relying on for > this patch to work. Adding dependcy so we can finish this bug as soon as > 949533 lands. Oh, I forgot to mention that I do think that the devtools test provides enough coverage in terms of testing.

Addressed Sid's review comments. Once again, the test for this change landed in bug 1013507. (In reply to Sid Stamm [:geekboy or :sstamm] from comment #2) > @@ +953,4 @@ > > NS_DispatchToMainThread(new CSPReportSenderRunnable(aBlockedContentSource, > > aOriginalURI, > > aViolatedPolicyIndex, > > + reportOnlyFlag, > > nit: prefix with "a". reportOnlyFlag was actually a local variable, hence no "a" as prefix :-) Anyway, the new version does not use the local variable any longer.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6) > reportOnlyFlag was actually a local variable, hence no "a" as prefix :-) > Anyway, the new version does not use the local variable any longer. obviously. *facepalm*

Comment on attachment 8444821 [details] [diff] [review] bug_1011058_v2.patch Review of attachment 8444821 [details] [diff] [review]: ----------------------------------------------------------------- Yep, looks good.

Adding sstamm as a reviewer and carrying over r+.

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=824b5446963b