Permission Action Reporting: Fix flaky Browsertest

chrome/browser/safe_browsing/mock_permission_report_sender.cc

 // Copyright 2016 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 "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;
 }
 
 }  // namespace safe_browsing