Fix NavigationItem use-after-free crash in |-goToItemAtIndex:|
If a history navigation item occurs and the current NavigationItem is
transient item, it will get discarded in CRWSessionController's
|-discardTransientItem|. This CL updates history navigation logic to
store copies of the current NavigationItem's information before calling
any CRWSessionController code that might deallocate it.
BUG=700319
Review-Url: https://codereview.chromium.org/2745653007
Cr-Commit-Position: refs/heads/master@{#456190}
(cherry picked from commit c0f6017abb9aeb5ae1c8e137b6a3671305298b40)
Review-Url: https://codereview.chromium.org/2757043002 .
Cr-Commit-Position: refs/branch-heads/3029@{#279}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}
diff --git a/ios/web/web_state/ui/crw_web_controller.mm b/ios/web/web_state/ui/crw_web_controller.mm
index e162b1a..3d319f5 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -465,14 +465,16 @@
// Facade for Mojo API.
@property(nonatomic, readonly) web::MojoFacade* mojoFacade;
-// Updates Desktop User Agent and calls webWillFinishHistoryNavigationFromEntry:
-// on CRWWebDelegate. TODO(crbug.com/684098): Remove this method and inline its
-// content.
-- (void)webWillFinishHistoryNavigationFromEntry:(CRWSessionEntry*)fromEntry;
-// Recreates web view if |entry| and |fromEntry| have different value for
-// IsOverridingUserAgent() flag.
-- (void)updateDesktopUserAgentForEntry:(CRWSessionEntry*)entry
- fromEntry:(CRWSessionEntry*)fromEntry;
+// TODO(crbug.com/684098): Remove these methods and inline their content.
+// Called before finishing a history navigation from a page with the given
+// UserAgentType.
+- (void)webWillFinishHistoryNavigationWithPreviousUserAgentType:
+ (web::UserAgentType)userAgentType;
+// Requires page reconstruction if |item| has a non-NONE UserAgentType and it
+// differs from that of |fromItem|.
+- (void)updateDesktopUserAgentForItem:(web::NavigationItem*)item
+ previousUserAgentType:(web::UserAgentType)userAgentType;
+
// Removes the container view from the hierarchy and resets the ivar.
- (void)resetContainerView;
// Called when the web page has changed document and/or URL, and so the page
@@ -608,9 +610,12 @@
// TODO(crbug.com/661316): Move this method to NavigationManager.
- (void)goDelta:(int)delta;
// Loads a new URL if the current entry is not from a pushState() navigation.
-// |fromEntry| is the CRWSessionEntry that was the current entry prior to the
-// navigation.
-- (void)finishHistoryNavigationFromEntry:(CRWSessionEntry*)fromEntry;
+// |fromURL| is the URL of the previous NavigationItem, |fromUserAgentType| is
+// that item's UserAgentType, and |sameDocument| is YES if the navigation is
+// between two pages with the same document.
+- (void)finishHistoryNavigationFromURL:(const GURL&)fromURL
+ userAgentType:(web::UserAgentType)fromUserAgentType
+ sameDocument:(BOOL)sameDocument;
// Informs the native controller if web usage is allowed or not.
- (void)setNativeControllerWebUsageEnabled:(BOOL)webUsageEnabled;
// Called when web controller receives a new message from the web page.
@@ -722,9 +727,8 @@
// Compares the two URLs being navigated between during a history navigation to
// determine if a # needs to be appended to the URL of |toItem| to trigger a
// hashchange event. If so, also saves the modified URL into |toItem|.
-- (GURL)URLForHistoryNavigationFromItem:(web::NavigationItem*)fromItem
- toItem:(web::NavigationItem*)toItem;
-
+- (GURL)URLForHistoryNavigationToItem:(web::NavigationItem*)toItem
+ previousURL:(const GURL&)previousURL;
// Finds all the scrollviews in the view hierarchy and makes sure they do not
// interfere with scroll to top when tapping the statusbar.
- (void)optOutScrollsToTopForSubviews;
@@ -1411,8 +1415,8 @@
[list.backList indexOfObject:item] != NSNotFound;
}
-- (GURL)URLForHistoryNavigationFromItem:(web::NavigationItem*)fromItem
- toItem:(web::NavigationItem*)toItem {
+- (GURL)URLForHistoryNavigationToItem:(web::NavigationItem*)toItem
+ previousURL:(const GURL&)previousURL {
// If navigating with native API, i.e. using a back forward list item,
// hashchange events will be triggered automatically, so no URL tampering is
// required.
@@ -1422,26 +1426,25 @@
return toItem->GetURL();
}
- const GURL& startURL = fromItem->GetURL();
- const GURL& endURL = toItem->GetURL();
+ const GURL& URL = toItem->GetURL();
// Check the state of the fragments on both URLs (aka, is there a '#' in the
// url or not).
- if (!startURL.has_ref() || endURL.has_ref()) {
- return endURL;
+ if (!previousURL.has_ref() || URL.has_ref()) {
+ return URL;
}
// startURL contains a fragment and endURL doesn't. Remove the fragment from
// startURL and compare the resulting string to endURL. If they are equal, add
// # to endURL to cause a hashchange event.
- GURL hashless = web::GURLByRemovingRefFromGURL(startURL);
+ GURL hashless = web::GURLByRemovingRefFromGURL(previousURL);
- if (hashless != endURL)
- return endURL;
+ if (hashless != URL)
+ return URL;
url::StringPieceReplacements<std::string> emptyRef;
emptyRef.SetRefStr("");
- GURL newEndURL = endURL.ReplaceComponents(emptyRef);
+ GURL newEndURL = URL.ReplaceComponents(emptyRef);
toItem->SetURL(newEndURL);
return newEndURL;
}
@@ -2127,17 +2130,26 @@
if (!_webStateImpl->IsShowingWebInterstitial())
[self recordStateInHistory];
- CRWSessionEntry* fromEntry = sessionController.currentEntry;
- CRWSessionEntry* toEntry = entries[index];
+
+ web::NavigationItem* previousItem = sessionController.currentItem;
+ GURL previousURL = previousItem ? previousItem->GetURL() : GURL::EmptyGURL();
+ web::UserAgentType previousUserAgentType =
+ previousItem ? previousItem->GetUserAgentType()
+ : web::UserAgentType::NONE;
+ web::NavigationItem* toItem = items[index].get();
+ BOOL sameDocumentNavigation =
+ [sessionController isSameDocumentNavigationBetweenItem:previousItem
+ andItem:toItem];
NSUserDefaults* userDefaults = [NSUserDefaults standardUserDefaults];
if (![userDefaults boolForKey:@"PendingIndexNavigationDisabled"]) {
[self clearTransientContentView];
[self updateDesktopUserAgentForEntry:toEntry fromEntry:fromEntry];
- BOOL sameDocumentNavigation = [sessionController
- isSameDocumentNavigationBetweenItem:fromEntry.navigationItem
- andItem:toEntry.navigationItem];
+ // Update the user agent before attempting the navigation.
+ [self updateDesktopUserAgentForItem:toItem
+ previousUserAgentType:previousUserAgentType];
+
if (sameDocumentNavigation) {
[sessionController goToItemAtIndex:index];
[self updateHTML5HistoryState];
@@ -2154,8 +2166,11 @@
}
} else {
[sessionController goToItemAtIndex:index];
- if (fromEntry)
- [self finishHistoryNavigationFromEntry:fromEntry];
+ if (previousURL.is_valid()) {
+ [self finishHistoryNavigationFromURL:previousURL
+ userAgentType:previousUserAgentType
+ sameDocument:sameDocumentNavigation];
+ }
}
}
@@ -2249,20 +2264,19 @@
}
}
-- (void)finishHistoryNavigationFromEntry:(CRWSessionEntry*)fromEntry {
- [self webWillFinishHistoryNavigationFromEntry:fromEntry];
+- (void)finishHistoryNavigationFromURL:(const GURL&)fromURL
+ userAgentType:(web::UserAgentType)fromUserAgentType
+ sameDocument:(BOOL)sameDocument {
+ [self webWillFinishHistoryNavigationWithPreviousUserAgentType:
+ fromUserAgentType];
// Only load the new URL if it has a different document than |fromEntry| to
// prevent extra page loads from NavigationItems created by hash changes or
// calls to window.history.pushState().
- BOOL shouldLoadURL = ![self.sessionController
- isSameDocumentNavigationBetweenItem:fromEntry.navigationItem
- andItem:self.currentNavItem];
- web::NavigationItemImpl* currentItem =
- self.currentSessionEntry.navigationItemImpl;
- GURL endURL = [self URLForHistoryNavigationFromItem:fromEntry.navigationItem
- toItem:currentItem];
- if (shouldLoadURL) {
+ web::NavigationItem* currentItem = self.currentNavItem;
+ GURL endURL =
+ [self URLForHistoryNavigationToItem:currentItem previousURL:fromURL];
+ if (!sameDocument) {
ui::PageTransition transition = ui::PageTransitionFromInt(
ui::PAGE_TRANSITION_RELOAD | ui::PAGE_TRANSITION_FORWARD_BACK);
@@ -2278,7 +2292,7 @@
// updated after the navigation is committed, as attempting to replace the URL
// here will result in a JavaScript SecurityError due to the URLs having
// different origins.
- if (!shouldLoadURL)
+ if (sameDocument)
[self updateHTML5HistoryState];
}
@@ -2371,21 +2385,21 @@
return _passKitDownloader.get();
}
-- (void)webWillFinishHistoryNavigationFromEntry:(CRWSessionEntry*)fromEntry {
- DCHECK(fromEntry);
- [self updateDesktopUserAgentForEntry:self.currentSessionEntry
- fromEntry:fromEntry];
- [_delegate webWillFinishHistoryNavigationFromEntry:fromEntry];
+- (void)webWillFinishHistoryNavigationWithPreviousUserAgentType:
+ (web::UserAgentType)userAgentType {
+ [self updateDesktopUserAgentForItem:self.currentNavItem
+ previousUserAgentType:userAgentType];
+ [_delegate webWillFinishHistoryNavigation];
}
-- (void)updateDesktopUserAgentForEntry:(CRWSessionEntry*)entry
- fromEntry:(CRWSessionEntry*)fromEntry {
- web::NavigationItemImpl* item = entry.navigationItemImpl;
- web::NavigationItemImpl* fromItem = fromEntry.navigationItemImpl;
+- (void)updateDesktopUserAgentForItem:(web::NavigationItem*)item
+ previousUserAgentType:(web::UserAgentType)userAgentType {
+ if (!item)
+ return;
web::UserAgentType itemUserAgentType = item->GetUserAgentType();
if (!item || !fromItem || itemUserAgentType == web::UserAgentType::NONE)
return;
- if (itemUserAgentType != fromItem->GetUserAgentType())
+ if (itemUserAgentType != userAgentType)
[self requirePageReconstruction];
}
diff --git a/ios/web/web_state/ui/crw_web_controller_unittest.mm b/ios/web/web_state/ui/crw_web_controller_unittest.mm
index eba3aeb..70497ed 100644
--- a/ios/web/web_state/ui/crw_web_controller_unittest.mm
+++ b/ios/web/web_state/ui/crw_web_controller_unittest.mm
@@ -50,8 +50,8 @@
@interface CRWWebController (PrivateAPI)
@property(nonatomic, readwrite) web::PageDisplayState pageDisplayState;
-- (GURL)URLForHistoryNavigationFromItem:(web::NavigationItem*)fromItem
- toItem:(web::NavigationItem*)toItem;
+- (GURL)URLForHistoryNavigationToItem:(web::NavigationItem*)toItem
+ previousURL:(const GURL&)previousURL;
@end
// Used to mock CRWWebDelegate methods with C++ params.
@@ -276,27 +276,28 @@
[urlsWithFragments addObject:[url stringByAppendingString:fragment]];
}
}
- web::NavigationItemImpl fromItem;
+
+ GURL previous_url;
web::NavigationItemImpl toItem;
// No start fragment: the end url is never changed.
for (NSString* start in urlsNoFragments) {
for (NSString* end in urlsWithFragments) {
- fromItem.SetURL(MAKE_URL(start));
+ previous_url = MAKE_URL(start);
toItem.SetURL(MAKE_URL(end));
EXPECT_EQ(MAKE_URL(end),
- [web_controller() URLForHistoryNavigationFromItem:&fromItem
- toItem:&toItem]);
+ [web_controller() URLForHistoryNavigationToItem:&toItem
+ previousURL:previous_url]);
}
}
// Both contain fragments: the end url is never changed.
for (NSString* start in urlsWithFragments) {
for (NSString* end in urlsWithFragments) {
- fromItem.SetURL(MAKE_URL(start));
+ previous_url = MAKE_URL(start);
toItem.SetURL(MAKE_URL(end));
EXPECT_EQ(MAKE_URL(end),
- [web_controller() URLForHistoryNavigationFromItem:&fromItem
- toItem:&toItem]);
+ [web_controller() URLForHistoryNavigationToItem:&toItem
+ previousURL:previous_url]);
}
}
for (unsigned start_index = 0; start_index < [urlsWithFragments count];
@@ -305,21 +306,22 @@
for (unsigned end_index = 0; end_index < [urlsNoFragments count];
++end_index) {
NSString* end = urlsNoFragments[end_index];
+ previous_url = MAKE_URL(start);
if (start_index / 2 != end_index) {
// The URLs have nothing in common, they are left untouched.
- fromItem.SetURL(MAKE_URL(start));
toItem.SetURL(MAKE_URL(end));
- EXPECT_EQ(MAKE_URL(end),
- [web_controller() URLForHistoryNavigationFromItem:&fromItem
- toItem:&toItem]);
+ EXPECT_EQ(
+ MAKE_URL(end),
+ [web_controller() URLForHistoryNavigationToItem:&toItem
+ previousURL:previous_url]);
} else {
// Start contains a fragment and matches end: An empty fragment is
// added.
- fromItem.SetURL(MAKE_URL(start));
toItem.SetURL(MAKE_URL(end));
- EXPECT_EQ(MAKE_URL([end stringByAppendingString:@"#"]),
- [web_controller() URLForHistoryNavigationFromItem:&fromItem
- toItem:&toItem]);
+ EXPECT_EQ(
+ MAKE_URL([end stringByAppendingString:@"#"]),
+ [web_controller() URLForHistoryNavigationToItem:&toItem
+ previousURL:previous_url]);
}
}
}
