Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
99423007:
[SafeBrowsing] Reset malware indicator on page nav start. (Closed)
|
Created:
7 years ago by Greg Billock Modified:
7 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[SafeBrowsing] Save malware indicator in NavigationEntry extra data.
R=mattm@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241168
Patch Set 1 #
Total comments: 9
Patch Set 2 : Add DidStartLoading #Patch Set 3 : Update to use NavEntry extra data #
Total comments: 35
Patch Set 4 : add test #Patch Set 5 : Rebase #
Total comments: 8
Patch Set 6 : . #
Total comments: 2
Patch Set 7 : use GetVisibleEntry #
Messages
Total messages: 20 (0 generated)
This is kind of the area where I'm out of my depth. So you might still want to double-check with Matt or someone who knows what's up later :-). https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.cc:386: return malware_or_phishing_match_; I'm not sure I follow this. AFAICT, the reason you need to move malware_or_phishing_match_ = false to the new function is because OnSafeBrowsingMatch() might be called between DidStartNavigationToPendingEntry() and DidNavigateMainFrame(), in which case the old code would have overwritten it. So this could potentially return true, but the comment implies that it shouldn't report a match for pending entries. https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/client_side_detection_host.h (right): https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.h:17: #include "content/public/browser/navigation_controller.h" I can see IWYU arguments for this, but do those apply when the class you're subclassing should have provided everything necessary to declare your override? [I honestly don't know, I could see it going both ways.]
https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.cc:386: return malware_or_phishing_match_; On 2013/12/12 21:28:44, shess wrote: > I'm not sure I follow this. AFAICT, the reason you need to move > malware_or_phishing_match_ = false to the new function is because > OnSafeBrowsingMatch() might be called between DidStartNavigationToPendingEntry() > and DidNavigateMainFrame(), in which case the old code would have overwritten > it. So this could potentially return true, but the comment implies that it > shouldn't report a match for pending entries. So in practice, the flaw I'm seeing is when switching away from a page with an SB warning. The OnSafeBrowsingMatch() calls should come in during page load, but if the page is navigated, the malware_or_phishing_match_ wasn't cleared until the DidNavigateMainFrame, which comes after the page is fully loaded and committed. But yes, I'm suspicious this is double-checking, and that in adjusting when it gets reset, this goes away. The reason I was confused is that I was thinking DidShowSBInterstitial was the one returning true. It already uses GetActiveEntry from the nav controller, and that should return the pending entry when there is one, and that in turn should not match the new page. But I think this is just double-checking, and can be ommitted. Does that sound right? https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/client_side_detection_host.h (right): https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.h:17: #include "content/public/browser/navigation_controller.h" On 2013/12/12 21:28:44, shess wrote: > I can see IWYU arguments for this, but do those apply when the class you're > subclassing should have provided everything necessary to declare your override? > [I honestly don't know, I could see it going both ways.] I'm flexible. Agreed that the super-type has to include it.
https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.cc:291: // Should this be DidStartLoading instead? Hm, I'm not entirely sure. The comment about "It is not called for renderer-initiated navigations unless they are sent to the browser process via OpenURL." make it sound like DidStartNavigationToPendingEntry might not be the right one. But I don't know whether DidStartLoading, DidStartProvisionalLoadForFrame, or what is best. https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.cc:386: return malware_or_phishing_match_; On 2013/12/12 22:14:44, Greg Billock wrote: > On 2013/12/12 21:28:44, shess wrote: > > I'm not sure I follow this. AFAICT, the reason you need to move > > malware_or_phishing_match_ = false to the new function is because > > OnSafeBrowsingMatch() might be called between > DidStartNavigationToPendingEntry() > > and DidNavigateMainFrame(), in which case the old code would have overwritten > > it. So this could potentially return true, but the comment implies that it > > shouldn't report a match for pending entries. > > So in practice, the flaw I'm seeing is when switching away from a page with an > SB warning. The OnSafeBrowsingMatch() calls should come in during page load, but > if the page is navigated, the malware_or_phishing_match_ wasn't cleared until > the DidNavigateMainFrame, which comes after the page is fully loaded and > committed. > > But yes, I'm suspicious this is double-checking, and that in adjusting when it > gets reset, this goes away. > > The reason I was confused is that I was thinking DidShowSBInterstitial was the > one returning true. It already uses GetActiveEntry from the nav controller, and > that should return the pending entry when there is one, and that in turn should > not match the new page. But I think this is just double-checking, and can be > ommitted. Does that sound right? It would be nice if there were tests for all the cases. Regarding this, is the || DidShowSBInterstitial() part actually necessary? Won't malware_or_phishing_match_ always have the state you want? I agree the comment needs clarification (is it talking about navigation to a malware site or away from?)
https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.cc:291: // Should this be DidStartLoading instead? On 2013/12/13 00:30:01, mattm wrote: > Hm, I'm not entirely sure. > > The comment about "It is not called for renderer-initiated navigations unless > they are sent to the browser process via OpenURL." make it sound like > DidStartNavigationToPendingEntry might not be the right one. Yeah, that's the thing that made me worry. > > But I don't know whether DidStartLoading, DidStartProvisionalLoadForFrame, or > what is best. Looking into this more, there are a couple weird edge cases for DidStartLoading -- i.e. javascript urls won't send it. It looks like it will be sent on normal renderer-initiated page loads, though. I think we need both. :-( https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.cc:386: return malware_or_phishing_match_; On 2013/12/13 00:30:01, mattm wrote: > On 2013/12/12 22:14:44, Greg Billock wrote: > > On 2013/12/12 21:28:44, shess wrote: > > > I'm not sure I follow this. AFAICT, the reason you need to move > > > malware_or_phishing_match_ = false to the new function is because > > > OnSafeBrowsingMatch() might be called between > > DidStartNavigationToPendingEntry() > > > and DidNavigateMainFrame(), in which case the old code would have > overwritten > > > it. So this could potentially return true, but the comment implies that it > > > shouldn't report a match for pending entries. > > > > So in practice, the flaw I'm seeing is when switching away from a page with an > > SB warning. The OnSafeBrowsingMatch() calls should come in during page load, > but > > if the page is navigated, the malware_or_phishing_match_ wasn't cleared until > > the DidNavigateMainFrame, which comes after the page is fully loaded and > > committed. > > > > But yes, I'm suspicious this is double-checking, and that in adjusting when it > > gets reset, this goes away. > > > > The reason I was confused is that I was thinking DidShowSBInterstitial was the > > one returning true. It already uses GetActiveEntry from the nav controller, > and > > that should return the pending entry when there is one, and that in turn > should > > not match the new page. But I think this is just double-checking, and can be > > ommitted. Does that sound right? > > > It would be nice if there were tests for all the cases. > > > Regarding this, is the || DidShowSBInterstitial() part actually necessary? Won't > malware_or_phishing_match_ always have the state you want? I think you're right. I'll take all this out and just return that. > I agree the comment needs clarification (is it talking about navigation to a > malware site or away from?)
https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/client_side_detection_host.cc:291: // Should this be DidStartLoading instead? On 2013/12/13 00:54:49, Greg Billock wrote: > On 2013/12/13 00:30:01, mattm wrote: > > Hm, I'm not entirely sure. > > > > The comment about "It is not called for renderer-initiated navigations unless > > they are sent to the browser process via OpenURL." make it sound like > > DidStartNavigationToPendingEntry might not be the right one. > > Yeah, that's the thing that made me worry. > > > > > But I don't know whether DidStartLoading, DidStartProvisionalLoadForFrame, or > > what is best. > > Looking into this more, there are a couple weird edge cases for DidStartLoading > -- i.e. javascript urls won't send it. What do you mean by javascript urls? window.open? > It looks like it will be sent on normal > renderer-initiated page loads, though. I think we need both. :-( I think there may be other subtle cases we haven't considered, too. Ex, starting on a malware page, start a navigation to another (slow) site (which would clear the malware_or_phishing_match_ value), and then cancel the navigation before it gets committed so that you stay on the current page. The current page would no longer be shown as malware. (Given some unverified assumptions about what happens if you cancel a pending navigation.) One thing I notice is that NavigationEntry has a SetExtraData method. I think it would probably be simpler and more reliable to just set a flag on the appropriate NavigationEntry when we get a malware match. Then we don't have to worry about when to clear a value, or what to do about cancelled navigations, etc.
On 2013/12/13 03:03:34, mattm wrote: > https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... > File chrome/browser/safe_browsing/client_side_detection_host.cc (right): > > https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... > chrome/browser/safe_browsing/client_side_detection_host.cc:291: // Should this > be DidStartLoading instead? > On 2013/12/13 00:54:49, Greg Billock wrote: > > On 2013/12/13 00:30:01, mattm wrote: > > > Hm, I'm not entirely sure. > > > > > > The comment about "It is not called for renderer-initiated navigations > unless > > > they are sent to the browser process via OpenURL." make it sound like > > > DidStartNavigationToPendingEntry might not be the right one. > > > > Yeah, that's the thing that made me worry. > > > > > > > > But I don't know whether DidStartLoading, DidStartProvisionalLoadForFrame, > or > > > what is best. > > > > Looking into this more, there are a couple weird edge cases for > DidStartLoading > > -- i.e. javascript urls won't send it. > > What do you mean by javascript urls? window.open? > > > It looks like it will be sent on normal > > renderer-initiated page loads, though. I think we need both. :-( > > > > I think there may be other subtle cases we haven't considered, too. > Ex, starting on a malware page, start a navigation to another (slow) site (which > would clear the malware_or_phishing_match_ value), and then cancel the > navigation before it gets committed so that you stay on the current page. The > current page would no longer be shown as malware. (Given some unverified > assumptions about what happens if you cancel a pending navigation.) Yeah, that makes me think still checking the DidShowSBInterstitial when there's no pending navigation is better. > > > One thing I notice is that NavigationEntry has a SetExtraData method. I think it > would probably be simpler and more reliable to just set a flag on the > appropriate NavigationEntry when we get a malware match. Then we don't have to > worry about when to clear a value, or what to do about cancelled navigations, > etc. And that sounds like it'd be the best. I'll swivel the stuff in the CSDH to just access that.
On 2013/12/13 07:03:37, Greg Billock wrote: > On 2013/12/13 03:03:34, mattm wrote: > > > https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... > > File chrome/browser/safe_browsing/client_side_detection_host.cc (right): > > > > > https://codereview.chromium.org/99423007/diff/1/chrome/browser/safe_browsing/... > > chrome/browser/safe_browsing/client_side_detection_host.cc:291: // Should this > > be DidStartLoading instead? > > On 2013/12/13 00:54:49, Greg Billock wrote: > > > On 2013/12/13 00:30:01, mattm wrote: > > > > Hm, I'm not entirely sure. > > > > > > > > The comment about "It is not called for renderer-initiated navigations > > unless > > > > they are sent to the browser process via OpenURL." make it sound like > > > > DidStartNavigationToPendingEntry might not be the right one. > > > > > > Yeah, that's the thing that made me worry. > > > > > > > > > > > But I don't know whether DidStartLoading, > DidStartProvisionalLoadForFrame, > > or > > > > what is best. > > > > > > Looking into this more, there are a couple weird edge cases for > > DidStartLoading > > > -- i.e. javascript urls won't send it. > > > > What do you mean by javascript urls? window.open? > > > > > It looks like it will be sent on normal > > > renderer-initiated page loads, though. I think we need both. :-( > > > > > > > > I think there may be other subtle cases we haven't considered, too. > > Ex, starting on a malware page, start a navigation to another (slow) site > (which > > would clear the malware_or_phishing_match_ value), and then cancel the > > navigation before it gets committed so that you stay on the current page. The > > current page would no longer be shown as malware. (Given some unverified > > assumptions about what happens if you cancel a pending navigation.) > > Yeah, that makes me think still checking the DidShowSBInterstitial when there's > no pending navigation is better. > > > > > > > One thing I notice is that NavigationEntry has a SetExtraData method. I think > it > > would probably be simpler and more reliable to just set a flag on the > > appropriate NavigationEntry when we get a malware match. Then we don't have to > > worry about when to clear a value, or what to do about cancelled navigations, > > etc. > > And that sounds like it'd be the best. I'll swivel the stuff in the CSDH to just > access that. OK, I've uploaded this. I'm not confident it covers all circumstances, though -- the nav entry chain is quite complex. How about redirects?
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:53: const bool kTrue = true; What's going with this?
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Update CL description https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:51: const char kSafeBrowsingHitKey[] = "safe_browsing_hit"; s/hit/match/ https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:359: kSafeBrowsingHitKey, base::ASCIIToUTF16("1")); This shouldn't be necessary (OnSafeBrowsingMatch should always be called before OnSafeBrowsingHit)? https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:368: const SafeBrowsingUIManager::UnsafeResource& resource) { derp. Doesn't this function also need to do the checks that the match is for the correct webcontents? https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:369: web_contents()->GetController().GetActiveEntry()->SetExtraData( also do null checks like OnSafeBrowsingHit does https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:378: bool ClientSideDetectionHost::DidPageReceiveSafeBrowsingMatch() const { nit: probably would be cleaner if it first just got a pointer to the correct NavigationEntry, and then didn't need to do the GetExtraData call in two places. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:378: bool ClientSideDetectionHost::DidPageReceiveSafeBrowsingMatch() const { null checks here too. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { GetActiveEntry will already return the pending entry if there is one.. or were you trying to avoid transient entries for some reason? (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...) https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:380: // Check the pending entry if we are displaying an interstitial page. Comment is a little confusing as it might not actually display an interstitial if it was already whitelisted. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:53: const bool kTrue = true; On 2013/12/13 21:10:55, Greg Billock wrote: > What's going with this? I guess it's so ExpectPreClassificationChecks can have tri-state arguments by passing NULL or a pointer to one of these (true, false, not-used). Tis a bit weird. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:374: test DidPageReceiveSafeBrowsingMatch() before committing the navigation too. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:379: ASSERT_TRUE(csd_host_->DidPageReceiveSafeBrowsingMatch()); Add tests of DidPageReceiveSafeBrowsingMatch() for navigating away from malware also (during the pending navigation, after committing it, and also after cancelling it)
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:379: ASSERT_TRUE(csd_host_->DidPageReceiveSafeBrowsingMatch()); Also test of getting a match during a pending navigation and then cancelling the navigation instead of committing it.
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/12/14 00:09:49, mattm wrote: > Update CL description Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:51: const char kSafeBrowsingHitKey[] = "safe_browsing_hit"; On 2013/12/14 00:09:49, mattm wrote: > s/hit/match/ Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:359: kSafeBrowsingHitKey, base::ASCIIToUTF16("1")); On 2013/12/14 00:09:49, mattm wrote: > This shouldn't be necessary (OnSafeBrowsingMatch should always be called before > OnSafeBrowsingHit)? Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:368: const SafeBrowsingUIManager::UnsafeResource& resource) { On 2013/12/14 00:09:49, mattm wrote: > derp. Doesn't this function also need to do the checks that the match is for the > correct webcontents? Looks like we should do if (!web_contents || !wc...GetActiveEntry) I'm not sure what you mean about testing for the match being against this web contents -- OnSafeBrowsingHit above just writes down the active UniqueID. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:369: web_contents()->GetController().GetActiveEntry()->SetExtraData( On 2013/12/14 00:09:49, mattm wrote: > also do null checks like OnSafeBrowsingHit does Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:378: bool ClientSideDetectionHost::DidPageReceiveSafeBrowsingMatch() const { On 2013/12/14 00:09:49, mattm wrote: > null checks here too. Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:378: bool ClientSideDetectionHost::DidPageReceiveSafeBrowsingMatch() const { On 2013/12/14 00:09:49, mattm wrote: > nit: probably would be cleaner if it first just got a pointer to the correct > NavigationEntry, and then didn't need to do the GetExtraData call in two places. Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { On 2013/12/14 00:09:49, mattm wrote: > GetActiveEntry will already return the pending entry if there is one.. or were > you trying to avoid transient entries for some reason? > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...) Yes, the transient entries are the reason for this clause -- while the SB page is up, GetActiveEntry returns that, so we think it isn't a hit when it is. The comment is supposed to clarify this -- should I reword it? https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:380: // Check the pending entry if we are displaying an interstitial page. On 2013/12/14 00:09:49, mattm wrote: > Comment is a little confusing as it might not actually display an interstitial > if it was already whitelisted. Yeah, this is for the other case -- that case should just be active (or pending) and then be correct. The bad case is the interstitial. At first I had an interstitial GetPageType check explicitly, but actually we always want to return pending state, since we want to update the site chip as soon as possible, in addition to during interstitials. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:53: const bool kTrue = true; On 2013/12/14 00:09:49, mattm wrote: > On 2013/12/13 21:10:55, Greg Billock wrote: > > What's going with this? > > I guess it's so ExpectPreClassificationChecks can have tri-state arguments by > passing NULL or a pointer to one of these (true, false, not-used). Tis a bit > weird. ok https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:374: On 2013/12/14 00:09:49, mattm wrote: > test DidPageReceiveSafeBrowsingMatch() before committing the navigation too. Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:379: ASSERT_TRUE(csd_host_->DidPageReceiveSafeBrowsingMatch()); On 2013/12/14 00:09:49, mattm wrote: > Add tests of DidPageReceiveSafeBrowsingMatch() for navigating away from malware > also (during the pending navigation, after committing it, and also after > cancelling it) Done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:379: ASSERT_TRUE(csd_host_->DidPageReceiveSafeBrowsingMatch()); On 2013/12/14 00:12:28, mattm wrote: > Also test of getting a match during a pending navigation and then cancelling the > navigation instead of committing it. Done.
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:368: const SafeBrowsingUIManager::UnsafeResource& resource) { On 2013/12/14 18:35:34, Greg Billock wrote: > On 2013/12/14 00:09:49, mattm wrote: > > derp. Doesn't this function also need to do the checks that the match is for > the > > correct webcontents? > > Looks like we should do if (!web_contents || !wc...GetActiveEntry) > > I'm not sure what you mean about testing for the match being against this web > contents -- OnSafeBrowsingHit above just writes down the active UniqueID. OnSafeBrowsingMatch and OnSafeBrowsingHit are both global notifications, they'll be sent out to all observers for a hit on any tab. However, there is a separate CSDH for each tab. So every CSDH will get the notification, and each one will have to test the render host/view ids to check if the notification is for the tab that CSDH is associated with. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { On 2013/12/14 18:35:34, Greg Billock wrote: > On 2013/12/14 00:09:49, mattm wrote: > > GetActiveEntry will already return the pending entry if there is one.. or were > > you trying to avoid transient entries for some reason? > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...) > > Yes, the transient entries are the reason for this clause -- while the SB page > is up, GetActiveEntry returns that, so we think it isn't a hit when it is. The > comment is supposed to clarify this -- should I reword it? Yeah, I didn't understand the comment. Maybe something like "If an interstitial page is showing, GetActiveEntry will return the transient NavigationEntry for the interstitial. The transient entry will not have the flag set, so use the pending entry instead if there is one." I'm not entirely familiar with transient entries. What happens if you get an interstitial after the page is showing and there is no pending NavigationEntry? Does it still create a transient entry? Does that mean you always want to check either Pending or LastCommitted, never Active? It seems like we also need some tests that actually create interstitials and make sure DidPageReceiveSafeBrowsingMatch returns the correct thing. https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:383: NavigationEntry* entry = web_contents()->GetController().GetActiveEntry(); Looking at the comments of GetActiveEntry vs GetVisibleEntry, it seems like this should be GetVisibleEntry? (Though this might cause some trouble with the other comment about only using Pending or Committed, since the navigationController doesn't expose the "safe_to_show_pending" calculation otherwise.) https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:386: entry = web_contents()->GetController().GetPendingEntry(); These lines might be clearer as entry = GetPendingEntry() if (!entry) entry = GetActiveEntry() ? https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (right): https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:386: void NavigateWithoutSBHitAndCommit(const GURL& safe_url) { Might as well check the DidShowSBInterstitial values in this method too.
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:368: const SafeBrowsingUIManager::UnsafeResource& resource) { On 2013/12/16 22:36:11, mattm wrote: > On 2013/12/14 18:35:34, Greg Billock wrote: > > On 2013/12/14 00:09:49, mattm wrote: > > > derp. Doesn't this function also need to do the checks that the match is for > > the > > > correct webcontents? > > > > Looks like we should do if (!web_contents || !wc...GetActiveEntry) > > > > I'm not sure what you mean about testing for the match being against this web > > contents -- OnSafeBrowsingHit above just writes down the active UniqueID. > > OnSafeBrowsingMatch and OnSafeBrowsingHit are both global notifications, they'll > be sent out to all observers for a hit on any tab. However, there is a separate > CSDH for each tab. So every CSDH will get the notification, and each one will > have to test the render host/view ids to check if the notification is for the > tab that CSDH is associated with. Ah, I see what you mean. OK, done. https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { On 2013/12/16 22:36:11, mattm wrote: > On 2013/12/14 18:35:34, Greg Billock wrote: > > On 2013/12/14 00:09:49, mattm wrote: > > > GetActiveEntry will already return the pending entry if there is one.. or > were > > > you trying to avoid transient entries for some reason? > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...) > > > > Yes, the transient entries are the reason for this clause -- while the SB page > > is up, GetActiveEntry returns that, so we think it isn't a hit when it is. The > > comment is supposed to clarify this -- should I reword it? > > Yeah, I didn't understand the comment. Maybe something like "If an interstitial > page is showing, GetActiveEntry will return the transient NavigationEntry for > the interstitial. The transient entry will not have the flag set, so use the > pending entry instead if there is one." > > I'm not entirely familiar with transient entries. What happens if you get an > interstitial after the page is showing and there is no pending NavigationEntry? > Does it still create a transient entry? Does that mean you always want to check > either Pending or LastCommitted, never Active? I think I always want to check Pending if there is one. There may still be a couple edge cases this needs to be adapted for, but I'm not sure how to predict what they are. > It seems like we also need some tests that actually create interstitials and > make sure DidPageReceiveSafeBrowsingMatch returns the correct thing. I think the tests cover that case in the ...WithSBHit test. Certainly they test that the state of the pending navigation takes precedence, which I think is the important facet of the code. https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:383: NavigationEntry* entry = web_contents()->GetController().GetActiveEntry(); On 2013/12/16 22:36:11, mattm wrote: > Looking at the comments of GetActiveEntry vs GetVisibleEntry, it seems like this > should be GetVisibleEntry? > (Though this might cause some trouble with the other comment about only using > Pending or Committed, since the navigationController doesn't expose the > "safe_to_show_pending" calculation otherwise.) Yeah, I thought about that, but decided to go with GetActiveEntry since that's what the match setting stuff uses. I think that in the cases when they get called, they'll be the same, but that whole system has a lot of complexity, so I could be mistaken, especially in odd edge cases. https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:386: entry = web_contents()->GetController().GetPendingEntry(); On 2013/12/16 22:36:11, mattm wrote: > These lines might be clearer as > entry = GetPendingEntry() > if (!entry) > entry = GetActiveEntry() > ? Done. https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (right): https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:386: void NavigateWithoutSBHitAndCommit(const GURL& safe_url) { On 2013/12/16 22:36:11, mattm wrote: > Might as well check the DidShowSBInterstitial values in this method too. Done.
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { On 2013/12/16 23:00:21, Greg Billock wrote: > On 2013/12/16 22:36:11, mattm wrote: > > On 2013/12/14 18:35:34, Greg Billock wrote: > > > On 2013/12/14 00:09:49, mattm wrote: > > > > GetActiveEntry will already return the pending entry if there is one.. or > > were > > > > you trying to avoid transient entries for some reason? > > > > > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...) > > > > > > Yes, the transient entries are the reason for this clause -- while the SB > page > > > is up, GetActiveEntry returns that, so we think it isn't a hit when it is. > The > > > comment is supposed to clarify this -- should I reword it? > > > > Yeah, I didn't understand the comment. Maybe something like "If an > interstitial > > page is showing, GetActiveEntry will return the transient NavigationEntry for > > the interstitial. The transient entry will not have the flag set, so use the > > pending entry instead if there is one." > > > > I'm not entirely familiar with transient entries. What happens if you get an > > interstitial after the page is showing and there is no pending > NavigationEntry? > > Does it still create a transient entry? Does that mean you always want to > check > > either Pending or LastCommitted, never Active? > > I think I always want to check Pending if there is one. There may still be a > couple edge cases this needs to be adapted for, but I'm not sure how to predict > what they are. Sorry, my comment neglected a point. In the case I mentioned, if my assumptions are correct, there would not be a pending entry, but there would be a transient entry. So GetPendingEntry would return NULL, GetActiveEntry would return the transient entry (the interstitial) rather than the current entry, which actually has the flag set, so you would not show the icon in that case. > > It seems like we also need some tests that actually create interstitials and > > make sure DidPageReceiveSafeBrowsingMatch returns the correct thing. > > I think the tests cover that case in the ...WithSBHit test. Certainly they test > that the state of the pending navigation takes precedence, which I think is the > important facet of the code. That test doesn't actually display an interstitial or create the transient entry, though. I think these tests would be very useful to verify that the assumptions are actually correct. https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:383: NavigationEntry* entry = web_contents()->GetController().GetActiveEntry(); On 2013/12/16 23:00:22, Greg Billock wrote: > On 2013/12/16 22:36:11, mattm wrote: > > Looking at the comments of GetActiveEntry vs GetVisibleEntry, it seems like > this > > should be GetVisibleEntry? > > (Though this might cause some trouble with the other comment about only using > > Pending or Committed, since the navigationController doesn't expose the > > "safe_to_show_pending" calculation otherwise.) > > Yeah, I thought about that, but decided to go with GetActiveEntry since that's > what the match setting stuff uses. I think that in the cases when they get > called, they'll be the same, but that whole system has a lot of complexity, so I > could be mistaken, especially in odd edge cases. I don't think the setting and getting necessarily should be the same. If there is a pending, renderer-initiated navigation that gets matched, then it clearly makes sense to set the flag on that pending navigation. On the other hand, if you are on a malware flagged page and it starts a render-initiated pending navigation to a non-flagged page, you would still want to show the malware icon until that navigation actually commits. Otherwise a page could theoretically use this to hide the malware icon from showing. https://codereview.chromium.org/99423007/diff/100001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/100001/chrome/browser/safe_brow... chrome/browser/safe_browsing/client_side_detection_host.cc:390: NavigationEntry* entry = web_contents()->GetController().GetPendingEntry(); I think there should still be some comment about this.
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { On 2013/12/16 23:28:51, mattm wrote: > > Sorry, my comment neglected a point. In the case I mentioned, if my assumptions > are correct, there would not be a pending entry, but there would be a transient > entry. So GetPendingEntry would return NULL, GetActiveEntry would return the > transient entry (the interstitial) rather than the current entry, which actually > has the flag set, so you would not show the icon in that case. Are there any such cases? That is, can we basically over-write a non-ending nav entry with an interstitial? If so, I can detect such cases with the check for the nav type, but I'm not sure what I'd do then. If the active entry has been replaced, will GetVisibleEntry then give me the right entry? Looking at the code, it looks like in that case (type == INTERSTITIAL && GetPendingEntry == NULL) I want GetLastCommittedEntry. Right? Or does that just not happen in practice? > > > > It seems like we also need some tests that actually create interstitials and > > > make sure DidPageReceiveSafeBrowsingMatch returns the correct thing. > > > > I think the tests cover that case in the ...WithSBHit test. Certainly they > test > > that the state of the pending navigation takes precedence, which I think is > the > > important facet of the code. > > > That test doesn't actually display an interstitial or create the transient > entry, though. I think these tests would be very useful to verify that the > assumptions are actually correct. I guess they'd verify that we can create those conditions, but I don't know that they'd verify those are the conditions that obtain in practice. I don't know the nav controller well enough to be able to predict strange edge cases. Reading the API, I'd guess that GetVisibleEntry is preferable to GetActiveEntry, but I'm not sure I could tell what the transient entries are or when they occur. https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/80001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:383: NavigationEntry* entry = web_contents()->GetController().GetActiveEntry(); On 2013/12/16 23:28:51, mattm wrote: > On 2013/12/16 23:00:22, Greg Billock wrote: > > On 2013/12/16 22:36:11, mattm wrote: > > > Looking at the comments of GetActiveEntry vs GetVisibleEntry, it seems like > > this > > > should be GetVisibleEntry? > > > (Though this might cause some trouble with the other comment about only > using > > > Pending or Committed, since the navigationController doesn't expose the > > > "safe_to_show_pending" calculation otherwise.) > > > > Yeah, I thought about that, but decided to go with GetActiveEntry since that's > > what the match setting stuff uses. I think that in the cases when they get > > called, they'll be the same, but that whole system has a lot of complexity, so > I > > could be mistaken, especially in odd edge cases. > > I don't think the setting and getting necessarily should be the same. > > If there is a pending, renderer-initiated navigation that gets matched, then it > clearly makes sense to set the flag on that pending navigation. > > On the other hand, if you are on a malware flagged page and it starts a > render-initiated pending navigation to a non-flagged page, you would still want > to show the malware icon until that navigation actually commits. Otherwise a > page could theoretically use this to hide the malware icon from showing. Yeah, that's tricky. It looks like there's good logic in the GetVisibleEntry around that point, so that's probably the thing to use. Will update. (Also, GetActiveEntry basically commands us to use GetVisibleEntry...) https://codereview.chromium.org/99423007/diff/100001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/100001/chrome/browser/safe_brow... chrome/browser/safe_browsing/client_side_detection_host.cc:390: NavigationEntry* entry = web_contents()->GetController().GetPendingEntry(); On 2013/12/16 23:28:51, mattm wrote: > I think there should still be some comment about this. drat. That got dropped in the shuffle. Done.
lgtm https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { On 2013/12/16 23:59:42, Greg Billock wrote: > On 2013/12/16 23:28:51, mattm wrote: > > > > Sorry, my comment neglected a point. In the case I mentioned, if my > assumptions > > are correct, there would not be a pending entry, but there would be a > transient > > entry. So GetPendingEntry would return NULL, GetActiveEntry would return the > > transient entry (the interstitial) rather than the current entry, which > actually > > has the flag set, so you would not show the icon in that case. > > Are there any such cases? That is, can we basically over-write a non-ending nav > entry with an interstitial? If so, I can detect such cases with the check for > the nav type, but I'm not sure what I'd do then. If the active entry has been > replaced, will GetVisibleEntry then give me the right entry? Looking at the > code, it looks like in that case (type == INTERSTITIAL && GetPendingEntry == > NULL) I want GetLastCommittedEntry. Right? Or does that just not happen in > practice? Yeah, a safebrowsing match can occur either on a pending navigation (when the match is on the main URL), or after the navigation is committed (when the match is on a subresource or a client-side-phishing match). The change you made seems right. > > > > > > It seems like we also need some tests that actually create interstitials > and > > > > make sure DidPageReceiveSafeBrowsingMatch returns the correct thing. > > > > > > I think the tests cover that case in the ...WithSBHit test. Certainly they > > test > > > that the state of the pending navigation takes precedence, which I think is > > the > > > important facet of the code. > > > > > > That test doesn't actually display an interstitial or create the transient > > entry, though. I think these tests would be very useful to verify that the > > assumptions are actually correct. > > I guess they'd verify that we can create those conditions, but I don't know that > they'd verify those are the conditions that obtain in practice. I don't know the > nav controller well enough to be able to predict strange edge cases. Reading the > API, I'd guess that GetVisibleEntry is preferable to GetActiveEntry, but I'm not > sure I could tell what the transient entries are or when they occur.
https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... File chrome/browser/safe_browsing/client_side_detection_host.cc (right): https://codereview.chromium.org/99423007/diff/40001/chrome/browser/safe_brows... chrome/browser/safe_browsing/client_side_detection_host.cc:379: if (web_contents()->GetController().GetPendingEntry()) { On 2013/12/17 00:09:19, mattm wrote: > On 2013/12/16 23:59:42, Greg Billock wrote: > > On 2013/12/16 23:28:51, mattm wrote: > > > > > > Sorry, my comment neglected a point. In the case I mentioned, if my > > assumptions > > > are correct, there would not be a pending entry, but there would be a > > transient > > > entry. So GetPendingEntry would return NULL, GetActiveEntry would return > the > > > transient entry (the interstitial) rather than the current entry, which > > actually > > > has the flag set, so you would not show the icon in that case. > > > > Are there any such cases? That is, can we basically over-write a non-ending > nav > > entry with an interstitial? If so, I can detect such cases with the check for > > the nav type, but I'm not sure what I'd do then. If the active entry has been > > replaced, will GetVisibleEntry then give me the right entry? Looking at the > > code, it looks like in that case (type == INTERSTITIAL && GetPendingEntry == > > NULL) I want GetLastCommittedEntry. Right? Or does that just not happen in > > practice? > > Yeah, a safebrowsing match can occur either on a pending navigation (when the > match is on the main URL), or after the navigation is committed (when the match > is on a subresource or a client-side-phishing match). The change you made seems > right. Cool. Yeah, I added a test for that case. Thanks! > > > > > > > > > It seems like we also need some tests that actually create interstitials > > and > > > > > make sure DidPageReceiveSafeBrowsingMatch returns the correct thing. > > > > > > > > I think the tests cover that case in the ...WithSBHit test. Certainly they > > > test > > > > that the state of the pending navigation takes precedence, which I think > is > > > the > > > > important facet of the code. > > > > > > > > > That test doesn't actually display an interstitial or create the transient > > > entry, though. I think these tests would be very useful to verify that the > > > assumptions are actually correct. > > > > I guess they'd verify that we can create those conditions, but I don't know > that > > they'd verify those are the conditions that obtain in practice. I don't know > the > > nav controller well enough to be able to predict strange edge cases. Reading > the > > API, I'd guess that GetVisibleEntry is preferable to GetActiveEntry, but I'm > not > > sure I could tell what the transient entries are or when they occur. >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/99423007/120001
Message was sent while issue was closed.
Change committed as 241168 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
