Catch CSP violations in InProcessBrowserTest

chrome/test/base/in_process_browser_test.cc

Index: chrome/test/base/in_process_browser_test.cc
diff --git a/chrome/test/base/in_process_browser_test.cc b/chrome/test/base/in_process_browser_test.cc
index 48b71dd2b46205f95d462440462f7ecd2ad3116c..0a73eb8df2cd8522b44d102375429d9e54afce43 100644
--- a/chrome/test/base/in_process_browser_test.cc
+++ b/chrome/test/base/in_process_browser_test.cc
@@ -91,6 +91,26 @@ namespace {
 // Passed as value of kTestType.
 const char kBrowserTestType[] = "browser";
 

[Tom Sepez, 2016/05/31 16:21:38]

nit: |static| redundant within namespace {}

[wychen, 2016/06/08 05:41:57]

Done.

+static bool seen_csp_error_messages_ = false;
+
+bool CSPLogHandler(int severity,

[Dan Beam, 2016/06/07 01:18:30]

this will conflict (potentially) with JS-based browser tests as they also
globally override log handling :(

[wychen, 2016/06/08 05:41:57]

I've rebased this CL on top of https://codereview.chromium.org/2034393004/,
which turns log message handler from a pointer to a stack.

+                   const char* file,
+                   int line,
+                   size_t message_start,
+                   const std::string& str) {
+  if (file == NULL || std::string("CONSOLE") != file)
+    return false;
+
+  // Catch CSP violation.
+  bool contains_error =
+      str.find("Content Security Policy") != std::string::npos;

[Paweł Hajdan Jr., 2016/05/30 08:43:47]

It seems fragile to me to rely on string parsing and a log handler to catch CSP
violations.

What happens if something else registers its own log handler in tests?

Is there rather e.g. some observer to register for notifications?

[wychen, 2016/05/31 08:11:27]

It's indeed not very robust in that case. Even though all CSP violation messages
contain that string now, there's no guarantee it will remain this case.

mkwst@, would it make sense to add notification in functions like
CSPDirectiveList::reportViolation*() so that violations are guaranteed to be
caught in browsertest level?

[Mike West, 2016/05/31 09:20:00]

What would you like to see added? Sorry, I'm not entirely following the thrust
of this CL. :)

[Tom Sepez, 2016/05/31 16:21:38]

I'm OK so long as we don't silently pass failures as the result of a change. As
it is now, we'll silently pass them.  Which is why I think the
SetExpectCSPErrorMessges() change is required to induce failures when the string
changes and the expected substrings are no longer found.

[wychen, 2016/05/31 18:17:45]

This sounds like a good median between current fragile implementation and the
heavyweight notification. This could detect changed messages, but if a test
registers its own log handler while violating CSP, it still silently passes. To
solve this hijacking issue, we can turn the log message handler to a stack, so
that derived test classes can still add/remove its own handler, while the old
ones still get the messages. I'm just thinking out loud. Would this worth the
trouble? All the callers of GetLogMessageHandler() seems to be compatible with
the stack usage.

Mike, what I was trying to do is to catch all unexpected CSP violations in
browsertest, especially for WebUI-related tests. Currently all the CSP
violations that we care in WebUI generate console messages containing "Content
Security Policy", hence this quick-n-dirty detection method.

+  if (severity == logging::LOG_INFO && contains_error) {
+    seen_csp_error_messages_ = true;
+  }
+
+  return false;
+}
+
 }  // namespace
 
 // Library used for testing accessibility.
@@ -135,6 +155,7 @@ InProcessBrowserTest::InProcessBrowserTest()
     : browser_(NULL),
       exit_when_last_browser_closes_(true),
       open_about_blank_on_browser_launch_(true),
+      ignore_csp_messages_(false),
       run_accessibility_checks_for_test_case_(false)
 #if defined(OS_MACOSX)
       , autorelease_pool_(NULL)
@@ -170,9 +191,13 @@ InProcessBrowserTest::InProcessBrowserTest()
 #if defined(USE_ASH)
   DefaultAshEventGeneratorDelegate::GetInstance();
 #endif
+
+  logging::SetLogMessageHandler(&CSPLogHandler);
+  seen_csp_error_messages_ = false;
 }
 
 InProcessBrowserTest::~InProcessBrowserTest() {
+  logging::SetLogMessageHandler(nullptr);
 }
 
 void InProcessBrowserTest::SetUp() {
@@ -344,6 +369,9 @@ void InProcessBrowserTest::TearDown() {
 #if defined(OS_WIN)
   com_initializer_.reset();
 #endif
+  if (!ignore_csp_messages_) {
+    EXPECT_FALSE(seen_csp_error_messages_);
+  }
   BrowserTestBase::TearDown();
 }
 
@@ -404,6 +432,10 @@ bool InProcessBrowserTest::SetUpUserDataDirectory() {
   return true;
 }
 
+void InProcessBrowserTest::SetIgnoreCSPErrorMessages(bool ignore) {
+  ignore_csp_messages_ = ignore;
+}
+
 #if !defined(OS_MACOSX)
 void InProcessBrowserTest::OpenDevToolsWindow(
     content::WebContents* web_contents) {