Closed Bug 980447 Opened 10 years ago Closed 10 years ago

add 'persistence type' argument to nsIQuotaManager.clearStoragesForURI

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(2 files, 2 obsolete files)

There is currently nsIQuotaManager.clear() and clearStoragesForURI(), but these functions both clear temporary and persistent.  A clearTemporaryStorage() (or perhaps clearTemporaryStorageForURI) would allow us to write better tests in, e.g., bug 965970.
We might want to add just a new argument, since I'm adding clearStoragesForPrincipal() and other equivalents elsewhere.
Ahh, good idea!
Hi Jan.  Is there any way we could get a patch for this in the near-term?  It would really help with testing bug 965970.
Actually, this is pretty simple, I'll whip up a patch.
Ok, I'll review.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8404179 - Flags: review?(Jan.Varga)
Comment on attachment 8404179 [details] [diff] [review]
improve-clear-storages

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

Thanks for helping with this!
Some comments ...

::: dom/quota/PersistenceType.h
@@ +56,5 @@
>  }
>  
> +inline Nullable<PersistenceType>
> +NullablePersistenceTypeFromText(const nsACString& aText)
> +{

Hm, I would rather not allow translation of unknown strings to null persistence type. What about putting this here:
if (aText.IsVoid()) {
  return Nullable<PersistenceType>();
}

@@ +65,5 @@
> +  if (aText.EqualsLiteral("temporary")) {
> +    return Nullable<PersistenceType>(PERSISTENCE_TYPE_TEMPORARY);
> +  }
> +
> +  return Nullable<PersistenceType>();

and throw an error here instead of returning nullable persistence type.

The method signature would need to be updated:
inline Nullable<PersistenceType>
NullablePersistenceTypeFromText(const nsACString& aText, mozilla::ErrorResult& aRv)

::: dom/quota/QuotaManager.cpp
@@ +2331,5 @@
>    matches.Find(mLiveStorages, pattern);
>  
>    for (uint32_t index = 0; index < matches.Length(); index++) {
> +    if (persistenceType.IsNull() ||
> +        matches[index]->Type() == persistenceType.Value()) {

Hm, this should be done in StorageMatcher.h in theory, but I don't think it's worth it. Some of this stuff is going away with IndexedDB and QuotaManager on PBackground anyway.

@@ +2662,5 @@
>    // Find the right SynchronizedOp.
>    SynchronizedOp* op;
>    if (aStorage) {
>      op = FindSynchronizedOp(aPattern,
>                              Nullable<PersistenceType>(aStorage->Type()),

Just use aPersistenceType here too.
I think it can be rewritten to:
op = FindSynchronizedOp(aPattern, aPersistenceType, aStorage ? aStorage->Id() : EmptyCString());

@@ +2849,5 @@
>  
>    nsAutoCString pattern;
>    GetOriginPatternStringMaybeIgnoreBrowser(aAppId, aBrowserOnly, pattern);
>  
> +  // Clear both temporary and persistence storages.

Nit: persistent ?

::: dom/quota/QuotaManager.h
@@ +198,3 @@
>    nsresult
>    AcquireExclusiveAccess(nsIOfflineStorage* aStorage,
>                           const nsACString& aOrigin,

I would add aPersistenceType here too.
OpenDatabaseHelper.cpp then needs to pass |mDatabase->Type()|

::: dom/quota/nsIQuotaManager.idl
@@ +51,5 @@
>    void
>    clearStoragesForURI(in nsIURI aURI,
>                        [optional] in unsigned long aAppId,
> +                      [optional] in boolean aInMozBrowserOnly,
> +                      [optional] in ACString aPersistenceClass);

hm, I would rather not introduce another terminology for "persistence", aPersistenceType should be ok, even when it's a string, I used it like this in TelemetryVFS.cpp
Attachment #8404179 - Flags: review?(Jan.Varga) → review+
Summary: add nsIQuotaManager.clearTemporageStorage() → add 'persistence type' argument to nsIQuotaManager.clearStoragesForURI
I missed something in the review, will comment soon.
Keywords: leave-open
+inline nsresult
+NullablePersistenceTypeFromText(const nsACString& aText,
+                                Nullable<PersistenceType> *aPersistenceType)

put * after the type:
Nullable<PersistenceType>* aPersistenceType

Also, the method shouldn't return NS_ERROR_UNEXPECTED, because I think we expect that an unknown string can be passed to clearStoragesForURI(). I would rather change it to NS_ERROR_FAILURE.

Then we need to translate the error to NS_ERROR_INVALID_ARG in clearStoragesForURI() implementation.
Ideally, right after |NS_ENSURE_ARG_POINTER(aURI);|

The bigger issue that I missed in the review is that AcquireExclusiveAccess() implementation in QuotaManager.cpp needs to be updated too.

This was done in ClearStoragesForURI():
   for (uint32_t index = 0; index < matches.Length(); index++) {
-    // We need to grab references to any live storages here to prevent them
-    // from dying while we invalidate them.
-    nsCOMPtr<nsIOfflineStorage> storage = matches[index];
-    storage->Invalidate();
+    if (persistenceType.IsNull() ||
+        matches[index]->Type() == persistenceType.Value()) {
+      // We need to grab references to any live storages here to prevent them
+      // from dying while we invalidate them.
+      nsCOMPtr<nsIOfflineStorage> storage = matches[index];
+      storage->Invalidate();
+    }
   }

Something similar needs to be done in AcquireExclusiveAccess() when live storages are collected.

I knew there was something missing in the original patch, so I didn't review it immediatelly.
I just didn't know what it was :)
Attached patch followup-fix (obsolete) — Splinter Review
Ah, good find.
Attachment #8406896 - Flags: review?(Jan.Varga)
Attached patch followup-fix (obsolete) — Splinter Review
Somehow previous upload failed...
Attachment #8406896 - Attachment is obsolete: true
Attachment #8406896 - Flags: review?(Jan.Varga)
Attachment #8406897 - Flags: review?(Jan.Varga)
Attached patch followup-fixSplinter Review
D'oh, qref
Attachment #8406897 - Attachment is obsolete: true
Attachment #8406897 - Flags: review?(Jan.Varga)
Attachment #8406898 - Flags: review?(Jan.Varga)
Comment on attachment 8406898 [details] [diff] [review]
followup-fix

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

::: dom/quota/QuotaManager.cpp
@@ +2721,2 @@
>      if (!matches.IsEmpty()) {
> +      for (uint32_t i = 0; i < Client::TYPE_MAX; i++) {

All this extra looping and checking looks terrible (not your fault).
Hopefully this stuff will be much simpler soon.
Attachment #8406898 - Flags: review?(Jan.Varga) → review+
(In reply to Jan Varga [:janv] from comment #15)
I actually started down the path of adding persistence type to StorageMatcher but hit some snags.  Glad to hear something better is coming.
https://hg.mozilla.org/mozilla-central/rev/79924a38cc00
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.