Commit 26771781 authored by Danyao Wang's avatar Danyao Wang Committed by Commit Bot

[Nav Experiment] Fix crash in [CRWWebController webViewURLDidChange].

This method looks up the current NavigationItem associated with
WKBackForwardList.currentItem and updates its URL to handle two edge
cases for location.replace. The lookup crashes if
WKBackForwardList.currentItem is nil. The correct NavigationItem to
update in this case is |self.currentNavItem|.

An egtest that triggers this scenario is added; though it's not clear
if this is the only case.

Bug: 866142
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I5c76ad50296127234f498741a62b796fb4397461
Reviewed-on: https://chromium-review.googlesource.com/1228442Reviewed-by: default avatarEugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592175}
parent 6d0e5f09
......@@ -218,6 +218,24 @@ id<GREYMatcher> PopupBlocker() {
[ChromeEarlGrey waitForMainTabCount:2];
}
// Tests that the correct URL is displayed for a child window opened with the
// script window.open('', '').location.replace('about:blank#hash').
// This is a regression test for crbug.com/866142.
- (void)testLocationReplaceInWindowOpenWithEmptyTarget {
GREYAssert(TapWebViewElementWithId(
"webScenarioLocationReplaceInWindowOpenWithEmptyTarget"),
@"Failed to tap "
@"\"webScenarioLocationReplaceInWindowOpenWithEmptyTarget\"");
[ChromeEarlGrey waitForMainTabCount:2];
// WebKit doesn't parse 'about:blank#hash' as about:blank with URL fragment.
// Instead, it percent encodes '#hash' and considers 'blank%23hash' as the
// resource identifier. Nevertheless, the '#' is significant in triggering the
// edge case in the bug. TODO(crbug.com/885249): Change back to '#'.
const GURL URL("about:blank%23hash");
[[EarlGrey selectElementWithMatcher:OmniboxText("about:blank%23hash")]
assertWithMatcher:grey_notNil()];
}
// Tests a link with JavaScript in the href.
+ (void)testWindowOpenWithJavaScriptInHref {
GREYAssert(
......
......@@ -39,6 +39,17 @@ found in the LICENSE file. -->
<td>about:blank opened in new tab</td>
</tr>
<tr id="_webScenarioLocationReplaceInWindowOpenWithEmptyTarget">
<td>
<input type="button"
onclick="window.open('','').location.replace('about:blank#hash');"
name="webScenarioLocationReplaceInWindowOpenWithEmptyTarget"
id="webScenarioLocationReplaceInWindowOpenWithEmptyTarget"
value="webScenarioLocationReplaceInWindowOpenWithEmptyTarget">
</td>
<td>Hash-only location.replace in new empty window</td>
</tr>
<tr id="_webScenarioWindowOpenRegularLink">
<td>
<a href="about:blank"
......
......@@ -10,6 +10,7 @@
#include "base/logging.h"
#include "base/mac/bundle_locations.h"
#include "base/memory/ptr_util.h"
#include "base/strings/string_util.h"
#include "base/strings/sys_string_conversions.h"
#import "ios/web/navigation/crw_navigation_item_holder.h"
#import "ios/web/navigation/navigation_item_impl.h"
......@@ -215,7 +216,10 @@ void WKBasedNavigationManagerImpl::CommitPendingItem() {
// WKWebView.
if (proxy.backForwardList && !proxy.backForwardList.currentItem) {
// WKWebView's URL should be about:blank for empty window open item.
DCHECK_EQ(url::kAboutBlankURL, net::GURLWithNSURL(proxy.URL).spec());
// TODO(crbug.com/885249): Use GURL::IsAboutBlank() instead.
DCHECK(base::StartsWith(net::GURLWithNSURL(proxy.URL).spec(),
url::kAboutBlankURL,
base::CompareCase::SENSITIVE));
// There should be no back-forward history for empty window open item.
DCHECK_EQ(0UL, proxy.backForwardList.backList.count);
DCHECK_EQ(0UL, proxy.backForwardList.forwardList.count);
......
......@@ -5305,9 +5305,20 @@ registerLoadRequestForURL:(const GURL&)requestURL
// update NavigationItem URL.
if (web::GetWebClient()->IsSlimNavigationManagerEnabled()) {
const GURL webViewURL = net::GURLWithNSURL(_webView.URL);
web::NavigationItem* currentItem = [[CRWNavigationItemHolder
web::NavigationItem* currentItem = nullptr;
if (_webView.backForwardList.currentItem) {
currentItem = [[CRWNavigationItemHolder
holderForBackForwardListItem:_webView.backForwardList.currentItem]
navigationItem];
} else {
// WKBackForwardList.currentItem may be nil in a corner case when
// location.replace is called with about:blank#hash in an empty window
// open tab. See crbug.com/866142.
DCHECK(self.webStateImpl->HasOpener());
DCHECK(!self.navigationManagerImpl->GetTransientItem());
DCHECK(!self.navigationManagerImpl->GetPendingItem());
currentItem = self.navigationManagerImpl->GetLastCommittedItem();
}
if (currentItem && webViewURL != currentItem->GetURL())
currentItem->SetURL(webViewURL);
}
......@@ -5385,7 +5396,6 @@ registerLoadRequestForURL:(const GURL&)requestURL
- (void)URLDidChangeWithoutDocumentChange:(const GURL&)newURL {
DCHECK(newURL == net::GURLWithNSURL([_webView URL]));
DCHECK(!_documentURL.host().empty() || _documentURL.SchemeIsFile());
if (base::FeatureList::IsEnabled(
web::features::kCrashOnUnexpectedURLChange)) {
......
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