Domain Reliability: Don't clear "upload pending" bit until upload starts

Someone noticed that occasionally the uploader sent an empty upload. I
looked into the scheduler and found that it was clearing the "upload
pending" bit early, when the upload was scheduled. This meant that if a
beacon arrived between scheduling and starting the upload, the bit was
still set on its behalf, despite it being included in the upload. Once
the upload finished, the scheduler would see the bit still set and
schedule another.

This patch adds a couple of unittests for interesting interleavings
(beacons that arrive between scheduling and starting the upload, and
between starting and finishing the upload), and clears the upload
pending bit in the right place.

BUG=356791

Review URL: https://codereview.chromium.org/272773004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269592 0039d316-1c4b-4281-b951-d872f2087c98

components/domain_reliability/scheduler.cc

@@ -89,6 +89,7 @@ void DomainReliabilityScheduler::OnBeaconAdded() {
 size_t DomainReliabilityScheduler::OnUploadStart() {
   DCHECK(upload_scheduled_);
   DCHECK_EQ(kInvalidCollectorIndex, collector_index_);
+  upload_pending_ = false;
   upload_scheduled_ = false;
   upload_running_ = true;
 
@@ -139,7 +140,6 @@ void DomainReliabilityScheduler::MaybeScheduleUpload() {
   if (!upload_pending_ || upload_scheduled_ || upload_running_)
     return;
 
-  upload_pending_ = false;
   upload_scheduled_ = true;
   old_first_beacon_time_ = first_beacon_time_;
 

components/domain_reliability/scheduler_unittest.cc

@@ -63,6 +63,7 @@ class DomainReliabilitySchedulerTest : public testing::Test {
 
     if (callback_called_ && expected_min == callback_min_
                          && expected_max == callback_max_) {
+      callback_called_ = false;
       return ::testing::AssertionSuccess();
     }
 
@@ -210,4 +211,40 @@ TEST_F(DomainReliabilitySchedulerTest, DetermineCollectorAtUpload) {
   scheduler_->OnUploadComplete(true);
 }
 
+TEST_F(DomainReliabilitySchedulerTest, BeaconWhilePending) {
+  CreateScheduler(1);
+
+  scheduler_->OnBeaconAdded();
+  ASSERT_TRUE(CheckPendingUpload(min_delay(), max_delay()));
+
+  // Second beacon should not call callback again.
+  scheduler_->OnBeaconAdded();
+  ASSERT_TRUE(CheckNoPendingUpload());
+  time_.Advance(min_delay());
+
+  // No pending upload after beacon.
+  ASSERT_TRUE(CheckStartingUpload(0));
+  scheduler_->OnUploadComplete(true);
+  ASSERT_TRUE(CheckNoPendingUpload());
+}
+
+TEST_F(DomainReliabilitySchedulerTest, BeaconWhileUploading) {
+  CreateScheduler(1);
+
+  scheduler_->OnBeaconAdded();
+  ASSERT_TRUE(CheckPendingUpload(min_delay(), max_delay()));
+  time_.Advance(min_delay());
+
+  // If a beacon arrives during the upload, a new upload should be pending.
+  ASSERT_TRUE(CheckStartingUpload(0));
+  scheduler_->OnBeaconAdded();
+  scheduler_->OnUploadComplete(true);
+  ASSERT_TRUE(CheckPendingUpload(min_delay(), max_delay()));
+
+  time_.Advance(min_delay());
+  ASSERT_TRUE(CheckStartingUpload(0));
+  scheduler_->OnUploadComplete(true);
+  ASSERT_TRUE(CheckNoPendingUpload());
+}
+
 }  // namespace domain_reliability