Skip to content

Commit

Permalink
Revert "Add documentId to WebNavigation GetFrame API"
Browse files Browse the repository at this point in the history 
This reverts commit dbba982.

Reason for revert:
Likely cause of consistent failures on the linux-ubsan-vptr bot:
https://ci.chromium.org/p/chromium/builders/ci/linux-ubsan-vptr

Failures are in browser_tests, which fail these two:
- PersistentBackground/WebNavigationApiTestWithContextType.TargetBlank/0
- PersistentBackground/WebNavigationApiTestWithContextType.TargetBlankIncognito/0
consistently after this change landed.

Failures look like:
Value of: catcher.GetNextResult()
  Actual: false
Expected: true
Failed 1 of 2 tests


Here's a full message with context if you need it for
PersistentBackground/WebNavigationApiTestWithContextType.TargetBlank/0
----
[ RUN      ] PersistentBackground/WebNavigationApiTestWithContextType.TargetBlank/0
[464:464:0301/144733.588542:WARNING:field_trial_util.cc(105)] Field trial config study skipped: DesktopTabGroupsUserEducation.Enabled (some of its features are already overridden)
[464:464:0301/144733.588746:WARNING:field_trial_util.cc(105)] Field trial config study skipped: GoogleLensDesktopContextMenuSearch.Enabled (some of its features are already overridden)
[464:464:0301/144733.588871:WARNING:field_trial_util.cc(105)] Field trial config study skipped: OmniboxUpdatedConnectionSecurityIndicatorsIPH.Enabled (some of its features are already overridden)
[464:464:0301/144733.588964:WARNING:field_trial_util.cc(105)] Field trial config study skipped: PreconnectToSearchDesktop.EnabledWithStartupDelayForegroundOnly (some of its features are already overridden)
[464:464:0301/144733.589045:WARNING:field_trial_util.cc(105)] Field trial config study skipped: SharedHighlightingIphDesktop.Enabled (some of its features are already overridden)
[464:464:0301/144733.589080:WARNING:field_trial_util.cc(105)] Field trial config study skipped: TabAudioMuting.Enabled (some of its features are already overridden)
[464:464:0301/144733.589102:WARNING:field_trial_util.cc(105)] Field trial config study skipped: TabSearchIPH.TabSearchIPH (some of its features are already overridden)
[464:464:0301/144733.589268:WARNING:field_trial_util.cc(105)] Field trial config study skipped: WebUITabStrip.Enabled (some of its features are already overridden)
libva error: va_getDriverName() failed with unknown libva error,driver_name=(null)
[656:656:0301/144733.806065:WARNING:sandbox_linux.cc(377)] InitializeSandbox() called with multiple threads in process gpu-process.
[656:656:0301/144733.850910:ERROR:gpu_memory_buffer_support_x11.cc(44)] dri3 extension not supported.
[464:464:0301/144733.932449:WARNING:bluez_dbus_manager.cc(248)] Floss manager not present, cannot set Floss enable/disable.
[464:718:0301/144733.993927:ERROR:object_proxy.cc(623)] Failed to call method: org.freedesktop.DBus.Properties.Get: object_path= /org/freedesktop/UPower: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.UPower was not provided by any .service files
[464:718:0301/144733.993972:WARNING:property.cc(144)] DaemonVersion: GetAndBlock: failed.
[464:718:0301/144733.994251:ERROR:object_proxy.cc(623)] Failed to call method: org.freedesktop.UPower.GetDisplayDevice: object_path= /org/freedesktop/UPower: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.UPower was not provided by any .service files
[464:718:0301/144733.995241:ERROR:object_proxy.cc(623)] Failed to call method: org.freedesktop.UPower.EnumerateDevices: object_path= /org/freedesktop/UPower: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.UPower was not provided by any .service files
[464:749:0301/144734.486797:WARNING:embedded_test_server.cc(665)] Request not handled. Returning 404: /favicon.ico
[464:464:0301/144734.572404:INFO:CONSOLE(0)] "[SUCCESS] targetBlank", source: chrome-extension://dakfcpefccmhodaclomjmbepmggkkebb/_generated_background_page.html (0)
[464:464:0301/144734.577512:INFO:CONSOLE(0)] "[FAIL] testGetFrame: API Test Error in testGetFrame
Actual: null
Expected: {"errorOccurred":false,"url":"http://127.0.0.1:33405/extensions/api_test/webnavigation/targetBlank/a.html","parentFrameId":-1,"documentId":"EEAB76DF6EE3FA5A5D1FD6CFFA67B4E6","documentLifecycle":"active","frameType":"outermost_frame"}
Error
    at extensions::test:248:20
    at chrome-extension://dakfcpefccmhodaclomjmbepmggkkebb/test_targetBlank.js:143:23", source: chrome-extension://dakfcpefccmhodaclomjmbepmggkkebb/_generated_background_page.html (0)
