Implement a manager for centralized quota and storage handling

RESOLVED FIXED in mozilla22

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: janv, Assigned: janv)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla22
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 14 obsolete attachments)

63.56 KB, patch
Details | Diff | Splinter Review
52.00 KB, patch
janv
: review+
Details | Diff | Splinter Review
2.78 KB, patch
janv
: review+
Details | Diff | Splinter Review
14.82 KB, patch
Details | Diff | Splinter Review
222.62 KB, patch
Details | Diff | Splinter Review
91.82 KB, patch
Details | Diff | Splinter Review
128.27 KB, patch
Details | Diff | Splinter Review
292.85 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
3.81 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
This manager is another step to unified quota handling. It will also support permanent and shared (temporary) storages.
(Assignee)

Updated

5 years ago
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 636284 [details] [diff] [review]
backup (basic infrastructure)
(Assignee)

Comment 2

5 years ago
Jonas, we talked about specifying the storage type for .open() and .deleteDatabase() ...
We can also use something like this:

partial interface IDBFactory {
  attribute DOMString mozStorageType; // temporary or permanent
};

partial interface IDBDatabase {
  readonly attribute DOMString mozStorageType;
};

so .open() and .deleteDatabase() would check actual value of the attribute

there are still the other possibilities:
.deleteDatabase("myDb", { storageType: "permanent" }) or
.deleteDatabase({ name: "myDb", storageType: "permanent" })
(Assignee)

Comment 3

5 years ago
and
.open("myDb", { version: 5, storageType: "permanent" }) or
.open({ name: "myDb", version: 5, storageType: "permanent" })
(Assignee)

Updated

5 years ago
Depends on: 773182
Depends on: 807021, 600307
Since this is touching localStorage (and probably DOM storage in general) I want to ask how this is going to be designed (API and roughly the implementation).  Problem is that localStorage should make the quota limit check synchronously (i'm changing this btw in the new implementation).  

Then, what is a planed target milestone and what is the realistic one.
I think localStorage will be using two separate policies:

A) Just like today, it will should have a 5MB limit for how much data is stored. I.e.
   the total data-size of all keys and all values shouldn't exceed 5MB. This limit
   needs to be synchronously enforced so that we can throw exceptions when data is
   written that would exceed this limit.
B) We should count the filesize of the underlying sqlite database towards the
   storage quota tracked by the quota manager added in this bug.

I don't think there's a reason to *just* count the key+value datasizes towards the quota managed by the manager in this bug. Doing so would just add a lot of complexity. The size that the user actually cares about is the space lost filesystem size anyway, so that's what we should be tracking when it comes to the quota manager.

We should still have the 5MB limit in A), but only as a means of reducing the amount of data that people store in localStorage. Because localStorage is a synchronous API, we're forced to either block page loading or block the whole thread while we are loading the localStorage data for a domain. So we want that operation to be as fast as possible. So we should keep the 5MB limit for that reason.

Likewise, I don't think that we should make localStorage throw if we run run into the quota manager limit for an origin. Throwing in that situation would result in webpages seeing exceptions at more or less random times. The 5MB limit in the spec was originally created to prevent such randomness and make the API reliable for webdevelopers to use.

Instead I think we should allow setting the value to happen. When we asynchronously attempt to write the data to disk and realize that we're hitting the quota manager limit, we should instead simply stop writing the data to disk and just use the memory cache. I'm guessing that's how we'd behave if the asynchronous writing failed for other reasons too?

Potentially we could in that case delete the data for that origin once the user leaves the page, as to make sure that we don't remain in a state when half of the page state is saved and half is not. I don't have strong opinions on that.

Does this sound reasonable?
(In reply to Jonas Sicking (:sicking) from comment #5)
> I think localStorage will be using two separate policies:
> 
> A) Just like today, it will should have a 5MB limit for how much data is
> stored. I.e.
>    the total data-size of all keys and all values shouldn't exceed 5MB. This
> limit
>    needs to be synchronously enforced so that we can throw exceptions when
> data is
>    written that would exceed this limit.

Agree, my new code will enforce this (last thing to finish properly).

> B) We should count the filesize of the underlying sqlite database towards the
>    storage quota tracked by the quota manager added in this bug.

I'd agree with this, bug problem is that the file size may have a lot of unallocated space since fragmentation.  We should then from time to time do vacuum on the database.  Possible with a background thread, I think, could be triggered when user cleans personal data (cookies).

> Instead I think we should allow setting the value to happen. When we
> asynchronously attempt to write the data to disk and realize that we're
> hitting the quota manager limit, we should instead simply stop writing the
> data to disk and just use the memory cache. 

Memory cache still has to have it's limits.

> I'm guessing that's how we'd
> behave if the asynchronous writing failed for other reasons too?

Right now, we just internally warn and that's it, I think.  The patches in bug 807021 seems to have some fail/retry logic (that I don't agree with, btw).  My new code doesn't have anything like that right now, but it is very simple to introduce it ; however, since localStorage more and more becomes storage for just a cookie-like type of data, usually not critical, then I don't think we should bother that much.

Quota may jitter, since it is etld+1 shared, so when foo.example.com deletes something, bar.example.com will have more space suddenly.  We may enforce this semi-synchronously.  I plan something like if (dataDelta > originAvailable || dataDelta > etld1Available || dataDelta > globalAvailable) -> ERROR_QUOTA_REACHED; etld1Available is determined asynchronously in a way that the storage object can be used prior etld1Available value determination.  The window to determine this value however will be very small.

> 
> Potentially we could in that case delete the data for that origin once the
> user leaves the page, as to make sure that we don't remain in a state when
> half of the page state is saved and half is not. I don't have strong
> opinions on that.

I don't know.  I'd turn the behavior to just a session-only like behavior silently.  Or have a storageError event?
> Quota may jitter, since it is etld+1 shared, so when foo.example.com deletes
> something, bar.example.com will have more space suddenly.  We may enforce
> this semi-synchronously.  I plan something like if (dataDelta >
> originAvailable || dataDelta > etld1Available || dataDelta >
> globalAvailable) -> ERROR_QUOTA_REACHED; etld1Available is determined
> asynchronously in a way that the storage object can be used prior
> etld1Available value determination.  The window to determine this value
> however will be very small.

In the localStorage code, I'm not sure that we need originAvailable or globalAvailable parameters. Simply having a synchronous 5MB limit for each eTLD+1 seems enough. That's how it is right now, no?

Once we integrate with the quota manager we'll have to switch to having one sqlite database file per eTLD+1, or one sqlite database per origin. If we use one sqlite database per origin it likely makes sense to adjust things so that the 5MB limit applies per origin instead of per eTLD+1.

The quota manager code will enforce a limit per eTLD+1 as well as a global limit, for any data that is written to disk.
(Assignee)

Comment 8

5 years ago
I'm going to take a look at the patch in bug 600307, especially how it affects the current quota manager (bug 787804)

I think eTLD+1 could be supported even if databases were stored per origin. Anyway we would then use the size of sqlite file to count quota (not the calculated one).
I think it'd be totally fine to make localStorage apply the 5MB-data-limit on a per-origin basis rather than on a per-eTLD+1 basis if it makes things simpler.

