Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(636)

Side by Side Diff: content/browser/frame_host/navigation_controller_impl.cc

Issue 1171973004: Ensure the new navigation classifier works in the real world. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: nits Created 5 years, 6 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
« no previous file with comments | « chrome/common/crash_keys.cc ('k') | content/browser/frame_host/render_frame_host_impl.h » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 // Copyright 2013 The Chromium Authors. All rights reserved. 1 // Copyright 2013 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 #include "content/browser/frame_host/navigation_controller_impl.h" 5 #include "content/browser/frame_host/navigation_controller_impl.h"
6 6
7 #include "base/bind.h" 7 #include "base/bind.h"
8 #include "base/command_line.h" 8 #include "base/command_line.h"
9 #include "base/debug/crash_logging.h"
9 #include "base/logging.h" 10 #include "base/logging.h"
10 #include "base/metrics/histogram.h" 11 #include "base/metrics/histogram.h"
11 #include "base/strings/string_number_conversions.h" // Temporary 12 #include "base/strings/string_number_conversions.h" // Temporary
12 #include "base/strings/string_util.h" 13 #include "base/strings/string_util.h"
13 #include "base/strings/utf_string_conversions.h" 14 #include "base/strings/utf_string_conversions.h"
14 #include "base/time/time.h" 15 #include "base/time/time.h"
15 #include "base/trace_event/trace_event.h" 16 #include "base/trace_event/trace_event.h"
16 #include "build/build_config.h" 17 #include "build/build_config.h"
17 #include "cc/base/switches.h" 18 #include "cc/base/switches.h"
18 #include "components/mime_util/mime_util.h" 19 #include "components/mime_util/mime_util.h"
(...skipping 766 matching lines...) Expand 10 before | Expand all | Expand 10 after
785 if (params.url_is_unreachable && failed_pending_entry_id_ != 0) { 786 if (params.url_is_unreachable && failed_pending_entry_id_ != 0) {
786 details->did_replace_entry = failed_pending_entry_should_replace_; 787 details->did_replace_entry = failed_pending_entry_should_replace_;
787 } else { 788 } else {
788 details->did_replace_entry = pending_entry_ && 789 details->did_replace_entry = pending_entry_ &&
789 pending_entry_->should_replace_entry(); 790 pending_entry_->should_replace_entry();
790 } 791 }
791 792
792 // Do navigation-type specific actions. These will make and commit an entry. 793 // Do navigation-type specific actions. These will make and commit an entry.
793 details->type = ClassifyNavigation(rfh, params); 794 details->type = ClassifyNavigation(rfh, params);
794 NavigationType new_type = ClassifyNavigationWithoutPageID(rfh, params); 795 NavigationType new_type = ClassifyNavigationWithoutPageID(rfh, params);
796 if (details->type == NAVIGATION_TYPE_NAV_IGNORE &&
797 new_type == NAVIGATION_TYPE_NAV_IGNORE) {
798 base::debug::SetCrashKeyValue("369661-doubleignore",
799 rfh->CommitCountString());
800 }
795 bool ignore_mismatch = false; 801 bool ignore_mismatch = false;
796 // There are disagreements on some Android bots over SAME_PAGE between the two 802 // There are disagreements on some Android bots over SAME_PAGE between the two
797 // classifiers so ignore disagreements if that's the case. 803 // classifiers so ignore disagreements if that's the case.
798 ignore_mismatch |= details->type == NAVIGATION_TYPE_EXISTING_PAGE && 804 ignore_mismatch |= details->type == NAVIGATION_TYPE_EXISTING_PAGE &&
799 new_type == NAVIGATION_TYPE_SAME_PAGE; 805 new_type == NAVIGATION_TYPE_SAME_PAGE;
800 ignore_mismatch |= details->type == NAVIGATION_TYPE_SAME_PAGE && 806 ignore_mismatch |= details->type == NAVIGATION_TYPE_SAME_PAGE &&
801 new_type == NAVIGATION_TYPE_EXISTING_PAGE; 807 new_type == NAVIGATION_TYPE_EXISTING_PAGE;
802 // There are mismatches in the field where the new classifier thinks it's 808 // There are mismatches in the field where the new classifier thinks it's
803 // AUTO_SUBFRAME and the old classifier somehow thinks it's NEW or IGNORE. For 809 // AUTO_SUBFRAME and the old classifier somehow thinks it's NEW or IGNORE. For
804 // IGNORE we know of at least one repro (https://crbug.com/492875), and for 810 // IGNORE we know of at least one repro (https://crbug.com/492875), and for
805 // NEW it's not entirely clear what's going on, but we're pretty sure the new 811 // NEW it's not entirely clear what's going on, but we're pretty sure the new
806 // classifier is correct for both cases, so we're letting these mismatches go. 812 // classifier is correct for both cases, so we're letting these mismatches go.
807 ignore_mismatch |= details->type == NAVIGATION_TYPE_NEW_SUBFRAME && 813 ignore_mismatch |= details->type == NAVIGATION_TYPE_NEW_SUBFRAME &&
808 new_type == NAVIGATION_TYPE_AUTO_SUBFRAME; 814 new_type == NAVIGATION_TYPE_AUTO_SUBFRAME;
809 ignore_mismatch |= details->type == NAVIGATION_TYPE_NAV_IGNORE && 815 ignore_mismatch |= details->type == NAVIGATION_TYPE_NAV_IGNORE &&
810 new_type == NAVIGATION_TYPE_AUTO_SUBFRAME; 816 new_type == NAVIGATION_TYPE_AUTO_SUBFRAME;
811 if (base::CommandLine::ForCurrentProcess()->HasSwitch( 817 if (base::CommandLine::ForCurrentProcess()->HasSwitch(
812 switches::kSitePerProcess)) { 818 switches::kSitePerProcess)) {
813 // We know that the old classifier is wrong for OOPIFs, so use the new one 819 // We know that the old classifier is wrong for OOPIFs, so use the new one
814 // in --site-per-process mode. 820 // in --site-per-process mode.
815 details->type = new_type; 821 details->type = new_type;
816 ignore_mismatch = true; 822 ignore_mismatch = true;
817 } 823 }
818 if (!ignore_mismatch) { 824 if (!ignore_mismatch) {
819 DCHECK_EQ(details->type, new_type); 825 base::debug::SetCrashKeyValue("369661-oldtype",
826 base::IntToString(details->type));
827 base::debug::SetCrashKeyValue("369661-newtype",
828 base::IntToString(new_type));
829 base::debug::SetCrashKeyValue("369661-navurl", params.url.spec());
830 base::debug::SetCrashKeyValue("369661-naventryid",
831 base::IntToString(params.nav_entry_id));
832 base::debug::SetCrashKeyValue("369661-didcreatenew",
833 params.did_create_new_entry ? "yes" : "no");
834 base::debug::SetCrashKeyValue("369661-pageid",
835 base::IntToString(params.page_id));
836 base::debug::SetCrashKeyValue(
837 "369661-maxpageid",
838 base::IntToString(delegate_->GetMaxPageIDForSiteInstance(
839 rfh->GetSiteInstance())));
840 CHECK_EQ(details->type, new_type);
820 } 841 }
821 842
822 // is_in_page must be computed before the entry gets committed. 843 // is_in_page must be computed before the entry gets committed.
823 details->is_in_page = IsURLInPageNavigation( 844 details->is_in_page = IsURLInPageNavigation(
824 params.url, params.was_within_same_page, rfh); 845 params.url, params.was_within_same_page, rfh);
825 846
826 switch (details->type) { 847 switch (details->type) {
827 case NAVIGATION_TYPE_NEW_PAGE: 848 case NAVIGATION_TYPE_NEW_PAGE:
828 RendererDidNavigateToNewPage(rfh, params, details->did_replace_entry); 849 RendererDidNavigateToNewPage(rfh, params, details->did_replace_entry);
829 break; 850 break;
(...skipping 99 matching lines...) Expand 10 before | Expand all | Expand 10 after
929 // - We were also getting these for failed loads (for example, bug 21849). 950 // - We were also getting these for failed loads (for example, bug 21849).
930 // The guess is that we get a "load commit" for the alternate error page, 951 // The guess is that we get a "load commit" for the alternate error page,
931 // but that doesn't affect the page ID, so we get the "old" one, which 952 // but that doesn't affect the page ID, so we get the "old" one, which
932 // could be invalid. This can also happen for a cross-site transition 953 // could be invalid. This can also happen for a cross-site transition
933 // that causes us to swap processes. Then the error page load will be in 954 // that causes us to swap processes. Then the error page load will be in
934 // a new process with no page IDs ever assigned (and hence a -1 value), 955 // a new process with no page IDs ever assigned (and hence a -1 value),
935 // yet the navigation controller still might have previous pages in its 956 // yet the navigation controller still might have previous pages in its
936 // list. 957 // list.
937 // 958 //
938 // In these cases, there's nothing we can do with them, so ignore. 959 // In these cases, there's nothing we can do with them, so ignore.
960 base::debug::SetCrashKeyValue("369661-oldignore",
961 rfh->CommitCountString() +
962 " no page id");
939 return NAVIGATION_TYPE_NAV_IGNORE; 963 return NAVIGATION_TYPE_NAV_IGNORE;
940 } 964 }
941 965
942 if (params.page_id > delegate_->GetMaxPageIDForSiteInstance( 966 if (params.page_id > delegate_->GetMaxPageIDForSiteInstance(
943 rfh->GetSiteInstance())) { 967 rfh->GetSiteInstance())) {
944 // Greater page IDs than we've ever seen before are new pages. We may or may 968 // Greater page IDs than we've ever seen before are new pages. We may or may
945 // not have a pending entry for the page, and this may or may not be the 969 // not have a pending entry for the page, and this may or may not be the
946 // main frame. 970 // main frame.
947 if (!rfh->GetParent()) 971 if (!rfh->GetParent())
948 return NAVIGATION_TYPE_NEW_PAGE; 972 return NAVIGATION_TYPE_NEW_PAGE;
949 973
950 // When this is a new subframe navigation, we should have a committed page 974 // When this is a new subframe navigation, we should have a committed page
951 // for which it's a suframe in. This may not be the case when an iframe is 975 // for which it's a suframe in. This may not be the case when an iframe is
952 // navigated on a popup navigated to about:blank (the iframe would be 976 // navigated on a popup navigated to about:blank (the iframe would be
953 // written into the popup by script on the main page). For these cases, 977 // written into the popup by script on the main page). For these cases,
954 // there isn't any navigation stuff we can do, so just ignore it. 978 // there isn't any navigation stuff we can do, so just ignore it.
955 if (!GetLastCommittedEntry()) 979 if (!GetLastCommittedEntry()) {
980 base::debug::SetCrashKeyValue("369661-oldignore",
981 rfh->CommitCountString() +
982 " new subframe no last committed");
956 return NAVIGATION_TYPE_NAV_IGNORE; 983 return NAVIGATION_TYPE_NAV_IGNORE;
984 }
957 985
958 // Valid subframe navigation. 986 // Valid subframe navigation.
959 return NAVIGATION_TYPE_NEW_SUBFRAME; 987 return NAVIGATION_TYPE_NEW_SUBFRAME;
960 } 988 }
961 989
962 // We only clear the session history when navigating to a new page. 990 // We only clear the session history when navigating to a new page.
963 DCHECK(!params.history_list_was_cleared); 991 DCHECK(!params.history_list_was_cleared);
964 992
965 // Now we know that the notification is for an existing page. Find that entry. 993 // Now we know that the notification is for an existing page. Find that entry.
966 int existing_entry_index = GetEntryIndexWithPageID( 994 int existing_entry_index = GetEntryIndexWithPageID(
(...skipping 30 matching lines...) Expand all
997 if (entries_[i]->site_instance()) 1025 if (entries_[i]->site_instance())
998 temp.append(base::IntToString(entries_[i]->site_instance()->GetId())); 1026 temp.append(base::IntToString(entries_[i]->site_instance()->GetId()));
999 else 1027 else
1000 temp.append("N"); 1028 temp.append("N");
1001 if (entries_[i]->site_instance() != rfh->GetSiteInstance()) 1029 if (entries_[i]->site_instance() != rfh->GetSiteInstance())
1002 temp.append("x"); 1030 temp.append("x");
1003 temp.append(","); 1031 temp.append(",");
1004 } 1032 }
1005 GURL url(temp); 1033 GURL url(temp);
1006 rfh->render_view_host()->Send(new ViewMsg_TempCrashWithData(url)); 1034 rfh->render_view_host()->Send(new ViewMsg_TempCrashWithData(url));
1035 base::debug::SetCrashKeyValue("369661-oldignore",
1036 rfh->CommitCountString() +
1037 " renderer smoking crack");
1007 return NAVIGATION_TYPE_NAV_IGNORE; 1038 return NAVIGATION_TYPE_NAV_IGNORE;
1008 } 1039 }
1009 NavigationEntryImpl* existing_entry = entries_[existing_entry_index].get(); 1040 NavigationEntryImpl* existing_entry = entries_[existing_entry_index].get();
1010 1041
1011 if (rfh->GetParent()) { 1042 if (rfh->GetParent()) {
1012 // All manual subframes would get new IDs and were handled above, so we 1043 // All manual subframes would get new IDs and were handled above, so we
1013 // know this is auto. Since the current page was found in the navigation 1044 // know this is auto. Since the current page was found in the navigation
1014 // entry list, we're guaranteed to have a last committed entry. 1045 // entry list, we're guaranteed to have a last committed entry.
1015 DCHECK(GetLastCommittedEntry()); 1046 DCHECK(GetLastCommittedEntry());
1016 return NAVIGATION_TYPE_AUTO_SUBFRAME; 1047 return NAVIGATION_TYPE_AUTO_SUBFRAME;
(...skipping 52 matching lines...) Expand 10 before | Expand all | Expand 10 after
1069 // this may or may not be the main frame. 1100 // this may or may not be the main frame.
1070 if (!rfh->GetParent()) { 1101 if (!rfh->GetParent()) {
1071 return NAVIGATION_TYPE_NEW_PAGE; 1102 return NAVIGATION_TYPE_NEW_PAGE;
1072 } 1103 }
1073 1104
1074 // When this is a new subframe navigation, we should have a committed page 1105 // When this is a new subframe navigation, we should have a committed page
1075 // in which it's a subframe. This may not be the case when an iframe is 1106 // in which it's a subframe. This may not be the case when an iframe is
1076 // navigated on a popup navigated to about:blank (the iframe would be 1107 // navigated on a popup navigated to about:blank (the iframe would be
1077 // written into the popup by script on the main page). For these cases, 1108 // written into the popup by script on the main page). For these cases,
1078 // there isn't any navigation stuff we can do, so just ignore it. 1109 // there isn't any navigation stuff we can do, so just ignore it.
1079 if (!GetLastCommittedEntry()) 1110 if (!GetLastCommittedEntry()) {
1111 base::debug::SetCrashKeyValue("369661-newignore",
1112 rfh->CommitCountString() +
1113 " new subframe no last committed");
1080 return NAVIGATION_TYPE_NAV_IGNORE; 1114 return NAVIGATION_TYPE_NAV_IGNORE;
1115 }
1081 1116
1082 // Valid subframe navigation. 1117 // Valid subframe navigation.
1083 return NAVIGATION_TYPE_NEW_SUBFRAME; 1118 return NAVIGATION_TYPE_NEW_SUBFRAME;
1084 } 1119 }
1085 1120
1086 // We only clear the session history when navigating to a new page. 1121 // We only clear the session history when navigating to a new page.
1087 DCHECK(!params.history_list_was_cleared); 1122 DCHECK(!params.history_list_was_cleared);
1088 1123
1089 if (rfh->GetParent()) { 1124 if (rfh->GetParent()) {
1090 // All manual subframes would be did_create_new_entry and handled above, so 1125 // All manual subframes would be did_create_new_entry and handled above, so
1091 // we know this is auto. 1126 // we know this is auto.
1092 if (GetLastCommittedEntry()) { 1127 if (GetLastCommittedEntry()) {
1093 return NAVIGATION_TYPE_AUTO_SUBFRAME; 1128 return NAVIGATION_TYPE_AUTO_SUBFRAME;
1094 } else { 1129 } else {
1095 // We ignore subframes created in non-committed pages; we'd appreciate if 1130 // We ignore subframes created in non-committed pages; we'd appreciate if
1096 // people stopped doing that. 1131 // people stopped doing that.
1132 base::debug::SetCrashKeyValue("369661-newignore",
1133 rfh->CommitCountString() +
1134 " auto subframe no last committed");
1097 return NAVIGATION_TYPE_NAV_IGNORE; 1135 return NAVIGATION_TYPE_NAV_IGNORE;
1098 } 1136 }
1099 } 1137 }
1100 1138
1101 if (params.nav_entry_id == 0) { 1139 if (params.nav_entry_id == 0) {
1102 // This is a renderer-initiated navigation (nav_entry_id == 0), but didn't 1140 // This is a renderer-initiated navigation (nav_entry_id == 0), but didn't
1103 // create a new page. 1141 // create a new page.
1104 1142
1105 // Just like above in the did_create_new_entry case, it's possible to 1143 // Just like above in the did_create_new_entry case, it's possible to
1106 // scribble onto an uncommitted page. Again, there isn't any navigation 1144 // scribble onto an uncommitted page. Again, there isn't any navigation
1107 // stuff that we can do, so ignore it here as well. 1145 // stuff that we can do, so ignore it here as well.
1108 NavigationEntry* last_committed = GetLastCommittedEntry(); 1146 NavigationEntry* last_committed = GetLastCommittedEntry();
1109 if (!last_committed) 1147 if (!last_committed) {
1148 base::debug::SetCrashKeyValue("369661-newignore",
1149 rfh->CommitCountString() +
1150 " renderer-initiated no last committed");
1110 return NAVIGATION_TYPE_NAV_IGNORE; 1151 return NAVIGATION_TYPE_NAV_IGNORE;
1152 }
1111 1153
1112 if (IsURLInPageNavigation(params.url, params.was_within_same_page, rfh)) { 1154 if (IsURLInPageNavigation(params.url, params.was_within_same_page, rfh)) {
1113 // This is history.replaceState(), which is renderer-initiated yet within 1155 // This is history.replaceState(), which is renderer-initiated yet within
1114 // the same page. 1156 // the same page.
1115 return NAVIGATION_TYPE_IN_PAGE; 1157 return NAVIGATION_TYPE_IN_PAGE;
1116 } else { 1158 } else {
1117 // This is history.reload() or a client-side redirect. 1159 // This is history.reload() or a client-side redirect.
1118 return NAVIGATION_TYPE_EXISTING_PAGE; 1160 return NAVIGATION_TYPE_EXISTING_PAGE;
1119 } 1161 }
1120 } 1162 }
(...skipping 27 matching lines...) Expand all
1148 } 1190 }
1149 1191
1150 // Now we know that the notification is for an existing page. Find that entry. 1192 // Now we know that the notification is for an existing page. Find that entry.
1151 int existing_entry_index = GetEntryIndexWithUniqueID(params.nav_entry_id); 1193 int existing_entry_index = GetEntryIndexWithUniqueID(params.nav_entry_id);
1152 if (existing_entry_index == -1) { 1194 if (existing_entry_index == -1) {
1153 // The page was not found. It could have been pruned because of the limit on 1195 // The page was not found. It could have been pruned because of the limit on
1154 // back/forward entries (not likely since we'll usually tell it to navigate 1196 // back/forward entries (not likely since we'll usually tell it to navigate
1155 // to such entries). It could also mean that the renderer is smoking crack. 1197 // to such entries). It could also mean that the renderer is smoking crack.
1156 // TODO(avi): Crash the renderer like we do in the old ClassifyNavigation? 1198 // TODO(avi): Crash the renderer like we do in the old ClassifyNavigation?
1157 NOTREACHED() << "Could not find nav entry with id " << params.nav_entry_id; 1199 NOTREACHED() << "Could not find nav entry with id " << params.nav_entry_id;
1200 base::debug::SetCrashKeyValue("369661-newignore",
1201 rfh->CommitCountString() +
1202 " renderer smoking crack");
1158 return NAVIGATION_TYPE_NAV_IGNORE; 1203 return NAVIGATION_TYPE_NAV_IGNORE;
1159 } 1204 }
1160 1205
1161 // Any top-level navigations with the same base (minus the reference fragment) 1206 // Any top-level navigations with the same base (minus the reference fragment)
1162 // are in-page navigations. (We weeded out subframe navigations above.) Most 1207 // are in-page navigations. (We weeded out subframe navigations above.) Most
1163 // of the time this doesn't matter since Blink doesn't tell us about subframe 1208 // of the time this doesn't matter since Blink doesn't tell us about subframe
1164 // navigations that don't actually navigate, but it can happen when there is 1209 // navigations that don't actually navigate, but it can happen when there is
1165 // an encoding override (it always sends a navigation request). 1210 // an encoding override (it always sends a navigation request).
1166 if (IsURLInPageNavigation(params.url, params.was_within_same_page, rfh)) 1211 if (IsURLInPageNavigation(params.url, params.was_within_same_page, rfh))
1167 return NAVIGATION_TYPE_IN_PAGE; 1212 return NAVIGATION_TYPE_IN_PAGE;
(...skipping 868 matching lines...) Expand 10 before | Expand all | Expand 10 after
2036 } 2081 }
2037 } 2082 }
2038 } 2083 }
2039 2084
2040 void NavigationControllerImpl::SetGetTimestampCallbackForTest( 2085 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
2041 const base::Callback<base::Time()>& get_timestamp_callback) { 2086 const base::Callback<base::Time()>& get_timestamp_callback) {
2042 get_timestamp_callback_ = get_timestamp_callback; 2087 get_timestamp_callback_ = get_timestamp_callback;
2043 } 2088 }
2044 2089
2045 } // namespace content 2090 } // namespace content
OLDNEW
« no previous file with comments | « chrome/common/crash_keys.cc ('k') | content/browser/frame_host/render_frame_host_impl.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698