[Cronet] Write effective experimental options to NetLog

[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806, 699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2738813004
Cr-Original-Commit-Position: refs/heads/master@{#457158}
Committed: https://chromium.googlesource.com/chromium/src/+/3c275e0a2333511ec831b3c396dc9f38a161a8c6
Review-Url: https://codereview.chromium.org/2738813004
Cr-Commit-Position: refs/heads/master@{#457550}
Committed: https://chromium.googlesource.com/chromium/src/+/d67295ebc58b086ac493e26fe8e4498c09ffc21e

[xunjieli, 2017-03-08 19:50:32]

Description was changed from

==========
[Cronet] Write effective experimental options to NetLog

This CL changes the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=699567
==========

to

==========
[Cronet] Write effective experimental options to NetLog

This CL changes the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-03-08 19:52:13]

xunjieli@chromium.org changed reviewers:
+ kapishnikov@chromium.org, mgersh@chromium.org

[xunjieli, 2017-03-08 19:52:14]

PTAL.

[xunjieli, 2017-03-08 19:54:17]

Description was changed from

==========
[Cronet] Write effective experimental options to NetLog

This CL changes the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Write effective experimental options to NetLog

This CL changes the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-03-08 20:06:29]

Description was changed from

==========
[Cronet] Write effective experimental options to NetLog

This CL changes the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-03-13 18:31:52]

lgtm, and sorry for the long delay. Thanks for fixing this!

https://codereview.chromium.org/2738813004/diff/20001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/2738813004/diff/20001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:87: // false if
it fails to open log file.
The comment about return value is out of date now.

[xunjieli, 2017-03-14 13:22:25]

Thanks, Miriam. 
Andrei: Do you want to take a look?

https://codereview.chromium.org/2738813004/diff/20001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/2738813004/diff/20001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:87: // false if
it fails to open log file.
On 2017/03/13 18:31:52, mgersh wrote:
> The comment about return value is out of date now.

Done.

[kapishnikov, 2017-03-14 14:37:09]

Sorry Helen for the late review. It looks good although I am not very familiar
with the netlog logging logic. A couple of comments below. LGTM

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:1011:
base::ScopedFILE file(base::OpenFile(file_path, "w"));
The same file can be opened for writing twice before the |file| variable goes
out of scope: once here, and once in
net::FileNetLogObserver::CreateUnbounded(...). I guess it is okay, just to
double check.

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:299: LOG(ERROR) << "Unrecognized
Cronet experimental option \"" << it.key()
Should we change the error to warning? It could be possible that some
experimental options will be removed in the future, so the new implementation
doesn't have them anymore.

[xunjieli, 2017-03-14 19:03:06]

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:1011:
base::ScopedFILE file(base::OpenFile(file_path, "w"));
On 2017/03/14 14:37:09, kapishnikov wrote:
> The same file can be opened for writing twice before the |file| variable goes
> out of scope: once here, and once in
> net::FileNetLogObserver::CreateUnbounded(...). I guess it is okay, just to
> double check.

Yes, it's okay. But on second thought, let's move this back to the original
method and keep the method signature to return false upon open failure. Done.

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:299: LOG(ERROR) << "Unrecognized
Cronet experimental option \"" << it.key()
On 2017/03/14 14:37:09, kapishnikov wrote:
> Should we change the error to warning? It could be possible that some
> experimental options will be removed in the future, so the new implementation
> doesn't have them anymore.

This doesn't introduce crash. I think this one is an error (comparable to
Log.e() in Android) that clients should take care of.

[xunjieli, 2017-03-14 19:03:26]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 19:04:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 20:09:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 20:09:52]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[kapishnikov, 2017-03-14 22:39:07]

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:299: LOG(ERROR) << "Unrecognized
Cronet experimental option \"" << it.key()
On 2017/03/14 19:03:06, xunjieli wrote:
> On 2017/03/14 14:37:09, kapishnikov wrote:
> > Should we change the error to warning? It could be possible that some
> > experimental options will be removed in the future, so the new
implementation
> > doesn't have them anymore.
> 
> This doesn't introduce crash. I think this one is an error (comparable to
> Log.e() in Android) that clients should take care of. 

There can be a scenario when the implementation is updated independently from
the client code. The implementation may not be necessary packaged inside the app
bundle but downloaded from somewhere else and updated independently after the
release of the app. If the implementation is updated to a version that does not
have the experimental option anymore, the app will suddenly start logging the
error but there is nothing that the developer can do. That is why I think that
the logging with the warning level might be better than the error one.

https://codereview.chromium.org/2738813004/diff/60001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2738813004/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:278:
throw new RuntimeException("Unable to start NetLog");
I think it is possible for an app in production to have a user preference that
enables netlog. A user can be asked to turn that preference on in order to
diagnose an issue. I think we should not throw an exception (crash the app) if
the logging cannot be started for some reason.

