Closed Bug 1402254 Opened 2 years ago Closed Last year

Add client::type in nsIQuotaManagerService::clearStoragesFromPrincipal

Categories

(Core :: Storage: Quota Manager, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: baku, Unassigned)

References

Details

Attachments

(1 file)

This is something important to have for WebExtension browsingData.removeIndexedDB.
I discussed this issue with janv.

We can also introduce a new method called:  nsIQuotaManagerService::clearStorageFromPrincipal()
Attached patch idb_real_1.patchSplinter Review
I'm not sure about aQuotaManager->RemoveQuotaForOrigin(aPersistenceType, group, origin) when mClientType is not null.
Attachment #8911085 - Flags: review?(jvarga)
Comment on attachment 8911085 [details] [diff] [review]
idb_real_1.patch

Review of attachment 8911085 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/sanitize.js
@@ -312,5 @@
>                let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(item.origin);
>                let uri = principal.URI;
>                if (uri.scheme == "http" || uri.scheme == "https" || uri.scheme == "file") {
>                  promises.push(new Promise(r => {
> -                  let req = quotaManagerService.clearStoragesForPrincipal(principal, null, true);

Hm, I overlooked this in previous review, we don't have to pass "true" as last argument, that is for clearing an origin and all other origins which only differ in origin attributes. Here we requested a list and we clear them one by one, so we don't need to use the "prefix" matching method.
> Hm, I overlooked this in previous review, we don't have to pass "true" as
> last argument, that is for clearing an origin and all other origins which
> only differ in origin attributes. Here we requested a list and we clear them
> one by one, so we don't need to use the "prefix" matching method.

Follow up? Separate bug.
(In reply to Andrea Marchesini [:baku] from comment #3)
> Follow up? Separate bug.

It's not a big deal, technically stuff should work as expected, it's just a neglible performance problem, you can file a new bug for that since this bug is about something else.
See bug 1402270.
Comment on attachment 8911085 [details] [diff] [review]
idb_real_1.patch

Review of attachment 8911085 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-browsingData.js
@@ +92,5 @@
>          let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(item.origin);
>          let uri = principal.URI;
>          if (uri.scheme == "http" || uri.scheme == "https" || uri.scheme == "file") {
>            promises.push(new Promise(r => {
> +            let req = quotaManagerService.clearStoragesForPrincipal(principal, null, null, true);

here you want to pass "idb" or you will do that once you have clearCache(), clearAsmJSCache ?

::: dom/quota/ActorsParent.cpp
@@ +918,5 @@
>    RefPtr<DirectoryLock> mDirectoryLock;
>  
>  protected:
>    Nullable<PersistenceType> mPersistenceType;
> +  Nullable<Client::Type> mClientType;

Here and elsewhere I would try to follow the order on disk:
profile/storage/$persistenceType/$origin/$clientType

@@ +7536,5 @@
>      if (NS_FAILED(rv)) {
>        NS_WARNING("Failed to remove directory, giving up!");
>      }
>  
> +    if (mClientType.IsNull() && aPersistenceType != PERSISTENCE_TYPE_PERSISTENT) {

This is a bit trickier, when we remove files for entire origin we can just remove quota internal objects for that all together too. But if we only delete a subset, we need to calculate how much we deleted and update quota objects accordingly. There's a method for that DecreaseUsageForOrigin(), but we can't just delete client's sub directory, we have to enumerate all the files and get file sizes.
There's one more complication here. Not all files are quota tracked, it's up to client to know what is tracked, so we probably need a new method in the Client interface and use that for clearing data. It will be client's responsibility to update quota.
It can also happen that after a client deleted its data, there's nothing else stored in the origin directory, right now I don't know if we want to handle that somehow, I'll thing about it...

::: dom/quota/Client.h
@@ +166,5 @@
> +  return NS_SUCCEEDED(Client::TypeFromText(NS_ConvertUTF8toUTF16(aText), type));
> +}
> +
> +inline nsresult
> +NullableClientTypeFromText(const nsACString& aText,

If we want to follow the style of the Client class, we should define this method in the Client class, not as a standalone method.

::: dom/quota/PQuota.ipdl
@@ +48,5 @@
>  {
>    PrincipalInfo principalInfo;
>    PersistenceType persistenceType;
>    bool persistenceTypeIsExplicit;
> +  nsCString clientType;

Type clientType
bool clientTypeIsExplicit

for that you need:
using mozilla::dom::quota::Client::Type
  from "mozilla/dom/quota/Client.h";

somewhere above

::: dom/quota/QuotaManagerService.cpp
@@ +710,5 @@
>      params.persistenceType() = persistenceType.Value();
>      params.persistenceTypeIsExplicit() = true;
>    }
>  
> +  if (!IsValidClientTypeFromText(aClientType)) {

I think the extra method IsValidClientTypeFromText is not needed, you can just call
NullableClientTypeFromText, check result and then use it in IPDL structs. Otherwise, you have to parse the string again in the parent process.
SerializationHelpers.h will need to be updated to support Client::Type
Attachment #8911085 - Flags: review?(jvarga) → feedback+
Priority: -- → P3
Blocks: 1286798
Priority: P3 → P2
Baku: is this bug still on your radar? If not, I'll put in the backlog.
Flags: needinfo?(amarchesini)
Actually, I had to fix this as part of next gen storage. So this will be fixed once that lands.
Assignee: amarchesini → jvarga
Flags: needinfo?(amarchesini)
Assignee: jvarga → nobody
Fixed by bug 1286798.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.