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

Issue 799616 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Reporting / Network Error Logging: Expose to Cronet.

Project Member Reported by juliatut...@chromium.org, Jan 5 2018

Issue description

Cronet embedders should be able to enable Network Error Logging (and the Reporting API it uses) for their apps.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 19 2018

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

commit 89fd5b15de4ff3392b92dcf333078c98dca3a361
Author: Julia Tuttle <juliatuttle@chromium.org>
Date: Fri Jan 19 22:45:48 2018

Network Error Logging: Move base::Feature check to NetworkContext.

Cronet doesn't support controlling base::Features, so:
1. For Chromium, move the base::Feature check to NetworkContext.
2. For Cronet, add an experimental feature.

TBR=mkwst@chromium.org

Bug:  799616 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I5baab77588d8f1ef9d292febca826777820b6a23
Reviewed-on: https://chromium-review.googlesource.com/858176
Commit-Queue: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530641}
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/chrome/browser/profiles/off_the_record_profile_io_data.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/chrome/browser/profiles/profile_browsertest.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/chrome/browser/profiles/profile_impl_io_data.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/content/network/network_context.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/content/network/network_context_unittest.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/content/shell/browser/shell_url_request_context_getter.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/net/BUILD.gn
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/net/network_error_logging/network_error_logging_end_to_end_test.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/net/network_error_logging/network_error_logging_service.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/net/network_error_logging/network_error_logging_service_unittest.cc
[delete] https://crrev.com/70bcf1b946ece006b4dc6a71fbddeaca14ae4ee5/net/reporting/reporting_feature.cc
[delete] https://crrev.com/70bcf1b946ece006b4dc6a71fbddeaca14ae4ee5/net/reporting/reporting_feature.h
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/net/reporting/reporting_service.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/net/reporting/reporting_service.h
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/services/network/public/cpp/BUILD.gn
[add] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/services/network/public/cpp/network_features.cc
[add] https://crrev.com/89fd5b15de4ff3392b92dcf333078c98dca3a361/services/network/public/cpp/network_features.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 19 2018

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

commit d07dc85e2df0d41705c51511fc9d383cc94b6c49
Author: Julia Tuttle <juliatuttle@chromium.org>
Date: Fri Jan 19 23:06:43 2018

Reporting API / Network Error Logging: Make available to Cronet.

Bug:  799616 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ic05966416c1f82dd593d061cccc9ac0a2728b588
Reviewed-on: https://chromium-review.googlesource.com/853172
Commit-Queue: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530650}
[modify] https://crrev.com/d07dc85e2df0d41705c51511fc9d383cc94b6c49/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/d07dc85e2df0d41705c51511fc9d383cc94b6c49/components/cronet/url_request_context_config_unittest.cc

Comment 3 by kbr@chromium.org, Jan 21 2018

89fd5b15de4ff3392b92dcf333078c98dca3a361 seems to have caused this persistent test failure in content_browsertests:
https://ci.chromium.org/buildbot/chromium.mac/Mac10.9%20Tests%20%28dbg%29/49160

I attempted a revert here:
https://chromium-review.googlesource.com/877801

but there's now a merge conflict.

Please investigate. Thanks.

Looking now!
Okay, somehow, base::FeatureList::IsFeatureEnabled is getting called with two different pointers to the single base::Feature I create here:

https://cs.chromium.org/chromium/src/services/network/public/cpp/network_features.cc?sq=package:chromium&l=9

I'm trying to figure out where the two addresses came from (they're clearly different parts of the address space).
Cc: mmenke@chromium.org
Okay, apparently one copy is in content_browsertests, and one copy is in libcontent.so.

Poking through some other similar feature, it looks like may be caused by incorrectly handling the visibility of features::kReporting (and presumably also features::kNetworkErrorLogging) -- other features, like kUkmFeature, are properly defined in only one object file (and referenced by others), but kReporting is defined in several.

I'm going to try adding a NETWORK_EXPORT sort of thing to //services/network to see if it fixes it.
...on the other hand, //services/metrics builds as a component, while //services/network builds as a static_library. We'll see if it helps or not.
Cc: kbr@chromium.org
Okay, looks like that helps. Put up https://chromium-review.googlesource.com/c/chromium/src/+/877260 to review fix.

Comment 9 by mastiz@chromium.org, Jan 22 2018

Labels: -Pri-3 Pri-1
Friendly ping for juliatuttle@: is there a status update or ETA for landing the fix? Thx!
Status update, sure: it works everywhere but fails to build in the windows component build.
ETA, kind of: it depends when I manage to sort that out.

If you'd like it fixed sooner, I can land this with the relevant code disabled on Windows for now.
Thx! I would recommend against a partial fix, since the most vocal failures in the waterwall are Window tests.

