Commit 267f0770 authored by Danyao Wang's avatar Danyao Wang Committed by Commit Bot

[Nav Experiment] Fix crash when WKWebView is removed from nav manager.

CRWWebController removes WKWebView when the web process is evicted
(crbug/815248). This leaves WKBasedNavigationManager in an inconsistent
state as previous_item_index_ may not be -1, but GetItemCount() is 0.
This causes a crash in OnNavigationItemCommitted() when user starts a
new navigation by typing into the omnibox.

Fix the crash by guarding the pointer dereference with a nullptr check.

Bug: 797756
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ie553efbe7bb558a8af955ca6f64e2b8aea58aacd
Reviewed-on: https://chromium-review.googlesource.com/996436Reviewed-by: default avatarEugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548250}
parent 6c58c0c6
...@@ -65,11 +65,9 @@ void LegacyNavigationManagerImpl::OnNavigationItemCommitted() { ...@@ -65,11 +65,9 @@ void LegacyNavigationManagerImpl::OnNavigationItemCommitted() {
details.previous_item_index = [session_controller_ previousItemIndex]; details.previous_item_index = [session_controller_ previousItemIndex];
if (details.previous_item_index >= 0) { if (details.previous_item_index >= 0) {
DCHECK([session_controller_ previousItem]); DCHECK([session_controller_ previousItem]);
details.previous_url = [session_controller_ previousItem]->GetURL();
details.is_in_page = IsFragmentChangeNavigationBetweenUrls( details.is_in_page = IsFragmentChangeNavigationBetweenUrls(
details.previous_url, details.item->GetURL()); [session_controller_ previousItem]->GetURL(), details.item->GetURL());
} else { } else {
details.previous_url = GURL();
details.is_in_page = NO; details.is_in_page = NO;
} }
......
...@@ -76,16 +76,11 @@ void WKBasedNavigationManagerImpl::OnNavigationItemCommitted() { ...@@ -76,16 +76,11 @@ void WKBasedNavigationManagerImpl::OnNavigationItemCommitted() {
details.item = GetLastCommittedItem(); details.item = GetLastCommittedItem();
DCHECK(details.item); DCHECK(details.item);
details.previous_item_index = GetPreviousItemIndex(); details.previous_item_index = GetPreviousItemIndex();
if (details.previous_item_index >= 0) { NavigationItem* previous_item = GetItemAtIndex(details.previous_item_index);
NavigationItem* previous_item = GetItemAtIndex(details.previous_item_index); details.is_in_page =
DCHECK(previous_item); previous_item ? IsFragmentChangeNavigationBetweenUrls(
details.previous_url = previous_item->GetURL(); previous_item->GetURL(), details.item->GetURL())
details.is_in_page = IsFragmentChangeNavigationBetweenUrls( : NO;
details.previous_url, details.item->GetURL());
} else {
details.previous_url = GURL();
details.is_in_page = NO;
}
delegate_->OnNavigationItemCommitted(details); delegate_->OnNavigationItemCommitted(details);
} }
......
...@@ -22,11 +22,8 @@ struct LoadCommittedDetails { ...@@ -22,11 +22,8 @@ struct LoadCommittedDetails {
// if there are no previous items. // if there are no previous items.
int previous_item_index; int previous_item_index;
// The previous URL that the user was on. This may be empty if none.
GURL previous_url;
// True if the navigation was in-page. This means that the active item's // True if the navigation was in-page. This means that the active item's
// URL and the |previous_url| are the same except for reference fragments. // URL and the previous URL are the same except for reference fragments.
bool is_in_page; bool is_in_page;
}; };
......
...@@ -462,7 +462,6 @@ TEST_F(WebStateImplTest, ObserverTest) { ...@@ -462,7 +462,6 @@ TEST_F(WebStateImplTest, ObserverTest) {
observer->commit_navigation_info()->load_details; observer->commit_navigation_info()->load_details;
EXPECT_EQ(details.item, actual_details.item); EXPECT_EQ(details.item, actual_details.item);
EXPECT_EQ(details.previous_item_index, actual_details.previous_item_index); EXPECT_EQ(details.previous_item_index, actual_details.previous_item_index);
EXPECT_EQ(details.previous_url, actual_details.previous_url);
EXPECT_EQ(details.is_in_page, actual_details.is_in_page); EXPECT_EQ(details.is_in_page, actual_details.is_in_page);
// Test that OnPageLoaded() is called with success when there is no error. // Test that OnPageLoaded() is called with success when there is no error.
......
...@@ -136,7 +136,6 @@ TEST_F(WebStateObserverBridgeTest, NavigationItemCommitted) { ...@@ -136,7 +136,6 @@ TEST_F(WebStateObserverBridgeTest, NavigationItemCommitted) {
LoadCommittedDetails load_details; LoadCommittedDetails load_details;
load_details.item = reinterpret_cast<web::NavigationItem*>(1); load_details.item = reinterpret_cast<web::NavigationItem*>(1);
load_details.previous_item_index = 15; load_details.previous_item_index = 15;
load_details.previous_url = GURL("https://chromium.test/");
load_details.is_in_page = true; load_details.is_in_page = true;
observer_bridge_.NavigationItemCommitted(&test_web_state_, load_details); observer_bridge_.NavigationItemCommitted(&test_web_state_, load_details);
...@@ -147,8 +146,6 @@ TEST_F(WebStateObserverBridgeTest, NavigationItemCommitted) { ...@@ -147,8 +146,6 @@ TEST_F(WebStateObserverBridgeTest, NavigationItemCommitted) {
[observer_ commitNavigationInfo]->load_details.item); [observer_ commitNavigationInfo]->load_details.item);
EXPECT_EQ(load_details.previous_item_index, EXPECT_EQ(load_details.previous_item_index,
[observer_ commitNavigationInfo]->load_details.previous_item_index); [observer_ commitNavigationInfo]->load_details.previous_item_index);
EXPECT_EQ(load_details.previous_url,
[observer_ commitNavigationInfo]->load_details.previous_url);
EXPECT_EQ(load_details.is_in_page, EXPECT_EQ(load_details.is_in_page,
[observer_ commitNavigationInfo]->load_details.is_in_page); [observer_ commitNavigationInfo]->load_details.is_in_page);
} }
......
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