f523d98bee1bbaaf1486ddb2221901e7e93c0cfa - chromium/src

[mpea...@chromium.org]

commit f523d98bee1bbaaf1486ddb2221901e7e93c0cfa
Author: Mark Pearson <mpea...@chromium.org>
AuthorDate: Wed Mar 02 00:06:17 2022
Commit: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
CommitDate: Wed Mar 02 00:06:17 2022

Revert "Add documentId to WebNavigation GetFrame API"

This reverts commit dbba982d42a9917687ac97d9c0597d835c11beea.

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...@chromium.org>
> Commit-Queue: Dave Tapuska <dtap...@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 <mpea...@chromium.org>
Owners-Override: Mark Pearson <mpea...@chromium.org>
Commit-Queue: Rubber Stamper <rubber-...@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-...@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#976477}

diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
index ba97b2d..6a16b9c8 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
@@ -453,69 +453,25 @@
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 = nullptr;
- if (!ExtensionTabUtil::GetTabById(tab_id, browser_context(),
- include_incognito_information(),
- &web_contents) ||
- !web_contents) {
- return RespondNow(OneArgument(base::Value()));
- }
-
- render_frame_host = ExtensionApiFrameIdMap::Get()->GetRenderFrameHostById(
- web_contents, 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()));
}

+ WebNavigationTabObserver* observer =
+ WebNavigationTabObserver::Get(web_contents);
+ DCHECK(observer);
+
+ content::RenderFrameHost* render_frame_host =
+ ExtensionApiFrameIdMap::Get()->GetRenderFrameHostById(web_contents,
+ frame_id);
+
auto* frame_navigation_state =
render_frame_host
? FrameNavigationState::GetForCurrentDocument(render_frame_host)
@@ -555,7 +511,7 @@
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) ||
@@ -563,6 +519,10 @@
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
diff --git a/chrome/browser/extensions/extension_tab_util.cc b/chrome/browser/extensions/extension_tab_util.cc
index 253ea51..e81672c 100644
--- a/chrome/browser/extensions/extension_tab_util.cc
+++ b/chrome/browser/extensions/extension_tab_util.cc
@@ -769,24 +769,6 @@
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);
diff --git a/chrome/browser/extensions/extension_tab_util.h b/chrome/browser/extensions/extension_tab_util.h
index a4faba97..0b196e5 100644
--- a/chrome/browser/extensions/extension_tab_util.h
+++ b/chrome/browser/extensions/extension_tab_util.h
@@ -202,12 +202,6 @@
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|
diff --git a/chrome/common/extensions/api/web_navigation.json b/chrome/common/extensions/api/web_navigation.json
index 77f907c..1ba0e91 100644
--- a/chrome/common/extensions/api/web_navigation.json
+++ b/chrome/common/extensions/api/web_navigation.json
@@ -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." }
}
}
],
diff --git a/chrome/test/data/extensions/api_test/webnavigation/getFrame/test_getFrame.js b/chrome/test/data/extensions/api_test/webnavigation/getFrame/test_getFrame.js
index 61cd3d3..9a3ead6 100644
--- a/chrome/test/data/extensions/api_test/webnavigation/getFrame/test_getFrame.js
+++ b/chrome/test/data/extensions/api_test/webnavigation/getFrame/test_getFrame.js
@@ -7,9 +7,6 @@
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 {
@@ -61,77 +58,6 @@
});
},

- 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(
diff --git a/chrome/test/data/extensions/api_test/webnavigation/targetBlank/test_targetBlank.js b/chrome/test/data/extensions/api_test/webnavigation/targetBlank/test_targetBlank.js
index a404a1b1..9533888 100644
--- a/chrome/test/data/extensions/api_test/webnavigation/targetBlank/test_targetBlank.js
+++ b/chrome/test/data/extensions/api_test/webnavigation/targetBlank/test_targetBlank.js
@@ -15,18 +15,10 @@
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",
@@ -135,21 +127,5 @@
// 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();
- });
- }
]);
});
diff --git a/extensions/browser/extension_api_frame_id_map.cc b/extensions/browser/extension_api_frame_id_map.cc
index 49f8b0b0..b28fa94 100644
--- a/extensions/browser/extension_api_frame_id_map.cc
+++ b/extensions/browser/extension_api_frame_id_map.cc
@@ -138,31 +138,6 @@
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 {
@@ -312,14 +287,10 @@
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);

diff --git a/extensions/browser/extension_api_frame_id_map.h b/extensions/browser/extension_api_frame_id_map.h
index 5c976bee..842eab5 100644
--- a/extensions/browser/extension_api_frame_id_map.h
+++ b/extensions/browser/extension_api_frame_id_map.h
@@ -145,14 +145,6 @@
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);

@@ -196,10 +188,6 @@
// 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