Count site data size for important sites

chrome/browser/engagement/important_sites_usage_counter.cc

Index: chrome/browser/engagement/important_sites_usage_counter.cc
diff --git a/chrome/browser/engagement/important_sites_usage_counter.cc b/chrome/browser/engagement/important_sites_usage_counter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a8dc0e5b5ab61ef8b355a127d2473aad5e725bf5
--- /dev/null
+++ b/chrome/browser/engagement/important_sites_usage_counter.cc
@@ -0,0 +1,101 @@
+// Copyright 2017 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 "chrome/browser/engagement/important_sites_usage_counter.h"
+

[dominickn, 2017/04/11 00:18:20]

Nit: #include <utility> for std::move

[dullweber, 2017/04/11 09:01:34]

Done.

+#include "base/bind_helpers.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/dom_storage_context.h"
+
+using ImportantDomainInfo = ImportantSitesUtil::ImportantDomainInfo;
+
+ImportantSitesUsageCounter::ImportantSitesUsageCounter(
+    std::vector<ImportantDomainInfo> sites,
+    storage::QuotaManager* quota_manager,
+    content::DOMStorageContext* dom_storage_context,
+    UsageCallback callback)
+    : callback_(callback),
+      sites_(std::move(sites)),
+      quota_manager_(quota_manager),
+      dom_storage_context_(dom_storage_context),
+      tasks_(-1) {
+  for (ImportantDomainInfo& site : sites_) {

[dominickn, 2017/04/11 00:18:20]

Nit: no braces for 1 line conditional

[dullweber, 2017/04/11 09:01:34]

Done.

+    site.usage = 0;
+  }
+}
+
+ImportantSitesUsageCounter::~ImportantSitesUsageCounter() {}
+
+void ImportantSitesUsageCounter::RunAndDestroySelf() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  tasks_ = 1;  // Task for this method
+  tasks_ += 1;
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&ImportantSitesUsageCounter::GetQuotaUsageOnIOThread,
+                 base::Unretained(this)));
+  tasks_ += 1;
+  dom_storage_context_->GetLocalStorageUsage(
+      base::Bind(&ImportantSitesUsageCounter::ReceiveLocalStorageUsage,
+                 base::Unretained(this)));
+  Done();
+}
+
+void ImportantSitesUsageCounter::GetQuotaUsageOnIOThread() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  quota_manager_->GetUsageInfo(
+      base::Bind(&ImportantSitesUsageCounter::ReceiveQuotaUsageOnIOThread,
+                 base::Unretained(this)));
+}
+
+void ImportantSitesUsageCounter::ReceiveQuotaUsageOnIOThread(
+    const std::vector<storage::UsageInfo>& usage_infos) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  content::BrowserThread::PostTask(

[dominickn, 2017/04/11 00:18:20]

I'm a bit concerned about the thread lifetime here. |usage_infos| is a member on
the GetUsageInfoTask within QuotaManager, which lives on the IO thread. I think
it's definitely possible for that GetUsageInfoTask to be deleted (since it's on
the IO thread) before ReceiveQuotaUsage() is posted and executed on the UI
thread. That'll be a use-after-free.

For example, the only other use of GetUsageInfo() is
chrome/browser/storage/storage_info_fetcher.cc, which copies the usage_infos
into a separate vector and posts to the UI thread using the separate vector.
That avoids thread lifetime issues.

Can you do the same here? I think it's preferable to copy than to block the IO
thread with your own processing here.

[dullweber, 2017/04/11 09:01:34]

Thanks for pointing that out. I just read through the callback documentation to
better understand how the different kinds of arguments work and I think passing
a const vector<>& should be safe because the callback will store a copy of it? 
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass...
and
https://www.chromium.org/developers/design-documents/threading#How_arguments_...

[dominickn, 2017/04/11 10:03:52]

I don't think that doc applies here because the usage_info argument isn't bound
in the callback (i.e. you don't have closure =
base::Bind(&ImportantSitesUsageCounter::ReceiveQuotaUsageOnIOThread,
base::Unretained(this), usage_info) followed by closure.Run() somewhere. That
would be okay.

Instead, it's passed in directly at
https://cs.chromium.org/chromium/src/storage/browser/quota/quota_manager.cc?s...
(|callback_| is your callback object in there). So this method is directly
invoked with a reference to the underlying GetUsageInfoTask's vector, so I think
you definitely have a threading lifetime issue here.

[dullweber, 2017/04/11 11:08:03]

I agree that the usage_infos parameter in ReceiveQuotaUsageOnIOThread is a
reference to the |entries_| member of GetUsageInfoTask but I think that the
vector is copied by using base::Bind() and ReceiveQuotaUsage will get a
reference to the copy:
base::Bind(&ImportantSitesUsageCounter::ReceiveQuotaUsage,
           base::Unretained(this), usage_infos)

The GetUsageInfoTask can't disappear while ReceiveQuotaUsageOnIOThread is called
because it is a synchronous call and ReceiveQuotaUsageOnIOThread will make the
copy using base::Bind().

+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&ImportantSitesUsageCounter::ReceiveQuotaUsage,
+                 base::Unretained(this), usage_infos));
+}
+
+void ImportantSitesUsageCounter::ReceiveQuotaUsage(
+    const std::vector<storage::UsageInfo>& usage_infos) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  for (const storage::UsageInfo& info : usage_infos)

[dominickn, 2017/04/11 00:18:20]

Nit: this loop needs braces since the body is more than one line.

[dullweber, 2017/04/11 09:01:34]

Done.

+    IncrementUsage(
+        ImportantSitesUtil::GetRegisterableDomainOrIPFromHost(info.host),
+        info.usage);
+  Done();
+}
+
+void ImportantSitesUsageCounter::ReceiveLocalStorageUsage(
+    const std::vector<content::LocalStorageUsageInfo>& storage_infos) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  for (const content::LocalStorageUsageInfo& info : storage_infos)

[dominickn, 2017/04/11 00:18:20]

Nit: this one needs braces because the body is 2 lines.

[dullweber, 2017/04/11 09:01:34]

Done.

+    IncrementUsage(ImportantSitesUtil::GetRegisterableDomainOrIP(info.origin),
+                   info.data_size);
+  Done();
+}
+
+// Look up the corresponding ImportantDomainInfo for |url| and increase its
+// usage by |size|.
+void ImportantSitesUsageCounter::IncrementUsage(const std::string& domain,
+                                                int64_t size) {
+  // Use a linear search over sites_ because it only has up to 10 entries.
+  auto it = std::find_if(sites_.begin(), sites_.end(),
+                         [domain](ImportantDomainInfo& info) {
+                           return info.registerable_domain == domain;
+                         });
+  if (it != sites_.end())
+    it->usage += size;
+}
+
+void ImportantSitesUsageCounter::Done() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK_GT(tasks_, 0);
+  if (--tasks_ == 0) {
+    callback_.Run(std::move(sites_));
+    base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
+  }
+}