../../chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:489: Failure
Value of: catcher.GetNextResult()
  Actual: false
Expected: true
Failed 1 of 2 tests
Stack trace:
#0 0x55c0a052ad5c extensions::WebNavigationApiTestWithContextType_TargetBlank_Test::RunTestOnMainThread()
#1 0x55c0a95c0429 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop()
#2 0x55c0a41d24e7 content::BrowserMainLoop::InterceptMainMessageLoopRun()
#3 0x55c0a41d25f2 content::BrowserMainLoop::RunMainMessageLoop()
#4 0x55c0a41d7381 content::BrowserMainRunnerImpl::Run()
#5 0x55c0a41cc372 content::BrowserMain()
#6 0x55c0a5c52a82 content::RunBrowserProcessMain()
#7 0x55c0a5c55580 content::ContentMainRunnerImpl::RunBrowser()
#8 0x55c0a5c547e7 content::ContentMainRunnerImpl::Run()
#9 0x55c0a5c4ff32 content::RunContentProcess()
#10 0x55c0a5c50a4e content::ContentMain()
#11 0x55c0a95bf226 content::BrowserTestBase::SetUp()
#12 0x55c0a82e1175 InProcessBrowserTest::SetUp()

[464:464:0301/144735.270606:WARNING:pref_notifier_impl.cc(41)] Pref observer for media_router.cast_allow_all_ips found at shutdown.
[  FAILED  ] PersistentBackground/WebNavigationApiTestWithContextType.TargetBlank/0, where GetParam() = 4-byte object <03-00 00-00> (2103 ms)
----

Original change's description:
> Add documentId to WebNavigation GetFrame API
>
> - Make tabId and frameId optional.
> - Support querying based solely on the documentId.
>
> See https://bit.ly/3G4RBEn for discussion regarding this change.
>
> BUG=1264911
>
> Change-Id: I399bc050d4dea144cdce79ff7084c31cd012b094
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3448388
> Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
> Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#976413}

Bug: 1264911
Change-Id: Ifc94658fcc11e214065f4d1d73c2831c9caa6172
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3499404
Auto-Submit: Mark Pearson <mpearson@chromium.org>
Owners-Override: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#976477}
  • Loading branch information
