QuotaManager::GetUsageAndQuota should not hung when the returned usage is negative

webkit/quota/quota_manager.cc

Index: webkit/quota/quota_manager.cc
diff --git a/webkit/quota/quota_manager.cc b/webkit/quota/quota_manager.cc
index 2d24e0b5b9b3ab718a55c9d0e378232f5dfe6102..b6266b4c6d1d54f983143ebae5fb02a93b3c152a 100644
--- a/webkit/quota/quota_manager.cc
+++ b/webkit/quota/quota_manager.cc
@@ -120,16 +120,25 @@ class QuotaManager::UsageAndQuotaDispatcherTask : public QuotaTask {
         global_usage_(-1),
         host_usage_(-1),
         quota_status_(kQuotaStatusUnknown),
+        waiting_callbacks_(1),
         callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
 
   virtual ~UsageAndQuotaDispatcherTask() {
     STLDeleteContainerPointers(callbacks_.begin(), callbacks_.end());
   }
 
-  virtual bool IsCompleted() const = 0;
-
+  // Subclasses must implement them.
+  virtual void RunBody() = 0;
   virtual void DispatchCallback(GetUsageAndQuotaCallback* callback) = 0;
 
+  virtual void Run() OVERRIDE {
+    RunBody();
+    // We initialize waiting_callbacks to 1 so that we won't run
+    // the completion callback until here even some of the callbacks
+    // are dispatched synchronously.
+    CheckCompleted();
+  }
+
   virtual void Aborted() OVERRIDE {
     for (CallbackList::iterator iter = callbacks_.begin();
         iter != callbacks_.end();
@@ -144,6 +153,7 @@ class QuotaManager::UsageAndQuotaDispatcherTask : public QuotaTask {
   virtual void Completed() OVERRIDE {
     DeleteSoon();
   }
+
   QuotaManager* manager() const {
     return static_cast<QuotaManager*>(observer());
   }
@@ -153,31 +163,35 @@ class QuotaManager::UsageAndQuotaDispatcherTask : public QuotaTask {
   int64 quota() const { return quota_; }
   int64 global_usage() const { return global_usage_; }
   int64 host_usage() const { return host_usage_; }
-  QuotaStatusCode status() const { return quota_status_; }
+  QuotaStatusCode quota_status() const { return quota_status_; }
 
   UsageCallback* NewGlobalUsageCallback() {
+    ++waiting_callbacks_;

[Dai Mikurube (NOT FULLTIME), 2011/05/20 09:19:08]

Counting this up when generating looks a little strange for me.  How about when
calling GetGlobalUsage?  (The same for other functions.)

[kinuko, 2011/05/20 09:27:23]

GetGlobalUsage() etc is called in subclasses and I wanted to make the base class
provide some 'automatic' mechanism for them (but I know what you mean).

Alternatively I could add a protected method like AddWaitingCallback() that just
increments waiting_callbacks_ and make subclasses call it each time it makes
additional request that takes callback.  I thought this might look a bit
redundant but wdyt.

[Dai Mikurube (NOT FULLTIME), 2011/05/23 02:16:11]

I guess you mean adding AddWaitingCallback(), for example, after (or before)
    manager()->temporary_usage_tracker_->GetGlobalUsage(
        NewGlobalUsageCallback());
in RunBody() of sub classes like UsageAndQuotaDispatcherTaskForTemporary.

I agree with it.  It's more reasonable than the current code.

[kinuko, 2011/05/23 03:31:54]

That was what I meant when I wrote the previous comment but instead I did
following in patch 3:
- kept the '++waiting_callbacks' code in NewXxxCallback method
- renamed NewXxxCallback to NewWaitableXxxCallback to explicitly indicate that
they are the methods that create a new waiting point (i.e. creates a new
callback which decrements waiting_callbacks when it is called).  I also added a
comment about that.  ptal thanks.

     return callback_factory_.NewCallback(
             &UsageAndQuotaDispatcherTask::DidGetGlobalUsage);
   }
 
   HostUsageCallback* NewHostUsageCallback() {
+    ++waiting_callbacks_;
     return callback_factory_.NewCallback(
             &UsageAndQuotaDispatcherTask::DidGetHostUsage);
   }
 
   QuotaCallback* NewGlobalQuotaCallback() {
+    ++waiting_callbacks_;
     return callback_factory_.NewCallback(
             &UsageAndQuotaDispatcherTask::DidGetGlobalQuota);
   }
 
   HostQuotaCallback* NewHostQuotaCallback() {
+    ++waiting_callbacks_;
     return callback_factory_.NewCallback(
             &UsageAndQuotaDispatcherTask::DidGetHostQuota);
   }
 
  private:
   void CheckCompleted() {
-    if (IsCompleted()) {
+    if (--waiting_callbacks_ <= 0) {
       // Dispatches callbacks.
       for (CallbackList::iterator iter = callbacks_.begin();
           iter != callbacks_.end();
@@ -202,6 +216,7 @@ class QuotaManager::UsageAndQuotaDispatcherTask : public QuotaTask {
   int64 host_usage_;
   QuotaStatusCode quota_status_;
   CallbackList callbacks_;
+  int waiting_callbacks_;
   ScopedCallbackFactory<UsageAndQuotaDispatcherTask> callback_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(UsageAndQuotaDispatcherTask);
@@ -215,7 +230,7 @@ class QuotaManager::UsageAndQuotaDispatcherTaskForTemporary
       : UsageAndQuotaDispatcherTask(manager, host, kStorageTypeTemporary) {}
 
  protected:
-  virtual void Run() OVERRIDE {
+  virtual void RunBody() OVERRIDE {
     manager()->temporary_usage_tracker_->GetGlobalUsage(
         NewGlobalUsageCallback());
     manager()->temporary_usage_tracker_->GetHostUsage(
@@ -223,16 +238,12 @@ class QuotaManager::UsageAndQuotaDispatcherTaskForTemporary
     manager()->GetTemporaryGlobalQuota(NewGlobalQuotaCallback());
   }
 
-  virtual bool IsCompleted() const OVERRIDE {
-    return (quota() >= 0 && global_usage() >= 0 && host_usage() >= 0);
-  }
-
   virtual void DispatchCallback(GetUsageAndQuotaCallback* callback) OVERRIDE {
     // TODO(kinuko): For now it returns pessimistic quota.  Change this
     // to return {usage, quota - nonevictable_usage} once eviction is
     // supported.
     int64 other_usage = global_usage() - host_usage();
-    callback->Run(status(), host_usage(), quota() - other_usage);
+    callback->Run(quota_status(), host_usage(), quota() - other_usage);
   }
 };
 
@@ -244,19 +255,15 @@ class QuotaManager::UsageAndQuotaDispatcherTaskForPersistent
       : UsageAndQuotaDispatcherTask(manager, host, kStorageTypePersistent) {}
 
  protected:
-  virtual void Run() OVERRIDE {
+  virtual void RunBody() OVERRIDE {
     manager()->persistent_usage_tracker_->GetHostUsage(
         host(), NewHostUsageCallback());
     manager()->GetPersistentHostQuota(
         host(), NewHostQuotaCallback());
   }
 
-  virtual bool IsCompleted() const OVERRIDE {
-    return (quota() >= 0 && host_usage() >= 0);
-  }
-
   virtual void DispatchCallback(GetUsageAndQuotaCallback* callback) OVERRIDE {
-    callback->Run(status(), host_usage(), quota());
+    callback->Run(quota_status(), host_usage(), quota());
   }
 };