Depending on how complicated this gets, and considering the revert has been ruled out above, I think we should consider disabling the affected tests temporarily.
juliatuttle@: can you try the way to fix the revert merge conflict & land the revert the CL instead? At this points, there are lots of tests affected & I think the best course of action is to revert your CL.
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 22 2018

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

commit 6fe983b59a17f40da9a51872a0f298fdbf8df4ff
Author: Julia Tuttle <juliatuttle@chromium.org>
Date: Mon Jan 22 19:29:49 2018

Reporting / Network Error Logging: Rollback several changes.

The first change of three I landed caused a test failure that somehow
wasn't caught by the commit queue, so roll back all three for now.

TBR=mmenke@chromium.org,bbauer@chromium.org,mef@chromium.org,mgersh@chromium.org

Bug:  799616 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id7558f96157bae31967248c851e11981335deb5c
Reviewed-on: https://chromium-review.googlesource.com/879041
Commit-Queue: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530948}
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/chrome/browser/profiles/off_the_record_profile_io_data.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/chrome/browser/profiles/profile_browsertest.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/chrome/browser/profiles/profile_impl_io_data.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/components/cronet/url_request_context_config_unittest.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/content/network/network_context.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/content/network/network_context_unittest.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/content/shell/browser/shell_url_request_context_getter.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/BUILD.gn
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/network_error_logging/network_error_logging_end_to_end_test.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/network_error_logging/network_error_logging_service.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/network_error_logging/network_error_logging_service_unittest.cc
[add] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/reporting/reporting_feature.cc
[add] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/reporting/reporting_feature.h
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/reporting/reporting_service.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/reporting/reporting_service.h
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/6fe983b59a17f40da9a51872a0f298fdbf8df4ff/services/network/public/cpp/BUILD.gn
[delete] https://crrev.com/11e3785d164fa33a85b91f5d6386873f5e2b148a/services/network/public/cpp/network_features.cc
[delete] https://crrev.com/11e3785d164fa33a85b91f5d6386873f5e2b148a/services/network/public/cpp/network_features.h

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 26 2018

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

commit 91a655d210d4672bc6d84cb5b24866256a8d305e
Author: Julia Tuttle <juliatuttle@chromium.org>
Date: Fri Jan 26 18:03:03 2018

Network Error Logging: Move base::Feature check to NetworkContext.

Cronet doesn't support controlling base::Features, so move the
base::Feature to //services/network (in a tiny component, because things
break otherwise), and move the feature check itself to NetworkContext.

(A future change will add an experimental option to Cronet to enable
Reporting and NEL.)

This is a fixed re-land of https://chromium-review.googlesource.com/c/chromium/src/+/858176.

Bug:  799616 
Change-Id: I79c8a011c75d6b79a92f9ffe85bf5d53d509dcbc
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/879441
Commit-Queue: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532006}
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/chrome/browser/profiles/off_the_record_profile_io_data.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/chrome/browser/profiles/profile_browsertest.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/chrome/browser/profiles/profile_impl_io_data.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/chrome/test/BUILD.gn
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/content/network/BUILD.gn
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/content/network/network_context.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/content/network/network_context_unittest.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/content/shell/BUILD.gn
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/content/shell/browser/shell_url_request_context_getter.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/content/test/BUILD.gn
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/net/BUILD.gn
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/net/network_error_logging/network_error_logging_end_to_end_test.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/net/network_error_logging/network_error_logging_service.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/net/network_error_logging/network_error_logging_service_unittest.cc
[delete] https://crrev.com/fc3f45c02e43760981314c95ffeaca5ffaebebc4/net/reporting/reporting_feature.cc
[delete] https://crrev.com/fc3f45c02e43760981314c95ffeaca5ffaebebc4/net/reporting/reporting_feature.h
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/net/reporting/reporting_service.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/net/reporting/reporting_service.h
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/services/network/public/cpp/BUILD.gn
[add] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/services/network/public/cpp/features.cc
[add] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/services/network/public/cpp/features.h
[add] https://crrev.com/91a655d210d4672bc6d84cb5b24866256a8d305e/services/network/public/cpp/features_export.h

Project Member

Comment 15 by bugdroid1@chromium.org, Jan 29 2018

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

commit 9715d16410805f7511b68d7860cc5d06c4576251
Author: Julia Tuttle <juliatuttle@chromium.org>
Date: Mon Jan 29 17:02:26 2018

Reporting API / Network Error Logging: Make available to Cronet.

This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/853172
which was rolled back as part of https://chromium-review.googlesource.com/c/chromium/src/+/879041.

Bug:  799616 
Change-Id: I5dae7cd41b55dd2151a06e0f708743cd19b85b41
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/881401
Commit-Queue: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532465}
[modify] https://crrev.com/9715d16410805f7511b68d7860cc5d06c4576251/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/9715d16410805f7511b68d7860cc5d06c4576251/components/cronet/url_request_context_config_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment