New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 699567 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

[Cronet] Error out on unrecognized experimental options

Project Member Reported by xunji...@chromium.org, Mar 8 2017

Issue description

Cronet has a way to set a JSON string as experimental options. In addition to log what this JSON string is, we can error out or show a warning on experimental options that are not recognized. 

This will make debugging easier as it's less likely for the native and client Java code to not be in sync.

 

Comment 1 by mef@chromium.org, Mar 8 2017

I think error out in *debug* maybe ok, but in release we should at most warn, as it is expected that experimental options could change or even go away over time.
I ended up adding some warnings and dumping the "effective" experimental options to NetLog: https://codereview.chromium.org/2738813004/
It's cool that we will also get other NetLog tabs for free.


Screenshot from 2017-03-08 14:58:03.png
41.2 KB View Download
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c275e0a2333511ec831b3c396dc9f38a161a8c6

commit 3c275e0a2333511ec831b3c396dc9f38a161a8c6
Author: xunjieli <xunjieli@chromium.org>
Date: Wed Mar 15 19:11:12 2017

[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}

[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/android/BUILD.gn
[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/android/cronet_url_request_context_adapter.h
[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java
[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/url_request_context_config.h
[modify] https://crrev.com/3c275e0a2333511ec831b3c396dc9f38a161a8c6/components/cronet/url_request_context_config_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f

commit 2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f
Author: cblume <cblume@chromium.org>
Date: Thu Mar 16 12:43:50 2017

Revert of [Cronet] Write effective experimental options to NetLog (patchset #5 id:80001 of https://codereview.chromium.org/2738813004/ )

Reason for revert:
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%20Cronet%20Builder%20Asan/builds/1791

Original issue's description:
> [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/+/3c275e0a2333511ec831b3c396dc9f38a161a8c6

TBR=kapishnikov@chromium.org,mgersh@chromium.org,xunjieli@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 584806 , 699567 

Review-Url: https://codereview.chromium.org/2751113004
Cr-Commit-Position: refs/heads/master@{#457408}

[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/android/BUILD.gn
[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/android/cronet_url_request_context_adapter.h
[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java
[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/url_request_context_config.h
[modify] https://crrev.com/2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f/components/cronet/url_request_context_config_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d67295ebc58b086ac493e26fe8e4498c09ffc21e

commit d67295ebc58b086ac493e26fe8e4498c09ffc21e
Author: xunjieli <xunjieli@chromium.org>
Date: Thu Mar 16 21:05:41 2017

[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}

[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/android/BUILD.gn
[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/android/cronet_url_request_context_adapter.h
[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java
[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/url_request_context_config.h
[modify] https://crrev.com/d67295ebc58b086ac493e26fe8e4498c09ffc21e/components/cronet/url_request_context_config_unittest.cc

Sign in to add a comment