Add assertions that the empty Platform::cryptographicallyRandomValues() overrides are not being use…

components/test_runner/test_common.cc

Index: components/test_runner/test_common.cc
diff --git a/components/test_runner/test_common.cc b/components/test_runner/test_common.cc
index ad06d5233e5a14889e49ea549d2c95eb1474d0ad..7f3081a0f2c05d66aa432ba339aa74c016c6a1cf 100644
--- a/components/test_runner/test_common.cc
+++ b/components/test_runner/test_common.cc
@@ -5,6 +5,7 @@
 #include "components/test_runner/test_common.h"
 
 #include "base/lazy_instance.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "third_party/WebKit/public/platform/Platform.h"
 #include "third_party/WebKit/public/web/WebKit.h"
@@ -30,7 +31,10 @@ class MockBlinkPlatform : NON_EXPORTED_BASE(public blink::Platform) {
   }
   ~MockBlinkPlatform() override {}
   void cryptographicallyRandomValues(unsigned char* buffer,
-                                     size_t length) override {}
+                                     size_t length) override {
+    NOTREACHED();

[jww, 2015/11/10 18:43:16]

Why isn't this an ASSERT_NOT_REACHED()? Wouldn't we be really sad if this was
reached in a Release build, to the point that all security guarantees are lost?

[eroman, 2015/11/10 19:26:14]

 (1) ASSERT_NOT_REACHED() is compiled out of Release builds, so if we wanted to
test in release I believe it would need to be "RELEASE_ASSERT_NOT_REACHED". I am
happy to change the existing ones to RELEASE_ASSERT_NOT_REACHED() if you think
that is good, I was just matching the existing pattern.

 (2) In this case since it is Chromium code, I wasn't sure if it was legit to
include files from wtf/, since the rest of this file is just using
WebKit/public. For that reason I used the Chromium equivalent "NOTREACHED()".

[jww, 2015/11/10 19:32:26]

Ah, got it! Good call all around.

(1) Yes, I think all of those call sites should probably be
RELEASE_ASSERT_NOT_REACHED(). It seems pretty critical to me that these should
not be reached in a Release build.

(2) Great call. You're right that it shouldn't use wtf/. I'm not sure if there's
a Release equivalent of NOTREACHED, so maybe just CHECK(false)?

[eroman, 2015/11/10 19:45:56]

Done:

 * I changed to RELEASE_ASSERT_NOT_REACHED() everywhere. I agree this is better
and will guard against any future copy-pastes.

 * In the case of the Chromium code, rather than adding a CHECK(false) I opted
to instead just implement with a call to base::RandBytes(). (Note that
base::RandBytes() is in fact a cryptographically secure implementation despite
the comments in header. Following up on this ambiguity in
https://code.google.com/p/chromium/issues/detail?id=553857).

+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(MockBlinkPlatform);
 };