Mark Pearson authored and Chromium LUCI CQ committed Mar 2, 2022
1 parent 4afa9ce commit f523d98
Show file tree
Hide file tree
Showing 8 changed files with 24 additions and 228 deletions.
80 changes: 20 additions & 60 deletions chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,68 +453,24 @@ void WebNavigationTabObserver::RenderFrameHostPendingDeletion(
ExtensionFunction::ResponseAction WebNavigationGetFrameFunction::Run() {
std::unique_ptr<GetFrame::Params> params(GetFrame::Params::Create(args()));
EXTENSION_FUNCTION_VALIDATE(params.get());
int tab_id = params->details.tab_id;
int frame_id = params->details.frame_id;

int tab_id = api::tabs::TAB_ID_NONE;
int frame_id = -1;

content::RenderFrameHost* render_frame_host = nullptr;
if (params->details.document_id) {
ExtensionApiFrameIdMap::DocumentId document_id =
ExtensionApiFrameIdMap::DocumentIdFromString(
*params->details.document_id);
if (!document_id)
return RespondNow(Error("Invalid documentId."));

// Note that we will globally find a RenderFrameHost but validate that
// we are in the right context still as we may be in the wrong profile
// or in incognito mode.
render_frame_host =
ExtensionApiFrameIdMap::Get()->GetRenderFrameHostByDocumentId(
document_id);

if (!render_frame_host)
return RespondNow(OneArgument(base::Value()));

content::WebContents* web_contents =
content::WebContents::FromRenderFrameHost(render_frame_host);
// We found the RenderFrameHost through a generic lookup so we must test to
// see if the WebContents is actually in our BrowserContext.
if (!ExtensionTabUtil::IsWebContentsInContext(
web_contents, browser_context(), include_incognito_information())) {
return RespondNow(OneArgument(base::Value()));
}

tab_id = ExtensionTabUtil::GetTabId(web_contents);
frame_id = ExtensionApiFrameIdMap::GetFrameId(render_frame_host);

// If the provided tab_id and frame_id do not match the calculated ones
// return.
if ((params->details.tab_id && *params->details.tab_id != tab_id) ||
(params->details.frame_id && *params->details.frame_id != frame_id)) {
return RespondNow(OneArgument(base::Value()));
}
} else {
// If documentId is not provided, tab_id and frame_id must be. Return early
// if not.
if (!params->details.tab_id || !params->details.frame_id) {
return RespondNow(Error(
"Either documentId or both tabId and frameId must be specified."));
}

tab_id = *params->details.tab_id;
frame_id = *params->details.frame_id;
content::WebContents* web_contents;
if (!ExtensionTabUtil::GetTabById(tab_id, browser_context(),
include_incognito_information(),
&web_contents) ||
!web_contents) {
return RespondNow(OneArgument(base::Value()));
}

content::WebContents* web_contents = nullptr;
if (!ExtensionTabUtil::GetTabById(tab_id, browser_context(),
include_incognito_information(),
&web_contents) ||
!web_contents) {
return RespondNow(OneArgument(base::Value()));
}
WebNavigationTabObserver* observer =
WebNavigationTabObserver::Get(web_contents);
DCHECK(observer);

render_frame_host = ExtensionApiFrameIdMap::Get()->GetRenderFrameHostById(
web_contents, frame_id);
}
content::RenderFrameHost* render_frame_host =
ExtensionApiFrameIdMap::Get()->GetRenderFrameHostById(web_contents,
frame_id);

auto* frame_navigation_state =
render_frame_host
Expand Down Expand Up @@ -555,14 +511,18 @@ ExtensionFunction::ResponseAction WebNavigationGetAllFramesFunction::Run() {
EXTENSION_FUNCTION_VALIDATE(params.get());
int tab_id = params->details.tab_id;

content::WebContents* web_contents = nullptr;
content::WebContents* web_contents;
if (!ExtensionTabUtil::GetTabById(tab_id, browser_context(),
include_incognito_information(),
&web_contents) ||
!web_contents) {
return RespondNow(OneArgument(base::Value()));
}

WebNavigationTabObserver* observer =
WebNavigationTabObserver::Get(web_contents);
DCHECK(observer);

std::vector<GetAllFrames::Results::DetailsType> result_list;

// We only iterate the frames in the active page. We currently do not
Expand Down
18 changes: 0 additions & 18 deletions chrome/browser/extensions/extension_tab_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,24 +769,6 @@ ExtensionTabUtil::GetAllActiveWebContentsForContext(
return active_contents;
}

// static
bool ExtensionTabUtil::IsWebContentsInContext(
content::WebContents* web_contents,
content::BrowserContext* browser_context,
bool include_incognito) {
// Look at the WebContents BrowserContext and see if it is the same.
content::BrowserContext* web_contents_browser_context =
web_contents->GetBrowserContext();
if (web_contents_browser_context == browser_context)
return true;

// If not it might be to include the incongito mode, so we if the profiles
// are the same or the parent.
return include_incognito && Profile::FromBrowserContext(browser_context)
->IsSameOrParent(Profile::FromBrowserContext(
web_contents_browser_context));
}

GURL ExtensionTabUtil::ResolvePossiblyRelativeURL(const std::string& url_string,
const Extension* extension) {
GURL url = GURL(url_string);
Expand Down
6 changes: 0 additions & 6 deletions chrome/browser/extensions/extension_tab_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,6 @@ class ExtensionTabUtil {
content::BrowserContext* browser_context,
bool include_incognito);

// Determines if the |web_contents| is in |browser_context| or it's OTR
// BrowserContext if |include_incognito| is true.
static bool IsWebContentsInContext(content::WebContents* web_contents,
content::BrowserContext* browser_context,
bool include_incognito);

// Takes |url_string| and returns a GURL which is either valid and absolute
// or invalid. If |url_string| is not directly interpretable as a valid (it is
// likely a relative URL) an attempt is made to resolve it. When |extension|
Expand Down
5 changes: 2 additions & 3 deletions chrome/common/extensions/api/web_navigation.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,14 @@
"name": "details",
"description": "Information about the frame to retrieve information about.",
"properties": {
"tabId": { "type": "integer", "optional": true, "minimum": 0, "description": "The ID of the tab in which the frame is." },
"tabId": { "type": "integer", "minimum": 0, "description": "The ID of the tab in which the frame is." },
"processId": {
"type": "integer",
"optional": true,
"deprecated": "Frames are now uniquely identified by their tab ID and frame ID; the process ID is no longer needed and therefore ignored.",
"description": "The ID of the process that runs the renderer for this tab."
},
"frameId": { "type": "integer", "optional": true, "minimum": 0, "description": "The ID of the frame in the given tab." },
"documentId": { "type": "string", "optional": true, "nodoc": true, "description": "The UUID of the document. If the frameId and/or tabId are provided they will be validated to match the document found by provided document ID." }
"frameId": { "type": "integer", "minimum": 0, "description": "The ID of the frame in the given tab." }
}
}
],
Expand Down
74 changes: 0 additions & 74 deletions chrome/test/data/extensions/api_test/webnavigation/getFrame/test_getFrame.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ const scriptUrl = '_test_resources/api_test/webnavigation/framework.js';
let ready;
let onScriptLoad = chrome.test.loadScript(scriptUrl);

const kNotSpecifiedErrorMessage =
'Either documentId or both tabId and frameId must be specified.';

if (inServiceWorker) {
ready = onScriptLoad;
} else {
Expand Down Expand Up @@ -61,77 +58,6 @@ ready.then(async function() {
});
},

function testGetFrameNoValues() {
chrome.webNavigation.getFrame({},
function (details) {
chrome.test.assertEq(null, details);
chrome.test.assertLastError(kNotSpecifiedErrorMessage);
chrome.test.succeed();
});
},

function testGetFrameNoFrameId() {
chrome.webNavigation.getFrame({tabId: tab.id, processId: processId},
function (details) {
chrome.test.assertEq(null, details);
chrome.test.assertLastError(kNotSpecifiedErrorMessage);
chrome.test.succeed();
});
},

function testGetFrameDocumentId() {
chrome.webNavigation.getFrame({tabId: tab.id, documentId: documentId},
function (details) {
chrome.test.assertEq({
errorOccurred: false,
url: URL,
parentFrameId: -1,
documentId: documentId,
documentLifecycle: "active",
frameType: "outermost_frame",
}, details);
chrome.test.succeed();
});
},

function testGetFrameDocumentIdAndFrameId() {
chrome.webNavigation.getFrame({tabId: tab.id, frameId: 0,
processId: processId,
documentId: documentId},
function (details) {
chrome.test.assertEq({
errorOccurred: false,
url: URL,
parentFrameId: -1,
documentId: documentId,
documentLifecycle: "active",
frameType: "outermost_frame",
}, details);
chrome.test.succeed();
});
},

function testGetFrameDocumentIdAndFrameIdDoNotMatch() {
chrome.webNavigation.getFrame({tabId: tab.id, frameId: 1,
processId: processId,
documentId: documentId},
function (details) {
chrome.test.assertEq(null, details);
chrome.test.succeed();
});
},

function testGetFrameInvalidDocumentId() {
chrome.webNavigation.getFrame({tabId: tab.id, frameId: 0,
processId: processId,
documentId: "42AB"},
function (details) {
chrome.test.assertLastError("Invalid documentId.");
chrome.test.assertEq(null, details);
chrome.test.succeed();
});
},

function testGetAllFrames() {
chrome.webNavigation.getAllFrames({tabId: tab.id}, function (details) {
chrome.test.assertEq(
Expand Down
24 changes: 0 additions & 24 deletions chrome/test/data/extensions/api_test/webnavigation/targetBlank/test_targetBlank.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,10 @@ loadScript.then(async function() {
var URL_TARGET = "http://127.0.0.1:" + port +
"/extensions/api_test/webnavigation/targetBlank/b.html";

var topDocumentId;

chrome.test.runTests([
// Opens a tab and waits for the user to click on a link with
// target=_blank in it.
function targetBlank() {
// store the real documentId for the testGetFrame later.
chrome.webNavigation.onCommitted.addListener(
function(details) {
if (!topDocumentId)
topDocumentId = details.documentId;
});
expect([
{ label: "a-onBeforeNavigate",
event: "onBeforeNavigate",
Expand Down Expand Up @@ -135,21 +127,5 @@ loadScript.then(async function() {
// Notify the api test that we're waiting for the user.
chrome.test.notifyPass();
},

// Verify GetFrame via documentId works correctly in incognito mode.
function testGetFrame() {
chrome.webNavigation.getFrame({documentId: topDocumentId},
function (details) {
chrome.test.assertEq({
errorOccurred: false,
url: URL_LOAD,
parentFrameId: -1,
documentId: topDocumentId,
documentLifecycle: 'active',
frameType: 'outermost_frame',
}, details);
chrome.test.succeed();
});
}
]);
});
33 changes: 2 additions & 31 deletions extensions/browser/extension_api_frame_id_map.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,31 +138,6 @@ content::RenderFrameHost* ExtensionApiFrameIdMap::GetRenderFrameHostById(
return rfh;
}

content::RenderFrameHost*
ExtensionApiFrameIdMap::GetRenderFrameHostByDocumentId(
const DocumentId& document_id) {
auto iter = document_id_map_.find(document_id);
if (iter == document_id_map_.end())
return nullptr;
return &iter->second->render_frame_host();
}

ExtensionApiFrameIdMap::DocumentId ExtensionApiFrameIdMap::DocumentIdFromString(
const std::string& document_id) {
if (document_id.length() != 32)
return DocumentId();

base::StringPiece string_piece(document_id);
uint64_t high = 0;
uint64_t low = 0;
if (!base::HexStringToUInt64(string_piece.substr(0, 16), &high) ||
!base::HexStringToUInt64(string_piece.substr(16, 16), &low)) {
return DocumentId();
}

return base::UnguessableToken::Deserialize(high, low);
}

ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::KeyToValue(
content::GlobalRenderFrameHostId key,
bool require_live_frame) const {
Expand Down Expand Up @@ -312,14 +287,10 @@ void ExtensionApiFrameIdMap::OnRenderFrameDeleted(
ExtensionApiFrameIdMap::ExtensionDocumentUserData::ExtensionDocumentUserData(
content::RenderFrameHost* render_frame_host)
: content::DocumentUserData<ExtensionDocumentUserData>(render_frame_host),
document_id_(DocumentId::Create()) {
Get()->document_id_map_[document_id_] = this;
}
document_id_(DocumentId::Create()) {}

ExtensionApiFrameIdMap::ExtensionDocumentUserData::
~ExtensionDocumentUserData() {
Get()->document_id_map_.erase(document_id_);
}
~ExtensionDocumentUserData() = default;

DOCUMENT_USER_DATA_KEY_IMPL(ExtensionApiFrameIdMap::ExtensionDocumentUserData);

Expand Down
12 changes: 0 additions & 12 deletions extensions/browser/extension_api_frame_id_map.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,6 @@ class ExtensionApiFrameIdMap {
content::WebContents* web_contents,
int frame_id);

// Find the current RenderFrameHost for a given extension documentID.
// Returns nullptr if not found.
content::RenderFrameHost* GetRenderFrameHostByDocumentId(
const DocumentId& document_id);

// Parses a serialized document id string to a DocumentId.
static DocumentId DocumentIdFromString(const std::string& document_id);

// Retrieves the FrameData for a given RenderFrameHost id.
[[nodiscard]] FrameData GetFrameData(content::GlobalRenderFrameHostId rfh_id);

Expand Down Expand Up @@ -196,10 +188,6 @@ class ExtensionApiFrameIdMap {
// continue after a frame is unloaded can access the FrameData.
using FrameDataMap = std::map<content::GlobalRenderFrameHostId, FrameData>;
FrameDataMap deleted_frame_data_map_;

// Holds mapping of DocumentIds to ExtensionDocumentUserData objects.
using DocumentIdMap = std::map<DocumentId, ExtensionDocumentUserData*>;
DocumentIdMap document_id_map_;
};

} // namespace extensions
Expand Down

0 comments on commit f523d98

Please sign in to comment.