Appcache::OnCorruptionDetected handling

webkit/browser/appcache/appcache_service.cc

Index: webkit/browser/appcache/appcache_service.cc
diff --git a/webkit/browser/appcache/appcache_service.cc b/webkit/browser/appcache/appcache_service.cc
index e951ad018a23e7cb85ef0dc56702f5268c0e6569..12b9d904b70d6a8eec5339094ad33ced1a7d2f19 100644
--- a/webkit/browser/appcache/appcache_service.cc
+++ b/webkit/browser/appcache/appcache_service.cc
@@ -425,7 +425,6 @@ void AppCacheService::CheckResponseHelper::OnReadDataComplete(int result) {
   delete this;
 }
 
-
 // AppCacheStorageReference ------
 
 AppCacheStorageReference::AppCacheStorageReference(
@@ -439,8 +438,7 @@ AppCacheService::AppCacheService(quota::QuotaManagerProxy* quota_manager_proxy)
     : appcache_policy_(NULL), quota_client_(NULL), handler_factory_(NULL),
       quota_manager_proxy_(quota_manager_proxy),
       request_context_(NULL),
-      force_keep_session_state_(false),
-      was_reinitialized_(false) {
+      force_keep_session_state_(false) {
   if (quota_manager_proxy_.get()) {
     quota_client_ = new AppCacheQuotaClient(this);
     quota_manager_proxy_->RegisterClient(quota_client_);
@@ -473,13 +471,35 @@ void AppCacheService::Initialize(const base::FilePath& cache_directory,
   storage_.reset(storage);
 }
 
-void AppCacheService::Reinitialize() {
-  AppCacheHistograms::CountReinitAttempt(was_reinitialized_);
-
-  // To avoid thrashing, we only do this once.
-  if (was_reinitialized_)
+void AppCacheService::ScheduleReinitialize() {
+  if (reinit_timer_.IsRunning())
     return;
-  was_reinitialized_ = true;
+
+  // Reinitialization only happens when corruption has been noticed.
+  // We don't want to thrash the disk but we also don't want to
+  // leave the appcache disabled for an indefinite period of time. Some
+  // users never shutdown the browser.
+
+  const base::TimeDelta kZeroDelta;
+  const base::TimeDelta kOneHour(base::TimeDelta::FromHours(1));
+  const base::TimeDelta k30Seconds(base::TimeDelta::FromSeconds(30));
+
+  // If enough time has past since the last reinit, reset the delay.

[jsbell, 2014/01/24 21:13:44]

This seems set up to handle very frequent corruption detections (e.g. multiple
times an hour). Should we have a counter e.g. "# of reinit requests w/in one
hour" or something, so we can determine if some systems are prone to this
behavior?

+  if (next_reinit_delay_ != kZeroDelta &&
+      base::Time::Now() - last_reinit_time_ > kOneHour) {
+    next_reinit_delay_ = kZeroDelta;
+  }
+  reinit_timer_.Start(FROM_HERE, next_reinit_delay_,
+                      this, &AppCacheService::Reinitialize);
+
+  // Adjust the delay for next time.
+  base::TimeDelta increment = std::max(k30Seconds, next_reinit_delay_);
+  next_reinit_delay_ = std::min(next_reinit_delay_ + increment,  kOneHour);

[jsbell, 2014/01/24 21:13:44]

I don't think this is strictly necessary; if the timer has grown to > 1 hour
then now - last_reinit_time_ should also be > 1 hour, so the above test should
have reset the delay to 0.

Won't hurt, though.

[michaeln, 2014/01/28 22:12:16]

Thnx for questioning, this doesn't do what i want it to do, I'll have a new cut
at it soon.

[michaeln, 2014/01/28 22:21:36]

Actually i think the logic was doing what i want, but i made it a little clearer
what's going on, I hope clear enough to see why this std::min call does need to
be there.

+}
+
+void AppCacheService::Reinitialize() {
+  AppCacheHistograms::CountReinitAttempt(!last_reinit_time_.is_null());
+  last_reinit_time_ = base::Time::Now();
 
   // Inform observers of about this and give them a chance to
   // defer deletion of the old storage object.