Commit 4d561f6c authored by Arthur Sonzogni's avatar Arthur Sonzogni Committed by Commit Bot

Revert "Make renderer-initiated navigation to about:blank to use the default path."

This reverts commit 2cd39eb0.

Reason for revert: The branch cut was supposed to be yesterday, but I
haven't received any confirmation is happened. To be sure revert this
commit for the moment.

Original change's description:
> Make renderer-initiated navigation to about:blank to use the default path.
> 
> We used to believe renderer initiated navigation from a document to
> about:blank needed to happen sometimes "synchronously". This isn't the
> case, a PostTask is currently used. The only real requirement is when
> committing the initial empty document.
> 
> This CL makes those navigation to use the default path instead.
> 
> This requires a few tests to be updated:
> 
> 1) The initiator and a few other properties needs to be properly set. There
>    are DCHECK in BeginNavigationInternal to enforce renderer initiated
>    navigation always have a defined initiator.
> 
> 2) runtime-console-log-handle-navigate.js: In console-log-navigate, console.log
>    can use the 'getter' of the object several times (up to 2) to print the
>    object. It triggers several about:blank navigations. Since they are now going
>    to the browser process, they are committed later compared to the script
>    execution. The test became less reliable. The test was using
>    dp.Runtime.onceExecutionContextCreated to wait for new documents to commit,
>    but waiting was started potentially after the event occured. The new test is
>    now creating the Promise before triggering the action that will later resolve
>    it. The action is either creating the iframe or making use of the logXXX()
>    function. This is now fully reliable.
> 
> Bug: 936696
> Change-Id: Ibf1a87f83d2e8bfd621d067f0a29a14a069384a8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1543258
> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Camille Lamy <clamy@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#652493}

TBR=dgozman@chromium.org,dcheng@chromium.org,nasko@chromium.org,clamy@chromium.org,alexmos@chromium.org,arthursonzogni@chromium.org

