Last Comment Bug 785884 - Implement support for temporary storage (aka shared pool)
: Implement support for temporary storage (aka shared pool)
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla26
Assigned To: Jan Varga [:janv]
:
Mentors:
Depends on: 1275496 763854 767944 773182 854323 878703 884936 886755 912948 924348
Blocks: 735415 742822 830072 847913 729320 916064 916072 916631
  Show dependency treegraph
 
Reported: 2012-08-27 08:09 PDT by Jan Varga [:janv]
Modified: 2016-05-25 19:51 PDT (History)
21 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
26+


Attachments
backup (114.51 KB, patch)
2012-10-15 06:29 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
backup2 (129.72 KB, patch)
2012-12-18 05:52 PST, Jan Varga [:janv]
no flags Details | Diff | Review
Part 2: Improve storage privilege handling, introduce a new privilege called Application (13.43 KB, patch)
2013-01-10 07:26 PST, Jan Varga [:janv]
no flags Details | Diff | Review
Part 3: Add support for indexedDB.open({ name: "foo", version: x, storage: "temporary/permanent" }) (78.84 KB, patch)
2013-01-10 07:28 PST, Jan Varga [:janv]
no flags Details | Diff | Review
Basic infractructure for eTLD+1 grouping (90.13 KB, patch)
2013-03-07 10:45 PST, Jan Varga [:janv]
no flags Details | Diff | Review
Basic infractructure for eTLD+1 grouping (99.67 KB, patch)
2013-03-08 04:10 PST, Jan Varga [:janv]
no flags Details | Diff | Review
patch v0.1 (321.75 KB, patch)
2013-06-16 15:50 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
patch v0.2 (313.98 KB, patch)
2013-07-02 04:56 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
interdiff (32.20 KB, patch)
2013-07-02 04:57 PDT, Jan Varga [:janv]
ehsan: review+
Details | Diff | Review
interdiff2 (12.04 KB, patch)
2013-07-02 18:04 PDT, Jan Varga [:janv]
ehsan: review+
Details | Diff | Review
patch v0.3 (315.71 KB, patch)
2013-07-02 18:08 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
patch v0.4 (340.68 KB, patch)
2013-08-12 15:45 PDT, Jan Varga [:janv]
bent.mozilla: review+
Details | Diff | Review
interdiff v0.3 - v0.4 (197.10 KB, patch)
2013-08-12 15:48 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
b2g fix (1.66 KB, patch)
2013-08-12 18:02 PDT, [:fabrice] Fabrice Desré
jvarga: review+
Details | Diff | Review
alternative b2g fix (680 bytes, patch)
2013-08-13 05:41 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
helgrind.log (130.10 KB, text/plain)
2013-08-15 05:35 PDT, Jan Varga [:janv]
no flags Details
valgrind-patch1 (11.74 KB, patch)
2013-08-15 05:37 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
valgrind-patch2 (7.03 KB, patch)
2013-08-15 05:38 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
valgrind-patch3 (12.92 KB, patch)
2013-08-15 05:38 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
patch v0.5 (338.80 KB, patch)
2013-09-09 10:55 PDT, Jan Varga [:janv]
bent.mozilla: review+
Details | Diff | Review
interdiff v0.4 - v0.5 (46.97 KB, patch)
2013-09-09 10:57 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
Add [EnforceRange] to IDBOpenDBOptions dictionary (2.79 KB, patch)
2013-09-10 00:36 PDT, Jan Varga [:janv]
bent.mozilla: review+
Details | Diff | Review

Description Jan Varga [:janv] 2012-08-27 08:09:17 PDT

    
Comment 1 Jan Varga [:janv] 2012-10-15 06:29:07 PDT
Created attachment 671406 [details] [diff] [review]
backup
Comment 2 Jan Varga [:janv] 2012-12-18 05:52:01 PST
Created attachment 693360 [details] [diff] [review]
backup2
Comment 3 Jan Varga [:janv] 2012-12-18 06:13:36 PST
So, we're going to add support for this syntax:
indexedDB.open({ name: "myDb", version: 1, persistenceType: "permanent|temporary" })
indexedDB.deleteDatabase({ name: "myDb", persistenceType: "permanent|temporary" })
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-12-18 08:10:50 PST
(In reply to Jan Varga [:janv] from comment #3)
> indexedDB.deleteDatabase({ name: "myDb", persistenceType:
> "permanent|temporary" })

Can we make that less verbose? s/persistenceType/storage/ maybe?
Comment 5 Jan Varga [:janv] 2012-12-18 09:21:26 PST
The word storage conflicts with some other stuff in the code, but I'm ok if we use it only in the dictionary (PersistenceType would be used internally in the implementation)
Comment 6 Jonas Sicking (:sicking) 2012-12-18 23:29:11 PST
"storage" as a name in the dictionary sounds ok with me. As does PersistenceType in the internal code.
Comment 7 Jan Varga [:janv] 2013-01-10 07:26:26 PST
Created attachment 700416 [details] [diff] [review]
Part 2: Improve storage privilege handling, introduce a new privilege called Application
Comment 8 Jan Varga [:janv] 2013-01-10 07:28:31 PST
Created attachment 700420 [details] [diff] [review]
Part 3: Add support for indexedDB.open({ name: "foo", version: x, storage: "temporary/permanent" })
Comment 9 Jan Varga [:janv] 2013-03-07 10:45:08 PST
Created attachment 722377 [details] [diff] [review]
Basic infractructure for eTLD+1 grouping

So I added GetJarPrefix() to nsIPrincipal. A group is then constructed as jarPrefix + baseDomain. An extended origin can be constructed similarly as jarPrefix + origin.

Jonas, should I mark the .extendedOrigin attribute as deprecated in the IDL ?
I think I converted all the callers to use the .jarPrefix, so we can remove the .extendedOrigin completely.
Comment 10 Jan Varga [:janv] 2013-03-07 11:00:47 PST
Jonas says ok, but there are also some JS callers that I need to fix.
Comment 11 Jan Varga [:janv] 2013-03-08 04:10:40 PST
Created attachment 722753 [details] [diff] [review]
Basic infractructure for eTLD+1 grouping
Comment 12 Jan Varga [:janv] 2013-06-16 15:50:18 PDT
Created attachment 763292 [details] [diff] [review]
patch v0.1
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2013-06-18 15:51:54 PDT
Comment on attachment 763292 [details] [diff] [review]
patch v0.1

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

I'm sure I've missed some stuff, especially in the quota code.  I should probably spend some time reading that code when you have the next version of the patch.

Note that some of the comments below are merely questions, they don't mean that you're doing anything wrong.  :-)

::: dom/indexedDB/IDBFactory.cpp
@@ +70,5 @@
>    ObjectStoreInfo* info;
>  };
>  
> +bool
> +GetVersionFromOptions(const mozilla::dom::IDBOpenDBOptions& aOptions,

Nit: please drop the namespace prefix, as you're using it above.

@@ +151,3 @@
>      if (NS_FAILED(rv)) {
>        // Not allowed.
>        *aFactory = nullptr;

Please move this to before the if block to make sure it also happens if the else block fails.

@@ +546,5 @@
>  
> +// static
> +PersistenceType
> +IDBFactory::GetPersistenceTypeFromOptions(
> +                                        const IDBOpenDBOptions& aOptions,

Nit: please move this up to the same line.

@@ +557,5 @@
> +      "Enum values should match.");
> +    MOZ_STATIC_ASSERT(
> +      static_cast<uint32_t>(mozilla::dom::PersistenceType::Temporary) ==
> +      static_cast<uint32_t>(PERSISTENCE_TYPE_TEMPORARY),
> +      "Enum values should match.");

It's not clear to me why you don't use the PersistanceType enum generated by the WebIDL compiler everywhere.  This seems unnecessary.

@@ +801,1 @@
>                               ErrorResult& aRv)

Can't this function be implemented in terms of the OpenForPrincipal function below?

@@ +881,1 @@
>                                 ErrorResult& aRv)

Same question here, why can't this be implemented in terms of open?

::: dom/indexedDB/test/unit/head.js
@@ +227,5 @@
>        throw new Error("Can't send subject to another process!");
>      }
>      return this.notifyObservers(subject, topic, data);
> +  },
> +  exactGC: function(win, callback) {

Please add a comment explaining why this is needed, and why you only gc twice.

::: dom/quota/PersistenceType.h
@@ +16,5 @@
> +  PERSISTENCE_TYPE_PERMANENT = 0,
> +  PERSISTENCE_TYPE_TEMPORARY,
> +
> +  // Only needed for IPC serialization helper, should never be used in code.
> +  PERSISTENCE_TYPE_INVALID

I'd prefer if you named the values using CamelCase, as we usually name preprocessor macros in all upper case.

::: dom/quota/QuotaCommon.h
@@ +26,5 @@
>  void
>  AssertIsOnIOThread();
> +
> +void
> +AssertCurrentThreadOwnsQuotaMutex();

This is not needed...

::: dom/quota/QuotaManager.cpp
@@ +84,5 @@
> +#define TEMP_STORAGE_LIMIT_RATIO   .2   //  20 %
> +#else
> +#define TEMP_STORAGE_LIMIT_MIN     50   //  50 MB
> +#define TEMP_STORAGE_LIMIT_MAX   1024   //   1 GB
> +#define TEMP_STORAGE_LIMIT_RATIO   .4   //  40 %

Please convert these to typed static const values.

@@ +274,5 @@
>    CallbackState mCallbackState;
>    bool mInMozBrowserOnly;
>  };
>  
> +class ResetOrClearRunnable MOZ_FINAL : public nsIRunnable,

You can make the code simpler by inheriting from nsRunnable here and elsewhere.

@@ +489,5 @@
>    aOrigin.ReplaceChar(kReplaceChars, '+');
>  }
>  
> +nsresult
> +EnsureDirectory(nsIFile* aDirectory, bool* aCreated)

Do you actually care about the error codes returned form this function?  If not, you can make it just returns a bool.

@@ +510,5 @@
> +    rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    *aCreated = true;
> +  }

This is prone to races, in case the directory is created between the call to Exists and Create.  You can just call Create() and set *aCreated based on whether it returns NS_ERROR_FILE_ALREADY_EXISTS.

@@ +582,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +CreateDirectoryMetadata(nsIFile* aDirectory, PRInt64 aTimestamp,

Nit: please use stdint types.

@@ +683,5 @@
> +    rv = idbDirectory->Append(idbDirectoryName);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    rv = idbDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
> +    if (NS_FAILED(rv)) {

You should check against NS_ERROR_FILE_ALREADY_EXISTS explicitly.  Create can fail for other reasons as well.

@@ +704,5 @@
> +      nsString leafName;
> +      rv = file->GetLeafName(leafName);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      if (!leafName.Equals(idbDirectoryName)) {

I think you should also check to make sure that |file| is a file, not a directory.

@@ +718,5 @@
> +  return NS_OK;
> +}
> +
> +// Returns ("smart") temp storage limit (in KB), given available disk space
> +// (also in KB).

Please document the logic of this function here.

@@ +980,5 @@
>    MutexAutoLock lock(mQuotaMutex);
>  
> +  GroupInfoPair* pair;
> +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> +    pair = new GroupInfoPair();

Who destroys these objects?

@@ +1161,5 @@
>    else {
>      fileSize = 0;
>    }
>  
> +  QuotaObject* quotaObject = nullptr;

Hmm, why don't you make this an nsRefPtr and get rid of |result| below?

@@ +1185,5 @@
>  
> +    originInfo->mQuotaObjects.Get(path, &quotaObject);
> +
> +    if (!quotaObject) {
> +      quotaObject = new QuotaObject(originInfo, path, fileSize);

Who destroys this object?

@@ +1861,5 @@
> +
> +#if 0
> +    *aDefaultPersistenceType =
> +      permission == nsIPermissionManager::ALLOW_ACTION ?
> +                    PERSISTENCE_TYPE_PERMANENT : PERSISTENCE_TYPE_TEMPORARY;

Why is this needed?

@@ +2257,5 @@
> +
> +  nsRefPtr<CollectOriginsHelper> helper =
> +    new CollectOriginsHelper(mQuotaMutex, aMinSizeToBeFreed);
> +
> +  // Unlock while calling out to XPCOM

Can you please add a comment explaining why this is needed?

@@ +2811,5 @@
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +
> +  bool exists;
> +  rv = directory->Exists(&exists);
> +  NS_ENSURE_SUCCESS_VOID(rv);

You shouldn't need to check for existence before removal.

@@ +3440,5 @@
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +
> +  bool exists;
> +  rv = directory->Exists(&exists);
> +  NS_ENSURE_SUCCESS_VOID(rv);

Same here.

::: dom/quota/QuotaManager.h
@@ +256,5 @@
> +  }
> +#else
> +  AssertCurrentThreadOwnsQuotaMutex()
> +  { }
> +#endif

You don't need to do this yourself.  AssertCurrentThreadOwns() is already a no-op in non-debug builds.

::: dom/quota/QuotaObject.cpp
@@ +154,5 @@
> +                                                 group, origin);
> +
> +#ifdef DEBUG
> +        originInfos[i] = nullptr;
> +        originInfo = originInfos[i];

You don't need to set originInfo to null since it's local to the loop scope, so you can't read its value in the next iteration.

::: dom/quota/QuotaObject.h
@@ +47,2 @@
>  
>    virtual ~QuotaObject()

It seems to me that the dtor doesn't need to be virtual, is that true?

::: dom/webidl/IDBFactory.webidl
@@ +24,5 @@
>  interface IDBFactory {
>    [Throws]
>    IDBOpenDBRequest
>    open(DOMString name,
> +       [EnforceRange] unsigned long long version);

Why are you making version non-optional?  Isn't it better to make the IDBOpenDBOperation argument in the overload below optional?

@@ +45,5 @@
>    [Throws, ChromeOnly]
>    IDBOpenDBRequest
>    openForPrincipal(Principal principal,
>                     DOMString name,
> +                   [EnforceRange] unsigned long long version);

Same here.

::: dom/webidl/PersistenceType.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +/* enum PersistenceType { "permanent", "temporary" }; */

Can you uncomment this and remove this enum from other webidl files?
Comment 14 Jan Varga [:janv] 2013-07-02 01:18:11 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #13)
> Comment on attachment 763292 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 763292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm sure I've missed some stuff, especially in the quota code.  I should
> probably spend some time reading that code when you have the next version of
> the patch.
> 
> Note that some of the comments below are merely questions, they don't mean
> that you're doing anything wrong.  :-)
> 
> ::: dom/indexedDB/IDBFactory.cpp
> @@ +70,5 @@
> >    ObjectStoreInfo* info;
> >  };
> >  
> > +bool
> > +GetVersionFromOptions(const mozilla::dom::IDBOpenDBOptions& aOptions,
> 
> Nit: please drop the namespace prefix, as you're using it above.

ok, fixed

> 
> @@ +151,3 @@
> >      if (NS_FAILED(rv)) {
> >        // Not allowed.
> >        *aFactory = nullptr;
> 
> Please move this to before the if block to make sure it also happens if the
> else block fails.

do you mean to after the if/else block ?

fixed

> 
> @@ +546,5 @@
> >  
> > +// static
> > +PersistenceType
> > +IDBFactory::GetPersistenceTypeFromOptions(
> > +                                        const IDBOpenDBOptions& aOptions,
> 
> Nit: please move this up to the same line.

I think Ben prefers the way it is now :)

> 
> @@ +557,5 @@
> > +      "Enum values should match.");
> > +    MOZ_STATIC_ASSERT(
> > +      static_cast<uint32_t>(mozilla::dom::PersistenceType::Temporary) ==
> > +      static_cast<uint32_t>(PERSISTENCE_TYPE_TEMPORARY),
> > +      "Enum values should match.");
> 
> It's not clear to me why you don't use the PersistanceType enum generated by
> the WebIDL compiler everywhere.  This seems unnecessary.

Yeah, I have tried to merge them, but the EnumSerializer needs an end mark
PERSISTENCE_TYPE_INVALID.
I could implement a separate serializer, not sure which one is better.

> 
> @@ +801,1 @@
> >                               ErrorResult& aRv)
> 
> Can't this function be implemented in terms of the OpenForPrincipal function
> below?

see below

> 
> @@ +881,1 @@
> >                                 ErrorResult& aRv)
> 
> Same question here, why can't this be implemented in terms of open?

oh, I just found out that the generated IDBOpenDBOptions is a struct, not a class
so the member variables (such as mVersion) are actually public

fixed

> 
> ::: dom/indexedDB/test/unit/head.js
> @@ +227,5 @@
> >        throw new Error("Can't send subject to another process!");
> >      }
> >      return this.notifyObservers(subject, topic, data);
> > +  },
> > +  exactGC: function(win, callback) {
> 
> Please add a comment explaining why this is needed, and why you only gc
> twice.

This is actually a function that we already have in real SpecialPowers
implementation.
Anyway, I added comments to both implementations

> 
> ::: dom/quota/PersistenceType.h
> @@ +16,5 @@
> > +  PERSISTENCE_TYPE_PERMANENT = 0,
> > +  PERSISTENCE_TYPE_TEMPORARY,
> > +
> > +  // Only needed for IPC serialization helper, should never be used in code.
> > +  PERSISTENCE_TYPE_INVALID
> 
> I'd prefer if you named the values using CamelCase, as we usually name
> preprocessor macros in all upper case.

hm, we have for example:
enum Mode
{
  READ_ONLY = 0,
  READ_WRITE,
  VERSION_CHANGE,

  // Only needed for IPC serialization helper, should never be used in code.
  MODE_INVALID
};
in IndexedDB implementation

but I think it makes sense to use CamelCase for PersistenceType since it is used
across modules

hm, on the other hand we have:
class Client
{
  ...
  enum Type {
    IDB = 0,
    //LS,
    //APPCACHE,
    TYPE_MAX
  };
  ...
};

which is exported too

> 
> ::: dom/quota/QuotaCommon.h
> @@ +26,5 @@
> >  void
> >  AssertIsOnIOThread();
> > +
> > +void
> > +AssertCurrentThreadOwnsQuotaMutex();
> 
> This is not needed...

Why ? I understand that |mQuotaMutex.AssertCurrentThreadOwns();| and
|quotaManager->AssertCurrentThreadOwnsQuotaMutex()| is noop in opt builds, but
there's also |QuotaManager* quotaManager = QuotaManager::Get()| which is useless
in optimized builds, no ?

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +84,5 @@
> > +#define TEMP_STORAGE_LIMIT_RATIO   .2   //  20 %
> > +#else
> > +#define TEMP_STORAGE_LIMIT_MIN     50   //  50 MB
> > +#define TEMP_STORAGE_LIMIT_MAX   1024   //   1 GB
> > +#define TEMP_STORAGE_LIMIT_RATIO   .4   //  40 %
> 
> Please convert these to typed static const values.

ok, done

> 
> @@ +274,5 @@
> >    CallbackState mCallbackState;
> >    bool mInMozBrowserOnly;
> >  };
> >  
> > +class ResetOrClearRunnable MOZ_FINAL : public nsIRunnable,
> 
> You can make the code simpler by inheriting from nsRunnable here and
> elsewhere.

ok, fixed all the runnables

> 
> @@ +489,5 @@
> >    aOrigin.ReplaceChar(kReplaceChars, '+');
> >  }
> >  
> > +nsresult
> > +EnsureDirectory(nsIFile* aDirectory, bool* aCreated)
> 
> Do you actually care about the error codes returned form this function?  If
> not, you can make it just returns a bool.

hm, at the moment the error bubbles to OpenDatabaseHelper::DoDatabaseWork()
where it is translated to NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
I agree that we could make it returns a bool from the current point of view, but
there's a grand plan to improve error reporting in quota and indexedDB module :)
So I would keep the nsresult here.

