From edce70216d642db3b5433c540b2cbef93a183430 Mon Sep 17 00:00:00 2001 From: "kaiwang@chromium.org" Date: Fri, 16 Nov 2012 04:36:34 +0000 Subject: Add function to CancelableTaskTracker and convert BootTimeLoader BUG=155883 Review URL: https://chromiumcodereview.appspot.com/11410073 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@168134 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/chromeos/boot_times_loader.cc | 52 ++++++++++------ chrome/browser/chromeos/boot_times_loader.h | 32 +++++----- .../browser/chromeos/login/version_info_updater.cc | 13 ++-- .../browser/chromeos/login/version_info_updater.h | 12 ++-- chrome/common/cancelable_task_tracker.cc | 36 +++++++++++ chrome/common/cancelable_task_tracker.h | 12 ++++ chrome/common/cancelable_task_tracker_unittest.cc | 70 +++++++++++++++++++++- 7 files changed, 177 insertions(+), 50 deletions(-) diff --git a/chrome/browser/chromeos/boot_times_loader.cc b/chrome/browser/chromeos/boot_times_loader.cc index 2137cd8..4aac9e2 100644 --- a/chrome/browser/chromeos/boot_times_loader.cc +++ b/chrome/browser/chromeos/boot_times_loader.cc @@ -11,7 +11,9 @@ #include "base/file_path.h" #include "base/file_util.h" #include "base/lazy_instance.h" +#include "base/location.h" #include "base/message_loop.h" +#include "base/message_loop_proxy.h" #include "base/metrics/histogram.h" #include "base/process_util.h" #include "base/string_number_conversions.h" @@ -67,8 +69,19 @@ const std::string GetTabUrl(RenderWidgetHost* rwh) { } return std::string(); } + +void PostCallbackIfNotCanceled( + const CancelableTaskTracker::IsCanceledCallback& is_canceled_cb, + base::TaskRunner* task_runner, + const chromeos::BootTimesLoader::GetBootTimesCallback& callback, + const chromeos::BootTimesLoader::BootTimes& boot_times) { + if (is_canceled_cb.Run()) + return; + task_runner->PostTask(FROM_HERE, base::Bind(callback, boot_times)); } +} // namespace + namespace chromeos { #define FPL(value) FILE_PATH_LITERAL(value) @@ -122,31 +135,34 @@ BootTimesLoader* BootTimesLoader::Get() { return g_boot_times_loader.Pointer(); } -BootTimesLoader::Handle BootTimesLoader::GetBootTimes( - CancelableRequestConsumerBase* consumer, - const GetBootTimesCallback& callback) { +CancelableTaskTracker::TaskId BootTimesLoader::GetBootTimes( + const GetBootTimesCallback& callback, + CancelableTaskTracker* tracker) { if (!BrowserThread::IsMessageLoopValid(BrowserThread::FILE)) { // This should only happen if Chrome is shutting down, so we don't do // anything. - return 0; + return CancelableTaskTracker::kBadTaskId; } const CommandLine& command_line = *CommandLine::ForCurrentProcess(); if (command_line.HasSwitch(switches::kTestType)) { // TODO(davemoore) This avoids boottimes for tests. This needs to be // replaced with a mock of BootTimesLoader. - return 0; + return CancelableTaskTracker::kBadTaskId; } - scoped_refptr > request( - new CancelableRequest(callback)); - AddRequest(request, consumer); + CancelableTaskTracker::IsCanceledCallback is_canceled; + CancelableTaskTracker::TaskId id = tracker->NewTrackedTaskId(&is_canceled); + GetBootTimesCallback callback_runner = + base::Bind(&PostCallbackIfNotCanceled, + is_canceled, base::MessageLoopProxy::current(), callback); BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, - base::Bind(&Backend::GetBootTimes, backend_, request)); - return request->handle(); + base::Bind(&Backend::GetBootTimesAndRunCallback, + backend_, is_canceled, callback_runner)); + return id; } // Extracts the uptime value from files located in /tmp, returning the @@ -214,8 +230,12 @@ static void SendBootTimesToUMA(const BootTimesLoader::BootTimes& boot_times) { DCHECK(file_util::PathExists(sent)); } -void BootTimesLoader::Backend::GetBootTimes( - const scoped_refptr& request) { +void BootTimesLoader::Backend::GetBootTimesAndRunCallback( + const CancelableTaskTracker::IsCanceledCallback& is_canceled_cb, + const GetBootTimesCallback& callback) { + if (is_canceled_cb.Run()) + return; + const FilePath::CharType kFirmwareBootTime[] = FPL("firmware-boot-time"); const FilePath::CharType kPreStartup[] = FPL("pre-startup"); const FilePath::CharType kChromeExec[] = FPL("chrome-exec"); @@ -224,9 +244,6 @@ void BootTimesLoader::Backend::GetBootTimes( const FilePath::CharType kLoginPromptReady[] = FPL("login-prompt-ready"); const FilePath::StringType uptime_prefix = kUptimePrefix; - if (request->canceled()) - return; - // Wait until firmware-boot-time file exists by reposting. FilePath log_dir(kLogPath); FilePath log_file = log_dir.Append(kFirmwareBootTime); @@ -234,7 +251,8 @@ void BootTimesLoader::Backend::GetBootTimes( BrowserThread::PostDelayedTask( BrowserThread::FILE, FROM_HERE, - base::Bind(&Backend::GetBootTimes, this, request), + base::Bind(&Backend::GetBootTimesAndRunCallback, + this, is_canceled_cb, callback), base::TimeDelta::FromMilliseconds(kReadAttemptDelayMs)); return; } @@ -258,7 +276,7 @@ void BootTimesLoader::Backend::GetBootTimes( SendBootTimesToUMA(boot_times); - request->ForwardResult(request->handle(), boot_times); + callback.Run(boot_times); } // Appends the given buffer into the file. Returns the number of bytes diff --git a/chrome/browser/chromeos/boot_times_loader.h b/chrome/browser/chromeos/boot_times_loader.h index 691a44ff..dc420e8 100644 --- a/chrome/browser/chromeos/boot_times_loader.h +++ b/chrome/browser/chromeos/boot_times_loader.h @@ -12,7 +12,7 @@ #include "base/callback_forward.h" #include "base/compiler_specific.h" #include "base/time.h" -#include "chrome/browser/common/cancelable_request.h" +#include "chrome/common/cancelable_task_tracker.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" #include "content/public/browser/render_widget_host.h" @@ -22,17 +22,14 @@ namespace chromeos { // BootTimesLoader loads the bootimes of Chrome OS from the file system. // Loading is done asynchronously on the file thread. Once loaded, // BootTimesLoader calls back to a method of your choice with the boot times. -// To use BootTimesLoader do the following: +// To use BootTimesLoader, do the following: // // . In your class define a member field of type chromeos::BootTimesLoader and -// CancelableRequestConsumerBase. +// CancelableTaskTracker. // . Define the callback method, something like: -// void OnBootTimesLoader(chromeos::BootTimesLoader::Handle, -// BootTimesLoader::BootTimes boot_times); -// . When you want the version invoke: loader.GetBootTimes(&consumer, callback); -class BootTimesLoader - : public CancelableRequestProvider, - public content::NotificationObserver { +// void OnBootTimesLoaded(const BootTimesLoader::BootTimes& boot_times); +// . When you want the version invoke: loader.GetBootTimes(callback, &tracker_); +class BootTimesLoader : public content::NotificationObserver { public: BootTimesLoader(); virtual ~BootTimesLoader(); @@ -60,17 +57,14 @@ class BootTimesLoader total(0) {} } BootTimes; - // Signature - typedef base::Callback GetBootTimesCallback; - - typedef CancelableRequest GetBootTimesRequest; - static BootTimesLoader* Get(); + typedef base::Callback GetBootTimesCallback; + // Asynchronously requests the info. - Handle GetBootTimes( - CancelableRequestConsumerBase* consumer, - const GetBootTimesCallback& callback); + CancelableTaskTracker::TaskId GetBootTimes( + const GetBootTimesCallback& callback, + CancelableTaskTracker* tracker); // Add a time marker for login. A timeline will be dumped to // /tmp/login-times-sent after login is done. If |send_to_uma| is true @@ -119,7 +113,9 @@ class BootTimesLoader public: Backend() {} - void GetBootTimes(const scoped_refptr& request); + void GetBootTimesAndRunCallback( + const CancelableTaskTracker::IsCanceledCallback& is_canceled_cb, + const GetBootTimesCallback& callback); private: friend class base::RefCountedThreadSafe; diff --git a/chrome/browser/chromeos/login/version_info_updater.cc b/chrome/browser/chromeos/login/version_info_updater.cc index 2c9ccc5..42aa2e4 100644 --- a/chrome/browser/chromeos/login/version_info_updater.cc +++ b/chrome/browser/chromeos/login/version_info_updater.cc @@ -59,10 +59,10 @@ void VersionInfoUpdater::StartUpdate(bool is_official_build) { base::Bind(&VersionInfoUpdater::OnVersion, base::Unretained(this)), &tracker_); boot_times_loader_.GetBootTimes( - &boot_times_consumer_, - base::Bind(is_official_build ? &VersionInfoUpdater::OnBootTimesNoop : - &VersionInfoUpdater::OnBootTimes, - base::Unretained(this))); + base::Bind(is_official_build ? &VersionInfoUpdater::OnBootTimesNoop + : &VersionInfoUpdater::OnBootTimes, + base::Unretained(this)), + &tracker_); } else { UpdateVersionLabel(); } @@ -195,11 +195,10 @@ void VersionInfoUpdater::OnVersion(const std::string& version) { } void VersionInfoUpdater::OnBootTimesNoop( - BootTimesLoader::Handle handle, BootTimesLoader::BootTimes boot_times) { -} + const BootTimesLoader::BootTimes& boot_times) {} void VersionInfoUpdater::OnBootTimes( - BootTimesLoader::Handle handle, BootTimesLoader::BootTimes boot_times) { + const BootTimesLoader::BootTimes& boot_times) { const char* kBootTimesNoChromeExec = "Non-firmware boot took %.2f seconds (kernel %.2fs, system %.2fs)"; const char* kBootTimesChromeExec = diff --git a/chrome/browser/chromeos/login/version_info_updater.h b/chrome/browser/chromeos/login/version_info_updater.h index c02ff71..fc9868d 100644 --- a/chrome/browser/chromeos/login/version_info_updater.h +++ b/chrome/browser/chromeos/login/version_info_updater.h @@ -75,21 +75,19 @@ class VersionInfoUpdater : public policy::CloudPolicySubsystem::Observer, // Callback from chromeos::VersionLoader giving the version. void OnVersion(const std::string& version); // Callback from chromeos::InfoLoader giving the boot times. - void OnBootTimes( - BootTimesLoader::Handle handle, BootTimesLoader::BootTimes boot_times); + void OnBootTimes(const BootTimesLoader::BootTimes& boot_times); // Null callback from chromeos::InfoLoader. - void OnBootTimesNoop( - BootTimesLoader::Handle handle, BootTimesLoader::BootTimes boot_times); + void OnBootTimesNoop(const BootTimesLoader::BootTimes& boot_times); // Handles asynchronously loading the version. VersionLoader version_loader_; // Used to request the version. - CancelableTaskTracker tracker_; + CancelableRequestConsumer version_consumer_; // Handles asynchronously loading the boot times. BootTimesLoader boot_times_loader_; - // Used to request the boot times. - CancelableRequestConsumer boot_times_consumer_; + // Used to request boot times. + CancelableTaskTracker tracker_; // Information pieces for version label. std::string version_text_; diff --git a/chrome/common/cancelable_task_tracker.cc b/chrome/common/cancelable_task_tracker.cc index 3570e05..28ee436 100644 --- a/chrome/common/cancelable_task_tracker.cc +++ b/chrome/common/cancelable_task_tracker.cc @@ -8,6 +8,7 @@ #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/location.h" #include "base/memory/ref_counted.h" #include "base/message_loop_proxy.h" #include "base/synchronization/cancellation_flag.h" @@ -33,6 +34,18 @@ void RunIfNotCanceledThenUntrack(const CancellationFlag* flag, untrack.Run(); } +bool IsCanceled(const CancellationFlag* flag, + base::ScopedClosureRunner* untrack_runner) { + return flag->IsSet(); +} + +void RunOrPostToTaskRunner(TaskRunner* task_runner, const Closure& closure) { + if (task_runner->RunsTasksOnCurrentThread()) + closure.Run(); + else + task_runner->PostTask(FROM_HERE, closure); +} + } // namespace // static @@ -84,6 +97,29 @@ CancelableTaskTracker::TaskId CancelableTaskTracker::PostTaskAndReply( return id; } +CancelableTaskTracker::TaskId CancelableTaskTracker::NewTrackedTaskId( + IsCanceledCallback* is_canceled_cb) { + DCHECK(thread_checker_.CalledOnValidThread()); + DCHECK(base::MessageLoopProxy::current()); + + TaskId id = next_id_; + next_id_++; // int64 is big enough that we ignore the potential overflow. + + // Both owned by |is_canceled_cb|. + CancellationFlag* flag = new CancellationFlag(); + base::ScopedClosureRunner* untrack_runner = new base::ScopedClosureRunner( + Bind(&RunOrPostToTaskRunner, + base::MessageLoopProxy::current(), + Bind(&CancelableTaskTracker::Untrack, + weak_factory_.GetWeakPtr(), id))); + + *is_canceled_cb = Bind(&IsCanceled, + base::Owned(flag), + base::Owned(untrack_runner)); + Track(id, flag); + return id; +} + void CancelableTaskTracker::TryCancel(TaskId id) { DCHECK(thread_checker_.CalledOnValidThread()); diff --git a/chrome/common/cancelable_task_tracker.h b/chrome/common/cancelable_task_tracker.h index 0a7289b..86450c3 100644 --- a/chrome/common/cancelable_task_tracker.h +++ b/chrome/common/cancelable_task_tracker.h @@ -28,6 +28,8 @@ // commonly used to cancel all outstanding tasks. // // 2. Both task and reply are deleted on the originating thread. +// +// 3. IsCanceledCallback is thread safe and can be run or deleted on any thread. #ifndef CHROME_COMMON_CANCELABLE_TASK_TRACKER_H_ #define CHROME_COMMON_CANCELABLE_TASK_TRACKER_H_ @@ -53,6 +55,8 @@ class CancelableTaskTracker { typedef int64 TaskId; static const TaskId kBadTaskId; + typedef base::Callback IsCanceledCallback; + CancelableTaskTracker(); // Cancels all tracked tasks. @@ -67,6 +71,14 @@ class CancelableTaskTracker { const base::Closure& task, const base::Closure& reply); + // Creates a tracked TaskId and an associated IsCanceledCallback. Client can + // later call TryCancel() with the returned TaskId, and run |is_canceled_cb| + // to check whether the TaskId is canceled. + // + // Note. This function is used to address some special cancelation requirement + // in existing code. You SHOULD NOT need this function in new code. + TaskId NewTrackedTaskId(IsCanceledCallback* is_canceled_cb); + // After calling this function, |task| and |reply| will not run. If the // cancelation happens when |task| is running or has finished running, |reply| // will not run. If |reply| is running or has finished running, cancellation diff --git a/chrome/common/cancelable_task_tracker_unittest.cc b/chrome/common/cancelable_task_tracker_unittest.cc index 60315bc..c6a7a85 100644 --- a/chrome/common/cancelable_task_tracker_unittest.cc +++ b/chrome/common/cancelable_task_tracker_unittest.cc @@ -67,12 +67,14 @@ class CancelableTaskTrackerTest : public testing::Test { virtual void TearDown() { UnblockTaskThread(); - // Create tracker on client thread. + // Destroy tracker on client thread. WaitableEvent tracker_destroyed(true, false); client_thread_runner_->PostTask( FROM_HERE, Bind(&CancelableTaskTrackerTest::DestroyTrackerOnClientThread, Unretained(this), &tracker_destroyed)); + + // This will also wait for any pending tasks on client thread. tracker_destroyed.Wait(); client_thread_->Stop(); @@ -112,6 +114,13 @@ class CancelableTaskTrackerTest : public testing::Test { &test_data_, event); } + Closure IncreaseTestDataIfNotCanceledAndSignalClosure( + const CancelableTaskTracker::IsCanceledCallback& is_canceled_cb, + WaitableEvent* event) { + return Bind(&CancelableTaskTrackerTest::IncreaseDataIfNotCanceledAndSignal, + &test_data_, is_canceled_cb, event); + } + Closure DecreaseTestDataClosure(WaitableEvent* event) { return Bind(&CancelableTaskTrackerTest::DecreaseData, Owned(new WaitableEventScoper(event)), &test_data_); @@ -134,6 +143,16 @@ class CancelableTaskTrackerTest : public testing::Test { event->Signal(); } + static void IncreaseDataIfNotCanceledAndSignal( + int* data, + const CancelableTaskTracker::IsCanceledCallback& is_canceled_cb, + WaitableEvent* event) { + if (!is_canceled_cb.Run()) + (*data)++; + if (event) + event->Signal(); + } + static void DecreaseData(WaitableEventScoper* event_scoper, int* data) { (*data) -= 2; } @@ -145,6 +164,7 @@ class CancelableTaskTrackerTest : public testing::Test { }; #if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)) && GTEST_HAS_DEATH_TEST + typedef CancelableTaskTrackerTest CancelableTaskTrackerDeathTest; TEST_F(CancelableTaskTrackerDeathTest, PostFromDifferentThread) { @@ -325,4 +345,52 @@ TEST_F(CancelableTaskTrackerTest, TrackerDestructedAfterTask) { EXPECT_EQ(1, test_data_); } +void CheckTrackedTaskIdOnSameThread_Test(CancelableTaskTrackerTest* test, + WaitableEvent* event) { + CancelableTaskTracker::IsCanceledCallback is_canceled_cb; + test->task_id_ = test->tracker_->NewTrackedTaskId(&is_canceled_cb); + ASSERT_NE(CancelableTaskTracker::kBadTaskId, test->task_id_); + + EXPECT_FALSE(is_canceled_cb.Run()); + + test->tracker_->TryCancel(test->task_id_); + EXPECT_TRUE(is_canceled_cb.Run()); + + test->task_id_ = test->tracker_->NewTrackedTaskId(&is_canceled_cb); + EXPECT_FALSE(is_canceled_cb.Run()); + + // Destroy tracker will cancel all tasks. + test->tracker_.reset(); + EXPECT_TRUE(is_canceled_cb.Run()); + + event->Signal(); +} + +TEST_F(CancelableTaskTrackerTest, CheckTrackedTaskIdOnSameThread) { + RunOnClientAndWait(&CheckTrackedTaskIdOnSameThread_Test); +} + +void CheckTrackedTaskIdOnDifferentThread_Test(CancelableTaskTrackerTest* test, + WaitableEvent* event) { + CancelableTaskTracker::IsCanceledCallback is_canceled_cb; + test->task_id_ = test->tracker_->NewTrackedTaskId(&is_canceled_cb); + ASSERT_NE(CancelableTaskTracker::kBadTaskId, test->task_id_); + + // Post task to task thread. + test->task_thread_runner_->PostTask( + FROM_HERE, + test->IncreaseTestDataIfNotCanceledAndSignalClosure(is_canceled_cb, + event)); + is_canceled_cb.Reset(); // So the one in task thread runner is the last ref, + // and will be destroyed on task thread. + + test->tracker_->TryCancel(test->task_id_); + test->UnblockTaskThread(); +} + +TEST_F(CancelableTaskTrackerTest, CheckTrackedTaskIdOnDifferentThread) { + RunOnClientAndWait(&CheckTrackedTaskIdOnDifferentThread_Test); + EXPECT_EQ(0, test_data_); +} + } // namespace -- cgit v1.1