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" })

(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?

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)

"storage" as a name in the dictionary sounds ok with me. As does PersistenceType in the internal code.

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.

Jonas says ok, but there are also some JS callers that I need to fix.

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?

(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.

(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 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 on attachment 770128 [details] [diff] [review] patch v0.2 Review of attachment 770128 [details] [diff] [review]: ----------------------------------------------------------------- I already reviewed the interdiff.

(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

(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

(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 on attachment 770555 [details] [diff] [review] interdiff2 Review of attachment 770555 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks!

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/

(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

Jan, this patch fixes the b2g failure on device - I did not send it to try though.

Fabrice, thanks a lot for tracking this down!

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.

(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.

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 on attachment 790757 [details] helgrind.log This looks good!

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 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!

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

(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)

(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.

(In reply to Jan Varga [:janv] from comment #43) > never mind, entire block can be simplified (w/o using the extra stack > pointer) Great!

(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

Support for [EnforceRange] in dictionaries has already landed on inbound.

It seems windows requires clobbering. https://hg.mozilla.org/integration/mozilla-inbound/rev/246ad6d960f4

(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.

(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

https://hg.mozilla.org/mozilla-central/rev/8734ea74053b https://hg.mozilla.org/mozilla-central/rev/246ad6d960f4

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)?

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?

Kyle or Ehsan can you address the questions in comments 56 and 57?

Why don't you ask Jan?

I'll write it up, a blog post is coming too.

(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)."

We need to document this, I filed bug 980439.

Feature was documented as per the bug reference in comment 62, so I'm ddc'ing this.