Keep per-app GUID for WebView Metrics

Keep per-app GUID for WebView Metrics

BUG=546754
Committed: https://crrev.com/94da14d7b72429eef5fadd87bb10e17f94c3735c
Cr-Commit-Position: refs/heads/master@{#369213}

[paulmiller, 2016-01-08 01:58:22]

Description was changed from

==========
Keep per-app GUID for WebView Metrics

BUG=546754
==========

to

==========
Keep per-app GUID for WebView Metrics

BUG=546754
==========

[paulmiller, 2016-01-08 01:58:22]

paulmiller@chromium.org changed reviewers:
+ sgurun@chromium.org

[paulmiller, 2016-01-08 01:58:36]

On 2016/01/08 01:58:22, paulmiller wrote:
> mailto:paulmiller@chromium.org changed reviewers:
> + mailto:sgurun@chromium.org

PTAL

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

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:43: const size_t GUID_SIZE
= 32 + 4;
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.

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:45: void
ReadOrWriteGUID(const base::FilePath guid_file_path, std::string* guid) {
rename to GetOrCreateGUID

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:63: std::string
g_returned_guid;
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]

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:43: const size_t GUID_SIZE
= 32 + 4;
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.

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:45: void
ReadOrWriteGUID(const base::FilePath guid_file_path, std::string* guid) {
will do

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:63: std::string
g_returned_guid;
will do

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

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:63: std::string
g_returned_guid;
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]

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:63: std::string
g_returned_guid;
On 2016/01/12 21:03:40, paulmiller wrote:
> I guess I'm also using it to pass the value from Initialize to
> InitializeWithGUID. 
yep, that is the only reason you use it.

>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.

Nop, not need to be a class member, just dynamically creating and passing as a
pointer is good.

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

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1563093005/diff/20001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:43: const size_t GUID_SIZE
= 32 + 4;
On 2016/01/12 20:53:02, paulmiller wrote:
> 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.

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.

[paulmiller, 2016-01-13 00:58:06]

uploaded #3.

I've discovered 3 problems:
-There's a race between Finalize and InitializeWithGUID.
-Finalize never actually gets called.
-We don't actually need Finalize.
The solution is to remove Finalize, which I'll do in a follow-up change, as well
as clarify some of the threading comments in here.

[sgurun-gerrit only, 2016-01-13 17:27:51]

lgtm

[paulmiller, 2016-01-13 17:28:29]

The CQ bit was checked by paulmiller@chromium.org

[commit-bot: I haz the power, 2016-01-13 17:29:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1563093005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1563093005/40001

[commit-bot: I haz the power, 2016-01-13 17:56:37]

Description was changed from

==========
Keep per-app GUID for WebView Metrics

BUG=546754
==========

to

==========
Keep per-app GUID for WebView Metrics

BUG=546754
==========

[commit-bot: I haz the power, 2016-01-13 17:56:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-13 17:58:25]

Description was changed from

==========
Keep per-app GUID for WebView Metrics

BUG=546754
==========

to

==========
Keep per-app GUID for WebView Metrics

BUG=546754

Committed: https://crrev.com/94da14d7b72429eef5fadd87bb10e17f94c3735c
Cr-Commit-Position: refs/heads/master@{#369213}
==========

[commit-bot: I haz the power, 2016-01-13 17:58:28]

Patchset 3 (id:??) landed as
https://crrev.com/94da14d7b72429eef5fadd87bb10e17f94c3735c
Cr-Commit-Position: refs/heads/master@{#369213}