[Extensions] Add browser tests for extension web storage data retention [chromium/src : main]
Simeon Vincent (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simeon Vincent (Gerrit)
Simeon Vincent added 1 comment![]( "Open in Gerrit")
Tim mentioned that you were working on some adjacent Web Platform Tests work. I'd appreciate it if you could take a high level look at this CL to make sure it's in reasonable shape before I reach out to the owners of the systems under test here.
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simeon Vincent (Gerrit)
Simeon Vincent added 1 comment![]( "Open in Gerrit")
Devlin, please take a look at the overall structure and approach here. Once you're happy with the general shape of the new tests, I'll tag in reviewers for the related systems under test.
Related details
- Devlin Cronin
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 18 comments![]( "Open in Gerrit")
Thanks, Simeon! This is a great start!
void CreateLocalStorageForKey(Profile* profile, const blink::StorageKey& key);nit 1: put these in a namespace, e.g. browsing_data_test_utils
nit 2: add function comments describing what these do
// #include "testing/gtest/include/gtest/gtest.h" // Not required?clean up unused includes
auto it = std::ranges::find_if(
usage_infos, [&key](const storage::mojom::StorageUsageInfoPtr& info) {
return info->storage_key == key;
});
return it != usage_infos.end();nit: simpler to use std::ranges::any_of (though I know this was largely cut-and-paste)
// This file contains tests that verify how web storage (local storage,nit: prefer placing this lower -- maybe below the static assert, or maybe above the test fixture definition (around line 64)
req.onerror = () => { resolve() };should this throw an error? It seems like we shouldn't expect errors from IDB
// web origin handling.nit: function comments should be descriptive, rather than imperative. "Retrieves the storage..."
// Retrieve the storage key of the text extensionnit: comments should end in a period
static blink::StorageKey GetExtStorageKey(const Extension* extension) {nit: avoid uncommon abbreviations in the code -- prefer "Extension"
return blink::StorageKey::CreateFirstParty(url::Origin::Create(ext_url));nit: simpler: `extension->origin()`
return nullptr;nit: add something like `ADD_FAILURE() << "Failed to load extension."`
content::GetIOThreadTaskRunner({})->PostTask(
FROM_HERE,
base::BindOnce(
[](storage::QuotaManager* qm,
scoped_refptr<base::SequencedTaskRunner> reply_runner,
std::set<storage::BucketLocator>* result,
base::OnceClosure quit) {
qm->GetEvictionBuckets(
INT64_MAX,
base::BindPostTask(
std::move(reply_runner),
base::BindOnce(
[](std::set<storage::BucketLocator>* result,
base::OnceClosure quit,
const std::set<storage::BucketLocator>& buckets) {
*result = buckets;
std::move(quit).Run();
},
result, std::move(quit))));
},
base::Unretained(qm), std::move(ui_runner), &result,
run_loop.QuitClosure()));nit: let's pull these two lambdas into separate variables to help reduce some of the callback nesting here
// TODO(simeonv): Add packaged apps to the test. Seepackaged apps are very, very deprecated. We shouldn't be adding them anywhere
ChromeBrowsingData_DefaultOriginTypes) {for these, please add test comments briefly describing what they do. Your ones above the tests above are great.
EXPECT_FALSE(api_test_utils::RunFunctionAndReturnSingleResult(optional: I'd maybe lean towards using the browser data remover code directly instead of going through the extension API, unless you explicitly wanted to exercise "another extension calls chrome.browsingData.remove()" -- in which case, I'd suggest having a second extension directly, rather than anonymously invoking the function. But I don't feel terribly strongly.
usage_infos = GetLocalStorage(profile());
EXPECT_TRUE(UsageInfosHasStorageKey(usage_infos, ext_key));
EXPECT_FALSE(UsageInfosHasStorageKey(usage_infos, web_key));Food for thought: for all these tests, I would personally lean towards having them be more end-to-end-y, testing behavior from the extensions POV. In particular, I would lean towards something like:
- Set a variable in local storage in JS
- Maybe-clear
- Fetch variable from local storage in JS
- Expect variable present or not
If you wanted to go that route, BackgroundScriptExecutor is probably your friend.
That's a bit more clear as to the implication of this, and IMO is more robust than checking that there is a storage key (which is origin-based, and less obvious about the data actually stored in it).
WDYT?
for (const auto& entry : *model) {
url::Origin entry_origin =
BrowsingDataModel::GetOriginForDataKey(*entry.data_key);
EXPECT_NE(entry_origin, ext_origin)
<< "Extension origin unexpectedly found in BrowsingDataModel: "
<< entry_origin;
}code coverage indicates the body of this is never being reached -- so model is empty. Is that expected?
base::RunLoop run_loop;
profile()->GetDefaultStoragePartition()->ClearData(
content::StoragePartition::REMOVE_DATA_MASK_LOCAL_STORAGE, ext_key,
base::Time(), base::Time::Max(), run_loop.QuitClosure());
run_loop.Run();nit: might be more clear to pull this out into a helper function
Related details
- Justin Lulejian
- Simeon Vincent
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. |
Simeon Vincent (Gerrit)
Simeon Vincent added 18 comments![]( "Open in Gerrit")
This this patchset addresses most of the feedback on the CL. I'm going to tackle refactoring the [`browserData.remove` tests feedback](https://chromium-review.googlesource.com/c/chromium/src/+/7884253/comment/7c899c56_8156213a/) in a follow-up.
void CreateLocalStorageForKey(Profile* profile, const blink::StorageKey& key);nit 1: put these in a namespace, e.g. browsing_data_test_utils
nit 2: add function comments describing what these do
Acknowledged
// #include "testing/gtest/include/gtest/gtest.h" // Not required?Simeon Vincentclean up unused includes
I shouldn't have commented these out.
- `testing/gtest/include/gtest/gtest.h` is used by `ASSERT_TRUE` and `ASSERT_FALSE` statements
- `third_party/blink/public/common/storage_key/storage_key.h` is used by `blink::StorageKey`
I did another pass on the includes and added a couple of files for resources that were previously used but not directly included.
auto it = std::ranges::find_if(
usage_infos, [&key](const storage::mojom::StorageUsageInfoPtr& info) {
return info->storage_key == key;
});
return it != usage_infos.end();nit: simpler to use std::ranges::any_of (though I know this was largely cut-and-paste)
Acknowledged
// This file contains tests that verify how web storage (local storage,nit: prefer placing this lower -- maybe below the static assert, or maybe above the test fixture definition (around line 64)
Acknowledged. I opted to move it after the `static_assert` rather than just above the text fixture because I figured this was describing the contents of the file rather than that specific fixture.
should this throw an error? It seems like we shouldn't expect errors from IDB
Acknowledged
nit: function comments should be descriptive, rather than imperative. "Retrieves the storage..."
I've updated this comment and the comments on other methods in this class.
nit: comments should end in a period
Acknowledged
static blink::StorageKey GetExtStorageKey(const Extension* extension) {nit: avoid uncommon abbreviations in the code -- prefer "Extension"
Acknowledged. Renamed to `GetExtensionStorageKey`.
return blink::StorageKey::CreateFirstParty(url::Origin::Create(ext_url));Simeon Vincentnit: simpler: `extension->origin()`
Acknowledged
nit: add something like `ADD_FAILURE() << "Failed to load extension."`
Acknowledged
content::GetIOThreadTaskRunner({})->PostTask(
FROM_HERE,
base::BindOnce(
[](storage::QuotaManager* qm,
scoped_refptr<base::SequencedTaskRunner> reply_runner,
std::set<storage::BucketLocator>* result,
base::OnceClosure quit) {
qm->GetEvictionBuckets(
INT64_MAX,
base::BindPostTask(
std::move(reply_runner),
base::BindOnce(
[](std::set<storage::BucketLocator>* result,
base::OnceClosure quit,
const std::set<storage::BucketLocator>& buckets) {
*result = buckets;
std::move(quit).Run();
},
result, std::move(quit))));
},
base::Unretained(qm), std::move(ui_runner), &result,
run_loop.QuitClosure()));nit: let's pull these two lambdas into separate variables to help reduce some of the callback nesting here
Acknowledged
packaged apps are very, very deprecated. We shouldn't be adding them anywhere
Acknowledged. I've removed this TODO.
for these, please add test comments briefly describing what they do. Your ones above the tests above are great.
Acknowledged
EXPECT_FALSE(api_test_utils::RunFunctionAndReturnSingleResult(optional: I'd maybe lean towards using the browser data remover code directly instead of going through the extension API, unless you explicitly wanted to exercise "another extension calls chrome.browsingData.remove()" -- in which case, I'd suggest having a second extension directly, rather than anonymously invoking the function. But I don't feel terribly strongly.
Makes sense to me.
usage_infos = GetLocalStorage(profile());
EXPECT_TRUE(UsageInfosHasStorageKey(usage_infos, ext_key));
EXPECT_FALSE(UsageInfosHasStorageKey(usage_infos, web_key));Food for thought: for all these tests, I would personally lean towards having them be more end-to-end-y, testing behavior from the extensions POV. In particular, I would lean towards something like:
- Set a variable in local storage in JS
- Maybe-clear
- Fetch variable from local storage in JS
- Expect variable present or not
If you wanted to go that route, BackgroundScriptExecutor is probably your friend.
That's a bit more clear as to the implication of this, and IMO is more robust than checking that there is a storage key (which is origin-based, and less obvious about the data actually stored in it).
WDYT?
That makes sense. My main hestiation is that it's not clear how to implement this in a browsertest, since serviceworkers don't have access to Local Storage, which is currently being used in the test.
I'll take a shot at it in my next patchset.
for (const auto& entry : *model) {
url::Origin entry_origin =
BrowsingDataModel::GetOriginForDataKey(*entry.data_key);
EXPECT_NE(entry_origin, ext_origin)
<< "Extension origin unexpectedly found in BrowsingDataModel: "
<< entry_origin;
}code coverage indicates the body of this is never being reached -- so model is empty. Is that expected?
Yes, but in retrospect that's a bit odd. I've updated the test to also include a web origin to ensure that the model isn't empty.
base::RunLoop run_loop;
profile()->GetDefaultStoragePartition()->ClearData(
content::StoragePartition::REMOVE_DATA_MASK_LOCAL_STORAGE, ext_key,
base::Time(), base::Time::Max(), run_loop.QuitClosure());
run_loop.Run();nit: might be more clear to pull this out into a helper function
Acknowledged. This has been moved into a new "ClearStorageKeyFromDefaultStoragePartition" helper.
Related details
- Devlin Cronin
- Justin Lulejian
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. |
Simeon Vincent (Gerrit)
Simeon Vincent added 2 comments![]( "Open in Gerrit")
EXPECT_FALSE(api_test_utils::RunFunctionAndReturnSingleResult(Simeon Vincentoptional: I'd maybe lean towards using the browser data remover code directly instead of going through the extension API, unless you explicitly wanted to exercise "another extension calls chrome.browsingData.remove()" -- in which case, I'd suggest having a second extension directly, rather than anonymously invoking the function. But I don't feel terribly strongly.
Makes sense to me.
Addressed in patchset 7.
usage_infos = GetLocalStorage(profile());
EXPECT_TRUE(UsageInfosHasStorageKey(usage_infos, ext_key));
EXPECT_FALSE(UsageInfosHasStorageKey(usage_infos, web_key));Simeon VincentFood for thought: for all these tests, I would personally lean towards having them be more end-to-end-y, testing behavior from the extensions POV. In particular, I would lean towards something like:
- Set a variable in local storage in JS
- Maybe-clear
- Fetch variable from local storage in JS
- Expect variable present or not
If you wanted to go that route, BackgroundScriptExecutor is probably your friend.
That's a bit more clear as to the implication of this, and IMO is more robust than checking that there is a storage key (which is origin-based, and less obvious about the data actually stored in it).
WDYT?
That makes sense. My main hestiation is that it's not clear how to implement this in a browsertest, since serviceworkers don't have access to Local Storage, which is currently being used in the test.
I'll take a shot at it in my next patchset.
Addressed in patchset 7.
Related details
- Devlin Cronin
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 10 comments![]( "Open in Gerrit")
Hi Simeon! Thanks for sending this. Did an initial pass. Apologies this didn't show up in my review notifications for some reason.
// Initialize a minimal localStorage area for |key| in |profile|'s default```suggestion
// Initialize a minimal localStorage area for `key` in `profile`'s default
```
here and elsewhere.
area->Put({'k', 'e', 'y'}, {'v', 'a', 'l', 'u', 'e'}, std::nullopt,```suggestion
area->Put({'k', 'e', 'y'}, {'v', 'a', 'l', 'u', 'e'},/*<param>=*/std::nullopt,
```
constexpr char kBackgroundInitIdb[] = R"(
new Promise(resolve => {
const req = indexedDB.open('test-db', 1);
req.onsuccess = () => { req.result.close(); resolve(); };
req.onerror = () => { chrome.test.fail("IDB Error: " + err.toString()); };
}).then(() => chrome.test.sendMessage('idb-ready'));
)";Since only used in `LoadTestExtensionWithIdb` we can move this there?
void BrowsingDataRemoverCallRemove(const Extension* extension,
const std::string& options_json,
const std::string& data_types_json) {
const std::string& script =
"chrome.browsingData.remove(" + options_json + ", " + data_types_json +
", () => chrome.test.sendScriptResult('done'));";
base::Value value = BackgroundScriptExecutor::ExecuteScript(
profile(), extension->id(), script,
BackgroundScriptExecutor::ResultCapture::kSendScriptResult);
}A bit of a nit, but I don't see the 'done' being validated.
```suggestion
bool BrowsingDataRemoverCallRemove(const Extension* extension,
const std::string& options_json,
const std::string& data_types_json) {
const std::string& script =
"chrome.browsingData.remove(" + options_json + ", " + data_types_json +
", () => chrome.test.sendScriptResult('done'));";
base::Value value = BackgroundScriptExecutor::ExecuteScript(
profile(), extension->id(), script,
BackgroundScriptExecutor::ResultCapture::kSendScriptResult);
return value == "done";
}
```
Then usage:
```
bool data_removed = BrowsingDataRemoverCallRemove(...);
ASSERT_TRUE(data_removed);
```
R"(setTimeout(async () => {Is the wrapping `setTimeout` actually necessary?
// Retrieve a set of buckets that QuotaManagerImpl can delete when under```suggestion
// Retrieve a set of buckets that `QuotaManagerImpl` can delete when under
```
here and other places where comments refer to code symbols to be consistent with the rest of the comments.
storage::QuotaManager* qm =
profile()->GetDefaultStoragePartition()->GetQuotaManager();
std::set<storage::BucketLocator> result;
base::RunLoop run_loop;
scoped_refptr<base::SequencedTaskRunner> ui_runner =
base::SequencedTaskRunner::GetCurrentDefault();
auto on_got_eviction_buckets = base::BindOnce(
[](std::set<storage::BucketLocator>* result, base::OnceClosure quit,
const std::set<storage::BucketLocator>& buckets) {
*result = buckets;
std::move(quit).Run();
},
&result, run_loop.QuitClosure());
auto get_eviction_buckets = base::BindOnce(
[](storage::QuotaManager* qm,
scoped_refptr<base::SequencedTaskRunner> reply_runner,
base::OnceCallback<void(const std::set<storage::BucketLocator>&)>
on_got) {
qm->GetEvictionBuckets(
INT64_MAX,
base::BindPostTask(std::move(reply_runner), std::move(on_got)));
},
base::Unretained(qm), std::move(ui_runner),Is it possible to pass `qm` as a scoped_refptr?
```suggestion
scoped_refptr<storage::QuotaManager> qm =
base::WrapRefCounted(profile()->GetDefaultStoragePartition()->GetQuotaManager());
std::set<storage::BucketLocator> result;
base::RunLoop run_loop;
scoped_refptr<base::SequencedTaskRunner> ui_runner =
base::SequencedTaskRunner::GetCurrentDefault();
auto on_got_eviction_buckets = base::BindOnce(
[](std::set<storage::BucketLocator>* result, base::OnceClosure quit,
const std::set<storage::BucketLocator>& buckets) {
*result = buckets;
std::move(quit).Run();
},
&result, run_loop.QuitClosure());
auto get_eviction_buckets = base::BindOnce(
[](storage::QuotaManager* qm,
scoped_refptr<base::SequencedTaskRunner> reply_runner,
base::OnceCallback<void(const std::set<storage::BucketLocator>&)>
on_got) {
qm->GetEvictionBuckets(
INT64_MAX,
base::BindPostTask(std::move(reply_runner), std::move(on_got)));
},
qm, std::move(ui_runner),
```
run_loop.Run();wrap waits with `SCOPED_TRACE("<what is being waited for>")` so that if the test fails/flakes later the future reviewer will get more immediate context.
IN_PROC_BROWSER_TEST_F(ExtensionWebStorageDurabilityTest, GetLocalStorageData) {```suggestion
IN_PROC_BROWSER_TEST_F(ExtensionWebStorageDurabilityTest, StoragePartitionClearData_ExtensionWithUnlimitedStorage) {
```
Related details
- Devlin Cronin
- Simeon Vincent
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 added 8 comments![]( "Open in Gerrit")
Thanks, Simeon!
auto it = std::ranges::find_if(
usage_infos, [&key](const storage::mojom::StorageUsageInfoPtr& info) {
return info->storage_key == key;
});
return it != usage_infos.end();Simeon Vincentnit: simpler to use std::ranges::any_of (though I know this was largely cut-and-paste)
Acknowledged
General note: "Acknowledged" is usually used for "There was no action taken, but I read the comment". "Done" is used for "I changed the code as requested".
(So for this comment now, "Acknowledged" makes sense : ))
const std::string& script =
"chrome.browsingData.remove(" + options_json + ", " + data_types_json +
", () => chrome.test.sendScriptResult('done'));";nit1: prefer composing with base::StringPrintf() instead of +.
nit2: prefer promises?
```
static constexpr char kScriptTemplate =
"chrome.browsingData.remove(%s, %s).then(() => chrome.test.sendScriptResult('done'));";
std::string script = base::StringPrintf(kScriptTemplate, options_json.c_str(), data_types_json.c_str());
```
storage::QuotaManager* qm =avoid uncommon abbreviations. Let's just spell out quota_manager.
Not likely any need to, since this is all local and "synchronous" in a test-only file.
Instead, I'd just drop the Unretained()s. They're not necessary.
// calls `chrome.browsingData()` with default `originTypes`.chrome.browsingData.remove()?
auto usageInfo = utils::GetLocalStorageInfo(profile());usage_info
// Verify that the extension origin doesn't own any entries in the model.
for (const auto& entry : *model) {
url::Origin entry_origin =
BrowsingDataModel::GetOriginForDataKey(*entry.data_key);
EXPECT_NE(entry_origin, ext_origin)
<< "Extension origin unexpectedly found in BrowsingDataModel: "
<< entry_origin;
}given the above, we know there's only one entry here -- let's just flip the EXPECT_EQ to ASSERT_EQ and then access the only element directly instead of having a for-loop
Related details
- Simeon Vincent
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. |