Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846873002:
Network traffic annotation added to URLFetcher. (Closed)
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 7 months ago Reviewers:
asanka CC:
Jialiu Lin, battre, cbentzel+watch_chromium.org, chromium-reviews, estark, msramek, net-reviews_chromium.org, meacer, Randy Smith (Not in Mondays) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to URLFetcher.
net/url_request/test_url_fetcher_factory.cc
net/url_request/url_fetcher.cc
net/url_request/url_fetcher_core.h
net/url_request/url_fetcher_core.cc
net/url_request/url_fetcher_impl.h
net/url_request/url_fetcher_impl.cc
net/url_request/url_fetcher_impl_unittest.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2846873002
Cr-Commit-Position: refs/heads/master@{#468991}
Committed: https://chromium.googlesource.com/chromium/src/+/3afb47b6452c43df9f5ad6b645040f483706ed3a
Patch Set 1 #
Total comments: 4
Patch Set 2 : Annotations moved. #Patch Set 3 : Unittests added. #Patch Set 4 : ios annotation added. #Patch Set 5 : None URLFetcher files removed. #Patch Set 6 : Missing header file added. #
Total comments: 2
Patch Set 7 : Comment addressed. #
Total comments: 4
Patch Set 8 : Comment addressed. #Patch Set 9 : Unnecessary const(s) removed. #
Messages
Total messages: 56 (39 generated)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rhalavati@chromium.org changed reviewers: + asanka@chromium.org
Asanka, Could you please pass this one as well to appropriate reviewer? Here is the intro: We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified 3 files in net/url_request and added annotation template to them. Please review them and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
Description was changed from ========== Network traffic annotation added to 3 files in net/url_request. Network traffic annotation is added to network requests of: net/url_request/report_sender.cc net/url_request/sdch_dictionary_fetcher.cc net/url_request/url_fetcher_core.cc BUG=656607 ========== to ========== Network traffic annotation added to 3 files in net/url_request. Network traffic annotation is added to network requests of: net/url_request/report_sender.cc net/url_request/sdch_dictionary_fetcher.cc net/url_request/url_fetcher_core.cc BUG=656607 ==========
asanka@chromium.org changed reviewers: + meacer@chromium.org, rdsmith@chromium.org
On 2017/04/27 14:01:10, Ramin Halavati wrote: > Asanka, > > Could you please pass this one as well to appropriate reviewer? > > Here is the intro: > We are annotating all network requests in Chromium with a new > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit > the requests and configure Chrome in a way that meets their security policies > and expectations. Furthermore, it allows us to generate better debugging data in > chrome://net-internals and measure bandwidth consumption on a per-request-type > basis. > > I've modified 3 files in net/url_request and added annotation template to them. > Please review them and suggest the required answers for the empty parts (marked > with '...'). Please note that the claims should be thorough and covering all > cases. In case you believe that annotation should be passed to the modified > function instead of originating from it, please tell me to change the CL > accordingly. > > Please take a look at the protobuf scheme in: > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > for the definition of the annotations. > > You can find a sample annotation in: > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > Entriprise policy options are here: > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > And more description on enterprise policy settings is here: > http://dev.chromium.org/administrators/policy-list-3 > > Please tell me if you need any clarifications from my side. If you want to learn > more, see: > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. meacer: Could you look into the annotation in report_sender.cc ? rdsmith: Could you look into sdch_dictionary_fetcher.cc ?
https://codereview.chromium.org/2846873002/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/2846873002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:577: original_url_, DEFAULT_PRIORITY, this, traffic_annotation); URLFetcherCore is just doing the work for URLFetcher. There's already a URLFetcher::Create() overload that takes a traffic annotation. That annotation currently isn't plumbed all the way down. It should be, so that it can be used here.
Ramin: Could you give me a sense as to what I'm supposed to do for this review? The annotation in sdch_dictionary_fetcher.cc naively looks like a template waiting to be filled in--if that's the case, can you point me at directions for filling it in? If that's not the case, can you give me guidance as to how I should review it? I understand SDCH, but I don't understand the structure of traffic annotations.
CC estark, jialiul: Can you please check my comments for net/url_request/report_sender.cc? https://codereview.chromium.org/2846873002/diff/1/net/url_request/report_send... File net/url_request/report_sender.cc (right): https://codereview.chromium.org/2846873002/diff/1/net/url_request/report_send... net/url_request/report_sender.cc:80: report_uri, DEFAULT_PRIORITY, this, traffic_annotation); * sender: "Safe Browsing Extended Reporting" However I'm not sure if SBER is the only user. estark: Do pinning failures also use SBER? * description: "Sends an anonymous report to a Google server about security issues". Or we can point them to the whitepaper: https://www.google.com/chrome/browser/privacy/whitepaper.html?hl=en-US#extend... * trigger: "A request can be sent when an SSL certificate error is encountered or SafeBrowsing detects a threat on a website" * data: "For SSL reports: the certificate that caused the error, hostname, user agent, et.c. For SafeBrowsing reports, .... For more information, see https://www.google.com/chrome/browser/privacy/whitepaper.html?hl=en-US#extend... * destination: GOOGLE_OWNED_SERVICE * cookies_allowed: false Needs to be confirmed: https://crbug.com/716098 * setting: "Users can disable these requests by unchecking 'Automatically report details of possible security incidents to Google' in the settings page or on SSL and SafeBrowsing error pages" * policy_exception_justification: "Not implemented" Not sure about this. estark, jialiul: Can you please confirm?
jialiul@chromium.org changed reviewers: + jialiul@chromium.org
https://codereview.chromium.org/2846873002/diff/1/net/url_request/report_send... File net/url_request/report_sender.cc (right): https://codereview.chromium.org/2846873002/diff/1/net/url_request/report_send... net/url_request/report_sender.cc:80: report_uri, DEFAULT_PRIORITY, this, traffic_annotation); On 2017/04/27 17:55:28, Mustafa Emre Acer wrote: > * sender: "Safe Browsing Extended Reporting" > > However I'm not sure if SBER is the only user. estark: Do pinning failures also > use SBER? > > * description: "Sends an anonymous report to a Google server about security > issues". > > Or we can point them to the whitepaper: > https://www.google.com/chrome/browser/privacy/whitepaper.html?hl=en-US#extend... > > * trigger: "A request can be sent when an SSL certificate error is encountered > or SafeBrowsing detects a threat on a website" > > * data: "For SSL reports: the certificate that caused the error, hostname, user > agent, et.c. For SafeBrowsing reports, .... For more information, see > https://www.google.com/chrome/browser/privacy/whitepaper.html?hl=en-US#extend... > > * destination: GOOGLE_OWNED_SERVICE > > * cookies_allowed: false > Needs to be confirmed: https://crbug.com/716098 > > * setting: "Users can disable these requests by unchecking 'Automatically report > details of possible security incidents to Google' in the settings page or on SSL > and SafeBrowsing error pages" > > * policy_exception_justification: "Not implemented" > > Not sure about this. estark, jialiul: Can you please confirm? I think the annotation should be added to each of its sub-classes. ReportSender is just a base class. It will be super confusing to elaborate on each case here.
https://codereview.chromium.org/2846873002/diff/1/net/url_request/report_send... File net/url_request/report_sender.cc (right): https://codereview.chromium.org/2846873002/diff/1/net/url_request/report_send... net/url_request/report_sender.cc:80: report_uri, DEFAULT_PRIORITY, this, traffic_annotation); On 2017/04/27 18:04:53, Jialiu Lin wrote: > On 2017/04/27 17:55:28, Mustafa Emre Acer wrote: > > * sender: "Safe Browsing Extended Reporting" > > > > However I'm not sure if SBER is the only user. estark: Do pinning failures > also > > use SBER? > > > > * description: "Sends an anonymous report to a Google server about security > > issues". > > > > Or we can point them to the whitepaper: > > > https://www.google.com/chrome/browser/privacy/whitepaper.html?hl=en-US#extend... > > > > * trigger: "A request can be sent when an SSL certificate error is encountered > > or SafeBrowsing detects a threat on a website" > > > > * data: "For SSL reports: the certificate that caused the error, hostname, > user > > agent, et.c. For SafeBrowsing reports, .... For more information, see > > > https://www.google.com/chrome/browser/privacy/whitepaper.html?hl=en-US#extend... > > > > * destination: GOOGLE_OWNED_SERVICE > > > > * cookies_allowed: false > > Needs to be confirmed: https://crbug.com/716098 > > > > * setting: "Users can disable these requests by unchecking 'Automatically > report > > details of possible security incidents to Google' in the settings page or on > SSL > > and SafeBrowsing error pages" > > > > * policy_exception_justification: "Not implemented" > > > > Not sure about this. estark, jialiul: Can you please confirm? > > I think the annotation should be added to each of its sub-classes. ReportSender > is just a base class. It will be super confusing to elaborate on each case here. > Yeah, I agree with Jialiu that it's infeasible to document every user of this class. net/ doesn't know anything about Safe Browsing, for example; it breaks layering to have it documented here.
Thank you very much for the comments. Jialiu, estark@: Is it good if I add the annotation to constructor of ReportSender? If not, where do you suggest. Randy: As you said this is just the template and I was asking help to fill the required info. Here is the full intro to annotation. Please tell me if I can help in any part of it: We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified 3 files in net/url_request and added annotation template to them. Please review them and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
Hi rhalavati, You probably don't need to annotate these three files at all, since they are all base classes. It would be much better to annotate their sub-classes (or where they are instantiated). What do you think?
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Network traffic annotation added to 3 files in net/url_request. Network traffic annotation is added to network requests of: net/url_request/report_sender.cc net/url_request/sdch_dictionary_fetcher.cc net/url_request/url_fetcher_core.cc BUG=656607 ========== to ========== Network traffic annotation added to URLFetcher. net/url_request/test_url_fetcher_factory.cc net/url_request/url_fetcher.cc net/url_request/url_fetcher_core.h net/url_request/url_fetcher_core.cc net/url_request/url_fetcher_impl.h net/url_request/url_fetcher_impl.cc net/url_request/url_fetcher_impl_unittest.cc BUG=656607 ==========
Patchset #5 (id:80001) has been deleted
rhalavati@chromium.org changed reviewers: - jialiul@chromium.org, meacer@chromium.org, rdsmith@chromium.org
Thank you very much, all comments addressed. As this CL very much expanded, I moved the changes related to 'SDCH Directory Fetcher' and 'ReportSender' to other CLs and kept URLFetcher updates here. Asanka, please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2846873002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_impl.cc (right): https://codereview.chromium.org/2846873002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_impl.cc:24: const net::NetworkTrafficAnnotationTag& traffic_annotation) why are all these NetworkTrafficAnnotationTarg& ? We don't need the extra level of indirection, right?
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Asanka, comment addressed, please review. Also I cannot understand why try-bots fail on this and ReportSender. Do you have any suggestions? https://codereview.chromium.org/2846873002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_impl.cc (right): https://codereview.chromium.org/2846873002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_impl.cc:24: const net::NetworkTrafficAnnotationTag& traffic_annotation) On 2017/05/02 20:38:59, asanka wrote: > why are all these NetworkTrafficAnnotationTarg& ? We don't need the extra level > of indirection, right? You are right, removed all.
Let's see how this dry run goes. https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... File net/url_request/url_fetcher_impl.cc (right): https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... net/url_request/url_fetcher_impl.cc:24: const net::NetworkTrafficAnnotationTag traffic_annotation) const unnecessary for pass-by-value (here and elsewhere). https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... File net/url_request/url_fetcher_impl.h (right): https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... net/url_request/url_fetcher_impl.h:39: const net::NetworkTrafficAnnotationTag traffic_annotation); const unnecessary.
oh and lgtm % nits
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Asanka, 'const's removed. https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... File net/url_request/url_fetcher_impl.cc (right): https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... net/url_request/url_fetcher_impl.cc:24: const net::NetworkTrafficAnnotationTag traffic_annotation) On 2017/05/03 14:45:44, asanka wrote: > const unnecessary for pass-by-value (here and elsewhere). Done. https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... File net/url_request/url_fetcher_impl.h (right): https://codereview.chromium.org/2846873002/diff/120002/net/url_request/url_fe... net/url_request/url_fetcher_impl.h:39: const net::NetworkTrafficAnnotationTag traffic_annotation); On 2017/05/03 14:45:45, asanka wrote: > const unnecessary. Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/2846873002/#ps170001 (title: "Unnecessary const(s) removed.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1493823539930590,
"parent_rev": "d69786293ebcd67df21ae765560289dfc065eb6d", "commit_rev":
"3afb47b6452c43df9f5ad6b645040f483706ed3a"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to URLFetcher. net/url_request/test_url_fetcher_factory.cc net/url_request/url_fetcher.cc net/url_request/url_fetcher_core.h net/url_request/url_fetcher_core.cc net/url_request/url_fetcher_impl.h net/url_request/url_fetcher_impl.cc net/url_request/url_fetcher_impl_unittest.cc BUG=656607 ========== to ========== Network traffic annotation added to URLFetcher. net/url_request/test_url_fetcher_factory.cc net/url_request/url_fetcher.cc net/url_request/url_fetcher_core.h net/url_request/url_fetcher_core.cc net/url_request/url_fetcher_impl.h net/url_request/url_fetcher_impl.cc net/url_request/url_fetcher_impl_unittest.cc BUG=656607 Review-Url: https://codereview.chromium.org/2846873002 Cr-Commit-Position: refs/heads/master@{#468991} Committed: https://chromium.googlesource.com/chromium/src/+/3afb47b6452c43df9f5ad6b64504... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:170001) as https://chromium.googlesource.com/chromium/src/+/3afb47b6452c43df9f5ad6b64504... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
