PreloadCheck class for extension pre-install checks

extensions/browser/preload_check_test_util.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/browser/preload_check_test_util.h"
+
+#include "base/logging.h"
+#include "base/run_loop.h"
+
+namespace extensions {
+
+PreloadCheckObserver::PreloadCheckObserver() : called_(false) {}
+PreloadCheckObserver::~PreloadCheckObserver() {}
+
+void PreloadCheckObserver::OnCheckComplete(PreloadCheck::Errors errors) {
+  CHECK(!called_);

[Devlin, 2017/03/14 01:44:34]

Can we ASSERT_FALSE here?

[michaelpg, 2017/03/14 21:58:30]

I'd prefer not to #include gtest.h here. I'd rather leave the assertions to the
test.

[Devlin, 2017/03/16 01:42:44]

Any particular reason?  It's already a test-only file.  (The reason I prefer
ASSERT here is because CHECK kills the process quite rudely, slowing down
subsequent tests, whereas ASSERT gracefully ends the test.)

[michaelpg, 2017/03/17 02:34:26]

Meh. The crash does look uglier, so Done.

+  called_ = true;
+  errors_ = errors;
+}
+
+void PreloadCheckObserver::Wait() {
+  if (called_)
+    return;
+
+  base::RunLoop().RunUntilIdle();

[Devlin, 2017/03/14 01:44:34]

This can be pretty finicky, given post task complexities.  Often what works
better is a model where we have a run_loop_ member, and then do:
void PreloadCheckObserver::OnCheckComplete(...) {
  run_loop_.Quit();
  ...
}

void PreloadCheckObserver::Wait() {
  run_loop_.Run();
}

(RunLoop::Run() also has the nice property of returning if Quit() has already
been called.)

[michaelpg, 2017/03/14 21:58:30]

Can you expand on "finicky", please? I copied this from some existing code, so
I'll admit I don't know the ins and outs of MessageLoops. But run_loop.h itself
suggests: "Create a RunLoop on the stack and call Run/Quit to run a nested
MessageLoop."

I believe that's what this is doing. It seems clear to me that Wait() blocks
until posted tasks have finished, at which point I can verify whether
OnCheckComplete()  was called.

Having a RunLoop member is less clear to me: the loop will block until
OnCheckComplete is recursively called to quit the loop? What happens if
OnCheckComplete is never called?

Take a look at BlacklistCheckTest::StopCheck -- renamed to ResetCheck in new
patch. This never terminates if Wait() calls run_loop_.Run(), presumably because
OnCheckComplete is never called so the loop is neither explicitly Quit(), nor
configured to quit when idle.

[Devlin, 2017/03/16 01:42:44]

The finicky bits are when RunUntilIdle() doesn't actually run long enough (i.e.,
until whatever you want to happen, has happened).  This won't commonly happen on
the main thread, but if you have multiple threads in play (e.g. reading
something from the file system), the main thread could be idle before the file
system read is complete.  There's also utilities for that
(content::RunAllBlockingPoolTasksUntilIdle or something like that), but even
that has subtleties.  And, as we get farther out, none of those would account
for if what you're waiting for is dependent on another process (such as waiting
for an extension to act in the renderer and notify the browser).

In most cases like this, what you really want to do is say "Run until [x]
happens", rather than "run the tasks you immediately know about in the message
loop" (though there are some exceptions).

So, that's the finicky bit.  But I forgot about the StopCheck() case, which is
indeed a pain.  There are ways around it... but none pretty.

I'm okay with this for now, for lack of better alternatives for StopCheck(), but
it's worth noting (perhaps adding a comment?) that if these checks ever get
complex, a single RunUntilIdle() call may be insufficient - possibly leading to
flakiness.

[michaelpg, 2017/03/17 02:34:26]

Meh again. How about using two functions, WaitForCompletion and WaitForIdle?

The downside is that the normal tests will hang for a long time if the callback
doesn't get called, but I'm OK with that since the test would be failing anyway
at that point.

+}
+
+}  // namespace extensions