service worker: Fix crash in EndEvent().

This happened when the eager code caching started a task but the timeout
timer aborted it. The eager code caching code wasn't aware of the
timeout so would call EndTask() and cause a crash  when the timeout
timer couldn't find the task.

Bug: 933273
Change-Id: I09e9b2472f32413343c22d0d271b7b3dda40ef90
Reviewed-on: https://chromium-review.googlesource.com/c/1482433Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634556}

content/renderer/service_worker/service_worker_context_client.h

@@ -365,6 +365,7 @@ class CONTENT_EXPORT ServiceWorkerContextClient
 
   void SetTimeoutTimerForTesting(
       std::unique_ptr<ServiceWorkerTimeoutTimer> timeout_timer);
+  ServiceWorkerTimeoutTimer* GetTimeoutTimerForTesting();
 
   // TODO(crbug.com/907311): Remove after we identified the cause of crash.
   void RecordDebugLog(const char* message);

content/renderer/service_worker/service_worker_context_client_unittest.cc

@@ -372,6 +372,11 @@ class ServiceWorkerContextClientTest : public testing::Test {
     return context_client;
   }
 
+  ServiceWorkerTimeoutTimer* GetTimeoutTimer(
+      ServiceWorkerContextClient* context_client) {
+    return context_client->GetTimeoutTimerForTesting();
+  }
+
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner() const {
     return task_runner_;
   }
@@ -615,4 +620,26 @@ TEST_F(ServiceWorkerContextClientTest, TaskInServiceWorker) {
   EXPECT_TRUE(context_client->RequestedTermination());
 }
 
+// Tests timing out a task created by WillStartTask().
+TEST_F(ServiceWorkerContextClientTest, AbortedTaskInServiceWorker) {
+  EnableServicification();
+  ContextClientPipes pipes;
+  MockWebServiceWorkerContextProxy mock_proxy;
+  ServiceWorkerContextClient* context_client =
+      CreateIdleContextClient(&pipes, &mock_proxy);
+
+  // Make a task that times out.
+  int task_id = context_client->WillStartTask();
+  task_runner()->FastForwardBy(ServiceWorkerTimeoutTimer::kEventTimeout +
+                               ServiceWorkerTimeoutTimer::kUpdateInterval +
+                               base::TimeDelta::FromSeconds(1));
+
+  // The event should have been aborted.
+  ServiceWorkerTimeoutTimer* timeout_timer = GetTimeoutTimer(context_client);
+  EXPECT_FALSE(timeout_timer->HasEvent(task_id));
+
+  // Calling DidEndTask() shouldn't crash.
+  context_client->DidEndTask(task_id);
+}
+
 }  // namespace content

content/renderer/service_worker/service_worker_timeout_timer.cc

@@ -6,7 +6,6 @@
 
 #include "base/atomic_sequence_num.h"
 #include "base/bind.h"
-#include "base/debug/alias.h"
 #include "base/stl_util.h"
 #include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
@@ -117,24 +116,10 @@ int ServiceWorkerTimeoutTimer::StartEventWithCustomTimeout(
   return event_id;
 }
 
