diff options
author | danakj <danakj@chromium.org> | 2014-10-29 11:24:49 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-10-29 18:25:32 +0000 |
commit | 1a3c317f3cca7694a4083f56d776607ad49498ca (patch) | |
tree | 5297b68f32ea163b767d3741d4ee150aa7836dfb /cc/base | |
parent | 5a3210bc7b24168dc00cf94b8c9b314ab6f26382 (diff) | |
download | chromium_src-1a3c317f3cca7694a4083f56d776607ad49498ca.zip chromium_src-1a3c317f3cca7694a4083f56d776607ad49498ca.tar.gz chromium_src-1a3c317f3cca7694a4083f56d776607ad49498ca.tar.bz2 |
cc: Fix weakptr destruction of DelayedUniqueNotifier on shutdown.
During shutdown, the ThreadProxy is destroyed on the main thread, after
being Closed on the compositor thread.
The DelayedUniqueNotifier has posted weak ptrs inside the class, and
when the class is destroyed those weak ptrs are invalidated. But the
DelayedUniqueNotifier is destroyed on the main thread with ThreadProxy
while the weak ptrs were created on the compositor thread, leading to
CHECK failures.
This adds a new Shutdown() method to DelayedUniqueNotifier that will
immediately invalidate the weak ptrs (on the correct thread) and leave
the DelayedUniqueNotifier in a state that it's actually not able to
post/create new weak ptrs. This resolves the CHECK failures as when
the DelayedUniqueNotifier is later destroyed, there are no weak ptrs
left to invalidate.
R=vmpstr
BUG=428347
Review URL: https://codereview.chromium.org/692573002
Cr-Commit-Position: refs/heads/master@{#301881}
Diffstat (limited to 'cc/base')
-rw-r--r-- | cc/base/delayed_unique_notifier.cc | 9 | ||||
-rw-r--r-- | cc/base/delayed_unique_notifier.h | 5 | ||||
-rw-r--r-- | cc/base/delayed_unique_notifier_unittest.cc | 69 |
3 files changed, 83 insertions, 0 deletions
diff --git a/cc/base/delayed_unique_notifier.cc b/cc/base/delayed_unique_notifier.cc index c297267..1824226 100644 --- a/cc/base/delayed_unique_notifier.cc +++ b/cc/base/delayed_unique_notifier.cc @@ -43,6 +43,15 @@ void DelayedUniqueNotifier::Cancel() { next_notification_time_ = base::TimeTicks(); } +void DelayedUniqueNotifier::Shutdown() { + // This function must destroy any weak ptrs since after being cancelled, this + // class may be destroyed on another thread during compositor shutdown. + weak_ptr_factory_.InvalidateWeakPtrs(); + // Deliberately leaves notification_pending_ = true forever so new tasks with + // weak ptrs can not be created. + notification_pending_ = true; +} + bool DelayedUniqueNotifier::HasPendingNotification() const { return notification_pending_ && !next_notification_time_.is_null(); } diff --git a/cc/base/delayed_unique_notifier.h b/cc/base/delayed_unique_notifier.h index 59d906b..02051be 100644 --- a/cc/base/delayed_unique_notifier.h +++ b/cc/base/delayed_unique_notifier.h @@ -37,6 +37,11 @@ class CC_EXPORT DelayedUniqueNotifier { // Cancel any previously scheduled runs. void Cancel(); + // Cancel previously scheduled runs and prevent any new runs from starting. + // After calling this the DelayedUniqueNotifier will have no outstanding + // WeakPtrs. + void Shutdown(); + // Returns true if a notification is currently scheduled to run. bool HasPendingNotification() const; diff --git a/cc/base/delayed_unique_notifier_unittest.cc b/cc/base/delayed_unique_notifier_unittest.cc index 41cde83..3e41d32 100644 --- a/cc/base/delayed_unique_notifier_unittest.cc +++ b/cc/base/delayed_unique_notifier_unittest.cc @@ -262,5 +262,74 @@ TEST_F(DelayedUniqueNotifierTest, CancelAndHasPendingNotification) { EXPECT_FALSE(notifier.HasPendingNotification()); } +TEST_F(DelayedUniqueNotifierTest, ShutdownWithScheduledTask) { + base::TimeDelta delay = base::TimeDelta::FromInternalValue(20); + TestNotifier notifier( + task_runner_.get(), + base::Bind(&DelayedUniqueNotifierTest::Notify, base::Unretained(this)), + delay); + + EXPECT_EQ(0, NotificationCount()); + + // Schedule for |delay| seconds from now. + base::TimeTicks schedule_time = + notifier.Now() + base::TimeDelta::FromInternalValue(10); + notifier.SetNow(schedule_time); + notifier.Schedule(); + EXPECT_TRUE(notifier.HasPendingNotification()); + + // Shutdown the notifier. + notifier.Shutdown(); + + // The task is still there, but... + std::deque<base::TestPendingTask> tasks = TakePendingTasks(); + ASSERT_EQ(1u, tasks.size()); + + // Running the task after shutdown does nothing since it's cancelled. + tasks[0].task.Run(); + EXPECT_EQ(0, NotificationCount()); + + tasks = TakePendingTasks(); + EXPECT_EQ(0u, tasks.size()); + + // We are no longer able to schedule tasks. + notifier.Schedule(); + tasks = TakePendingTasks(); + ASSERT_EQ(0u, tasks.size()); + + // Verify after the scheduled time happens there is still no task. + notifier.SetNow(notifier.Now() + delay); + tasks = TakePendingTasks(); + ASSERT_EQ(0u, tasks.size()); +} + +TEST_F(DelayedUniqueNotifierTest, ShutdownPreventsSchedule) { + base::TimeDelta delay = base::TimeDelta::FromInternalValue(20); + TestNotifier notifier( + task_runner_.get(), + base::Bind(&DelayedUniqueNotifierTest::Notify, base::Unretained(this)), + delay); + + EXPECT_EQ(0, NotificationCount()); + + // Schedule for |delay| seconds from now. + base::TimeTicks schedule_time = + notifier.Now() + base::TimeDelta::FromInternalValue(10); + notifier.SetNow(schedule_time); + + // Shutdown the notifier. + notifier.Shutdown(); + + // Scheduling a task no longer does anything. + notifier.Schedule(); + std::deque<base::TestPendingTask> tasks = TakePendingTasks(); + ASSERT_EQ(0u, tasks.size()); + + // Verify after the scheduled time happens there is still no task. + notifier.SetNow(notifier.Now() + delay); + tasks = TakePendingTasks(); + ASSERT_EQ(0u, tasks.size()); +} + } // namespace } // namespace cc |