1401850 - Implement nsIQuotaManagerService::clearContentData

Status: RESOLVED INVALID

(Core :: Storage: Quota Manager, enhancement)

Description

[Andrea Marchesini [:baku]]

This is needed for bug 1401542, bug 1047098 and bug 1333050.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: nsIQuotaManagerService::ClearContentData()

Comment 2

[Andrea Marchesini [:baku]]

Attachment: nsIQuotaManagerService::ClearContentData()

FinalizeOnOwningThreadReady must be called also when mOriginList is empty.
Note that I don't want to touch mOriginList in FinalizeRequired(). In case there is nothing to remove, I think it's better to do an extra couple of Dispatch().

Comment 3

[Jan Varga [:janv]]

Ok, this looks better, reviewing ...

Comment 4

[Jan Varga [:janv]]

During review I've got an idea how do implement this without touching C++ code which is already quite complex.
In the meantime a new method has been implemented nsIQuotaManagerService::GetUsage, this method returns an array with information about all origins that have stored data.
The array can be just filtered for http/https/file schemes and call clearStoragesForPrincipal for individual origins. That's it.

baku, could you try this ?

Sorry, that I didn't figure out this sooner.

Comment 5

[Jan Varga [:janv]]

Here's an xpcshell test that uses getUsage()
https://dxr.mozilla.org/mozilla-central/source/dom/quota/test/unit/test_getUsage.js

Comment 6

[Andrea Marchesini [:baku]]

What you are saying doesn't work because of the exclusive lock.
This is exactly what I was proposing initially exposing Clear(). See your comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1333050#c44

Comment 7

[Andrea Marchesini [:baku]]

Basically, if your comment there is right, any use of clearStoragesForPrincipal() in our codebase is wrong. Except for testing, of course.

Comment 8

[Jan Varga [:janv]]

But clear() covers everything and clearStoragesForPrincipal() covers just one origin.

clear() needs to get exclusive lock for everything which means all existing operations are aborted (not just ones for http/https/file). When you call getUsage() it gets a shared lock for everything, so nothing is aborted. Then individual clearStoragesForPrincipal() calls will request an exclusive lock for just one particular origin and abort operations only for that one origin.

It's like an exclusive lock for entire SQL table or a lock for just one row in a table.

Let me know if it's still not clear.

Comment 9

[Andrea Marchesini [:baku]]

So, what's the difference between what you are proposing and my first approach here: https://bugzilla.mozilla.org/show_bug.cgi?id=1333050#c44 ?

Comment 10

[Jan Varga [:janv]]

Your first approach creates a new class ClearContentOp.
Itts ctor calls:
  QuotaRequestBase(/* aExclusive */ true)
which then calls:
NormalOriginOperationBase(Nullable<PersistenceType>(),
                          OriginScope::FromNull(),
                          aExclusive)

these 3 arguments will be used when a lock for the operation is requested.
Nullable<PersistenceType>() means all persistence types
OriginScope::FromNull() means all origins
aExclusive is true

So, it's an exclusive lock for everything and aborts operations for all origins.

I'm proposing to use getUsage() which doesn't have clear anything and just collect all origins that have stored data. For that a shared lock is enough.