Commit e6e17b9d authored by felt's avatar felt Committed by Commit bot

Update the Safe Browsing interstitial histograms

This organizes the Safe Browsing interstitial histograms in a way that
should scale better as new warnings or buttons are added.

Plus: all of the recording is done from within the command handling
method, instead of in the secondary methods like OnProceed.

BUG=

Review URL: https://codereview.chromium.org/576653002

Cr-Commit-Position: refs/heads/master@{#295294}
parent 83e3491e
...@@ -113,26 +113,6 @@ const char kEventNameOther[] = "safebrowsing_other_interstitial_"; ...@@ -113,26 +113,6 @@ const char kEventNameOther[] = "safebrowsing_other_interstitial_";
base::LazyInstance<SafeBrowsingBlockingPage::UnsafeResourceMap> base::LazyInstance<SafeBrowsingBlockingPage::UnsafeResourceMap>
g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER; g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;
// This enum is used for a histogram. Don't reorder, delete, or insert
// elements. New elements should be added before MAX_ACTION only.
enum DetailedDecision {
MALWARE_SHOW_NEW_SITE = 0,
MALWARE_PROCEED_NEW_SITE,
MALWARE_SHOW_CROSS_SITE,
MALWARE_PROCEED_CROSS_SITE,
PHISHING_SHOW_NEW_SITE,
PHISHING_PROCEED_NEW_SITE,
PHISHING_SHOW_CROSS_SITE,
PHISHING_PROCEED_CROSS_SITE,
MAX_DETAILED_ACTION
};
void RecordDetailedUserAction(DetailedDecision decision) {
UMA_HISTOGRAM_ENUMERATION("SB2.InterstitialActionDetails",
decision,
MAX_DETAILED_ACTION);
}
} // namespace } // namespace
// static // static
...@@ -201,7 +181,11 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage( ...@@ -201,7 +181,11 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
else else
interstitial_type_ = TYPE_PHISHING; interstitial_type_ = TYPE_PHISHING;
RecordUserAction(SHOW); RecordUserDecision(SHOW);
RecordUserInteraction(TOTAL_VISITS);
if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled))
RecordUserDecision(PROCEEDING_DISABLED);
HistoryService* history_service = HistoryServiceFactory::GetForProfile( HistoryService* history_service = HistoryServiceFactory::GetForProfile(
Profile::FromBrowserContext(web_contents->GetBrowserContext()), Profile::FromBrowserContext(web_contents->GetBrowserContext()),
Profile::EXPLICIT_ACCESS); Profile::EXPLICIT_ACCESS);
...@@ -284,10 +268,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) { ...@@ -284,10 +268,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
if (command == kLearnMoreCommand) { if (command == kLearnMoreCommand) {
// User pressed "Learn more". // User pressed "Learn more".
#if defined(ENABLE_EXTENSIONS) RecordUserInteraction(SHOW_LEARN_MORE);
if (sampling_event_.get())
sampling_event_->set_has_viewed_learn_more(true);
#endif
GURL learn_more_url(interstitial_type_ == TYPE_PHISHING ? GURL learn_more_url(interstitial_type_ == TYPE_PHISHING ?
kLearnMorePhishingUrlV2 : kLearnMoreMalwareUrlV2); kLearnMorePhishingUrlV2 : kLearnMoreMalwareUrlV2);
learn_more_url = google_util::AppendGoogleLocaleParam( learn_more_url = google_util::AppendGoogleLocaleParam(
...@@ -303,6 +284,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) { ...@@ -303,6 +284,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
if (command == kShowPrivacyCommand) { if (command == kShowPrivacyCommand) {
// User pressed "Safe Browsing privacy policy". // User pressed "Safe Browsing privacy policy".
RecordUserInteraction(SHOW_PRIVACY_POLICY);
GURL privacy_url( GURL privacy_url(
l10n_util::GetStringUTF8(IDS_SAFE_BROWSING_PRIVACY_POLICY_URL)); l10n_util::GetStringUTF8(IDS_SAFE_BROWSING_PRIVACY_POLICY_URL));
privacy_url = google_util::AppendGoogleLocaleParam( privacy_url = google_util::AppendGoogleLocaleParam(
...@@ -321,6 +303,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) { ...@@ -321,6 +303,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) { if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) {
proceed_blocked = true; proceed_blocked = true;
} else { } else {
RecordUserDecision(PROCEED);
interstitial_page_->Proceed(); interstitial_page_->Proceed();
// |this| has been deleted after Proceed() returns. // |this| has been deleted after Proceed() returns.
return; return;
...@@ -328,6 +311,8 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) { ...@@ -328,6 +311,8 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
} }
if (command == kTakeMeBackCommand || proceed_blocked) { if (command == kTakeMeBackCommand || proceed_blocked) {
// Don't record the user action here because there are other ways of
// triggering DontProceed, like clicking the back button.
if (is_main_frame_load_blocked_) { if (is_main_frame_load_blocked_) {
// If the load is blocked, we want to close the interstitial and discard // If the load is blocked, we want to close the interstitial and discard
// the pending entry. // the pending entry.
...@@ -374,6 +359,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) { ...@@ -374,6 +359,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
std::string bad_url_spec = unsafe_resources_[element_index].url.spec(); std::string bad_url_spec = unsafe_resources_[element_index].url.spec();
if (command == kShowDiagnosticCommand) { if (command == kShowDiagnosticCommand) {
// We're going to take the user to Google's SafeBrowsing diagnostic page. // We're going to take the user to Google's SafeBrowsing diagnostic page.
RecordUserInteraction(SHOW_DIAGNOSTIC);
std::string diagnostic = std::string diagnostic =
base::StringPrintf(kSbDiagnosticUrl, base::StringPrintf(kSbDiagnosticUrl,
net::EscapeQueryParamValue(bad_url_spec, true).c_str()); net::EscapeQueryParamValue(bad_url_spec, true).c_str());
...@@ -392,11 +378,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) { ...@@ -392,11 +378,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& cmd) {
} }
if (command == kExpandedSeeMoreCommand) { if (command == kExpandedSeeMoreCommand) {
#if defined(ENABLE_EXTENSIONS) RecordUserInteraction(SHOW_ADVANCED);
// ExperienceSampling: We track that the user expanded the details.
if (sampling_event_.get())
sampling_event_->set_has_viewed_details(true);
#endif
return; return;
} }
...@@ -420,7 +402,6 @@ void SafeBrowsingBlockingPage::SetReportingPreference(bool report) { ...@@ -420,7 +402,6 @@ void SafeBrowsingBlockingPage::SetReportingPreference(bool report) {
void SafeBrowsingBlockingPage::OnProceed() { void SafeBrowsingBlockingPage::OnProceed() {
proceeded_ = true; proceeded_ = true;
RecordUserAction(PROCEED);
// Send the malware details, if we opted to. // Send the malware details, if we opted to.
FinishMalwareDetails(malware_details_proceed_delay_ms_); FinishMalwareDetails(malware_details_proceed_delay_ms_);
...@@ -440,12 +421,6 @@ void SafeBrowsingBlockingPage::OnProceed() { ...@@ -440,12 +421,6 @@ void SafeBrowsingBlockingPage::OnProceed() {
unsafe_resource_map->erase(iter); unsafe_resource_map->erase(iter);
} }
#if defined(ENABLE_EXTENSIONS)
// ExperienceSampling: Notify that user decided to proceed.
if (sampling_event_.get())
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kProceed);
#endif
// Now that this interstitial is gone, we can show the new one. // Now that this interstitial is gone, we can show the new one.
if (blocking_page) if (blocking_page)
blocking_page->Show(); blocking_page->Show();
...@@ -470,7 +445,9 @@ void SafeBrowsingBlockingPage::OnDontProceed() { ...@@ -470,7 +445,9 @@ void SafeBrowsingBlockingPage::OnDontProceed() {
if (proceeded_) if (proceeded_)
return; return;
RecordUserAction(DONT_PROCEED); if (!IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled))
RecordUserDecision(DONT_PROCEED);
// Send the malware details, if we opted to. // Send the malware details, if we opted to.
FinishMalwareDetails(0); // No delay FinishMalwareDetails(0); // No delay
...@@ -499,13 +476,6 @@ void SafeBrowsingBlockingPage::OnDontProceed() { ...@@ -499,13 +476,6 @@ void SafeBrowsingBlockingPage::OnDontProceed() {
navigation_entry_index_to_remove_)); navigation_entry_index_to_remove_));
navigation_entry_index_to_remove_ = -1; navigation_entry_index_to_remove_ = -1;
} }
#if defined(ENABLE_EXTENSIONS)
// ExperienceSampling: Notify that user decided to go back.
// This also occurs if the user navigates away or closes the tab.
if (sampling_event_.get())
sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kDeny);
#endif
} }
void SafeBrowsingBlockingPage::OnGotHistoryCount(bool success, void SafeBrowsingBlockingPage::OnGotHistoryCount(bool success,
...@@ -515,108 +485,88 @@ void SafeBrowsingBlockingPage::OnGotHistoryCount(bool success, ...@@ -515,108 +485,88 @@ void SafeBrowsingBlockingPage::OnGotHistoryCount(bool success,
num_visits_ = num_visits; num_visits_ = num_visits;
} }
void SafeBrowsingBlockingPage::RecordUserAction(BlockingPageEvent event) { void SafeBrowsingBlockingPage::RecordUserDecision(Decision decision) {
// This enum is used for a histogram. Don't reorder, delete, or insert switch (interstitial_type_) {
// elements. New elements should be added before MAX_ACTION only. case TYPE_MALWARE:
enum { UMA_HISTOGRAM_ENUMERATION("interstitial.malware.decision",
MALWARE_SHOW = 0, decision,
MALWARE_DONT_PROCEED, MAX_DECISION);
MALWARE_FORCED_DONT_PROCEED,
MALWARE_PROCEED,
MULTIPLE_SHOW,
MULTIPLE_DONT_PROCEED,
MULTIPLE_FORCED_DONT_PROCEED,
MULTIPLE_PROCEED,
PHISHING_SHOW,
PHISHING_DONT_PROCEED,
PHISHING_FORCED_DONT_PROCEED,
PHISHING_PROCEED,
MALWARE_SHOW_ADVANCED,
MULTIPLE_SHOW_ADVANCED,
PHISHING_SHOW_ADVANCED,
MAX_ACTION
} histogram_action = MAX_ACTION;
switch (event) {
case SHOW:
switch (interstitial_type_) {
case TYPE_MALWARE:
histogram_action = MALWARE_SHOW;
break;
case TYPE_PHISHING:
histogram_action = PHISHING_SHOW;
break;
}
break; break;
case PROCEED: case TYPE_PHISHING:
switch (interstitial_type_) { UMA_HISTOGRAM_ENUMERATION("interstitial.phishing.decision",
case TYPE_MALWARE: decision,
histogram_action = MALWARE_PROCEED; MAX_DECISION);
break;
case TYPE_PHISHING:
histogram_action = PHISHING_PROCEED;
break;
}
break; break;
case DONT_PROCEED: default:
if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) { NOTREACHED();
switch (interstitial_type_) { }
case TYPE_MALWARE:
histogram_action = MALWARE_FORCED_DONT_PROCEED; #if defined(ENABLE_EXTENSIONS)
break; if (sampling_event_.get()) {
case TYPE_PHISHING: switch (decision) {
histogram_action = PHISHING_FORCED_DONT_PROCEED; case PROCEED:
break; sampling_event_->CreateUserDecisionEvent(
} ExperienceSamplingEvent::kProceed);
} else { break;
switch (interstitial_type_) { case DONT_PROCEED:
case TYPE_MALWARE: sampling_event_->CreateUserDecisionEvent(
histogram_action = MALWARE_DONT_PROCEED; ExperienceSamplingEvent::kDeny);
break; break;
case TYPE_PHISHING: case SHOW:
histogram_action = PHISHING_DONT_PROCEED; case PROCEEDING_DISABLED:
break; case MAX_DECISION:
} break;
} }
}
#endif
// Record additional information about malware sites that users have
// visited before.
if (num_visits_ < 1 || interstitial_type_ != TYPE_MALWARE)
return;
if (decision == PROCEED || decision == DONT_PROCEED) {
UMA_HISTOGRAM_ENUMERATION("interstitial.malware.decision.repeat_visit",
SHOW,
MAX_DECISION);
UMA_HISTOGRAM_ENUMERATION("interstitial.malware.decision.repeat_visit",
decision,
MAX_DECISION);
}
}
void SafeBrowsingBlockingPage::RecordUserInteraction(Interaction interaction) {
switch (interstitial_type_) {
case TYPE_MALWARE:
UMA_HISTOGRAM_ENUMERATION("interstitial.malware.interaction",
interaction,
MAX_INTERACTION);
break; break;
case SHOW_ADVANCED: case TYPE_PHISHING:
switch (interstitial_type_) { UMA_HISTOGRAM_ENUMERATION("interstitial.phishing.interaction",
case TYPE_MALWARE: interaction,
histogram_action = MALWARE_SHOW_ADVANCED; MAX_INTERACTION);
break;
case TYPE_PHISHING:
histogram_action = PHISHING_SHOW_ADVANCED;
break;
}
break; break;
default: default:
NOTREACHED() << "Unexpected event: " << event; NOTREACHED();
}
if (histogram_action == MAX_ACTION) {
NOTREACHED();
} else {
UMA_HISTOGRAM_ENUMERATION("SB2.InterstitialAction", histogram_action,
MAX_ACTION);
} }
if (event == PROCEED || event == DONT_PROCEED) { #if defined(ENABLE_EXTENSIONS)
if (num_visits_ == 0) { if (!sampling_event_.get())
RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ? return;
MALWARE_SHOW_NEW_SITE : PHISHING_SHOW_NEW_SITE); switch (interaction) {
if (event == PROCEED) { case SHOW_LEARN_MORE:
RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ? sampling_event_->set_has_viewed_learn_more(true);
MALWARE_PROCEED_NEW_SITE : PHISHING_PROCEED_NEW_SITE); break;
} case SHOW_ADVANCED:
} sampling_event_->set_has_viewed_details(true);
if (unsafe_resources_[0].is_subresource) { break;
RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ? case SHOW_PRIVACY_POLICY:
MALWARE_SHOW_CROSS_SITE : PHISHING_SHOW_CROSS_SITE); case SHOW_DIAGNOSTIC:
if (event == PROCEED) { case TOTAL_VISITS:
RecordDetailedUserAction((interstitial_type_ == TYPE_MALWARE) ? case MAX_INTERACTION:
MALWARE_PROCEED_CROSS_SITE : PHISHING_PROCEED_CROSS_SITE); break;
}
}
} }
#endif
} }
void SafeBrowsingBlockingPage::FinishMalwareDetails(int64 delay_ms) { void SafeBrowsingBlockingPage::FinishMalwareDetails(int64 delay_ms) {
......
...@@ -125,16 +125,27 @@ class SafeBrowsingBlockingPage : public content::InterstitialPageDelegate { ...@@ -125,16 +125,27 @@ class SafeBrowsingBlockingPage : public content::InterstitialPageDelegate {
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingBlockingPageTest, FRIEND_TEST_ALL_PREFIXES(SafeBrowsingBlockingPageTest,
MalwareReportsToggling); MalwareReportsToggling);
enum BlockingPageEvent { // These enums are used for histograms. Don't reorder, delete, or insert
// elements. New elements should be added before MAX_ACTION only.
enum Decision {
SHOW, SHOW,
PROCEED, PROCEED,
DONT_PROCEED, DONT_PROCEED,
PROCEEDING_DISABLED,
MAX_DECISION
};
enum Interaction {
TOTAL_VISITS,
SHOW_ADVANCED, SHOW_ADVANCED,
SHOW_PRIVACY_POLICY,
SHOW_DIAGNOSTIC,
SHOW_LEARN_MORE,
MAX_INTERACTION
}; };
// Records a user action for this interstitial, using the form // Record a user decision or interaction to the appropriate UMA histogram.
// SBInterstitial[Phishing|Malware|Multiple][Show|Proceed|DontProceed]. void RecordUserDecision(Decision decision);
void RecordUserAction(BlockingPageEvent event); void RecordUserInteraction(Interaction interaction);
// Used to query the HistoryService to see if the URL is in history. For UMA. // Used to query the HistoryService to see if the URL is in history. For UMA.
void OnGotHistoryCount(bool success, int num_visits, base::Time first_visit); void OnGotHistoryCount(bool success, int num_visits, base::Time first_visit);
......
...@@ -10453,6 +10453,45 @@ Therefore, the affected-histogram name has to have at least one dot in it. ...@@ -10453,6 +10453,45 @@ Therefore, the affected-histogram name has to have at least one dot in it.
</summary> </summary>
</histogram> </histogram>
<histogram name="interstitial.malware.decision" enum="SB3InterstitialDecision">
<owner>felt@chromium.org</owner>
<summary>
User decision when presented with a Safe Browsing malware interstitial.
</summary>
</histogram>
<histogram name="interstitial.malware.decision.repeat_visit"
enum="SB3InterstitialDecision">
<owner>felt@chromium.org</owner>
<summary>
User decision when presented with a Safe Browsing malware interstitial for a
website that the user has visited before.
</summary>
</histogram>
<histogram name="interstitial.malware.interaction"
enum="SB3InterstitialInteraction">
<owner>felt@chromium.org</owner>
<summary>
User interactions with a Safe Browsing malware interstitial.
</summary>
</histogram>
<histogram name="interstitial.phishing.decision" enum="SB3InterstitialDecision">
<owner>felt@chromium.org</owner>
<summary>
User decision when presented with a Safe Browsing phishing interstitial.
</summary>
</histogram>
<histogram name="interstitial.phishing.interaction"
enum="SB3InterstitialInteraction">
<owner>felt@chromium.org</owner>
<summary>
User interactions with a Safe Browsing phishing interstitial.
</summary>
</histogram>
<histogram name="interstitial.ssl" enum="SSLResponseTypesV2"> <histogram name="interstitial.ssl" enum="SSLResponseTypesV2">
<owner>felt@chromium.org</owner> <owner>felt@chromium.org</owner>
<summary> <summary>
...@@ -27537,6 +27576,9 @@ Therefore, the affected-histogram name has to have at least one dot in it. ...@@ -27537,6 +27576,9 @@ Therefore, the affected-histogram name has to have at least one dot in it.
</histogram> </histogram>
<histogram name="SB2.InterstitialAction" enum="SB2InterstitialAction"> <histogram name="SB2.InterstitialAction" enum="SB2InterstitialAction">
<obsolete>
Deprecated, replaced by: interstitial.malware.* and interstitial.phishing.*.
</obsolete>
<owner>felt@chromium.org</owner> <owner>felt@chromium.org</owner>
<summary> <summary>
Track number of times Safe Browsing interstitials have been shown, and how Track number of times Safe Browsing interstitials have been shown, and how
...@@ -27546,6 +27588,9 @@ Therefore, the affected-histogram name has to have at least one dot in it. ...@@ -27546,6 +27588,9 @@ Therefore, the affected-histogram name has to have at least one dot in it.
<histogram name="SB2.InterstitialActionDetails" <histogram name="SB2.InterstitialActionDetails"
enum="SB2InterstitialActionDetails"> enum="SB2InterstitialActionDetails">
<obsolete>
Deprecated, replaced by: interstitial.malware.* and interstitial.phishing.*.
</obsolete>
<owner>felt@chromium.org</owner> <owner>felt@chromium.org</owner>
<summary> <summary>
Tracks the click-through rate for specific cases of the interstitial. Tracks the click-through rate for specific cases of the interstitial.
...@@ -49410,6 +49455,9 @@ To add a new entry, add it with any value and run test to compute valid value. ...@@ -49410,6 +49455,9 @@ To add a new entry, add it with any value and run test to compute valid value.
</enum> </enum>
<enum name="SB2InterstitialAction" type="int"> <enum name="SB2InterstitialAction" type="int">
<obsolete>
Deprecated 9/2014.
</obsolete>
<int value="0" label="MALWARE_SHOW"/> <int value="0" label="MALWARE_SHOW"/>
<int value="1" label="MALWARE_DONT_PROCEED"/> <int value="1" label="MALWARE_DONT_PROCEED"/>
<int value="2" label="MALWARE_FORCED_DONT_PROCEED"/> <int value="2" label="MALWARE_FORCED_DONT_PROCEED"/>
...@@ -49428,6 +49476,9 @@ To add a new entry, add it with any value and run test to compute valid value. ...@@ -49428,6 +49476,9 @@ To add a new entry, add it with any value and run test to compute valid value.
</enum> </enum>
<enum name="SB2InterstitialActionDetails" type="int"> <enum name="SB2InterstitialActionDetails" type="int">
<obsolete>
Deprecated 9/2014.
</obsolete>
<int value="0" label="MALWARE_SHOW_NEW_SITE"/> <int value="0" label="MALWARE_SHOW_NEW_SITE"/>
<int value="1" label="MALWARE_PROCEED_NEW_SITE"/> <int value="1" label="MALWARE_PROCEED_NEW_SITE"/>
<int value="2" label="MALWARE_SHOW_CROSS_SITE"/> <int value="2" label="MALWARE_SHOW_CROSS_SITE"/>
...@@ -49480,6 +49531,21 @@ To add a new entry, add it with any value and run test to compute valid value. ...@@ -49480,6 +49531,21 @@ To add a new entry, add it with any value and run test to compute valid value.
<int value="7" label="BACKUP_NETWORK_SUCCESS"/> <int value="7" label="BACKUP_NETWORK_SUCCESS"/>
</enum> </enum>
<enum name="SB3InterstitialDecision" type="int">
<int value="0" label="SHOW"/>
<int value="1" label="PROCEED"/>
<int value="2" label="DONT_PROCEED"/>
<int value="3" label="PROCEEDING_DISABLED"/>
</enum>
<enum name="SB3InterstitialInteraction" type="int">
<int value="0" label="TOTAL_VISITS"/>
<int value="1" label="SHOW_ADVANCED"/>
<int value="2" label="SHOW_PRIVACY"/>
<int value="3" label="SHOW_DIAGNOSTIC"/>
<int value="4" label="SHOW_LEARN_MORE"/>
</enum>
<enum name="SBClientDetectionPreClassificationCheckFail" type="int"> <enum name="SBClientDetectionPreClassificationCheckFail" type="int">
<int value="0" label="PROXY_FETCH"/> <int value="0" label="PROXY_FETCH"/>
<int value="1" label="PRIVATE_IP"/> <int value="1" label="PRIVATE_IP"/>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment