Skip to content

Commit

Permalink
Prerender2: Accept FrameTreeNodeId for primary main frames too
Browse files Browse the repository at this point in the history 
If a valid FrameTreeNodeId for the primary main frame is specified as
frameId, ScriptExecutor::Handler inconsistently convert it to 0 internally, and ExecuteCodeFunction panics on receiving the results
with unexpected frameId 0.

This happens regardless of Prerendering, but is happening more
frequently probably because Extensions have more chance to specify
stale frameIds with prerendering, and it matches to the newly activated
page's frameId by chance.

In the ScriptExecutor::Handler, we push RenderFrameHost into
`pending_render_frames_` and recalculate the relevant frameId
before the script execution. This is synchronous operation and
assumed as safe. But if FrameTreeNodeId reaches here, the
recalculated frameId for the result gets to be 0.
Thus ExecuteCodeFunction is confused on the frameId different
between the request and result. This patch modifies the Result
preparation to be done with the exact given frameId.

This patch also reverts the previous debugging code in
ExecuteCodeFunction.

Bug: 1346998
Change-Id: Ie3c0635e994e94659c1cc9dd5fdac3268f10e08a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3810982
Reviewed-by: Huanpo Lin <robertlin@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1035014}
  • Loading branch information
@toyoshim
toyoshim authored and Chromium LUCI CQ committed Aug 15, 2022
1 parent 0902e24 commit acb2fe7
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 53 deletions.
39 changes: 30 additions & 9 deletions chrome/test/data/extensions/api_test/tabs/prerendering/test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ async function testGetTitleByDocumentId() {

// Checks if injected scripts continue working while activating the pre-rendered
// pages. Also, checks if content scripts can activate the pre-rendered page.
async function testActivationOnExecution() {
async function testActivateOnExecution() {
await setup();
await new Promise(resolve => {
// Inject a script that keeps alive until the page activation.
Expand All @@ -97,21 +97,41 @@ async function testActivationOnExecution() {
// Start monitoring tab update events.
chrome.tabs.onUpdated.addListener(function cb(updatedTabId, changeInfo, tab) {
chrome.test.assertEq(tabId, updatedTabId);
if (changeInfo.title && changeInfo.title == 'activated') {
if (changeInfo.title && changeInfo.title === 'activated') {
chrome.tabs.onUpdated.removeListener(cb);
chrome.test.succeed();
}
});

// Inject a script that activates the pre-rendered page.
await new Promise(resolve => {
chrome.tabs.executeScript(
tabId, {code: 'window.location.href = "./prerendering.html";'},
result => {
// No results, but just checks if it doesn't crash.
resolve();
chrome.tabs.executeScript(
tabId, {code: 'window.location.href = "./prerendering.html";'});
}

async function testExecuteAfterActivation() {
await setup();

// Start monitoring tab update events.
chrome.tabs.onUpdated.addListener(function cb(updatedTabId, changeInfo, tab) {
chrome.test.assertEq(tabId, updatedTabId);
if (changeInfo.status && changeInfo.status === 'complete') {
// Specify the hidden FrameTreeNodeId for the activated frame. JavaScript
// could not know it, but it should just work as 0 is just an alternative
// and internal FrameTreeNodeId should also work like a frameId.
chrome.tabs.executeScript(
tabId, { frameId: 1, code: 'document.title' },
results => {
chrome.tabs.onUpdated.removeListener(cb);
chrome.test.assertEq(1, results.length);
chrome.test.assertEq('prerendering', results[0]);
chrome.test.succeed();
});
}
});

// Inject a script that activates the pre-rendered page.
chrome.tabs.executeScript(
tabId, {code: 'window.location.href = "./prerendering.html";'});
}

// Checks if navigation via chrome.tabs.update() doesn't activate pre-rendered
Expand Down Expand Up @@ -147,7 +167,8 @@ chrome.test.getConfig(async config => {
testGetTitleForAllFrames,
testGetTitleByFrameId,
testGetTitleByDocumentId,
testActivationOnExecution,
testActivateOnExecution,
testExecuteAfterActivation,
testDontActivateByUpdate,
]);
});
13 changes: 4 additions & 9 deletions extensions/browser/api/execute_code_function.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <utility>

#include "base/bind.h"
#include "base/debug/alias.h"
#include "extensions/browser/api/extension_types_utils.h"
#include "extensions/browser/extension_api_frame_id_map.h"
#include "extensions/browser/extensions_browser_client.h"
Expand Down Expand Up @@ -234,14 +233,10 @@ void ExecuteCodeFunction::OnExecuteCodeFinished(
// exist), we provide a different error message for backwards
// compatibility.
if (!root_frame_result->frame_responded) {
DEBUG_ALIAS_FOR_CSTR(root_frame_error, root_frame_result->error.c_str(),
64);
int found_root_frame_id = root_frame_result->frame_id;
base::debug::Alias(&found_root_frame_id);
Respond(Error(root_frame_id_ == ExtensionApiFrameIdMap::kTopFrameId
? "The tab was closed."
: "The frame was removed."));
return;
root_frame_result->error =
root_frame_id_ == ExtensionApiFrameIdMap::kTopFrameId
? "The tab was closed."
: "The frame was removed.";
}

Respond(Error(std::move(root_frame_result->error)));
Expand Down
77 changes: 42 additions & 35 deletions extensions/browser/script_executor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,11 @@ class Handler : public content::WebContentsObserver {
continue;
}

pending_render_frames_.push_back(frame);
// `frame_id` can be a FrameTreeNodeId of the primary main frame. In such
// cases, ExtensionApiFrameIdMap::GetFrameId(frame) resolves the given
// `frame` as 0. To keep the original ID as is, pass `frame_id` and use it
// directly to prepare a relevant FrameResult.
PushPendingRenderFrame(frame, frame_id);
}

// If there is a single frame specified (and it was valid), we consider it
Expand All @@ -104,32 +108,13 @@ class Handler : public content::WebContentsObserver {
// `pending_render_frames_` and add them if they are alive (and not already
// contained in `pending_frames`).
if (scope == ScriptExecutor::INCLUDE_SUB_FRAMES) {
auto append_frame = [](content::WebContents* web_contents,
std::vector<raw_ptr<content::RenderFrameHost>>*
pending_frames,
content::RenderFrameHost* frame) {
// Avoid inner web contents. If we need to execute scripts on
// inner WebContents this class needs to be updated.
// See crbug.com/1301320.
if (content::WebContents::FromRenderFrameHost(frame) != web_contents) {
return content::RenderFrameHost::FrameIterationAction::kSkipChildren;
}
if (!frame->IsRenderFrameLive() ||
base::Contains(*pending_frames, frame)) {
return content::RenderFrameHost::FrameIterationAction::kContinue;
}

pending_frames->push_back(frame);
return content::RenderFrameHost::FrameIterationAction::kContinue;
};

// We iterate over the requested frames. Note we can't use an iterator
// as the for loop will mutate `pending_render_frames_`.
size_t requested_frame_count = pending_render_frames_.size();
const size_t requested_frame_count = pending_render_frames_.size();
for (size_t i = 0; i < requested_frame_count; ++i) {
pending_render_frames_.at(i)->ForEachRenderFrameHost(
base::BindRepeating(append_frame, web_contents,
&pending_render_frames_));
base::BindRepeating(&Handler::MaybeAddSubFrame,
base::Unretained(this)));
}
}

Expand Down Expand Up @@ -172,6 +157,40 @@ class Handler : public content::WebContentsObserver {
Finish();
}

content::RenderFrameHost::FrameIterationAction MaybeAddSubFrame(
content::RenderFrameHost* frame) {
// Avoid inner web contents. If we need to execute scripts on inner
// WebContents this class needs to be updated.
// See https://crbug.com/1301320.
if (content::WebContents::FromRenderFrameHost(frame) != web_contents()) {
return content::RenderFrameHost::FrameIterationAction::kSkipChildren;
}
if (!frame->IsRenderFrameLive() ||
base::Contains(pending_render_frames_, frame)) {
return content::RenderFrameHost::FrameIterationAction::kContinue;
}

PushPendingRenderFrame(frame, ExtensionApiFrameIdMap::GetFrameId(frame));
return content::RenderFrameHost::FrameIterationAction::kContinue;
}

void PushPendingRenderFrame(raw_ptr<content::RenderFrameHost> frame,
int frame_id) {
pending_render_frames_.push_back(frame);

// Preallocate the results to hold the initial `frame_id` and `document_id`.
// As the primary main frame uses a magic number 0 for the `frame_id`, it
// can be changed if the primary page is changed. It happens on pre-rendered
// page activation or portal page activation on MPArch. The `document_id`
// can be stale if navigation happens and the same renderer is reused in the
// case, e.g. navigation from about:blank, or same-origin navigation.
ScriptExecutor::FrameResult result;
result.frame_id = frame_id;
result.document_id = ExtensionApiFrameIdMap::GetDocumentId(frame);
DCHECK(!base::Contains(results_, frame->GetFrameToken()));
results_[frame->GetFrameToken()] = std::move(result);
}

void AddWillNotInjectResult(
int frame_id,
const ExtensionApiFrameIdMap::DocumentId& document_id,
Expand Down Expand Up @@ -217,18 +236,6 @@ class Handler : public content::WebContentsObserver {
DCHECK(frame->IsRenderFrameLive());
DCHECK(base::Contains(pending_render_frames_, frame));

// Preallocate the results to hold the initial `frame_id` and `document_id`.
// As the primary main frame uses a magic number 0 for the `frame_id`, it
// can be changed if the primary page is changed. It happens on pre-rendered
// page activation or portal page activation on MPArch. The `document_id`
// can be stale if navigation happens and the same renderer is reused in the
// case, e.g. navigation from about:blank, or same-origin navigation.
ScriptExecutor::FrameResult result;
result.frame_id = ExtensionApiFrameIdMap::GetFrameId(frame);
result.document_id = ExtensionApiFrameIdMap::GetDocumentId(frame);
DCHECK(!base::Contains(results_, frame->GetFrameToken()));
results_[frame->GetFrameToken()] = std::move(result);

ContentScriptTracker::WillExecuteCode(pass_key, frame, host_id_);
ExtensionWebContentsObserver::GetForWebContents(web_contents())
->GetLocalFrame(frame)
Expand Down

0 comments on commit acb2fe7

Please sign in to comment.