[Extensions] Filter profiles for tabs.discard appropriately [chromium/src : main]
Tim (Gerrit)
Tim added 3 comments![]( "Open in Gerrit")
Adding in fdoray@ for the changes in `chrome/browser/performance_manager/policies/` and `chrome/browser/resource_coordinator/`. Could you take a look at the additions in those and see if you are okay with them?
// Verify that the extension is allowed to see the discarded tab.
// We use profile matching instead of just checking IsOffTheRecord() to
// properly handle incognito 'split' mode. In split mode, the incognito
// background script needs to see its own incognito tabs but shouldn't see
// regular tabs. 'spanning' mode extensions with incognito access will pass
// the IsSameOrParent() check.
Profile* profile = Profile::FromBrowserContext(browser_context());
Profile* tab_profile =
Profile::FromBrowserContext(contents->GetBrowserContext());
bool can_see_tab =
profile == tab_profile ||
(include_incognito_information() && profile->IsSameOrParent(tab_profile));
if (!can_see_tab) {
return RespondNow(NoArguments());
}Timoh, hmm... so this can discard a tab not just from the off the record profile, but from a different profile entirely?
It's good we fix whether the extension can see that, but that still seems undesirable.
Would it be possible to instead tweak DiscardLeastImportantTab() to restrict it to a given browser context? That would potentially be a nicer change and also solve this bug. But that might be a larger change -- WDYT?
Yeah, that makes a lot of sense. Subtle things would have also been able to be inferred based on if there was an error vs a silent return with the other approach. Adding full profile filtering wasn't much more overhead.
DiscardEligibilityPolicy::DiscardReason discard_reason,
base::TimeDelta minimum_time_in_background) {Note: There's an argument for keeping this surface (and maybe ImmediatelyDiscardMultiplePages) uniform and also take an allowed_browser_context_ids set, but I thought it was better to only add it where we need it for now (DiscardAPage).
Related details
- Devlin Cronin
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim added 1 comment![]( "Open in Gerrit")
Sorry for the test failures folk, I forgot about having to use BrowserWindowInterface instead of Browser to avoid issues on the desktop-android bots. Should be good now.
Related details
- Devlin Cronin
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 6 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
6 comments
LGTM! Thank you, Tim!
if (include_incognito_information() && !profile->IsOffTheRecord()) {this check isn't necessary, I think -- include_incognito_information() is only true for spanning mode extensions
for (Profile* otr_profile : profile->GetAllOffTheRecordProfiles()) {we generally only use the primary OTR profile for extensions. This theoretically isn't *that* big of a deal (yet), but I thikn we should be consistent
// than silently discarding incognito tabs.is it non-deterministic how many tabs get discarded?
std::vector<base::DictValue> results;
for (int i = 0; i < 4; ++i) {
auto discard_no_id = base::MakeRefCounted<TabsDiscardFunction>();
discard_no_id->set_extension(extension.get());
if (api_test_utils::RunFunction(discard_no_id.get(), "[]", profile(),
api_test_utils::FunctionMode::kNone)) {
if (discard_no_id->GetResultListForTest() &&
!discard_no_id->GetResultListForTest()->empty()) {
results.push_back(utils::ToDict(
discard_no_id->GetResultListForTest()->front().Clone()));
}
} else {
EXPECT_EQ("Cannot find a tab to discard.", discard_no_id->GetError());
}
}
// We should have at least one result from the discardable normal tab.
EXPECT_GT(results.size(), 0u);
for (const auto& result : results) {
// Check that the returned tab does not have sensitive incognito
// information. It must be a normal tab. Since the extension has "tabs"
// permission, it should include url and title for the normal tab.
EXPECT_FALSE(api_test_utils::GetBoolean(result, "incognito"));
EXPECT_TRUE(result.contains("url"));
EXPECT_TRUE(result.contains("title"));
}optional: here and below, do you think any of these tests would be easier / cleaner with a TestExtensionDir and executing script in the SW with chrome.test?
(I'm fine either way)
// specific set of browser contexts.nit: put this below `urgent_protection_time` docs, since it's after in the param list
Related details
- Francois Pierre Doray
- Tim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim added 6 comments![]( "Open in Gerrit")
Swapping in joenotcharles as fdoray is OO till August.
joenotcharles@: Could you look at the changes in chrome/browser/performance_manager/policies/ and chrome/browser/resource_coordinator/?
if (include_incognito_information() && !profile->IsOffTheRecord()) {this check isn't necessary, I think -- include_incognito_information() is only true for spanning mode extensions
Done
for (Profile* otr_profile : profile->GetAllOffTheRecordProfiles()) {we generally only use the primary OTR profile for extensions. This theoretically isn't *that* big of a deal (yet), but I thikn we should be consistent
Done
is it non-deterministic how many tabs get discarded?
Not so much non-deterministic, but more just potentially racy and I worry might differ on different platforms (ExtensionTabsTest.DiscardWithoutId is already disabled for Mac and ExtensionTabsTest.DiscardedProperty is disabled for Linux). Although I think I was being overly cautious here, to the point that this loop doesn't really give any guarantee that the error was ever returned.
I've changed this to be much more explicit in how I _understand_ the discarding behavior to behave and added comments explaining the assumptions that led to the specific numbers being used in the checks here. I worry this may lead to flaking in some ways on different platform behavior, but I did a gtest_repeat=200 on linux using swarming and that at least didn't hit any problems.
std::vector<base::DictValue> results;
for (int i = 0; i < 4; ++i) {
auto discard_no_id = base::MakeRefCounted<TabsDiscardFunction>();
discard_no_id->set_extension(extension.get());
if (api_test_utils::RunFunction(discard_no_id.get(), "[]", profile(),
api_test_utils::FunctionMode::kNone)) {
if (discard_no_id->GetResultListForTest() &&
!discard_no_id->GetResultListForTest()->empty()) {
results.push_back(utils::ToDict(
discard_no_id->GetResultListForTest()->front().Clone()));
}
} else {
EXPECT_EQ("Cannot find a tab to discard.", discard_no_id->GetError());
}
}
// We should have at least one result from the discardable normal tab.
EXPECT_GT(results.size(), 0u);
for (const auto& result : results) {
// Check that the returned tab does not have sensitive incognito
// information. It must be a normal tab. Since the extension has "tabs"
// permission, it should include url and title for the normal tab.
EXPECT_FALSE(api_test_utils::GetBoolean(result, "incognito"));
EXPECT_TRUE(result.contains("url"));
EXPECT_TRUE(result.contains("title"));
}optional: here and below, do you think any of these tests would be easier / cleaner with a TestExtensionDir and executing script in the SW with chrome.test?
(I'm fine either way)
Going that route the TestExtensionDir boilerplate ends up a lot larger and I find it overall less clear with back and forth messaging. Directly creating the TabsDiscardFunction also matches the approach existing discard tests use around here.
nit: put this below `urgent_protection_time` docs, since it's after in the param list
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim added 2 comments![]( "Open in Gerrit")
Adding people back to the attention set after changing up the tabs-test a bit and fixing the issues with android bot again (incognito tab counts are different on android, because the incognito browser launches with 1 tab, as opposed to other platforms launching with 0).
Devlin: Could you take another look over the tabs tests as they had to change a fair bit to make them more deterministic (and deal with android discrepancies).
Joe: Could you take a look at the performance manager and resource coordinator code?
Thanks!
int normal_tab_count = GetTabListInterface()->GetTabCount();
// Note: To avoid having to deal with any platform differences between default
// tabs when creating a browser changing the total count, we just validate we
// have more than 0 tabs, so we know the loop below actually does something.
EXPECT_GT(normal_tab_count, 0);I made these dynamically calculated throughout after discovering the difference with incognito android getting an extra tab. An alternative would be to just hard code how many tabs we expect and add +1 on android incognito, but I felt that is giving this test too much knowledge of specific implementation details from elsewhere in the code. Doing it dynamically makes this resistant to any future changes that might happen there as well.
Related details
- Devlin Cronin
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted and added 4 comments![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
4 comments
LGTM with nits.
base::flat_set<base::UnguessableToken> allowed_tokens;Optional nit: current guidance is to use `absl::flat_hash_set` by default. (Not a big deal here because the set's always so small, though.)
DiscardEligibilityPolicy::DiscardReason discard_reason,
base::TimeDelta minimum_time_in_background) {Note: There's an argument for keeping this surface (and maybe ImmediatelyDiscardMultiplePages) uniform and also take an allowed_browser_context_ids set, but I thought it was better to only add it where we need it for now (DiscardAPage).
Acknowledged
EXPECT_TRUE(IsTabDiscarded(Nit: this seems like it could be flaky, since it only attempts 1 tab discard it might randomly discard the incognito tab anyway. I suggest calling DiscardAPage twice, and validating that only the one expected page gets discarded, then call it again with nullopt and verify that the regular tab is discarded now.
Related details
- Devlin Cronin
- Tim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
int normal_tab_count = GetTabListInterface()->GetTabCount();
// Note: To avoid having to deal with any platform differences between default
// tabs when creating a browser changing the total count, we just validate we
// have more than 0 tabs, so we know the loop below actually does something.
EXPECT_GT(normal_tab_count, 0);I made these dynamically calculated throughout after discovering the difference with incognito android getting an extra tab. An alternative would be to just hard code how many tabs we expect and add +1 on android incognito, but I felt that is giving this test too much knowledge of specific implementation details from elsewhere in the code. Doing it dynamically makes this resistant to any future changes that might happen there as well.
- Tim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tim
| Commit-Queue | +2 |
3 comments
Optional nit: current guidance is to use `absl::flat_hash_set` by default. (Not a big deal here because the set's always so small, though.)
Done
Nit: this seems like it could be flaky, since it only attempts 1 tab discard it might randomly discard the incognito tab anyway. I suggest calling DiscardAPage twice, and validating that only the one expected page gets discarded, then call it again with nullopt and verify that the regular tab is discarded now.
Done, although I went with three calls just to be thorough. I also added several comments laying out the reasoning as to exactly _why_ we don't expect certain tabs to be discarded.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[Extensions] Filter profiles for tabs.discard appropriately
When a tab ID is not passed to tabs.discard we leave the choice of which
tab to discard up to resource_coordinator::DiscardLeastImportantTab.
This CL adds the ability to pass a set of allowed_browser_context_ids to
additionally limit which browser contexts the resource coordinator can
discard from and passes the appropriate IDs for these based on the
extension incognito settings.
Also adds associated tests to cover these cases.
- M chrome/browser/extensions/api/tabs/tabs_api.cc
- M chrome/browser/extensions/api/tabs/tabs_test.cc
- M chrome/browser/performance_manager/policies/page_discarding_helper.cc
- M chrome/browser/performance_manager/policies/page_discarding_helper.h
- M chrome/browser/performance_manager/policies/page_discarding_helper_browsertest.cc
- M chrome/browser/resource_coordinator/utils.cc
- M chrome/browser/resource_coordinator/utils.h
Code-Review: +1 by Joe Mason, +1 by Devlin Cronin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |