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

webkit/quota/quota_manager.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webkit/quota/quota_manager.h"
 
 #include <algorithm>
 #include <deque>
 #include <set>
 
@@ skipping 102 matching lines @@
       QuotaManager* manager,
       const std::string& host,
       StorageType type)
       : QuotaTask(manager),
         host_(host),
         type_(type),
         quota_(-1),
         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 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();
         ++iter) {
       (*iter)->Run(kQuotaErrorAbort, 0, 0);
       delete *iter;
     }
     callbacks_.clear();
     DeleteSoon();
   }
 
   virtual void Completed() OVERRIDE {
     DeleteSoon();
   }
+
   QuotaManager* manager() const {
     return static_cast<QuotaManager*>(observer());
   }
 
   std::string host() const { return host_; }
   StorageType type() const { return type_; }
   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();
           ++iter) {
         DispatchCallback(*iter);
         delete *iter;
       }
       callbacks_.clear();
       UsageAndQuotaDispatcherTaskMap& dispatcher_map =
           manager()->usage_and_quota_dispatchers_;
       DCHECK(dispatcher_map.find(std::make_pair(host_, type_)) !=
              dispatcher_map.end());
       dispatcher_map.erase(std::make_pair(host_, type_));
       CallCompleted();
     }
   }
 
   const std::string host_;
   const StorageType type_;
   int64 quota_;
   int64 global_usage_;
   int64 host_usage_;
   QuotaStatusCode quota_status_;
   CallbackList callbacks_;
+  int waiting_callbacks_;
   ScopedCallbackFactory<UsageAndQuotaDispatcherTask> callback_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(UsageAndQuotaDispatcherTask);
 };
 
 class QuotaManager::UsageAndQuotaDispatcherTaskForTemporary
     : public QuotaManager::UsageAndQuotaDispatcherTask {
  public:
   UsageAndQuotaDispatcherTaskForTemporary(
       QuotaManager* manager, const std::string host)
       : UsageAndQuotaDispatcherTask(manager, host, kStorageTypeTemporary) {}
 
  protected:
-  virtual void Run() OVERRIDE {
+  virtual void RunBody() OVERRIDE {
     manager()->temporary_usage_tracker_->GetGlobalUsage(
         NewGlobalUsageCallback());
     manager()->temporary_usage_tracker_->GetHostUsage(
         host(), NewHostUsageCallback());
     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);
   }
 };
 
 class QuotaManager::UsageAndQuotaDispatcherTaskForPersistent
     : public QuotaManager::UsageAndQuotaDispatcherTask {
  public:
   UsageAndQuotaDispatcherTaskForPersistent(
       QuotaManager* manager, const std::string host)
       : 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());
   }
 };
 
 // static
 QuotaManager::UsageAndQuotaDispatcherTask*
 QuotaManager::UsageAndQuotaDispatcherTask::Create(
     QuotaManager* manager, const std::string& host, StorageType type) {
   switch (type) {
     case kStorageTypeTemporary:
       return new UsageAndQuotaDispatcherTaskForTemporary(
@@ skipping 811 matching lines @@
 
 QuotaManagerProxy::QuotaManagerProxy(
     QuotaManager* manager, base::MessageLoopProxy* io_thread)
     : manager_(manager), io_thread_(io_thread) {
 }
 
 QuotaManagerProxy::~QuotaManagerProxy() {
 }
 
 }  // namespace quota