-// TODO(falken): Debugging for https://crbug.com/933273.
-void ServiceWorkerTimeoutTimer::CrashBecauseNoEvent(
-    int event_id,
-    const base::Location& from_here) {
-  DEBUG_ALIAS_FOR_CSTR(from_here_copy, from_here.ToString().c_str(), 256);
-  bool in_dtor = in_dtor_;
-  base::debug::Alias(&in_dtor);
-  CHECK(false);
-}
+void ServiceWorkerTimeoutTimer::EndEvent(int event_id) {
+  CHECK(HasEvent(event_id));
 
-void ServiceWorkerTimeoutTimer::EndEvent(int event_id,
-                                         const base::Location& from_here) {
   auto iter = id_event_map_.find(event_id);
-  if (iter == id_event_map_.end()) {
-    CrashBecauseNoEvent(event_id, from_here);
-    return;
-  }
-  CHECK(iter != id_event_map_.end());
   inflight_events_.erase(iter->second);
   id_event_map_.erase(iter);
 
@@ -142,6 +127,10 @@ void ServiceWorkerTimeoutTimer::EndEvent(int event_id,
     OnNoInflightEvent();
 }
 
+bool ServiceWorkerTimeoutTimer::HasEvent(int event_id) const {
+  return id_event_map_.find(event_id) != id_event_map_.end();
+}
+
 std::unique_ptr<ServiceWorkerTimeoutTimer::StayAwakeToken>
 ServiceWorkerTimeoutTimer::CreateStayAwakeToken() {
   if (!blink::ServiceWorkerUtils::IsServicificationEnabled())

content/renderer/service_worker/service_worker_timeout_timer.h

@@ -10,7 +10,6 @@
 
 #include "base/callback.h"
 #include "base/containers/queue.h"
-#include "base/location.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
@@ -78,8 +77,11 @@ class CONTENT_EXPORT ServiceWorkerTimeoutTimer {
   int StartEventWithCustomTimeout(
       base::OnceCallback<void(int /* event_id */)> abort_callback,
       base::TimeDelta timeout);
-  void CrashBecauseNoEvent(int event_id, const base::Location& from_here);
-  void EndEvent(int event_id, const base::Location& from_here);
+
+  void EndEvent(int event_id);
+
+  // Returns true if |event_id| was started and hasn't ended.
+  bool HasEvent(int event_id) const;
 
   // Creates a StayAwakeToken to ensure that the idle timer won't be triggered
   // while any of these are alive.

content/renderer/service_worker/service_worker_timeout_timer_unittest.cc

@@ -66,7 +66,7 @@ base::OnceClosure CreateDispatchingEventTask(
 
         out_tags->emplace_back(std::move(tag));
 
-        timer->EndEvent(event_id, FROM_HERE);
+        timer->EndEvent(event_id);
         EXPECT_FALSE(event.has_aborted());
       },
       timer, std::move(tag), out_tags);
@@ -131,12 +131,12 @@ TEST_F(ServiceWorkerTimeoutTimerTest, IdleTimer) {
   // Nothing happens since there are two inflight events.
   EXPECT_FALSE(is_idle);
 
-  timer.EndEvent(event_id_2, FROM_HERE);
+  timer.EndEvent(event_id_2);
   task_runner()->FastForwardBy(kIdleInterval);
   // Nothing happens since there is an inflight event.
   EXPECT_FALSE(is_idle);
 
-  timer.EndEvent(event_id_1, FROM_HERE);
+  timer.EndEvent(event_id_1);
   task_runner()->FastForwardBy(kIdleInterval);
   // |idle_callback| should be fired.
   EXPECT_TRUE(is_idle);
@@ -148,7 +148,7 @@ TEST_F(ServiceWorkerTimeoutTimerTest, IdleTimer) {
   EXPECT_FALSE(is_idle);
 
   std::unique_ptr<StayAwakeToken> token = timer.CreateStayAwakeToken();
-  timer.EndEvent(event_id_3, FROM_HERE);
+  timer.EndEvent(event_id_3);
   task_runner()->FastForwardBy(kIdleInterval);
   // Nothing happens since there is a living StayAwakeToken.
   EXPECT_FALSE(is_idle);
@@ -196,7 +196,7 @@ TEST_F(ServiceWorkerTimeoutTimerTest, EventTimer) {
 
   EXPECT_FALSE(event1.has_aborted());
   EXPECT_FALSE(event2.has_aborted());
-  timer.EndEvent(event1.event_id(), FROM_HERE);
+  timer.EndEvent(event1.event_id());
   task_runner()->FastForwardBy(ServiceWorkerTimeoutTimer::kEventTimeout +
                                base::TimeDelta::FromSeconds(1));
 
@@ -348,7 +348,7 @@ TEST_F(ServiceWorkerTimeoutTimerTest, SetIdleTimerDelayToZero) {
     // Nothing happens since there is an inflight event.
     EXPECT_FALSE(is_idle);
 
-    timer.EndEvent(event_id, FROM_HERE);
+    timer.EndEvent(event_id);
     // EndEvent() immediately triggers the idle callback.
     EXPECT_TRUE(is_idle);
   }
@@ -364,11 +364,11 @@ TEST_F(ServiceWorkerTimeoutTimerTest, SetIdleTimerDelayToZero) {
     // Nothing happens since there are two inflight events.
     EXPECT_FALSE(is_idle);
 
-    timer.EndEvent(event_id_1, FROM_HERE);
+    timer.EndEvent(event_id_1);
     // Nothing happens since there is an inflight event.
     EXPECT_FALSE(is_idle);
 
-    timer.EndEvent(event_id_2, FROM_HERE);
+    timer.EndEvent(event_id_2);
     // EndEvent() immediately triggers the idle callback when no inflight events
     // exist.
     EXPECT_TRUE(is_idle);