Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249213002:
[OBSOLETE] Reporting: Initial implementation. (Closed)
|
Created:
4 years, 4 months ago by Julia Tuttle Modified:
3 years, 9 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org, battre, vasilii Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReporting: Initial implementation.
This implements the draft Reporting API, as specified here:
http://wicg.github.io/reporting/
Here is a design doc:
https://docs.google.com/document/d/1uwNzFv7DltdYXgOSQFufFcnDzkJr9kugRSS2whhrN18/edit
Patch Set 1 #Patch Set 2 : splat #Patch Set 3 : splat #Patch Set 4 : splat #Patch Set 5 : Split into layered component, tons of other changes #Patch Set 6 : Start adding unittests #Patch Set 7 : Fix ProfileImplIOData #
Total comments: 22
Patch Set 8 : Make requested changes, add some comments. #Patch Set 9 : Don't hold endpoint pointers across deliveries; don't build content on iOS #Patch Set 10 : Fix android build failure. #Patch Set 11 : Fix a couple more issues found by the trybots. #Patch Set 12 : Export mojo interface #Patch Set 13 : Make several refactors. #Patch Set 14 : Get URLRequestContextGetter just-in-time. #Patch Set 15 : rebase #Patch Set 16 : Whole bunch of changes. #Patch Set 17 : rebase #
Total comments: 52
Patch Set 18 : Major revision #Patch Set 19 : Split out a bunch of code into other CLs. #Patch Set 20 : Try fixing unittest compile error on Android? #
Messages
Total messages: 88 (74 generated)
The CQ bit was checked by juliatuttle@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by juliatuttle@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: Exceeded global retry quota
The CQ bit was checked by juliatuttle@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by juliatuttle@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Reporting: Prototype. ========== to ========== Initial implementation of Reporting API This implements the draft Reporting API, as specified here: http://wicg.github.io/reporting/ Here is a design doc: https://docs.google.com/document/d/1uwNzFv7DltdYXgOSQFufFcnDzkJr9kugRSS2whhrN... ==========
juliatuttle@chromium.org changed reviewers: + rdsmith@chromium.org
Initial round of comments--very high level, mostly to make sure I'm understanding the context for the CL. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/content/browser/reporting_network_delegate.cc (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/content/browser/reporting_network_delegate.cc:62: FROM_HERE, base::Bind(ui_process_header_, origin, header_value)); So we're adding a thread hop for every response that we receive? Ow? Can we put in some kind of filter to reduce that cost? ETA: Ok, clearly there's something I'm missing. Why is this service on the UI thread at all? It seems a pretty pure IO (== net) concept. I understand that reports may be generated from the UI thread, but I'd think posting to the IO thread when we have a report is cheaper than posting to the UI thread on every response? https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/content/browser/reporting_network_delegate.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/content/browser/reporting_network_delegate.h:24: : public net::LayeredNetworkDelegate { nit: Call out with comment implementation of LayeredNetworkDelegate interface in below decls. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/content/browser/reporting_network_delegate.h:47: protected: Why? Nothing inherits from this, does it? (I'm pretty sure methods don't have to be declared at the same level of protection at different levels of the inheritance hierarchy, so I'm inclined to think this should be private. There's arguments that making public base class members private in a subclass is rude because it violates the "ISA" without providing any protection (you just cast and call). But that to my mind that doesn't apply to protected members, which folks holding a pointer to can't call anyway.) https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_cache.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_cache.h:18: // Section 2.4 I presume this is a reference into the spec, but I think files should be understandable without context, so I'd include the full reference. Also, a comment summarizing the goal/service this class is vending would be valuable (both for future readers and for your current reviewer :-}). Requiring people to read the spec to understand the code isn't completely friendly; there are times when it's the right move, but this doesn't strike me as one. "Storage for active endpoints and queued reports." would do it here, for me. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_cache.h:19: class REPORTING_EXPORT ReportingCache { Not actionable: From a purely code analysis POV, the functionality this class provides doesn't strike me as justifying a separate class. The queue of endpoints is conceptually separate from the queue of reports, and both strike me as pretty easily managed by just having an EndpointMap/ReportSet in whatever object owns this one. I do understand that the spec calls out the concept of a ReportingCache, but specs don't actually specify implementation (usually) and what I take that section as saying is "This is the information that the implementation is responsible for storing over time (between header reception, report queueing, and report delivery, specifically)." So as I do the rest of the review I may want to nuke this class and have a comment somewhere explaining how the Reporting Cache concept from the spec is implemented. Mentioning it now so that you can argue with the philosophy up front if you want. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_cache.h:29: const std::unique_ptr<ReportingEndpoint>* GetEndpoint(const GURL& url) const; Why return a const pointer to a std::unique_ptr<>? That would seem to say "Look but don't touch" but where is there about the owning pointer that's useful if you don't touch? I.e. why not return a const ReportingEndpoint*? I think of passing around unique_ptrs only when ownership is being transferred. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_manager.cc (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.cc:49: // visible? I'm trying to think what the right place would be. Developer console? Not sure how to do the plumbing, though. ETA: I think this is one of those "I want to know where we're eventually going to go, even if we don't get there in this CL" situations. In the long-term, how do you imagine exposing error reporting? https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.cc:76: goto next_report; Why not "break;"? https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.cc:76: goto next_report; I think this matching-and-exit algorithm means that it's not quite as simple as a group being per-origin. If b.c specifies an endpoint in group "CSP" with subdomains included, and a.b.c specifies a different endpoint in group "CSP" and asks for CSP errors to be sent to group CSP, those errors could get sent to either endpoint. I suspect that's what the spec wants, but I also don't feel like it's completely fair to call groups "per-endpoint" :-}; it's more that both origin and group have to match. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_manager.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Attaching a top-level comment at a random location: In your mind, what has to happen for this CL to land? The question I'm actually concerned about is: What has to happen before we turn this feature on? But it looks to me as if, if this CL landed, the feature would be up and running ("Now witness the power of ..." :-}). But there's a lot in the design doc you show as not yet implemented. What aspects of that block enabling this feature? And how are you going to avoid enabling this feature until those aspects are done (if you land this CL)? https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_report.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_report.h:25: GURL origin; There's a lack of context (well, other than the spec, but see my other comments) for what the above are; a bit more of a comment about what they mean and what the distinction between them is?
The CQ bit was checked by juliatuttle@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 ========== Initial implementation of Reporting API This implements the draft Reporting API, as specified here: http://wicg.github.io/reporting/ Here is a design doc: https://docs.google.com/document/d/1uwNzFv7DltdYXgOSQFufFcnDzkJr9kugRSS2whhrN... ========== to ========== Reporting: Initial implementation. This implements the draft Reporting API, as specified here: http://wicg.github.io/reporting/ Here is a design doc: https://docs.google.com/document/d/1uwNzFv7DltdYXgOSQFufFcnDzkJr9kugRSS2whhrN... ==========
PTAL, rdsmith. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/content/browser/reporting_network_delegate.cc (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/content/browser/reporting_network_delegate.cc:62: FROM_HERE, base::Bind(ui_process_header_, origin, header_value)); On 2016/10/21 20:15:10, Randy Smith - Not in Mondays wrote: > So we're adding a thread hop for every response that we receive? Ow? Can we > put in some kind of filter to reduce that cost? > > ETA: Ok, clearly there's something I'm missing. Why is this service on the UI > thread at all? It seems a pretty pure IO (== net) concept. I understand that > reports may be generated from the UI thread, but I'd think posting to the IO > thread when we have a report is cheaper than posting to the UI thread on every > response? We're adding a thread hop for every response that we receive with a Report-To header. I *could* move more things to the IO thread, but I recall a *lot* of hassle around profile creation/destruction and getting the URLRequestContextGetter when I did that in Domain Reliability. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/content/browser/reporting_network_delegate.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/content/browser/reporting_network_delegate.h:24: : public net::LayeredNetworkDelegate { On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > nit: Call out with comment implementation of LayeredNetworkDelegate interface in > below decls. Done. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/content/browser/reporting_network_delegate.h:47: protected: On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > Why? Nothing inherits from this, does it? > > (I'm pretty sure methods don't have to be declared at the same level of > protection at different levels of the inheritance hierarchy, so I'm inclined to > think this should be private. There's arguments that making public base class > members private in a subclass is rude because it violates the "ISA" without > providing any protection (you just cast and call). But that to my mind that > doesn't apply to protected members, which folks holding a pointer to can't call > anyway.) Done. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_cache.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_cache.h:18: // Section 2.4 On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > I presume this is a reference into the spec, but I think files should be > understandable without context, so I'd include the full reference. > > Also, a comment summarizing the goal/service this class is vending would be > valuable (both for future readers and for your current reviewer :-}). Requiring > people to read the spec to understand the code isn't completely friendly; there > are times when it's the right move, but this doesn't strike me as one. "Storage > for active endpoints and queued reports." would do it here, for me. > Done. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_cache.h:19: class REPORTING_EXPORT ReportingCache { On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > Not actionable: From a purely code analysis POV, the functionality this class > provides doesn't strike me as justifying a separate class. The queue of > endpoints is conceptually separate from the queue of reports, and both strike me > as pretty easily managed by just having an EndpointMap/ReportSet in whatever > object owns this one. I do understand that the spec calls out the concept of a > ReportingCache, but specs don't actually specify implementation (usually) and > what I take that section as saying is "This is the information that the > implementation is responsible for storing over time (between header reception, > report queueing, and report delivery, specifically)." So as I do the rest of > the review I may want to nuke this class and have a comment somewhere explaining > how the Reporting Cache concept from the spec is implemented. Mentioning it now > so that you can argue with the philosophy up front if you want. I will end up making the cache able to serialize and unserialize itself eventually, at which point it may not exist until later, and will have more code than just trivial map/set manipulation. That said, a part of me feels like flattening Reporting{Cache,Manager,Service} would simplify things quite a bit. Thoughts? https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_cache.h:29: const std::unique_ptr<ReportingEndpoint>* GetEndpoint(const GURL& url) const; On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > Why return a const pointer to a std::unique_ptr<>? That would seem to say "Look > but don't touch" but where is there about the owning pointer that's useful if > you don't touch? I.e. why not return a const ReportingEndpoint*? I think of > passing around unique_ptrs only when ownership is being transferred. I can't find a good way to erase from a vector<unique_ptr<T>> given a const T& in O(1) -- there's no way to compare the two. Am I missing something? (I hope I am.) https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_manager.cc (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.cc:49: // visible? On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > I'm trying to think what the right place would be. Developer console? Not sure > how to do the plumbing, though. > > ETA: I think this is one of those "I want to know where we're eventually going > to go, even if we don't get there in this CL" situations. In the long-term, how > do you imagine exposing error reporting? The DevTools console, most likely. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.cc:76: goto next_report; On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > I think this matching-and-exit algorithm means that it's not quite as simple as > a group being per-origin. If b.c specifies an endpoint in group "CSP" with > subdomains included, and a.b.c specifies a different endpoint in group "CSP" and > asks for CSP errors to be sent to group CSP, those errors could get sent to > either endpoint. I suspect that's what the spec wants, but I also don't feel > like it's completely fair to call groups "per-endpoint" :-}; it's more that both > origin and group have to match. ...I *don't* think that's what the spec wants. *I* want it to match the most specific origin. I'll add a comment as such, and file a spec issue for it. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.cc:76: goto next_report; On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > Why not "break;"? I think I was worried I'd end up breaking out of the wrong loop, but I think you're right, so done. https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_manager.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_manager.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > Attaching a top-level comment at a random location: In your mind, what has to > happen for this CL to land? > > The question I'm actually concerned about is: What has to happen before we turn > this feature on? But it looks to me as if, if this CL landed, the feature would > be up and running ("Now witness the power of ..." :-}). But there's a lot in > the design doc you show as not yet implemented. What aspects of that block > enabling this feature? And how are you going to avoid enabling this feature > until those aspects are done (if you land this CL)? These need to happen before we turn it on: rudimentary report scheduling, a cap on the number of endpoints/reports, histograms, Finch control and command-line flags, unit tests, a browser test. They don't have to happen in *this* CL, but if they don't, we'd need to nerf this CL so it doesn't enable the feature half-baked. (Finch control defaulting to off would do it.) These can happen in later CLs: persistence of endpoints across sessions, wiring to CSP or another client, fancier report scheduling, fancier garbage collection policy. These need to happen before beta/stable launch in parallel with development: normal launch review stuff (privacy, security, etc.) https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... File components/reporting/core/browser/reporting_report.h (right): https://codereview.chromium.org/2249213002/diff/120001/components/reporting/c... components/reporting/core/browser/reporting_report.h:25: GURL origin; On 2016/10/21 20:15:11, Randy Smith - Not in Mondays wrote: > There's a lack of context (well, other than the spec, but see my other comments) > for what the above are; a bit more of a comment about what they mean and what > the distinction between them is? Hmm. origin is basically always guaranteed to be url.GetOrigin(). I'll comment it for now. I'll file a spec issue to combine the fields. (Right now, the spec theoretically allows someone to queue a report with an origin of https://foo/bar and a URL of https://baz/quux.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by juliatuttle@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by juliatuttle@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by juliatuttle@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by juliatuttle@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 juliatuttle@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_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 juliatuttle@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 juliatuttle@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by juliatuttle@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by juliatuttle@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
battre@chromium.org changed reviewers: + sleevi@google.com
Ryan, can you please take a look this? My gut feeling is that our discussions about a headless usage of the profile's request context apply here even more than for the Credential Management API because errors are reported out of band. Julia: I'll let Ryan explain his concerns but the summary is IMO the following: By using the main request context without UI, you risk for example that an authorized proxy cannot request credentials (due to a lack of UI surface) and memorize that for future requests. Similar issues appear in the context of other security decision points.
On 2016/12/23 13:25:30, battre (OOO until Jan 2) wrote: > Ryan, can you please take a look this? My gut feeling is that our discussions > about a headless usage of the profile's request context apply here even more > than for the Credential Management API because errors are reported out of band. > > Julia: I'll let Ryan explain his concerns but the summary is IMO the following: > By using the main request context without UI, you risk for example that an > authorized proxy cannot request credentials (due to a lack of UI surface) and > memorize that for future requests. Similar issues appear in the context of other > security decision points. Does this mean that, in certain proxy configurations, if the user's session expires (or never existed), requests (to deliver reports) will fail until the user uses the UI to authenticate again? That's unfortunate, but I don't think it should stop us from delivering them in the many other cases where it would work. We could also detect that particular case and delay uploads until/unless the user reauthenticates to the proxy. Is there some other issue I'm missing? Is there a different URLRequestContext I should be using?
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org - sleevi@google.com
Holidays and all, but yes, we should not be using the profile's URL request context in the browser context here, outside of the //content renderer stack. I hadn't realized you were implementing the reporting API, otherwise I would have chimed up sooner. At the unfortunate risk of unnecessary delays here, we should set some time post no-meetings-week to sync up (ideally, with rdsmith@ and mmenke@) and go over the design doc to discuss some of these concerns and figure out an appropriate strategy. (Note, this affects not just proxies, but interactions like safe browsing, extensions, and fetch - and will definitely require care to get right)
On 2016/12/23 17:12:01, Julia Tuttle wrote: > On 2016/12/23 13:25:30, battre (OOO until Jan 2) wrote: > > Ryan, can you please take a look this? My gut feeling is that our discussions > > about a headless usage of the profile's request context apply here even more > > than for the Credential Management API because errors are reported out of > band. > > > > Julia: I'll let Ryan explain his concerns but the summary is IMO the > following: > > By using the main request context without UI, you risk for example that an > > authorized proxy cannot request credentials (due to a lack of UI surface) and > > memorize that for future requests. Similar issues appear in the context of > other > > security decision points. > > Does this mean that, in certain proxy configurations, if the user's session > expires (or never existed), requests (to deliver reports) will fail until the > user uses the UI to authenticate again? > > That's unfortunate, but I don't think it should stop us from delivering them in > the many other cases where it would work. We could also detect that particular > case and delay uploads until/unless the user reauthenticates to the proxy. > > Is there some other issue I'm missing? I think the primary issue isn't so much the delivery of the reports (which, in some sense, is the NER implementation's problem), but the possibility of "poisoning" the main profile with the results of the report. Specifically, if delivering the report requires authentication, the URLRequest::Delegate will be called to prompt the user with a authentication dialog. I haven't traced out what URLRequest::Delegate you're using (though I should), but generally if a request isn't directly associated with a renderer, the URLRequest::Delegate associated with that request just fails authentication requests (How would you prompt the user without a related user operation/web page? How would you explain to the user in the prompt what you were asking them to authenticate?). That failure gets recorded in an authentication cache associated with the URLRequestContext, and if the user later does something that would have resulted in a prompt, that prompt is bypassed because the "user" already signalled that they didn't want to or couldn't authenticate. The take-home of all this is that one shouldn't do state-changing operations on an profile's URLRequestContext unless you're sure those operations are integrated with the user prompting that normally goes on for that URLRequestContext. A simpler rule of thumb might be "If it doesn't come from the renderer, don't use the profile's URLRequestContext", though we're still well short of being able to make and trust such pithy recommendations. Ryan's suggested this for an upcoming eng-review meeting, so we'll probably be hashing through it and trying to come up with better overall recommendations soon. What are your thoughts about whether or not the SystemURLRequestContext would be appropriate here? If it isn't, that's unfortunate for this CL (as it'll be blocked on us figuring out what we should be doing in these contexts) but useful for the larger picture (because it'll help drive what the big picture recommendations should be). > > Is there a different URLRequestContext I should be using?
battre@chromium.org changed reviewers: + battre@chromium.org
+vasilii as this discussion affects him as well for the Credential Management API
On 2016/12/29 19:31:28, Randy Smith - Not in Mondays wrote: > What are your thoughts about whether or not the SystemURLRequestContext would be > appropriate here? If it isn't, that's unfortunate for this CL (as it'll be > blocked on us figuring out what we should be doing in these contexts) but useful > for the larger picture (because it'll help drive what the big picture > recommendations should be). Potential concerns with using the system context: 1. It might leak data between profiles. 2. It might leak data from private, proxied contexts (e.g. a company using a SPDY proxy as a VPN) to the Internet, although it will still require SSL for the endpoints. 3. It can't use the profile's proxy or authentication state, so it won't be able to report back to sites behind those. Specifically, here's how I'm worried 1 might look: 1. You're a sketchy ad network, and you want to correlate visits across profiles. 2. You set up a fake endpoint that always fails uploads. 3. On any request from a page, you configure that endpoint, adding your existing unique ID as a query parameter. 4. You set up a fake resource designed to generate reports, perhaps by knowingly violating its own CSP. 5. Somewhere convenient (a hidden nested iframe, perhaps), you generate a report for the configured origin with a nonce. 6. You get reports to each of the endpoints (since Reporting will try other ones if the first fails) with the nonce, telling you that those unique IDs all belong to the same user. Are we trying to guard against this kind of thing, or is it inevitable? As an alternative, could I continue using the Profile's context, but provide my own Delegate that simply *cancels* requests if authentication is requested? Would that avoid poisoning the credential cache?
On 2017/01/02 14:01:07, Julia Tuttle wrote: > On 2016/12/29 19:31:28, Randy Smith - Not in Mondays wrote: > > What are your thoughts about whether or not the SystemURLRequestContext would > be > > appropriate here? If it isn't, that's unfortunate for this CL (as it'll be > > blocked on us figuring out what we should be doing in these contexts) but > useful > > for the larger picture (because it'll help drive what the big picture > > recommendations should be). > > Potential concerns with using the system context: > > 1. It might leak data between profiles. > 2. It might leak data from private, proxied contexts (e.g. a company using a > SPDY proxy as a VPN) to the Internet, although it will still require SSL for the > endpoints. > 3. It can't use the profile's proxy or authentication state, so it won't be able > to report back to sites behind those. > > Specifically, here's how I'm worried 1 might look: > > 1. You're a sketchy ad network, and you want to correlate visits across > profiles. > 2. You set up a fake endpoint that always fails uploads. > 3. On any request from a page, you configure that endpoint, adding your existing > unique ID as a query parameter. > 4. You set up a fake resource designed to generate reports, perhaps by knowingly > violating its own CSP. > 5. Somewhere convenient (a hidden nested iframe, perhaps), you generate a report > for the configured origin with a nonce. > 6. You get reports to each of the endpoints (since Reporting will try other ones > if the first fails) with the nonce, telling you that those unique IDs all belong > to the same user. > > Are we trying to guard against this kind of thing, or is it inevitable? > > As an alternative, could I continue using the Profile's context, but provide my > own Delegate that simply *cancels* requests if authentication is requested? > Would that avoid poisoning the credential cache? I don't think we have that functionality, though it's beginning to sound like we need it.
FWIW, stepping back.. I think it's completely reasonable to rule out authenticated reporting endpoints - e.g. if the report request triggers an authentication flow, we drop it on the floor; it may also be reasonable to strip any cookies, etc.
On 2017/01/03 19:46:23, igrigorik wrote: > FWIW, stepping back.. I think it's completely reasonable to rule out > authenticated reporting endpoints - e.g. if the report request triggers an > authentication flow, we drop it on the floor; it may also be reasonable to strip > any cookies, etc. I appreciate your effort to finding a constructive resolution for this, but I don't think that suggestion works in a predictable platform. We should work to discuss this issue on the spec more before finalizing on any particular behaviour. While it's good to know your personal position, that doesn't particularly align with Fetch or other platform behaviours, and that significance is not worth downplaying.
On 2017/01/02 14:01:07, Julia Tuttle wrote: > 2. It might leak data from private, proxied contexts (e.g. a company using a > SPDY proxy as a VPN) to the Internet, although it will still require SSL for the > endpoints. For what it is worth, we've not (yet) stated that proxies are a privacy boundary, particularly because, given the Web Platform and Chrome's implementation, it is not. I can appreciate seeing that as a bug (or, conversely, as a feature), and I can appreciate not wanting to make it worse (if subjectively seen as bad), but it's at least worth calling out that proxies/network config do not represent a privacy boundary. Other aspects of the privacy boundary (caching, socket pools) are at least relevant, but the subtlety here was worth calling out.
A whole lot of comments that I think stand (even as we look at the URLRequest thing), but also may highlight things on the design doc to take a careful look at. https://codereview.chromium.org/2249213002/diff/320001/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/2249213002/diff/320001/net/net.gypi#newcode1241 net/net.gypi:1241: 'reporting/reporting_controls.cc', From a design perspective, could you explain more why you introduced a top-level directory called //reporting ? Given that the Reporting API is a Web Platform feature (e.g. not an intrinsic behaviour of an HTTP or networking client, spec'd as part of W3C/WICG), I can see a compelling argument that this belongs in //content instead (or possibly //third_party/WebKit). https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_controls.cc (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_controls.cc:52: return kDefaultEnabled; As mentioned in the header, this all seems like something base::FeatureList is meant to embody, and it would be good to know if that was intentionally avoided. This will also hopefully eliminate the need for HistogramEnabledState(), since such configs can be represented using the field trial config reporting already used. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_controls.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_controls.h:14: // default. 1) //net should never mention layers above it (in comments or code), because that's a layering violation. For example, it's impossible to evaluate what this code would do in the case of Chrome for iOS or in the case of Cronet - both of which have //net part of them. 2) From this header, it's not clear why a simple use of base::Feature wasn't used here? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_metrics.cc (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.cc:17: DCHECK_NE(HEADER_FATE_MAX, fate); All of these DCHECKs are arguably unnecessary I appreciate you're trying to guard against programmer error, but in C++, any possible integer may have been coerced into such an enum, leaving it in an undefined state. It would appear from these DCHECKs that you're trying to validate that the enumeration value is in range of the histogram, but that's not at all sufficient to do, and I'm not sure the programmer error you're attempting to guard against is reasonable/likely. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.cc:25: if (ttl > base::TimeDelta()) { This is subtle and under-documented (in your design or your header) as to why a negative TTL is not measured, even though you anticipated it enough to defend against it. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.cc:50: UMA_HISTOGRAM_COUNTS_100000("Reporting.DeliveryByteCount", byte_count); I'm very skeptical of a lot of these metrics, and don't see the design doc really showing how they'll be owned or influence the design. I appreciate and am sensitive to the fact that unless we measure from the get-go, we won't know, but I'd like to push back on each and every one of these measurements to make sure we know precisely why we're going to measure them and what we anticipate using them for, so that we only collect what is needed to influence the feature and the user experience. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_metrics.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.h:12: enum EnabledState { 1) Do all of these need to be in "public" headers? Ideally, no. 2) Document - none of these states are self-obvious https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.h:67: void HistogramEnabledState(EnabledState state); naming/design: It seems not at all self-obvious when reading code and seeing a call to net::HistogramEnabledState(ENABLED_STATE_ENABLED_DEFAULT) is going to do, documentation or not. It seems like much of this may be better served being grouped in a logical namespace (net::reporting)? In thinking very carefully how //net will be used in the future, does this API benefit from splitting (like QUIC to some extent, and like //content exclusively) into the notion of "public" and "private" implementation parts? It seems like this bleeds a lot into a namespace without very clear naming. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_report.cc (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.cc:11: ReportingReport::ReportingReport() {} = default; https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_report.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:15: struct ReportingReport { From a design standpoint, this feels more like a class (or will evolve into one) than a struct, per https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:18: ~ReportingReport(); Rule of three applies here - in general, if you provide a dtor, you should provide copy and assignment. It would seem you're not doing so here because you're only concerned about squelching warnings about complex ctors/dtors (and the code-size implication they have), but the same applies here to copy/assignment. Further, the copy/assignment is complex (because of line 22) - so it deserves either a DISALLOW_COPY_AND_ASSIGN (which further deflates the "is-a struct" argument) or explicit management (which I'm unclear of what the right answer is) https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:20: std::string ToString() const; From reading the header: Why does this exist? What does this do? Is it for debugging? GTEst? Is it the serializing of the class? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:22: std::unique_ptr<base::Value> body; What is this? Why is it a base::Value versus another structure? Does anything guarantee any form of consistency (or non-mutability)? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:23: // URL of content that triggered report. nelines between 22-23 https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:27: // separately, so they are separate fields for now. Line break between 24-25 and 29-30 As comments go, I think this leaves more questions than answers. For example, is there a bug filed on this? Why does the spec allow callers to separate them? What's an example of when you should or should not? What happens if origin isn't valid at all - what do you do? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:33: base::TimeTicks timestamp; naming: "timestamp" is self-describing without adding new information. Should this be base::TimeTicks creation_time? Does this require monotonicity? Civil time conversion? How is this used? All unclear. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:35: int attempts; size_t for counts - https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:37: bool pending; This feels like an interface bleed - why should the report keep track of whether it itself is pending? This seems to leak in details about how the report is used, rather than what a report is. The same applies to |attempts|. Both of these seem to represent a state about-a report, not state that is-a report. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_service.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:26: class NET_EXPORT ReportingService { High level Design remarks: 1) This feels like a lot of the implementation details have bled into the public header - meaning anytime the implementatino changes, that's sort of spread through to every consumer. Concrete Suggestions: a) Forward declare whenever possible b) Carefully evaluate whether you need a private class member function versus a function within an unnamed namespace c) Carefully evaluate whether this class is doing "too much" 2) The testing seems to be litering *ForTesting functions in to the public interface. While a presubmit helps minimize that, it feels like it might result in a more carefully balanced operation if you make use of text fixtures, rather than public interface. a) Similarly, if you're needing that much of your "implementation detail" to be provided via ForTesting() methods, it may be a sign that your layering or design is trying to do too much, and would benefit from more decoupling. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:50: static Policy GetDefault(); DESIGN: Why is an explicit GetDefault required? Why can't default constructing the object return a sane default? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:56: void set_uploader(std::unique_ptr<ReportingUploader> uploader); This seems to have more subtlety than just "set_uploader" https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:65: void set_clock_for_testing(std::unique_ptr<base::TickClock> clock); SetClockForTesting https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:73: struct Client { Why can't you forward declare this? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:92: struct Endpoint { Why can't you forward declare this? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:114: struct EndpointTuple { Why can't you forward declare this? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:130: struct Delivery { Why can't you forward declare this?
The CQ bit was checked by juliatuttle@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...
https://codereview.chromium.org/2249213002/diff/320001/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/2249213002/diff/320001/net/net.gypi#newcode1241 net/net.gypi:1241: 'reporting/reporting_controls.cc', On 2017/01/03 21:28:12, Ryan Sleevi wrote: > From a design perspective, could you explain more why you introduced a top-level > directory called //reporting ? > > Given that the Reporting API is a Web Platform feature (e.g. not an intrinsic > behaviour of an HTTP or networking client, spec'd as part of W3C/WICG), I can > see a compelling argument that this belongs in //content instead (or possibly > //third_party/WebKit). Initially, I was going to put it in a component. The reason I put it in //net is that other parts of //net (e.g. HPKP, HSTS, Expect-Staple) would end up depending on it, and the interface for that looked *really* hairy, and mmenke suggested putting it //net was a viable option. If you can help me come up with a not-ugly way for it to live elsewhere, I'd be okay with that. I confess I haven't thoroughly evaluated the "have a stub interface in //net that's implemented by a component" approach yet. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_controls.cc (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_controls.cc:52: return kDefaultEnabled; On 2017/01/03 21:28:12, Ryan Sleevi wrote: > As mentioned in the header, this all seems like something base::FeatureList is > meant to embody, and it would be good to know if that was intentionally avoided. > > This will also hopefully eliminate the need for HistogramEnabledState(), since > such configs can be represented using the field trial config reporting already > used. Obsolete; I'm now using base::Feature. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_controls.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_controls.h:14: // default. On 2017/01/03 21:28:12, Ryan Sleevi (OOO) wrote: > 1) //net should never mention layers above it (in comments or code), because > that's a layering violation. For example, it's impossible to evaluate what this > code would do in the case of Chrome for iOS or in the case of Cronet - both of > which have //net part of them. > > 2) From this header, it's not clear why a simple use of base::Feature wasn't > used here? Obsolete; I'm now using base::Feature. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_metrics.cc (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.cc:17: DCHECK_NE(HEADER_FATE_MAX, fate); On 2017/01/03 21:28:13, Ryan Sleevi wrote: > All of these DCHECKs are arguably unnecessary > > I appreciate you're trying to guard against programmer error, but in C++, any > possible integer may have been coerced into such an enum, leaving it in an > undefined state. It would appear from these DCHECKs that you're trying to > validate that the enumeration value is in range of the histogram, but that's not > at all sufficient to do, and I'm not sure the programmer error you're attempting > to guard against is reasonable/likely. Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.cc:25: if (ttl > base::TimeDelta()) { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > This is subtle and under-documented (in your design or your header) as to why a > negative TTL is not measured, even though you anticipated it enough to defend > against it. I'm not defending against a negative TTL; I'm differentiating a *zero* TTL (which means "remove this endpoint") from a positive TTL (which means "set or update this endpoint"). I've added a comment above the if to clarify. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.cc:50: UMA_HISTOGRAM_COUNTS_100000("Reporting.DeliveryByteCount", byte_count); On 2017/01/03 21:28:13, Ryan Sleevi wrote: > I'm very skeptical of a lot of these metrics, and don't see the design doc > really showing how they'll be owned or influence the design. > > I appreciate and am sensitive to the fact that unless we measure from the > get-go, we won't know, but I'd like to push back on each and every one of these > measurements to make sure we know precisely why we're going to measure them and > what we anticipate using them for, so that we only collect what is needed to > influence the feature and the user experience. Alright, let me give it some thought. The ones I'm particularly interested in are ones related to reports and deliveries -- are we successfully uploading most reports within a reasonable amount of time, and not losing too many to network changes and such? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_metrics.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.h:12: enum EnabledState { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > 1) Do all of these need to be in "public" headers? Ideally, no. They don't need to be visible outside //net, no. What makes a header file "public"? > 2) Document - none of these states are self-obvious Is it good or bad to let histograms.xml be the canonical documentation for the states? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_metrics.h:67: void HistogramEnabledState(EnabledState state); On 2017/01/03 21:28:13, Ryan Sleevi wrote: > naming/design: > > It seems not at all self-obvious when reading code and seeing a call to > > net::HistogramEnabledState(ENABLED_STATE_ENABLED_DEFAULT) > > is going to do, documentation or not. It seems like much of this may be better > served being grouped in a logical namespace (net::reporting)? > > In thinking very carefully how //net will be used in the future, does this API > benefit from splitting (like QUIC to some extent, and like //content > exclusively) into the notion of "public" and "private" implementation parts? It > seems like this bleeds a lot into a namespace without very clear naming. I think it totally would benefit from that, but I'm unsure how to do it. Is it just enforced by custom (and presubmit) that stuff outside //content can't depend on headers outside //content/public? https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_report.cc (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.cc:11: ReportingReport::ReportingReport() {} On 2017/01/03 21:28:13, Ryan Sleevi wrote: > = default; Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_report.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:15: struct ReportingReport { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > From a design standpoint, this feels more like a class (or will evolve into one) > than a struct, per > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes My gut feeling is that it's a struct, but I'm making it a class since it's got a unique_ptr member and (therefore, and since I don't *need* to copy or assign it) I'm adding DISALLOW_COPY_OR_ASSIGN. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:18: ~ReportingReport(); On 2017/01/03 21:28:13, Ryan Sleevi wrote: > Rule of three applies here - in general, if you provide a dtor, you should > provide copy and assignment. > > It would seem you're not doing so here because you're only concerned about > squelching warnings about complex ctors/dtors (and the code-size implication > they have), but the same applies here to copy/assignment. > > Further, the copy/assignment is complex (because of line 22) - so it deserves > either a DISALLOW_COPY_AND_ASSIGN (which further deflates the "is-a struct" > argument) or explicit management (which I'm unclear of what the right answer is) Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:20: std::string ToString() const; On 2017/01/03 21:28:13, Ryan Sleevi wrote: > From reading the header: Why does this exist? What does this do? Is it for > debugging? GTEst? Is it the serializing of the class? It was for testing, but I'm gonna remove it for now. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:22: std::unique_ptr<base::Value> body; On 2017/01/03 21:28:13, Ryan Sleevi wrote: > What is this? Why is it a base::Value versus another structure? Does anything > guarantee any form of consistency (or non-mutability)? It's the report body, which is any JSON. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:23: // URL of content that triggered report. On 2017/01/03 21:28:13, Ryan Sleevi wrote: > nelines between 22-23 Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:27: // separately, so they are separate fields for now. On 2017/01/03 21:28:13, Ryan Sleevi wrote: > Line break between 24-25 and 29-30 > > As comments go, I think this leaves more questions than answers. For example, is > there a bug filed on this? Why does the spec allow callers to separate them? > What's an example of when you should or should not? What happens if origin isn't > valid at all - what do you do? There is a bug filed, at https://github.com/WICG/reporting/issues/25. If the origin isn't valid at all, the report will never get picked up for delivery, and will expire after an hour or so. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:33: base::TimeTicks timestamp; On 2017/01/03 21:28:13, Ryan Sleevi wrote: > naming: "timestamp" is self-describing without adding new information. Should > this be base::TimeTicks creation_time? Renamed to "queued". > Does this require monotonicity? Civil time conversion? How is this used? All > unclear. Added a comment to specify how it's used. It's reported to the endpoint as an age, and reports aren't persisted across sessions, so I *think* TimeTicks is preferable to Time. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:35: int attempts; On 2017/01/03 21:28:13, Ryan Sleevi wrote: > size_t for counts - > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_report.h:37: bool pending; On 2017/01/03 21:28:13, Ryan Sleevi wrote: > This feels like an interface bleed - why should the report keep track of whether > it itself is pending? This seems to leak in details about how the report is > used, rather than what a report is. > > The same applies to |attempts|. Both of these seem to represent a state about-a > report, not state that is-a report. Obsolete; I'm making Report an inner, private class of ReportingService, so callers don't have to deal with it at all. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... File net/reporting/reporting_service.h (right): https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:26: class NET_EXPORT ReportingService { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > High level Design remarks: > 1) This feels like a lot of the implementation details have bled into the public > header - meaning anytime the implementatino changes, that's sort of spread > through to every consumer. > > Concrete Suggestions: > a) Forward declare whenever possible > b) Carefully evaluate whether you need a private class member function versus a > function within an unnamed namespace > c) Carefully evaluate whether this class is doing "too much" > > 2) The testing seems to be litering *ForTesting functions in to the public > interface. While a presubmit helps minimize that, it feels like it might result > in a more carefully balanced operation if you make use of text fixtures, rather > than public interface. > a) Similarly, if you're needing that much of your "implementation detail" to > be provided via ForTesting() methods, it may be a sign that your layering or > design is trying to do too much, and would benefit from more decoupling. My original design had a separate "ReportingCache" class that just did the data storage, so it was easier for tests to check the effect of parsing headers or queueing reports. Maybe I should go back to that design. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:50: static Policy GetDefault(); On 2017/01/03 21:28:13, Ryan Sleevi wrote: > DESIGN: Why is an explicit GetDefault required? Why can't default constructing > the object return a sane default? Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:56: void set_uploader(std::unique_ptr<ReportingUploader> uploader); On 2017/01/03 21:28:13, Ryan Sleevi wrote: > This seems to have more subtlety than just "set_uploader" Not really; it's just a separate setter instead of a constructor parameter so it can be cleared early in profile destruction to avoid circular dependencies like the mess we had with Domain Reliability. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:65: void set_clock_for_testing(std::unique_ptr<base::TickClock> clock); On 2017/01/03 21:28:13, Ryan Sleevi wrote: > SetClockForTesting Why? It is actually just a plain setter under the hood. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:73: struct Client { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > Why can't you forward declare this? Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:92: struct Endpoint { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > Why can't you forward declare this? Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:114: struct EndpointTuple { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > Why can't you forward declare this? Done. https://codereview.chromium.org/2249213002/diff/320001/net/reporting/reportin... net/reporting/reporting_service.h:130: struct Delivery { On 2017/01/03 21:28:13, Ryan Sleevi wrote: > Why can't you forward declare this? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by juliatuttle@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by juliatuttle@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
