Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
859903002:
Compare non-whitelisted off-domain inclusions against the browsing history. (Closed)
DescriptionCompare non-whitelisted off-domain inclusions against browsing history.
BUG=412468
Committed: https://crrev.com/5918e01e4163038e2afd5d69761f18072146ae32
Cr-Commit-Position: refs/heads/master@{#313820}
Patch Set 1 #Patch Set 2 : fix compile #Patch Set 3 : merge and fix compile issues caused by trunk changes to ServiceAccessType #
Total comments: 12
Patch Set 4 : histograms cleanup #Patch Set 5 : merge r312228 #
Total comments: 21
Patch Set 6 : review:grt/asvitkine #
Total comments: 2
Patch Set 7 : Use RenderProcessHost instead of RenderFrameHost to obtain Profile* with one less level of indirect… #Patch Set 8 : fix compile? #
Total comments: 4
Patch Set 9 : GMOCK, nit, and compile fix #
Total comments: 1
Messages
Total messages: 33 (8 generated)
gab@chromium.org changed reviewers: + grt@chromium.org
Hey Greg, finally got it working with the HistoryService (those unittests were a pain...!) I'd like your opinion on the overall design first (I have to run and didn't have time to do a pass over potential stupid nits...). Still need to modify histograms.xml accordingly, but otherwise I think it's more or less good to go. Cheers! Gab
Patchset #2 (id:20001) has been deleted
gab@chromium.org changed reviewers: + asvitkine@chromium.org
Alright, cleaned up and ready for full review now. @Alexei for histograms in parallel. Thanks! Gab
https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:67: HistoryService* history_service = new HistoryService( Should this be a scoped_ptr? If you're returning ownership to the caller, return value should be a scoped_ptr too. https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:366: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31429: +<histogram name="SBOffDomainInclusion.AbortEmptyMainFrameURL" Have you considered naming these so the grouping is more obvious? e.g. SBOffDomainInclusion.Aborted.EmptyMainFrameURL and so forth... Then, you can use a histogram_suffixes construct here to avoid the redundancy in the XML file.
Thanks for the quick feedback, replies below. https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:67: HistoryService* history_service = new HistoryService( On 2015/01/20 17:48:13, Alexei Svitkine wrote: > Should this be a scoped_ptr? If you're returning ownership to the caller, return > value should be a scoped_ptr too. No, ProfileKeyedServices are owned by the profiles, the factories only hand out raw pointers to them (and initially build them if none is associated with a given profile). https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:366: }; On 2015/01/20 17:48:13, Alexei Svitkine wrote: > DISALLOW_COPY_AND_ASSIGN I've noticed that test fixtures don't typically bother with DISALLOW_COPY_AND_ASSIGN. Statistically it looks like ~25% of unit test fixtures have it: 771 (DISALLOW_COPY_AND_ASSIGN): https://code.google.com/p/chromium/codesearch#search/&q=DISALLOW_COPY_AND_ASS... vs 3002 (All test fixtures): https://code.google.com/p/chromium/codesearch#search/&q=(class%5C%20.*Test)%2... https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31429: +<histogram name="SBOffDomainInclusion.AbortEmptyMainFrameURL" On 2015/01/20 17:48:13, Alexei Svitkine wrote: > Have you considered naming these so the grouping is more obvious? > > e.g. SBOffDomainInclusion.Aborted.EmptyMainFrameURL > > and so forth... > > Then, you can use a histogram_suffixes construct here to avoid the redundancy in > the XML file. How's redundancy reduced? They still all mean different things, would the suffixes not have a description? Wouldn't that reduce the information? This doesn't feel like a natural case for suffixes as the enum in the product code would then be split across two histograms group which would be somewhat weird.
https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:67: HistoryService* history_service = new HistoryService( On 2015/01/20 18:10:48, gab wrote: > On 2015/01/20 17:48:13, Alexei Svitkine wrote: > > Should this be a scoped_ptr? If you're returning ownership to the caller, > return > > value should be a scoped_ptr too. > > No, ProfileKeyedServices are owned by the profiles, the factories only hand out > raw pointers to them (and initially build them if none is associated with a > given profile). If so, why is it safe to delete it when Init() returns false? I didn't see anything in the documentation for Init() that talks about ownership... https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:366: }; On 2015/01/20 18:10:48, gab wrote: > On 2015/01/20 17:48:13, Alexei Svitkine wrote: > > DISALLOW_COPY_AND_ASSIGN > > I've noticed that test fixtures don't typically bother with > DISALLOW_COPY_AND_ASSIGN. > > Statistically it looks like ~25% of unit test fixtures have it: > > 771 (DISALLOW_COPY_AND_ASSIGN): > https://code.google.com/p/chromium/codesearch#search/&q=DISALLOW_COPY_AND_ASS... > > vs > > 3002 (All test fixtures): > https://code.google.com/p/chromium/codesearch#search/&q=(class%5C%20.*Test)%2... https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eT-8FIcgRq8/tRqrC... https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31429: +<histogram name="SBOffDomainInclusion.AbortEmptyMainFrameURL" On 2015/01/20 18:10:48, gab wrote: > On 2015/01/20 17:48:13, Alexei Svitkine wrote: > > Have you considered naming these so the grouping is more obvious? > > > > e.g. SBOffDomainInclusion.Aborted.EmptyMainFrameURL > > > > and so forth... > > > > Then, you can use a histogram_suffixes construct here to avoid the redundancy > in > > the XML file. > > How's redundancy reduced? They still all mean different things, would the > suffixes not have a description? Wouldn't that reduce the information? I believe each suffix can have a description that will be appended to the histogram's description. > > This doesn't feel like a natural case for suffixes as the enum in the product > code would then be split across two histograms group which would be somewhat > weird. Sorry, I don't understand what you mean. All of these .Abort* histograms are using the same enum - ContentResourceType... histogram_suffixes just makes the XML in this file a bit more concise. Also, the naming convention I propose (regardless of the use of histogram_suffixes) provides better grouping on the dashboard (e.g. you can do .* to get all of the reasons together.)
Patchset #4 (id:80001) has been deleted
Okay, how about this? https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:67: HistoryService* history_service = new HistoryService( On 2015/01/20 18:17:48, Alexei Svitkine wrote: > On 2015/01/20 18:10:48, gab wrote: > > On 2015/01/20 17:48:13, Alexei Svitkine wrote: > > > Should this be a scoped_ptr? If you're returning ownership to the caller, > > return > > > value should be a scoped_ptr too. > > > > No, ProfileKeyedServices are owned by the profiles, the factories only hand > out > > raw pointers to them (and initially build them if none is associated with a > > given profile). > > If so, why is it safe to delete it when Init() returns false? I didn't see > anything in the documentation for Init() that talks about ownership... My bad, I responded too quick, somehow I thought the discussion was on line 209 below where we get a raw HistoryService*. I agree with you that here the return type should be scoped_ptr<KeyedService> but this is unfortunately not how BrowserContextKeyedService's and their factories functions are designed (see https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...). This is using the same approach as https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf... perhaps a common helper could be built, I was hoping to have grt@ weigh in on his opinion for that given he's the one who implemented the copied instance. https://codereview.chromium.org/859903002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:366: }; On 2015/01/20 18:17:47, Alexei Svitkine wrote: > On 2015/01/20 18:10:48, gab wrote: > > On 2015/01/20 17:48:13, Alexei Svitkine wrote: > > > DISALLOW_COPY_AND_ASSIGN > > > > I've noticed that test fixtures don't typically bother with > > DISALLOW_COPY_AND_ASSIGN. > > > > Statistically it looks like ~25% of unit test fixtures have it: > > > > 771 (DISALLOW_COPY_AND_ASSIGN): > > > https://code.google.com/p/chromium/codesearch#search/&q=DISALLOW_COPY_AND_ASS... > > > > vs > > > > 3002 (All test fixtures): > > > https://code.google.com/p/chromium/codesearch#search/&q=(class%5C%20.*Test)%2... > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eT-8FIcgRq8/tRqrC... Okay, thanks, done. https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/859903002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31429: +<histogram name="SBOffDomainInclusion.AbortEmptyMainFrameURL" On 2015/01/20 18:17:48, Alexei Svitkine wrote: > On 2015/01/20 18:10:48, gab wrote: > > On 2015/01/20 17:48:13, Alexei Svitkine wrote: > > > Have you considered naming these so the grouping is more obvious? > > > > > > e.g. SBOffDomainInclusion.Aborted.EmptyMainFrameURL > > > > > > and so forth... > > > > > > Then, you can use a histogram_suffixes construct here to avoid the > redundancy > > in > > > the XML file. > > > > How's redundancy reduced? They still all mean different things, would the > > suffixes not have a description? Wouldn't that reduce the information? > > I believe each suffix can have a description that will be appended to the > histogram's description. > > > > > This doesn't feel like a natural case for suffixes as the enum in the product > > code would then be split across two histograms group which would be somewhat > > weird. > > Sorry, I don't understand what you mean. All of these .Abort* histograms are > using the same enum - ContentResourceType... > > histogram_suffixes just makes the XML in this file a bit more concise. Also, the > naming convention I propose (regardless of the use of histogram_suffixes) > provides better grouping on the dashboard (e.g. you can do .* to get all of the > reasons together.) > What I was trying to say is that it feels weird that some AnalysisEvent would result in a suffixed histogram and others in a regular histogram. I tried something, let me know if you prefer that. I like the conciseness of the histograms.xml change now, but don't like the split of some histograms being declared up top and some other in the suffix for the same enum. Some pluses some minuses, so I don't really mind one way or another.
histograms lgtm, but curious to see the discussion about HistoryService ownership https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:270: OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED || Nit: Given this is in off_domain_inclusion_detector_unittest.cc, I wonder if you really need such a long name for the enum (OffDomainInclusionTestCases) - since it's making this code much uglier due to all the wrapping. Consider shortening the name or even using a regular enum rather than enum class, to make this cleaner.
Looks good. Will review the test after callback vs. virtual method is resolved. Cheers. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:35: // |render_process_id| and |render_frame_id| or NULL if the resolution can't be NULL -> null or nullptr as per chromium-dev discussion. PK prefers nullptr, while I (and others) prefer null. Take your pick. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:38: Profile* GetProfileFromRenderFrameId(int render_process_id, teeny tiny nit: drop "Get" from the name, making this Profile* ProfileFromRenderFrameId(int render_process_id, int render_frame_id) { https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:60: OffDomainInclusionInfo() {} initialize members: OffDomainInclusionInfo() : render_process_id(0), render_frame_id(0) {} maybe put resource_type(RESOURCE_TYPE_LAST_TYPE) in there, too? https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: HistoryServiceFactory::GetForProfileWithoutCreating(profile); My gut tells me that this should be GetForProfileIfExists(profile, ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and see what you think? https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:20: class Time; line 103 passes a base::Time by value, so i don't think a forward decl is sufficient. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:68: const GetProfileFromRenderFrameIdCallback& it looks like this callback is here so that tests can provide their own convenience implementation. calling through a protected virtual method (that is overridden in the test) has considerably less overhead. i recall reading on chromium-dev that running a callback is relatively expensive on android in particular. i think it's simpler to use a virtual method, unless there some subtlety in the use of the callback that i'm missing.
https://codereview.chromium.org/859903002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/859903002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31452: + Deprecated 01/2015. can a comment here somehow direct readers of the code and viewers of the dashboard to the new name?
Thanks, done, PTAL. Cheers! Gab https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:35: // |render_process_id| and |render_frame_id| or NULL if the resolution can't be On 2015/01/21 01:58:24, grt wrote: > NULL -> null or nullptr as per chromium-dev discussion. PK prefers nullptr, > while I (and others) prefer null. Take your pick. Done. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:38: Profile* GetProfileFromRenderFrameId(int render_process_id, On 2015/01/21 01:58:24, grt wrote: > teeny tiny nit: drop "Get" from the name, making this > Profile* ProfileFromRenderFrameId(int render_process_id, int render_frame_id) { Done. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:60: OffDomainInclusionInfo() {} On 2015/01/21 01:58:24, grt wrote: > initialize members: > OffDomainInclusionInfo() : render_process_id(0), render_frame_id(0) {} > maybe put resource_type(RESOURCE_TYPE_LAST_TYPE) in there, too? Done. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: HistoryServiceFactory::GetForProfileWithoutCreating(profile); On 2015/01/21 01:58:24, grt wrote: > My gut tells me that this should be GetForProfileIfExists(profile, > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and see what > you think? It felt to me like, since I was explicitly checking for incognito, I didn't need to bother with the overhead of the slightly more complex call (which AFAIK may even wrap things to prevent writes/etc. which would be overkill here). The API doesn't make it clear at all what the difference is for these more or less equivalent method names... https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:20: class Time; On 2015/01/21 01:58:24, grt wrote: > line 103 passes a base::Time by value, so i don't think a forward decl is > sufficient. Hmm I guess that's true since the caller needs to know how to copy it. Good catch, done. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h:68: const GetProfileFromRenderFrameIdCallback& On 2015/01/21 01:58:24, grt wrote: > it looks like this callback is here so that tests can provide their own > convenience implementation. calling through a protected virtual method (that is > overridden in the test) has considerably less overhead. i recall reading on > chromium-dev that running a callback is relatively expensive on android in > particular. i think it's simpler to use a virtual method, unless there some > subtlety in the use of the callback that i'm missing. Done. https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:270: OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED || On 2015/01/20 22:24:57, Alexei Svitkine wrote: > Nit: Given this is in off_domain_inclusion_detector_unittest.cc, I wonder if you > really need such a long name for the enum (OffDomainInclusionTestCases) - since > it's making this code much uglier due to all the wrapping. Consider shortening > the name or even using a regular enum rather than enum class, to make this > cleaner. Done, still like using enum class, but cleaned up naming to drop "off-domain" since it's implicit. https://codereview.chromium.org/859903002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/859903002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31452: + Deprecated 01/2015. On 2015/01/21 02:04:42, grt wrote: > can a comment here somehow direct readers of the code and viewers of the > dashboard to the new name? Done.
robertshield@chromium.org changed reviewers: + robertshield@chromium.org
https://codereview.chromium.org/859903002/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:243: // this call can be made synchronously, but this would need to be made I have the same requirement for my trigger analyzer. Given that this needs to be true, do you need a main_thread_task_runner_ member?
https://codereview.chromium.org/859903002/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:243: // this call can be made synchronously, but this would need to be made On 2015/01/26 20:14:47, robertshield wrote: > I have the same requirement for my trigger analyzer. Given that this needs to be > true, do you need a main_thread_task_runner_ member? I don't absolutely need it but then I would need every other method to DCHECK that it's on the UI thread (i.e. "its main" thread). I'm trying to avoid depending on BrowserThread:: as much as possible. Getting the profile left me no choice, but at least, as-is, it makes it clear to a future reader that this is the only point of dependency on BrowserThread (reading multi-threaded code recently with SafeBrowsingDatabase/etc. the hardest part for a new reader is always figuring out how things currently work, having assumptions/dependencies clearly documented and even enforced by design is the best way to ease a future reader's experience IMO).
https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: HistoryServiceFactory::GetForProfileWithoutCreating(profile); On 2015/01/23 21:35:33, gab wrote: > On 2015/01/21 01:58:24, grt wrote: > > My gut tells me that this should be GetForProfileIfExists(profile, > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and see what > > you think? > > It felt to me like, since I was explicitly checking for incognito, I didn't need > to bother with the overhead of the slightly more complex call (which AFAIK may > even wrap things to prevent writes/etc. which would be overkill here). > > The API doesn't make it clear at all what the difference is for these more or > less equivalent method names... My concern is about correctness rather than overhead. Reasons I think (but am not 100% certain) IMPLICIT_ACCESS is correct: - The doc for EXPLICIT_ACCESS says "The caller plans to perform a read or write that takes place as a result of the user input." The detector does not perform writes, and is not run in response to user input in the same sense as other profile services. - History persistence may be disabled on non-OTR profiles (http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...). GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case. Would you check with a profile or history owner to figure out which is the correct approach? Thanks. https://codereview.chromium.org/859903002/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:142: class MockOffDomainInclusionDetector : public OffDomainInclusionDetector { If you don't have an aversion to GMock, this class could be replaced with a true mock and something like: ON_CALL(mock_detector_, ProfileFromRenderProcessId(Eq(render_process_id))) .WillByDefault(Return(testing_profile)); EXPECT_CALL may be appropriate if you want to assert that the call is made. https://codereview.chromium.org/859903002/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:234: if (ShouldAddSimulatedURLsToHistory()) { nit: no braces?
Patchset #9 (id:200001) has been deleted
gab@chromium.org changed reviewers: + sky@chromium.org
Done. @sky (as a chrome\browser\history owner): we would like your input on a comment below (https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro...). Thanks! Gab https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: HistoryServiceFactory::GetForProfileWithoutCreating(profile); On 2015/01/27 15:28:11, grt wrote: > On 2015/01/23 21:35:33, gab wrote: > > On 2015/01/21 01:58:24, grt wrote: > > > My gut tells me that this should be GetForProfileIfExists(profile, > > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and see > what > > > you think? > > > > It felt to me like, since I was explicitly checking for incognito, I didn't > need > > to bother with the overhead of the slightly more complex call (which AFAIK may > > even wrap things to prevent writes/etc. which would be overkill here). > > > > The API doesn't make it clear at all what the difference is for these more or > > less equivalent method names... > > My concern is about correctness rather than overhead. Reasons I think (but am > not 100% certain) IMPLICIT_ACCESS is correct: > - The doc for EXPLICIT_ACCESS says "The caller plans to perform a read or write > that takes place as a result of the user input." The detector does not perform > writes, and is not run in response to user input in the same sense as other > profile services. Right, which is why I avoided using either IMPLICT_ACCESS or EXPLICIT_ACCESS. The fact that the API provides a side-channel that essentially is equivalent to IMPLICIT_ACCESS in practice is true, but as-is I don't depend on either ServiceAccessType pattern. > > - History persistence may be disabled on non-OTR profiles > (http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...). > GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case. > > Would you check with a profile or history owner to figure out which is the > correct approach? Thanks. Sounds good, @sky: any tip here on how to use HistoryServiceFactory? The API doesn't provide any useful tips on when to use GetForProfileIfExists() vs GetForProfileWithoutCreating(). Thanks! https://codereview.chromium.org/859903002/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:142: class MockOffDomainInclusionDetector : public OffDomainInclusionDetector { On 2015/01/27 15:28:11, grt wrote: > If you don't have an aversion to GMock, this class could be replaced with a true > mock and something like: > > ON_CALL(mock_detector_, ProfileFromRenderProcessId(Eq(render_process_id))) > .WillByDefault(Return(testing_profile)); > > EXPECT_CALL may be appropriate if you want to assert that the call is made. Done. https://codereview.chromium.org/859903002/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:234: if (ShouldAddSimulatedURLsToHistory()) { On 2015/01/27 15:28:11, grt wrote: > nit: no braces? Done.
https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: HistoryServiceFactory::GetForProfileWithoutCreating(profile); On 2015/01/28 19:33:06, gab wrote: > On 2015/01/27 15:28:11, grt wrote: > > On 2015/01/23 21:35:33, gab wrote: > > > On 2015/01/21 01:58:24, grt wrote: > > > > My gut tells me that this should be GetForProfileIfExists(profile, > > > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and see > > what > > > > you think? > > > > > > It felt to me like, since I was explicitly checking for incognito, I didn't > > need > > > to bother with the overhead of the slightly more complex call (which AFAIK > may > > > even wrap things to prevent writes/etc. which would be overkill here). > > > > > > The API doesn't make it clear at all what the difference is for these more > or > > > less equivalent method names... > > > > My concern is about correctness rather than overhead. Reasons I think (but am > > not 100% certain) IMPLICIT_ACCESS is correct: > > - The doc for EXPLICIT_ACCESS says "The caller plans to perform a read or > write > > that takes place as a result of the user input." The detector does not perform > > writes, and is not run in response to user input in the same sense as other > > profile services. > > Right, which is why I avoided using either IMPLICT_ACCESS or EXPLICIT_ACCESS. > The fact that the API provides a side-channel that essentially is equivalent to > IMPLICIT_ACCESS in practice is true, but as-is I don't depend on either > ServiceAccessType pattern. If by "side-channel" you mean GetForProfileWithoutCreating, then it is not essentially equivalent to IMPLICIT_ACCESS. It is essentially equivalent to EXPLICIT_ACCESSS, which I believe is exactly the opposite of what you should have here. Unless, that is, I'm completely failing to read and understand the HistoryServiceFactory code and comments (always possible). I'm looking forward to some light from the sky on this. :-) > > - History persistence may be disabled on non-OTR profiles > > > (http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...). > > GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case. > > > > Would you check with a profile or history owner to figure out which is the > > correct approach? Thanks. > > Sounds good, @sky: any tip here on how to use HistoryServiceFactory? The API > doesn't provide any useful tips on when to use GetForProfileIfExists() vs > GetForProfileWithoutCreating(). > > Thanks! https://codereview.chromium.org/859903002/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc (right): https://codereview.chromium.org/859903002/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc:353: off_domain_inclusion_detector_.reset(new MockOffDomainInclusionDetector( new NiceMock<MockOffDomainInclusionDetector> here and get rid of NiceMock in the class definition.
https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: HistoryServiceFactory::GetForProfileWithoutCreating(profile); On 2015/01/28 21:10:50, grt wrote: > On 2015/01/28 19:33:06, gab wrote: > > On 2015/01/27 15:28:11, grt wrote: > > > On 2015/01/23 21:35:33, gab wrote: > > > > On 2015/01/21 01:58:24, grt wrote: > > > > > My gut tells me that this should be GetForProfileIfExists(profile, > > > > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and > see > > > what > > > > > you think? > > > > > > > > It felt to me like, since I was explicitly checking for incognito, I > didn't > > > need > > > > to bother with the overhead of the slightly more complex call (which AFAIK > > may > > > > even wrap things to prevent writes/etc. which would be overkill here). > > > > > > > > The API doesn't make it clear at all what the difference is for these more > > or > > > > less equivalent method names... > > > > > > My concern is about correctness rather than overhead. Reasons I think (but > am > > > not 100% certain) IMPLICIT_ACCESS is correct: > > > - The doc for EXPLICIT_ACCESS says "The caller plans to perform a read or > > write > > > that takes place as a result of the user input." The detector does not > perform > > > writes, and is not run in response to user input in the same sense as other > > > profile services. > > > > Right, which is why I avoided using either IMPLICT_ACCESS or EXPLICIT_ACCESS. > > The fact that the API provides a side-channel that essentially is equivalent > to > > IMPLICIT_ACCESS in practice is true, but as-is I don't depend on either > > ServiceAccessType pattern. > > If by "side-channel" you mean GetForProfileWithoutCreating, then it is not > essentially equivalent to IMPLICIT_ACCESS. It is essentially equivalent to > EXPLICIT_ACCESSS, which I believe is exactly the opposite of what you should > have here. Unless, that is, I'm completely failing to read and understand the > HistoryServiceFactory code and comments (always possible). I'm looking forward > to some light from the sky on this. :-) > > > > - History persistence may be disabled on non-OTR profiles > > > > > > (http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...). > > > GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case. > > > > > > Would you check with a profile or history owner to figure out which is the > > > correct approach? Thanks. > > > > Sounds good, @sky: any tip here on how to use HistoryServiceFactory? The API > > doesn't provide any useful tips on when to use GetForProfileIfExists() vs > > GetForProfileWithoutCreating(). > > > > Thanks! > I don't have enough context about this code to tell which it should be. How do we end up here? What action do we want to take?
@sky: more context below, happy to answer additional questions, essentially we're trying to understand when one should use GetForProfileIfExists(ServiceAccessType) vs GetForProfileWithoutCreating(), thanks! https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc (right): https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: HistoryServiceFactory::GetForProfileWithoutCreating(profile); On 2015/01/28 21:35:02, sky wrote: > On 2015/01/28 21:10:50, grt wrote: > > On 2015/01/28 19:33:06, gab wrote: > > > On 2015/01/27 15:28:11, grt wrote: > > > > On 2015/01/23 21:35:33, gab wrote: > > > > > On 2015/01/21 01:58:24, grt wrote: > > > > > > My gut tells me that this should be GetForProfileIfExists(profile, > > > > > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit and > > see > > > > what > > > > > > you think? > > > > > > > > > > It felt to me like, since I was explicitly checking for incognito, I > > didn't > > > > need > > > > > to bother with the overhead of the slightly more complex call (which > AFAIK > > > may > > > > > even wrap things to prevent writes/etc. which would be overkill here). > > > > > > > > > > The API doesn't make it clear at all what the difference is for these > more > > > or > > > > > less equivalent method names... > > > > > > > > My concern is about correctness rather than overhead. Reasons I think (but > > am > > > > not 100% certain) IMPLICIT_ACCESS is correct: > > > > - The doc for EXPLICIT_ACCESS says "The caller plans to perform a read or > > > write > > > > that takes place as a result of the user input." The detector does not > > perform > > > > writes, and is not run in response to user input in the same sense as > other > > > > profile services. > > > > > > Right, which is why I avoided using either IMPLICT_ACCESS or > EXPLICIT_ACCESS. > > > The fact that the API provides a side-channel that essentially is equivalent > > to > > > IMPLICIT_ACCESS in practice is true, but as-is I don't depend on either > > > ServiceAccessType pattern. > > > > If by "side-channel" you mean GetForProfileWithoutCreating, then it is not > > essentially equivalent to IMPLICIT_ACCESS. It is essentially equivalent to > > EXPLICIT_ACCESSS, which I believe is exactly the opposite of what you should > > have here. Unless, that is, I'm completely failing to read and understand the > > HistoryServiceFactory code and comments (always possible). I'm looking forward > > to some light from the sky on this. :-) > > > > > > - History persistence may be disabled on non-OTR profiles > > > > > > > > > > (http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...). > > > > GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case. > > > > > > > > Would you check with a profile or history owner to figure out which is the > > > > correct approach? Thanks. > > > > > > Sounds good, @sky: any tip here on how to use HistoryServiceFactory? The API > > > doesn't provide any useful tips on when to use GetForProfileIfExists() vs > > > GetForProfileWithoutCreating(). > > > > > > Thanks! > > > > I don't have enough context about this code to tell which it should be. How do > we end up here? What action do we want to take? Apologies for the lack of context. tl;dr; we want to cross-check a given resource inclusion against the user's browsing history iff the HistoryService already exists. We already abort early if this request comes from an incognito profile (see line 264 above). Essentially I think we want GetForProfileIfExists(IMPLICIT_ACCESS), but given we already explicitly check for incognito above, it felt like GetForProfileWithoutCreating() was simpler -- otherwise what's the goal of having GetForProfileWithoutCreating() in this API if not to avoid a dependency on ServiceAccessType given GetForProfileWithoutCreating() is really the same as GetForProfileIfExists(EXPLICIT_ACCESS)? Longer story on how we end up here: We end up here if a resource on a page was "off-domain" and was not in the safe browsing list of whitelisted inclusions. At this point we want to cross-check the off-domain inclusion against the user's local browsing history before declaring it suspicious. But we don't want to force the HistoryService to be created (i.e. intentionally dropping early analyses on profile creation). We also don't want to analyze requests made from an incognito profile and explicitly handle this on line 264 above.
On 2015/01/29 14:53:41, gab wrote: > @sky: more context below, happy to answer additional questions, essentially > we're trying to understand when one should use > GetForProfileIfExists(ServiceAccessType) vs GetForProfileWithoutCreating(), > thanks! > > https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... > File > chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc > (right): > > https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: > HistoryServiceFactory::GetForProfileWithoutCreating(profile); > On 2015/01/28 21:35:02, sky wrote: > > On 2015/01/28 21:10:50, grt wrote: > > > On 2015/01/28 19:33:06, gab wrote: > > > > On 2015/01/27 15:28:11, grt wrote: > > > > > On 2015/01/23 21:35:33, gab wrote: > > > > > > On 2015/01/21 01:58:24, grt wrote: > > > > > > > My gut tells me that this should be GetForProfileIfExists(profile, > > > > > > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit > and > > > see > > > > > what > > > > > > > you think? > > > > > > > > > > > > It felt to me like, since I was explicitly checking for incognito, I > > > didn't > > > > > need > > > > > > to bother with the overhead of the slightly more complex call (which > > AFAIK > > > > may > > > > > > even wrap things to prevent writes/etc. which would be overkill here). > > > > > > > > > > > > The API doesn't make it clear at all what the difference is for these > > more > > > > or > > > > > > less equivalent method names... > > > > > > > > > > My concern is about correctness rather than overhead. Reasons I think > (but > > > am > > > > > not 100% certain) IMPLICIT_ACCESS is correct: > > > > > - The doc for EXPLICIT_ACCESS says "The caller plans to perform a read > or > > > > write > > > > > that takes place as a result of the user input." The detector does not > > > perform > > > > > writes, and is not run in response to user input in the same sense as > > other > > > > > profile services. > > > > > > > > Right, which is why I avoided using either IMPLICT_ACCESS or > > EXPLICIT_ACCESS. > > > > The fact that the API provides a side-channel that essentially is > equivalent > > > to > > > > IMPLICIT_ACCESS in practice is true, but as-is I don't depend on either > > > > ServiceAccessType pattern. > > > > > > If by "side-channel" you mean GetForProfileWithoutCreating, then it is not > > > essentially equivalent to IMPLICIT_ACCESS. It is essentially equivalent to > > > EXPLICIT_ACCESSS, which I believe is exactly the opposite of what you should > > > have here. Unless, that is, I'm completely failing to read and understand > the > > > HistoryServiceFactory code and comments (always possible). I'm looking > forward > > > to some light from the sky on this. :-) > > > > > > > > - History persistence may be disabled on non-OTR profiles > > > > > > > > > > > > > > > (http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...). > > > > > GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case. > > > > > > > > > > Would you check with a profile or history owner to figure out which is > the > > > > > correct approach? Thanks. > > > > > > > > Sounds good, @sky: any tip here on how to use HistoryServiceFactory? The > API > > > > doesn't provide any useful tips on when to use GetForProfileIfExists() vs > > > > GetForProfileWithoutCreating(). > > > > > > > > Thanks! > > > > > > > I don't have enough context about this code to tell which it should be. How do > > we end up here? What action do we want to take? > > Apologies for the lack of context. > > tl;dr; we want to cross-check a given resource inclusion against the user's > browsing history iff the HistoryService already exists. We already abort early > if this request comes from an incognito profile (see line 264 above). > Essentially I think we want GetForProfileIfExists(IMPLICIT_ACCESS), but given we > already explicitly check for incognito above, it felt like > GetForProfileWithoutCreating() was simpler I think the important thing is to pick the call with the proper semantics, not the one that is simpler. The former will be correct now and in the future if the shape/color of the API changes in some way that preserves the meaning. > -- otherwise what's the goal of > having GetForProfileWithoutCreating() in this API if not to avoid a dependency > on ServiceAccessType given GetForProfileWithoutCreating() is really the same as > GetForProfileIfExists(EXPLICIT_ACCESS)? > > > Longer story on how we end up here: > > We end up here if a resource on a page was "off-domain" and was not in the safe > browsing list of whitelisted inclusions. > > At this point we want to cross-check the off-domain inclusion against the user's > local browsing history before declaring it suspicious. > > But we don't want to force the HistoryService to be created (i.e. intentionally > dropping early analyses on profile creation). > > We also don't want to analyze requests made from an incognito profile and > explicitly handle this on line 264 above. Do you want to analyze requests for non-OTR profiles where history storage is disabled via policy?
On 2015/01/29 15:35:53, grt wrote: > On 2015/01/29 14:53:41, gab wrote: > > @sky: more context below, happy to answer additional questions, essentially > > we're trying to understand when one should use > > GetForProfileIfExists(ServiceAccessType) vs GetForProfileWithoutCreating(), > > thanks! > > > > > https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... > > File > > > chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc > > (right): > > > > > https://codereview.chromium.org/859903002/diff/120001/chrome/browser/safe_bro... > > > chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.cc:269: > > HistoryServiceFactory::GetForProfileWithoutCreating(profile); > > On 2015/01/28 21:35:02, sky wrote: > > > On 2015/01/28 21:10:50, grt wrote: > > > > On 2015/01/28 19:33:06, gab wrote: > > > > > On 2015/01/27 15:28:11, grt wrote: > > > > > > On 2015/01/23 21:35:33, gab wrote: > > > > > > > On 2015/01/21 01:58:24, grt wrote: > > > > > > > > My gut tells me that this should be GetForProfileIfExists(profile, > > > > > > > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into it a bit > > and > > > > see > > > > > > what > > > > > > > > you think? > > > > > > > > > > > > > > It felt to me like, since I was explicitly checking for incognito, I > > > > didn't > > > > > > need > > > > > > > to bother with the overhead of the slightly more complex call (which > > > AFAIK > > > > > may > > > > > > > even wrap things to prevent writes/etc. which would be overkill > here). > > > > > > > > > > > > > > The API doesn't make it clear at all what the difference is for > these > > > more > > > > > or > > > > > > > less equivalent method names... > > > > > > > > > > > > My concern is about correctness rather than overhead. Reasons I think > > (but > > > > am > > > > > > not 100% certain) IMPLICIT_ACCESS is correct: > > > > > > - The doc for EXPLICIT_ACCESS says "The caller plans to perform a read > > or > > > > > write > > > > > > that takes place as a result of the user input." The detector does not > > > > perform > > > > > > writes, and is not run in response to user input in the same sense as > > > other > > > > > > profile services. > > > > > > > > > > Right, which is why I avoided using either IMPLICT_ACCESS or > > > EXPLICIT_ACCESS. > > > > > The fact that the API provides a side-channel that essentially is > > equivalent > > > > to > > > > > IMPLICIT_ACCESS in practice is true, but as-is I don't depend on either > > > > > ServiceAccessType pattern. > > > > > > > > If by "side-channel" you mean GetForProfileWithoutCreating, then it is not > > > > essentially equivalent to IMPLICIT_ACCESS. It is essentially equivalent to > > > > EXPLICIT_ACCESSS, which I believe is exactly the opposite of what you > should > > > > have here. Unless, that is, I'm completely failing to read and understand > > the > > > > HistoryServiceFactory code and comments (always possible). I'm looking > > forward > > > > to some light from the sky on this. :-) > > > > > > > > > > - History persistence may be disabled on non-OTR profiles > > > > > > > > > > > > > > > > > > > > > (http://www.chromium.org/administrators/policy-list-3#SavingBrowserHistoryDisa...). > > > > > > GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this case. > > > > > > > > > > > > Would you check with a profile or history owner to figure out which is > > the > > > > > > correct approach? Thanks. > > > > > > > > > > Sounds good, @sky: any tip here on how to use HistoryServiceFactory? The > > API > > > > > doesn't provide any useful tips on when to use GetForProfileIfExists() > vs > > > > > GetForProfileWithoutCreating(). > > > > > > > > > > Thanks! > > > > > > > > > > I don't have enough context about this code to tell which it should be. How > do > > > we end up here? What action do we want to take? > > > > Apologies for the lack of context. > > > > tl;dr; we want to cross-check a given resource inclusion against the user's > > browsing history iff the HistoryService already exists. We already abort early > > if this request comes from an incognito profile (see line 264 above). > > Essentially I think we want GetForProfileIfExists(IMPLICIT_ACCESS), but given > we > > already explicitly check for incognito above, it felt like > > GetForProfileWithoutCreating() was simpler > > I think the important thing is to pick the call with the proper semantics, not > the one that is simpler. The former will be correct now and in the future if the > shape/color of the API changes in some way that preserves the meaning. Exactly, and to me the proper semantics is "I don't care" since I already explicitly check for incognito before. And thus not explicitly depending on the semantics of EXPLICIT vs IMPLICIT access sounds best. > > > -- otherwise what's the goal of > > having GetForProfileWithoutCreating() in this API if not to avoid a dependency > > on ServiceAccessType given GetForProfileWithoutCreating() is really the same > as > > GetForProfileIfExists(EXPLICIT_ACCESS)? > > > > > > Longer story on how we end up here: > > > > We end up here if a resource on a page was "off-domain" and was not in the > safe > > browsing list of whitelisted inclusions. > > > > At this point we want to cross-check the off-domain inclusion against the > user's > > local browsing history before declaring it suspicious. > > > > But we don't want to force the HistoryService to be created (i.e. > intentionally > > dropping early analyses on profile creation). > > > > We also don't want to analyze requests made from an incognito profile and > > explicitly handle this on line 264 above. > > Do you want to analyze requests for non-OTR profiles where history storage is > disabled via policy? Sure, worst case history will be empty and this will be a no-op. This is never a privacy breach as it can only *reduce* the amount of things we consider suspicious (in fact, given this argument, I'd say we could even eventually consider reports from incognito profiles -- which once again strengthens my belief that the HistoryServiceFactory::Get*() call used should have semantics of "I don't care about ServiceAccessType").
As you are finding EXPLICIT vs IMPLICIT is a bit arbitrary. In many cases you end up picking the one that does what you want instead mapping it to the correct term, because they are too arbitrary. I suggest going with EXPLICIT and moving on. That said, if you're feeling particularly ambitious think of a better way to do what EXPLICIT/IMPLICIT are trying to. They have never sat well with me. On Thu, Jan 29, 2015 at 9:06 AM, <gab@chromium.org> wrote: > On 2015/01/29 15:35:53, grt wrote: > >> On 2015/01/29 14:53:41, gab wrote: >> > @sky: more context below, happy to answer additional questions, >> essentially >> > we're trying to understand when one should use >> > GetForProfileIfExists(ServiceAccessType) vs >> GetForProfileWithoutCreating(), >> > thanks! >> > >> > >> > > https://codereview.chromium.org/859903002/diff/120001/ > chrome/browser/safe_browsing/incident_reporting/off_domain_ > inclusion_detector.cc > >> > File >> > >> > > chrome/browser/safe_browsing/incident_reporting/off_domain_ > inclusion_detector.cc > >> > (right): >> > >> > >> > > https://codereview.chromium.org/859903002/diff/120001/ > chrome/browser/safe_browsing/incident_reporting/off_domain_ > inclusion_detector.cc#newcode269 > >> > >> > > chrome/browser/safe_browsing/incident_reporting/off_domain_ > inclusion_detector.cc:269: > >> > HistoryServiceFactory::GetForProfileWithoutCreating(profile); >> > On 2015/01/28 21:35:02, sky wrote: >> > > On 2015/01/28 21:10:50, grt wrote: >> > > > On 2015/01/28 19:33:06, gab wrote: >> > > > > On 2015/01/27 15:28:11, grt wrote: >> > > > > > On 2015/01/23 21:35:33, gab wrote: >> > > > > > > On 2015/01/21 01:58:24, grt wrote: >> > > > > > > > My gut tells me that this should be >> > GetForProfileIfExists(profile, > >> > > > > > > > ServiceAccessType::IMPLICIT_ACCESS). Could you look into >> it a >> > bit > >> > and >> > > > see >> > > > > > what >> > > > > > > > you think? >> > > > > > > >> > > > > > > It felt to me like, since I was explicitly checking for >> incognito, >> > I > >> > > > didn't >> > > > > > need >> > > > > > > to bother with the overhead of the slightly more complex call >> > (which > >> > > AFAIK >> > > > > may >> > > > > > > even wrap things to prevent writes/etc. which would be >> overkill >> here). >> > > > > > > >> > > > > > > The API doesn't make it clear at all what the difference is >> for >> these >> > > more >> > > > > or >> > > > > > > less equivalent method names... >> > > > > > >> > > > > > My concern is about correctness rather than overhead. Reasons I >> > think > >> > (but >> > > > am >> > > > > > not 100% certain) IMPLICIT_ACCESS is correct: >> > > > > > - The doc for EXPLICIT_ACCESS says "The caller plans to perform >> a >> > read > >> > or >> > > > > write >> > > > > > that takes place as a result of the user input." The detector >> does >> > not > >> > > > perform >> > > > > > writes, and is not run in response to user input in the same >> sense >> > as > >> > > other >> > > > > > profile services. >> > > > > >> > > > > Right, which is why I avoided using either IMPLICT_ACCESS or >> > > EXPLICIT_ACCESS. >> > > > > The fact that the API provides a side-channel that essentially is >> > equivalent >> > > > to >> > > > > IMPLICIT_ACCESS in practice is true, but as-is I don't depend on >> > either > >> > > > > ServiceAccessType pattern. >> > > > >> > > > If by "side-channel" you mean GetForProfileWithoutCreating, then it >> is >> > not > >> > > > essentially equivalent to IMPLICIT_ACCESS. It is essentially >> equivalent >> > to > >> > > > EXPLICIT_ACCESSS, which I believe is exactly the opposite of what >> you >> should >> > > > have here. Unless, that is, I'm completely failing to read and >> > understand > >> > the >> > > > HistoryServiceFactory code and comments (always possible). I'm >> looking >> > forward >> > > > to some light from the sky on this. :-) >> > > > >> > > > > > - History persistence may be disabled on non-OTR profiles >> > > > > > >> > > > > >> > > > >> > > >> > >> > > (http://www.chromium.org/administrators/policy-list-3# > SavingBrowserHistoryDisabled). > >> > > > > > GetForProfileIfExists(IMPLICIT_ACCESS) will return null in this >> > case. > >> > > > > > >> > > > > > Would you check with a profile or history owner to figure out >> which >> > is > >> > the >> > > > > > correct approach? Thanks. >> > > > > >> > > > > Sounds good, @sky: any tip here on how to use >> HistoryServiceFactory? >> > The > >> > API >> > > > > doesn't provide any useful tips on when to use >> GetForProfileIfExists() >> vs >> > > > > GetForProfileWithoutCreating(). >> > > > > >> > > > > Thanks! >> > > > >> > > >> > > I don't have enough context about this code to tell which it should >> be. >> > How > >> do >> > > we end up here? What action do we want to take? >> > >> > Apologies for the lack of context. >> > >> > tl;dr; we want to cross-check a given resource inclusion against the >> user's >> > browsing history iff the HistoryService already exists. We already abort >> > early > >> > if this request comes from an incognito profile (see line 264 above). >> > Essentially I think we want GetForProfileIfExists(IMPLICIT_ACCESS), but >> > given > >> we >> > already explicitly check for incognito above, it felt like >> > GetForProfileWithoutCreating() was simpler >> > > I think the important thing is to pick the call with the proper >> semantics, not >> the one that is simpler. The former will be correct now and in the future >> if >> > the > >> shape/color of the API changes in some way that preserves the meaning. >> > > Exactly, and to me the proper semantics is "I don't care" since I already > explicitly check for incognito before. > > And thus not explicitly depending on the semantics of EXPLICIT vs IMPLICIT > access sounds best. > > > > -- otherwise what's the goal of >> > having GetForProfileWithoutCreating() in this API if not to avoid a >> > dependency > >> > on ServiceAccessType given GetForProfileWithoutCreating() is really the >> same >> as >> > GetForProfileIfExists(EXPLICIT_ACCESS)? >> > >> > >> > Longer story on how we end up here: >> > >> > We end up here if a resource on a page was "off-domain" and was not in >> the >> safe >> > browsing list of whitelisted inclusions. >> > >> > At this point we want to cross-check the off-domain inclusion against >> the >> user's >> > local browsing history before declaring it suspicious. >> > >> > But we don't want to force the HistoryService to be created (i.e. >> intentionally >> > dropping early analyses on profile creation). >> > >> > We also don't want to analyze requests made from an incognito profile >> and >> > explicitly handle this on line 264 above. >> > > Do you want to analyze requests for non-OTR profiles where history >> storage is >> disabled via policy? >> > > Sure, worst case history will be empty and this will be a no-op. This is > never a > privacy breach as it can only *reduce* the amount of things we consider > suspicious (in fact, given this argument, I'd say we could even eventually > consider reports from incognito profiles -- which once again strengthens my > belief that the HistoryServiceFactory::Get*() call used should have > semantics of > "I don't care about ServiceAccessType"). > > https://codereview.chromium.org/859903002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for chiming in, Scott. LGTM.
On 2015/01/29 19:01:27, grt wrote: > Thanks for chiming in, Scott. LGTM. Thanks Scott, going with GetWithoutCreating() as this is basically GetIfExists(EXPLICIT) without the explicit decision to use one or the other (FWIW, I find it weird that there are these two separate yet mostly identical calls, it would be nice to update the API with clear guidelines). Cheers, Gab
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/859903002/220001
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5918e01e4163038e2afd5d69761f18072146ae32 Cr-Commit-Position: refs/heads/master@{#313820} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