> 
> @@ +510,5 @@
> > +    rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    *aCreated = true;
> > +  }
> 
> This is prone to races, in case the directory is created between the call to
> Exists and Create.  You can just call Create() and set *aCreated based on
> whether it returns NS_ERROR_FILE_ALREADY_EXISTS.

Hm, if something else can create the directory then we are doomed anyway I think.
All the directory management can happen on the IO thread only. Transactions can
run on other background threads but they only read/write from/to .sqlite files
Well, actually with this patch, a transaction running on its thread can remove
entire origin directories when some space needs to be freed. But no one else can
create origin directories.

> 
> @@ +582,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +nsresult
> > +CreateDirectoryMetadata(nsIFile* aDirectory, PRInt64 aTimestamp,
> 
> Nit: please use stdint types.

fixed

> 
> @@ +683,5 @@
> > +    rv = idbDirectory->Append(idbDirectoryName);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    rv = idbDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
> > +    if (NS_FAILED(rv)) {
> 
> You should check against NS_ERROR_FILE_ALREADY_EXISTS explicitly.  Create
> can fail for other reasons as well.

fixed

> 
> @@ +704,5 @@
> > +      nsString leafName;
> > +      rv = file->GetLeafName(leafName);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      if (!leafName.Equals(idbDirectoryName)) {
> 
> I think you should also check to make sure that |file| is a file, not a
> directory.

so we used to have this structure for persistent storages:
main dir: profile/indexedDB
origin dir: profile/indexedDB/http+++localhost
db file: profile/indexedDB/http+++localhost/1511236768bal.sqlite
db dir for stored files: profile/indexedDB/http+++localhost/1511236768bal

the centralized quota and storage manager added one more subdir to it:
main dir: profile/indexedDB
origin dir: profile/indexedDB/http+++localhost
origin dir metadata file: profile/indexedDB/http+++localhost/.metadata
storage specific dir: profile/indexedDB/http+++localhost/idb
db file: profile/indexedDB/http+++localhost/idb/1511236768bal.sqlite
db dir for stored files: profile/indexedDB/http+++localhost/idb/1511236768bal

so if the metadata file doesn't exist we need to put all db files and db dirs
for stored files below a new subdir "idb"

> 
> @@ +718,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +// Returns ("smart") temp storage limit (in KB), given available disk space
> > +// (also in KB).
> 
> Please document the logic of this function here.

The documentation is actually already there, I moved it here

> 
> @@ +980,5 @@
> >    MutexAutoLock lock(mQuotaMutex);
> >  
> > +  GroupInfoPair* pair;
> > +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> > +    pair = new GroupInfoPair();
> 
> Who destroys these objects?

What do you mean? Everything in this method should be infallible.

> 
> @@ +1161,5 @@
> >    else {
> >      fileSize = 0;
> >    }
> >  
> > +  QuotaObject* quotaObject = nullptr;
> 
> Hmm, why don't you make this an nsRefPtr and get rid of |result| below?

I can't do that, QuotaObject implements a customized AddRef/Release() and they
need to acquire the same lock.

> 
> @@ +1185,5 @@
> >  
> > +    originInfo->mQuotaObjects.Get(path, &quotaObject);
> > +
> > +    if (!quotaObject) {
> > +      quotaObject = new QuotaObject(originInfo, path, fileSize);
> 
> Who destroys this object?

Again, we use infallible arrays/hashtables here.
The QuotaObject is special, the caller of GetQuotaObject() will be the owner.
So once the caller is done with work, QuotaObject will be released automatically.
However there can be multiple callers requesting the same QuotaObject. That's
why we keep weak references to QuotaObjects in OriginInfo.mQuotaObjects and
we have special AddRef/Release implementation.

> 
> @@ +1861,5 @@
> > +
> > +#if 0
> > +    *aDefaultPersistenceType =
> > +      permission == nsIPermissionManager::ALLOW_ACTION ?
> > +                    PERSISTENCE_TYPE_PERMANENT : PERSISTENCE_TYPE_TEMPORARY;
> 
> Why is this needed?

For now, I don't want to change the default storage type. We need to do a bit
more testing, advocating, etc. before we change the default storage type to
"temporary" 

> 
> @@ +2257,5 @@
> > +
> > +  nsRefPtr<CollectOriginsHelper> helper =
> > +    new CollectOriginsHelper(mQuotaMutex, aMinSizeToBeFreed);
> > +
> > +  // Unlock while calling out to XPCOM
> 
> Can you please add a comment explaining why this is needed?

Hm, it seems that nsThread needs to acquire its own lock (possible deadlock in 
theory) and it also calls an observer. The observer can do various stuff like IO,
so it's probably better to unlock before dispatching new runnables.

Should I say something like this in the comment?

> 
> @@ +2811,5 @@
> > +  NS_ENSURE_SUCCESS_VOID(rv);
> > +
> > +  bool exists;
> > +  rv = directory->Exists(&exists);
> > +  NS_ENSURE_SUCCESS_VOID(rv);
> 
> You shouldn't need to check for existence before removal.

Hm, I'm not sure about this one. For example rmdir() on unix can return an error
when the named directory doesn't exist.
Here's a  similar code we used to have in IDB (now in QuotaManager)
http://mxr.mozilla.org/comm-esr17/source/mozilla/dom/indexedDB/IndexedDatabaseManager.cpp#1459
I actually experimentally removed the Exists() check and I then got a fatal
assertion "Failed to remove directory!" when the temp storage test was running.
So I think the check must stay there.

> 
> @@ +3440,5 @@
> > +  NS_ENSURE_SUCCESS_VOID(rv);
> > +
> > +  bool exists;
> > +  rv = directory->Exists(&exists);
> > +  NS_ENSURE_SUCCESS_VOID(rv);
> 
> Same here.

see above

> 
> ::: dom/quota/QuotaManager.h
> @@ +256,5 @@
> > +  }
> > +#else
> > +  AssertCurrentThreadOwnsQuotaMutex()
> > +  { }
> > +#endif
> 
> You don't need to do this yourself.  AssertCurrentThreadOwns() is already a
> no-op in non-debug builds.

ok, fixed

> 
> ::: dom/quota/QuotaObject.cpp
> @@ +154,5 @@
> > +                                                 group, origin);
> > +
> > +#ifdef DEBUG
> > +        originInfos[i] = nullptr;
> > +        originInfo = originInfos[i];
> 
> You don't need to set originInfo to null since it's local to the loop scope,
> so you can't read its value in the next iteration.

ok, removed |originInfo = originInfos[i];|

> 
> ::: dom/quota/QuotaObject.h
> @@ +47,2 @@
> >  
> >    virtual ~QuotaObject()
> 
> It seems to me that the dtor doesn't need to be virtual, is that true?

yeah, true
fixed

> 
> ::: dom/webidl/IDBFactory.webidl
> @@ +24,5 @@
> >  interface IDBFactory {
> >    [Throws]
> >    IDBOpenDBRequest
> >    open(DOMString name,
> > +       [EnforceRange] unsigned long long version);
> 
> Why are you making version non-optional?  Isn't it better to make the
> IDBOpenDBOperation argument in the overload below optional?

That doesn't work, here's the error message:

WebIDL.WebIDLError: error: Dictionary argument or union argument containing a di
ctionary not followed by a required argument must be optional, IDBFactory.webidl
 line 31:24
       IDBOpenDBOptions options);
                        ^

> 
> @@ +45,5 @@
> >    [Throws, ChromeOnly]
> >    IDBOpenDBRequest
> >    openForPrincipal(Principal principal,
> >                     DOMString name,
> > +                   [EnforceRange] unsigned long long version);
> 
> Same here.

see above

> 
> ::: dom/webidl/PersistenceType.webidl
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > + */
> > +
> > +/* enum PersistenceType { "permanent", "temporary" }; */
> 
> Can you uncomment this and remove this enum from other webidl files?

ok, but I had to fix a code generator issue, bug 886755


I'll attach updated and rebased patch soon.
Comment 15 Jan Varga [:janv] 2013-07-02 04:56:06 PDT
Created attachment 770128 [details] [diff] [review]
patch v0.2
Comment 16 Jan Varga [:janv] 2013-07-02 04:57:13 PDT
Created attachment 770129 [details] [diff] [review]
interdiff
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-02 12:58:49 PDT
(In reply to Jan Varga [:janv] from comment #14)
> > @@ +546,5 @@
> > >  
> > > +// static
> > > +PersistenceType
> > > +IDBFactory::GetPersistenceTypeFromOptions(
> > > +                                        const IDBOpenDBOptions& aOptions,
> > 
> > Nit: please move this up to the same line.
> 
> I think Ben prefers the way it is now :)

Sigh :(  He's wrong, but ok!  ;-)

> > @@ +557,5 @@
> > > +      "Enum values should match.");
> > > +    MOZ_STATIC_ASSERT(
> > > +      static_cast<uint32_t>(mozilla::dom::PersistenceType::Temporary) ==
> > > +      static_cast<uint32_t>(PERSISTENCE_TYPE_TEMPORARY),
> > > +      "Enum values should match.");
> > 
> > It's not clear to me why you don't use the PersistanceType enum generated by
> > the WebIDL compiler everywhere.  This seems unnecessary.
> 
> Yeah, I have tried to merge them, but the EnumSerializer needs an end mark
> PERSISTENCE_TYPE_INVALID.
> I could implement a separate serializer, not sure which one is better.

Hmm, yeah, ok, maybe using the WebIDL generated enum is not worth it here.

> > ::: dom/quota/PersistenceType.h
> > @@ +16,5 @@
> > > +  PERSISTENCE_TYPE_PERMANENT = 0,
> > > +  PERSISTENCE_TYPE_TEMPORARY,
> > > +
> > > +  // Only needed for IPC serialization helper, should never be used in code.
> > > +  PERSISTENCE_TYPE_INVALID
> > 
> > I'd prefer if you named the values using CamelCase, as we usually name
> > preprocessor macros in all upper case.
> 
> hm, we have for example:
> enum Mode
> {
>   READ_ONLY = 0,
>   READ_WRITE,
>   VERSION_CHANGE,
> 
>   // Only needed for IPC serialization helper, should never be used in code.
>   MODE_INVALID
> };
> in IndexedDB implementation
> 
> but I think it makes sense to use CamelCase for PersistenceType since it is
> used
> across modules
> 
> hm, on the other hand we have:
> class Client
> {
>   ...
>   enum Type {
>     IDB = 0,
>     //LS,
>     //APPCACHE,
>     TYPE_MAX
>   };
>   ...
> };
> 
> which is exported too

Yeah, unfortunately we don't have a consistent coding style.  If this is the common style in this code, then that's fine.

> > ::: dom/quota/QuotaCommon.h
> > @@ +26,5 @@
> > >  void
> > >  AssertIsOnIOThread();
> > > +
> > > +void
> > > +AssertCurrentThreadOwnsQuotaMutex();
> > 
> > This is not needed...
> 
> Why ? I understand that |mQuotaMutex.AssertCurrentThreadOwns();| and
> |quotaManager->AssertCurrentThreadOwnsQuotaMutex()| is noop in opt builds,
> but
> there's also |QuotaManager* quotaManager = QuotaManager::Get()| which is
> useless
> in optimized builds, no ?

You can use DebugOnly if that creates a warning during compilation, but the optimizer should be able to get rid of that anyway.

> > @@ +489,5 @@
> > >    aOrigin.ReplaceChar(kReplaceChars, '+');
> > >  }
> > >  
> > > +nsresult
> > > +EnsureDirectory(nsIFile* aDirectory, bool* aCreated)
> > 
> > Do you actually care about the error codes returned form this function?  If
> > not, you can make it just returns a bool.
> 
> hm, at the moment the error bubbles to OpenDatabaseHelper::DoDatabaseWork()
> where it is translated to NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR
> I agree that we could make it returns a bool from the current point of view,
> but
> there's a grand plan to improve error reporting in quota and indexedDB
> module :)
> So I would keep the nsresult here.

Yes, makes sense.  Thanks for the explanation.

> > @@ +510,5 @@
> > > +    rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
> > > +    NS_ENSURE_SUCCESS(rv, rv);
> > > +
> > > +    *aCreated = true;
> > > +  }
> > 
> > This is prone to races, in case the directory is created between the call to
> > Exists and Create.  You can just call Create() and set *aCreated based on
> > whether it returns NS_ERROR_FILE_ALREADY_EXISTS.
> 
> Hm, if something else can create the directory then we are doomed anyway I
> think.
> All the directory management can happen on the IO thread only. Transactions
> can
> run on other background threads but they only read/write from/to .sqlite
> files
> Well, actually with this patch, a transaction running on its thread can
> remove
> entire origin directories when some space needs to be freed. But no one else
> can
> create origin directories.

By "something", I meant something outside of our process, i.e., some other program running on the system.  I agree that is a far fetched error scenario, but I think it should be easy enough to address...

> > @@ +704,5 @@
> > > +      nsString leafName;
> > > +      rv = file->GetLeafName(leafName);
> > > +      NS_ENSURE_SUCCESS(rv, rv);
> > > +
> > > +      if (!leafName.Equals(idbDirectoryName)) {
> > 
> > I think you should also check to make sure that |file| is a file, not a
> > directory.
> 
> so we used to have this structure for persistent storages:
> main dir: profile/indexedDB
> origin dir: profile/indexedDB/http+++localhost
> db file: profile/indexedDB/http+++localhost/1511236768bal.sqlite
> db dir for stored files: profile/indexedDB/http+++localhost/1511236768bal
> 
> the centralized quota and storage manager added one more subdir to it:
> main dir: profile/indexedDB
> origin dir: profile/indexedDB/http+++localhost
> origin dir metadata file: profile/indexedDB/http+++localhost/.metadata
> storage specific dir: profile/indexedDB/http+++localhost/idb
> db file: profile/indexedDB/http+++localhost/idb/1511236768bal.sqlite
> db dir for stored files: profile/indexedDB/http+++localhost/idb/1511236768bal
> 
> so if the metadata file doesn't exist we need to put all db files and db dirs
> for stored files below a new subdir "idb"

OK, but I still think that you should make this check.  What happens if I create a "file" named |idb| in the above case for example?  Your code expects it to be a directory and never checks to make sure that is actually the case.

> > @@ +980,5 @@
> > >    MutexAutoLock lock(mQuotaMutex);
> > >  
> > > +  GroupInfoPair* pair;
> > > +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> > > +    pair = new GroupInfoPair();
> > 
> > Who destroys these objects?
> 
> What do you mean? Everything in this method should be infallible.

I meant, who has the responsibility to delete these GroupInfoPair objects?  I think we should document that clearly.

> > 
> > @@ +1161,5 @@
> > >    else {
> > >      fileSize = 0;
> > >    }
> > >  
> > > +  QuotaObject* quotaObject = nullptr;
> > 
> > Hmm, why don't you make this an nsRefPtr and get rid of |result| below?
> 
> I can't do that, QuotaObject implements a customized AddRef/Release() and
> they
> need to acquire the same lock.

That should be OK.  nsRefPtr will work fine for any type implementing an AddRef/Release() function.

> > @@ +1185,5 @@
> > >  
> > > +    originInfo->mQuotaObjects.Get(path, &quotaObject);
> > > +
> > > +    if (!quotaObject) {
> > > +      quotaObject = new QuotaObject(originInfo, path, fileSize);
> > 
> > Who destroys this object?
> 
> Again, we use infallible arrays/hashtables here.
> The QuotaObject is special, the caller of GetQuotaObject() will be the owner.
> So once the caller is done with work, QuotaObject will be released
> automatically.
> However there can be multiple callers requesting the same QuotaObject. That's
> why we keep weak references to QuotaObjects in OriginInfo.mQuotaObjects and
> we have special AddRef/Release implementation.

See above, I was basically talking about how we make sure that we don't leak the object we're new'ing here.

> > @@ +1861,5 @@
> > > +
> > > +#if 0
> > > +    *aDefaultPersistenceType =
> > > +      permission == nsIPermissionManager::ALLOW_ACTION ?
> > > +                    PERSISTENCE_TYPE_PERMANENT : PERSISTENCE_TYPE_TEMPORARY;
> > 
> > Why is this needed?
> 
> For now, I don't want to change the default storage type. We need to do a bit
> more testing, advocating, etc. before we change the default storage type to
> "temporary" 

Makes sense.

> > @@ +2257,5 @@
> > > +
> > > +  nsRefPtr<CollectOriginsHelper> helper =
> > > +    new CollectOriginsHelper(mQuotaMutex, aMinSizeToBeFreed);
> > > +
> > > +  // Unlock while calling out to XPCOM
> > 
> > Can you please add a comment explaining why this is needed?
> 
> Hm, it seems that nsThread needs to acquire its own lock (possible deadlock
> in 
> theory) and it also calls an observer. The observer can do various stuff
> like IO,
> so it's probably better to unlock before dispatching new runnables.
> 
> Should I say something like this in the comment?

Yes, please.

> > @@ +2811,5 @@
> > > +  NS_ENSURE_SUCCESS_VOID(rv);
> > > +
> > > +  bool exists;
> > > +  rv = directory->Exists(&exists);
> > > +  NS_ENSURE_SUCCESS_VOID(rv);
> > 
> > You shouldn't need to check for existence before removal.
> 
> Hm, I'm not sure about this one. For example rmdir() on unix can return an
> error
> when the named directory doesn't exist.
> Here's a  similar code we used to have in IDB (now in QuotaManager)
> http://mxr.mozilla.org/comm-esr17/source/mozilla/dom/indexedDB/
> IndexedDatabaseManager.cpp#1459
> I actually experimentally removed the Exists() check and I then got a fatal
> assertion "Failed to remove directory!" when the temp storage test was
> running.
> So I think the check must stay there.

Hmm.  I guess you could fix that by looking at the error code returned by nsIFile::Remove to see whether it has failed to remove the directory because it did not exist before the removal (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST?) but maybe that's not worth it here?

> > ::: dom/webidl/IDBFactory.webidl
> > @@ +24,5 @@
> > >  interface IDBFactory {
> > >    [Throws]
> > >    IDBOpenDBRequest
> > >    open(DOMString name,
> > > +       [EnforceRange] unsigned long long version);
> > 
> > Why are you making version non-optional?  Isn't it better to make the
> > IDBOpenDBOperation argument in the overload below optional?
> 
> That doesn't work, here's the error message:
> 
> WebIDL.WebIDLError: error: Dictionary argument or union argument containing
> a di
> ctionary not followed by a required argument must be optional,
> IDBFactory.webidl
>  line 31:24
>        IDBOpenDBOptions options);
>                         ^

Ah, OK.

> I'll attach updated and rebased patch soon.

Thanks a lot for providing the interdiff!  Makes it a lot easier to review.
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-02 13:02:46 PDT
Comment on attachment 770129 [details] [diff] [review]
interdiff

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

Looks good, with the comments below addressed.

::: dom/indexedDB/IDBFactory.cpp
@@ +795,5 @@
> +    privilege = mPrivilege;
> +    defaultPersistenceType = mDefaultPersistenceType;
> +  }
> +
> +  uint64_t version;

You can just init this to 0, and remove the else block below.

::: dom/quota/QuotaManager.cpp
@@ +155,1 @@
>    NS_DECL_NSIRUNNABLE

You can just remove these macros now.  The point of nsRunnable is that it enables you to implement an nsIRunnable by just overriding the Run() function.
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-02 13:02:57 PDT
Comment on attachment 770128 [details] [diff] [review]
patch v0.2

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

I already reviewed the interdiff.
Comment 20 Jan Varga [:janv] 2013-07-02 17:33:07 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18)
> Comment on attachment 770129 [details] [diff] [review]
> interdiff
> 
> Review of attachment 770129 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, with the comments below addressed.
> 
> ::: dom/indexedDB/IDBFactory.cpp
> @@ +795,5 @@
> > +    privilege = mPrivilege;
> > +    defaultPersistenceType = mDefaultPersistenceType;
> > +  }
> > +
> > +  uint64_t version;
> 
> You can just init this to 0, and remove the else block below.