[xunjieli, 2017-03-14 23:10:46]

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2738813004/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:299: LOG(ERROR) << "Unrecognized
Cronet experimental option \"" << it.key()
On 2017/03/14 22:39:07, kapishnikov wrote:
> On 2017/03/14 19:03:06, xunjieli wrote:
> > On 2017/03/14 14:37:09, kapishnikov wrote:
> > > Should we change the error to warning? It could be possible that some
> > > experimental options will be removed in the future, so the new
> implementation
> > > doesn't have them anymore.
> > 
> > This doesn't introduce crash. I think this one is an error (comparable to
> > Log.e() in Android) that clients should take care of. 
> 
> There can be a scenario when the implementation is updated independently from
> the client code. The implementation may not be necessary packaged inside the
app
> bundle but downloaded from somewhere else and updated independently after the
> release of the app. If the implementation is updated to a version that does
not
> have the experimental option anymore, the app will suddenly start logging the
> error but there is nothing that the developer can do. That is why I think that
> the logging with the warning level might be better than the error one.

Done.

https://codereview.chromium.org/2738813004/diff/60001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2738813004/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:278:
throw new RuntimeException("Unable to start NetLog");
On 2017/03/14 22:39:07, kapishnikov wrote:
> I think it is possible for an app in production to have a user preference that
> enables netlog.

No. Enabling NetLog is too heavy for production. It should only be used during
debugging.

>  A user can be asked to turn that preference on in order to
> diagnose an issue. I think we should not throw an exception (crash the app) if
> the logging cannot be started for some reason.

This was added in crbug.com/497860. I didn't change the behavior in this CL.

[kapishnikov, 2017-03-15 05:01:55]

lgtm

[xunjieli, 2017-03-15 15:33:21]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 15:33:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 16:27:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 16:27:18]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-03-15 19:05:58]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-03-15 19:05:59]

The patchset sent to the CQ was uploaded after l-g-t-m from mgersh@chromium.org
Link to the patchset: https://codereview.chromium.org/2738813004/#ps80001
(title: "Change LOG(ERROR) to LOG(WARNING)")

[commit-bot: I haz the power, 2017-03-15 19:06:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 19:11:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489604758835650,
"parent_rev": "b437e9336734feedcf529fac96cb21e7fa031a9a", "commit_rev":
"3c275e0a2333511ec831b3c396dc9f38a161a8c6"}

[commit-bot: I haz the power, 2017-03-15 19:11:58]

Description was changed from

==========
[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2738813004
Cr-Commit-Position: refs/heads/master@{#457158}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c275e0a2333511ec831b3c396dc...
==========

[commit-bot: I haz the power, 2017-03-15 19:12:06]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3c275e0a2333511ec831b3c396dc...

[cblume, 2017-03-16 05:14:14]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2751113004/ by cblume@chromium.org.

The reason for reverting is: The bot

Android Cronet Builder Asan

is reporting

Failed steps failed cronet_unittests failed cronet_test_instrumentation_apk

I suspect this patch might be why.


https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20Cr....

[xunjieli, 2017-03-16 17:42:28]

Description was changed from

==========
[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2738813004
Cr-Commit-Position: refs/heads/master@{#457158}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c275e0a2333511ec831b3c396dc...
==========

to

==========
[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2738813004
Cr-Commit-Position: refs/heads/master@{#457158}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c275e0a2333511ec831b3c396dc...
==========

[xunjieli, 2017-03-16 17:44:06]

Miriam and Andrei: PTAL at diff btw #5 and #6. I thought DictionaryValue
supports Remove() while iteration, but it doesn't. This led to the
use-after-free bug as seen on asan bot.

[kapishnikov, 2017-03-16 19:55:33]

lgtm

[xunjieli, 2017-03-16 19:55:47]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 19:56:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-03-16 20:33:37]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-03-16 20:33:41]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-03-16 20:33:42]

The patchset sent to the CQ was uploaded after l-g-t-m from mgersh@chromium.org
Link to the patchset: https://codereview.chromium.org/2738813004/#ps100001
(title: "Fix use-after-free bug")

[commit-bot: I haz the power, 2017-03-16 20:34:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 21:05:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489696421963030,
"parent_rev": "f2be81a465a7b2ef07b3667f83de1009206acace", "commit_rev":
"d67295ebc58b086ac493e26fe8e4498c09ffc21e"}

[commit-bot: I haz the power, 2017-03-16 21:06:32]

Description was changed from

==========
[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2738813004
Cr-Commit-Position: refs/heads/master@{#457158}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c275e0a2333511ec831b3c396dc...
==========

to

==========
[Cronet] Write effective experimental options to NetLog

This CL does the following:
(1) Have a warning in Experimental Options parsing code if there is an
    unrecognized option.
(2) Make cronet_url_request_context_adapter use the new FileNetLogObserver which
    allows writing polled data.
(3) Put the effective experimental options in the polled data.

BUG=584806,699567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2738813004
Cr-Original-Commit-Position: refs/heads/master@{#457158}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c275e0a2333511ec831b3c396dc...
Review-Url: https://codereview.chromium.org/2738813004
Cr-Commit-Position: refs/heads/master@{#457550}
Committed:
https://chromium.googlesource.com/chromium/src/+/d67295ebc58b086ac493e26fe8e4...
==========

[commit-bot: I haz the power, 2017-03-16 21:06:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d67295ebc58b086ac493e26fe8e4...