Permission Action Reporting: Fix flaky Browsertest

chrome/browser/safe_browsing/mock_permission_report_sender.cc

Index: chrome/browser/safe_browsing/mock_permission_report_sender.cc
diff --git a/chrome/browser/safe_browsing/mock_permission_report_sender.cc b/chrome/browser/safe_browsing/mock_permission_report_sender.cc
index cfc8ed024d0f576b99affcf27c649f85a7641c47..f3290202c33bb03ea0de88a899ee5d8f62edbb9a 100644
--- a/chrome/browser/safe_browsing/mock_permission_report_sender.cc
+++ b/chrome/browser/safe_browsing/mock_permission_report_sender.cc
@@ -4,31 +4,73 @@
 
 #include "chrome/browser/safe_browsing/mock_permission_report_sender.h"
 
+#include "base/run_loop.h"
+
 namespace safe_browsing {
 
 MockPermissionReportSender::MockPermissionReportSender()
     : net::ReportSender(nullptr, DO_NOT_SEND_COOKIES) {
+  DCHECK(thread_checker_.CalledOnValidThread());

[Sam McNally, 2016/08/17 07:52:38]

Using a ThreadChecker in a class that's shared among threads isn't going to work
well. In this case, the constructor, destructor and Send() are run on the IO
thread, but the rest are on the UI thread.

[kcarattini, 2016/08/19 03:59:46]

Done.

   number_of_reports_ = 0;
 }
 
-MockPermissionReportSender::~MockPermissionReportSender() {}
+MockPermissionReportSender::~MockPermissionReportSender() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
 
 void MockPermissionReportSender::Send(const GURL& report_uri,
                                       const std::string& report) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+    base::RunLoop run_loop;
+    quit_closure_ = run_loop.QuitClosure();
+    content::BrowserThread::PostTaskAndReply(

[Sam McNally, 2016/08/17 07:52:38]

You don't need the RunLoop here. The IO thread doesn't need to wait for the data
to be set on the UI thread. PostTask() should be sufficient.

[kcarattini, 2016/08/19 03:59:46]

Done.

+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(
+            &MockPermissionReportSender::SetReportOnUIThread,
+            base::Unretained(this)),
+        run_loop.QuitClosure());
+    run_loop.Run();
+}
+
+// UI thread
+void MockReportSender::WaitForReportSet() {
+  report_set_ = false;

[Sam McNally, 2016/08/17 07:52:38]

I don't think |report_set_| is needed.

[kcarattini, 2016/08/19 03:59:46]

Done. I still don't quite understand how this is safe. Presumably the UI thread
is already running the code over in the test file. At some point, presumably
before WaitForReportSet() is called by the test code, Send() (above) gets called
on the IO thread and then the SetReport...() and Notify...() methods get called
on the UI thread. How are we guaranteed that WaitForReportSet() runs on the UI
thread before NotifyReportSet() does?

How does the declaration on the RunLoop here affect the way things are processed
on the UI thread (which is already running the code in the test file before this
RunLoop is declared)?

Thanks!

[kcarattini, 2016/08/22 05:09:05]

I've figured this out now, thanks!

+  base::RunLoop run_loop;
+  quit_closure_ = run_loop.QuitClosure();
+  run_loop.Run();
+}
+
+// UI thread
+void MockReportSender::SetReportOnUIThread() {
   latest_report_uri_ = report_uri;
   latest_report_ = report;
   number_of_reports_++;
+
+  report_set_ = true;
+  NotifyReportSet();
+}
+
+// UI thread
+void MockReportSender::NotifyReportSet() {
+  if (!quit_closure_.is_null() && report_set_) {
+    quit_closure_.Run();
+    quit_closure_.Reset();
+  }
 }
 
 const GURL& MockPermissionReportSender::latest_report_uri() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   return latest_report_uri_;
 }
 
 const std::string& MockPermissionReportSender::latest_report() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   return latest_report_;
 }
 
 int MockPermissionReportSender::GetAndResetNumberOfReportsSent() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   int new_reports = number_of_reports_;
   number_of_reports_ = 0;
   return new_reports;