done

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +155,1 @@
> >    NS_DECL_NSIRUNNABLE
> 
> You can just remove these macros now.  The point of nsRunnable is that it
> enables you to implement an nsIRunnable by just overriding the Run()
> function.

done
Comment 21 Jan Varga [:janv] 2013-07-02 18:02:29 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> (In reply to Jan Varga [:janv] from comment #14)
> > > ::: dom/quota/QuotaCommon.h
> > > @@ +26,5 @@
> > > >  void
> > > >  AssertIsOnIOThread();
> > > > +
> > > > +void
> > > > +AssertCurrentThreadOwnsQuotaMutex();
> > > 
> > > This is not needed...
> > 
> > Why ? I understand that |mQuotaMutex.AssertCurrentThreadOwns();| and
> > |quotaManager->AssertCurrentThreadOwnsQuotaMutex()| is noop in opt builds,
> > but
> > there's also |QuotaManager* quotaManager = QuotaManager::Get()| which is
> > useless
> > in optimized builds, no ?
> 
> You can use DebugOnly if that creates a warning during compilation, but the
> optimizer should be able to get rid of that anyway.

ok, I reworked it

> 
> > > @@ +510,5 @@
> > > > +    rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
> > > > +    NS_ENSURE_SUCCESS(rv, rv);
> > > > +
> > > > +    *aCreated = true;
> > > > +  }
> > > 
> > > This is prone to races, in case the directory is created between the call to
> > > Exists and Create.  You can just call Create() and set *aCreated based on
> > > whether it returns NS_ERROR_FILE_ALREADY_EXISTS.
> > 
> > Hm, if something else can create the directory then we are doomed anyway I
> > think.
> > All the directory management can happen on the IO thread only. Transactions
> > can
> > run on other background threads but they only read/write from/to .sqlite
> > files
> > Well, actually with this patch, a transaction running on its thread can
> > remove
> > entire origin directories when some space needs to be freed. But no one else
> > can
> > create origin directories.
> 
> By "something", I meant something outside of our process, i.e., some other
> program running on the system.  I agree that is a far fetched error
> scenario, but I think it should be easy enough to address...

ok, fixed

