Inform Background Service handlers when recording time expires.

Handlers should be aware that the recording time has expired so that the
UI can be updated. The handlers are lazily informed when that happens.

Also remove DCHECKs in (Start/Stop)Recording since multiple handlers can
make recording state changes at the same time.

Bug: 927726
Change-Id: I921083e694f37665608257d5a35b4108b20d60d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545068
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#646353}

content/browser/devtools/devtools_background_services_context.cc

@@ -56,6 +56,12 @@ DevToolsBackgroundServicesContext::DevToolsBackgroundServicesContext(
   for (const auto& expiration_time : expiration_times) {
     DCHECK(devtools::proto::BackgroundService_IsValid(expiration_time.first));
     expiration_times_[expiration_time.first] = expiration_time.second;
+
+    auto service =
+        static_cast<devtools::proto::BackgroundService>(expiration_time.first);
+    // If the recording permission for |service| has expired, set it to null.
+    if (IsRecordingExpired(service))
+      expiration_times_[expiration_time.first] = base::Time();
   }
 }
 
@@ -75,8 +81,6 @@ void DevToolsBackgroundServicesContext::StartRecording(
     devtools::proto::BackgroundService service) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  DCHECK(!IsRecording(service));
-
   // TODO(rayankans): Make the time delay finch configurable.
   base::Time expiration_time = base::Time::Now() + base::TimeDelta::FromDays(3);
   expiration_times_[service] = expiration_time;
@@ -92,9 +96,7 @@ void DevToolsBackgroundServicesContext::StopRecording(
     devtools::proto::BackgroundService service) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  DCHECK(IsRecording(service));
   expiration_times_[service] = base::Time();
-
   GetContentClient()->browser()->UpdateDevToolsBackgroundServiceExpiration(
       browser_context_, service, base::Time());
 
@@ -104,7 +106,16 @@ void DevToolsBackgroundServicesContext::StopRecording(
 
 bool DevToolsBackgroundServicesContext::IsRecording(
     devtools::proto::BackgroundService service) {
-  return base::Time::Now() < expiration_times_[service];
+  // Returns whether |service| has been enabled. When the expiration time has
+  // been met it will be lazily updated to be null.
+  return !expiration_times_[service].is_null();
+}
+
+bool DevToolsBackgroundServicesContext::IsRecordingExpired(
+    devtools::proto::BackgroundService service) {
+  // Copy the expiration time to avoid data races.
+  const base::Time expiration_time = expiration_times_[service];
+  return !expiration_time.is_null() && expiration_time < base::Time::Now();
 }
 
 void DevToolsBackgroundServicesContext::GetLoggedBackgroundServiceEvents(
@@ -198,6 +209,17 @@ void DevToolsBackgroundServicesContext::LogBackgroundServiceEvent(
   if (!IsRecording(service))
     return;
 
+  if (IsRecordingExpired(service)) {
+    // We should stop recording because of the expiration time. We should
+    // also inform the observers that we stopped recording.
+    base::PostTaskWithTraits(
+        FROM_HERE, {BrowserThread::UI},
+        base::BindOnce(
+            &DevToolsBackgroundServicesContext::OnRecordingTimeExpired,
+            weak_ptr_factory_.GetWeakPtr(), service));
+    return;
+  }
+
   devtools::proto::BackgroundServiceEvent event;
   event.set_timestamp(
       base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
@@ -228,4 +250,14 @@ void DevToolsBackgroundServicesContext::NotifyEventObservers(
     observer.OnEventReceived(event);
 }
 
+void DevToolsBackgroundServicesContext::OnRecordingTimeExpired(
+    devtools::proto::BackgroundService service) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // This could have been stopped by the user in the meanwhile, or we
+  // received duplicate time expiry events.
+  if (IsRecordingExpired(service))
+    StopRecording(service);
+}
+
 }  // namespace content

content/browser/devtools/devtools_background_services_context.h

@@ -100,6 +100,9 @@ class CONTENT_EXPORT DevToolsBackgroundServicesContext
   friend class base::RefCountedThreadSafe<DevToolsBackgroundServicesContext>;
   ~DevToolsBackgroundServicesContext();
 
+  // Whether |service| has an expiration time and it was exceeded.
+  bool IsRecordingExpired(devtools::proto::BackgroundService service);
+
   void GetLoggedBackgroundServiceEventsOnIO(
       devtools::proto::BackgroundService service,
       GetLoggedBackgroundServiceEventsCallback callback);
@@ -115,6 +118,8 @@ class CONTENT_EXPORT DevToolsBackgroundServicesContext
   void NotifyEventObservers(
       const devtools::proto::BackgroundServiceEvent& event);
 
+  void OnRecordingTimeExpired(devtools::proto::BackgroundService service);
+
   BrowserContext* browser_context_;
   scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_;
 

content/browser/devtools/devtools_background_services_context_unittest.cc

@@ -343,6 +343,24 @@ TEST_F(DevToolsBackgroundServicesContextTest, RecordingExpiration) {
 
   SimulateOneWeekPassing();
   EXPECT_FALSE(GetExpirationTime().is_null());
+
+  // Recording should be true, with an expired value.
+  EXPECT_TRUE(IsRecording());
+
+  // Logging should not happen.
+  EXPECT_CALL(*this, OnEventReceived(_)).Times(0);
+  LogTestBackgroundServiceEvent("f1");
+
+  // Observers should be informed that recording stopped.
+  EXPECT_CALL(
+      *this,
+      OnRecordingStateChanged(
+          false, devtools::proto::BackgroundService::TEST_BACKGROUND_SERVICE));
+
+  thread_bundle_.RunUntilIdle();
+
+  // The expiration time entry should be cleared.
+  EXPECT_TRUE(GetExpirationTime().is_null());
   EXPECT_FALSE(IsRecording());
 }