It's something that we can decide once we migrate the localStorage code to using the quota manager I think.
(In reply to Jonas Sicking (:sicking) from comment #7)
> In the localStorage code, I'm not sure that we need originAvailable or
> globalAvailable parameters. Simply having a synchronous 5MB limit for each
> eTLD+1 seems enough. That's how it is right now, no?

It is how it is implemented now, how my new implementation does it, and how it is specified, yes.

> 
> Once we integrate with the quota manager we'll have to switch to having one
> sqlite database file per eTLD+1, or one sqlite database per origin. If we
> use one sqlite database per origin it likely makes sense to adjust things so
> that the 5MB limit applies per origin instead of per eTLD+1.
> 
> The quota manager code will enforce a limit per eTLD+1 as well as a global
> limit, for any data that is written to disk.

Yep, the tricky part is just to calculate eTLD+1 consumption in a way that it doesn't block and it doesn't make large time windows allowing consumers to go over the quota.  The new localStorage impl counts eTLD+1 asynchronously.  So it has to keep track per-origin as a safe check to not allow storing large data during the time eTLD+1 consumption calculation is still pending.

(In reply to Jan Varga [:janv] from comment #8)
> I'm going to take a look at the patch in bug 600307, especially how it affects 
> the current quota manager (bug 787804)

I quickly took a look at your patch and the QuotaInfo object seems to be something similar to my DOMStorageQuota object in the "addition1" incremental patch.

> I think eTLD+1 could be supported even if databases were stored per origin.
> Anyway we would then use the size of sqlite file to count quota (not the
> calculated one).

Agree.

(In reply to Jonas Sicking (:sicking) from comment #9)
> I think it'd be totally fine to make localStorage apply the 5MB-data-limit
> on a per-origin basis rather than on a per-eTLD+1 basis if it makes things
> simpler.

Much simpler, but is against the spec.
(In reply to Honza Bambas (:mayhemer) from comment #10)
> Much simpler, but is against the spec.

It's not actually. The spec says:

"User agents should guard against sites storing data under their origin's other affiliated sites, e.g. storing up to the limit in a1.example.com, a2.example.com, a3.example.com, etc, circumventing the main example.com storage limit."

First off, this is just a "should" level requirement. Not a "must" level one (it's quite explicit that "should" means that you don't have to do it).

Additionally, as long as we have a centralized quota manager which limits total filesize per eTLD+1 we are fulfilling the above rule.

In fact, the spec even says:

"A mostly arbitrary limit of five megabytes per origin is suggested. Implementation feedback is welcome and will be used to update this suggestion in the future."

With the word "origin" being a link to the definition of an origin.

So all in all I think it's quite clear that we can use a 5MB per origin limit. Especially if we apply a file-size-per-eTLD+1 limit.

See http://www.whatwg.org/specs/web-apps/current-work/multipage/webstorage.html#disk-space-0
(In reply to Jonas Sicking (:sicking) from comment #11)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> > Much simpler, but is against the spec.
> 
> It's not actually. The spec says:

Very nice interpretation :)  I can stick with it.
(Assignee)

Updated

5 years ago
Depends on: 787804
(In reply to Jonas Sicking (:sicking) from comment #5)
> Likewise, I don't think that we should make localStorage throw if we run run
> into the quota manager limit for an origin. Throwing in that situation would
> result in webpages seeing exceptions at more or less random times. The 5MB
> limit in the spec was originally created to prevent such randomness and make
> the API reliable for webdevelopers to use.
> 
> Instead I think we should allow setting the value to happen. When we
> asynchronously attempt to write the data to disk and realize that we're
> hitting the quota manager limit, we should instead simply stop writing the
> data to disk and just use the memory cache. I'm guessing that's how we'd
> behave if the asynchronous writing failed for other reasons too?
> 
> Potentially we could in that case delete the data for that origin once the
> user leaves the page, as to make sure that we don't remain in a state when
> half of the page state is saved and half is not. I don't have strong
> opinions on that.
> 
> Does this sound reasonable?

So if I understand correctly, docs.google.com could store 3MB and google.com could then store another 3MB and once the user closes their browser, some or all of that data gets thrown out? That can't be to spec, can it?
Flags: needinfo?(jonas)
Just to clarify, my objection is that this would create seemingly random data loss in LocalStorage
(In reply to Vladan Djeric (:vladan) from comment #13)
> So if I understand correctly, docs.google.com could store 3MB and google.com
> could then store another 3MB and once the user closes their browser, some or
> all of that data gets thrown out?

My proposal is that the 5MB limit applies per-origin and that we'd synchronously enforce this limit.

However the on-disk limit that is per-eTLD+1 follows the same limits that we will have for indexedDB, appcache and other storage APIs. I.e 20% of the total temp storage per eTLD+1. That limit will usually be much larger than 5MB.

So if docs.google.com stores 3MB and google.com store another 3MB we'd simply have a total of 6MB for the google.com eTLD+1. Which would likely be just fine.

Though there is a risk that if google.com also uses a bunch of IndexedDB data that it could reach above the limit and then get deleted, but that seems like pretty low risk.

There's also a risk that another website will use temporary storage and put us above the global limit which would cause us to delete any temporary data associated with google.com, including the 6MB of localStorage data.

> That can't be to spec, can it?

Acctually, the spec is quite clear that the implementation is allowed to delete data anytime it wants for resource constraints reasons. So we have a lot of freedom here.

The more relevant question is how do we balance the user needs for not having left-over stuff on the harddrive, with the users desire to not have websites randomly breaking.
Flags: needinfo?(jonas)
(Assignee)

Updated

5 years ago
Summary: Implement a manager for centralized quota handling → Implement a manager for centralized quota and storage handling
(Assignee)

Comment 16

5 years ago
Created attachment 697091 [details] [diff] [review]
Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnectionHelper and IDBDatabase
Attachment #636284 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #697091 - Attachment description: Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnection Helper and IDBDatabase → Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnectionHelper and IDBDatabase
(Assignee)

Comment 17

5 years ago
Created attachment 697309 [details] [diff] [review]
Part 2: s/database/storage
(Assignee)

Comment 18

5 years ago
Created attachment 697315 [details] [diff] [review]
Part 3: Remove unused member in TransactionThreadPool
(Assignee)

Comment 19

5 years ago
Created attachment 697316 [details] [diff] [review]
Part 4: Split AutoEnterWindow, OriginInfo and QuotaObject class into separate source files
(Assignee)

Updated

5 years ago
Attachment #697091 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #697309 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #697315 - Flags: review?(bent.mozilla)
(Assignee)

Comment 20

5 years ago
Comment on attachment 697316 [details] [diff] [review]
Part 4: Split AutoEnterWindow, OriginInfo and QuotaObject class into separate source files

I decided to put these objects into separated files because the next patch (not attached yet) will add a lot of stuff from IndexedDatabaseManager to QuotaManager. Temp storage patch will add a lot of new stuff too.
Attachment #697316 - Flags: review?(bent.mozilla)
(Assignee)

Comment 21

5 years ago
Created attachment 697984 [details] [diff] [review]
Part 5: Move entire storage management from IndexedDatabaseManager to QuotaManager, also split some classes into separate files
Attachment #697984 - Flags: review?(bent.mozilla)
(Assignee)

Comment 22

5 years ago
There's one more patch in my queue for this bug. It introduces a concept of a storage client. It's an abstract interface for quota manager clients. Each storage API must provide an implementation of this interface in order to participate in centralized quota and storage handling.
The patch removes QuotaManager's dependency on FileManager and TransactionThreadPool.
It's almost ready, will attach it in few days.

I have also "almost" finished patches for generic temporary storage handling and also IDB specific patch to support permanent and temporary storages at the same time. These will be reviewed in separate bugs.
(Assignee)

Comment 23

5 years ago
Created attachment 698663 [details] [diff] [review]
Part 6: Abstract directory initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and TransactionThreadPool

Almost complete patch. QuotaManager::AcquireExclusiveAccess() and QuotaManager::RunSynchronizedOp() need more love, especially to utilize the new ArrayCluster class and to call WaitForStoragesToComplete() for all clients, not only IDB.
(Assignee)

Updated

5 years ago
Attachment #698663 - Attachment description: Part 6: Abstract directory initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and Transa → Part 6: Abstract directory initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and TransactionThreadPool
(Assignee)

Comment 24

5 years ago
Created attachment 698665 [details] [diff] [review]
Rollup patch
(Assignee)

Comment 25

5 years ago
Created attachment 700232 [details] [diff] [review]
Part 6: Abstract directory initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and Transa
Attachment #698663 - Attachment is obsolete: true
Attachment #700232 - Flags: review?(bent.mozilla)
(Assignee)

Comment 26

5 years ago
Created attachment 700238 [details] [diff] [review]
Rollup patch
Attachment #698665 - Attachment is obsolete: true
Comment on attachment 697091 [details] [diff] [review]
Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnectionHelper and IDBDatabase

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

::: dom/indexedDB/IDBDatabase.h
@@ +53,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_NSIIDBDATABASE
> +
> +  // nsIFileStorage
> +  nsIAtom* StorageId() MOZ_OVERRIDE

This is a little bit dangerous and I should have seen this sooner when you added nsIFileStorage...

We basically have a bunch of methods now that have both a virtual and a non-virtual implementation. Since we don't have any subclasses at the moment it's easy to call either version. However, once we have a subclass that overrides one of these methods then suddenly the two methods with similar names will do very different things. Which is the correct one to call? It's not safe to have both.

One option maybe is to make nsIFileStorage and nsIOfflineStorage not be true interfaces and let them have (private) members. The you could have non-virtual inline functions for getters and such.

The other option is to just remove the inlined versions and always call the virtuals.

@@ +55,5 @@
> +
> +  // nsIFileStorage
> +  nsIAtom* StorageId() MOZ_OVERRIDE
> +  {
> +    return Id();

Virtual methods should always be defined in the cpp file (since they can't be inlined anyway).

::: dom/indexedDB/IDBFileHandle.cpp
@@ +66,5 @@
>  
>  already_AddRefed<nsISupports>
>  IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly)
>  {
> +  IDBDatabase* database = static_cast<IDBDatabase*>(mFileStorage.get());

This won't always be safe, right? And why do you need an IDBDatabase here? If you really need the origin then mFileStorage should either 1) be an nsIOfflineStorage, 2) be QI-able to nsIOfflineStorage, or 3) should be a IDBDatabase proper.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +38,5 @@
>  #include "nsThreadUtils.h"
>  #include "nsXPCOM.h"
>  #include "nsXPCOMPrivate.h"
>  
> +#include "IDBRequest.h"

Nit: this should be alphabetized below.

@@ +602,5 @@
>    }
>  
>    // Add this database to its origin array if it exists, create it otherwise.
> +  nsTArray<nsIOfflineStorage*>* array;
> +  if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) {

Grab the origin in a stack var so you don't call the virtual twice.

@@ +605,5 @@
> +  nsTArray<nsIOfflineStorage*>* array;
> +  if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) {
> +    nsAutoPtr<nsTArray<nsIOfflineStorage*> > newArray(
> +      new nsTArray<nsIOfflineStorage*>());
> +    mLiveDatabases.Put(aDatabase->StorageOrigin(), newArray);

Nit: Not your fault but this autoptr is unnecessary nowadays. Just:

  array = new nsTArray<nsIOfflineStorage*>();
  mLiveDatabases.Put(origin, newArray);

@@ +625,5 @@
>  
>    // Remove this database from its origin array, maybe remove the array if it
>    // is then empty.
> +  nsTArray<nsIOfflineStorage*>* array;
> +  if (mLiveDatabases.Get(aDatabase->StorageOrigin(), &array) &&

Stack var for origin again.
Attachment #697091 - Flags: review?(bent.mozilla)
Comment on attachment 697309 [details] [diff] [review]
Part 2: s/database/storage

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

My eyes glazed over a little while reviewing this one...
Attachment #697309 - Flags: review?(bent.mozilla) → review+
Comment on attachment 697315 [details] [diff] [review]
Part 3: Remove unused member in TransactionThreadPool

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

Nice!
Attachment #697315 - Flags: review?(bent.mozilla) → review+
Comment on attachment 697316 [details] [diff] [review]
Part 4: Split AutoEnterWindow, OriginInfo and QuotaObject class into separate source files

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

I see the benefit of splitting QuotaObject from QuotaManager, but the rest seems unnecessary.

::: dom/quota/AutoEnterWindow.h
@@ +14,5 @@
> +class nsPIDOMWindow;
> +
> +BEGIN_QUOTA_NAMESPACE
> +
> +class AutoEnterWindow

Hm, I don't understand why you're moving this out of QuotaManager.h since it can't compile on its own anyway.

::: dom/quota/OriginInfo.h
@@ +14,5 @@
> +BEGIN_QUOTA_NAMESPACE
> +
> +class QuotaObject;
> +
> +class OriginInfo

It looks to me like this could stay as part of QuotaObject.h since you always use them together. Why split them up?
Attachment #697316 - Flags: review?(bent.mozilla)
Comment on attachment 697984 [details] [diff] [review]
Part 5: Move entire storage management from IndexedDatabaseManager to QuotaManager, also split some classes into separate files

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

::: browser/base/content/pageinfo/permissions.js
@@ +11,4 @@
>  
>  var gPermURI;
>  var gPrefs;
> +var gUsageRunnable;

Nit: This should be something like 'gUsageCallback'

@@ +106,5 @@
>                       .getService(Components.interfaces.nsIObserverService);
>    os.removeObserver(permissionObserver, "perm-changed");
>  
> +  if (gUsageRunnable) {
> +    gUsageRunnable.cancel();

Nit: You should probably null this out now too.

@@ +173,5 @@
>      permissionManager.remove(gPermURI.host, "indexedDB-unlimited");
>    }
>    if (aPartId == "fullscreen" && permission == UNKNOWN) {
>      permissionManager.remove(gPermURI.host, "fullscreen");
> +  }

Nit: You don't want blame for this line.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +16,5 @@
>  #include "nsThreadUtils.h"
>  #include "nsDOMMediaStream.h"
>  #include "nsDirectoryServiceDefs.h"
>  #include "nsComponentManagerUtils.h"
> +#include "nsRefPtrHashtable.h"

Is this needed for anything?

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +26,3 @@
>  
>  USING_INDEXEDDB_NAMESPACE
> +using namespace mozilla;

What is this needed for? If it's just StaticRefPtr you should just use that.

@@ +103,3 @@
>      sIsMainProcess = XRE_GetProcessType() == GeckoProcessType_Default;
>  
> +    nsAutoPtr<IndexedDatabaseManager> instance(new IndexedDatabaseManager());

This isn't safe, you're calling methods when instance has a refcount of 0.

@@ +111,2 @@
>  
> +    ClearOnShutdown(&gInstance);

Wait... So what's closing down the transaction thread pool? That should stay inside IDB.

@@ +142,2 @@
>  
> +  mFileManagers.Init();

This should go in the constructor (since it's infallible)

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +29,2 @@
>  {
> +  friend class nsAutoPtr<IndexedDatabaseManager>;

This class should never be used with nsAutoPtr.

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1633,5 @@
>  #endif
>  
>    mState = eFiringEvents; // In case we fail somewhere along the line.
>  
> +  if (QuotaManager::IsShuttingDown()) {

It's weird that this got moved to QuotaManager... You call IndexedDatabaseManager::Get() below and I don't know how you can assert it if this function has been moved.

::: dom/ipc/TabParent.cpp
@@ +968,5 @@
>    // to read any database.  That's a security hole, but we don't ship a
>    // configuration which creates non browser-or-app children, so it's not a big
>    // deal.
>    if (!aASCIIOrigin.EqualsLiteral("chrome") && IsBrowserOrApp() &&
> +      !QuotaManager::TabContextMayAccessOrigin(*this, aASCIIOrigin)) {

I don't really like this change. The quota manager shouldn't have any say over whether a particular window is allowed to use IndexedDB. This should be moved back to IndexedDatabaseManager.

::: dom/quota/SynchronizedOp.h
@@ +20,5 @@
> +
> +// A struct that contains the information corresponding to a pending or
> +// running operation that requires synchronization (e.g. opening a db,
> +// clearing dbs for an origin, etc).
> +struct SynchronizedOp

I can't tell why you moved this outside of QuotaManager (it's not exported, seems to only be used in QuotaManager.cpp). I think you should probably move it back.

::: dom/quota/nsIQuotaManager.idl
@@ +22,5 @@
> +   * @param aCallback
> +   *        The callback that will be called when the usage is available.
> +   */
> +  [optional_argc]
> +  nsICancelableRunnable

This should just be nsICancelable or something... You don't want JS to call run() on it.

I realize nsICancelable takes an nsresult arg, so maybe we need something new.

::: js/xpconnect/src/dom_quickstubs.qsconf
@@ +294,5 @@
>      'nsIBoxObject.height',
>  
>      # FileReader
>      'nsIDOMFileReader.*',
> +

Nit: undo
Attachment #697984 - Flags: review?(bent.mozilla)
Comment on attachment 700232 [details] [diff] [review]
Part 6: Abstract directory initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and Transa

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

::: dom/file/FileService.h
@@ +47,5 @@
>    NotifyLockedFileCompleted(LockedFile* aLockedFile);
>  
> +  void
> +  WaitForStoragesToComplete(nsTArray<nsCOMPtr<nsIFileStorage> >& aStorages,
> +                            nsIRunnable* aCallback);

Add MOZ_OVERRIDE.

::: dom/indexedDB/Client.cpp
@@ +28,5 @@
> +  nsAString::size_type filenameLen = aFilename.Length();
> +  nsAString::size_type sqliteLen = sqlite.Length();
> +
> +  if (sqliteLen > filenameLen ||
> +      Substring(aFilename, filenameLen - sqliteLen, sqliteLen) != sqlite) {

Just use StringEndsWith()

::: dom/indexedDB/Client.h
@@ +12,5 @@
> +#include "mozilla/dom/quota/Client.h"
> +
> +BEGIN_INDEXEDDB_NAMESPACE
> +
> +class Client : public quota::Client

I think this should be fully qualified, 'mozilla::dom::quota::Client'

::: dom/indexedDB/FileManager.h
@@ +14,5 @@
>  
>  #include "mozilla/dom/quota/StoragePrivilege.h"
>  #include "nsDataHashtable.h"
>  
> +#define JOURNAL_DIRECTORY_NAME "journals"

Can this be moved to Client.cpp?

::: dom/indexedDB/IDBFileHandle.cpp
@@ +66,5 @@
>  
>  already_AddRefed<nsISupports>
>  IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly)
>  {
> +  IDBDatabase* database = IDBDatabase::FromStorage(mFileStorage);

Why do we need a concrete pointer here?

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1663,5 @@
> +  rv = dbDirectory->Exists(&exists);
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +  if (exists) {
> +#ifdef DEBUG

Reverse this so that the debug-only code comes second?

@@ +2433,5 @@
>    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>  
>    NS_ASSERTION(directory, "What?");
>  
> +  rv = directory->Append(NS_LITERAL_STRING("idb"));

"idb" should be #defined at the top of the file so that you aren't susceptible to typos.

::: dom/indexedDB/TransactionThreadPool.cpp
@@ +468,5 @@
>    NS_ASSERTION(aDatabase, "Null pointer!");
>  
>    // Get list of transactions for this database id
>    DatabaseTransactionInfo* dbTransactionInfo;
> +  if (!mTransactionsInProgress.Get(aDatabase->Id(),

Leave as StorageId(), here and below.

::: dom/quota/ArrayCluster.h
@@ +18,5 @@
> +{
> +public:
> +  ArrayClusterBase()
> +  {
> +    mArrays = new nsTArray<ValueType>[Length];

Why is this heap allocated? |nsAutoTArray<nsTArray<ValueType>, Length>| is probably what you should use right? In fact, if you just inherit nsAutoTArray you get a bunch of these methods you redefine below for free.

@@ +24,5 @@
> +
> +  nsTArray<ValueType>&
> +  ArrayAt(uint32_t aIndex) const
> +  {
> +    NS_ASSERTION(aIndex < Length, "Bad index!");

New code should use MOZ_ASSERT whenever possible (since it's actually fatal in debug builds).

@@ +101,5 @@
> +  nsAutoArrayPtr<nsTArray<ValueType> > mArrays;
> +};
> +
> +template <class ValueType>
> +class ArrayCluster : public ArrayClusterBase<ValueType, CLIENT_TYPE_MAX>

If you're not defining anything useful here you could just use a typedef. Or just not have a base class and add a default template argument for length.

::: dom/quota/Client.h
@@ +18,5 @@
> +
> +// An abstract interface for quota manager clients.
> +// Each storage API must provide an implementation of this interface in order
> +// to participate in centralized quota and storage handling.
> +class Client

This should probably be refcounted. You use nsAutoPtr to manage the lifetimes of these things but I imagine that we will have problems with this down the road.

@@ +21,5 @@
> +// to participate in centralized quota and storage handling.
> +class Client
> +{
> +public:
> +  virtual ~Client()

You want this to be protected

@@ +27,5 @@
> +
> +  virtual nsresult
> +  InitDirectory(nsIFile* aDirectory,
> +                const nsACString& aOrigin,
> +                UsageRunnable* aUsageRunnable) = 0;

I don't understand why we're initializing a directory here... Wouldn't it make more sense for the client interface to only talk about initializing an origin? Or all origins?

@@ +31,5 @@
> +                UsageRunnable* aUsageRunnable) = 0;
> +
> +  virtual nsresult
> +  GetUsageForDirectory(nsIFile* aDirectory,
> +                       UsageRunnable* aUsageRunnable) = 0;

Same here, this seems like it should be per origin, not directory.

::: dom/quota/ClientType.h
@@ +10,5 @@
> +#include "mozilla/dom/quota/QuotaCommon.h"
> +
> +BEGIN_QUOTA_NAMESPACE
> +
> +enum ClientType {

It would be really great if we could figure out how to make this go away. What are we missing in the interface methods of Client that makes this necessary?

::: dom/quota/Makefile.in
@@ +33,5 @@
>    $(NULL)
>  
>  EXPORTS_mozilla/dom/quota = \
>    AcquireListener.h \
> +  ArrayCluster.h \

Why does this need to be exported?

::: dom/quota/StorageMatcher.h
@@ +45,5 @@
> +
> +public:
> +  template <class T>
> +  void
> +  Find(const T& aHashtable,

You could nail this down a little by making the type nsBaseHashtable<T,U,V>... It's a little uglier but it's easier for callers to figure out what they're supposed to use.

@@ +116,5 @@
> +  MatchPattern(const nsACString& aKey,
> +               BaseType* aValue,
> +               void* aUserArg)
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Why does the thread matter here?

@@ +191,5 @@
> +};
> +
> +template <class ValueType>
> +class StorageMatcher :
> +  public MatcherBase<ArrayCluster<nsIOfflineStorage*>, ValueType>

typedef or remove base class and add default template args.

::: dom/quota/nsIOfflineStorage.h
@@ +8,5 @@
>  #define nsIOfflineStorage_h__
>  
>  #include "nsIFileStorage.h"
>  
> +#include "mozilla/dom/quota/ClientType.h"

Can you forward-declare this instead?
Attachment #700232 - Flags: review?(bent.mozilla)
(Assignee)

Comment 33

5 years ago
Created attachment 715216 [details] [diff] [review]
Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnection Helper and IDBDatabase
Attachment #697091 - Attachment is obsolete: true
Attachment #715216 - Flags: review?(bent.mozilla)
(Assignee)

Comment 34

5 years ago
Created attachment 715217 [details] [diff] [review]
Part 1 interdiff
(Assignee)

Comment 35

5 years ago
(In reply to ben turner [:bent] from comment #27)
> Comment on attachment 697091 [details] [diff] [review]
> Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnectionHelper
> and IDBDatabase
> 
> Review of attachment 697091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/IDBDatabase.h
> @@ +53,5 @@
> >    NS_DECL_ISUPPORTS_INHERITED
> >    NS_DECL_NSIIDBDATABASE
> > +
> > +  // nsIFileStorage
> > +  nsIAtom* StorageId() MOZ_OVERRIDE
> 
> This is a little bit dangerous and I should have seen this sooner when you
> added nsIFileStorage...
> 
> We basically have a bunch of methods now that have both a virtual and a
> non-virtual implementation. Since we don't have any subclasses at the moment
> it's easy to call either version. However, once we have a subclass that
> overrides one of these methods then suddenly the two methods with similar
> names will do very different things. Which is the correct one to call? It's
> not safe to have both.
> 
> One option maybe is to make nsIFileStorage and nsIOfflineStorage not be true
> interfaces and let them have (private) members. The you could have
> non-virtual inline functions for getters and such.
> 
> The other option is to just remove the inlined versions and always call the
> virtuals.

I chose this option. 

> 
> @@ +55,5 @@
> > +
> > +  // nsIFileStorage
> > +  nsIAtom* StorageId() MOZ_OVERRIDE
> > +  {
> > +    return Id();
> 
> Virtual methods should always be defined in the cpp file (since they can't
> be inlined anyway).

ok, fixed

> 
> ::: dom/indexedDB/IDBFileHandle.cpp
> @@ +66,5 @@
> >  
> >  already_AddRefed<nsISupports>
> >  IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly)
> >  {
> > +  IDBDatabase* database = static_cast<IDBDatabase*>(mFileStorage.get());
> 
> This won't always be safe, right? And why do you need an IDBDatabase here?
> If you really need the origin then mFileStorage should either 1) be an
> nsIOfflineStorage, 2) be QI-able to nsIOfflineStorage, or 3) should be a
> IDBDatabase proper.

ok, I went for a QI

> 
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +38,5 @@
> >  #include "nsThreadUtils.h"
> >  #include "nsXPCOM.h"
> >  #include "nsXPCOMPrivate.h"
> >  
> > +#include "IDBRequest.h"
> 
> Nit: this should be alphabetized below.

ok

> 
> @@ +602,5 @@
> >    }
> >  
> >    // Add this database to its origin array if it exists, create it otherwise.
> > +  nsTArray<nsIOfflineStorage*>* array;
> > +  if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) {
> 
> Grab the origin in a stack var so you don't call the virtual twice.

ok, fixed

> 
> @@ +605,5 @@
> > +  nsTArray<nsIOfflineStorage*>* array;
> > +  if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) {
> > +    nsAutoPtr<nsTArray<nsIOfflineStorage*> > newArray(
> > +      new nsTArray<nsIOfflineStorage*>());
> > +    mLiveDatabases.Put(aDatabase->StorageOrigin(), newArray);
> 
> Nit: Not your fault but this autoptr is unnecessary nowadays. Just:
> 
>   array = new nsTArray<nsIOfflineStorage*>();
>   mLiveDatabases.Put(origin, newArray);

I think I did that later in one of those patches.
Anyway, fixed.

> 
> @@ +625,5 @@
> >  
> >    // Remove this database from its origin array, maybe remove the array if it
> >    // is then empty.
> > +  nsTArray<nsIOfflineStorage*>* array;
> > +  if (mLiveDatabases.Get(aDatabase->StorageOrigin(), &array) &&
> 
> Stack var for origin again.

ok
(Assignee)

Comment 36

5 years ago
Created attachment 715325 [details] [diff] [review]
Part 2: s/database/storage

rebased patch
Attachment #697309 - Attachment is obsolete: true
Attachment #715325 - Flags: review+
(Assignee)

Comment 37

5 years ago
Created attachment 715326 [details] [diff] [review]
Part 3: Remove unused member in TransactionThreadPool

rebased patch
Attachment #697315 - Attachment is obsolete: true
Attachment #715326 - Flags: review+
(Assignee)

Comment 38

5 years ago
Created attachment 715327 [details] [diff] [review]
Part 4: Split OriginInfo and QuotaObject class into separate source files
Attachment #697316 - Attachment is obsolete: true
Attachment #715327 - Flags: review?(bent.mozilla)
(Assignee)

Comment 39

5 years ago
(In reply to ben turner [:bent] from comment #30)
> Comment on attachment 697316 [details] [diff] [review]
> Part 4: Split AutoEnterWindow, OriginInfo and QuotaObject class into
> separate source files
> 
> Review of attachment 697316 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see the benefit of splitting QuotaObject from QuotaManager, but the rest
> seems unnecessary.
> 
> ::: dom/quota/AutoEnterWindow.h
> @@ +14,5 @@
> > +class nsPIDOMWindow;
> > +
> > +BEGIN_QUOTA_NAMESPACE
> > +
> > +class AutoEnterWindow
> 
> Hm, I don't understand why you're moving this out of QuotaManager.h since it
> can't compile on its own anyway.
> 
> ::: dom/quota/OriginInfo.h
> @@ +14,5 @@
> > +BEGIN_QUOTA_NAMESPACE
> > +
> > +class QuotaObject;
> > +
> > +class OriginInfo
> 
> It looks to me like this could stay as part of QuotaObject.h since you
> always use them together. Why split them up?

ok to both comments
(Assignee)

Comment 40

5 years ago
Created attachment 717244 [details] [diff] [review]
Part 5: Move entire storage management from IndexedDatabaseManager to QuotaManager, also split some classes into separate files
Attachment #697984 - Attachment is obsolete: true
Attachment #717244 - Flags: review?(bent.mozilla)
(Assignee)

Comment 41

5 years ago
(In reply to ben turner [:bent] from comment #31)
> Comment on attachment 697984 [details] [diff] [review]
> Part 5: Move entire storage management from IndexedDatabaseManager to
> QuotaManager, also split some classes into separate files
> 
> Review of attachment 697984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/pageinfo/permissions.js
> @@ +11,4 @@
> >  
> >  var gPermURI;
> >  var gPrefs;
> > +var gUsageRunnable;
> 
> Nit: This should be something like 'gUsageCallback'

ok, renamed to gUsageRequest

> 
> @@ +106,5 @@
> >                       .getService(Components.interfaces.nsIObserverService);
> >    os.removeObserver(permissionObserver, "perm-changed");
> >  
> > +  if (gUsageRunnable) {
> > +    gUsageRunnable.cancel();
> 
> Nit: You should probably null this out now too.

ok

> 
> @@ +173,5 @@
> >      permissionManager.remove(gPermURI.host, "indexedDB-unlimited");
> >    }
> >    if (aPartId == "fullscreen" && permission == UNKNOWN) {
> >      permissionManager.remove(gPermURI.host, "fullscreen");
> > +  }
> 
> Nit: You don't want blame for this line.

reverted

> 
> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +16,5 @@
> >  #include "nsThreadUtils.h"
> >  #include "nsDOMMediaStream.h"
> >  #include "nsDirectoryServiceDefs.h"
> >  #include "nsComponentManagerUtils.h"
> > +#include "nsRefPtrHashtable.h"
> 
> Is this needed for anything?

MediaEngineWebRTC.h has nsRefPtrHashtable members and the hashtable class is
indirectly included through nsDOMFile.h. nsDOMFile.h includes
IndexedDatabaseManager.h which doesn't need nsRefPtrHashtable anymore.

> 
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +26,3 @@
> >  
> >  USING_INDEXEDDB_NAMESPACE
> > +using namespace mozilla;
> 
> What is this needed for? If it's just StaticRefPtr you should just use that.

yeah, removed entirely (there's only one StaticRefPtr)

> 
> @@ +103,3 @@
> >      sIsMainProcess = XRE_GetProcessType() == GeckoProcessType_Default;
> >  
> > +    nsAutoPtr<IndexedDatabaseManager> instance(new IndexedDatabaseManager());
> 
> This isn't safe, you're calling methods when instance has a refcount of 0.

hm, I think this was needed to not call the Destroy() method (which sets the
closed flag) when something fails during initialization.
ok, so I added a new flag gInitialized for this

> 
> @@ +111,2 @@
> >  
> > +    ClearOnShutdown(&gInstance);
> 
> Wait... So what's closing down the transaction thread pool? That should stay
> inside IDB.

QuotaManager is closing down the transaction thread pool in this patch.
The following (final) patch moves it to IDB specific quota manager client, but
that's still being called from quota manager. I think the call should be made from
quota manager and impl should live in IDB, shouldn't it ?

> 
> @@ +142,2 @@
> >  
> > +  mFileManagers.Init();
> 
> This should go in the constructor (since it's infallible)

yeah, moved

> 
> ::: dom/indexedDB/IndexedDatabaseManager.h
> @@ +29,2 @@
> >  {
> > +  friend class nsAutoPtr<IndexedDatabaseManager>;
> 
> This class should never be used with nsAutoPtr.

ok, removed

> 
> ::: dom/indexedDB/OpenDatabaseHelper.cpp
> @@ +1633,5 @@
> >  #endif
> >  
> >    mState = eFiringEvents; // In case we fail somewhere along the line.
> >  
> > +  if (QuotaManager::IsShuttingDown()) {
> 
> It's weird that this got moved to QuotaManager... You call
> IndexedDatabaseManager::Get() below and I don't know how you can assert it
> if this function has been moved.

we need to discuss about this one ...

> 
> ::: dom/ipc/TabParent.cpp
> @@ +968,5 @@
> >    // to read any database.  That's a security hole, but we don't ship a
> >    // configuration which creates non browser-or-app children, so it's not a big
> >    // deal.
> >    if (!aASCIIOrigin.EqualsLiteral("chrome") && IsBrowserOrApp() &&
> > +      !QuotaManager::TabContextMayAccessOrigin(*this, aASCIIOrigin)) {
> 
> I don't really like this change. The quota manager shouldn't have any say
> over whether a particular window is allowed to use IndexedDB. This should be
> moved back to IndexedDatabaseManager.

ok, I moved it back, but some helper methods are still in QuotaManager

> 
> ::: dom/quota/SynchronizedOp.h
> @@ +20,5 @@
> > +
> > +// A struct that contains the information corresponding to a pending or
> > +// running operation that requires synchronization (e.g. opening a db,
> > +// clearing dbs for an origin, etc).
> > +struct SynchronizedOp
> 
> I can't tell why you moved this outside of QuotaManager (it's not exported,
> seems to only be used in QuotaManager.cpp). I think you should probably move
> it back.

ok, moved back

> 
> ::: dom/quota/nsIQuotaManager.idl
> @@ +22,5 @@
> > +   * @param aCallback
> > +   *        The callback that will be called when the usage is available.
> > +   */
> > +  [optional_argc]
> > +  nsICancelableRunnable
> 
> This should just be nsICancelable or something... You don't want JS to call
> run() on it.
> 
> I realize nsICancelable takes an nsresult arg, so maybe we need something
> new.

Well, I created a new interface nsIQuotaRequest with only one method cancel()
for now. We can extend it in future.

> 
> ::: js/xpconnect/src/dom_quickstubs.qsconf
> @@ +294,5 @@
> >      'nsIBoxObject.height',
> >  
> >      # FileReader
> >      'nsIDOMFileReader.*',
> > +
> 
> Nit: undo

reverted
(Assignee)

Comment 42

5 years ago
Created attachment 717246 [details] [diff] [review]
cummulative interdiff
Attachment #715217 - Attachment is obsolete: true
(Assignee)

Comment 43

5 years ago
Created attachment 717610 [details] [diff] [review]
Part 6: Abstract origin initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and TransactionThreadPool
Attachment #700232 - Attachment is obsolete: true
Attachment #717610 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #717610 - Attachment description: Part 6: Abstract origin initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and Transacti → Part 6: Abstract origin initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and TransactionThreadPool
(Assignee)

Comment 44

5 years ago
Created attachment 717612 [details] [diff] [review]
Rollup patch
Attachment #700238 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
Created attachment 717616 [details] [diff] [review]
cummulative interdiff
Attachment #717246 - Attachment is obsolete: true
(Assignee)

Comment 46

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=16e111e9210a
https://tbpl.mozilla.org/?tree=Try&rev=58d4f219b59e
(Assignee)

Comment 47

5 years ago
(In reply to ben turner [:bent] from comment #32)
> Comment on attachment 700232 [details] [diff] [review]
> Part 6: Abstract directory initialization, usage calculation and transaction
> service specific tasks into a new interface called Client. This removes
> QuotaManager's dependency on FileManager and Transa
> 
> Review of attachment 700232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/file/FileService.h
> @@ +47,5 @@
> >    NotifyLockedFileCompleted(LockedFile* aLockedFile);
> >  
> > +  void
> > +  WaitForStoragesToComplete(nsTArray<nsCOMPtr<nsIFileStorage> >& aStorages,
> > +                            nsIRunnable* aCallback);
> 
> Add MOZ_OVERRIDE.

not sure about this one

> 
> ::: dom/indexedDB/Client.cpp
> @@ +28,5 @@
> > +  nsAString::size_type filenameLen = aFilename.Length();
> > +  nsAString::size_type sqliteLen = sqlite.Length();
> > +
> > +  if (sqliteLen > filenameLen ||
> > +      Substring(aFilename, filenameLen - sqliteLen, sqliteLen) != sqlite) {
> 
> Just use StringEndsWith()

ok, fixed

> 
> ::: dom/indexedDB/Client.h
> @@ +12,5 @@
> > +#include "mozilla/dom/quota/Client.h"
> > +
> > +BEGIN_INDEXEDDB_NAMESPACE
> > +
> > +class Client : public quota::Client
> 
> I think this should be fully qualified, 'mozilla::dom::quota::Client'

ok

> 
> ::: dom/indexedDB/FileManager.h
> @@ +14,5 @@
> >  
> >  #include "mozilla/dom/quota/StoragePrivilege.h"
> >  #include "nsDataHashtable.h"
> >  
> > +#define JOURNAL_DIRECTORY_NAME "journals"
> 
> Can this be moved to Client.cpp?

hmm, I can move it to Client.h since FileManager.cpp needs it too

> 
> ::: dom/indexedDB/IDBFileHandle.cpp
> @@ +66,5 @@
> >  
> >  already_AddRefed<nsISupports>
> >  IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly)
> >  {
> > +  IDBDatabase* database = IDBDatabase::FromStorage(mFileStorage);
> 
> Why do we need a concrete pointer here?

we don't need it anymore, I removed this change

> 
> ::: dom/indexedDB/OpenDatabaseHelper.cpp
> @@ +1663,5 @@
> > +  rv = dbDirectory->Exists(&exists);
> > +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> > +
> > +  if (exists) {
> > +#ifdef DEBUG
> 
> Reverse this so that the debug-only code comes second?

ok, done

> 
> @@ +2433,5 @@
> >    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> >  
> >    NS_ASSERTION(directory, "What?");
> >  
> > +  rv = directory->Append(NS_LITERAL_STRING("idb"));
> 
> "idb" should be #defined at the top of the file so that you aren't
> susceptible to typos.

ok, done

> 
> ::: dom/indexedDB/TransactionThreadPool.cpp
> @@ +468,5 @@
> >    NS_ASSERTION(aDatabase, "Null pointer!");
> >  
> >    // Get list of transactions for this database id
> >    DatabaseTransactionInfo* dbTransactionInfo;
> > +  if (!mTransactionsInProgress.Get(aDatabase->Id(),
> 
> Leave as StorageId(), here and below.

I changed Id() to be the virtual method.

> 
> ::: dom/quota/ArrayCluster.h
> @@ +18,5 @@
> > +{
> > +public:
> > +  ArrayClusterBase()
> > +  {
> > +    mArrays = new nsTArray<ValueType>[Length];
> 
> Why is this heap allocated? |nsAutoTArray<nsTArray<ValueType>, Length>| is
> probably what you should use right? In fact, if you just inherit
> nsAutoTArray you get a bunch of these methods you redefine below for free.

ok, changed to |nsAutoTArray<nsTArray<ValueType>, Length>|
but I'm not sure how I would get those methods for free

> 
> @@ +24,5 @@
> > +
> > +  nsTArray<ValueType>&
> > +  ArrayAt(uint32_t aIndex) const
> > +  {
> > +    NS_ASSERTION(aIndex < Length, "Bad index!");
> 
> New code should use MOZ_ASSERT whenever possible (since it's actually fatal
> in debug builds).

ok, changed

> 
> @@ +101,5 @@
> > +  nsAutoArrayPtr<nsTArray<ValueType> > mArrays;
> > +};
> > +
> > +template <class ValueType>
> > +class ArrayCluster : public ArrayClusterBase<ValueType, CLIENT_TYPE_MAX>
> 
> If you're not defining anything useful here you could just use a typedef. Or
> just not have a base class and add a default template argument for length.

ok, I added a default template argument for length

> 
> ::: dom/quota/Client.h
> @@ +18,5 @@
> > +
> > +// An abstract interface for quota manager clients.
> > +// Each storage API must provide an implementation of this interface in order
> > +// to participate in centralized quota and storage handling.
> > +class Client
> 
> This should probably be refcounted. You use nsAutoPtr to manage the
> lifetimes of these things but I imagine that we will have problems with this
> down the road.

ok, changed

> 
> @@ +21,5 @@
> > +// to participate in centralized quota and storage handling.
> > +class Client
> > +{
> > +public:
> > +  virtual ~Client()
> 
> You want this to be protected

ok, done

> 
> @@ +27,5 @@
> > +
> > +  virtual nsresult
> > +  InitDirectory(nsIFile* aDirectory,
> > +                const nsACString& aOrigin,
> > +                UsageRunnable* aUsageRunnable) = 0;
> 
> I don't understand why we're initializing a directory here... Wouldn't it
> make more sense for the client interface to only talk about initializing an
> origin? Or all origins?

ok, now I have to construct the directory again in the client impl, but I think
it's a good compromise (clean API vs perf)

> 
> @@ +31,5 @@
> > +                UsageRunnable* aUsageRunnable) = 0;
> > +
> > +  virtual nsresult
> > +  GetUsageForDirectory(nsIFile* aDirectory,
> > +                       UsageRunnable* aUsageRunnable) = 0;
> 
> Same here, this seems like it should be per origin, not directory.

ok

> 
> ::: dom/quota/ClientType.h
> @@ +10,5 @@
> > +#include "mozilla/dom/quota/QuotaCommon.h"
> > +
> > +BEGIN_QUOTA_NAMESPACE
> > +
> > +enum ClientType {
> 
> It would be really great if we could figure out how to make this go away.
> What are we missing in the interface methods of Client that makes this
> necessary?

I replaced StorageClientType() with GetClient() in nsIOfflineStorage
and also added GetType() to Client.
So, ClientType has been removed completely.

> 
> ::: dom/quota/Makefile.in
> @@ +33,5 @@
> >    $(NULL)
> >  
> >  EXPORTS_mozilla/dom/quota = \
> >    AcquireListener.h \
> > +  ArrayCluster.h \
> 
> Why does this need to be exported?

QuotaManager.h includes it

> 
> ::: dom/quota/StorageMatcher.h
> @@ +45,5 @@
> > +
> > +public:
> > +  template <class T>
> > +  void
> > +  Find(const T& aHashtable,
> 
> You could nail this down a little by making the type
> nsBaseHashtable<T,U,V>... It's a little uglier but it's easier for callers
> to figure out what they're supposed to use.

ok, done

> 
> @@ +116,5 @@
> > +  MatchPattern(const nsACString& aKey,
> > +               BaseType* aValue,
> > +               void* aUserArg)
> > +  {
> > +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> 
> Why does the thread matter here?

removed (also in all similar methods)
changed NS_ASSERTION() to MOZ_ASSERT() too

> 
> @@ +191,5 @@
> > +};
> > +
> > +template <class ValueType>
> > +class StorageMatcher :
> > +  public MatcherBase<ArrayCluster<nsIOfflineStorage*>, ValueType>
> 
> typedef or remove base class and add default template args.

ok, I added a default template argument for BaseType

> 
> ::: dom/quota/nsIOfflineStorage.h
> @@ +8,5 @@
> >  #define nsIOfflineStorage_h__
> >  
> >  #include "nsIFileStorage.h"
> >  
> > +#include "mozilla/dom/quota/ClientType.h"
> 
> Can you forward-declare this instead?

this is not needed anymore
(Assignee)

Updated

4 years ago
Blocks: 847913
(Assignee)

Comment 48

4 years ago
Created attachment 721554 [details] [diff] [review]
Rollup patch

Up to date rollup patch.
Attachment #717612 - Attachment is obsolete: true
(Assignee)

Comment 49

4 years ago
I just removed "class mozIStorageServiceQuotaManagement" from FileManager.h (which should have been removed earlier) and incorporated the fix for bug 836943.
Comment on attachment 721554 [details] [diff] [review]
Rollup patch

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

::: content/base/src/nsDocument.cpp
@@ +7618,5 @@
>    }
>  
> +  // Check if we have running offline storage transactions
> +  quota::QuotaManager* quotaManager = quota::QuotaManager::Get();
> +  if (quotaManager && quotaManager->HasOpenTransactions(win)) {

It's not clear to me that this change is correct. IDB has some behavioral requirement here but do all other storage backends? It seems like this shouldn't be a QuotaManager function.

::: dom/base/nsGlobalWindow.cpp
@@ +1344,5 @@
>  
> +  // Close all offline storages for this window.
> +  quota::QuotaManager* quotaManager = quota::QuotaManager::Get();
> +  if (quotaManager) {
> +    quotaManager->AbortCloseStoragesForWindow(this);

Again, not sure about this. It makes sense for IDB but what about others (especially those without transactions and rollback)?

::: dom/file/nsIFileStorage.h
@@ +19,5 @@
>  {
>  public:
>    NS_DECLARE_STATIC_IID_ACCESSOR(NS_FILESTORAGE_IID)
>  
> +  NS_IMETHOD_(nsIAtom*)

Hm, one of these days I bet we'll run some automatic s/NS_IMETHOD_(foo)/virtual foo/ replacement. Why are you making this change?

::: dom/indexedDB/Client.h
@@ +21,5 @@
> +  typedef mozilla::dom::quota::UsageRunnable UsageRunnable;
> +
> +public:
> +  NS_IMETHOD_(nsrefcnt)
> +  AddRef();

These need MOZ_OVERRIDE

::: dom/indexedDB/IDBDatabase.cpp
@@ +205,2 @@
>  
> +  db->mQuotaClient = quotaManager->GetClient(Client::IDB);

Can this fail? If not you should assert it.

@@ +219,5 @@
> +IDBDatabase*
> +IDBDatabase::FromStorage(nsIOfflineStorage* aStorage)
> +{
> +  return aStorage->GetClient()->GetType() == Client::IDB ?
> +         static_cast<IDBDatabase*>(aStorage) : nullptr;

I'm don't understand this. Why are we going to all the trouble of creating abstract interfaces for everything if there are cases where we still need to know the exact concrete type?

::: dom/quota/QuotaManager.cpp
@@ +469,2 @@
>  
> +  mClients.AppendElements(Client::TYPE_MAX);

This looks fragile. What about calling SetCapacity here and then appending the ones you actually create.

@@ +946,5 @@
> +
> +already_AddRefed<mozilla::dom::quota::Client>
> +QuotaManager::GetClient(Client::Type aClientType)
> +{
> +  nsRefPtr<Client> client = mClients[aClientType];

Since this is a direct array access from an externally visible function you should probably use SafeElementAt() here.

::: dom/quota/nsIOfflineStorage.h
@@ +44,5 @@
> +  NS_IMETHOD_(bool)
> +  IsClosed() = 0;
> +
> +  NS_IMETHOD_(void)
> +  Invalidate() = 0;

It would be good to document the difference between Close and Invalidate here.
Attachment #715327 - Flags: review?(bent.mozilla)
Attachment #717244 - Flags: review?(bent.mozilla)
Attachment #717610 - Flags: review?(bent.mozilla)
Attachment #715216 - Flags: review?(bent.mozilla)
(Assignee)

Comment 51

4 years ago
Created attachment 724366 [details] [diff] [review]
Rollup patch
Attachment #721554 - Attachment is obsolete: true
(Assignee)

Comment 52

4 years ago
Created attachment 724368 [details] [diff] [review]
final fixes (interdiff)
(Assignee)

Comment 53

4 years ago
try looks good

https://tbpl.mozilla.org/?tree=Try&rev=41a67462740b
(Assignee)

Comment 54

4 years ago
(In reply to ben turner [:bent] from comment #50)
> Comment on attachment 721554 [details] [diff] [review]
> Rollup patch
> 
> Review of attachment 721554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsDocument.cpp
> @@ +7618,5 @@
> >    }
> >  
> > +  // Check if we have running offline storage transactions
> > +  quota::QuotaManager* quotaManager = quota::QuotaManager::Get();
> > +  if (quotaManager && quotaManager->HasOpenTransactions(win)) {
> 
> It's not clear to me that this change is correct. IDB has some behavioral
> requirement here but do all other storage backends? It seems like this
> shouldn't be a QuotaManager function.
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +1344,5 @@
> >  
> > +  // Close all offline storages for this window.
> > +  quota::QuotaManager* quotaManager = quota::QuotaManager::Get();
> > +  if (quotaManager) {
> > +    quotaManager->AbortCloseStoragesForWindow(this);
> 
> Again, not sure about this. It makes sense for IDB but what about others
> (especially those without transactions and rollback)?

talked to bent on irc, this can be fixed later in a followup bug or as part
of LocalStorage quota client implementation

> 
> ::: dom/file/nsIFileStorage.h
> @@ +19,5 @@
> >  {
> >  public:
> >    NS_DECLARE_STATIC_IID_ACCESSOR(NS_FILESTORAGE_IID)
> >  
> > +  NS_IMETHOD_(nsIAtom*)
> 
> Hm, one of these days I bet we'll run some automatic
> s/NS_IMETHOD_(foo)/virtual foo/ replacement. Why are you making this change?

the Close() method is declared in both interfaces, nsIOfflineStorage and
nsIIDBDatabase, and IDBDatabase implements both
So Close() needs to use NS_IMETHOD (I changed other methods too for consistency)

> 
> ::: dom/indexedDB/Client.h
> @@ +21,5 @@
> > +  typedef mozilla::dom::quota::UsageRunnable UsageRunnable;
> > +
> > +public:
> > +  NS_IMETHOD_(nsrefcnt)
> > +  AddRef();
> 
> These need MOZ_OVERRIDE

ok, added

> 
> ::: dom/indexedDB/IDBDatabase.cpp
> @@ +205,2 @@
> >  
> > +  db->mQuotaClient = quotaManager->GetClient(Client::IDB);
> 
> Can this fail? If not you should assert it.

ok, added an assertion

> 
> @@ +219,5 @@
> > +IDBDatabase*
> > +IDBDatabase::FromStorage(nsIOfflineStorage* aStorage)
> > +{
> > +  return aStorage->GetClient()->GetType() == Client::IDB ?
> > +         static_cast<IDBDatabase*>(aStorage) : nullptr;
> 
> I'm don't understand this. Why are we going to all the trouble of creating
> abstract interfaces for everything if there are cases where we still need to
> know the exact concrete type?

Ok, so I now understand why we should remove this method (the need for such
method means that the QuotaManager and Client API is not well designed.
However we can fix it later as part of LocalStorage quota client, etc.

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +469,2 @@
> >  
> > +  mClients.AppendElements(Client::TYPE_MAX);
> 
> This looks fragile. What about calling SetCapacity here and then appending
> the ones you actually create.

Well, the auto array should have the capacity set already. I added a new
assertion for that.
The rest is reworked as you suggested and it should be safer.

> 
> @@ +946,5 @@
> > +
> > +already_AddRefed<mozilla::dom::quota::Client>
> > +QuotaManager::GetClient(Client::Type aClientType)
> > +{
> > +  nsRefPtr<Client> client = mClients[aClientType];
> 
> Since this is a direct array access from an externally visible function you
> should probably use SafeElementAt() here.

ok, fixed
however, the compiler I'm using doesn't allow to pass random stuff to this
function, but that's probably not true for all C++ compilers

> 
> ::: dom/quota/nsIOfflineStorage.h
> @@ +44,5 @@
> > +  NS_IMETHOD_(bool)
> > +  IsClosed() = 0;
> > +
> > +  NS_IMETHOD_(void)
> > +  Invalidate() = 0;
> 
> It would be good to document the difference between Close and Invalidate
> here.

done
(Assignee)

Updated

4 years ago
Attachment #724366 - Flags: review?(bent.mozilla)
Comment on attachment 724366 [details] [diff] [review]
Rollup patch

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

I think this looks good! Thanks!
Attachment #724366 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 56

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7d3afd0ed794
(Assignee)

Comment 57

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d558e07caf4
Additional fix
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d5e48c5def
https://hg.mozilla.org/mozilla-central/rev/8d558e07caf4
https://hg.mozilla.org/mozilla-central/rev/b7d5e48c5def
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Comment 60

4 years ago
Is there any high-level overview, documentation or spec wiki page describing how the manager works and what it does as well as how it can be interacted with (from backend as well as frontend)?
(Assignee)

Comment 61

4 years ago
There's no such description yet, I will write one when the last bit lands, that's the temporary storage support, bug 785884.
If you need some info immediately, just ping me directly or maybe in the dev-webapi mailing list.

Comment 62

4 years ago
In the interim, please add dev-doc-needed to the keywords. 

(I sent you an email on the 29th March, I hope it didn't get stuck in the SPAM checker …)
Depends on: 855331
(Assignee)

Updated

4 years ago
Blocks: 785884
(Assignee)

Updated

4 years ago
No longer blocks: 847913
(Assignee)

Updated

4 years ago
No longer depends on: 773182
(Assignee)

Updated

4 years ago
Depends on: 820715

Updated

4 years ago
Blocks: 917362
(Assignee)

Comment 63

4 years ago
(In reply to Jan Varga [:janv] from comment #54)
> > ::: dom/base/nsGlobalWindow.cpp
> > @@ +1344,5 @@
> > >  
> > > +  // Close all offline storages for this window.
> > > +  quota::QuotaManager* quotaManager = quota::QuotaManager::Get();
> > > +  if (quotaManager) {
> > > +    quotaManager->AbortCloseStoragesForWindow(this);
> > 
> > Again, not sure about this. It makes sense for IDB but what about others
> > (especially those without transactions and rollback)?
> 
> talked to bent on irc, this can be fixed later in a followup bug or as part
> of LocalStorage quota client implementation

filed bug 942542 for this

Updated

4 years ago
Keywords: dev-doc-needed

Comment 64

3 years ago
Jan, why is this stuff quickstubbed?  Does it need to be?
Flags: needinfo?(Jan.Varga)
(Assignee)

Comment 65

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #64)
> Jan, why is this stuff quickstubbed?  Does it need to be?

Those few methods aren't called a lot, so it doesn't have to be quickstubbed. Feel free to remove them from quickstubs.
Flags: needinfo?(Jan.Varga)
You need to log in before you can comment on or make changes to this bug.