bent suggested to do this according to his initial feedback on the temporary storage implementation and additional needs like counting quota towards eTLD+1 The idea is to add minimal logic to mozstorage VFS implementation and do all the quota handling in our own manager.

This patch seems to pass all tests. I'll attach a new patch once I clean up stuff.

added support for origin matching by pattern

the patch contains other small changes

Comment on attachment 677989 [details] [diff] [review] Merged QuotaManager into IndexedDatabaseManager We're going to use the separate Quota Manager (per discussion with Jonas and Ben)

Comment on attachment 678939 [details] [diff] [review] patch v0.3 Review of attachment 678939 [details] [diff] [review]: ----------------------------------------------------------------- The QuotaManager mutex usage is pretty rough. In general you should only hold it while updating internal data structures to guarantee that other threads don't block for a nontrivial amount of time. I've pointed out some specific problems below. ::: dom/file/nsIFileStorage.h @@ +40,5 @@ > NS_DEFINE_STATIC_IID_ACCESSOR(nsIFileStorage, NS_FILESTORAGE_IID) > > #define NS_DECL_NSIFILESTORAGE \ > + virtual const nsACString& \ > + StorageOrigin(); \ Nit: Add MOZ_OVERRIDE to these. ::: dom/indexedDB/IDBDatabase.cpp @@ +793,5 @@ > return NS_OK; > } > > +const nsACString& > +IDBDatabase::StorageOrigin() Nit: Move to header ::: dom/indexedDB/IDBObjectStore.cpp @@ +48,5 @@ > #define FILE_COPY_BUFFER_SIZE 32768 > > USING_INDEXEDDB_NAMESPACE > using namespace mozilla::dom; > +USING_QUOTA_NAMESPACE can you just use FileOutputStream? ::: dom/indexedDB/IndexedDatabaseInlines.h @@ +82,5 @@ > +inline void > +IncrementUsage(uint64_t* aUsage, uint64_t aDelta) > +{ > + // Watch for overflow! > + if ((INT64_MAX - *aUsage) <= aDelta) { I don't know what I'm missing but it looks like we're doing this wrong. This should use UINT64_MAX and then just test <, right? ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +1114,5 @@ > + QuotaManager* quotaManager = QuotaManager::Get(); > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); > + > + quotaManager->InitQuotaForOrigin(aOrigin, > + GetIndexedDBQuotaMB() * 1024 * 1024, Since this is now our own API how about we just make it take megabytes instead of having to multiply outside? ::: dom/quota/FileStreams.cpp @@ +8,5 @@ > + > +USING_QUOTA_NAMESPACE > + > +QuotaObjectHelper::QuotaObjectHelper(const nsACString& aOrigin) > +: mOrigin(aOrigin) Nit: This can go in the header @@ +18,5 @@ > +{ > + QuotaManager* quotaManager = QuotaManager::Get(); > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); > + > + mQuotaObject = quotaManager->GetQuotaObject(mOrigin, aFile); You should assert that this is null before setting. ::: dom/quota/FileStreams.h @@ +20,5 @@ > +public: > + QuotaObjectHelper(const nsACString& aOrigin); > + > + inline void > + CreateQuotaObject(nsIFile* aFile); Nit: None of these are inlines, just remove that keyword. @@ +40,5 @@ > + nsRefPtr<QuotaObject> mQuotaObject; > +}; > + > +#define NS_DECL_QUOTA_OPEN \ > + virtual nsresult DoOpen(); Nit: Add MOZ_OVERRIDE to these. @@ +42,5 @@ > + > +#define NS_DECL_QUOTA_OPEN \ > + virtual nsresult DoOpen(); > + > +#define NS_DECL_QUOTA_WRITE \ Hm, instead of doing all this macro magic let's use templates instead. In the header: template <class Base, bool AllowWrite> class QuotaObjectHelper : public Base { QuotaObjectHelper(const nsACString& aOrigin); virtual nsresult DoOpen() MOZ_OVERRIDE; NS_IMETHOD Write(...) MOZ_OVERRIDE; NS_IMETHOD SetEOF() MOZ_OVERRIDE; NS_IMETHOD Close() MOZ_OVERRIDE; }; class FileInputStream : public QuotaObjectHelper<nsFileInputStream, false> { FileInputStream(const nsACString& aOrigin); virtual ~FileInputStream() { Close(); } }; Then, in the cpp: template <class Base, bool AllowWrite> NS_IMETHODIMP FileInputStream<Base, AllowWrite>::DoOpen() { CreateQuotaObject(mOpenParams.localFile); nsresult rv = Base::DoOpen(); NS_ENSURE_SUCCESS(rv, rv); MaybeUpdateSize(mOpenParams.ioFlags); return NS_OK; } You'll need two for Write: template <class Base, bool AllowWrite> NS_IMETHODIMP FileInputStream<Base, AllowWrite>::Write(const char* aBuf, uint32_t aCount, uint32_t* _retval) { MOZ_STATIC_ASSERT(AllowWrite); nsresult rv = MaybeAllocateMoreSpace(aCount); NS_ENSURE_SUCCESS(rv, rv); rv = Base::Write(aBuf, aCount, _retval); NS_ENSURE_SUCCESS(rv, rv); return NS_OK; } template <class Base> NS_IMETHODIMP FileInputStream<Base, false>::Write(const char* aBuf, uint32_t aCount, uint32_t* _retval) { nsresult rv = Base::Write(aBuf, aCount, _retval); NS_ENSURE_SUCCESS(rv, rv); return NS_OK; } ::: dom/quota/QuotaManager.cpp @@ +21,5 @@ > +nsAutoPtr<QuotaManager> gInstance; > + > +} // anonymous namespace > + > +QuotaManager::QuotaManager() This (and the destructor) can be moved into the header I think. @@ +63,5 @@ > + > +nsresult > +QuotaManager::Init() > +{ > + mOriginInfos.Init(); Since you're relying on infallibility here just nuke the Init method entirely and stick this in the constructor. @@ +118,5 @@ > + return nullptr; > + } > + > + nsString path; > + nsresult rv = aFile->GetPath(path); It's almost always a bad idea to call outside your module with a lock held. Let's rework this. @@ +125,5 @@ > + QuotaObject* info = nullptr; > + originInfo->mQuotaObjects.Get(path, &info); > + > + if (!info) { > + NS_ASSERTION(!NS_IsMainThread(), "Performing sync IO on the main thread!"); What actually prevents this from happening on the main thread? @@ +134,5 @@ > + rv = aFile->Exists(&exists); > + NS_ENSURE_SUCCESS(rv, nullptr); > + > + if (exists) { > + rv = aFile->GetFileSize(&fileSize); Hm, this is going to block while you're holding your mutex. That will keep the quota system from working until the IO completes... @@ +164,5 @@ > + return GetQuotaObject(aOrigin, file); > +} > + > +PLDHashOperator > +QuotaManager::RemoveQuotaCallback(const nsACString& aKey, Nit: This name is a little weird. RemoveQuotaForPattern? @@ +183,5 @@ > + return PL_DHASH_NEXT; > +} > + > +QuotaManager::QuotaInfo::QuotaInfo(OriginInfo* aOriginInfo, const nsAString& aPath, > + int64_t aSize) Nit: move to header @@ +193,5 @@ > +QuotaManager::QuotaInfo::AddRef() > +{ > + NS_PRECONDITION(int32_t(mRefCnt) >= 0, "illegal refcnt"); > + nsrefcnt count = NS_AtomicIncrementRefcnt(mRefCnt); > + NS_LOG_ADDREF(this, count, "QuotaObject", sizeof(*this)); Nit: QuotaInfo @@ +198,5 @@ > + return (nsrefcnt) count; > +} > + > +NS_METHOD_(nsrefcnt) > +QuotaManager::QuotaInfo::Release() This is a crashing race. Consider: Thread 1: QuotaManager::GetQuotaObject(), enters mutex, cswitch. Thread 2: QuotaInfo::Release(), decrements refcount, blocks on mutex, cswitch. Thread 1: QuotaInfo::AddRef(), releases mutex, cswitch. Thread 2: removes QuotaInfo* from hash, deletes the object. @@ +202,5 @@ > +QuotaManager::QuotaInfo::Release() > +{ > + NS_PRECONDITION(0 != mRefCnt, "dup release"); > + nsrefcnt count = NS_AtomicDecrementRefcnt(mRefCnt); > + NS_LOG_RELEASE(this, count, "QuotaObject"); Nit: QuotaInfo @@ +235,5 @@ > +QuotaManager::QuotaInfo::MaybeAllocateMoreSpace(int64_t aOffset, int32_t aCount) > +{ > + int64_t end = aOffset + aCount; > + > + if (mSize >= end) { mSize isn't protected here... @@ +244,5 @@ > + QuotaManager* quotaManager = QuotaManager::Get(); > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); > + > + MutexAutoLock lock(quotaManager->QuotaMutex()); > + Nit: trailing whitespace @@ +247,5 @@ > + MutexAutoLock lock(quotaManager->QuotaMutex()); > + > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; > + if (newUsage > mOriginInfo->mLimit) { > + if (!indexedDB::IndexedDatabaseManager::QuotaIsLifted()) { This isn't ok, you're calling a function that may block while holding your quota mutex. At the least this will prevent the quota system from working on other threads while waiting on a user prompt, and at worst the main thread tries to access some quota information and the app deadlocks. @@ +264,5 @@ > + > +QuotaManager::OriginInfo::OriginInfo(const nsACString& aOrigin, int64_t aLimit, int64_t aUsage) > +: mOrigin(aOrigin), mLimit(aLimit), mUsage(aUsage) > +{ > + mQuotaObjects.Init(); This can go into the header. ::: dom/quota/QuotaManager.h @@ +14,5 @@ > +#include "nsRefPtrHashtable.h" > + > +BEGIN_QUOTA_NAMESPACE > + > +class QuotaObject Hm, you only have one subclass of QuotaObject (QuotaInfo). Why not just lose the base class and only have QuotaInfo? @@ +63,5 @@ > + already_AddRefed<QuotaObject> > + GetQuotaObject(const nsACString& aOrigin, > + const nsAString& aPath); > + > + mozilla::Mutex& QuotaMutex() Things like this shouldn't be public. It looks like QuotaInfo is the only thing that uses it, can you just make that a friend? @@ +97,5 @@ > + > + private: > + nsAutoRefCnt mRefCnt; > + > + nsRefPtr<OriginInfo> mOriginInfo; Looks like this should be a weak pointer... QuotaManager owns OriginInfos, then OriginInfos sort of own QuotaInfo. ::: storage/public/mozIStorageService.idl @@ +109,5 @@ > + /** > + * See openDatabase(). Exactly the same only with custom params > + * passed to SQLite and VFS implementations. > + */ > + mozIStorageConnection openDatabaseWithParams( I don't really like this API, it seems to me that this should take either a) a string that will be auto-converted into a URI, or b) an nsIFileURL object. ::: storage/src/TelemetryVFS.cpp @@ +85,5 @@ > > struct telemetry_file { > sqlite3_file base; // Base class. Must be first > Histograms *histograms; // histograms pertaining to this file > + QuotaObject *quotaObject; // quota object for this file Why not just use a nsRefPtr here? @@ +321,5 @@ > + quotaObject = quotaManager->GetQuotaObject(nsDependentCString(origin), > + NS_ConvertUTF8toUTF16(zName)); > + > + } > + quotaObject.forget(&p->quotaObject); This (and the quotaObject decl) should be inside the if block. @@ +327,4 @@ > rc = orig_vfs->xOpen(orig_vfs, zName, p->pReal, flags, pOutFlags); > if( rc != SQLITE_OK ) > return rc; > + Nit: best to avoid this kind of change, below too. ::: storage/src/mozStorageService.cpp @@ +720,5 @@ > + > + nsresult rv = msc->initialize(aDatabaseFile, aParamCount, aKeys, aValues); > + NS_ENSURE_SUCCESS(rv, rv); > + > + NS_ADDREF(*_connection = msc); Nit: msc.forget(_connection)

