Commit 97810541 authored by Camille Lamy's avatar Camille Lamy Committed by Chromium LUCI CQ

Revert "Reinitialize isolated world window proxies on navigation if needed."

This reverts commit d24a515a.

Reason for revert: Several  http/tests/security/isolatedWorld are failing following this CL landing. See https://bugs.chromium.org/p/chromium/issues/detail?id=1163893.

Original change's description:
> Reinitialize isolated world window proxies on navigation if needed.
>
> When a window proxy is initialized for a child frame, it must be
> reinitialized on every subsequent navigation to correctly hook up the
> window proxy (which may have cached JS references).
>
> Previously, this was only happening for the main world window proxy. One
> visible side effect is that if an extension cached a window proxy
> reference to a frame that was then navigated, the cached reference would
> not work until something else forced the window proxy to be
> reinitialized.
>
> Fixing this has an unintended side effect in an inspector protocol test
> which logs context creation: since isolated world window proxies are
> reinitialized in an arbitrary order (specifically, HashMap iteration
> order), the expected results become arbitrary as well. Instead of hoping
> that iteration order remains consistent, update the test to only log the
> context creation events it actually cares about.
>
> Change-Id: I11d7e13f7c2810d1c662ccf40f37c880e5026db5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2607680
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#840983}

TBR=dcheng@chromium.org,yukishiino@chromium.org,haraken@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Bug: 1163893
Change-Id: I67acd05b659eb0134582301931f1ed2f3d7844bb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612850
Commit-Queue: Camille Lamy <clamy@chromium.org>
Reviewed-by: default avatarCamille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841024}
parent 565bce6e
......@@ -169,8 +169,8 @@ void LocalWindowProxy::Initialize() {
scoped_refptr<const SecurityOrigin> origin;
if (world_->IsMainWorld()) {
// This also updates the ActivityLogger for the main world.
UpdateDocumentForMainWorld();
// ActivityLogger for main world is updated within updateDocumentInternal().
UpdateDocumentInternal();
origin = GetFrame()->DomWindow()->GetSecurityOrigin();
} else {
UpdateActivityLogger();
......@@ -433,6 +433,7 @@ void LocalWindowProxy::SetSecurityToken(const SecurityOrigin* origin) {
}
void LocalWindowProxy::UpdateDocument() {
DCHECK(world_->IsMainWorld());
// For an uninitialized main window proxy, there's nothing we need
// to update. The update is done when the window proxy gets initialized later.
if (lifecycle_ == Lifecycle::kContextIsUninitialized)
......@@ -448,14 +449,10 @@ void LocalWindowProxy::UpdateDocument() {
return;
}
if (!world_->IsMainWorld())
return;
UpdateDocumentForMainWorld();
UpdateDocumentInternal();
}
void LocalWindowProxy::UpdateDocumentForMainWorld() {
DCHECK(world_->IsMainWorld());
void LocalWindowProxy::UpdateDocumentInternal() {
UpdateActivityLogger();
UpdateDocumentProperty();
UpdateSecurityOrigin(GetFrame()->DomWindow()->GetSecurityOrigin());
......
......@@ -92,10 +92,9 @@ class LocalWindowProxy final : public WindowProxy {
// Triggers updates of objects that are associated with a Document:
// - the activity logger
// - the document DOM wrapper (performance optimization for accessing
// window.document in the main world)
// - the document DOM wrapper
// - the security origin
void UpdateDocumentForMainWorld();
void UpdateDocumentInternal();
// The JavaScript wrapper for the document object is cached on the global
// object for fast access. UpdateDocumentProperty sets the wrapper
......
......@@ -177,7 +177,7 @@ v8::ExtensionConfiguration ScriptController::ExtensionsFor(
}
void ScriptController::UpdateDocument() {
window_proxy_manager_->UpdateDocument();
window_proxy_manager_->MainWorldProxyMaybeUninitialized()->UpdateDocument();
}
void ScriptController::ExecuteJavaScriptURL(
......
......@@ -114,16 +114,6 @@ WindowProxy* WindowProxyManager::WindowProxyMaybeUninitialized(
return window_proxy;
}
void LocalWindowProxyManager::UpdateDocument() {
MainWorldProxyMaybeUninitialized()->UpdateDocument();
for (auto& entry : isolated_worlds_) {
auto* isolated_window_proxy =
static_cast<LocalWindowProxy*>(entry.value.Get());
isolated_window_proxy->UpdateDocument();
}
}
void LocalWindowProxyManager::UpdateSecurityOrigin(
const SecurityOrigin* security_origin) {
static_cast<LocalWindowProxy*>(window_proxy_.Get())
......
......@@ -93,8 +93,6 @@ class LocalWindowProxyManager
return static_cast<LocalWindowProxy*>(window_proxy_.Get());
}
void UpdateDocument();
// Sets the given security origin to the main world's context. Also updates
// the security origin of the context for each isolated world.
void UpdateSecurityOrigin(const SecurityOrigin*);
......
......@@ -2,7 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "third_party/blink/public/web/web_script_source.h"
#include "third_party/blink/renderer/core/testing/sim/sim_request.h"
#include "third_party/blink/renderer/core/testing/sim/sim_test.h"
#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
......@@ -50,46 +49,4 @@ TEST_F(WindowProxyTest, ReinitializedAfterNavigation) {
ASSERT_GT(ConsoleMessages().size(), 0U);
EXPECT_EQ("PASSED", ConsoleMessages()[0]);
}
TEST_F(WindowProxyTest, IsolatedWorldReinitializedAfterNavigation) {
SimRequest main_resource("https://example.com/index.html", "text/html");
LoadURL("https://example.com/index.html");
main_resource.Complete(R"HTML(
<!DOCTYPE html>
<html><body><iframe></iframe></body></html>
)HTML");
ASSERT_TRUE(MainFrame().FirstChild());
v8::HandleScope scope(v8::Isolate::GetCurrent());
const int32_t kIsolatedWorldId = 42;
// Save a reference to the top `window` in the isolated world.
v8::Local<v8::Value> window_top =
MainFrame().ExecuteScriptInIsolatedWorldAndReturnValue(
kIsolatedWorldId, WebScriptSource("window"));
ASSERT_TRUE(window_top->IsObject());
// Save a reference to the child frame's window proxy in the isolated world.
v8::Local<v8::Value> saved_child_window =
MainFrame().ExecuteScriptInIsolatedWorldAndReturnValue(
kIsolatedWorldId, WebScriptSource("saved = window[0]"));
ASSERT_TRUE(saved_child_window->IsObject());
frame_test_helpers::LoadFrame(MainFrame().FirstChild()->ToWebLocalFrame(),
"data:text/html,<body><p>Hello</p></body>");
ASSERT_TRUE(MainFrame().FirstChild());
// Test if the window proxy of the navigated frame was reinitialized. The
// `top` attribute of the saved child frame's window proxy reference should
// refer to the same object as the top-level window proxy reference that was
// cached earlier.
v8::Local<v8::Value> top_via_saved =
MainFrame().ExecuteScriptInIsolatedWorldAndReturnValue(
kIsolatedWorldId, WebScriptSource("saved.top"));
EXPECT_TRUE(top_via_saved->IsObject());
EXPECT_TRUE(window_top->StrictEquals(top_via_saved));
}
} // namespace blink
......@@ -6,13 +6,9 @@
const scriptIds = [];
dp.Runtime.onConsoleAPICalled(msg => testRunner.log(msg.params.args[0].value));
let logContextCreationCallback = msg => {
dp.Runtime.onExecutionContextCreated(msg => {
if (msg.params.context.name.includes('world'))
testRunner.log(msg.params.context.name);
};
dp.Runtime.onExecutionContextCreated(msg => {
if (logContextCreationCallback)
logContextCreationCallback(msg);
});
testRunner.log('Adding scripts');
......@@ -31,7 +27,6 @@
testRunner.log('Failed script removal');
}
logContextCreationCallback = null;
await session.navigate('../resources/blank.html');
testRunner.completeTest();
......
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