Change-Id: I6a5fe676275d59027907a1c9540dfe941fab3e18
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 936696
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1574320Reviewed-by: default avatarArthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652499}
parent 8d2eba94
......@@ -33,9 +33,8 @@ Referrer Referrer::SanitizeForRequest(const GURL& request,
sanitized_referrer.policy = network::mojom::ReferrerPolicy::kNever;
}
bool is_web_scheme = request.SchemeIsHTTPOrHTTPS() || request.IsAboutBlank();
if (!is_web_scheme || !sanitized_referrer.url.SchemeIsValidForReferrer()) {
if (!request.SchemeIsHTTPOrHTTPS() ||
!sanitized_referrer.url.SchemeIsValidForReferrer()) {
sanitized_referrer.url = GURL();
return sanitized_referrer;
}
......
......@@ -6508,39 +6508,29 @@ void RenderFrameImpl::BeginNavigation(
for (auto& observer : observers_)
observer.DidStartNavigation(url, info->navigation_type);
// First navigation in a frame to an empty document must be handled
// synchronously.
bool is_first_real_empty_document_navigation =
WebDocumentLoader::WillLoadUrlAsEmpty(url) &&
!frame_->HasCommittedFirstRealLoad();
if (is_first_real_empty_document_navigation) {
CommitSyncNavigation(std::move(info));
// Navigations which require network request should be sent to the browser.
if (!use_archive && IsURLHandledByNetworkStack(url)) {
BeginNavigationInternal(std::move(info));
return;
}
// Navigation to about:srcdoc or to an MHTML archive don't need to consult
// the browser. The document content is already available in the renderer
// process.
// TODO(arthursonzogni): Remove this. Everything should use the default code
// path and be driven by the browser process.
if (use_archive || url == content::kAboutSrcDocURL) {
if (!frame_->CreatePlaceholderDocumentLoader(*info, BuildDocumentState()))
return;
// Only the first navigation in a frame to an empty document must be
// handled synchronously, the others are required to happen
// asynchronously. So a PostTask is used.
sync_navigation_callback_.Reset(
base::BindOnce(&RenderFrameImpl::CommitSyncNavigation,
weak_factory_.GetWeakPtr(), base::Passed(&info)));
frame_->GetTaskRunner(blink::TaskType::kInternalLoading)
->PostTask(FROM_HERE, sync_navigation_callback_.callback());
// First navigaiton in a frame to an empty document must be handled
// synchronously.
if (WebDocumentLoader::WillLoadUrlAsEmpty(url) &&
!frame_->HasCommittedFirstRealLoad()) {
CommitSyncNavigation(std::move(info));
return;
}
// Everything else is handled asynchronously by the browser process through
// BeginNavigation.
BeginNavigationInternal(std::move(info));
// Everything else (does not require networking, not an empty document)
// will be committed asynchronously in the renderer.
if (!frame_->CreatePlaceholderDocumentLoader(*info, BuildDocumentState()))
return;
sync_navigation_callback_.Reset(
base::BindOnce(&RenderFrameImpl::CommitSyncNavigation,
weak_factory_.GetWeakPtr(), base::Passed(&info)));
frame_->GetTaskRunner(blink::TaskType::kInternalLoading)
->PostTask(FROM_HERE, sync_navigation_callback_.callback());
return;
}
......
......@@ -1762,11 +1762,6 @@ class CONTENT_EXPORT RenderFrameImpl
RenderFrameMediaPlaybackOptions renderer_media_playback_options_;
// Used by renderer initiated navigations not driven by the browser process:
// - navigation to about:srcdoc.
// - navigation using an MHTML archive.
// TODO(arthursonzogni): Remove this. Everything should use the default code
// path and be driven by the browser process.
base::CancelableOnceCallback<void()> sync_navigation_callback_;
class MHTMLBodyLoaderClient;
......
......@@ -95,7 +95,6 @@
#include "ui/gfx/codec/jpeg_codec.h"
#include "ui/gfx/range/range.h"
#include "ui/native_theme/native_theme_features.h"
#include "url/url_constants.h"
#if defined(OS_ANDROID)
#include "third_party/blink/public/platform/web_coalesced_input_event.h"
......@@ -886,19 +885,15 @@ class AlwaysForkingRenderViewTest : public RenderViewImplTest {
TEST_F(AlwaysForkingRenderViewTest, BeginNavigationDoesNotForkEmptyUrl) {
GURL example_url("http://example.com");
GURL empty_url("");
GURL blank_url("about:blank");
LoadHTMLWithUrlOverride("<body></body", example_url.spec().c_str());
EXPECT_EQ(example_url,
GURL(frame()->GetWebFrame()->GetDocumentLoader()->GetUrl()));
// Empty url should never fork.
blink::WebURLRequest request(empty_url);
request.SetFetchRequestMode(network::mojom::FetchRequestMode::kNavigate);
request.SetFetchRedirectMode(network::mojom::FetchRedirectMode::kManual);
request.SetRequestContext(blink::mojom::RequestContextType::INTERNAL);
request.SetRequestorOrigin(blink::WebSecurityOrigin::Create(example_url));
auto navigation_info = std::make_unique<blink::WebNavigationInfo>();
navigation_info->url_request = request;
navigation_info->url_request = blink::WebURLRequest(empty_url);
navigation_info->frame_type =
network::mojom::RequestContextFrameType::kTopLevel;
navigation_info->navigation_policy = blink::kWebNavigationPolicyCurrentTab;
......@@ -909,20 +904,15 @@ TEST_F(AlwaysForkingRenderViewTest, BeginNavigationDoesNotForkEmptyUrl) {
TEST_F(AlwaysForkingRenderViewTest, BeginNavigationDoesNotForkAboutBlank) {
GURL example_url("http://example.com");
GURL blank_url(url::kAboutBlankURL);
GURL blank_url("about:blank");
LoadHTMLWithUrlOverride("<body></body", example_url.spec().c_str());
EXPECT_EQ(example_url,
GURL(frame()->GetWebFrame()->GetDocumentLoader()->GetUrl()));
// about:blank should never fork.
blink::WebURLRequest request(blank_url);
request.SetFetchRequestMode(network::mojom::FetchRequestMode::kNavigate);
request.SetFetchRedirectMode(network::mojom::FetchRedirectMode::kManual);
request.SetRequestContext(blink::mojom::RequestContextType::INTERNAL);
request.SetRequestorOrigin(blink::WebSecurityOrigin::Create(example_url));
// About blank should never fork.
auto navigation_info = std::make_unique<blink::WebNavigationInfo>();
navigation_info->url_request = request;
navigation_info->url_request = blink::WebURLRequest(blank_url);
navigation_info->frame_type =
network::mojom::RequestContextFrameType::kTopLevel;
navigation_info->navigation_policy = blink::kWebNavigationPolicyCurrentTab;
......
......@@ -827,13 +827,7 @@ void BlinkTestRunner::OnReset() {
// Navigating to about:blank will make sure that no new loads are initiated
// by the renderer.
waiting_for_reset_ = true;
auto request = blink::WebURLRequest(GURL(url::kAboutBlankURL));
request.SetFetchRequestMode(network::mojom::FetchRequestMode::kNavigate);
request.SetFetchRedirectMode(network::mojom::FetchRedirectMode::kManual);
request.SetRequestContext(blink::mojom::RequestContextType::INTERNAL);
request.SetRequestorOrigin(blink::WebSecurityOrigin::CreateUniqueOpaque());
main_frame->StartNavigation(request);
main_frame->StartNavigation(WebURLRequest(GURL(url::kAboutBlankURL)));
}
void BlinkTestRunner::OnTestFinishedInSecondaryRenderer() {
......
<script>
function navigateOnce()
function navigate()
{
let ran = false;
return function() {
if (ran)
return;
ran = true;
anchor = document.createElement("a");
anchor.href = "about:blank";
anchor.click();
}
}
function logArray()
{
array = [];
array.__defineGetter__(0, navigateOnce());
array.__defineGetter__(0, () => navigate());
console.log(array);
}
function logDate()
{
var a = new Date();
a.toString = navigateOnce();
a.toString = () => navigate();
console.log(a);
}
function logDateWithArg()
{
var a = new Date();
a.toString = navigateOnce();
a.toString = () => navigate();
console.log(42, a);
}
</script>
</script>
\ No newline at end of file
(async function(testRunner) {
var {page, session, dp} = await testRunner.startBlank(`This tests how navigation is handled from inside debugger code (console.log).`);
setTimeout(()=>testRunner.completeTest(), 3000);
await session.evaluateAsync(`
function appendIframe(url) {
var frame = document.createElement('iframe');
frame.id = 'iframe';
frame.src = url;
document.body.appendChild(frame);
return new Promise(resolve => frame.onload = resolve);
}
appendIframe('${testRunner.url('../resources/console-log-navigate-on-load.html')}')
`);
await dp.Runtime.enable();
await checkExpression('logArray()');
await checkExpression('logDate()');
await checkExpression('logDateWithArg()');
testRunner.completeTest();
async function checkExpression(expression) {
const iframeUrl = testRunner.url('../resources/console-log-navigate.html');
const iframeContextCreated = dp.Runtime.onceExecutionContextCreated();
await session.evaluateAsync(`
new Promise(resolve => {
let frame = document.createElement('iframe');
frame.src = "${iframeUrl}";
frame.onload = resolve;
document.body.appendChild(frame);
});
`);
const contextId = (await iframeContextCreated).params.context.id;
var contextId;
await dp.Runtime.onceExecutionContextCreated();
dp.Runtime.onceExecutionContextCreated().then(result => contextId = result.params.context.id);
await session.evaluateAsync(`appendIframe('${testRunner.url('../resources/console-log-navigate.html')}')`);
testRunner.log(`Got new context: ${contextId !== undefined}`);
const aboutBlankContextCreated = dp.Runtime.onceExecutionContextCreated();
testRunner.log(await dp.Runtime.evaluate({
expression: expression,
contextId: contextId
}));
await aboutBlankContextCreated;
testRunner.log(await dp.Runtime.evaluate({ expression: expression, contextId: contextId }));
}
})
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