OK, thanks for the comments, I'll try to fix them ASAP

(In reply to ben turner [:bent] from comment #7) > Comment on attachment 678939 [details] [diff] [review] > patch v0.3 > > Review of attachment 678939 [details] [diff] [review]: > ----------------------------------------------------------------- > > The QuotaManager mutex usage is pretty rough. In general you should only > hold it while updating internal data structures to guarantee that other > threads don't block for a nontrivial amount of time. I've pointed out some > specific problems below. ok, thanks > > ::: dom/file/nsIFileStorage.h > @@ +40,5 @@ > > NS_DEFINE_STATIC_IID_ACCESSOR(nsIFileStorage, NS_FILESTORAGE_IID) > > > > #define NS_DECL_NSIFILESTORAGE \ > > + virtual const nsACString& \ > > + StorageOrigin(); \ > > Nit: Add MOZ_OVERRIDE to these. fixed > > ::: dom/indexedDB/IDBDatabase.cpp > @@ +793,5 @@ > > return NS_OK; > > } > > > > +const nsACString& > > +IDBDatabase::StorageOrigin() > > Nit: Move to header hm, I would have to drop the nice macro NS_DECL_NSIFILESTORAGE other nsIFileStorage methods are in the cpp too I think we could make an exception here... > > ::: dom/indexedDB/IDBObjectStore.cpp > @@ +48,5 @@ > > #define FILE_COPY_BUFFER_SIZE 32768 > > > > USING_INDEXEDDB_NAMESPACE > > using namespace mozilla::dom; > > +USING_QUOTA_NAMESPACE > > can you just use FileOutputStream? sure, fixed > > ::: dom/indexedDB/IndexedDatabaseInlines.h > @@ +82,5 @@ > > +inline void > > +IncrementUsage(uint64_t* aUsage, uint64_t aDelta) > > +{ > > + // Watch for overflow! > > + if ((INT64_MAX - *aUsage) <= aDelta) { > > I don't know what I'm missing but it looks like we're doing this wrong. This > should use UINT64_MAX and then just test <, right? yeah, it's now fixed > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +1114,5 @@ > > + QuotaManager* quotaManager = QuotaManager::Get(); > > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); > > + > > + quotaManager->InitQuotaForOrigin(aOrigin, > > + GetIndexedDBQuotaMB() * 1024 * 1024, > > Since this is now our own API how about we just make it take megabytes > instead of having to multiply outside? ok, fixed > > ::: dom/quota/FileStreams.cpp > @@ +8,5 @@ > > + > > +USING_QUOTA_NAMESPACE > > + > > +QuotaObjectHelper::QuotaObjectHelper(const nsACString& aOrigin) > > +: mOrigin(aOrigin) > > Nit: This can go in the header ok > > @@ +18,5 @@ > > +{ > > + QuotaManager* quotaManager = QuotaManager::Get(); > > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); > > + > > + mQuotaObject = quotaManager->GetQuotaObject(mOrigin, aFile); > > You should assert that this is null before setting. ok > > ::: dom/quota/FileStreams.h > @@ +20,5 @@ > > +public: > > + QuotaObjectHelper(const nsACString& aOrigin); > > + > > + inline void > > + CreateQuotaObject(nsIFile* aFile); > > Nit: None of these are inlines, just remove that keyword. ok > > @@ +40,5 @@ > > + nsRefPtr<QuotaObject> mQuotaObject; > > +}; > > + > > +#define NS_DECL_QUOTA_OPEN \ > > + virtual nsresult DoOpen(); > > Nit: Add MOZ_OVERRIDE to these. done > > @@ +42,5 @@ > > + > > +#define NS_DECL_QUOTA_OPEN \ > > + virtual nsresult DoOpen(); > > + > > +#define NS_DECL_QUOTA_WRITE \ > > Hm, instead of doing all this macro magic let's use templates instead. ok, fixed anyway, I did it a bit differently > ::: dom/quota/QuotaManager.cpp > @@ +21,5 @@ > > +nsAutoPtr<QuotaManager> gInstance; > > + > > +} // anonymous namespace > > + > > +QuotaManager::QuotaManager() > > This (and the destructor) can be moved into the header I think. done > > @@ +63,5 @@ > > + > > +nsresult > > +QuotaManager::Init() > > +{ > > + mOriginInfos.Init(); > > Since you're relying on infallibility here just nuke the Init method > entirely and stick this in the constructor. ok, done > > @@ +118,5 @@ > > + return nullptr; > > + } > > + > > + nsString path; > > + nsresult rv = aFile->GetPath(path); > > It's almost always a bad idea to call outside your module with a lock held. > Let's rework this. ok, reworked > > @@ +125,5 @@ > > + QuotaObject* info = nullptr; > > + originInfo->mQuotaObjects.Get(path, &info); > > + > > + if (!info) { > > + NS_ASSERTION(!NS_IsMainThread(), "Performing sync IO on the main thread!"); > > What actually prevents this from happening on the main thread? I made this method non main thread only > > @@ +134,5 @@ > > + rv = aFile->Exists(&exists); > > + NS_ENSURE_SUCCESS(rv, nullptr); > > + > > + if (exists) { > > + rv = aFile->GetFileSize(&fileSize); > > Hm, this is going to block while you're holding your mutex. That will keep > the quota system from working until the IO completes... ok, reworked > > @@ +164,5 @@ > > + return GetQuotaObject(aOrigin, file); > > +} > > + > > +PLDHashOperator > > +QuotaManager::RemoveQuotaCallback(const nsACString& aKey, > > Nit: This name is a little weird. RemoveQuotaForPattern? ok, renamed to RemoveQuotaForPatternCallback > > @@ +183,5 @@ > > + return PL_DHASH_NEXT; > > +} > > + > > +QuotaManager::QuotaInfo::QuotaInfo(OriginInfo* aOriginInfo, const nsAString& aPath, > > + int64_t aSize) > > Nit: move to header ok, done > > @@ +193,5 @@ > > +QuotaManager::QuotaInfo::AddRef() > > +{ > > + NS_PRECONDITION(int32_t(mRefCnt) >= 0, "illegal refcnt"); > > + nsrefcnt count = NS_AtomicIncrementRefcnt(mRefCnt); > > + NS_LOG_ADDREF(this, count, "QuotaObject", sizeof(*this)); > > Nit: QuotaInfo nothing to do here, we removed the subclass > > @@ +198,5 @@ > > + return (nsrefcnt) count; > > +} > > + > > +NS_METHOD_(nsrefcnt) > > +QuotaManager::QuotaInfo::Release() > > This is a crashing race. Consider: > > Thread 1: QuotaManager::GetQuotaObject(), enters mutex, cswitch. > Thread 2: QuotaInfo::Release(), decrements refcount, blocks on mutex, > cswitch. > Thread 1: QuotaInfo::AddRef(), releases mutex, cswitch. > Thread 2: removes QuotaInfo* from hash, deletes the object. fixed, addref/release is now protected by the quota mutex > > @@ +202,5 @@ > > +QuotaManager::QuotaInfo::Release() > > +{ > > + NS_PRECONDITION(0 != mRefCnt, "dup release"); > > + nsrefcnt count = NS_AtomicDecrementRefcnt(mRefCnt); > > + NS_LOG_RELEASE(this, count, "QuotaObject"); > > Nit: QuotaInfo nothing to do here, we removed the subclass > > @@ +235,5 @@ > > +QuotaManager::QuotaInfo::MaybeAllocateMoreSpace(int64_t aOffset, int32_t aCount) > > +{ > > + int64_t end = aOffset + aCount; > > + > > + if (mSize >= end) { > > mSize isn't protected here... It was done this way in test_quota.c and I thought it was a performance trade off. Anyway, it's now protected by the mutex > > @@ +244,5 @@ > > + QuotaManager* quotaManager = QuotaManager::Get(); > > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); > > + > > + MutexAutoLock lock(quotaManager->QuotaMutex()); > > + > > Nit: trailing whitespace removed > > @@ +247,5 @@ > > + MutexAutoLock lock(quotaManager->QuotaMutex()); > > + > > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; > > + if (newUsage > mOriginInfo->mLimit) { > > + if (!indexedDB::IndexedDatabaseManager::QuotaIsLifted()) { > > This isn't ok, you're calling a function that may block while holding your > quota mutex. At the least this will prevent the quota system from working on > other threads while waiting on a user prompt, and at worst the main thread > tries to access some quota information and the app deadlocks. per discussion on IRC, this is not an issue if we use quota manager only from background threads > > @@ +264,5 @@ > > + > > +QuotaManager::OriginInfo::OriginInfo(const nsACString& aOrigin, int64_t aLimit, int64_t aUsage) > > +: mOrigin(aOrigin), mLimit(aLimit), mUsage(aUsage) > > +{ > > + mQuotaObjects.Init(); > > This can go into the header. ok, fixed > > ::: dom/quota/QuotaManager.h > @@ +14,5 @@ > > +#include "nsRefPtrHashtable.h" > > + > > +BEGIN_QUOTA_NAMESPACE > > + > > +class QuotaObject > > Hm, you only have one subclass of QuotaObject (QuotaInfo). Why not just lose > the base class and only have QuotaInfo? I wanted to hide OriginInfo in QuotaManager Anyway, I reworked it and QuotaObject and OriginInfo is now defined outside of the QuotaManager class > > @@ +63,5 @@ > > + already_AddRefed<QuotaObject> > > + GetQuotaObject(const nsACString& aOrigin, > > + const nsAString& aPath); > > + > > + mozilla::Mutex& QuotaMutex() > > Things like this shouldn't be public. It looks like QuotaInfo is the only > thing that uses it, can you just make that a friend? sure, fixed > > @@ +97,5 @@ > > + > > + private: > > + nsAutoRefCnt mRefCnt; > > + > > + nsRefPtr<OriginInfo> mOriginInfo; > > Looks like this should be a weak pointer... QuotaManager owns OriginInfos, > then OriginInfos sort of own QuotaInfo. ok, I changed it and added some additional code to handle the change > > ::: storage/public/mozIStorageService.idl > @@ +109,5 @@ > > + /** > > + * See openDatabase(). Exactly the same only with custom params > > + * passed to SQLite and VFS implementations. > > + */ > > + mozIStorageConnection openDatabaseWithParams( > > I don't really like this API, it seems to me that this should take either a) > a string that will be auto-converted into a URI, or b) an nsIFileURL object. sorry :( ok, I reworked it > > ::: storage/src/TelemetryVFS.cpp > @@ +85,5 @@ > > > > struct telemetry_file { > > sqlite3_file base; // Base class. Must be first > > Histograms *histograms; // histograms pertaining to this file > > + QuotaObject *quotaObject; // quota object for this file > > Why not just use a nsRefPtr here? ok, replaced with a nsRefPtr > > @@ +321,5 @@ > > + quotaObject = quotaManager->GetQuotaObject(nsDependentCString(origin), > > + NS_ConvertUTF8toUTF16(zName)); > > + > > + } > > + quotaObject.forget(&p->quotaObject); > > This (and the quotaObject decl) should be inside the if block. actually, we can remove this local variable now > > @@ +327,4 @@ > > rc = orig_vfs->xOpen(orig_vfs, zName, p->pReal, flags, pOutFlags); > > if( rc != SQLITE_OK ) > > return rc; > > + > > Nit: best to avoid this kind of change, below too. ok, fixed > > ::: storage/src/mozStorageService.cpp > @@ +720,5 @@ > > + > > + nsresult rv = msc->initialize(aDatabaseFile, aParamCount, aKeys, aValues); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + NS_ADDREF(*_connection = msc); > > Nit: msc.forget(_connection) ok, I fixed it in other places too, while I was there and it seems OpenSpecialDatabase() had some leaky code :)

(In reply to Jan Varga [:janv] from comment #9) > > > > ::: dom/indexedDB/IDBDatabase.cpp > > @@ +793,5 @@ > > > return NS_OK; > > > } > > > > > > +const nsACString& > > > +IDBDatabase::StorageOrigin() > > > > Nit: Move to header > > hm, I would have to drop the nice macro NS_DECL_NSIFILESTORAGE > other nsIFileStorage methods are in the cpp too > I think we could make an exception here... I mean, *some* other methods. On the other hand there are some nsIFileStorage methods in the cpp that need IndexedDatabaseManager.h include

Comment on attachment 689269 [details] [diff] [review] patch v0.4 Review of attachment 689269 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/LockedFile.cpp @@ +952,5 @@ > mAborted = true; > } > > for (uint32_t index = 0; index < mParallelStreams.Length(); index++) { > + nsCOMPtr<nsIInputStream> stream = I don't actually understand these changes... What's going on here? ::: dom/indexedDB/FileManager.cpp @@ +383,5 @@ > +// static > +nsresult > +FileManager::GetUsage(nsIFile* aDirectory, uint64_t* aUsage) > +{ > + *aUsage = 0; Nit: Let's not set the outparam until we return success. ::: dom/indexedDB/IDBFileHandle.cpp @@ +74,4 @@ > if (aReadOnly) { > + nsRefPtr<FileInputStream> stream = FileInputStream::Create( > + origin, aFile, -1, -1, nsIFileInputStream::DEFER_OPEN); > + NS_ENSURE_TRUE(stream, nullptr); Nit: move this after the if block (and test 'result') so it doesn't have to be duplicated. ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +420,5 @@ > NS_LITERAL_CSTRING("IndexedDB I/O"), > LazyIdleThread::ManualShutdown); > > + // Make sure that the quota manager is up. > + QuotaManager::GetOrCreate(); You should check this for failure. ::: dom/quota/FileStreams.cpp @@ +21,5 @@ > + > +void > +QuotaObjectHelper::MaybeUpdateSize(int32_t aIoFlags) > +{ > + if (mQuotaObject && aIoFlags & PR_TRUNCATE) { Nit: This is correct but for clarity let's add braces around the & args. @@ +117,5 @@ > + uint32_t* _retval) > +{ > + // XXXvarga which one to choose? "this" works on try, not sure about the other > + // one > + //nsresult rv = FileQuotaStream<FileStreamBase>::MaybeAllocateMoreSpace(aCount); You should just be able to call 'MaybeAllocateMoreSpace(aCount)' directly. @@ +141,5 @@ > + return stream.forget(); > +} > + > + > +NS_IMPL_ISUPPORTS_INHERITED0(FileOutputStream, nsFileOutputStream) Nit: extra newline above. @@ +155,5 @@ > + return stream.forget(); > +} > + > + > +NS_IMPL_ISUPPORTS_INHERITED0(FileStream, nsFileStream) Nit: here too. ::: dom/quota/FileStreams.h @@ +14,5 @@ > +#include "QuotaManager.h" > + > +BEGIN_QUOTA_NAMESPACE > + > +class QuotaObjectHelper I don't understand the purpose of this class now. Can't you move all this to FileQuotaStream? @@ +49,5 @@ > +class FileQuotaStream : public FileStreamBase, > + public QuotaObjectHelper > +{ > +public: > + // FileStreamBase override Nit: This is actually an nsFileStreamBase override, here and below. ::: dom/quota/QuotaManager.cpp @@ +20,5 @@ > +nsAutoPtr<QuotaManager> gInstance; > + > +PLDHashOperator > +RemoveQuotaForPatternCallback(const nsACString& aKey, > + nsRefPtr<OriginInfo>& aValue, void* aUserArg) Nit: aUserArg on next line. @@ +53,5 @@ > +void > +QuotaObject::Release() > +{ > + QuotaManager* quotaManager = QuotaManager::Get(); > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); Could this be null after shutdown? If something leaks, for instance? @@ +101,5 @@ > + if (mSize >= end || !mOriginInfo) { > + return true; > + } > + > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; Assert that mUsage <= mSize. @@ +103,5 @@ > + } > + > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; > + if (newUsage > mOriginInfo->mLimit) { > + if (!indexedDB::IndexedDatabaseManager::QuotaIsLifted()) { We need a followup to move all this machinery into the QuotaManager. I'm really nervous about us using two mutexes for this (mQuotaMutex as well as the one in IndexedDatabaseManager), so we should do this next. @@ +115,5 @@ > + "Should have cleared in ClearOriginInfos_Locked!"); > + > + quotaManager->mOriginInfos.Remove(origin); > + > + return true; You're not updating mSize here, probably should for sanity. @@ +127,5 @@ > + > +// static > +PLDHashOperator > +OriginInfo::ClearOriginInfoCallback(const nsAString& aKey, > + QuotaObject* aValue, void* aUserArg) Nit: aUserArg on next line. @@ +168,5 @@ > +{ > + OriginInfo* info = new OriginInfo(aOrigin, aLimit * 1024 * 1024, aUsage); > + > + MutexAutoLock lock(mQuotaMutex); > + mOriginInfos.Put(aOrigin, info); Nit: First assert that you're not replacing an existing entry. ::: dom/quota/QuotaManager.h @@ +27,5 @@ > + QuotaObject(OriginInfo* aOriginInfo, const nsAString& aPath, int64_t aSize) > + : mOriginInfo(aOriginInfo), mPath(aPath), mSize(aSize) > + { } > + > + virtual ~QuotaObject() You should make this private. @@ +66,5 @@ > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(OriginInfo) > + > +private: > + void > + ClearOriginInfos_Locked() Nit: LockedClearOriginInfos. @@ +68,5 @@ > +private: > + void > + ClearOriginInfos_Locked() > + { > + mQuotaObjects.EnumerateRead(ClearOriginInfoCallback, nullptr); You should assert that the mutex is held here. Maybe you'll have to do some #ifdef DEBUG trickery. @@ +132,5 @@ > + } > + > + nsRefPtrHashtable<nsCStringHashKey, OriginInfo> mOriginInfos; > + > + mozilla::Mutex mQuotaMutex; Nit: Let's move this above mOriginInfos. ::: netwerk/base/src/nsFileStreams.cpp @@ +981,5 @@ > > //////////////////////////////////////////////////////////////////////////////// > // nsFileStream > > +NS_IMPL_ISUPPORTS_INHERITED3(nsFileStream, Nit: Fix this trailing whitespace while you're here. ::: storage/public/mozIStorageService.idl @@ +20,1 @@ > interface mozIStorageService : nsISupports { I understand why you're adding this method here but I think we can get away without adding nsIFileURL support here. Really all you want to do is associate an origin with a database file, right? But to do that via URLs you have to build a URL string and then let SQLite parse it all back out. Instead, let's use pragmas. http://www.sqlite.org/c3ref/c_fcntl_chunk_size.html#sqlitefcntlpragma You can modify the xFileControl method in the VFS to parse "mozOrigin" or something and then call sqlite3_file_control() to set it. Does that sound better? Then you wouldn't have to change anything else the API or storage backend to support this. The one downside is that we really only want to allow this to be called when the connection is first opened. If you do want to go this route then we need to get :asuth to review the changes to the storage API. @@ +115,5 @@ > + * @param aURI > + * A nsIURI that represents the database that is to be opened. > + * The URI must QI to nsIFileURL. > + */ > + mozIStorageConnection openDatabaseWithURI(in nsIURI aURI); Why not just take nsIFileURL? ::: storage/src/mozStorageConnection.cpp @@ +494,5 @@ > + NS_ASSERTION (aDatabaseFile, "Passed null file!"); > + NS_ASSERTION (!mDBConn, "Initialize called on already opened database!"); > + SAMPLE_LABEL("storage", "Connection::initialize"); > + > + mDatabaseFile = aDatabaseFile; I realize that the old code did this but I don't think it's smart to hold this if you're going to fail somewhere below. @@ +517,5 @@ > + NS_ASSERTION (aURI, "Passed null URI!"); > + NS_ASSERTION (!mDBConn, "Initialize called on already opened database!"); > + SAMPLE_LABEL("storage", "Connection::initialize"); > + > + mURI = aURI; Here too. ::: storage/src/mozStorageService.cpp @@ +628,2 @@ > if (::strcmp(aStorageKey, "memory") == 0) { > + nsRefPtr<Connection> msc = new Connection(this, SQLITE_OPEN_READWRITE); Adding the nsRefPtr here is good but duplicating the new/init/forget code isn't. Can we stick to the old code and just add a nsRefPtr? @@ +705,5 @@ > + // reasons. > + int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_SHAREDCACHE | > + SQLITE_OPEN_CREATE | SQLITE_OPEN_URI; > + nsRefPtr<Connection> msc = new Connection(this, flags); > + NS_ENSURE_TRUE(msc, NS_ERROR_OUT_OF_MEMORY); Nit: new is infallible now.

ok, will send a new patch soon

> ::: storage/public/mozIStorageService.idl > @@ +20,1 @@ > > interface mozIStorageService : nsISupports { > > I understand why you're adding this method here but I think we can get away > without adding nsIFileURL support here. Really all you want to do is > associate an origin with a database file, right? But to do that via URLs you > have to build a URL string and then let SQLite parse it all back out. > Instead, let's use pragmas. > > http://www.sqlite.org/c3ref/c_fcntl_chunk_size.html#sqlitefcntlpragma > > You can modify the xFileControl method in the VFS to parse "mozOrigin" or > something and then call sqlite3_file_control() to set it. > > Does that sound better? Then you wouldn't have to change anything else the > API or storage backend to support this. The one downside is that we really > only want to allow this to be called when the connection is first opened. Well, it sounds good, I even started implementing it, but ... The main difference between using file URIs and PRAGMAs is that for URIs the origin parameter is available before the real xOpen() is called. xFileControl() is called after xOpen(). If you remember, I'm using the hash table of all quota objects for an origin info in the temporary storage implementation to check if the origin has any opened files. So if the quota object is created after the file is opened, then in theory other thread could trigger deletion of this origin, since it doesn't know that the file is opened. Well, we could add additional methods to make the origin info to be in "use" (before the real xOpen call and to TelemetryVFS's xFileControl), but it would cause additional locking/unlocking and it doesn't look so nice. I think I addressed all other comments, I should be able to submit a new patch today (w/o the change to use PRAGMAs). Oh there's one thing I don't get: @@ +101,5 @@ > + if (mSize >= end || !mOriginInfo) { > + return true; > + } > + > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; Assert that mUsage <= mSize.

(In reply to ben turner [:bent] from comment #13) > Comment on attachment 689269 [details] [diff] [review] > patch v0.4 > > Review of attachment 689269 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/file/LockedFile.cpp > @@ +952,5 @@ > > mAborted = true; > > } > > > > for (uint32_t index = 0; index < mParallelStreams.Length(); index++) { > > + nsCOMPtr<nsIInputStream> stream = > > I don't actually understand these changes... What's going on here? See the changes in IDBFileHandle::CreateStream(). Basically, we were creating a stream that always supported nsIInputStream and nsIOutputStream, so Close() could be called on the nsIOutputStream too. Now when we switched to "almost standard" XPCOM/Necko file streams, we create a stream which only supports nsIInputStream for readonly filehandle operations. Readwrite operations are handled by "input/output" streams which QI to both nsIInputStream and nsIOutputStream. That's why we have to change the QI before calling Close() > > ::: dom/indexedDB/FileManager.cpp > @@ +383,5 @@ > > +// static > > +nsresult > > +FileManager::GetUsage(nsIFile* aDirectory, uint64_t* aUsage) > > +{ > > + *aUsage = 0; > > Nit: Let's not set the outparam until we return success. fixed > > ::: dom/indexedDB/IDBFileHandle.cpp > @@ +74,4 @@ > > if (aReadOnly) { > > + nsRefPtr<FileInputStream> stream = FileInputStream::Create( > > + origin, aFile, -1, -1, nsIFileInputStream::DEFER_OPEN); > > + NS_ENSURE_TRUE(stream, nullptr); > > Nit: move this after the if block (and test 'result') so it doesn't have to > be duplicated. fixed > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +420,5 @@ > > NS_LITERAL_CSTRING("IndexedDB I/O"), > > LazyIdleThread::ManualShutdown); > > > > + // Make sure that the quota manager is up. > > + QuotaManager::GetOrCreate(); > > You should check this for failure. fixed > > ::: dom/quota/FileStreams.cpp > @@ +21,5 @@ > > + > > +void > > +QuotaObjectHelper::MaybeUpdateSize(int32_t aIoFlags) > > +{ > > + if (mQuotaObject && aIoFlags & PR_TRUNCATE) { > > Nit: This is correct but for clarity let's add braces around the & args. done > > @@ +117,5 @@ > > + uint32_t* _retval) > > +{ > > + // XXXvarga which one to choose? "this" works on try, not sure about the other > > + // one > > + //nsresult rv = FileQuotaStream<FileStreamBase>::MaybeAllocateMoreSpace(aCount); > > You should just be able to call 'MaybeAllocateMoreSpace(aCount)' directly. clang doesn't like it: /Users/varga/Sources/Mozilla/mozilla-central/dom/quota/FileStreams.cpp:122:17: error: use of undeclared identifier 'MaybeAllocateMoreSpace' nsresult rv = MaybeAllocateMoreSpace(aCount); http://stackoverflow.com/questions/1120833/derived-template-class-access-to-base-class-member-data anyway, I removed the "this->" prefix and went for "FileQuotaStream<FileStreamBase>::" prefix for now will check on try > > @@ +141,5 @@ > > + return stream.forget(); > > +} > > + > > + > > +NS_IMPL_ISUPPORTS_INHERITED0(FileOutputStream, nsFileOutputStream) > > Nit: extra newline above. fixed > > @@ +155,5 @@ > > + return stream.forget(); > > +} > > + > > + > > +NS_IMPL_ISUPPORTS_INHERITED0(FileStream, nsFileStream) > > Nit: here too. fixed > > ::: dom/quota/FileStreams.h > @@ +14,5 @@ > > +#include "QuotaManager.h" > > + > > +BEGIN_QUOTA_NAMESPACE > > + > > +class QuotaObjectHelper > > I don't understand the purpose of this class now. Can't you move all this to > FileQuotaStream? well, it looked a bit clearer with the extra class anyway, I removed it actually I didn't like that virtual CurrentOffset() method > > @@ +49,5 @@ > > +class FileQuotaStream : public FileStreamBase, > > + public QuotaObjectHelper > > +{ > > +public: > > + // FileStreamBase override > > Nit: This is actually an nsFileStreamBase override, here and below. fixed > > ::: dom/quota/QuotaManager.cpp > @@ +20,5 @@ > > +nsAutoPtr<QuotaManager> gInstance; > > + > > +PLDHashOperator > > +RemoveQuotaForPatternCallback(const nsACString& aKey, > > + nsRefPtr<OriginInfo>& aValue, void* aUserArg) > > Nit: aUserArg on next line. fixed > > @@ +53,5 @@ > > +void > > +QuotaObject::Release() > > +{ > > + QuotaManager* quotaManager = QuotaManager::Get(); > > + NS_ASSERTION(quotaManager, "Shouldn't be null!"); > > Could this be null after shutdown? If something leaks, for instance? fixed, I added a falback to NS_AtomicIncrementRefcnt/NS_AtomicDecrementRefcnt > > @@ +101,5 @@ > > + if (mSize >= end || !mOriginInfo) { > > + return true; > > + } > > + > > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; > > Assert that mUsage <= mSize. hmm, I don't get this one > > @@ +103,5 @@ > > + } > > + > > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; > > + if (newUsage > mOriginInfo->mLimit) { > > + if (!indexedDB::IndexedDatabaseManager::QuotaIsLifted()) { > > We need a followup to move all this machinery into the QuotaManager. I'm > really nervous about us using two mutexes for this (mQuotaMutex as well as > the one in IndexedDatabaseManager), so we should do this next. filed bug 820715 > > @@ +115,5 @@ > > + "Should have cleared in ClearOriginInfos_Locked!"); > > + > > + quotaManager->mOriginInfos.Remove(origin); > > + > > + return true; > > You're not updating mSize here, probably should for sanity. done > > @@ +127,5 @@ > > + > > +// static > > +PLDHashOperator > > +OriginInfo::ClearOriginInfoCallback(const nsAString& aKey, > > + QuotaObject* aValue, void* aUserArg) > > Nit: aUserArg on next line. fixed > > @@ +168,5 @@ > > +{ > > + OriginInfo* info = new OriginInfo(aOrigin, aLimit * 1024 * 1024, aUsage); > > + > > + MutexAutoLock lock(mQuotaMutex); > > + mOriginInfos.Put(aOrigin, info); > > Nit: First assert that you're not replacing an existing entry. done > > ::: dom/quota/QuotaManager.h > @@ +27,5 @@ > > + QuotaObject(OriginInfo* aOriginInfo, const nsAString& aPath, int64_t aSize) > > + : mOriginInfo(aOriginInfo), mPath(aPath), mSize(aSize) > > + { } > > + > > + virtual ~QuotaObject() > > You should make this private. done > > @@ +66,5 @@ > > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(OriginInfo) > > + > > +private: > > + void > > + ClearOriginInfos_Locked() > > Nit: LockedClearOriginInfos. fixed > > @@ +68,5 @@ > > +private: > > + void > > + ClearOriginInfos_Locked() > > + { > > + mQuotaObjects.EnumerateRead(ClearOriginInfoCallback, nullptr); > > You should assert that the mutex is held here. Maybe you'll have to do some > #ifdef DEBUG trickery. done > > @@ +132,5 @@ > > + } > > + > > + nsRefPtrHashtable<nsCStringHashKey, OriginInfo> mOriginInfos; > > + > > + mozilla::Mutex mQuotaMutex; > > Nit: Let's move this above mOriginInfos. done > > ::: netwerk/base/src/nsFileStreams.cpp > @@ +981,5 @@ > > > > //////////////////////////////////////////////////////////////////////////////// > > // nsFileStream > > > > +NS_IMPL_ISUPPORTS_INHERITED3(nsFileStream, > > Nit: Fix this trailing whitespace while you're here. done, also removed the one after the macro > > ::: storage/public/mozIStorageService.idl > @@ +20,1 @@ > > interface mozIStorageService : nsISupports { > > I understand why you're adding this method here but I think we can get away > without adding nsIFileURL support here. Really all you want to do is > associate an origin with a database file, right? But to do that via URLs you > have to build a URL string and then let SQLite parse it all back out. > Instead, let's use pragmas. > > http://www.sqlite.org/c3ref/c_fcntl_chunk_size.html#sqlitefcntlpragma > > You can modify the xFileControl method in the VFS to parse "mozOrigin" or > something and then call sqlite3_file_control() to set it. > > Does that sound better? Then you wouldn't have to change anything else the > API or storage backend to support this. The one downside is that we really > only want to allow this to be called when the connection is first opened. > > If you do want to go this route then we need to get :asuth to review the > changes to the storage API. see my previous bugzilla comment > > @@ +115,5 @@ > > + * @param aURI > > + * A nsIURI that represents the database that is to be opened. > > + * The URI must QI to nsIFileURL. > > + */ > > + mozIStorageConnection openDatabaseWithURI(in nsIURI aURI); > > Why not just take nsIFileURL? nsIURI saves a QI :) anyway, I changed it as you suggested I also added a static helper to get the file url from file and origin > > ::: storage/src/mozStorageConnection.cpp > @@ +494,5 @@ > > + NS_ASSERTION (aDatabaseFile, "Passed null file!"); > > + NS_ASSERTION (!mDBConn, "Initialize called on already opened database!"); > > + SAMPLE_LABEL("storage", "Connection::initialize"); > > + > > + mDatabaseFile = aDatabaseFile; > > I realize that the old code did this but I don't think it's smart to hold > this if you're going to fail somewhere below. fixed > > @@ +517,5 @@ > > + NS_ASSERTION (aURI, "Passed null URI!"); > > + NS_ASSERTION (!mDBConn, "Initialize called on already opened database!"); > > + SAMPLE_LABEL("storage", "Connection::initialize"); > > + > > + mURI = aURI; > > Here too. fixed > > ::: storage/src/mozStorageService.cpp > @@ +628,2 @@ > > if (::strcmp(aStorageKey, "memory") == 0) { > > + nsRefPtr<Connection> msc = new Connection(this, SQLITE_OPEN_READWRITE); > > Adding the nsRefPtr here is good but duplicating the new/init/forget code > isn't. Can we stick to the old code and just add a nsRefPtr? yeah, fixed > > @@ +705,5 @@ > > + // reasons. > > + int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_SHAREDCACHE | > > + SQLITE_OPEN_CREATE | SQLITE_OPEN_URI; > > + nsRefPtr<Connection> msc = new Connection(this, flags); > > + NS_ENSURE_TRUE(msc, NS_ERROR_OUT_OF_MEMORY); > > Nit: new is infallible now. fixed

Comment on attachment 691372 [details] [diff] [review] interdiff v0.4 - v0.5 Review of attachment 691372 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBFactory.cpp @@ +259,5 @@ > + nsCOMPtr<nsIURI> uri; > + nsresult rv = NS_NewFileURI(getter_AddRefs(uri), aDatabaseFile); > + NS_ENSURE_SUCCESS(rv, nullptr); > + > + nsCOMPtr<nsIURL> url = do_QueryInterface(uri); You can do all the below stuff with nsIFileURL. Just QI once. @@ +260,5 @@ > + nsresult rv = NS_NewFileURI(getter_AddRefs(uri), aDatabaseFile); > + NS_ENSURE_SUCCESS(rv, nullptr); > + > + nsCOMPtr<nsIURL> url = do_QueryInterface(uri); > + NS_ENSURE_TRUE(url, nullptr); This can just be asserted. ::: dom/indexedDB/IDBFactory.h @@ +14,5 @@ > > #include "nsCycleCollectionParticipant.h" > > class nsIAtom; > +class nsIFileURL; Nit: Also add nsIFile. (Not sure how it's leaking in already but it may not in the future). ::: dom/indexedDB/OpenDatabaseHelper.cpp @@ +1716,5 @@ > NS_ASSERTION(IndexedDatabaseManager::IsMainProcess(), "Wrong process!"); > NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!"); > > + nsCOMPtr<nsIFileURL> dbFileUrl = IDBFactory::GetDatabaseFileURL(aDBFile, > + aOrigin); Nit: nsCOMPtr<nsIFileURL> dbFileUrl = IDBFactory::GetDatabaseFileURL(aDBFile, aOrigin); @@ +1724,5 @@ > do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID); > NS_ENSURE_TRUE(ss, NS_ERROR_FAILURE); > > nsCOMPtr<mozIStorageConnection> connection; > + nsresult rv = ss->OpenDatabaseWithFileURL(dbFileUrl, Similar here if it fits. ::: dom/quota/FileStreams.cpp @@ +34,5 @@ > } > > template <class FileStreamBase> > +void > +FileQuotaStream<FileStreamBase>::CreateQuotaObject(nsIFile* aFile) Can we just move the implementation here into DoOpen? And same for MaybeUpdateSize. Then ClearQuotaObject can be moved into Close and MaybeAllocateMoreSpace can be moved into Write? ::: dom/quota/QuotaManager.cpp @@ +198,5 @@ > QuotaManager::InitQuotaForOrigin(const nsACString& aOrigin, > int64_t aLimit, > int64_t aUsage) > { > + MutexAutoLock lock(mQuotaMutex); You can move this below the allocation, right? Just before the assertion. ::: storage/src/mozStorageConnection.cpp @@ +528,3 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + nsCOMPtr<nsIURI> uri = do_QueryInterface(aFileURL); This QI is unnecessary, nsIFileURL inherits nsIURI.

Comment on attachment 691371 [details] [diff] [review] patch v0.5 Review of attachment 691371 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the above changes. You'll need to get review from asuth or mak for the storage changes. Thanks for doing this!

man, why I didn't notice that nsIFileURL inherits from nsIURI ... anyway, it seems we're close, thanks

Andrew, please take a look at changes in storage module. If you don't have time, please let me know.

one more change: diff --git a/storage/src/mozStorageConnection.cpp b/storage/src/mozStorageConnection.cpp --- a/storage/src/mozStorageConnection.cpp +++ b/storage/src/mozStorageConnection.cpp @@ -7,18 +7,17 @@ #include <stdio.h> #include "nsError.h" #include "nsIMutableArray.h" #include "nsAutoPtr.h" #include "nsIMemoryReporter.h" #include "nsThreadUtils.h" #include "nsIFile.h" -#include "nsNetUtil.h" -#include "nsEscape.h" +#include "nsIFileURL.h" #include "mozilla/Telemetry.h" #include "mozilla/Mutex.h" #include "mozilla/CondVar.h" #include "mozilla/Attributes.h" #include "mozIStorageAggregateFunction.h" #include "mozIStorageCompletionCallback.h" #include "mozIStorageFunction.h"

Comment on attachment 691633 [details] [diff] [review] patch v0.6 This looks like a fantastic improvement over the existing test_quota approach! To summarize my understanding of what's going on here as it relates to storage: - Before this patch, the quota was tracked and enforced at a low-level by test_quota, with high-level control coming from IndexedDatabaseManager being responsible for defining origins using SetQuotaForFilenamePattern and indicating opted-out origins using the mozIStorageServiceQuotaManagement bridge interface in storage. Blobs stored outside the database were tracked/enforced by test_quota by having the file stream logic make calls to sqlite3_quota_*. - After this patch, the quota is basically entirely tracked and enforced (outside storage/SQLite) by QuotaManager. Blobs stored out the database don't exist as far as mozStorage is concerned. - We're ditching the quota VFS in favor of hooking quota support directly into the (default) telemetry VFS. - All of the API surface that got added for quota support is being removed from storage which was thoughtfully centralized in mozIStorageServiceQuotaManagement so this removal is easy. - We add support for the cool new URI method for opening databases and providing parameters at the same time. This is new API surface that will be maintained even if IndexedDB changes its quota enforcement in the future. - The Telemetry VFS now makes calls to QuotaObject/QuotaManager that will acquire a mutex and may do synchronous I/O, namely: - QuotaObject::MaybeAllocateMoreSpace, called by xWrite without stopwatch timers active, makes calls to indexedDB::IndexedDatabaseManager::QuotaIsLifted while holding the QuotaManager mutex. QuotaIsLifted has very unbounded runtime because the call blocks on dispatch to the main thread and may even wait for the user to click a button on a UI affordance like a door hanger. - QuotaManager::GetQuotaObject, called by xOpen with stopwatch timers active, makes synchronous nsIFile calls while not holding the mutex. - QuotaObject::UpdateSize, called by xTruncate with stopwatch timers active, does nothing, but does acquire a mutex. Review actions requests required for r=asuth. Which is to say, r=asuth if you do the following: - Don't hold the mutex while calling QuotaIsLifted. There aren't a lot of comments, so it's hard to tell what the rationale is for holding the mutex during the call, but it doesn't seem necessary since QuotaIsLifted and CheckQuotaHelper have no problem being called concurrently from multiple threads. The logic will need to refresh the data from the hash after re-acquiring the lock, of course. Note: It seems like QuotaIsLifted may result in some non-helpful serialization of calls to it since it continues to hold mQuotaHelper during its call to PromptAndReturnQuotaIsDisabled when it could temporarily unlock it for that call too. - Don't make calls to QuotaObject while the telemetry performance stopwatches are active and/or get Vladan's sign-off on the impact of these changes. I've cc'ed Vladan since Taras' bugzilla name says he is out of town, and he can hopefully provide a decisive answer, but it seems like we don't want our telemetry data skewed by the synchronous file I/O, crazy long UI feedback waits directly triggered by hitting quota, or super-long mutex holds brought on by such things happening while the quota manager is holding a mutex. - Either add comments to the storage calls to QuotaObject that mention the potentially long-running things that can happen in those calls, or add comments to the QuotaObject headers that document it.

(In reply to Andrew Sutherland (:asuth) from comment #25) > Comment on attachment 691633 [details] [diff] [review] > patch v0.6 > > This looks like a fantastic improvement over the existing test_quota > approach! Thanks, especially for your prompt reply > > To summarize my understanding of what's going on here as it relates to > storage: > > - Before this patch, the quota was tracked and enforced at a low-level by > test_quota, with high-level control coming from IndexedDatabaseManager being > responsible for defining origins using SetQuotaForFilenamePattern and > indicating opted-out origins using the mozIStorageServiceQuotaManagement > bridge interface in storage. Blobs stored outside the database were > tracked/enforced by test_quota by having the file stream logic make calls to > sqlite3_quota_*. Right > > - After this patch, the quota is basically entirely tracked and enforced > (outside storage/SQLite) by QuotaManager. Blobs stored out the database > don't exist as far as mozStorage is concerned. Right, basically the patch provides hooks to QuotaManager for SQLite VFS and XPCOM/Necko file streams. > > - We're ditching the quota VFS in favor of hooking quota support directly > into the (default) telemetry VFS. Right > > - All of the API surface that got added for quota support is being removed > from storage which was thoughtfully centralized in > mozIStorageServiceQuotaManagement so this removal is easy. Right > > - We add support for the cool new URI method for opening databases and > providing parameters at the same time. This is new API surface that will be > maintained even if IndexedDB changes its quota enforcement in the future. Right, however, I'm going to add additional parameter "storageType" in near future as part of temporary storage implementation for IndexedDB. So we will be touching TelemetryVFS.cpp again, but it should be tiny change. > > - The Telemetry VFS now makes calls to QuotaObject/QuotaManager that will > acquire a mutex and may do synchronous I/O, namely: It was the case with test_quota too, but it wasn't so visible. test_quota uses a similar mutex and we actually found some bugs related to this mutex. > > - QuotaObject::MaybeAllocateMoreSpace, called by xWrite without stopwatch > timers active, makes calls to > indexedDB::IndexedDatabaseManager::QuotaIsLifted while holding the > QuotaManager mutex. QuotaIsLifted has very unbounded runtime because the > call blocks on dispatch to the main thread and may even wait for the user to > click a button on a UI affordance like a door hanger. Like I said, it was the case before and bent pointed out this too, but it would be only a problem if we allowed e.g. QuotaManager::GetQuotaObject() to be called on the main thread. We don't allow it right now, there's an explicit assertion for it and we don't even plan to allow it. LocalStorage implementation should use QuotaManager in future too, I'm actually working on it, but the initial QuotaManager must land first and then temporary storage for IndexedDB. LocalStorage implementation is being rewritten from scratch right now (honza++) and all LS realated I/O will be done on a background thread, so once the LS rewrite is done, we'll be able to call into QuotaManager off the main thread. > > - QuotaManager::GetQuotaObject, called by xOpen with stopwatch timers > active, makes synchronous nsIFile calls while not holding the mutex. > > - QuotaObject::UpdateSize, called by xTruncate with stopwatch timers active, > does nothing, but does acquire a mutex. > I think I should talk to vladan ... > > Review actions requests required for r=asuth. Which is to say, r=asuth if > you do the following: > > - Don't hold the mutex while calling QuotaIsLifted. There aren't a lot of > comments, so it's hard to tell what the rationale is for holding the mutex > during the call, but it doesn't seem necessary since QuotaIsLifted and > CheckQuotaHelper have no problem being called concurrently from multiple > threads. The logic will need to refresh the data from the hash after > re-acquiring the lock, of course. Note: It seems like QuotaIsLifted may > result in some non-helpful serialization of calls to it since it continues > to hold mQuotaHelper during its call to PromptAndReturnQuotaIsDisabled when > it could temporarily unlock it for that call too. Like I explained above, this shouldn't be a problem if we don't allow calls into QuotaManager from the main thread. I also think that we can't avoid some serialization. When we reach the limit for an origin we have to block all other threads, since we don't know if the user will allow or deny allocating more than 50 MB for an origin. If this shows up as a performance problem in future, we can fix it to use per origin mutexes, but I doubt that, because we don't hold the mutex during I/O, only when we update data structures. It's up to higher layers to handle concurrent write access to the same file. I'm not sure I understand how we would "refresh the data from the hash after re-acquiring the lock". > > - Don't make calls to QuotaObject while the telemetry performance > stopwatches are active and/or get Vladan's sign-off on the impact of these > changes. I've cc'ed Vladan since Taras' bugzilla name says he is out of > town, and he can hopefully provide a decisive answer, but it seems like we > don't want our telemetry data skewed by the synchronous file I/O, crazy long > UI feedback waits directly triggered by hitting quota, or super-long mutex > holds brought on by such things happening while the quota manager is holding > a mutex. Ok, I will talk to vladan about this. It was done this wasy before (just wasn't visible), but I think we can make it better or document it at least. > > - Either add comments to the storage calls to QuotaObject that mention the > potentially long-running things that can happen in those calls, or add > comments to the QuotaObject headers that document it. Ok, this looks reasonable

(In reply to Jan Varga [:janv] from comment #26) > > - Don't make calls to QuotaObject while the telemetry performance > > stopwatches are active and/or get Vladan's sign-off on the impact of these > > changes. I've cc'ed Vladan since Taras' bugzilla name says he is out of > > town, and he can hopefully provide a decisive answer, but it seems like we > > don't want our telemetry data skewed by the synchronous file I/O, crazy long > > UI feedback waits directly triggered by hitting quota, or super-long mutex > > holds brought on by such things happening while the quota manager is holding > > a mutex. > > Ok, I will talk to vladan about this. > It was done this wasy before (just wasn't visible), but I think we can make > it better or document it at least. What about doing it in a followup bug ? TelemetryVFS now calls original VFS that is actually the quota VFS (test_quota) So all those stopwatches are currently affected by the mutex in test_quota anyway.

(In reply to Jan Varga [:janv] from comment #26) > > - Don't hold the mutex while calling QuotaIsLifted. There aren't a lot of > > comments, so it's hard to tell what the rationale is for holding the mutex > > during the call, but it doesn't seem necessary since QuotaIsLifted and > > CheckQuotaHelper have no problem being called concurrently from multiple > > threads. The logic will need to refresh the data from the hash after > > re-acquiring the lock, of course. Note: It seems like QuotaIsLifted may > > result in some non-helpful serialization of calls to it since it continues > > to hold mQuotaHelper during its call to PromptAndReturnQuotaIsDisabled when > > it could temporarily unlock it for that call too. > > Like I explained above, this shouldn't be a problem if we don't allow calls > into QuotaManager from the main thread. I also think that we can't avoid > some serialization. When we reach the limit for an origin we have to block > all other threads, since we don't know if the user will allow or deny > allocating more than 50 MB for an origin. > If this shows up as a performance problem in future, we can fix it to use > per origin mutexes, but I doubt that, because we don't hold the mutex during > I/O, only when we update data structures. It's up to higher layers to handle > concurrent write access to the same file. > I'm not sure I understand how we would "refresh the data from the hash after > re-acquiring the lock". To elaborate, I agree that it makes sense that all the threads for the same origin should end up blocked pending the outcome of the user's decision. But with this patch (as I read it), what happens is that *everything* quota managed will end up blocked pending the outcome of the user's decision. So if an IndexedDB page in a tab that's in the background hits its quota, IndexedDB in the foreground tab will stop working even if it's nowhere near its quota. It's my impression that the door-hanger mechanism for a background tab is not going to trigger while you are not looking (but it's been 6 months since I checked that code out) so this could take quite some time to resolve. This seems easily resolved by dropping the QuotaManager mutex while calling into QuotaIsLifted. The non-helpful serialization I am talking about there is that QuotaIsLifted holds the mQuotaHelper mutex during its call to PromptAndReturnQuotaIsDisabled. So if origin A hits its quota limit and dispatches the CheckQuotaHelper to the main thread followed by origin B hitting its quota, origin B won't dispatch a CheckQuotaHelper to the main thread until the user has decided what to do about the situation for origin A. This situation can be improved by having QuotaIsLifted drop the mQuotaHelper mutex during the call to PromptAndReturnQuotaIsDisabled. The net result of these improvements is that if one or more origins end up hitting their limits, all other quota operations for different origins can continue to operate. All requests for the limit-hitting origins will just end up waiting in PromptAndReturnQuotaIsDisabled. The "refresh the data from the hash after re-acquiring the lock" bit is that with these, changes, you could have accesses to the same origin on different threads where one will request an increment that it would put it over the quota (and hence end up in PromptAndReturnQuotaIsDisabled), and then the other will request an increment that will not put it over the quota. As the patch stands, the small one currently wouldn't succeed, but if you make the changes, it would succeed, so all the values from the QuotaObject would need to be re-fetched after re-acquiring the mutex. I do understand that the test_quota pMutex is currently held for similar durations, but I don't see a convincing argument for continuing to do that; as I describe above, it seems like 2 minor code changes are required and the long-mutex-holding will end.

(In reply to Jan Varga [:janv] from comment #27) > What about doing it in a followup bug ? > TelemetryVFS now calls original VFS that is actually the quota VFS > (test_quota) > So all those stopwatches are currently affected by the mutex in test_quota > anyway. If Vladan says a followup bug is fine, then a followup bug is fine.

Andrew, I'll try to do it, thanks for the explanation.

(In reply to Andrew Sutherland (:asuth) from comment #28) Thanks for looking at this Andrew! I have a few comments/corrections here: > But with this patch (as I read it), what happens is that *everything* quota > managed will end up blocked pending the outcome of the user's decision. This is true. > This seems easily resolved by dropping the QuotaManager mutex while calling > into QuotaIsLifted. No, the real solution will be to only have one mutex for all this (quotamanager and indexeddbmanager). This is why I insisted that we file bug 820715 and we need to work on it ASAP. > The non-helpful serialization I am talking about there > is that QuotaIsLifted holds the mQuotaHelper mutex during its call to > PromptAndReturnQuotaIsDisabled. So if origin A hits its quota limit and > dispatches the CheckQuotaHelper to the main thread followed by origin B > hitting its quota, origin B won't dispatch a CheckQuotaHelper to the main > thread until the user has decided what to do about the situation for origin > A. This isn't true, the PromptAndReturnQuotaIsDisabled drops the mutex while the CheckQuotaHelper is running. See http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/CheckQuotaHelper.cpp#85 . Multiple origins can hit their quotas and display prompts at the same time. > The "refresh the data from the hash after re-acquiring the lock" bit is that This is basically already done in the exiting code (in QuotaIsLiftedInternal). I would much prefer we simply unify the two locks in bug 820715. > I do understand that the test_quota pMutex is currently held for similar > durations, but I don't see a convincing argument for continuing to do that; Sure, we could do this in a followup though.

(In reply to ben turner [:bent] from comment #31) > No, the real solution will be to only have one mutex for all this > (quotamanager and indexeddbmanager). This is why I insisted that we file bug > 820715 and we need to work on it ASAP. My concern is that these things tend to not actually get worked. Since this bug itself is a cleanup bug, it suggests I am not dealing with lazy people like myself, but people interested in hygienically precise bugs. So as long as bug 820715 actually gets marked as blocking the feature motivating this work and treated that way, I'm now fine with punting that change to that bug. Note: It would be terrifying if the code still looked the same as it does now but QuotaIsLiftedInternal started dropping and reacquiring mutexes inside of itself, so everybody please make sure that a good comment happens before the call and/or the header declarations of QuotaIsLifted(Internal) get a good comment, etc. > This isn't true, the PromptAndReturnQuotaIsDisabled drops the mutex while > the CheckQuotaHelper is running. See > http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/CheckQuotaHelper. > cpp#85 . Multiple origins can hit their quotas and display prompts at the > same time. Oops! I missed that the mutex was being passed into the CheckQuotaHelper constructor and so they are all using the same mutex and the condition variable will drop the mutex while waiting. That is good.

I'll make the followup to have just one mutex, my priority, I promise :) Seriously, we all care about good code. vladan, it seems that your input here is the last thing that prevents this from landing, would be great if we could land the patch this week

(In reply to Jan Varga [:janv] from comment #27) > What about doing it in a followup bug ? > TelemetryVFS now calls original VFS that is actually the quota VFS > (test_quota) > So all those stopwatches are currently affected by the mutex in test_quota > anyway. This rationale seems fine to me, but I need to look at the current VFS chaining and a bit of the test_quota code before I can feel confident. I apologize for the delay but I haven't looked at TelemetryVFS or test_quota code until now. (In reply to Jan Varga [:janv] from comment #26) > LocalStorage implementation is being rewritten from > scratch right now (honza++) and all LS realated I/O will be done on a > background thread, so once the LS rewrite is done, we'll be able to call > into QuotaManager off the main thread. Not quite. Honza's current rewrite patch actually does the quota check on the main thread. This is not an implementation quirk, LocalStorage is a synchronous API and quota has to be checked before the main thread can return from the synchronous localStorage.setItem() call. So even if quota checks were done on a helper thread, the main thread would have to block on the result of the quota check. This issue can be mitigated by extending Honza's preload mechanism so that we pre-fetch quota use before it is needed, but we will not always win this race: setItem() could get called before the pre-fetch completes, resulting in main thread blocking. As an aside, note that event-loop spinning is not considered an acceptable solution to main-thread blocking as it introduces a whole host of other problems.

(In reply to Vladan Djeric (:vladan) from comment #34) > (In reply to Jan Varga [:janv] from comment #27) > > What about doing it in a followup bug ? > > TelemetryVFS now calls original VFS that is actually the quota VFS > > (test_quota) > > So all those stopwatches are currently affected by the mutex in test_quota > > anyway. > > This rationale seems fine to me, but I need to look at the current VFS > chaining and a bit of the test_quota code before I can feel confident. I > apologize for the delay but I haven't looked at TelemetryVFS or test_quota > code until now. ok > > (In reply to Jan Varga [:janv] from comment #26) > > LocalStorage implementation is being rewritten from > > scratch right now (honza++) and all LS realated I/O will be done on a > > background thread, so once the LS rewrite is done, we'll be able to call > > into QuotaManager off the main thread. > > Not quite. Honza's current rewrite patch actually does the quota check on > the main thread. This is not an implementation quirk, LocalStorage is a > synchronous API and quota has to be checked before the main thread can > return from the synchronous localStorage.setItem() call. So even if quota > checks were done on a helper thread, the main thread would have to block on > the result of the quota check. Well, it looks we're going to do two kinds of quota checking in LS implementation. The one that calls into QuotaManager should be done from a background thread. see https://bugzilla.mozilla.org/show_bug.cgi?id=767944#c5 > > This issue can be mitigated by extending Honza's preload mechanism so that > we pre-fetch quota use before it is needed, but we will not always win this > race: setItem() could get called before the pre-fetch completes, resulting > in main thread blocking. > > As an aside, note that event-loop spinning is not considered an acceptable > solution to main-thread blocking as it introduces a whole host of other > problems. yeah, the other alternative is to block scripts until the preloading is finished however, it's not easy to cover all the cases

(In reply to Vladan Djeric (:vladan) from comment #34) > Not quite. Honza's current rewrite patch actually does the quota check on > the main thread. This is not an implementation quirk, LocalStorage is a > synchronous API and quota has to be checked before the main thread can > return from the synchronous localStorage.setItem() call. So even if quota > checks were done on a helper thread, the main thread would have to block on > the result of the quota check. > > This issue can be mitigated by extending Honza's preload mechanism so that > we pre-fetch quota use before it is needed, but we will not always win this > race: setItem() could get called before the pre-fetch completes, resulting > in main thread blocking. > > As an aside, note that event-loop spinning is not considered an acceptable > solution to main-thread blocking as it introduces a whole host of other > problems. Exactly described, thanks Vladan. The new patch as now sits in bugzilla and as I tend to land it does only a per-origin quota check. (the current implementation, though, does eTLD+1 as suggested by the DOM storage spec ; I've loosen this requirement based on talk with Jonas) Hence, we can check we are not going over the 5MB quota only after all data for an origin has been read from the database (or after some other usage enumeration, that can happen on the background thread). In bug 600307 there is also an additional patch introducing eTLD+1 check, but it works fully asynchronously and may sometimes also loose the race with the first write access. Since localStorage is source of performance issues and uses small 5MB limit and should be used for non-critical data, we may consider be less precise on the quota check. (In reply to Jan Varga [:janv] from comment #35) > yeah, the other alternative is to block scripts until the preloading is > finished > however, it's not easy to cover all the cases I am using a modified Nightly with my DOM storage patch on 3 machines now to get some telemetry for preloads (links at the bottom of http://www.janbambas.cz/new-faster-localstorage-in-firefox-21/ if anyone interested). Also according our current telemetry data loads from the database seems to be fast. Script blocking may be what we need, since block times will usually be just tens of milliseconds tops. Its better to delay script execution then to block UI main loop or page rendering for this time.

(In reply to Jan Varga [:janv] from comment #27) > > Ok, I will talk to vladan about this. > > It was done this wasy before (just wasn't visible), but I think we can make > > it better or document it at least. > > What about doing it in a followup bug ? > TelemetryVFS now calls original VFS that is actually the quota VFS > (test_quota) > So all those stopwatches are currently affected by the mutex in test_quota > anyway. I agree, nothing changes with respect to Telemetry timers other than some of the quota work getting moved outside the scope of the timer in xWrite. I am OK with fixing the existing problems with Telemtry VFS timings in a new bug. (In reply to Honza Bambas (:mayhemer) from comment #36) > Exactly described, thanks Vladan. The new patch as now sits in bugzilla and > as I tend to land it does only a per-origin quota check. (the current > implementation, though, does eTLD+1 as suggested by the DOM storage spec ; > I've loosen this requirement based on talk with Jonas) Hence, we can check > we are not going over the 5MB quota only after all data for an origin has > been read from the database (or after some other usage enumeration, that can > happen on the background thread). > In bug 600307 there is also an additional patch introducing eTLD+1 check, but > it works fully asynchronously and may sometimes also loose the race with the > first write access. You could fetch eTLD+1 quota use during preload as I do during main-thread fetch in bug 807021, but perhaps that's not compatible with the future direction of LocalStorage (e.g. SQLite DB per origin). Lost preload races could be handled with main thread blocking. > http://www.janbambas.cz/new-faster-localstorage-in-firefox-21/ if anyone > interested). Also according our current telemetry data loads from the > database seems to be fast. Script blocking may be what we need, since block > times will usually be just tens of milliseconds tops. Its better to delay > script execution then to block UI main loop or page rendering for this time. Script blocking would unnecessarily delay page loading for pages that don't use LocalStorage, but we can only figure out cost vs benefit with field data.

(In reply to Vladan Djeric (:vladan) from comment #37) > (In reply to Jan Varga [:janv] from comment #27) > > > Ok, I will talk to vladan about this. > > > It was done this wasy before (just wasn't visible), but I think we can make > > > it better or document it at least. > > > > What about doing it in a followup bug ? > > TelemetryVFS now calls original VFS that is actually the quota VFS > > (test_quota) > > So all those stopwatches are currently affected by the mutex in test_quota > > anyway. > > I agree, nothing changes with respect to Telemetry timers other than some of > the quota work getting moved outside the scope of the timer in xWrite. I am > OK with fixing the existing problems with Telemtry VFS timings in a new bug. > ok, I'll file a new followup bug thanks all for the reviews and comments! > > http://www.janbambas.cz/new-faster-localstorage-in-firefox-21/ if anyone > > interested). Also according our current telemetry data loads from the > > database seems to be fast. Script blocking may be what we need, since block > > times will usually be just tens of milliseconds tops. Its better to delay > > script execution then to block UI main loop or page rendering for this time. > > Script blocking would unnecessarily delay page loading for pages that don't > use LocalStorage, but we can only figure out cost vs benefit with field data. Yeah, we could also preload only the raw file for an origin, and do all the processing synchronously on the main thread. That would save some cycles. But I didn't have time to think about all the details of this approach. Also, once LS moves to temporary storage, some unused data might just disappear automatically in time and then the preloading wouldn't be needed anymore (e.g pages that switched to some other storage API). Anyway, as you said, it's only a guess.

\o/