> 
> > > @@ +704,5 @@
> > > > +      nsString leafName;
> > > > +      rv = file->GetLeafName(leafName);
> > > > +      NS_ENSURE_SUCCESS(rv, rv);
> > > > +
> > > > +      if (!leafName.Equals(idbDirectoryName)) {
> > > 
> > > I think you should also check to make sure that |file| is a file, not a
> > > directory.
> > 
> > so we used to have this structure for persistent storages:
> > main dir: profile/indexedDB
> > origin dir: profile/indexedDB/http+++localhost
> > db file: profile/indexedDB/http+++localhost/1511236768bal.sqlite
> > db dir for stored files: profile/indexedDB/http+++localhost/1511236768bal
> > 
> > the centralized quota and storage manager added one more subdir to it:
> > main dir: profile/indexedDB
> > origin dir: profile/indexedDB/http+++localhost
> > origin dir metadata file: profile/indexedDB/http+++localhost/.metadata
> > storage specific dir: profile/indexedDB/http+++localhost/idb
> > db file: profile/indexedDB/http+++localhost/idb/1511236768bal.sqlite
> > db dir for stored files: profile/indexedDB/http+++localhost/idb/1511236768bal
> > 
> > so if the metadata file doesn't exist we need to put all db files and db dirs
> > for stored files below a new subdir "idb"
> 
> OK, but I still think that you should make this check.  What happens if I
> create a "file" named |idb| in the above case for example?  Your code
> expects it to be a directory and never checks to make sure that is actually
> the case.

hm, I added a check for that after the Create() call
but I don't think I need to check the file in the loop

> 
> > > @@ +980,5 @@
> > > >    MutexAutoLock lock(mQuotaMutex);
> > > >  
> > > > +  GroupInfoPair* pair;
> > > > +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> > > > +    pair = new GroupInfoPair();
> > > 
> > > Who destroys these objects?
> > 
> > What do you mean? Everything in this method should be infallible.
> 
> I meant, who has the responsibility to delete these GroupInfoPair objects? 
> I think we should document that clearly.

ok, added a comment

> 
> > > 
> > > @@ +1161,5 @@
> > > >    else {
> > > >      fileSize = 0;
> > > >    }
> > > >  
> > > > +  QuotaObject* quotaObject = nullptr;
> > > 
> > > Hmm, why don't you make this an nsRefPtr and get rid of |result| below?
> > 
> > I can't do that, QuotaObject implements a customized AddRef/Release() and
> > they
> > need to acquire the same lock.
> 
> That should be OK.  nsRefPtr will work fine for any type implementing an
> AddRef/Release() function.

Assigning something to nsRefPtr<QuotaObject> while I'm holding the mutex will
deadlock, no ?
(nsRefPtr needs to call AddRef when assigning something non null to it and
AddRef needs to acquire the same mutex)

> 
> > > @@ +1185,5 @@
> > > >  
> > > > +    originInfo->mQuotaObjects.Get(path, &quotaObject);
> > > > +
> > > > +    if (!quotaObject) {
> > > > +      quotaObject = new QuotaObject(originInfo, path, fileSize);
> > > 
> > > Who destroys this object?
> > 
> > Again, we use infallible arrays/hashtables here.
> > The QuotaObject is special, the caller of GetQuotaObject() will be the owner.
> > So once the caller is done with work, QuotaObject will be released
> > automatically.
> > However there can be multiple callers requesting the same QuotaObject. That's
> > why we keep weak references to QuotaObjects in OriginInfo.mQuotaObjects and
> > we have special AddRef/Release implementation.
> 
> See above, I was basically talking about how we make sure that we don't leak
> the object we're new'ing here.

I added some comments to the code

> 
> > > @@ +2257,5 @@
> > > > +
> > > > +  nsRefPtr<CollectOriginsHelper> helper =
> > > > +    new CollectOriginsHelper(mQuotaMutex, aMinSizeToBeFreed);
> > > > +
> > > > +  // Unlock while calling out to XPCOM
> > > 
> > > Can you please add a comment explaining why this is needed?
> > 
> > Hm, it seems that nsThread needs to acquire its own lock (possible deadlock
> > in 
> > theory) and it also calls an observer. The observer can do various stuff
> > like IO,
> > so it's probably better to unlock before dispatching new runnables.
> > 
> > Should I say something like this in the comment?
> 
> Yes, please.

done

> 
> > > @@ +2811,5 @@
> > > > +  NS_ENSURE_SUCCESS_VOID(rv);
> > > > +
> > > > +  bool exists;
> > > > +  rv = directory->Exists(&exists);
> > > > +  NS_ENSURE_SUCCESS_VOID(rv);
> > > 
> > > You shouldn't need to check for existence before removal.
> > 
> > Hm, I'm not sure about this one. For example rmdir() on unix can return an
> > error
> > when the named directory doesn't exist.
> > Here's a  similar code we used to have in IDB (now in QuotaManager)
> > http://mxr.mozilla.org/comm-esr17/source/mozilla/dom/indexedDB/
> > IndexedDatabaseManager.cpp#1459
> > I actually experimentally removed the Exists() check and I then got a fatal
> > assertion "Failed to remove directory!" when the temp storage test was
> > running.
> > So I think the check must stay there.
> 
> Hmm.  I guess you could fix that by looking at the error code returned by
> nsIFile::Remove to see whether it has failed to remove the directory because
> it did not exist before the removal (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST?)
> but maybe that's not worth it here?
> 

ok, fixed
Comment 22 Jan Varga [:janv] 2013-07-02 18:04:25 PDT
Created attachment 770555 [details] [diff] [review]
interdiff2
Comment 23 Jan Varga [:janv] 2013-07-02 18:08:32 PDT
Created attachment 770557 [details] [diff] [review]
patch v0.3
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-03 08:22:53 PDT
(In reply to Jan Varga [:janv] from comment #21)
> > > > @@ +1161,5 @@
> > > > >    else {
> > > > >      fileSize = 0;
> > > > >    }
> > > > >  
> > > > > +  QuotaObject* quotaObject = nullptr;
> > > > 
> > > > Hmm, why don't you make this an nsRefPtr and get rid of |result| below?
> > > 
> > > I can't do that, QuotaObject implements a customized AddRef/Release() and
> > > they
> > > need to acquire the same lock.
> > 
> > That should be OK.  nsRefPtr will work fine for any type implementing an
> > AddRef/Release() function.
> 
> Assigning something to nsRefPtr<QuotaObject> while I'm holding the mutex will
> deadlock, no ?
> (nsRefPtr needs to call AddRef when assigning something non null to it and
> AddRef needs to acquire the same mutex)

Oh, I see the problem now, OK.
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2013-07-03 08:25:45 PDT
Comment on attachment 770555 [details] [diff] [review]
interdiff2

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

Looks great, thanks!
Comment 26 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-25 12:21:22 PDT
Comment on attachment 770557 [details] [diff] [review]
patch v0.3

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

Sorry this took so long :(

In general things look fine!

::: dom/base/nsDOMWindowUtils.cpp
@@ +2876,5 @@
> +  }
> +
> +  quota::PersistenceType persistenceType =
> +    indexedDB::IDBFactory::GetPersistenceTypeFromOptions(options,
> +                                                         defaultPersistence);

'PersistenceType' lives in quota namespace, but we pull it through some function on IDBFactory? I think we should make this quota only.

::: dom/bindings/Nullable.h
@@ +63,5 @@
>    bool IsNull() const {
>      return mIsNull;
>    }
>  
> +  bool Equals(const Nullable<T>& aOtherNullable) const

Can't this be simplified to:

  return (mIsNull && aOtherNullable.mIsNull) ||
         (!mIsNull && !aOtherNullable.mIsNull &&
          mValue == aOtherNullable.mValue);

@@ +83,5 @@
> +  {
> +    return !Equals(aOtherNullable);
> +  }
> +
> +  bool operator!() const

Don't you want 'operator bool'?

::: dom/indexedDB/CheckPermissionsHelper.cpp
@@ +98,5 @@
>  CheckPermissionsHelper::Run()
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  if (mHelper->mPersistenceType == quota::PERSISTENCE_TYPE_TEMPORARY) {

This seems wasteful to me. If we're temporary then why are we using a CheckPermissionHelper to begin with?

::: dom/indexedDB/IDBDatabase.cpp
@@ +495,5 @@
>  
>  NS_IMETHODIMP
> +IDBDatabase::GetMozStorage(nsAString& aStorage)
> +{
> +  nsresult rv = PersistenceTypeToText(mPersistenceType, aStorage);

Hrm, how does the compiler find this in the quota namespace? I think you must have a leak somewhere.

::: dom/indexedDB/IDBDatabase.h
@@ +180,5 @@
>    nsRefPtr<DatabaseInfo> mPreviousDatabaseInfo;
>    nsCOMPtr<nsIAtom> mDatabaseId;
>    nsString mName;
>    nsString mFilePath;
> +  PersistenceType mPersistenceType;

Add an (invalid) initializer for this.

::: dom/indexedDB/IDBFactory.cpp
@@ +282,5 @@
> +  NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +  rv = fileUrl->SetQuery(NS_LITERAL_CSTRING("persistenceType=") + type +
> +                         NS_LITERAL_CSTRING("&group=") + aGroup +
> +                         NS_LITERAL_CSTRING("&origin=") + aOrigin);

I wonder if we shouldn't query-encode these. Maybe debug-only assertions for now?

@@ +532,5 @@
> +                                        const IDBOpenDBOptions& aOptions,
> +                                        PersistenceType aDefaultPersistenceType)
> +{
> +  if (aOptions.mStorage.WasPassed()) {
> +    MOZ_STATIC_ASSERT(

This doesn't really belong here... Maybe in QuotaManager or something?

@@ +543,5 @@
> +      "Enum values should match.");
> +
> +    return PersistenceType(static_cast<int>(aOptions.mStorage.Value()));
> +  }
> +  else {

Nit: else after a return

@@ +608,5 @@
> +  }
> +
> +  if (aPrivilege == Chrome) {
> +    // Chrome privilege, ignore the persistence type parameter.
> +    aPersistenceType = PERSISTENCE_TYPE_PERMANENT;

Chrome can't request temporary storage?

@@ +689,5 @@
>    return IDBFactoryBinding::Wrap(aCx, aScope, this);
>  }
>  
> +already_AddRefed<nsIIDBOpenDBRequest>
> +IDBFactory::Open(const NonNull<nsAString>& aName, uint64_t aVersion,

Let's go the other way. Rather than having this function construct an options object just to get sent into common code we should instead have the code that must deal with options objects call into this function.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +43,5 @@
>  static NS_DEFINE_CID(kDOMSOF_CID, NS_DOM_SCRIPT_OBJECT_FACTORY_CID);
>  
> +BEGIN_INDEXEDDB_NAMESPACE
> +
> +class FileManagerInfo

Several of these methods are const-safe and should be marked as such.

@@ +71,5 @@
> +                                 const nsAString& aName);
> +
> +private:
> +  nsTArray<nsRefPtr<FileManager> >*
> +  GetArrayForPersistenceType(PersistenceType aPersistenceType);

This should return a reference, not a pointer, since there's no way it can happen (and you're not currently null-checking anyway).

@@ +435,5 @@
>  
> +  info->AddFileManager(aFileManager);
> +}
> +
> +struct MOZ_STACK_CLASS InvalidateInfo

Nit: This should live in the anon namespace

@@ +442,5 @@
> +  : persistenceType(aPersistenceType), pattern(aPattern)
> +  { }
> +
> +  PersistenceType persistenceType;
> +  nsCString pattern;

This should be a 'const nsACString&' since it's a stack variable.

@@ +669,5 @@
>     return NS_ERROR_UNEXPECTED;
>   }
>  
> +already_AddRefed<FileManager>
> +FileManagerInfo::GetFileManager(PersistenceType aPersistenceType,

Assert IO thread here and in these other methods.

@@ +676,5 @@
> +  nsTArray<nsRefPtr<FileManager > >* managers =
> +     GetArrayForPersistenceType(aPersistenceType);
> +
> +  for (uint32_t i = 0; i < managers->Length(); i++) {
> +    nsRefPtr<FileManager>& fileManager = managers->ElementAt(i);

This can be const I think

@@ +678,5 @@
> +
> +  for (uint32_t i = 0; i < managers->Length(); i++) {
> +    nsRefPtr<FileManager>& fileManager = managers->ElementAt(i);
> +
> +    if (fileManager->DatabaseName().Equals(aName)) {

Nit: == looks way nicer than Equals(), here and several other places.

@@ +693,5 @@
> +{
> +  nsTArray<nsRefPtr<FileManager > >* managers =
> +    GetArrayForPersistenceType(aFileManager->Type());
> +
> +  managers->AppendElement(aFileManager);

I think you should do a DEBUG-only "!managers->Contains(aFileManager)" assertion here before appending.

@@ +702,5 @@
> +{
> +  uint32_t i;
> +
> +  for (i = 0; i < mPermStorageFileManagers.Length(); i++) {
> +    mPermStorageFileManagers[i]->Invalidate();

This can't trigger recursion, can it?

@@ +754,5 @@
> +
> +    case PERSISTENCE_TYPE_INVALID:
> +    default:
> +      NS_NOTREACHED("Bad storage type value!");
> +      return nullptr;

Might as well do MOZ_CRASH here.

::: dom/indexedDB/nsIIDBDatabase.idl
@@ +23,5 @@
>    readonly attribute DOMString name;
>  
>    readonly attribute unsigned long long version;
>  
> +  readonly attribute DOMString mozStorage;

The plan going forward (at least as far as I know) is that we will not be adding new 'mozFoo' properties or methods any longer. They should be unprefixed but hidden behind an 'experimental' pref so that most users don't see them. I would suggest "dom.indexedDB.experimental.foo" for this kind of thing.

Also I'm not sure that this property in particular is all that useful. Is it something you're planning on proposing to the indexedDB mailing list?

::: dom/indexedDB/test/test_persistenceType.html
@@ +8,5 @@
> +
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +
> +  <script type="text/javascript;version=1.7">

Wait, why doesn't this include test_persistenceType.js?

::: dom/ipc/PBrowser.ipdl
@@ +241,5 @@
>       */
>      POfflineCacheUpdate(URIParams manifestURI, URIParams documentURI,
>                          bool stickDocument);
>  
> +    sync PIndexedDB(nsCString group, nsCString asciiOrigin)

We need a followup to remove this sync message :)

::: dom/quota/CollectOriginsHelper.cpp
@@ +49,5 @@
> +  QuotaManager* quotaManager = QuotaManager::Get();
> +  NS_ASSERTION(quotaManager, "Shouldn't be null!");
> +
> +  mSizeToBeFreed =
> +    quotaManager->CollectOriginsForEviction(mMinSizeToBeFreed, mOriginInfos);

This will trigger race detector warnings I think since you're accessing the same memory with and without the lock held. I think it would be simpler to use stack vars here and then swap them with the members later below once you've locked.

::: dom/quota/CollectOriginsHelper.h
@@ +17,5 @@
> +BEGIN_QUOTA_NAMESPACE
> +
> +class OriginInfo;
> +
> +class CollectOriginsHelper MOZ_FINAL : public nsIRunnable

I don't see why you can't put this fully inside QuotaManager.cpp. It doesn't need to be exposed anywhere, right?

::: dom/quota/PersistenceType.h
@@ +40,5 @@
> +  return NS_OK;
> +}
> +
> +inline nsresult
> +PersistenceTypeToText(PersistenceType aPersistenceType, nsACString& aText)

returning bool should be fine

::: dom/quota/QuotaManager.cpp
@@ +79,5 @@
> +// On Android, smaller/older devices may have very little storage and
> +// device owners may be sensitive to storage footprint: Use a smaller
> +// percentage of available space and a smaller minimum/maximum.
> +static const uint64_t gTempStorageLimitMin =     10;     //  10 MB
> +static const uint64_t gTempStorageLimitMax =    200;     // 200 MB

Nit: These should have "KB" suffixes.

@@ +82,5 @@
> +static const uint64_t gTempStorageLimitMin =     10;     //  10 MB
> +static const uint64_t gTempStorageLimitMax =    200;     // 200 MB
> +static const double   gTempStorageLimitRatio =   .2;     //  20 %
> +#else
> +static const uint64_t gTempStorageLimitMin =     50;     //  50 MB

I think these should probably be overridable via prefs. Some devices may need different limits.

@@ +204,5 @@
>  // files in the origin's directory before dispatching itself back to the main
>  // thread. When on the main thread the runnable will call the callback and then
>  // notify the QuotaManager that the job has been completed.
>  class AsyncUsageRunnable MOZ_FINAL : public UsageRunnable,
> +                                     public nsRunnable,

It's really confusing that UsageRunnable isn't actually a runnable. Can you rename that while you're here?

@@ +345,2 @@
>  void
>  AssertIsOnIOThread()

Just make this call IsOnIOThread

@@ +385,5 @@
>  QuotaManager* gInstance = nullptr;
>  int32_t gShutdown = 0;
>  
>  int32_t gStorageQuotaMB = DEFAULT_QUOTA_MB;
> +int32_t gTempStorageLimitKB = -1;

We're never going to need to care about KB here. Let's use MB everywhere?

@@ +581,5 @@
> +
> +  rv = binaryStream->SetOutputStream(outputStream);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ADDREF(*aStream = binaryStream);

binaryStream.forget(aStream)

@@ +587,5 @@
> +}
> +
> +nsresult
> +CreateDirectoryMetadata(nsIFile* aDirectory, int64_t aTimestamp,
> +                        const nsACString& aGroup, const nsACString& aOrigin)

You could avoid the timestamp param and just call PR_Now() in here

@@ +596,5 @@
> +  nsresult rv =
> +    GetDirectoryMetadataStream(aDirectory, false, getter_AddRefs(stream));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!stream) {

You can assert this.

@@ +640,5 @@
> +
> +  rv = binaryStream->SetInputStream(bufferedStream);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  PRUint64 timestamp;

uint64_t. Double check to make sure you didn't add other PR* types in other places.

@@ +643,5 @@
> +
> +  PRUint64 timestamp;
> +  rv = binaryStream->Read64(&timestamp);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  *aTimestamp = timestamp;

Nit: I'd prefer you not set any out params until you're about to return NS_OK.

@@ +734,5 @@
> +// space users have free on their hard drive. We use a tiered scheme: the more
> +// space available, the larger the temp storage will be. However, we do not want
> +// to enable the temp storage to grow to an unbounded size, so the larger the
> +// user's available space is, the smaller of a percentage we take. We set a
> +// lower bound of 50MB and an upper bound of 1GB (10MB/200MB on Android).

Nit: Mention the constants above here rather than specific amounts since they can change.

@@ +736,5 @@
> +// to enable the temp storage to grow to an unbounded size, so the larger the
> +// user's available space is, the smaller of a percentage we take. We set a
> +// lower bound of 50MB and an upper bound of 1GB (10MB/200MB on Android).
> +uint64_t
> +SmartTempStorageLimit(const uint64_t availKB)

This returns MB right? Add that to the name.

Nit: aAvailableKB

@@ +740,5 @@
> +SmartTempStorageLimit(const uint64_t availKB)
> +{
> +  if (availKB > 100 * 1024 * 1024) {
> +    // skip computing if we're over 100 GB
> +    return gTempStorageLimitMax * 1024;

This is a little arbitrary, right? This should somehow be based on the constants above or else they won't work as you expect.

@@ +743,5 @@
> +    // skip computing if we're over 100 GB
> +    return gTempStorageLimitMax * 1024;
> +  }
> +
> +  // Grow/shrink in 10 MB units, deliberately, so that in the common case we

This sounds like a constant that might need to be different on mobile vs. desktop.

@@ +745,5 @@
> +  }
> +
> +  // Grow/shrink in 10 MB units, deliberately, so that in the common case we
> +  // don't shrink temp storage and evict origin data every time we initialize.
> +  uint64_t sz10MBs = 0;

How about we name this something like 'chunkCount'?

@@ +746,5 @@
> +
> +  // Grow/shrink in 10 MB units, deliberately, so that in the common case we
> +  // don't shrink temp storage and evict origin data every time we initialize.
> +  uint64_t sz10MBs = 0;
> +  uint64_t avail10MBs = availKB / (1024 * 10);

'availableChunks'?

@@ +755,5 @@
> +    avail10MBs = 2500;
> +  }
> +
> +  // 1 % of space between 7GB -> 25 GB
> +  if (avail10MBs > 700) {

These all need to be "else if". Otherwise you always go into every branch and always end up using the smallest amount of space :)

@@ +769,5 @@
> +
> +  // gTempStorageLimitRatio of space up to 500 MB (gTempStorageLimitMin
> +  // at minimum)
> +  sz10MBs += std::max<uint64_t>(gTempStorageLimitMin / 10,
> +                    static_cast<uint64_t>(avail10MBs * gTempStorageLimitRatio));

Is this meant to be the minimum case? (e.g. the final 'else'?)

@@ +785,5 @@
> +  nsresult rv = aDirectory->GetDiskSpaceAvailable(&bytesAvailable);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bytesAvailable =
> +    static_cast<uint64_t>((bytesAvailable + aCurrentUsage) / 1024);

Maybe first assert that bytesAvailable is >= 0?

@@ +787,5 @@
> +
> +  bytesAvailable =
> +    static_cast<uint64_t>((bytesAvailable + aCurrentUsage) / 1024);
> +
> +  *aLimit = 1024 * SmartTempStorageLimit(bytesAvailable);

Is there a reason for the two separate functions? Seems like you could avoid the unit conversions and just combine the two functions.

@@ +919,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = permStorageDir->Append(NS_LITERAL_STRING("indexedDB"));

Can we take this opportunity to fix the storage directory naming? It's always been wrong to have "<profile>/indexedDB/<origin>/idb/" paths (my fault, sorry).

Let's change to "<profile>/storage" now before we let the madness spread any further.

@@ +927,5 @@
> +    nsCOMPtr<nsIFile> tempStorageDir;
> +    rv = baseDir->Clone(getter_AddRefs(tempStorageDir));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    rv = tempStorageDir->Append(NS_LITERAL_STRING("storageTemp"));

Hm, I was expecting symmetrical directories:

  <profile>/storage/permanent/<origin>/idb
  <profile>/storage/temporary/<origin>/idb

@@ +975,5 @@
>    return NS_OK;
>  }
>  
>  void
> +QuotaManager::InitQuotaForOrigin(PersistenceType aPersistenceType,

What thread will this run on?

@@ +1009,4 @@
>  }
>  
>  void
> +QuotaManager::DecreaseUsageForOrigin(PersistenceType aPersistenceType,

What thread will this run on?

@@ +1017,5 @@
>    MutexAutoLock lock(mQuotaMutex);
>  
> +  GroupInfoPair* pair;
> +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> +    return;

This should never happen, right?

@@ +1022,5 @@
> +  }
> +
> +  nsRefPtr<GroupInfo> groupInfo = pair->LockedGetGroupInfo(aPersistenceType);
> +
> +  if (!groupInfo) {

Nit: extra newline

@@ +1033,5 @@
>    }
>  }
>  
>  void
> +QuotaManager::UpdateOriginAccessTime(PersistenceType aPersistenceType,

This must always happen on the main thread, right? Needs an assertion.

@@ +1041,5 @@
> +  MutexAutoLock lock(mQuotaMutex);
> +
> +  GroupInfoPair* pair;
> +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> +    return;

This should never happen, right?

@@ +1060,5 @@
> +      return;
> +    }
> +
> +    {
> +      MutexAutoUnlock autoUnlock(mQuotaMutex);

Nit: Braces not needed here, your scope is correct without them.

@@ +1062,5 @@
> +
> +    {
> +      MutexAutoUnlock autoUnlock(mQuotaMutex);
> +
> +      SaveOriginAccessTime(aOrigin, timestamp);

Hm. Since you're unlocked you are subject to races here (unless this is always main thread only)

@@ +1095,5 @@
> +
> +  NS_ASSERTION(mTempStorageUsage == 0, "Should be zero!");
> +}
> +
> +struct MOZ_STACK_CLASS RemoveQuotaInfo

This can go in the anon namespace.

@@ +1173,5 @@
>    else {
>      fileSize = 0;
>    }
>  
> +  QuotaObject* quotaObject = nullptr;

Nit: No need to initialize this

@@ +1205,5 @@
>    }
>  
> +  // The caller becomes the owner of the QuotaObject, that is, the caller is
> +  // is responsible to delete it when the last reference is removed.
> +  nsRefPtr<QuotaObject> result = quotaObject;

Why not just stick this above the lock scope and assign to it directly?

@@ +1611,5 @@
> +    rv = EnsureDirectory(directory, &created);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (created) {
> +      rv = CreateDirectoryMetadata(directory);

Why aren't you adding a modified time here? It's bizarre that there are two CreateDirectoryMetadata functions and they don't do the same thing...

@@ +1615,5 @@
> +      rv = CreateDirectoryMetadata(directory);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +
> +    if (mInitializedOrigins.Contains(aOrigin)) {

Shouldn't this early return happen before trying to create directories?

@@ +1626,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mInitializedOrigins.AppendElement(aOrigin);
> +
> +    NS_ADDREF(*aDirectory = directory);

directory.forget()

@@ +1634,5 @@
> +  NS_ASSERTION(aPersistenceType == PERSISTENCE_TYPE_TEMPORARY, "Huh?");
> +  NS_ASSERTION(aTrackQuota, "Huh?");
> +
> +  if (!mTempStorageInitialized) {
> +    mTempStorageInitialized = true;

So what happens if something fails in here? It looks like the next time through it will look like it succeeds, but it won't be initialized properly...

@@ +1715,5 @@
> +    if (aOriginOrPattern.IsOrigin()) {
> +      mInitializedOrigins.RemoveElement(aOriginOrPattern);
> +    }
> +    else {
> +      for (int32_t i = mInitializedOrigins.Length() - 1; i >= 0; i--) {

First assert that mInitializedOrigins.Length() is less than int32_max

@@ +1762,2 @@
>  {
>    nsCString str(aOrigin);

This should be nsAutoCString since we're doing all this append stuff.

@@ +1762,5 @@
>  {
>    nsCString str(aOrigin);
>    str.Append("*");
>    str.Append(NS_ConvertUTF16toUTF8(aName));
> +  str.Append("*");

Can you switch these to use single quotes? s/"/'/

@@ +1800,5 @@
> +}
> +
> +// static
> +nsresult
> +QuotaManager::GetInfoFromPrincipal(nsIPrincipal* aPrincipal,

Need to assert main thread here.

@@ +1879,5 @@
> +                                         PERMISSION_DEFAUT_PERMANENT_STORAGE,
> +                                         &permission);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +#if 0

This should be removed right?

@@ +1968,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +QuotaManager::Clear()

This is dangerous enough that I think we should guard it with a pref. Then you could just set the pref during your testing.

@@ +2042,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +QuotaManager::Reset()

Same here.

@@ +2056,5 @@
> +  runnable->AdvanceState();
> +
> +  // Give the runnable some help by invalidating any storages in the way.
> +  StorageMatcher<nsAutoTArray<nsIOfflineStorage*, 20> > matches;
> +    matches.Find(mLiveStorages);

Nit: indentation

@@ +2583,5 @@
> +    NS_ASSERTION(quotaManager, "Shouldn't be null!");
> +
> +    if (groupInfo->mUsage > GROUP_LIMIT(quotaManager->mTempStorageLimit)) {
> +      nsTArray<OriginInfo*>* doomedOrigins =
> +        static_cast<nsTArray<OriginInfo*>* >(aUserArg);

Nit: no space between * and >

@@ +2618,5 @@
> +  nsRefPtr<GroupInfo> groupInfo =
> +    aValue->LockedGetGroupInfo(PERSISTENCE_TYPE_TEMPORARY);
> +  if (groupInfo) {
> +    nsTArray<OriginInfo*>* originInfos =
> +      static_cast<nsTArray<OriginInfo*>* >(aUserArg);

Nit: No space here either

@@ +2627,5 @@
> +  return PL_DHASH_NEXT;
> +}
> +
> +void
> +QuotaManager::ClearOrigins(nsTArray<OriginInfo*>& aOrigins)

const nsTArray

@@ +2633,5 @@
> +  for (uint32_t index = 0; index < aOrigins.Length(); index++) {
> +    OriginInfo* originInfo = aOrigins[index];
> +
> +    nsCString group = originInfo->mGroupInfo->mGroup;
> +    nsCString origin = originInfo->mOrigin;

No need for these stack vars right? They can probably just be refs.

@@ +2645,5 @@
> +  }
> +}
> +
> +void
> +QuotaManager::CheckTempStorageLimits()

This function does an awful lot of grabbing and dropping the lock. Is there no way to streamlne this a bit?

@@ +2657,5 @@
> +  }
> +
> +  ClearOrigins(originInfos);
> +
> +  if (mTempStorageUsage > mTempStorageLimit) {

How is it safe to check this without holding the lock?

@@ +2669,5 @@
> +    originInfos.Sort(OriginInfoComparator());
> +
> +    uint64_t usage = 0;
> +    for (uint32_t i = 0; i < originInfos.Length(); i++) {
> +      if (mTempStorageUsage - usage <= mTempStorageLimit) {

here too...

@@ +2670,5 @@
> +
> +    uint64_t usage = 0;
> +    for (uint32_t i = 0; i < originInfos.Length(); i++) {
> +      if (mTempStorageUsage - usage <= mTempStorageLimit) {
> +        originInfos.TruncateLength(i);

Wait, won't this do the opposite of what you want? You're going to end up holding on to the oldest origins, not the newest.

@@ +2713,5 @@
> +
> +  return PL_DHASH_NEXT;
> +}
> +
> +struct InactiveOriginsInfo

Stick this in the anonymous namespace

@@ +2815,5 @@
> +      inactiveOrigins.TruncateLength(index);
> +      break;
> +    }
> +
> +    OriginInfo* originInfo = inactiveOrigins[index];

No need for the stack var.

@@ +2840,5 @@
> +  return 0;
> +}
> +
> +void
> +QuotaManager::DeleteFilesForOrigin(const nsACString& aOrigin)

Nit: Might as well call this 'DeleteTemporaryFilesForOrigin'

@@ +2863,5 @@
> +
> +  nsRefPtr<FinalizeOriginEvictionRunnable> runnable =
> +    new FinalizeOriginEvictionRunnable(aOrigins);
> +
> +  if (IsOnIOThread()) {

Why would this be false? You already asserted that you're not on the main thread

@@ +3354,5 @@
> +        continue;
> +      }
> +
> +#ifdef XP_MACOSX
> +      if (leafName.EqualsLiteral(DSSTORE_FILE_NAME)) {

I've recently noticed that this shouldn't be mac only since it's theoretically possible to share profiles between windows and mac machines. Let's remove the ifdefs here and in the origin init code.

@@ +3382,5 @@
> +      }
> +
> +      nsRefPtr<Client>& client = aQuotaManager->mClients[clientType];
> +
> +      if (!initialized) {

Nit: if you handle both then start with 'initialized', not '!initialized'

@@ +3618,5 @@
> +{
> +  for (uint32_t index = 0; index < mOrigins.Length(); index++) {
> +    aQuotaManager->OriginClearCompleted(PERSISTENCE_TYPE_TEMPORARY,
> +                                        OriginOrPatternString::FromOrigin(
> +                                        mOrigins[index]));

Nit: indent last line better

::: dom/quota/QuotaManager.h
@@ +25,5 @@
>  #include "StoragePrivilege.h"
>  
>  #define QUOTA_MANAGER_CONTRACTID "@mozilla.org/dom/quota/manager;1"
>  
> +#define GROUP_LIMIT(x) (x / 5)

Let's get rid of this. It just makes the code harder to read.

@@ +52,3 @@
>  struct SynchronizedOp;
>  
> +typedef Nullable<PersistenceType> PersistenceScope;

I really don't think this helps much. It's not clear what a "persistence scope" is, or why it would be different from a "persistence type", so I'd suggest removing this typedef and using Nullable<PersistenceType> directly in the code.

@@ +281,5 @@
>  
>    static already_AddRefed<nsIAtom>
>    GetStorageId(const nsACString& aOrigin,
> +               const nsAString& aName,
> +               PersistenceType aPersistenceType);

Everywhere else you have type, then (sometimes) group, then origin, then name. Let's not deviate :)

::: dom/quota/QuotaObject.cpp
@@ +117,5 @@
> +    uint64_t newGroupUsage = groupInfo->mUsage + delta;
> +
> +    // Temporary storage has a hard limit for group usage (20 % of the
> +    // global limit).
> +    if (newGroupUsage > GROUP_LIMIT(quotaManager->mTempStorageLimit)) {

Let's just make GROUP_LIMIT into a small inlined function instead of a macro.

@@ +164,5 @@
> +      // all the essential variables and recheck the group limit.
> +
> +      delta = end - mSize;
> +
> +      newUsage = mOriginInfo->mUsage + delta;

What guarantees mOriginInfo is still valid here? Could it have gone away while you were unlocked?

@@ +173,5 @@
> +        // Unfortunately some other thread increased the group usage in the
> +        // meantime and we are not below the group limit anymore.
> +
> +        // However, the origin eviction must be finalized in this case too.
> +        {

Nit: Braces not needed here.

@@ +210,5 @@
> +
> +      return true;
> +    }
> +
> +    mOriginInfo->mUsage = newUsage;

How about you make this block happen first since it's so much shorter? It's hard to read through all that above and figure out what conditions apply here.

::: dom/quota/QuotaObject.h
@@ +82,5 @@
>  
> +  bool
> +  operator==(const OriginInfo& aOther) const
> +  {
> +    return mAccessTime == aOther.mAccessTime;

Is this really all you need to check here? I would think you need to at least check mOrigin

@@ +249,5 @@
> +    return mPermStorageGroupInfo || mTempStorageGroupInfo;
> +  }
> +
> +  nsRefPtr<GroupInfo>*
> +  GetGroupInfoForPersistenceType(PersistenceType aPersistenceType);

Nit: Let's make this return a reference.

::: dom/quota/nsIOfflineStorage.h
@@ +42,5 @@
> +  NS_IMETHOD_(PersistenceType)
> +  Type() = 0;
> +
> +  NS_IMETHOD_(const nsACString&)
> +  Group() = 0;

Hm, these methods are going to be identical for all subclasses, right? Why not just add the members here so that subclasses don't have to provide their own identical implementations?

::: dom/quota/nsIQuotaManager.idl
@@ +31,5 @@
>  
>    /**
> +   * Removes all storages. The files may not be deleted immediately depending
> +   * on prohibitive concurrent operations.
> +   * Be carefull, this removes all ever stored data!

Nit:  s/carefull/careful/, and "this removes *all* the data that has ever been stored"

::: storage/src/TelemetryVFS.cpp
@@ +367,5 @@
> +                                     nsDependentCString(origin),
> +                                     NS_ConvertUTF8toUTF16(zName));
> +    }
> +    else {
> +      NS_WARNING("Passed wrong persistence type!");

Do you need to explicitly set p->quotaObject to null here?

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1162,5 @@
>      Cu.forceCC();
>    },
>  
> +  // Due to various dependencies between JS objects and C++ objects, an ordinary
> +  // forceGC doesn't necessarily clears all unused objects, thus the GC and CC

Nit: s/clears/clear/
Comment 27 Jan Varga [:janv] 2013-08-12 15:45:51 PDT
Created attachment 789216 [details] [diff] [review]
patch v0.4
Comment 28 Jan Varga [:janv] 2013-08-12 15:48:06 PDT
Created attachment 789218 [details] [diff] [review]
interdiff v0.3 - v0.4
Comment 29 Jan Varga [:janv] 2013-08-12 16:13:57 PDT
(In reply to ben turner [:bent] from comment #26)
> Comment on attachment 770557 [details] [diff] [review]
> patch v0.3
> 
> Review of attachment 770557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry this took so long :(
> 
> In general things look fine!
> 
> ::: dom/base/nsDOMWindowUtils.cpp
> @@ +2876,5 @@
> > +  }
> > +
> > +  quota::PersistenceType persistenceType =
> > +    indexedDB::IDBFactory::GetPersistenceTypeFromOptions(options,
> > +                                                         defaultPersistence);
> 
> 'PersistenceType' lives in quota namespace, but we pull it through some
> function on IDBFactory? I think we should make this quota only.

ok, a slightly modified method now lives in PersistenceType.h

> 
> ::: dom/bindings/Nullable.h
> @@ +63,5 @@
> >    bool IsNull() const {
> >      return mIsNull;
> >    }
> >  
> > +  bool Equals(const Nullable<T>& aOtherNullable) const
> 
> Can't this be simplified to:
> 
>   return (mIsNull && aOtherNullable.mIsNull) ||
>          (!mIsNull && !aOtherNullable.mIsNull &&
>           mValue == aOtherNullable.mValue);

yeah, it can

> 
> @@ +83,5 @@
> > +  {
> > +    return !Equals(aOtherNullable);
> > +  }
> > +
> > +  bool operator!() const
> 
> Don't you want 'operator bool'?

yeah, fixed

> 
> ::: dom/indexedDB/CheckPermissionsHelper.cpp
> @@ +98,5 @@
> >  CheckPermissionsHelper::Run()
> >  {
> >    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> >  
> > +  if (mHelper->mPersistenceType == quota::PERSISTENCE_TYPE_TEMPORARY) {
> 
> This seems wasteful to me. If we're temporary then why are we using a
> CheckPermissionHelper to begin with?

ok, I've tried to do it in a cleaner way

> 
> ::: dom/indexedDB/IDBDatabase.cpp
> @@ +495,5 @@
> >  
> >  NS_IMETHODIMP
> > +IDBDatabase::GetMozStorage(nsAString& aStorage)
> > +{
> > +  nsresult rv = PersistenceTypeToText(mPersistenceType, aStorage);
> 
> Hrm, how does the compiler find this in the quota namespace? I think you
> must have a leak somewhere.

http://stackoverflow.com/questions/5392699/why-does-c-parameter-scope-affect-function-lookup-within-a-namespace

> 
> ::: dom/indexedDB/IDBDatabase.h
> @@ +180,5 @@
> >    nsRefPtr<DatabaseInfo> mPreviousDatabaseInfo;
> >    nsCOMPtr<nsIAtom> mDatabaseId;
> >    nsString mName;
> >    nsString mFilePath;
> > +  PersistenceType mPersistenceType;
> 
> Add an (invalid) initializer for this.

ok, I also removed |mDatabaseId(0)| while I was there
actually, it now lives in nsIOfflineStorage.h

> 
> ::: dom/indexedDB/IDBFactory.cpp
> @@ +282,5 @@
> > +  NS_ENSURE_SUCCESS(rv, nullptr);
> > +
> > +  rv = fileUrl->SetQuery(NS_LITERAL_CSTRING("persistenceType=") + type +
> > +                         NS_LITERAL_CSTRING("&group=") + aGroup +
> > +                         NS_LITERAL_CSTRING("&origin=") + aOrigin);
> 
> I wonder if we shouldn't query-encode these. Maybe debug-only assertions for
> now?

Hm, it seems that nsStandardURL::SetQuery() does the job for us ?

> 
> @@ +532,5 @@
> > +                                        const IDBOpenDBOptions& aOptions,
> > +                                        PersistenceType aDefaultPersistenceType)
> > +{
> > +  if (aOptions.mStorage.WasPassed()) {
> > +    MOZ_STATIC_ASSERT(
> 
> This doesn't really belong here... Maybe in QuotaManager or something?

ok, the method now lives in PersistenceType.h

> 
> @@ +543,5 @@
> > +      "Enum values should match.");
> > +
> > +    return PersistenceType(static_cast<int>(aOptions.mStorage.Value()));
> > +  }
> > +  else {
> 
> Nit: else after a return

fixed

> 
> @@ +608,5 @@
> > +  }
> > +
> > +  if (aPrivilege == Chrome) {
> > +    // Chrome privilege, ignore the persistence type parameter.
> > +    aPersistenceType = PERSISTENCE_TYPE_PERMANENT;
> 
> Chrome can't request temporary storage?

can we do this as a followup bug ?
(it wasn't in original requirements)

> 
> @@ +689,5 @@
> >    return IDBFactoryBinding::Wrap(aCx, aScope, this);
> >  }
> >  
> > +already_AddRefed<nsIIDBOpenDBRequest>
> > +IDBFactory::Open(const NonNull<nsAString>& aName, uint64_t aVersion,
> 
> Let's go the other way. Rather than having this function construct an
> options object just to get sent into common code we should instead have the
> code that must deal with options objects call into this function.

ok, fixed

> 
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +43,5 @@
> >  static NS_DEFINE_CID(kDOMSOF_CID, NS_DOM_SCRIPT_OBJECT_FACTORY_CID);
> >  
> > +BEGIN_INDEXEDDB_NAMESPACE
> > +
> > +class FileManagerInfo
> 
> Several of these methods are const-safe and should be marked as such.

ok, done

> 
> @@ +71,5 @@
> > +                                 const nsAString& aName);
> > +
> > +private:
> > +  nsTArray<nsRefPtr<FileManager> >*
> > +  GetArrayForPersistenceType(PersistenceType aPersistenceType);
> 
> This should return a reference, not a pointer, since there's no way it can
> happen (and you're not currently null-checking anyway).

yeah, fixed

> 
> @@ +435,5 @@
> >  
> > +  info->AddFileManager(aFileManager);
> > +}
> > +
> > +struct MOZ_STACK_CLASS InvalidateInfo
> 
> Nit: This should live in the anon namespace

ok

> 
> @@ +442,5 @@
> > +  : persistenceType(aPersistenceType), pattern(aPattern)
> > +  { }
> > +
> > +  PersistenceType persistenceType;
> > +  nsCString pattern;
> 
> This should be a 'const nsACString&' since it's a stack variable.

ok, fixed

> 
> @@ +669,5 @@
> >     return NS_ERROR_UNEXPECTED;
> >   }
> >  
> > +already_AddRefed<FileManager>
> > +FileManagerInfo::GetFileManager(PersistenceType aPersistenceType,
> 
> Assert IO thread here and in these other methods.

done

> 
> @@ +676,5 @@
> > +  nsTArray<nsRefPtr<FileManager > >* managers =
> > +     GetArrayForPersistenceType(aPersistenceType);
> > +
> > +  for (uint32_t i = 0; i < managers->Length(); i++) {
> > +    nsRefPtr<FileManager>& fileManager = managers->ElementAt(i);
> 
> This can be const I think

yeah, it's now required since I changed the method to be const-safe

> 
> @@ +678,5 @@
> > +
> > +  for (uint32_t i = 0; i < managers->Length(); i++) {
> > +    nsRefPtr<FileManager>& fileManager = managers->ElementAt(i);
> > +
> > +    if (fileManager->DatabaseName().Equals(aName)) {
> 
> Nit: == looks way nicer than Equals(), here and several other places.

fixed

> 
> @@ +693,5 @@
> > +{
> > +  nsTArray<nsRefPtr<FileManager > >* managers =
> > +    GetArrayForPersistenceType(aFileManager->Type());
> > +
> > +  managers->AppendElement(aFileManager);
> 
> I think you should do a DEBUG-only "!managers->Contains(aFileManager)"
> assertion here before appending.

done

> 
> @@ +702,5 @@
> > +{
> > +  uint32_t i;
> > +
> > +  for (i = 0; i < mPermStorageFileManagers.Length(); i++) {
> > +    mPermStorageFileManagers[i]->Invalidate();
> 
> This can't trigger recursion, can it?

no, I don't think so

> 
> @@ +754,5 @@
> > +
> > +    case PERSISTENCE_TYPE_INVALID:
> > +    default:
> > +      NS_NOTREACHED("Bad storage type value!");
> > +      return nullptr;
> 
> Might as well do MOZ_CRASH here.

done (replaced NS_NOTREACHED with MOZ_CRASH)

> 
> ::: dom/indexedDB/nsIIDBDatabase.idl
> @@ +23,5 @@
> >    readonly attribute DOMString name;
> >  
> >    readonly attribute unsigned long long version;
> >  
> > +  readonly attribute DOMString mozStorage;
> 
> The plan going forward (at least as far as I know) is that we will not be
> adding new 'mozFoo' properties or methods any longer. They should be
> unprefixed but hidden behind an 'experimental' pref so that most users don't
> see them. I would suggest "dom.indexedDB.experimental.foo" for this kind of
> thing.

ok, added "dom.indexedDB.experimental.enabled"

> 
> Also I'm not sure that this property in particular is all that useful. Is it
> something you're planning on proposing to the indexedDB mailing list?

yeah, along with IDBFactory changes

> 
> ::: dom/indexedDB/test/test_persistenceType.html
> @@ +8,5 @@
> > +
> > +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> > +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> > +
> > +  <script type="text/javascript;version=1.7">
> 
> Wait, why doesn't this include test_persistenceType.js?

because chrome behaves differently for now 

> 
> ::: dom/ipc/PBrowser.ipdl
> @@ +241,5 @@
> >       */
> >      POfflineCacheUpdate(URIParams manifestURI, URIParams documentURI,
> >                          bool stickDocument);
> >  
> > +    sync PIndexedDB(nsCString group, nsCString asciiOrigin)
> 
> We need a followup to remove this sync message :)

ok, will file a bug for that

> 
> ::: dom/quota/CollectOriginsHelper.cpp
> @@ +49,5 @@
> > +  QuotaManager* quotaManager = QuotaManager::Get();
> > +  NS_ASSERTION(quotaManager, "Shouldn't be null!");
> > +
> > +  mSizeToBeFreed =
> > +    quotaManager->CollectOriginsForEviction(mMinSizeToBeFreed, mOriginInfos);
> 
> This will trigger race detector warnings I think since you're accessing the
> same memory with and without the lock held. I think it would be simpler to
> use stack vars here and then swap them with the members later below once
> you've locked.

ok

> 
> ::: dom/quota/CollectOriginsHelper.h
> @@ +17,5 @@
> > +BEGIN_QUOTA_NAMESPACE
> > +
> > +class OriginInfo;
> > +
> > +class CollectOriginsHelper MOZ_FINAL : public nsIRunnable
> 
> I don't see why you can't put this fully inside QuotaManager.cpp. It doesn't
> need to be exposed anywhere, right?

this class used to be much bigger, but you're right it can now live in
QuotaManager.cpp

> 
> ::: dom/quota/PersistenceType.h
> @@ +40,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +inline nsresult
> > +PersistenceTypeToText(PersistenceType aPersistenceType, nsACString& aText)
> 
> returning bool should be fine

hm, it seems even void should be fine

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +79,5 @@
> > +// On Android, smaller/older devices may have very little storage and
> > +// device owners may be sensitive to storage footprint: Use a smaller
> > +// percentage of available space and a smaller minimum/maximum.
> > +static const uint64_t gTempStorageLimitMin =     10;     //  10 MB
> > +static const uint64_t gTempStorageLimitMax =    200;     // 200 MB
> 
> Nit: These should have "KB" suffixes.

done

> 
> @@ +82,5 @@
> > +static const uint64_t gTempStorageLimitMin =     10;     //  10 MB
> > +static const uint64_t gTempStorageLimitMax =    200;     // 200 MB
> > +static const double   gTempStorageLimitRatio =   .2;     //  20 %
> > +#else
> > +static const uint64_t gTempStorageLimitMin =     50;     //  50 MB
> 
> I think these should probably be overridable via prefs. Some devices may
> need different limits.

done

> 
> @@ +204,5 @@
> >  // files in the origin's directory before dispatching itself back to the main
> >  // thread. When on the main thread the runnable will call the callback and then
> >  // notify the QuotaManager that the job has been completed.
> >  class AsyncUsageRunnable MOZ_FINAL : public UsageRunnable,
> > +                                     public nsRunnable,
> 
> It's really confusing that UsageRunnable isn't actually a runnable. Can you
> rename that while you're here?

yeah, done

> 
> @@ +345,2 @@
> >  void
> >  AssertIsOnIOThread()
> 
> Just make this call IsOnIOThread

nice

> 
> @@ +385,5 @@
> >  QuotaManager* gInstance = nullptr;
> >  int32_t gShutdown = 0;
> >  
> >  int32_t gStorageQuotaMB = DEFAULT_QUOTA_MB;
> > +int32_t gTempStorageLimitKB = -1;
> 
> We're never going to need to care about KB here. Let's use MB everywhere?

I need KB for testing purposes.

> 
> @@ +581,5 @@
> > +
> > +  rv = binaryStream->SetOutputStream(outputStream);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  NS_ADDREF(*aStream = binaryStream);
> 
> binaryStream.forget(aStream)

fixed

> 
> @@ +587,5 @@
> > +}
> > +
> > +nsresult
> > +CreateDirectoryMetadata(nsIFile* aDirectory, int64_t aTimestamp,
> > +                        const nsACString& aGroup, const nsACString& aOrigin)
> 
> You could avoid the timestamp param and just call PR_Now() in here

hm, I need it for InitializeOrigin() too

> 
> @@ +596,5 @@
> > +  nsresult rv =
> > +    GetDirectoryMetadataStream(aDirectory, false, getter_AddRefs(stream));
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  if (!stream) {
> 
> You can assert this.

yeah, it can't be null in this case

> 
> @@ +640,5 @@
> > +
> > +  rv = binaryStream->SetInputStream(bufferedStream);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  PRUint64 timestamp;
> 
> uint64_t. Double check to make sure you didn't add other PR* types in other
> places.

ok

> 
> @@ +643,5 @@
> > +
> > +  PRUint64 timestamp;
> > +  rv = binaryStream->Read64(&timestamp);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  *aTimestamp = timestamp;
> 
> Nit: I'd prefer you not set any out params until you're about to return
> NS_OK.

ok

> 
> @@ +734,5 @@
> > +// space users have free on their hard drive. We use a tiered scheme: the more
> > +// space available, the larger the temp storage will be. However, we do not want
> > +// to enable the temp storage to grow to an unbounded size, so the larger the
> > +// user's available space is, the smaller of a percentage we take. We set a
> > +// lower bound of 50MB and an upper bound of 1GB (10MB/200MB on Android).
> 
> Nit: Mention the constants above here rather than specific amounts since
> they can change.

ok

> 
> @@ +736,5 @@
> > +// to enable the temp storage to grow to an unbounded size, so the larger the
> > +// user's available space is, the smaller of a percentage we take. We set a
> > +// lower bound of 50MB and an upper bound of 1GB (10MB/200MB on Android).
> > +uint64_t
> > +SmartTempStorageLimit(const uint64_t availKB)
> 
> This returns MB right? Add that to the name.
> 
> Nit: aAvailableKB

the method doesn't exist anymore

> 
> @@ +740,5 @@
> > +SmartTempStorageLimit(const uint64_t availKB)
> > +{
> > +  if (availKB > 100 * 1024 * 1024) {
> > +    // skip computing if we're over 100 GB
> > +    return gTempStorageLimitMax * 1024;
> 
> This is a little arbitrary, right? This should somehow be based on the
> constants above or else they won't work as you expect.

hm, I just removed that block

> 
> @@ +743,5 @@
> > +    // skip computing if we're over 100 GB
> > +    return gTempStorageLimitMax * 1024;
> > +  }
> > +
> > +  // Grow/shrink in 10 MB units, deliberately, so that in the common case we
> 
> This sounds like a constant that might need to be different on mobile vs.
> desktop.

ok

> 
> @@ +745,5 @@
> > +  }
> > +
> > +  // Grow/shrink in 10 MB units, deliberately, so that in the common case we
> > +  // don't shrink temp storage and evict origin data every time we initialize.
> > +  uint64_t sz10MBs = 0;
> 
> How about we name this something like 'chunkCount'?

ok, actually we don't need it anymore

> 
> @@ +746,5 @@
> > +
> > +  // Grow/shrink in 10 MB units, deliberately, so that in the common case we
> > +  // don't shrink temp storage and evict origin data every time we initialize.
> > +  uint64_t sz10MBs = 0;
> > +  uint64_t avail10MBs = availKB / (1024 * 10);
> 
> 'availableChunks'?

ok, availableKB

> 
> @@ +755,5 @@
> > +    avail10MBs = 2500;
> > +  }
> > +
> > +  // 1 % of space between 7GB -> 25 GB
> > +  if (avail10MBs > 700) {
> 
> These all need to be "else if". Otherwise you always go into every branch
> and always end up using the smallest amount of space :)

ok, we talked about this ...
This method was heavily based on necko's disk cache size calculation.
I didn't notice it could be simplified so much.
It now looks much better :)

> 
> @@ +769,5 @@
> > +
> > +  // gTempStorageLimitRatio of space up to 500 MB (gTempStorageLimitMin
> > +  // at minimum)
> > +  sz10MBs += std::max<uint64_t>(gTempStorageLimitMin / 10,
> > +                    static_cast<uint64_t>(avail10MBs * gTempStorageLimitRatio));
> 
> Is this meant to be the minimum case? (e.g. the final 'else'?)

yeah

> 
> @@ +785,5 @@
> > +  nsresult rv = aDirectory->GetDiskSpaceAvailable(&bytesAvailable);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  bytesAvailable =
> > +    static_cast<uint64_t>((bytesAvailable + aCurrentUsage) / 1024);
> 
> Maybe first assert that bytesAvailable is >= 0?

ok

> 
> @@ +787,5 @@
> > +
> > +  bytesAvailable =
> > +    static_cast<uint64_t>((bytesAvailable + aCurrentUsage) / 1024);
> > +
> > +  *aLimit = 1024 * SmartTempStorageLimit(bytesAvailable);
> 
> Is there a reason for the two separate functions? Seems like you could avoid
> the unit conversions and just combine the two functions.

merged

> 
> @@ +919,3 @@
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +    rv = permStorageDir->Append(NS_LITERAL_STRING("indexedDB"));
> 
> Can we take this opportunity to fix the storage directory naming? It's
> always been wrong to have "<profile>/indexedDB/<origin>/idb/" paths (my
> fault, sorry).
> 
> Let's change to "<profile>/storage" now before we let the madness spread any
> further.

ok, done

> 
> @@ +927,5 @@
> > +    nsCOMPtr<nsIFile> tempStorageDir;
> > +    rv = baseDir->Clone(getter_AddRefs(tempStorageDir));
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    rv = tempStorageDir->Append(NS_LITERAL_STRING("storageTemp"));
> 
> Hm, I was expecting symmetrical directories:
> 
>   <profile>/storage/permanent/<origin>/idb
>   <profile>/storage/temporary/<origin>/idb

ok, fixed

> 
> @@ +975,5 @@
> >    return NS_OK;
> >  }
> >  
> >  void
> > +QuotaManager::InitQuotaForOrigin(PersistenceType aPersistenceType,
> 
> What thread will this run on?

it could run on any thread in theory, but there's currently only one caller
that always runs on the IO thread, so I made it IO thread only too

> 
> @@ +1009,4 @@
> >  }
> >  
> >  void
> > +QuotaManager::DecreaseUsageForOrigin(PersistenceType aPersistenceType,
> 
> What thread will this run on?

dtto

> 
> @@ +1017,5 @@
> >    MutexAutoLock lock(mQuotaMutex);
> >  
> > +  GroupInfoPair* pair;
> > +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> > +    return;
> 
> This should never happen, right?

No, DOM Blob/File objects may exist after their origin directory got cleared.
There's a time window when their manager is still valid, so we end up calling
QuotaManager()::DecreaseUsageForOrigin from AsyncDeleteFileRunnable::Run()

> 
> @@ +1022,5 @@
> > +  }
> > +
> > +  nsRefPtr<GroupInfo> groupInfo = pair->LockedGetGroupInfo(aPersistenceType);
> > +
> > +  if (!groupInfo) {
> 
> Nit: extra newline

fixed

> 
> @@ +1033,5 @@
> >    }
> >  }
> >  
> >  void
> > +QuotaManager::UpdateOriginAccessTime(PersistenceType aPersistenceType,
> 
> This must always happen on the main thread, right? Needs an assertion.

done

> 
> @@ +1041,5 @@
> > +  MutexAutoLock lock(mQuotaMutex);
> > +
> > +  GroupInfoPair* pair;
> > +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> > +    return;
> 
> This should never happen, right?

No, IDBDatabase objects may exist after their origin directory got cleared...

> 
> @@ +1060,5 @@
> > +      return;
> > +    }
> > +
> > +    {
> > +      MutexAutoUnlock autoUnlock(mQuotaMutex);
> 
> Nit: Braces not needed here, your scope is correct without them.

removed

> 
> @@ +1062,5 @@
> > +
> > +    {
> > +      MutexAutoUnlock autoUnlock(mQuotaMutex);
> > +
> > +      SaveOriginAccessTime(aOrigin, timestamp);
> 
> Hm. Since you're unlocked you are subject to races here (unless this is
> always main thread only)

yeah, this is main thread only

> 
> @@ +1095,5 @@
> > +
> > +  NS_ASSERTION(mTempStorageUsage == 0, "Should be zero!");
> > +}
> > +
> > +struct MOZ_STACK_CLASS RemoveQuotaInfo
> 
> This can go in the anon namespace.

ok

> 
> @@ +1173,5 @@
> >    else {
> >      fileSize = 0;
> >    }
> >  
> > +  QuotaObject* quotaObject = nullptr;
> 
> Nit: No need to initialize this

oh sh..
I removed the initialization and the mochitest stopped working ...
I found out that calling |originInfo->mQuotaObjects.Get(path, &quotaObject)|
and checking quotaObject after the call is wrong in this context, it should be
|quotaObject = originInfo->mQuotaObjects.Get(path)| or the return value should
be checked instead.

fixing nits can be sometimes helpful :)

> 
> @@ +1205,5 @@
> >    }
> >  
> > +  // The caller becomes the owner of the QuotaObject, that is, the caller is
> > +  // is responsible to delete it when the last reference is removed.
> > +  nsRefPtr<QuotaObject> result = quotaObject;
> 
> Why not just stick this above the lock scope and assign to it directly?

QuotaObject::AddRef needs to acquire the same mutex.
Added a comment for that.

> 
> @@ +1611,5 @@
> > +    rv = EnsureDirectory(directory, &created);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    if (created) {
> > +      rv = CreateDirectoryMetadata(directory);
> 
> Why aren't you adding a modified time here? It's bizarre that there are two
> CreateDirectoryMetadata functions and they don't do the same thing...

Hm, the .metadata file for persistent storage is currently empty.
It's only used to indicate that the upgrade (the one where we add "idb" subdir)
successfully finished.

I changed the method name to CreateDirectoryUpgradeStamp()

> 
> @@ +1615,5 @@
> > +      rv = CreateDirectoryMetadata(directory);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +    }
> > +
> > +    if (mInitializedOrigins.Contains(aOrigin)) {
> 
> Shouldn't this early return happen before trying to create directories?

hm, I think this was needed when there was no method to remove entries from
mInitializedOrigins
ok, I put that check first.

> 
> @@ +1626,5 @@
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    mInitializedOrigins.AppendElement(aOrigin);
> > +
> > +    NS_ADDREF(*aDirectory = directory);
> 
> directory.forget()

ok

> 
> @@ +1634,5 @@
> > +  NS_ASSERTION(aPersistenceType == PERSISTENCE_TYPE_TEMPORARY, "Huh?");
> > +  NS_ASSERTION(aTrackQuota, "Huh?");
> > +
> > +  if (!mTempStorageInitialized) {
> > +    mTempStorageInitialized = true;
> 
> So what happens if something fails in here? It looks like the next time
> through it will look like it succeeds, but it won't be initialized
> properly...

ok, the error handling is now done more gracefully

> 
> @@ +1715,5 @@
> > +    if (aOriginOrPattern.IsOrigin()) {
> > +      mInitializedOrigins.RemoveElement(aOriginOrPattern);
> > +    }
> > +    else {
> > +      for (int32_t i = mInitializedOrigins.Length() - 1; i >= 0; i--) {
> 
> First assert that mInitializedOrigins.Length() is less than int32_max

reworked, so the assertion is not needed

> 
> @@ +1762,2 @@
> >  {
> >    nsCString str(aOrigin);
> 
> This should be nsAutoCString since we're doing all this append stuff.

fixed

> 
> @@ +1762,5 @@
> >  {
> >    nsCString str(aOrigin);
> >    str.Append("*");
> >    str.Append(NS_ConvertUTF16toUTF8(aName));
> > +  str.Append("*");
> 
> Can you switch these to use single quotes? s/"/'/

done

> 
> @@ +1800,5 @@
> > +}
> > +
> > +// static
> > +nsresult
> > +QuotaManager::GetInfoFromPrincipal(nsIPrincipal* aPrincipal,
> 
> Need to assert main thread here.

done

> 
> @@ +1879,5 @@
> > +                                         PERMISSION_DEFAUT_PERMANENT_STORAGE,
> > +                                         &permission);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +#if 0
> 
> This should be removed right?

ok

> 
> @@ +1968,5 @@
> >    return NS_OK;
> >  }
> >  
> >  NS_IMETHODIMP
> > +QuotaManager::Clear()
> 
> This is dangerous enough that I think we should guard it with a pref. Then
> you could just set the pref during your testing.

done

> 
> @@ +2042,5 @@
> >    return NS_OK;
> >  }
> >  
> >  NS_IMETHODIMP
> > +QuotaManager::Reset()
> 
> Same here.

done

> 
> @@ +2056,5 @@
> > +  runnable->AdvanceState();
> > +
> > +  // Give the runnable some help by invalidating any storages in the way.
> > +  StorageMatcher<nsAutoTArray<nsIOfflineStorage*, 20> > matches;
> > +    matches.Find(mLiveStorages);
> 
> Nit: indentation

fixed

> 
> @@ +2583,5 @@
> > +    NS_ASSERTION(quotaManager, "Shouldn't be null!");
> > +
> > +    if (groupInfo->mUsage > GROUP_LIMIT(quotaManager->mTempStorageLimit)) {
> > +      nsTArray<OriginInfo*>* doomedOrigins =
> > +        static_cast<nsTArray<OriginInfo*>* >(aUserArg);
> 
> Nit: no space between * and >

removed

> 
> @@ +2618,5 @@
> > +  nsRefPtr<GroupInfo> groupInfo =
> > +    aValue->LockedGetGroupInfo(PERSISTENCE_TYPE_TEMPORARY);
> > +  if (groupInfo) {
> > +    nsTArray<OriginInfo*>* originInfos =
> > +      static_cast<nsTArray<OriginInfo*>* >(aUserArg);
> 
> Nit: No space here either

removed

> 
> @@ +2627,5 @@
> > +  return PL_DHASH_NEXT;
> > +}
> > +
> > +void
> > +QuotaManager::ClearOrigins(nsTArray<OriginInfo*>& aOrigins)
> 
> const nsTArray

ok

> 
> @@ +2633,5 @@
> > +  for (uint32_t index = 0; index < aOrigins.Length(); index++) {
> > +    OriginInfo* originInfo = aOrigins[index];
> > +
> > +    nsCString group = originInfo->mGroupInfo->mGroup;
> > +    nsCString origin = originInfo->mOrigin;
> 
> No need for these stack vars right? They can probably just be refs.

hm, I don't think so
RemoveQuotaForOrigin() removes the originInfo, so it's not possible to use refs

> 
> @@ +2645,5 @@
> > +  }
> > +}
> > +
> > +void
> > +QuotaManager::CheckTempStorageLimits()
> 
> This function does an awful lot of grabbing and dropping the lock. Is there
> no way to streamlne this a bit?

simplified to only two grab/drop pairs

> 
> @@ +2657,5 @@
> > +  }
> > +
> > +  ClearOrigins(originInfos);
> > +
> > +  if (mTempStorageUsage > mTempStorageLimit) {
> 
> How is it safe to check this without holding the lock?

fixed

> 
> @@ +2669,5 @@
> > +    originInfos.Sort(OriginInfoComparator());
> > +
> > +    uint64_t usage = 0;
> > +    for (uint32_t i = 0; i < originInfos.Length(); i++) {
> > +      if (mTempStorageUsage - usage <= mTempStorageLimit) {
> 
> here too...

fixed

> 
> @@ +2670,5 @@
> > +
> > +    uint64_t usage = 0;
> > +    for (uint32_t i = 0; i < originInfos.Length(); i++) {
> > +      if (mTempStorageUsage - usage <= mTempStorageLimit) {
> > +        originInfos.TruncateLength(i);
> 
> Wait, won't this do the opposite of what you want? You're going to end up
> holding on to the oldest origins, not the newest.

Why would I want to clear the newest origins ?

> 
> @@ +2713,5 @@
> > +
> > +  return PL_DHASH_NEXT;
> > +}
> > +
> > +struct InactiveOriginsInfo
> 
> Stick this in the anonymous namespace

ok

> 
> @@ +2815,5 @@
> > +      inactiveOrigins.TruncateLength(index);
> > +      break;
> > +    }
> > +
> > +    OriginInfo* originInfo = inactiveOrigins[index];
> 
> No need for the stack var.

ok

> 
> @@ +2840,5 @@
> > +  return 0;
> > +}
> > +
> > +void
> > +QuotaManager::DeleteFilesForOrigin(const nsACString& aOrigin)
> 
> Nit: Might as well call this 'DeleteTemporaryFilesForOrigin'

ok

> 
> @@ +2863,5 @@
> > +
> > +  nsRefPtr<FinalizeOriginEvictionRunnable> runnable =
> > +    new FinalizeOriginEvictionRunnable(aOrigins);
> > +
> > +  if (IsOnIOThread()) {
> 
> Why would this be false? You already asserted that you're not on the main
> thread

so FinalizeOriginEvictionRunnable can run in two modes:
1. initialized from a background thread (e.g. transaction thread pool)
- dispatch to the main thread
- on the main thread - dispatch to the io thread
- on the io thread - io thread work, dispatch to the main thread
- on the main thread - allow next sync op

AFAIK, I can't dispatch to the IO thread from a background thread directly.

2. initialized from the io thread
- io thread work, dispath to the main thread
- on the main thread - allow next sync op

I reworked the runnable to be more like other runnables that run multiple times
on different threads.

> 
> @@ +3354,5 @@
> > +        continue;
> > +      }
> > +
> > +#ifdef XP_MACOSX
> > +      if (leafName.EqualsLiteral(DSSTORE_FILE_NAME)) {
> 
> I've recently noticed that this shouldn't be mac only since it's
> theoretically possible to share profiles between windows and mac machines.
> Let's remove the ifdefs here and in the origin init code.

done

> 
> @@ +3382,5 @@
> > +      }
> > +
> > +      nsRefPtr<Client>& client = aQuotaManager->mClients[clientType];
> > +
> > +      if (!initialized) {
> 
> Nit: if you handle both then start with 'initialized', not '!initialized'

ok, fixed

> 
> @@ +3618,5 @@
> > +{
> > +  for (uint32_t index = 0; index < mOrigins.Length(); index++) {
> > +    aQuotaManager->OriginClearCompleted(PERSISTENCE_TYPE_TEMPORARY,
> > +                                        OriginOrPatternString::FromOrigin(
> > +                                        mOrigins[index]));
> 
> Nit: indent last line better

done

> 
> ::: dom/quota/QuotaManager.h
> @@ +25,5 @@
> >  #include "StoragePrivilege.h"
> >  
> >  #define QUOTA_MANAGER_CONTRACTID "@mozilla.org/dom/quota/manager;1"
> >  
> > +#define GROUP_LIMIT(x) (x / 5)
> 
> Let's get rid of this. It just makes the code harder to read.

ok

> 
> @@ +52,3 @@
> >  struct SynchronizedOp;
> >  
> > +typedef Nullable<PersistenceType> PersistenceScope;
> 
> I really don't think this helps much. It's not clear what a "persistence
> scope" is, or why it would be different from a "persistence type", so I'd
> suggest removing this typedef and using Nullable<PersistenceType> directly
> in the code.

I must confess that the original idea was to simplify indentation :)

> 
> @@ +281,5 @@
> >  
> >    static already_AddRefed<nsIAtom>
> >    GetStorageId(const nsACString& aOrigin,
> > +               const nsAString& aName,
> > +               PersistenceType aPersistenceType);
> 
> Everywhere else you have type, then (sometimes) group, then origin, then
> name. Let's not deviate :)

you really don't miss anything :)

> 
> ::: dom/quota/QuotaObject.cpp
> @@ +117,5 @@
> > +    uint64_t newGroupUsage = groupInfo->mUsage + delta;
> > +
> > +    // Temporary storage has a hard limit for group usage (20 % of the
> > +    // global limit).
> > +    if (newGroupUsage > GROUP_LIMIT(quotaManager->mTempStorageLimit)) {
> 
> Let's just make GROUP_LIMIT into a small inlined function instead of a macro.

ok

> 
> @@ +164,5 @@
> > +      // all the essential variables and recheck the group limit.
> > +
> > +      delta = end - mSize;
> > +
> > +      newUsage = mOriginInfo->mUsage + delta;
> 
> What guarantees mOriginInfo is still valid here? Could it have gone away
> while you were unlocked?

No, I don't think it could have gone away, so I added an assertion for that.
I also moved |NS_ASSERTION(originInfo != mOriginInfo, "Deleting itself!");| to
a place where the lock is held, just in case ...

> 
> @@ +173,5 @@
> > +        // Unfortunately some other thread increased the group usage in the
> > +        // meantime and we are not below the group limit anymore.
> > +
> > +        // However, the origin eviction must be finalized in this case too.
> > +        {
> 
> Nit: Braces not needed here.

yeah, fixed

> 
> @@ +210,5 @@
> > +
> > +      return true;
> > +    }
> > +
> > +    mOriginInfo->mUsage = newUsage;
> 
> How about you make this block happen first since it's so much shorter? It's
> hard to read through all that above and figure out what conditions apply
> here.

I assume that you meant to swap entire blocks that handle persistent and
temporary storage separately

> 
> ::: dom/quota/QuotaObject.h
> @@ +82,5 @@
> >  
> > +  bool
> > +  operator==(const OriginInfo& aOther) const
> > +  {
> > +    return mAccessTime == aOther.mAccessTime;
> 
> Is this really all you need to check here? I would think you need to at
> least check mOrigin

hm, I removed these operators, the comparison logic is now only in
OriginInfoLRUComparator class

> 
> @@ +249,5 @@
> > +    return mPermStorageGroupInfo || mTempStorageGroupInfo;
> > +  }
> > +
> > +  nsRefPtr<GroupInfo>*
> > +  GetGroupInfoForPersistenceType(PersistenceType aPersistenceType);
> 
> Nit: Let's make this return a reference.

ok

> 
> ::: dom/quota/nsIOfflineStorage.h
> @@ +42,5 @@
> > +  NS_IMETHOD_(PersistenceType)
> > +  Type() = 0;
> > +
> > +  NS_IMETHOD_(const nsACString&)
> > +  Group() = 0;
> 
> Hm, these methods are going to be identical for all subclasses, right? Why
> not just add the members here so that subclasses don't have to provide their
> own identical implementations?

ok

> 
> ::: dom/quota/nsIQuotaManager.idl
> @@ +31,5 @@
> >  
> >    /**
> > +   * Removes all storages. The files may not be deleted immediately depending
> > +   * on prohibitive concurrent operations.
> > +   * Be carefull, this removes all ever stored data!
> 
> Nit:  s/carefull/careful/, and "this removes *all* the data that has ever
> been stored"

fixed

> 
> ::: storage/src/TelemetryVFS.cpp
> @@ +367,5 @@
> > +                                     nsDependentCString(origin),
> > +                                     NS_ConvertUTF8toUTF16(zName));
> > +    }
> > +    else {
> > +      NS_WARNING("Passed wrong persistence type!");
> 
> Do you need to explicitly set p->quotaObject to null here?

I changed the code to always initialize p->quotaObject

> 
> ::: testing/specialpowers/content/specialpowersAPI.js
> @@ +1162,5 @@
> >      Cu.forceCC();
> >    },
> >  
> > +  // Due to various dependencies between JS objects and C++ objects, an ordinary
> > +  // forceGC doesn't necessarily clears all unused objects, thus the GC and CC
> 
> Nit: s/clears/clear/

fixed
Comment 30 [:fabrice] Fabrice Desré 2013-08-12 18:02:58 PDT
Created attachment 789299 [details] [diff] [review]
b2g fix

Jan, this patch fixes the b2g failure on device - I did not send it to try though.
Comment 31 Jan Varga [:janv] 2013-08-13 05:41:44 PDT
Created attachment 789526 [details] [diff] [review]
alternative b2g fix

Fabrice, thanks a lot for tracking this down!
Comment 32 Jan Varga [:janv] 2013-08-13 05:42:40 PDT
https://tbpl.mozilla.org/?tree=Try&rev=06b168f9d6ad
Comment 33 Jan Varga [:janv] 2013-08-13 10:02:32 PDT
Comment on attachment 789299 [details] [diff] [review]
b2g fix

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

I guess there are no devices that need this "upgrade" anymore.
Comment 34 [:fabrice] Fabrice Desré 2013-08-13 10:25:46 PDT
(In reply to Jan Varga [:janv] from comment #33)
> 
> I guess there are no devices that need this "upgrade" anymore.

Yep, we needed that when the directory names for indexedDB changed, but that was a long time ago.
Comment 35 Jan Varga [:janv] 2013-08-15 05:35:53 PDT
Created attachment 790757 [details]
helgrind.log

Ok, here's a helgrind log for:
make xpcshell-tests TEST_PATH=dom/indexedDB/test/unit/test_temporary_storage.js EXTRA_TEST_ARGS='--debugger=valgrind --debugger-args="--tool=helgrind --smc-check=all-non-file" --verbose'

I think there are no problems related to quota or indexedDB module.
However there are many warnings related to the component manager, SafeMutex::mOwnerThread not being protected ?
Comment 36 Jan Varga [:janv] 2013-08-15 05:37:28 PDT
Created attachment 790758 [details] [diff] [review]
valgrind-patch1
Comment 37 Jan Varga [:janv] 2013-08-15 05:38:05 PDT
Created attachment 790759 [details] [diff] [review]
valgrind-patch2
Comment 38 Jan Varga [:janv] 2013-08-15 05:38:34 PDT
Created attachment 790760 [details] [diff] [review]
valgrind-patch3
Comment 39 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-28 10:28:00 PDT
Comment on attachment 790757 [details]
helgrind.log

This looks good!
Comment 40 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-04 22:33:33 PDT
Comment on attachment 789218 [details] [diff] [review]
interdiff v0.3 - v0.4

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

::: dom/indexedDB/Client.cpp
@@ +122,5 @@
>        int64_t fileSize;
>        rv = file->GetFileSize(&fileSize);
>        NS_ENSURE_SUCCESS(rv, rv);
>  
> +      aUsageInfo->AppendToDatabaseUsage(uint64_t(fileSize));

I know this is old but can you assert that fileSize >= 0 before casting?

::: dom/indexedDB/IDBFactory.cpp
@@ +817,5 @@
> +      aRv.ThrowTypeError(MSG_INVALID_VERSION);
> +      return nullptr;
> +    }
> +    version.Construct();
> +    version.Value() = aVersion.Value();

Do you need this stack 'version'? After bailing out for invalid range above why no just pass 'aVersion' straight through?

::: dom/indexedDB/IDBFactory.h
@@ +198,5 @@
> +       const Optional<uint64_t>& aVersion,
> +       const Optional<mozilla::dom::StorageType>& aStorageType, bool aDelete,
> +       ErrorResult& aRv);
> +
> +  // Temp helper, [EnforceRange] is not yet supported in dictionaries)

Nit: This needs some kind of "remove when bug xxx is fixed" comment (and a real bug number of course).

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +80,5 @@
>  
> +  const nsTArray<nsRefPtr<FileManager> >&
> +  GetImmutableArray(PersistenceType aPersistenceType) const
> +  {
> +    return const_cast<FileManagerInfo*>(this)->GetArray(aPersistenceType);

const_cast usually means you're doing something funny... Here I only see you using 'GetImmutableArray' once, and there you should have no problem assigning a non-const ref into a const ref. I think this method can just go away.

::: dom/indexedDB/OpenDatabaseHelper.h
@@ +149,5 @@
>  
>    // State variables
>    enum OpenDatabaseState {
>      eCreated = 0, // Not yet dispatched to the DB thread
> +    eOpenAllowed, // Waiting for open allowed/open allowed

Nit: eOpenPending? eWaitingForOpenAllowed? 'eOpenAllowed' sounds like it has already happened.

::: dom/indexedDB/test/helpers.js
@@ +265,5 @@
> +}
> +
> +function resetExperimental()
> +{
> +  SpecialPowers.setBoolPref(experimentalPref, experimentalEnabled);

This should probably just do clearUserPref and you shouldn't need to store the old value.

::: dom/indexedDB/test/unit/head.js
@@ +194,5 @@
> +}
> +
> +function resetExperimental()
> +{
> +  SpecialPowers.setBoolPref(experimentalPref, experimentalEnabled);

Same here.

::: dom/quota/PersistenceType.h
@@ +39,3 @@
>    }
>  
> +  MOZ_CRASH("Should never get here!");

This can be MOZ_NOT_REACHED

@@ +58,3 @@
>    }
>  
> +  MOZ_CRASH("Should never get here!");

Here too.

@@ +73,2 @@
>    else {
>      return false;

MOZ_ASSERT(false) here?

@@ +77,5 @@
>    return true;
>  }
>  
>  inline bool
>  PersistenceTypeFromText(const char* aText, PersistenceType& aPersistenceType)

I kinda wish we could remove this... Is that possible?

@@ +87,5 @@
> +PersistenceTypeFromStorage(const Optional<mozilla::dom::StorageType>& aStorage,
> +                           PersistenceType aDefaultPersistenceType)
> +{
> +  if (aStorage.WasPassed()) {
> +    static_assert(

Sorry, I meant that the static_assert didn't belong in a method. It belongs in some file-level scope. QuotaManager.cpp should work.

::: dom/quota/QuotaManager.cpp
@@ +65,5 @@
>  // Preference that users can set to override DEFAULT_QUOTA_MB
>  #define PREF_STORAGE_QUOTA "dom.indexedDB.warningQuota"
>  
> +// Preference that users can set to override temporary storage limit calculation
> +#define PREF_TEMPORARY_STORAGE_LIMIT "dom.quotaManager.temporaryStorage.limit"

It's a little weird to have a pref string that could be a branch or could be a value... If "dom.quotaManager.temporaryStorage.limit" is set then there's a hard limit, otherwise if "dom.quotaManager.temporaryStorage.limit.min" is set we do the calculation with that value included? Does this work (e.g. not cause failures below)?

@@ +74,5 @@
> +#define PREF_TEMPORARY_STORAGE_LIMIT_CHUNK PREF_TEMPORARY_STORAGE_LIMIT ".chunk"
> +#define PREF_TEMPORARY_STORAGE_LIMIT_RATIO PREF_TEMPORARY_STORAGE_LIMIT ".ratio"
> +
> +// Preference that is used to enable testing features
> +#define PREF_TESTING_FEATURES "dom.quotaManager.testing.enabled"

Here too "enabled" is redundant. Just "dom.quotaManager.testing" should be fine.

@@ +84,5 @@
>  // origin.
>  #define METADATA_FILE_NAME ".metadata"
>  
> +// Constants for temporary storage limit computing.
> +static const int32_t  gDefaultTemporaryStorageLimitKB =        -1;

Nit: Sorry, missed this before: static consts are 'kFoo', not 'gFoo'.

@@ +89,5 @@
>  #ifdef ANDROID
>  // On Android, smaller/older devices may have very little storage and
>  // device owners may be sensitive to storage footprint: Use a smaller
>  // percentage of available space and a smaller minimum/maximum.
> +static const uint32_t gDefaultTemporaryStorageLimitMinKB =     10 * 1024;

Shouldn't these all live in the anon namespace?

@@ +102,3 @@
>  #endif
>  
> +#define PERMISSION_DEFAUT_PERSISTENT_STORAGE "default-persistent-storage"

Nit: I'd move this up with the other defines.

@@ +137,5 @@
>    nsTArray<nsCOMPtr<nsIRunnable> > mDelayedRunnables;
>    ArrayCluster<nsIOfflineStorage*> mStorages;
>  };
>  
> +class CollectOriginsHelper MOZ_FINAL : public nsIRunnable

Needs a private destructor.

Also, your other runnables inherit nsRunnable, not nsIRunnable. I have a small preference for this style (since it gives you meaningful leak logs), but I don't care too strongly.

@@ +146,5 @@
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE
> +
> +  int64_t
> +  WaitAndReturnOriginsForEviction(nsTArray<OriginInfo*>& aOriginInfos);

Nit: 'Block' is scarier-sounding than 'Wait', maybe switch to that?

Also might want to comment what the returned value is supposed to mean since it's not really obvious from the method name.

@@ +151,5 @@
> +
> +private:
> +  uint64_t mMinSizeToBeFreed;
> +
> +  mozilla::Mutex& mMutex;

Please comment what is protected with this... "The members below" maybe?

@@ +424,5 @@
> +      case IO:
> +        mCallbackState = Complete;
> +        return;
> +      default:
> +        NS_NOTREACHED("Can't advance past Complete!");

Nit: MOZ_NOT_REACHED for new code.

@@ +567,5 @@
> +  : mCollection(aCollection), mOrigins(aOrigins)
> +  { }
> +
> +  OriginCollection& mCollection;
> +  nsTArray<OriginInfo*>& mOrigins;

Nit: You've got 'mFoo' members here, and in your other struct above you have 'foo' members. Pick one style ;)

@@ +863,5 @@
> +  // 1 % of space between 7GB -> 25 GB
> +  // 5 % of space between 500 MB -> 7 GB
> +  // gTemporaryStorageLimitRatio of space up to 500 MB
> +
> +#define _25GB (25 * 1024 * 1024)

Nit: Maybe add 'ul' to all of these?

@@ +865,5 @@
> +  // gTemporaryStorageLimitRatio of space up to 500 MB
> +
> +#define _25GB (25 * 1024 * 1024)
> +#define _7GB (7 * 1024 * 1024)
> +#define _500MB (500 * 1024)

Nit: I think this is technically fine but leading underscores are kinda confusing (leading underscores with a capital letter are C++ reserved, for instance) so I'd just lose them.

@@ +886,5 @@
> +      PERCENTAGE(_500MB, 0, gTemporaryStorageLimitRatio)
> +    );
> +  }
> +
> +  else if (availableKB > _500MB) {

Nit: Extra newline above here.

@@ +903,5 @@
> +#undef _7GB
> +#undef _500MB
> +#undef PERCENTAGE
> +
> +  if (resultKB < gTemporaryStorageLimitMinKB) {

Use mozilla::clamped here.

@@ +1096,1 @@
>      NS_WARNING("Unable to respond to temp storage pref changes!");

This will probably fail if you have any of the min/max/etc settings, so warning isn't appropriate here.

@@ +1096,2 @@
>      NS_WARNING("Unable to respond to temp storage pref changes!");
> +    gTemporaryStorageLimitKB = gDefaultTemporaryStorageLimitKB;

All of these are already initialized properly so this is unnecessary. Since you're not warning here either I think you could just not check the result.

@@ +1096,5 @@
>      NS_WARNING("Unable to respond to temp storage pref changes!");
> +    gTemporaryStorageLimitKB = gDefaultTemporaryStorageLimitKB;
> +  }
> +
> +  rv = Preferences::AddUintVarCache(&gTemporaryStorageLimitMinKB,

Warnings probably are appropriate here, so I think you can shorten this whole block up like so:

  if (NS_FAILED(Preferences::AddUintVarCache(a, b, c)) ||
      NS_FAILED(Preferences::AddUintVarCache(d, e, f)) /* etc */) {
    NS_WARNING("Unable to respond to temp storage pref changes!");
  }

@@ +1794,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +QuotaManager::EnsureStorageAreaIsInitialized()

I'd rename this to something more descriptive... UpgradeIndexedDBDir? ConvertStorageDir? Something else.

@@ +1817,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!exists) {
> +    // Nothing to upgrade.
> +    return NS_OK;

You should set mStorageAreaInitialized true here.

@@ +2247,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
>  QuotaManager::Clear()

Assert main thread, for Reset too.

@@ +2250,5 @@
>  NS_IMETHODIMP
>  QuotaManager::Clear()
>  {
> +  if (!gTestingEnabled) {
> +    return NS_OK;

I'd warn here. And in Reset.

@@ +3343,5 @@
> +  mCondVar(aMutex, "CollectOriginsHelper::mCondVar"),
> +  mSizeToBeFreed(0),
> +  mWaiting(true)
> +{
> +  NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");

Nit: MOZ_ASSERT for new code.

@@ +3385,5 @@
> +
> +  mOriginInfos.SwapElements(originInfos);
> +  mSizeToBeFreed = sizeToBeFreed;
> +  mWaiting = false;
> +  mCondVar.NotifyAll();

Just Notify(), there should be only one thread waiting.

::: dom/quota/QuotaManager.h
@@ +277,5 @@
> +    return mTemporaryStoragePath;
> +  }
> +
> +  uint64_t
> +  GetGroupLimit()

Nit: const

::: dom/quota/QuotaObject.cpp
@@ +153,1 @@
>      groupInfo->mUsage = newGroupUsage;

Nit: No need for the temporary stack value.

@@ +178,5 @@
> +  uint64_t newTemporaryStorageUsage = quotaManager->mTemporaryStorageUsage +
> +                                      delta;
> +
> +  if (newTemporaryStorageUsage > quotaManager->mTemporaryStorageLimit) {
> +    // This will block the thread without holding the lock while waitting.

What does this mean? The comment basically describes a busy wait or IO wait... Maybe this comment needs to be moved to where you actually block?

@@ +180,5 @@
> +
> +  if (newTemporaryStorageUsage > quotaManager->mTemporaryStorageLimit) {
> +    // This will block the thread without holding the lock while waitting.
> +
> +    nsTArray<OriginInfo*> originInfos;

Maybe make this an auto array, with like 10 elements or something?

@@ +231,5 @@
> +    newGroupUsage = groupInfo->mUsage + delta;
> +
> +    if (newGroupUsage > quotaManager->GetGroupLimit()) {
> +      // Unfortunately some other thread increased the group usage in the
> +      // meantime and we are not below the group limit anymore.

Don't we need to retry this process then? Otherwise we're going to fail here.

Maybe we can worry about this in a followup.

::: dom/quota/QuotaObject.h
@@ +79,5 @@
>    }
>  
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(OriginInfo)
>  
> +  int64_t AccessTime() const

Nit: return type on its own line.

@@ +122,3 @@
>  {
>  public:
>    bool Equals(const OriginInfo* a, const OriginInfo* b) const {

Nit: return type and { on their own lines, here and for LessThan

@@ +152,5 @@
>  
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(GroupInfo)
>  
>    bool
> +  IsForPersistentStorage()

Nit: This can be const, IsForTemporaryStorage too.

::: dom/quota/nsIOfflineStorage.h
@@ +36,5 @@
> +  nsIOfflineStorage()
> +  : mPersistenceType(mozilla::dom::quota::PERSISTENCE_TYPE_INVALID)
> +  { }
> +
> +  virtual ~nsIOfflineStorage()

This is refcounted and should therefore be protected. I think your constructor should be too.

::: dom/webidl/IDBDatabase.webidl
@@ +43,5 @@
>                  attribute EventHandler       onversionchange;
>  };
>  
>  partial interface IDBDatabase {
> +    [Pref="dom.indexedDB.experimental.enabled"]

I'm thinking we should have 'dom.indexedDB.experimental' if we want just one pref for all experimental stuff ('enabled' is redundant). Otherwise we should have 'dom.indexedDB.experimental.temporaryStorage'if we want to try a more limited scope.

What do you think? Are we ever going to have a ton of experimental things that we might want to only selectively enable? I kinda think no...

@@ +44,5 @@
>  };
>  
>  partial interface IDBDatabase {
> +    [Pref="dom.indexedDB.experimental.enabled"]
> +    readonly    attribute DOMString          storage;

Shouldn't this be 'storageType'?

::: storage/src/TelemetryVFS.cpp
@@ +371,5 @@
>      else {
>        NS_WARNING("Passed wrong persistence type!");
>      }
>    }
> +  quotaObject.swap(p->quotaObject);

I can't quite figure out what this is about... Why the extra stack pointer here? Something to do with the funny locked refcount?

This needs commenting if it's really needed.
Comment 41 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-04 22:34:10 PDT
Comment on attachment 789216 [details] [diff] [review]
patch v0.4

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

r=me with previous comments addressed. Thanks for your patience here!
Comment 42 Jan Varga [:janv] 2013-09-05 11:29:06 PDT
Ok, thanks for the review!
Before I send a new patch, can you clarify some of your comments ?

> ::: dom/indexedDB/IDBFactory.cpp
> @@ +817,5 @@
> > +      aRv.ThrowTypeError(MSG_INVALID_VERSION);
> > +      return nullptr;
> > +    }
> > +    version.Construct();
> > +    version.Value() = aVersion.Value();
> 
> Do you need this stack 'version'? After bailing out for invalid range above
> why no just pass 'aVersion' straight through?

That would lead to infinite recursion, I need to call the other Open() method
with |const Optional<uint64_t>& aVersion| argument.
I'm not sure if it is possible to use some kind of casting here.

> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +80,5 @@
> >  
> > +  const nsTArray<nsRefPtr<FileManager> >&
> > +  GetImmutableArray(PersistenceType aPersistenceType) const
> > +  {
> > +    return const_cast<FileManagerInfo*>(this)->GetArray(aPersistenceType);
> 
> const_cast usually means you're doing something funny... Here I only see you
> using 'GetImmutableArray' once, and there you should have no problem
> assigning a non-const ref into a const ref. I think this method can just go
> away.

If I change:
const nsTArray<nsRefPtr<FileManager> >& managers =
    GetImmutableArray(aPersistenceType);

to:

const nsTArray<nsRefPtr<FileManager> >& managers =
    GetArray(aPersistenceType);

I get this error:
/Users/varga/Sources/Mozilla/mozilla-central/dom/indexedDB/IndexedDatabaseManager.cpp:689:5: error: 
      member function 'GetArray' not viable: 'this' argument has type 'const
      mozilla::dom::indexedDB::FileManagerInfo', but function is not marked
      const
    GetArray(aPersistenceType);
    ^~~~~~~~
/Users/varga/Sources/Mozilla/mozilla-central/dom/indexedDB/IndexedDatabaseManager.cpp:78:3: note: 
      'GetArray' declared here
  GetArray(PersistenceType aPersistenceType);
  ^
1 error generated.

> ::: dom/quota/PersistenceType.h
> @@ +39,3 @@
> >    }
> >  
> > +  MOZ_CRASH("Should never get here!");
> 
> This can be MOZ_NOT_REACHED

hm, Justin changed all MOZ_NOT_REACHED to MOZ_CRASH in IndexedDBParent
Bug 802686 - s/MOZ_NOT_REACHED/MOZ_CRASH/

I don't know why, I can't read that bug.

> @@ +865,5 @@
> > +  // gTemporaryStorageLimitRatio of space up to 500 MB
> > +
> > +#define _25GB (25 * 1024 * 1024)
> > +#define _7GB (7 * 1024 * 1024)
> > +#define _500MB (500 * 1024)
> 
> Nit: I think this is technically fine but leading underscores are kinda
> confusing (leading underscores with a capital letter are C++ reserved, for
> instance) so I'd just lose them.

If I change it to:
#define 25GB (25 * 1024 * 1024)

I get this error:
/Users/varga/Sources/Mozilla/mozilla-central/dom/quota/QuotaManager.cpp:867:9: error: 
      macro names must be identifiers

#define 25GB (25 * 1024 * 1024)


> @@ +231,5 @@
> > +    newGroupUsage = groupInfo->mUsage + delta;
> > +
> > +    if (newGroupUsage > quotaManager->GetGroupLimit()) {
> > +      // Unfortunately some other thread increased the group usage in the
> > +      // meantime and we are not below the group limit anymore.
> 
> Don't we need to retry this process then? Otherwise we're going to fail here.

When we reach the group limit we just return the quota exceeded error.
We try to delete other origins only if we are under the group limit.
So I don't think it makes sense to retry the process.

> ::: storage/src/TelemetryVFS.cpp
> @@ +371,5 @@
> >      else {
> >        NS_WARNING("Passed wrong persistence type!");
> >      }
> >    }
> > +  quotaObject.swap(p->quotaObject);
> 
> I can't quite figure out what this is about... Why the extra stack pointer
> here? Something to do with the funny locked refcount?
> 
> This needs commenting if it's really needed.

your previous comment was:
::: storage/src/TelemetryVFS.cpp
@@ +367,5 @@
> +                                     nsDependentCString(origin),
> +                                     NS_ConvertUTF8toUTF16(zName));
> +    }
> +    else {
> +      NS_WARNING("Passed wrong persistence type!");

Do you need to explicitly set p->quotaObject to null here?

and my reply:
I changed the code to always initialize p->quotaObject

so, p->quotaObject is always initialized either with nullptr or real quota object pointer
Comment 43 Jan Varga [:janv] 2013-09-06 12:26:36 PDT
(In reply to Jan Varga [:janv] from comment #42)
> > ::: storage/src/TelemetryVFS.cpp
> > @@ +371,5 @@
> > >      else {
> > >        NS_WARNING("Passed wrong persistence type!");
> > >      }
> > >    }
> > > +  quotaObject.swap(p->quotaObject);
> > 
> > I can't quite figure out what this is about... Why the extra stack pointer
> > here? Something to do with the funny locked refcount?
> > 
> > This needs commenting if it's really needed.
> 
> your previous comment was:
> ::: storage/src/TelemetryVFS.cpp
> @@ +367,5 @@
> > +                                     nsDependentCString(origin),
> > +                                     NS_ConvertUTF8toUTF16(zName));
> > +    }
> > +    else {
> > +      NS_WARNING("Passed wrong persistence type!");
> 
> Do you need to explicitly set p->quotaObject to null here?
> 
> and my reply:
> I changed the code to always initialize p->quotaObject
> 
> so, p->quotaObject is always initialized either with nullptr or real quota
> object pointer

never mind, entire block can be simplified (w/o using the extra stack pointer)
Comment 44 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-09 05:26:36 PDT
(In reply to Jan Varga [:janv] from comment #42)
> That would lead to infinite recursion, I need to call the other Open() method
> with |const Optional<uint64_t>& aVersion| argument.

Just pass 'aVersion.Value()'.

> If I change:
> const nsTArray<nsRefPtr<FileManager> >& managers =
>     GetImmutableArray(aPersistenceType);

Sigh. Oh well :(

> > This can be MOZ_NOT_REACHED

MOZ_ASSUME_UNREACHABLE.

> If I change it to:
> #define 25GB (25 * 1024 * 1024)

Yeah, the underscore is fine I guess.
Comment 45 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-09-09 05:27:07 PDT
(In reply to Jan Varga [:janv] from comment #43)
> never mind, entire block can be simplified (w/o using the extra stack
> pointer)

Great!
Comment 46 Jan Varga [:janv] 2013-09-09 08:48:10 PDT
https://tbpl.mozilla.org/?tree=Try&rev=a40ed3235627
Comment 47 Jan Varga [:janv] 2013-09-09 10:55:15 PDT
Created attachment 801685 [details] [diff] [review]
patch v0.5
Comment 48 Jan Varga [:janv] 2013-09-09 10:57:52 PDT
Created attachment 801688 [details] [diff] [review]
interdiff v0.4 - v0.5
Comment 49 Jan Varga [:janv] 2013-09-09 11:01:03 PDT
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #40)
> Comment on attachment 789218 [details] [diff] [review]
> interdiff v0.3 - v0.4
> 
> Review of attachment 789218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/Client.cpp
> @@ +122,5 @@
> >        int64_t fileSize;
> >        rv = file->GetFileSize(&fileSize);
> >        NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +      aUsageInfo->AppendToDatabaseUsage(uint64_t(fileSize));
> 
> I know this is old but can you assert that fileSize >= 0 before casting?

ok

> 
> ::: dom/indexedDB/IDBFactory.cpp
> @@ +817,5 @@
> > +      aRv.ThrowTypeError(MSG_INVALID_VERSION);
> > +      return nullptr;
> > +    }
> > +    version.Construct();
> > +    version.Value() = aVersion.Value();
> 
> Do you need this stack 'version'? After bailing out for invalid range above
> why no just pass 'aVersion' straight through?

left unchanged

> 
> ::: dom/indexedDB/IDBFactory.h
> @@ +198,5 @@
> > +       const Optional<uint64_t>& aVersion,
> > +       const Optional<mozilla::dom::StorageType>& aStorageType, bool aDelete,
> > +       ErrorResult& aRv);
> > +
> > +  // Temp helper, [EnforceRange] is not yet supported in dictionaries)
> 
> Nit: This needs some kind of "remove when bug xxx is fixed" comment (and a
> real bug number of course).

filed bug 912948 and added the comment

> 
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +80,5 @@
> >  
> > +  const nsTArray<nsRefPtr<FileManager> >&
> > +  GetImmutableArray(PersistenceType aPersistenceType) const
> > +  {
> > +    return const_cast<FileManagerInfo*>(this)->GetArray(aPersistenceType);
> 
> const_cast usually means you're doing something funny... Here I only see you
> using 'GetImmutableArray' once, and there you should have no problem
> assigning a non-const ref into a const ref. I think this method can just go
> away.

left unchanged

> 
> ::: dom/indexedDB/OpenDatabaseHelper.h
> @@ +149,5 @@
> >  
> >    // State variables
> >    enum OpenDatabaseState {
> >      eCreated = 0, // Not yet dispatched to the DB thread
> > +    eOpenAllowed, // Waiting for open allowed/open allowed
> 
> Nit: eOpenPending? eWaitingForOpenAllowed? 'eOpenAllowed' sounds like it has
> already happened.

ok, eOpenPending sounds good

> 
> ::: dom/indexedDB/test/helpers.js
> @@ +265,5 @@
> > +}
> > +
> > +function resetExperimental()
> > +{
> > +  SpecialPowers.setBoolPref(experimentalPref, experimentalEnabled);
> 
> This should probably just do clearUserPref and you shouldn't need to store
> the old value.

ok

> 
> ::: dom/indexedDB/test/unit/head.js
> @@ +194,5 @@
> > +}
> > +
> > +function resetExperimental()
> > +{
> > +  SpecialPowers.setBoolPref(experimentalPref, experimentalEnabled);
> 
> Same here.

ok

> 
> ::: dom/quota/PersistenceType.h
> @@ +39,3 @@
> >    }
> >  
> > +  MOZ_CRASH("Should never get here!");
> 
> This can be MOZ_NOT_REACHED

ok, replaced with MOZ_ASSUME_UNREACHABLE

> 
> @@ +58,3 @@
> >    }
> >  
> > +  MOZ_CRASH("Should never get here!");
> 
> Here too.

ok

> 
> @@ +73,2 @@
> >    else {
> >      return false;
> 
> MOZ_ASSERT(false) here?

hmm, we can actually make it return PersistenceType directly

> 
> @@ +77,5 @@
> >    return true;
> >  }
> >  
> >  inline bool
> >  PersistenceTypeFromText(const char* aText, PersistenceType& aPersistenceType)
> 
> I kinda wish we could remove this... Is that possible?

yeah, removed

> 
> @@ +87,5 @@
> > +PersistenceTypeFromStorage(const Optional<mozilla::dom::StorageType>& aStorage,
> > +                           PersistenceType aDefaultPersistenceType)
> > +{
> > +  if (aStorage.WasPassed()) {
> > +    static_assert(
> 
> Sorry, I meant that the static_assert didn't belong in a method. It belongs
> in some file-level scope. QuotaManager.cpp should work.

done

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +65,5 @@
> >  // Preference that users can set to override DEFAULT_QUOTA_MB
> >  #define PREF_STORAGE_QUOTA "dom.indexedDB.warningQuota"
> >  
> > +// Preference that users can set to override temporary storage limit calculation
> > +#define PREF_TEMPORARY_STORAGE_LIMIT "dom.quotaManager.temporaryStorage.limit"
> 
> It's a little weird to have a pref string that could be a branch or could be
> a value... If "dom.quotaManager.temporaryStorage.limit" is set then there's
> a hard limit, otherwise if "dom.quotaManager.temporaryStorage.limit.min" is
> set we do the calculation with that value included? Does this work (e.g. not
> cause failures below)?

I've decided to split it:
dom.quotaManager.temporaryStorage.limit ->
  dom.quotaManager.temporaryStorage.fixedLimit
dom.quotaManager.temporaryStorage.limit.min/max/ratio/chunk ->
  dom.quotaManager.temporaryStorage.smartLimit.min/max/ratio/chunk

> 
> @@ +74,5 @@
> > +#define PREF_TEMPORARY_STORAGE_LIMIT_CHUNK PREF_TEMPORARY_STORAGE_LIMIT ".chunk"
> > +#define PREF_TEMPORARY_STORAGE_LIMIT_RATIO PREF_TEMPORARY_STORAGE_LIMIT ".ratio"
> > +
> > +// Preference that is used to enable testing features
> > +#define PREF_TESTING_FEATURES "dom.quotaManager.testing.enabled"
> 
> Here too "enabled" is redundant. Just "dom.quotaManager.testing" should be
> fine.

ok

> 
> @@ +84,5 @@
> >  // origin.
> >  #define METADATA_FILE_NAME ".metadata"
> >  
> > +// Constants for temporary storage limit computing.
> > +static const int32_t  gDefaultTemporaryStorageLimitKB =        -1;
> 
> Nit: Sorry, missed this before: static consts are 'kFoo', not 'gFoo'.

yeah, fixed

> 
> @@ +89,5 @@
> >  #ifdef ANDROID
> >  // On Android, smaller/older devices may have very little storage and
> >  // device owners may be sensitive to storage footprint: Use a smaller
> >  // percentage of available space and a smaller minimum/maximum.
> > +static const uint32_t gDefaultTemporaryStorageLimitMinKB =     10 * 1024;
> 
> Shouldn't these all live in the anon namespace?

ok, moved

> 
> @@ +102,3 @@
> >  #endif
> >  
> > +#define PERMISSION_DEFAUT_PERSISTENT_STORAGE "default-persistent-storage"
> 
> Nit: I'd move this up with the other defines.

ok

> 
> @@ +137,5 @@
> >    nsTArray<nsCOMPtr<nsIRunnable> > mDelayedRunnables;
> >    ArrayCluster<nsIOfflineStorage*> mStorages;
> >  };
> >  
> > +class CollectOriginsHelper MOZ_FINAL : public nsIRunnable
> 
> Needs a private destructor.

ok

> 
> Also, your other runnables inherit nsRunnable, not nsIRunnable. I have a
> small preference for this style (since it gives you meaningful leak logs),
> but I don't care too strongly.

ah, ok

> 
> @@ +146,5 @@
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> > +  NS_DECL_NSIRUNNABLE
> > +
> > +  int64_t
> > +  WaitAndReturnOriginsForEviction(nsTArray<OriginInfo*>& aOriginInfos);
> 
> Nit: 'Block' is scarier-sounding than 'Wait', maybe switch to that?

ok

> 
> Also might want to comment what the returned value is supposed to mean since
> it's not really obvious from the method name.

done

> 
> @@ +151,5 @@
> > +
> > +private:
> > +  uint64_t mMinSizeToBeFreed;
> > +
> > +  mozilla::Mutex& mMutex;
> 
> Please comment what is protected with this... "The members below" maybe?

done

> 
> @@ +424,5 @@
> > +      case IO:
> > +        mCallbackState = Complete;
> > +        return;
> > +      default:
> > +        NS_NOTREACHED("Can't advance past Complete!");
> 
> Nit: MOZ_NOT_REACHED for new code.

ok, replaced with MOZ_ASSUME_UNREACHABLE

> 
> @@ +567,5 @@
> > +  : mCollection(aCollection), mOrigins(aOrigins)
> > +  { }
> > +
> > +  OriginCollection& mCollection;
> > +  nsTArray<OriginInfo*>& mOrigins;
> 
> Nit: You've got 'mFoo' members here, and in your other struct above you have
> 'foo' members. Pick one style ;)

fixed

> 
> @@ +863,5 @@
> > +  // 1 % of space between 7GB -> 25 GB
> > +  // 5 % of space between 500 MB -> 7 GB
> > +  // gTemporaryStorageLimitRatio of space up to 500 MB
> > +
> > +#define _25GB (25 * 1024 * 1024)
> 
> Nit: Maybe add 'ul' to all of these?

hmm, I converted them to typed static const values

> 
> @@ +865,5 @@
> > +  // gTemporaryStorageLimitRatio of space up to 500 MB
> > +
> > +#define _25GB (25 * 1024 * 1024)
> > +#define _7GB (7 * 1024 * 1024)
> > +#define _500MB (500 * 1024)
> 
> Nit: I think this is technically fine but leading underscores are kinda
> confusing (leading underscores with a capital letter are C++ reserved, for
> instance) so I'd just lose them.

left unchanged

> 
> @@ +886,5 @@
> > +      PERCENTAGE(_500MB, 0, gTemporaryStorageLimitRatio)
> > +    );
> > +  }
> > +
> > +  else if (availableKB > _500MB) {
> 
> Nit: Extra newline above here.

removed

> 
> @@ +903,5 @@
> > +#undef _7GB
> > +#undef _500MB
> > +#undef PERCENTAGE
> > +
> > +  if (resultKB < gTemporaryStorageLimitMinKB) {
> 
> Use mozilla::clamped here.

done

> 
> @@ +1096,1 @@
> >      NS_WARNING("Unable to respond to temp storage pref changes!");
> 
> This will probably fail if you have any of the min/max/etc settings, so
> warning isn't appropriate here.

I kept the warning since I split the prefs

> 
> @@ +1096,2 @@
> >      NS_WARNING("Unable to respond to temp storage pref changes!");
> > +    gTemporaryStorageLimitKB = gDefaultTemporaryStorageLimitKB;
> 
> All of these are already initialized properly so this is unnecessary. Since
> you're not warning here either I think you could just not check the result.

ok, removed the extra initialization

> 
> @@ +1096,5 @@
> >      NS_WARNING("Unable to respond to temp storage pref changes!");
> > +    gTemporaryStorageLimitKB = gDefaultTemporaryStorageLimitKB;
> > +  }
> > +
> > +  rv = Preferences::AddUintVarCache(&gTemporaryStorageLimitMinKB,
> 
> Warnings probably are appropriate here, so I think you can shorten this
> whole block up like so:
> 
>   if (NS_FAILED(Preferences::AddUintVarCache(a, b, c)) ||
>       NS_FAILED(Preferences::AddUintVarCache(d, e, f)) /* etc */) {
>     NS_WARNING("Unable to respond to temp storage pref changes!");
>   }

done

> 
> @@ +1794,5 @@
> >    return NS_OK;
> >  }
> >  
> >  nsresult
> > +QuotaManager::EnsureStorageAreaIsInitialized()
> 
> I'd rename this to something more descriptive... UpgradeIndexedDBDir?
> ConvertStorageDir? Something else.

ok, renamed to MaybeUpgradeIndexedDBDirectory

> 
> @@ +1817,5 @@
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  if (!exists) {
> > +    // Nothing to upgrade.
> > +    return NS_OK;
> 
> You should set mStorageAreaInitialized true here.

ah, good catch

> 
> @@ +2247,5 @@
> >    return NS_OK;
> >  }
> >  
> >  NS_IMETHODIMP
> >  QuotaManager::Clear()
> 
> Assert main thread, for Reset too.

ok

> 
> @@ +2250,5 @@
> >  NS_IMETHODIMP
> >  QuotaManager::Clear()
> >  {
> > +  if (!gTestingEnabled) {
> > +    return NS_OK;
> 
> I'd warn here. And in Reset.

ok

> 
> @@ +3343,5 @@
> > +  mCondVar(aMutex, "CollectOriginsHelper::mCondVar"),
> > +  mSizeToBeFreed(0),
> > +  mWaiting(true)
> > +{
> > +  NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
> 
> Nit: MOZ_ASSERT for new code.

ok

> 
> @@ +3385,5 @@
> > +
> > +  mOriginInfos.SwapElements(originInfos);
> > +  mSizeToBeFreed = sizeToBeFreed;
> > +  mWaiting = false;
> > +  mCondVar.NotifyAll();
> 
> Just Notify(), there should be only one thread waiting.

ah, yes

> 
> ::: dom/quota/QuotaManager.h
> @@ +277,5 @@
> > +    return mTemporaryStoragePath;
> > +  }
> > +
> > +  uint64_t
> > +  GetGroupLimit()
> 
> Nit: const

fixed

> 
> ::: dom/quota/QuotaObject.cpp
> @@ +153,1 @@
> >      groupInfo->mUsage = newGroupUsage;
> 
> Nit: No need for the temporary stack value.

fixed

> 
> @@ +178,5 @@
> > +  uint64_t newTemporaryStorageUsage = quotaManager->mTemporaryStorageUsage +
> > +                                      delta;
> > +
> > +  if (newTemporaryStorageUsage > quotaManager->mTemporaryStorageLimit) {
> > +    // This will block the thread without holding the lock while waitting.
> 
> What does this mean? The comment basically describes a busy wait or IO
> wait... Maybe this comment needs to be moved to where you actually block?

That comment is a shortened version of:
"This will block the thread, but it will also drop the mutex while
waiting. The mutex will be reacquired again when the waiting is
finished."
which we have before a similar call for persistent storage:
quotaManager->LockedQuotaIsLifted()

My intention was to say that the method doesn't hold the lock, so after the call
the temp variables aren't valid anymore and need to be recalculated.

> 
> @@ +180,5 @@
> > +
> > +  if (newTemporaryStorageUsage > quotaManager->mTemporaryStorageLimit) {
> > +    // This will block the thread without holding the lock while waitting.
> > +
> > +    nsTArray<OriginInfo*> originInfos;
> 
> Maybe make this an auto array, with like 10 elements or something?

ok

> 
> @@ +231,5 @@
> > +    newGroupUsage = groupInfo->mUsage + delta;
> > +
> > +    if (newGroupUsage > quotaManager->GetGroupLimit()) {
> > +      // Unfortunately some other thread increased the group usage in the
> > +      // meantime and we are not below the group limit anymore.
> 
> Don't we need to retry this process then? Otherwise we're going to fail here.

left unchanged

> 
> Maybe we can worry about this in a followup.
> 
> ::: dom/quota/QuotaObject.h
> @@ +79,5 @@
> >    }
> >  
> >    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(OriginInfo)
> >  
> > +  int64_t AccessTime() const
> 
> Nit: return type on its own line.

fixed

> 
> @@ +122,3 @@
> >  {
> >  public:
> >    bool Equals(const OriginInfo* a, const OriginInfo* b) const {
> 
> Nit: return type and { on their own lines, here and for LessThan

fixed

> 
> @@ +152,5 @@
> >  
> >    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(GroupInfo)
> >  
> >    bool
> > +  IsForPersistentStorage()
> 
> Nit: This can be const, IsForTemporaryStorage too.

fixed

> 
> ::: dom/quota/nsIOfflineStorage.h
> @@ +36,5 @@
> > +  nsIOfflineStorage()
> > +  : mPersistenceType(mozilla::dom::quota::PERSISTENCE_TYPE_INVALID)
> > +  { }
> > +
> > +  virtual ~nsIOfflineStorage()
> 
> This is refcounted and should therefore be protected. I think your
> constructor should be too.

ok

> 
> ::: dom/webidl/IDBDatabase.webidl
> @@ +43,5 @@
> >                  attribute EventHandler       onversionchange;
> >  };
> >  
> >  partial interface IDBDatabase {
> > +    [Pref="dom.indexedDB.experimental.enabled"]
> 
> I'm thinking we should have 'dom.indexedDB.experimental' if we want just one
> pref for all experimental stuff ('enabled' is redundant). Otherwise we
> should have 'dom.indexedDB.experimental.temporaryStorage'if we want to try a
> more limited scope.
> 
> What do you think? Are we ever going to have a ton of experimental things
> that we might want to only selectively enable? I kinda think no...

ok, one pref should be enough

> 
> @@ +44,5 @@
> >  };
> >  
> >  partial interface IDBDatabase {
> > +    [Pref="dom.indexedDB.experimental.enabled"]
> > +    readonly    attribute DOMString          storage;
> 
> Shouldn't this be 'storageType'?

yeah, I converted it to the enum

> 
> ::: storage/src/TelemetryVFS.cpp
> @@ +371,5 @@
> >      else {
> >        NS_WARNING("Passed wrong persistence type!");
> >      }
> >    }
> > +  quotaObject.swap(p->quotaObject);
> 
> I can't quite figure out what this is about... Why the extra stack pointer
> here? Something to do with the funny locked refcount?
> 
> This needs commenting if it's really needed.

removed the extra stack pointer
Comment 50 Jan Varga [:janv] 2013-09-10 00:36:59 PDT
Created attachment 802116 [details] [diff] [review]
Add [EnforceRange] to IDBOpenDBOptions dictionary

Support for [EnforceRange] in dictionaries has already landed on inbound.
Comment 52 Jan Varga [:janv] 2013-09-10 23:49:41 PDT
It seems windows requires clobbering.

https://hg.mozilla.org/integration/mozilla-inbound/rev/246ad6d960f4
Comment 53 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2013-09-11 12:05:37 PDT
(In reply to Jan Varga [:janv] from comment #52)
> It seems windows requires clobbering.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/246ad6d960f4

Please file a bug on this.
Comment 54 Jan Varga [:janv] 2013-09-11 12:26:25 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #53)
> (In reply to Jan Varga [:janv] from comment #52)
> > It seems windows requires clobbering.
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/246ad6d960f4
> 
> Please file a bug on this.

filed bug 915337
Comment 56 Jean-Yves Perrier [:teoli] 2013-09-13 04:17:18 PDT
Hi!

Just a quick question about this feature r/ documentation:
- Is this a non-standard extension of the IndexedDB spec, Web-visible and available to all our platforms (desktop-mobile-apps)?
Comment 57 Asa Dotzler [:asa] 2013-09-13 12:54:07 PDT
Is this relnote worthy? If so, could someone with expertise write up a one or two sentence note for the developer section of the release notes?
Comment 58 Lukas Blakk [:lsblakk] use ?needinfo 2013-09-13 13:10:40 PDT
Kyle or Ehsan can you address the questions in comments 56 and 57?
Comment 59 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2013-09-13 14:23:00 PDT
Why don't you ask Jan?
Comment 60 Jan Varga [:janv] 2013-09-13 20:27:48 PDT
I'll write it up, a blog post is coming too.
Comment 61 Jan Varga [:janv] 2013-09-13 22:56:09 PDT
(In reply to Jean-Yves Perrier [:teoli] from comment #56)
> Hi!
> 
> Just a quick question about this feature r/ documentation:
> - Is this a non-standard extension of the IndexedDB spec, Web-visible and
> available to all our platforms (desktop-mobile-apps)?

Yes, the "storage type" option is not yet in IndexedDB specification, but we're working on getting it standardized (Google has expressed the same intention since they already support temp storage).


The feature is Web-visible, but temp storage is not used by default yet, it has to be requested explicitely:

indexedDB.open("myDb", { version: 1, storage: "temporary" });


It should work on all platforms.



(In reply to Asa Dotzler [:asa] from comment #57)
> Is this relnote worthy? If so, could someone with expertise write up a one
> or two sentence note for the developer section of the release notes?

What about:
"IndexedDB can now be used as a "optimistic" storage area so it doesn't require any prompts and data is stored in a pool with LRU eviction policy, in short temporary storage (bug 785884)."
Comment 62 Jan Varga [:janv] 2014-03-06 10:06:12 PST
We need to document this, I filed bug 980439.
Comment 63 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-12-01 06:27:00 PST
Feature was documented as per the bug reference in comment 62, so I'm ddc'ing this.

Note You need to log in before you can comment on or make changes to this bug.