Keep per-app GUID for WebView Metrics

android_webview/browser/aw_metrics_service_client.cc

Index: android_webview/browser/aw_metrics_service_client.cc
diff --git a/android_webview/browser/aw_metrics_service_client.cc b/android_webview/browser/aw_metrics_service_client.cc
index 16622b0b3cf5f91ce1da07520bcf869f91bd2f97..fc8545f44819c5f5656f0790f0eef0c2e16a7de1 100644
--- a/android_webview/browser/aw_metrics_service_client.cc
+++ b/android_webview/browser/aw_metrics_service_client.cc
@@ -6,10 +6,13 @@
 
 #include "android_webview/common/aw_version_info_values.h"
 #include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/guid.h"
 #include "base/i18n/rtl.h"
 #include "base/prefs/pref_service.h"
 #include "components/metrics/call_stack_profile_metrics_provider.h"
 #include "components/metrics/gpu/gpu_metrics_provider.h"
+#include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/metrics_service.h"
 #include "components/metrics/metrics_state_manager.h"
 #include "components/metrics/net/net_metrics_log_uploader.h"
@@ -36,6 +39,29 @@ scoped_ptr<metrics::ClientInfo> LoadClientInfo() {
   return client_info;
 }
 
+// A GUID in text form is composed of 32 hex digits and 4 hyphens.
+const size_t GUID_SIZE = 32 + 4;

[sgurun-gerrit only, 2016/01/12 20:33:35]

himm, did not really like this since it creates a invisible dependency to GUID
size and there is no trust issue with the file anyway. so you could read the
whole file as well. but won't insist changing it since the GUID format will not
change soon.

[paulmiller, 2016/01/12 20:53:02]

Chrome actually uses "GUID" to mean "UUID", for which the 36-character format is
fairly standard:
https://en.wikipedia.org/wiki/Universally_unique_identifier
I agree it would be better to not have this constant here in the first place,
but base/guid doesn't make any alternative available.

I agree that maliciously breaking WebView by writing a giant file here is
unlikely, but not all breakages are malicious. I consider it good defensive
programming to limit file read sizes wherever a hard limit is already defined.

[sgurun-gerrit only, 2016/01/12 21:18:21]

yea but the format of everything changes by time. when the format changes,
especially with no test coverage like this, things break in a unexpected way
(and such breakages happen all the time). The better way is probably defining a
constant at the site where they generate GUID and use it making the dependency
explicit. but I am still ok with what we have here.

+
+void ReadOrWriteGUID(const base::FilePath guid_file_path, std::string* guid) {

[sgurun-gerrit only, 2016/01/12 20:33:35]

rename to GetOrCreateGUID

[paulmiller, 2016/01/12 20:53:02]

will do

+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+
+  // Try to read an existing GUID.
+  if (base::ReadFileToString(guid_file_path, guid, GUID_SIZE)) {
+    if (base::IsValidGUID(*guid))
+      return;
+    else
+      LOG(ERROR) << "Found invalid GUID";
+  }
+
+  // We must write a new GUID.
+  *guid = base::GenerateGUID();
+  if (!base::WriteFile(guid_file_path, guid->c_str(), guid->size()))
+    LOG(ERROR) << "Failed to write new GUID";
+  return;
+}
+
+std::string g_returned_guid;

[sgurun-gerrit only, 2016/01/12 20:33:35]

you are using this only for returning a value from the functor. Instead of this,
dynamically allocate a string in call site, and pass the pointer of this method
to Reply method in postTaskandReply as a base::owned, so it destroys.

[paulmiller, 2016/01/12 20:53:02]

will do

[paulmiller, 2016/01/12 21:03:40]

I guess I'm also using it to pass the value from Initialize to
InitializeWithGUID. So I think it has to at least be a private member, if not a
global. Would you prefer that? It seems ugly either way.

[sgurun-gerrit only, 2016/01/12 21:13:15]

yep, that is the only reason you use it.
Nop, not need to be a class member, just dynamically creating and passing as a
pointer is good.

+
 }  // namespace
 
 // static
@@ -43,15 +69,33 @@ AwMetricsServiceClient* AwMetricsServiceClient::GetInstance() {
   return g_lazy_instance_.Pointer();
 }
 
-// static
 void AwMetricsServiceClient::Initialize(
     PrefService* pref_service,
-    net::URLRequestContextGetter* request_context) {
+    net::URLRequestContextGetter* request_context,
+    const base::FilePath guid_file_path) {
   DCHECK(!is_initialized_);
 
   pref_service_ = pref_service;
   request_context_ = request_context;
 
+  // Initialization happens on the UI thread, but getting the GUID should happen
+  // on the file I/O thread. So we start to initialize, then post to get the
+  // GUID, and then pick up where we left off, back on the UI thread, in
+  // InitializeWithGUID.
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&ReadOrWriteGUID, guid_file_path, &g_returned_guid),
+      base::Bind(&AwMetricsServiceClient::InitializeWithGUID,
+                 base::Unretained(this)));
+}
+
+void AwMetricsServiceClient::InitializeWithGUID() {
+  DCHECK(!is_initialized_);
+
+  pref_service_->SetString(metrics::prefs::kMetricsClientID,
+                           g_returned_guid);
+
   metrics_state_manager_ = metrics::MetricsStateManager::Create(
       pref_service_, base::Bind(&AwMetricsServiceClient::is_reporting_enabled,
                                 base::Unretained(this)),