I should be able to post an initial patch soon ... It's still far from being complete, but it would be good to discuss the design at this point. The design differs a bit from what we were talking about. The file refcounting is a bit tricky :)

initial patch, some work still needs to be done

Here's a list of what I think we should consider/solve: 1. High priority issues - delete files on thread pool or IO thread ? (current implementation dispatches it to thread pool) - how to delete files when there's no live database for it? (open database, delete file(s) and close database or just queue it and delete when the database is opened again ?) - how and when invalidate file manager(s) (need to coordinate with deleteDatabase() and setVersion() patches) - file copying, is it efficient enough ? (it uses a 32k buffer on stack) - verify that SQL triggers don't hurt performance, the update_refcount() function is called only when OLD.file_ids or NEW.file_ids is not null, so it affects only objects with blobs/files The update_refcount function is not created for readonly transactions. However I'm not sure how much it slows down sqlite internally. Other P1s in my opinion: - fix quota management to count files too - don't duplicate already stored "source" files for example: trans.objectStore("objectstore1").put(myblob, "somekey"); trans.objectStore("objectstore2").put(myblob, "otherkey"); should store myblob only once 2. Medium priority - don't duplicate slices in db (if a slice of a file is stored in the same database as the file) - queue delete file requests (using a timer ?) and do it in one transaction - try to remove useless addref/release of file info objects in some places 3. Low priority - collapse small blobs - split original file when it's referenced only by slices (consider a 4 GB file. If the first 10 bytes and the last 10 bytes are stored as slices in the DB, we probably don't want to store the entire 4 GB file) - file sharing across databases - store files in multiple directories - reuse released ids (really low priority, ids are defined as PRInt64) Let me know if the priorities are ok

Here's how I would prioritize: P1. These are things required to ship: - fix quota management to count files too - don't duplicate already stored "source" files for example: trans.objectStore("objectstore1").put(myblob, "somekey"); trans.objectStore("objectstore2").put(myblob, "otherkey"); should store myblob only once P2. These give better performance in common cases: - collapse small blobs - queue delete file requests (using a timer ?) and do it in one transaction P3. We should do this once we have a levelDB backend: - file sharing across databases P4. These give better performance in rare cases: - don't duplicate slices in db (if a slice of a file is stored in the same database as the file) - split original file when it's referenced only by slices (consider a 4 GB file. If the first 10 bytes and the last 10 bytes are stored as slices in the DB, we probably don't want to store the entire 4 GB file) P5. I don't think we should do this at all: - reuse released ids (really low priority, ids are defined as PRInt64) P?. I don't know what these mean, so I don't know how to prioritize: - try to remove useless addref/release of file info objects in some places - store files in multiple directories

(In reply to Jonas Sicking (:sicking) from comment #5) > Here's how I would prioritize: > > P1. These are things required to ship: > - fix quota management to count files too > - don't duplicate already stored "source" files > for example: > trans.objectStore("objectstore1").put(myblob, "somekey"); > trans.objectStore("objectstore2").put(myblob, "otherkey"); > should store myblob only once > > P2. These give better performance in common cases: > - collapse small blobs > - queue delete file requests (using a timer ?) and do it in one transaction > > P3. We should do this once we have a levelDB backend: > - file sharing across databases > > P4. These give better performance in rare cases: > - don't duplicate slices in db > (if a slice of a file is stored in the same database as the file) > - split original file when it's referenced only by slices > (consider a 4 GB file. If the first 10 bytes and the last 10 bytes are > stored as slices in the DB, we probably don't want to store the entire 4 GB > file) > ok, sounds reasonable > P5. I don't think we should do this at all: > - reuse released ids (really low priority, ids are defined as PRInt64) ok, no problem > > P?. I don't know what these mean, so I don't know how to prioritize: > - try to remove useless addref/release of file info objects in some places it might be possible to use "FileInfo* fileInfo| instead of |nsRefPtr<FileInfo> fileInfo| in some places of the code FileInfo is thread safe > - store files in multiple directories blobs/files are now copied to just one directory, this can be a problem if there are too many files, say 1000-10000 or more

(In reply to Jan Varga from comment #4) > - how to delete files when there's no live database for it? (open database, > delete file(s) and close database or just queue it and delete when the > database is opened again ?) Ben, do you have an opinion on that ?

(In reply to Jan Varga from comment #7) > Ben, do you have an opinion on that ? I don't really understand the situation you're describing, I don't think. Why would you need an open database to delete files? Oh, and let's please use the IO thread for deleting files, that's more or less exactly what it's there for. The transaction thread pool really is just for transactions.

Sure, if we want to remove files on the IO thread then it's completely different situation. I was not sure about it. I thought the IO thread could be a bottleneck if multiple databases are removing files at once. Anyway, now it's clear, thanks

So the situation is something like this. Before getting into the actual steps, keep in mind that we with this patch will have a new sqlite table which keeps track of file metadata. This is needed in order to be able to share os-file if the same blob is stored multiple in a database. 1. Transaction 1 is started 2. Blob is inserted into a database 3. Add a row to the file-table (the table keeping all the file metadata) 4. Transaction 1 is committed. 5. Transaction 2 is started. 6. Blob is read from the database and a in-memory Blob object is created which refers to the OS-file. 7. The blob is removed from the database. 8. Since the in-memory Blob still exists, we don't remove the row from the file-table. 9. Transaction 2 is committed. 10. The database is closed. 11. The reference to the in-memory Blob object is released. With the approach currently in the patch, we'll at this point want to delete the row in the database as well as delete the OS-file. This is a problem if the database is no longer open. However, I realized that we can change the approach given the discussion me and Jan had on irc yesterday. In step 8, when the refcount goes to zero, delete the row from the file-table but don't delete the OS-file. Instead simply set the notInFileTable flag on the file-info object so that when the file-info is deleted from memory, we just kill the OS-file, but there's no need to make database modifications! That also neatly solves the following action item: - queue delete file requests (using a timer ?) and do it in one transaction since we no longer need to touch the database at all!

... and when the in-memory Blob object is stored in a database again, we create a row in file table again using metadata in file-info object. It seems it should work. The reason I've done that differently is that I planned to collapse small blobs by storing them in a separate (per idb database) sqlite database intended just for small blobs. We would ATTACH to it before starting a transaction and we would have to touch it when a small blob is removed from an idb database. This separate database could be opened in read uncommitted mode, so reads would never be blocked by writes. Small blobs don't change, so the read uncommitted mode shouldn't be a problem I think. I also investigated the http cache design, but I'm not sure how we would handle transactions if we used a similar design to store small blobs. It would be a lot of work for sure and I think sqlite can do it for us too. There are some perf numbers (by D. Richard Hipp): http://www.mail-archive.com/sqlite-users@sqlite.org/msg29089.html So for small blobs like <16KB or even <64KB, sqlite is faster than disk IO. and it handles transactions for us, even across databases. Anyway, I didn't realize that we can optimize file deletion when a blob is stored as OS file. Thanks for pointing it out.

Comment on attachment 562847 [details] [diff] [review] patch v0.1 I looked over what you've got here and I think it looks really, really nice! There are some things that I would change but most are trivial and can wait for review. One thing that I noticed, though, is that we could store the metadata about the blobs (name, size, mime) as part of the structured clone so that we don't have to look it up in the file table (or keep it around in memory). Also, I don't think we should have a separate nsIStoredBlob interface, you can just add your [noscript] things to nsIDOMBlob.

(In reply to Jonas Sicking (:sicking) from comment #5) > Here's how I would prioritize: > > P1. These are things required to ship: > - fix quota management to count files too I fixed this by adding support for "virtual" files to mozStorage. It looks like this: ss = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID); nsCOMPtr<mozIStorageVirtualFile> virtualFile; ss->GetVirtualFile(NS_LITERAL_CSTRING("quota"), getter_AddRefs(virtualFile)); virtualFile->Init(file); // file is an nsIFile virtualFile->Open(); virtualFile->Write(copyBuffer, numRead, offset); virtualFile->Sync(); virtualFile->Close(); all these methods forward to quota vfs (test_quota.c) I tried other solutions, but this one is the most elegant I think The performance seems to be ok, and the quota is handled perfectly. For example when I have an empty db and I try to store a 1 GB file and the quota is set to 50 MB, I get the quota exceeded dialog when first 50 MBs of the file are copied. So in theory when there are two parallel transactions, one is removing files and another is storing new files, the quota won't exceed. Also, we use the same "sync" method as SQLite does. I couldn't simply decrease the quota by file size before copying it and then wait for SQLite to write to a db file to trigger a quota check. A quota check is triggered only when db filed expand. > - don't duplicate already stored "source" files > for example: > trans.objectStore("objectstore1").put(myblob, "somekey"); > trans.objectStore("objectstore2").put(myblob, "otherkey"); > should store myblob only once > I fixed it by adding: class nsDOMFileBase { ... class FileIdEntry { public: FileIdEntry(indexedDB::FileManager* aFileManager, PRInt64 aId) : mFileManager(aFileManager), mId(aId) { } ~FileIdEntry() { } nsRefPtr<indexedDB::FileManager> mFileManager; PRInt64 mId; }; // List of stored files for which this blob was used as a source. // We don't store file infos here intentionally, a strong ref would block // removal of unreferenced files. nsTArray<nsAutoPtr<FileIdEntry> > mFileIdEntries; } So every time a blob/file is stored in indexedDB a new entry is created. The list of entries is checked before a blob is copied. If an entry exists and it was stored in the same database then the file is not copied, we just increase the reference count in db. I also found out how we could collapse small blobs when they need to be shared across databases. The problem is that a write transaction locks entire db/table in SQLite. So a global SQLite database for small blobs would be a bottleneck. Having separate "small blob" database for each real database is problematic too. Instead we could have a pool of small blob databases like we have a pool of threads. We could then find "not yet used" small blob database or just create a new one before starting a transaction. e.g. "blobs_001.sqlite", "blobs_002.sqlite", etc. This small blob database would be attached to the main db connection. FileInfo would then contain another member - mType (0 would mean standalone OS file, 1 - stored in blobs_001.sqlite, 2 - stored in blobs_002.sqlute, etc.) Global (per origin) file manager would then cache db connections to these small blob databases. These connections would be used only to read small blobs (not write)

patch ready for review

(In reply to Jonas Sicking (:sicking) from comment #5) > Here's how I would prioritize: > > P1. These are things required to ship: > - fix quota management to count files too > - don't duplicate already stored "source" files > for example: > trans.objectStore("objectstore1").put(myblob, "somekey"); > trans.objectStore("objectstore2").put(myblob, "otherkey"); > should store myblob only once this is fixed in last patch (In reply to ben turner [:bent] from comment #8) > Oh, and let's please use the IO thread for deleting files, that's more or > less exactly what it's there for. The transaction thread pool really is just > for transactions. fixed in last patch (In reply to Jonas Sicking (:sicking) from comment #10) > In step 8, when the refcount goes to zero, delete the row from the > file-table but don't delete the OS-file. Instead simply set the > notInFileTable flag on the file-info object so that when the file-info is > deleted from memory, we just kill the OS-file, but there's no need to make > database modifications! > > That also neatly solves the following action item: > - queue delete file requests (using a timer ?) and do it in one transaction > since we no longer need to touch the database at all! fixed in last patch... Actually it seems we don't even need the notInFileTable flag. There's just a database trigger which automatically removes rows with zero refcount. On the other side when a blob is removed from the database, but there's still an in-memory blob and this blob is stored again, the os file and id is recycled. A new row is added to file table automatically by checking "affected" rows in UpdateRefcountFunction::DatabaseUpdateFunction() (In reply to ben turner [:bent] from comment #12) > One thing that I noticed, though, is that we could store the metadata about > the blobs (name, size, mime) as part of the structured clone so that we > don't have to look it up in the file table (or keep it around in memory). fixed in last patch I moved isFile and name attribute from file table to structured clone. It seems that size can be obtained from the stored file. Now I realized that I somehow forgot to handle the mime property :( > > Also, I don't think we should have a separate nsIStoredBlob interface, you > can just add your [noscript] things to nsIDOMBlob. I didn't find a way to avoid that.

(In reply to Jan Varga [:janv] from comment #15) > > Also, I don't think we should have a separate nsIStoredBlob interface, you > > can just add your [noscript] things to nsIDOMBlob. > > I didn't find a way to avoid that. ok, I merge it into nsIDOMBlob interface also, automatic tests are almost finished

marking for sec-review, sched 10-Oct-2011

Invalidation of file managers during shutdown and after "clear origin" is mostly done too. Now I'm preparing for merges with bugs 697247, 692669 and 694145 :)

I have a problem with adding a new test directory ... There's dom/indexedDB/test currently and I want to add a new subdir called "file" or "files. So the full path is dom/indexedDB/test/file Once I do it, I get a failure in mochitest-4 on the try server, but only on win7. Here's the log: 40 ERROR TEST-UNEXPECTED-FAIL | /tests/embedding/test/test_window_open_units.html | wrong width - got 398, expected 400 41 ERROR TEST-UNEXPECTED-FAIL | /tests/embedding/test/test_window_open_units.html | wrong height - got 198, expected 200 It seems that the new test directory, dom/indexedDB/test/file causes embedding/test to move from the end of mochitest-3 to the beginning of mochitest-4 Who would know how to fix that ?

ok, I'm going to just prefix all the tests: test_file_XXX.html

Sounds good. We've been trying to reduce the number of directories that we put source and tests in, so test_file_... sounds great.

mozIStorageVirtualFile could be renamed to mozIStorageFile I think

Comment on attachment 574628 [details] [diff] [review] patch v0.3 >diff --git a/content/base/public/nsDOMFile.h b/content/base/public/nsDOMFile.h >--- a/content/base/public/nsDOMFile.h >+++ b/content/base/public/nsDOMFile.h >@@ -239,24 +269,26 @@ protected: > } > virtual already_AddRefed<nsIDOMBlob> > CreateSlice(PRUint64 aStart, PRUint64 aLength, > const nsAString& aContentType); > > friend class DataOwnerAdapter; // Needs to see DataOwner > class DataOwner { > public: >- NS_INLINE_DECL_REFCOUNTING(DataOwner) >+ NS_IMETHOD_(nsrefcnt) AddRef(); >+ NS_IMETHOD_(nsrefcnt) Release(); > DataOwner(void* aMemoryBuffer) > : mData(aMemoryBuffer) > { > } > ~DataOwner() { > PR_Free(mData); > } >+ nsAutoRefCnt mRefCnt; > void* mData; > }; > > // Used when backed by a memory store > nsRefPtr<DataOwner> mDataOwner; > }; this was fixed by bug 700512

Comment on attachment 574628 [details] [diff] [review] patch v0.3 Review of attachment 574628 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good! Lots of comments below, but don't be discouraged, it's a very big patch! A bunch of the comments below are little nit-y style things, no big deal. There are a few bigger issues hidden among them, however. ::: content/base/public/nsDOMFile.h @@ +69,5 @@ > nsresult NS_NewBlobBuilder(nsISupports* *aSupports); > > +using namespace mozilla::dom; > + > +class nsDOMFile This doesn't appear to really be useful. Just add your observer logic to nsDOMFileBase? @@ +170,1 @@ > : nsDOMFileBase(EmptyString(), EmptyString(), PR_UINT64_MAX), This seems wrong... Why not pass the name arg through? @@ +274,5 @@ > friend class DataOwnerAdapter; // Needs to see DataOwner > class DataOwner { > public: > + NS_IMETHOD_(nsrefcnt) AddRef(); > + NS_IMETHOD_(nsrefcnt) Release(); Pretty sure we have a threadsafe inline version now (it's brand new), please just change the macro. @@ +282,5 @@ > } > ~DataOwner() { > PR_Free(mData); > } > + nsAutoRefCnt mRefCnt; Should be unnecessary. ::: content/base/public/nsIDOMFile.idl @@ +62,1 @@ > interface nsIDOMBlob : nsISupports All the methods/attributes you add here should have some comments associated... What they do, when they should or shouldn't be called, etc. @@ +76,5 @@ > + [noscript] void addDestructionObserver(in FileInfo aObserver); > + [noscript] void removeDestructionObserver(in FileInfo aObserver); > + > + [noscript] void init(in FileInfo aFileInfo); > + [noscript] readonly attribute FileInfo fileInfo; Since this is noscript anyway just make it [notxpcom] also and then it will just return a FileInfo* instead of nsresult. @@ +77,5 @@ > + [noscript] void removeDestructionObserver(in FileInfo aObserver); > + > + [noscript] void init(in FileInfo aFileInfo); > + [noscript] readonly attribute FileInfo fileInfo; > + readonly attribute DOMString mozFileId; Do we want this to be scriptable? I don't think so... @@ +86,4 @@ > interface nsIDOMFile : nsIDOMBlob > { > readonly attribute DOMString name; > + [noscript] attribute AUTF8String utf8Name; Not sure this is worth having. Why not just convert 'name' when you need this? ::: content/base/src/nsDOMFile.cpp @@ +136,5 @@ > } > > +nsDOMFile::nsDOMFile() > +{ > + if (NS_AtomicIncrementRefcnt(gDOMFileInstanceCount) == 1) { This isn't actually safe... The increments/decrements are going to be atomic but the read/write of gDOMFileMutex is free to race with any other thread. Creating a mutex, for instance, involves a malloc and many other instructions. The scheduler could switch to another thread before setting gDOMFileMutex and then that thread might assume it would have a mutex based on a non-zero refcount. We need to figure out another way to do this... @@ +188,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsDOMFileBase::GetUtf8Name(nsACString& aName) Let's remove these. @@ +346,5 @@ > +nsDOMFileBase::AddDestructionObserver(indexedDB::FileInfo* aObserver) > +{ > + MutexAutoLock lock(*gDOMFileMutex); > + > + if (!mDestructionObservers.AppendElement(aObserver)) { How about asserting !Contains(aObserver) so we don't end up with duplicates? @@ +348,5 @@ > + MutexAutoLock lock(*gDOMFileMutex); > + > + if (!mDestructionObservers.AppendElement(aObserver)) { > + NS_WARNING("Out of memory?"); > + return NS_ERROR_OUT_OF_MEMORY; Thankfully nsTArray has been made infallible. This code will never run, so just remove it. @@ +369,5 @@ > + > +NS_IMETHODIMP > +nsDOMFileBase::Init(indexedDB::FileInfo* aFileInfo) > +{ > + return NS_OK; You intend for this to be overwritten, yes? Add an NS_NOTREACHED() explaining that and return NS_ERROR_UNEXPECTED. Same for the other two below. @@ +467,1 @@ > nsCOMPtr<nsISupports> file = do_QueryObject(new nsDOMFileFile()); Hm, don't you want a blob here? @@ +608,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP > +nsDOMFileFile::Init(indexedDB::FileInfo* aFileInfo) Which thread should this be called on? Any? I hope it's main only since GetMozFileId seems to restrict usage to the main thread... @@ +616,5 @@ > + } > + > + if (!aFileInfo) { > + return NS_ERROR_INVALID_ARG; > + } NS_ENSURE_ARG_POINTER is what you want here. @@ +626,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +nsDOMFileFile::GetFileInfo(indexedDB::FileInfo** aFileInfo) Actually, I think this is doing the wrong thing. What we really want is for this object to hold a list of FileInfos for per database. Something like this: struct LiveFileInfos { nsIAtom* mDatabaseId; nsRefPtr<FileInfo> mFileInfo; }; class nsDOMFileFile { ... nsTArray<LiveFileInfos> mLiveFileInfos; }; Then GetFileInfo should take the databaseId as a parameter. @@ +636,5 @@ > +NS_IMETHODIMP > +nsDOMFileFile::GetMozFileId(nsAString& aFileId) > +{ > + if (mFileInfo) { > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); Is this only true if we have a mFileInfo? Could another thread be in the process of modifying mFileInfo when this is called (do we need a lock)? What guarantees that we can't call into IsCallerTrustedForCapability() off the main thread in an optimized build? @@ +670,5 @@ > return DataOwnerAdapter::Create(mDataOwner, mStart, mLength, aStream); > } > > +NS_IMPL_THREADSAFE_ADDREF(nsDOMMemoryFile::DataOwner) > +NS_IMPL_THREADSAFE_RELEASE(nsDOMMemoryFile::DataOwner) khuey should be fixing this in a different bug, let's not mess with this in this patch. ::: dom/indexedDB/DatabaseInfo.h @@ +152,5 @@ > IndexInfo info; > Key value; > }; > > +struct StructuredCloneReadInfo { This stuff doesn't belong in DatabaseInfo.h... Maybe IndexedDatabase.h? @@ +170,5 @@ > + PRUint64 offsetToKeyProp; > + nsTArray<nsCOMPtr<nsIDOMBlob> > blobs; > +}; > + > +struct StructuredCloneEntry { As far as I can tell StructuredCloneReadInfo is always used in conjunction with a JSAutoStructuredCloneBuffer. Let's just add a JSAutoStructuredCloneBuffer member to StructuredCloneReadInfo and remove StructuredCloneEntry altogether. ::: dom/indexedDB/FileInfo.cpp @@ +43,5 @@ > +USING_INDEXEDDB_NAMESPACE > + > +FileInfo::~FileInfo() > +{ > + NS_ASSERTION(NS_IsMainThread(), "File info destroyed on wrong thread!"); If this is all the destructor is going to do then make it #ifdef DEBUG in the header. That way the compiler can inline it in an opt build. @@ +47,5 @@ > + NS_ASSERTION(NS_IsMainThread(), "File info destroyed on wrong thread!"); > +} > + > +void > +FileInfo::AddRef() Hm, lots of duplicated code in these... How about doing this: void UpdateReferences(nsAutoRefCnt& aRefCount, PRInt32 aDelta, bool aClear = false) { { MutexAutoLock lock(mFileManager->mMutex); aRefCount = aClear ? 0 : aRefCount + aDelta; if (mRefCnt + mDBRefCnt + mSliceRefCnt > 0) { return; } LockedCleanup(); } Cleanup(); delete this; } @@ +130,5 @@ > + delete this; > +} > + > +nsresult > +FileInfo::CacheBlob(nsIDOMBlob* aBlob) I don't understand the purpose of holding the cached blob here. You never seem to use it, you hold a weak ref so it can't help keep the blob alive, and there is no accessor method to get it out of a FileInfo. It just seems to add complexity, requiring the synchronization of the cached blob index hashtable and the destruction observers... Can't we just move all this code directly into the file manager? @@ +141,5 @@ > + mCachedBlob = aBlob; > + > + if (!mFileManager->mCachedBlobIndex.Put(mCachedBlob, this)) { > + NS_WARNING("Out of memory?"); > + return NS_ERROR_OUT_OF_MEMORY; Since TArrays are infallible this method should return void. @@ +178,5 @@ > + > +void > +FileInfo::Cleanup() > +{ > + if (mCachedBlob && NS_FAILED(mCachedBlob->RemoveDestructionObserver(this))) { Hm, elsewhere you never mess with mCachedBlob outside the file manager lock. This seems racy. @@ +189,5 @@ > + mFileManager->Invalidated()) { > + return; > + } > + > + nsRefPtr<IndexedDatabaseManager> mgr = IndexedDatabaseManager::GetOrCreate(); Is it possible to get here without the IDBManager existing? I suspect not, but not totally sure. If I'm right then please just use Get() here. Otherwise it is possible for GetOrCreate to fail, so you shouldn't just assert it. ::: dom/indexedDB/FileInfo.h @@ +39,5 @@ > +#ifndef mozilla_dom_indexeddb_fileinfo_h__ > +#define mozilla_dom_indexeddb_fileinfo_h__ > + > +#include "nsIDOMFile.h" > +#include "IndexedDatabase.h" Nit: Please make sure that IndexedDatabase.h appears first. @@ +46,5 @@ > +BEGIN_INDEXEDDB_NAMESPACE > + > +class FileInfo > +{ > +friend class FileManager; Nit: indent 2 spaces here. @@ +51,5 @@ > +public: > + FileInfo(FileManager* aFileManager) > + : mFileManager(aFileManager), > + mId(-1), > + mCachedBlob(nsnull) { } Nit: shift these left 2 spaces. @@ +58,5 @@ > + void AddRef(); > + void Release(); > + void UpdateDBRefs(PRInt32 aDelta); > + void ClearDBRefs(); > + void UpdateSliceRefs(PRInt32 aDelta); Once we have a common refcount function then each of the above could be rewritten: void AddRef() { UpdateReferences(mRefCnt, 1); } ... void ClearDBRefs() { UpdateReferences(mDBRefCnt, 0, true); } @@ +60,5 @@ > + void UpdateDBRefs(PRInt32 aDelta); > + void ClearDBRefs(); > + void UpdateSliceRefs(PRInt32 aDelta); > + > + FileManager* Manager() Nit: Make this a const method. @@ +65,5 @@ > + { > + return mFileManager; > + } > + > + PRInt64 Id() Same here. @@ +72,5 @@ > + } > + > + nsresult CacheBlob(nsIDOMBlob* aBlob); > + > + void NoticeBlobDestruction(); Like I said above, I hope we can remove these. ::: dom/indexedDB/FileManager.cpp @@ +61,5 @@ > + static_cast<nsTArray<FileInfo*>*>(aUserArg); > + > + if (!array->AppendElement(aValue)) { > + NS_WARNING("Out of memory!"); > + return PL_DHASH_STOP; This can't happen, remove. @@ +73,5 @@ > +NS_IMPL_THREADSAFE_ADDREF(FileManager) > +NS_IMPL_THREADSAFE_RELEASE(FileManager) > + > +nsresult > +FileManager::SetDirectory(nsIFile* aDirectory) Which thread is this supposed to be called on? It's doing IO so you should at least assert that it's not on the main thread. @@ +90,5 @@ > + rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755); > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + mDirectory = aDirectory; I don't think we should store this. nsIFiles aren't threadsafe, really, and we're going to call Clone on this one from several different threads later on. I think you should just store the path and make new nsIFile objects from that when you're calling Clone now. @@ +99,5 @@ > +nsresult > +FileManager::Load(mozIStorageConnection* aConnection) > +{ > + NS_ENSURE_TRUE(mFileInfos.Init(), NS_ERROR_OUT_OF_MEMORY); > + NS_ENSURE_TRUE(mCachedBlobIndex.Init(), NS_ERROR_OUT_OF_MEMORY); Yeah, these are fallible, so sadly you do have to check here. @@ +108,5 @@ > + "FROM file" > + ), getter_AddRefs(stmt)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsTArray<PRInt64> noRefArray; nsAutoTArray, maybe 10 elements? We're not expecting anything here unless we crashed or rolled back a transaction, right? @@ +126,5 @@ > + continue; > + } > + > + nsRefPtr<FileInfo> fileInfo = new FileInfo(this); > + NS_ENSURE_TRUE(fileInfo, NS_ERROR_OUT_OF_MEMORY); new is infallible, no need to check here. @@ +131,5 @@ > + > + fileInfo->mId = id; > + fileInfo->mDBRefCnt = refcount; > + > + if (!mFileInfos.Put(fileInfo->mId, fileInfo)) { Just use id here. @@ +136,5 @@ > + NS_WARNING("Out of memory?"); > + return NS_ERROR_OUT_OF_MEMORY; > + } > + > + mLastFileId = PR_MAX(fileInfo->mId, mLastFileId); id again, and use NS_MAX, not PR_MAX. @@ +139,5 @@ > + > + mLastFileId = PR_MAX(fileInfo->mId, mLastFileId); > + } > + > + if (noRefArray.Length() > 0) { Nit: Prefer !noRefArray.IsEmpty() @@ +143,5 @@ > + if (noRefArray.Length() > 0) { > + nsCAutoString ids; > + > + for (PRUint32 index = 0; index < noRefArray.Length(); index++) { > + PRInt64 id = noRefArray.ElementAt(index); Make this const PRInt64& @@ +149,5 @@ > + nsCOMPtr<nsIFile> file = GetFileForId(id); > + NS_ENSURE_TRUE(file, NS_ERROR_FAILURE); > + > + rv = file->Remove(false); > + NS_ENSURE_SUCCESS(rv, rv); I don't think you want to return if Remove fails. What happens if the file is gone, for instance? Warn only, I think. @@ +159,5 @@ > + } > + > + nsCAutoString query; > + query = NS_LITERAL_CSTRING("DELETE FROM file WHERE id IN (") + ids + > + NS_LITERAL_CSTRING(")"); Hm, I'm a little worried about this. If you remove the files first and then execute the query what happens if it fails for some reason? I think it might be safer to execute the query first and then remove the files only after that succeeded. @@ +211,5 @@ > + > +nsresult > +FileManager::Invalidate() > +{ > + if (PR_ATOMIC_SET(&mInvalidated, 1)) { This is unnecessary, you're going to grab the mutex in a sec anyway, just use a bool and set it to false inside the lock. @@ +215,5 @@ > + if (PR_ATOMIC_SET(&mInvalidated, 1)) { > + NS_ERROR("Invalidate more than once?!"); > + } > + > + nsTArray<FileInfo*> fileInfos; This should be a nsAutoTArray, give it maybe 20 elements? @@ +218,5 @@ > + > + nsTArray<FileInfo*> fileInfos; > + { > + MutexAutoLock lock(mMutex); > + mFileInfos.EnumerateRead(EnumerateToTArray, &fileInfos); First call fileInfos.SetCapacity(mFileInfos.Count()) to avoid lots of allocations. @@ +283,5 @@ > + nsAutoString id; > + id.AppendInt(aId); > + > + nsCOMPtr<nsIFile> file; > + nsresult rv = mDirectory->Clone(getter_AddRefs(file)); See above, I don't think it's safe to call clone() here. ::: dom/indexedDB/FileManager.h @@ +44,5 @@ > +#include "nsDataHashtable.h" > +#include "mozilla/Mutex.h" > +#include "nsIFile.h" > +#include "nsIDOMFile.h" > +#include "IndexedDatabase.h" Nit: Here's the ordering scheme I've tried to maintain in indexeddb code, please reorder these here and elsewhere. 1. IndexedDatabase.h 2. Interfaces 3. Other moz headers 4. Other indexeddb class headers 5. Forward declarations of other classes. @@ +57,5 @@ > +public: > + FileManager(const nsACString& aOrigin, > + const nsAString& aDatabaseName) > + : mOrigin(aOrigin), > + mDatabaseName(aDatabaseName), Nit: Move these left two spaces. @@ +61,5 @@ > + mDatabaseName(aDatabaseName), > + mLoaded(false), > + mInvalidated(0), > + mLastFileId(0), > + mMutex("FileManager::mMutex") { } Nit: {} on its own line. @@ +63,5 @@ > + mInvalidated(0), > + mLastFileId(0), > + mMutex("FileManager::mMutex") { } > + > + ~FileManager() { } Nit: {} on its own line. @@ +66,5 @@ > + > + ~FileManager() { } > + > + NS_IMETHOD_(nsrefcnt) AddRef(); > + NS_IMETHOD_(nsrefcnt) Release(); These can use khuey's INLINE_THREADSAFE version @@ +68,5 @@ > + > + NS_IMETHOD_(nsrefcnt) AddRef(); > + NS_IMETHOD_(nsrefcnt) Release(); > + > + nsACString& Origin() Nit: const @@ +73,5 @@ > + { > + return mOrigin; > + } > + > + nsAString& DatabaseName() Nit: const @@ +78,5 @@ > + { > + return mDatabaseName; > + } > + > + bool Loaded() Nit: const @@ +83,5 @@ > + { > + return mLoaded; > + } > + > + bool Invalidated() Nit: const @@ +88,5 @@ > + { > + return !!mInvalidated; > + } > + > + nsresult SetDirectory(nsIFile* aDirectory); All these methods need some comment saying which thread they can be called from (or some blanket statement like "all can be called from any thread" or something). Event better is to assert the correct thread in the beginning of each method if possible. @@ +108,5 @@ > + > + nsCString mOrigin; > + nsString mDatabaseName; > + > + bool mLoaded; Nit: In general we try to stick bool and other < 4byte things last so that the compiler packs the members efficiently (at least we hope so!). @@ +109,5 @@ > + nsCString mOrigin; > + nsString mDatabaseName; > + > + bool mLoaded; > + PRInt32 mInvalidated; Make this bool too once you've used the lock instead of atomic ops. @@ +121,5 @@ > + > + // File infos indexed by blob > + nsDataHashtable<nsVoidPtrHashKey, FileInfo*> mCachedBlobIndex; > + > + mozilla::Mutex mMutex; This needs a comment explaining what it protects. ::: dom/indexedDB/IDBCursor.h @@ +40,5 @@ > #ifndef mozilla_dom_indexeddb_idbcursor_h__ > #define mozilla_dom_indexeddb_idbcursor_h__ > > +#include "FileManager.h" > +#include "mozilla/dom/indexedDB/DatabaseInfo.h" These are public headers so they need their export path here. ::: dom/indexedDB/IDBDatabase.h @@ +144,5 @@ > > void EnterSetVersionTransaction(); > void ExitSetVersionTransaction(); > > + FileManager* Manager() Nit: const ::: dom/indexedDB/IDBIndex.cpp @@ +834,5 @@ > + nsnull > + }; > + > + bool result = IDBObjectStore::DeserializeValue(aCx, mCloneBuffer, aVal, > + &callbacks, &mCloneReadInfo); These callbacks should be automatic inside IDBObjectStore::DeserializeValue, so you don't have to remember to provide the callbacks each time. ::: dom/indexedDB/IDBObjectStore.cpp @@ +42,5 @@ > #include "nsIJSContextStack.h" > > #include "jsclone.h" > #include "mozilla/storage.h" > +#include "mozIStorageVirtualFile.h" Nit: Please move this up with the other interface include. @@ +51,5 @@ > #include "nsServiceManagerUtils.h" > #include "nsThreadUtils.h" > +#include "mozilla/dom/StructuredCloneTags.h" > +#include "nsDOMFile.h" > +#include "nsNetUtil.h" Nit, please alphabetize. @@ +60,5 @@ > #include "IDBIndex.h" > #include "IDBKeyRange.h" > #include "IDBTransaction.h" > #include "DatabaseInfo.h" > +#include "FileManager.h" Nit: Here too :) @@ +730,5 @@ > + if (isNull) { > + return NS_OK; > + } > + > + nsAutoString ids; nsString @@ +734,5 @@ > + nsAutoString ids; > + rv = aStatement->GetString(aIndex, ids); > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + nsTArray<PRInt64> array; nsAutoTArray @@ +739,5 @@ > + rv = ConvertFileIdsToArray(ids, array); > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + for (PRUint32 i = 0; i < array.Length(); i++) { > + PRInt64 id = array.ElementAt(i); const PRInt64& @@ +870,5 @@ > + } > + nchars = SwapBytes(nchars); > + > + char* buffer = > + static_cast<char*>(nsMemory::Alloc(nchars + 1)); Instead of doing this let's just do: nsCString name; char* buffer = name.BeginWriting(nchars + 1); Then you can avoid the adopting bit (though I think you have to manually call SetLength after you're done). @@ +897,5 @@ > + > + jsval wrappedBlob; > + nsresult rv = > + nsContentUtils::WrapNative(aCx, JS_GetGlobalForScopeChain(aCx), blob, > + &NS_GET_IID(nsIDOMBlob), &wrappedBlob); This is always going to give you a Blob wrapper, you'll want a File wrapper for nsIDOMFiles. @@ +945,5 @@ > + PRUint32 index = cloneWriteInfo->blobs.Length(); > + > + nsCOMPtr<nsIDOMFile> file = do_QueryInterface(supports); > + if (file) { > + nsCAutoString name; nsCString. @@ +974,5 @@ > + return JS_FALSE; > +} > + > +nsresult > +IDBObjectStore::ConvertFileIdsToArray(const nsAString& aFileIds, Hm, please use nsCharSeparatedTokenizer instead of rolling your own. @@ +1794,5 @@ > + > + nsAutoString fileIds; > + > + for (PRUint32 index = 0; index < mCloneWriteInfo.blobs.Length(); index++) { > + nsCOMPtr<nsIDOMBlob> domBlob = mCloneWriteInfo.blobs[index]; You want nsCOMPtr<nsIDOMBlob>& here so that you don't call addref/release in the loop. @@ +1796,5 @@ > + > + for (PRUint32 index = 0; index < mCloneWriteInfo.blobs.Length(); index++) { > + nsCOMPtr<nsIDOMBlob> domBlob = mCloneWriteInfo.blobs[index]; > + > + nsRefPtr<FileManager> fileManager = mDatabase->Manager(); This should be lifted outside the loop. @@ +1802,5 @@ > + nsRefPtr<FileInfo> fileInfo; > + PRInt64 id = -1; > + > + // Check first if it is a blob created from this db > + rv = domBlob->GetFileInfo(getter_AddRefs(fileInfo)); See comments about GetFileInfo above, I think we should scrap this hash lookup and hold the file infos for every database in the domBlob. @@ +1875,5 @@ > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + // Insert it into the database > + stmt = mTransaction->GetCachedStatement(NS_LITERAL_CSTRING( > + "INSERT INTO file(id, refcount) " This shouldn't be necessary. If you add something to object_data or ai_object_data with non-empty file_ids column then your insert/update trigger will fire and you should get an entry in the updatefunction hash for this file id and then you'll write the entry with the correct refcount at commit time. ::: dom/indexedDB/IDBTransaction.cpp @@ +293,5 @@ > + NS_ENSURE_TRUE(function, NS_ERROR_OUT_OF_MEMORY); > + > + rv = connection->CreateFunction( > + NS_LITERAL_CSTRING("update_refcount"), 2, function); > + NS_ENSURE_SUCCESS(rv, false); Returning false here is wrong. rv. @@ +308,5 @@ > > rv = stmt->Execute(); > NS_ENSURE_SUCCESS(rv, false); > > + function.swap(mFunction); We need a better name here. mUpdateFileRefcountFunction? @@ +951,5 @@ > return NS_OK; > } > + > +UpdateRefcountFunction::UpdateRefcountFunction(FileManager* aFileManager) > + : mFileManager(aFileManager) Nit: Move left two spaces @@ +953,5 @@ > + > +UpdateRefcountFunction::UpdateRefcountFunction(FileManager* aFileManager) > + : mFileManager(aFileManager) > +{ > + mFileInfoEntries.Init(); This can fail in low mem. Please add a separate Init method and call this there so that you can handle this failure. @@ +956,5 @@ > +{ > + mFileInfoEntries.Init(); > +} > + > +NS_IMPL_THREADSAFE_ISUPPORTS1(UpdateRefcountFunction, mozIStorageFunction) This doesn't need to be threadsafe, does it? Everything should happen on the transaction thread. @@ +974,5 @@ > + PRInt32 type1; > + aValues->GetTypeOfIndex(0, &type1); > + > + PRInt32 type2; > + aValues->GetTypeOfIndex(1, &type2); Since you're not checking the return value here I'd initialize type1 and type2 to mozIStorageValueArray::VALUE_TYPE_NULL. @@ +978,5 @@ > + aValues->GetTypeOfIndex(1, &type2); > + > + if (type1 == mozIStorageValueArray::VALUE_TYPE_NULL && > + type2 == mozIStorageValueArray::VALUE_TYPE_NULL) { > + NS_WARNING("Shouldn't be called!"); You should assert this. @@ +982,5 @@ > + NS_WARNING("Shouldn't be called!"); > + } > +#endif > + > + rv = ProcessValue(aValues, 0, -1); How about you use true/false here instead of 1/-1? Or maybe make an enum eDecrement & eIncrement? @@ +992,5 @@ > + return NS_OK; > +} > + > +nsresult > +UpdateRefcountFunction::UpdateDatabase(mozIStorageConnection* aConnection) This can be inlined in the header. @@ +994,5 @@ > + > +nsresult > +UpdateRefcountFunction::UpdateDatabase(mozIStorageConnection* aConnection) > +{ > + mFileInfoEntries.EnumerateRead(DatabaseUpdateFunction, aConnection); Hm, you're not propagating errors here. You need to make a smarter closure arg here so that you know when something goes wrong. @@ +1000,5 @@ > + return NS_OK; > +} > + > +void > +UpdateRefcountFunction::UpdateFileInfos() This can be inlined too. @@ +1016,5 @@ > + if (type == mozIStorageValueArray::VALUE_TYPE_NULL) { > + return NS_OK; > + } > + > + nsAutoString ids; nsString. @@ +1050,5 @@ > +UpdateRefcountFunction::DatabaseUpdateFunction(const PRUint64& aKey, > + FileInfoEntry* aValue, > + void* aUserArg) > +{ > + // We don't ignore zero deltas intentionally, we need to execute such updates If you don't do the thing above where you intentionally write 0 rows then you can ignore deltas of 0 (which seems much better to me). @@ +1063,5 @@ > + id.AppendInt(aKey); > + > + nsCAutoString query; > + query = NS_LITERAL_CSTRING("UPDATE file SET refcount = refcount + ") + delta + > + NS_LITERAL_CSTRING(" WHERE id = ") + id; Since this query is potentially going to run lots of times you should use a cached query and bind the arguments (delta and id) to it each time. That will speed this up significantly. You'll need to pass the IDBTransaction as part of the closure, and make sure you use a statement scoper helper. ::: dom/indexedDB/IDBTransaction.h @@ +292,5 @@ > + FileInfoUpdateFunction(const PRUint64& aKey, > + FileInfoEntry* aValue, > + void* aUserArg); > + > + nsRefPtr<FileManager> mFileManager; Does this need to be a strong ref? Is doesn't seem possible for the transaction or this updateRefCountFunction to outlive its FileManager. ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +635,5 @@ > correctThread, > "Running on the wrong thread!"); > } > #endif > + *aDirectory = nsnull; This shouldn't be needed, we will never set an invalid directory without returning an error code. @@ +658,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + nsTArray<nsRefPtr<FileManager> >* array; > + if (mFileManagers.Get(aOrigin, &array)) { If you just want to check that an entry exists you can pass null for the second arg here and lose the 'array' variable. @@ +710,5 @@ > + rv = file->IsDirectory(&isDirectory); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (isDirectory) { > + continue; This isn't right, we need to add all the files in the subfolder to the quota management system. Otherwise a db could add unlimited files. @@ +713,5 @@ > + if (isDirectory) { > + continue; > + } > + > + nsAutoString leafName; Please just use nsString here (and below). nsAutoString is really only useful if you plan on doing multiple modifications that could result in several allocations. @@ +717,5 @@ > + nsAutoString leafName; > + rv = file->GetLeafName(leafName); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsAutoString fileDirectoryName; Nit: This is the database base filename... Let's rename to dbBaseFilename @@ +720,5 @@ > + > + nsAutoString fileDirectoryName; > + if (!GetBaseFilename(leafName, fileDirectoryName)) { > + NS_WARNING("Unknown file found!"); > + continue; I think we should include any file under this directory in quota management, so if we find some random file I think we should count it. @@ +732,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCOMPtr<mozIStorageConnection> connection; > + rv = ss->OpenDatabaseWithVFS(file, NS_LITERAL_CSTRING("quota"), > + getter_AddRefs(connection)); Let's please not do this. We have one place right now where connections are created and that's really important. Here, for instance, you're not checking schema version, so if we ever change something in the database table this could fail. You're also having to duplicate the VFS code, and I'd like to avoid that too. I think we need to rearchitect this opening procedure. Rather than having this live in the IndexedDatabaseManager it should instead be part of OpenDatabaseHelper. Maybe IndexedDatabaseManager just needs to hold a boolean flag indicating whether or not an origin has been initialized. @@ +759,5 @@ > + NS_ERROR("Database has no name!"); > + return NS_ERROR_UNEXPECTED; > + } > + > + nsAutoString databaseName; nsString @@ +763,5 @@ > + nsAutoString databaseName; > + rv = stmt->GetString(0, databaseName); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsRefPtr<FileManager> fileManager = new FileManager(aOrigin, databaseName); Hm, I'd rather we did this lazily. It's hard to imagine that a database needs to interact with all its files, especially at the same time. @@ +818,5 @@ > + if (!fileManager) { > + fileManager = new FileManager(aOrigin, aDatabaseName); > + > + if (!array->AppendElement(fileManager)) { > + NS_WARNING("Out of memory?"); This is infallible. @@ +827,5 @@ > + return fileManager.forget(); > +} > + > +void > +IndexedDatabaseManager::InvalidateFileManagersFor(const nsACString& aOrigin) Nit: Either "InvalidateAllFileManagers" or "InvalidateFileManagersForOrigin" @@ +840,5 @@ > + } > +} > + > +void > +IndexedDatabaseManager::InvalidateFileManagerFor(const nsACString& aOrigin, Nit: Just "InvalidateFileManager" @@ +857,5 @@ > + break; > + } > + } > + > + if (array->IsEmpty()) { Nit: This check can go inside the loop where you actually remove an element. @@ +1217,5 @@ > // Call the callback unless we were canceled. > if (!mCanceled) { > + PRUint64 usage = mUsage; > + // Watch for overflow! > + if (NS_UNLIKELY((LL_MAXINT - usage) <= mFileUsage)) { Can we make this an inline function? You do something similar below in GetUsageForDirectory. These NS_(UN)LIKELY things are semi-useless and make the code harder to read. Let's avoid them. @@ +1265,5 @@ > + > +nsresult > +IndexedDatabaseManager::AsyncUsageRunnable::GetUsageForDirectory( > + nsIFile* aDirectory, > + PRUint64* aUsage) Hm. I think this should be a static function, and I don't think its worth separating out db sizes and file sizes. They all count towards the same quota and we don't provide the web pages any API to determine this. @@ +1295,3 @@ > > + if (isDirectory) { > + rv = GetUsageForDirectory(file, &mFileUsage); Hm, this means that any subdirectory of the files directory will be counted also. I think we should a) track our recursion depth here to make sure that we never go beyond one subdirectory, and b) issue a warning here since that should never happen. @@ +1443,5 @@ > + NS_ENSURE_TRUE(ss, NS_ERROR_FAILURE); > + > + nsCOMPtr<mozIStorageVirtualFile> virtualFile; > + nsresult rv = ss->GetVirtualFile(NS_LITERAL_CSTRING("quota"), > + getter_AddRefs(virtualFile)); I'm not sure why this is necessary... Can't you just delete the file and then call UpdateQuota? @@ +1449,5 @@ > + NS_WARNING("Failed to get virtual file!"); > + return NS_ERROR_FAILURE; > + } > + > + rv = virtualFile->Init(mFile); We shouldn't mess with the file that we got from the main thread. Instead I think you should just store the path in the runnable and then create a new file object here. ::: dom/indexedDB/IndexedDatabaseManager.h @@ +291,5 @@ > + NS_ASSERTION(mFile, "Null file!"); > + } > + > + private: > + nsCOMPtr<nsIFile> mFile; This may be released on the IO thread which is bad news. ::: dom/indexedDB/OpenDatabaseHelper.cpp @@ +85,5 @@ > + return NS_OK; > +} > + > +nsresult > +CreateTables6(mozIStorageConnection* aDBConn) We should have a better name here... CreateFileTables? @@ +90,5 @@ > +{ > + // Table `file` > + nsresult rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "CREATE TABLE file (" > + "id INTEGER, " Make this INTEGER PRIMARY KEY, then remove the PRIMARY KEY line below. @@ +465,5 @@ > return NS_OK; > } > > nsresult > +UpgradeSchemaFrom5To6(mozIStorageConnection* aConnection) All of this has changed a bit, sorry :( @@ +1435,5 @@ > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + nsCOMPtr<nsIFile> dbFile; > + rv = directory->Clone(getter_AddRefs(dbFile)); > + NS_ENSURE_SUCCESS(rv, rv); Can't return rv here, it's not going to be an INDEXEDDB_ error code. @@ +1473,5 @@ > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + if (exists) { > + // FIXME: use virtual files! > + rv = fileDirectory->Remove(true); I guess this will need to be fixed before we check this in. ::: storage/public/Makefile.in @@ +70,5 @@ > mozIStorageBaseStatement.idl \ > mozIStorageAsyncStatement.idl \ > mozIStorageServiceQuotaManagement.idl \ > mozIStorageVacuumParticipant.idl \ > + mozIStorageVirtualFile.idl \ Nit: Use spaces, not tabs. ::: storage/public/mozIStorageServiceQuotaManagement.idl @@ -127,5 @@ > * exists then its size information will be added or refreshed. If the > * file does not exist then the file will be removed from tracking > * under the quota system. > */ > - void updateQutoaInformationForFile(in nsIFile aFile); Eek! Glad you caught that. ::: storage/public/mozIStorageVirtualFile.idl @@ +39,5 @@ > + > +interface nsIFile; > + > +[scriptable, uuid(3ab22a71-c7cf-465d-afe6-108f9de619a5)] > +interface mozIStorageVirtualFile : nsISupports This interface needs lots of comments explaining what it's used for. @@ +42,5 @@ > +[scriptable, uuid(3ab22a71-c7cf-465d-afe6-108f9de619a5)] > +interface mozIStorageVirtualFile : nsISupports > +{ > + > + void init(in nsIFile aFile); Why have separate Init and Open methods? Please combine unless I'm missing something. @@ +48,5 @@ > + void open(); > + > + void delete(); > + > + void close(); Must delete always be called if open() succeeds? Please comment. @@ +54,5 @@ > + void write(in string aBuf, > + in long aCount, > + in long long aOffset); > + > + void sync(); Must this be called before close()? Please comment. @@ +56,5 @@ > + in long long aOffset); > + > + void sync(); > + > + long long size(); unsigned? And make this a readonly attribute, not a function. ::: storage/src/mozStorageService.cpp @@ +648,5 @@ > +NS_IMETHODIMP > +Service::GetVirtualFile(const nsACString &aVFSName, > + mozIStorageVirtualFile **aResult) > +{ > + *aResult = nsnull; Nit: Move this into the |!vfs| block so that we only set *aResult once. @@ +651,5 @@ > +{ > + *aResult = nsnull; > + > + sqlite3_vfs* vfs = nsnull; > + if (aVFSName.EqualsLiteral("default")) { Please use sqlite3_vfs_find() here. We don't want to hardcode anything like this. ::: storage/src/mozStorageVirtualFile.cpp @@ +51,5 @@ > + > +VirtualFile::~VirtualFile() > +{ > + if (mFd) { > + nsresult rv = Sync(); Please don't call virtual methods in destructors. I think it's sufficient to assert that mFd is null here and force users to call close themselves. @@ +87,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + nsCAutoString path; > + rv = mFile->GetNativePath(path); This isn't what you want... You want GetPath followed by a ConvertUTF16toUTF8 since SQLite deals with UTF8 paths. @@ +97,5 @@ > + } > + > + int outFlags = 0; > + int rc = mVFS->xOpen(mVFS, PromiseFlatCString(path).get(), fd, > + SQLITE_OPEN_MAIN_DB|SQLITE_OPEN_CREATE|SQLITE_OPEN_READWRITE, &outFlags); From the docs: "SQLite guarantees that the zFilename parameter to xOpen is either a NULL pointer or string obtained from xFullPathname()" I think you need to do the same... And this seems fragile. I'll check with the SQLite devs to see if there's a better way to do this sort of thing without having to duplicate a lot of SQLite's internal logic. ::: storage/src/mozStorageVirtualFile.h @@ +58,5 @@ > +private: > + sqlite3_vfs* mVFS; > + sqlite3_file* mFd; > + > + nsCOMPtr<nsIFile> mFile; I don't think you really want to hold the nsIFile here... Can't you just hold the path as a string? That seems to be all you ever use, and you wouldn't have to convert it every time.

Also, if possible, please try to track changes as an interdiff so we can narrow down our changes!

Ok, I'll fix all those nits first to drastically reduce the list :) We need to discuss about some comments, especially the cached blob stuff, nsDOMFileFile::GetFileInfo(), proper mutex initialization in nsDOMFile, origin directory initialization and maybe others.

attaching it here, so bent can take a look at sqlite virtual table stuff

I'll attach a new patch today with the issues addressed + some other fixes and improvements.

(In reply to ben turner [:bent] from comment #25) > Comment on attachment 574628 [details] [diff] [review] > patch v0.3 > > Review of attachment 574628 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks really good! Lots of comments below, but don't be discouraged, > it's a very big patch! > > A bunch of the comments below are little nit-y style things, no big deal. > There are a few bigger issues hidden among them, however. > > ::: content/base/public/nsDOMFile.h > @@ +69,5 @@ > > nsresult NS_NewBlobBuilder(nsISupports* *aSupports); > > > > +using namespace mozilla::dom; > > + > > +class nsDOMFile > > This doesn't appear to really be useful. Just add your observer logic to > nsDOMFileBase? > I removed it completely, nsDOMFile::mFileInfos is now now protected by IndexedDatabaseManager::DOMFileMutex() > @@ +170,1 @@ > > : nsDOMFileBase(EmptyString(), EmptyString(), PR_UINT64_MAX), > > This seems wrong... Why not pass the name arg through? The name used to be initialized later from the file info object. However, the name is now serialized in structured clone, so it can be passed directly to the constructor. I reworked it this way and I also added type and size to structured clone. > > @@ +274,5 @@ > > friend class DataOwnerAdapter; // Needs to see DataOwner > > class DataOwner { > > public: > > + NS_IMETHOD_(nsrefcnt) AddRef(); > > + NS_IMETHOD_(nsrefcnt) Release(); > > Pretty sure we have a threadsafe inline version now (it's brand new), please > just change the macro. yeah, this is already fixed on trunk > > @@ +282,5 @@ > > } > > ~DataOwner() { > > PR_Free(mData); > > } > > + nsAutoRefCnt mRefCnt; > > Should be unnecessary. yeah, the same as above > > ::: content/base/public/nsIDOMFile.idl > @@ +62,1 @@ > > interface nsIDOMBlob : nsISupports > > All the methods/attributes you add here should have some comments > associated... What they do, when they should or shouldn't be called, etc. fixed > > @@ +76,5 @@ > > + [noscript] void addDestructionObserver(in FileInfo aObserver); > > + [noscript] void removeDestructionObserver(in FileInfo aObserver); > > + > > + [noscript] void init(in FileInfo aFileInfo); > > + [noscript] readonly attribute FileInfo fileInfo; > > Since this is noscript anyway just make it [notxpcom] also and then it will > just return a FileInfo* instead of nsresult. fixed > > @@ +77,5 @@ > > + [noscript] void removeDestructionObserver(in FileInfo aObserver); > > + > > + [noscript] void init(in FileInfo aFileInfo); > > + [noscript] readonly attribute FileInfo fileInfo; > > + readonly attribute DOMString mozFileId; > > Do we want this to be scriptable? I don't think so... fixed, testing is now done using DOMWindowUtils > > @@ +86,4 @@ > > interface nsIDOMFile : nsIDOMBlob > > { > > readonly attribute DOMString name; > > + [noscript] attribute AUTF8String utf8Name; > > Not sure this is worth having. Why not just convert 'name' when you need > this? > removed > ::: content/base/src/nsDOMFile.cpp > @@ +136,5 @@ > > } > > > > +nsDOMFile::nsDOMFile() > > +{ > > + if (NS_AtomicIncrementRefcnt(gDOMFileInstanceCount) == 1) { > > This isn't actually safe... The increments/decrements are going to be atomic > but the read/write of gDOMFileMutex is free to race with any other thread. > Creating a mutex, for instance, involves a malloc and many other > instructions. The scheduler could switch to another thread before setting > gDOMFileMutex and then that thread might assume it would have a mutex based > on a non-zero refcount. We need to figure out another way to do this... reworked, IndexedDatabaseManager::DOMFileMutex() now protects mFileInfos > > @@ +188,5 @@ > > return NS_OK; > > } > > > > NS_IMETHODIMP > > +nsDOMFileBase::GetUtf8Name(nsACString& aName) > > Let's remove these. removed > > @@ +346,5 @@ > > +nsDOMFileBase::AddDestructionObserver(indexedDB::FileInfo* aObserver) > > +{ > > + MutexAutoLock lock(*gDOMFileMutex); > > + > > + if (!mDestructionObservers.AppendElement(aObserver)) { > > How about asserting !Contains(aObserver) so we don't end up with duplicates? added > > @@ +348,5 @@ > > + MutexAutoLock lock(*gDOMFileMutex); > > + > > + if (!mDestructionObservers.AppendElement(aObserver)) { > > + NS_WARNING("Out of memory?"); > > + return NS_ERROR_OUT_OF_MEMORY; > > Thankfully nsTArray has been made infallible. This code will never run, so > just remove it. done the same for mFileInfos > > @@ +369,5 @@ > > + > > +NS_IMETHODIMP > > +nsDOMFileBase::Init(indexedDB::FileInfo* aFileInfo) > > +{ > > + return NS_OK; > > You intend for this to be overwritten, yes? Add an NS_NOTREACHED() > explaining that and return NS_ERROR_UNEXPECTED. Same for the other two below. this method has been removed > > @@ +467,1 @@ > > nsCOMPtr<nsISupports> file = do_QueryObject(new nsDOMFileFile()); > > Hm, don't you want a blob here? this method has been removed > > @@ +608,5 @@ > > return NS_OK; > > } > > > > +NS_IMETHODIMP > > +nsDOMFileFile::Init(indexedDB::FileInfo* aFileInfo) > > Which thread should this be called on? Any? I hope it's main only since > GetMozFileId seems to restrict usage to the main thread... this method has been removed > > @@ +616,5 @@ > > + } > > + > > + if (!aFileInfo) { > > + return NS_ERROR_INVALID_ARG; > > + } > > NS_ENSURE_ARG_POINTER is what you want here. this method has been removed > > @@ +626,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +nsDOMFileFile::GetFileInfo(indexedDB::FileInfo** aFileInfo) > > Actually, I think this is doing the wrong thing. What we really want is for > this object to hold a list of FileInfos for per database. Something like > this: > > struct LiveFileInfos { > nsIAtom* mDatabaseId; > nsRefPtr<FileInfo> mFileInfo; > }; > > class nsDOMFileFile { > ... > nsTArray<LiveFileInfos> mLiveFileInfos; > }; > > Then GetFileInfo should take the databaseId as a parameter. > this has been reworked according to the agreement to delete os files less aggressively > @@ +636,5 @@ > > +NS_IMETHODIMP > > +nsDOMFileFile::GetMozFileId(nsAString& aFileId) > > +{ > > + if (mFileInfo) { > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > Is this only true if we have a mFileInfo? Could another thread be in the > process of modifying mFileInfo when this is called (do we need a lock)? What > guarantees that we can't call into IsCallerTrustedForCapability() off the > main thread in an optimized build? this has been removed, the support for testing is now in DOMWindowUtils > > @@ +670,5 @@ > > return DataOwnerAdapter::Create(mDataOwner, mStart, mLength, aStream); > > } > > > > +NS_IMPL_THREADSAFE_ADDREF(nsDOMMemoryFile::DataOwner) > > +NS_IMPL_THREADSAFE_RELEASE(nsDOMMemoryFile::DataOwner) > > khuey should be fixing this in a different bug, let's not mess with this in > this patch. > yeah, this is fixed on trunk > ::: dom/indexedDB/DatabaseInfo.h > @@ +152,5 @@ > > IndexInfo info; > > Key value; > > }; > > > > +struct StructuredCloneReadInfo { > > This stuff doesn't belong in DatabaseInfo.h... Maybe IndexedDatabase.h? fixed (moved to IndexedDatabase.h) > > @@ +170,5 @@ > > + PRUint64 offsetToKeyProp; > > + nsTArray<nsCOMPtr<nsIDOMBlob> > blobs; > > +}; > > + > > +struct StructuredCloneEntry { > > As far as I can tell StructuredCloneReadInfo is always used in conjunction > with a JSAutoStructuredCloneBuffer. Let's just add a > JSAutoStructuredCloneBuffer member to StructuredCloneReadInfo and remove > StructuredCloneEntry altogether. > fixed (also added the buffer to StructuredCloneWriteInfo) > ::: dom/indexedDB/FileInfo.cpp > @@ +43,5 @@ > > +USING_INDEXEDDB_NAMESPACE > > + > > +FileInfo::~FileInfo() > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "File info destroyed on wrong thread!"); > > If this is all the destructor is going to do then make it #ifdef DEBUG in > the header. That way the compiler can inline it in an opt build. fixed > > @@ +47,5 @@ > > + NS_ASSERTION(NS_IsMainThread(), "File info destroyed on wrong thread!"); > > +} > > + > > +void > > +FileInfo::AddRef() > > Hm, lots of duplicated code in these... How about doing this: > > void UpdateReferences(nsAutoRefCnt& aRefCount, > PRInt32 aDelta, > bool aClear = false) > { > { > MutexAutoLock lock(mFileManager->mMutex); > > aRefCount = aClear ? 0 : aRefCount + aDelta; > > if (mRefCnt + mDBRefCnt + mSliceRefCnt > 0) { > return; > } > > LockedCleanup(); > } > > Cleanup(); > > delete this; > } > fixed > @@ +130,5 @@ > > + delete this; > > +} > > + > > +nsresult > > +FileInfo::CacheBlob(nsIDOMBlob* aBlob) > > I don't understand the purpose of holding the cached blob here. You never > seem to use it, you hold a weak ref so it can't help keep the blob alive, > and there is no accessor method to get it out of a FileInfo. It just seems > to add complexity, requiring the synchronization of the cached blob index > hashtable and the destruction observers... Can't we just move all this code > directly into the file manager? > as I already mentioned, all this stuff has been reworked ... the code is now much simpler (since we can hold strong references to cached blobs) > @@ +141,5 @@ > > + mCachedBlob = aBlob; > > + > > + if (!mFileManager->mCachedBlobIndex.Put(mCachedBlob, this)) { > > + NS_WARNING("Out of memory?"); > > + return NS_ERROR_OUT_OF_MEMORY; > > Since TArrays are infallible this method should return void. > this has been removed, so no need to fix > @@ +178,5 @@ > > + > > +void > > +FileInfo::Cleanup() > > +{ > > + if (mCachedBlob && NS_FAILED(mCachedBlob->RemoveDestructionObserver(this))) { > > Hm, elsewhere you never mess with mCachedBlob outside the file manager lock. > This seems racy. > removed > @@ +189,5 @@ > > + mFileManager->Invalidated()) { > > + return; > > + } > > + > > + nsRefPtr<IndexedDatabaseManager> mgr = IndexedDatabaseManager::GetOrCreate(); > > Is it possible to get here without the IDBManager existing? I suspect not, > but not totally sure. If I'm right then please just use Get() here. > Otherwise it is possible for GetOrCreate to fail, so you shouldn't just > assert it. simple Get() works too, fixed > > ::: dom/indexedDB/FileInfo.h > @@ +39,5 @@ > > +#ifndef mozilla_dom_indexeddb_fileinfo_h__ > > +#define mozilla_dom_indexeddb_fileinfo_h__ > > + > > +#include "nsIDOMFile.h" > > +#include "IndexedDatabase.h" > > Nit: Please make sure that IndexedDatabase.h appears first. fixed > > @@ +46,5 @@ > > +BEGIN_INDEXEDDB_NAMESPACE > > + > > +class FileInfo > > +{ > > +friend class FileManager; > > Nit: indent 2 spaces here. fixed > > @@ +51,5 @@ > > +public: > > + FileInfo(FileManager* aFileManager) > > + : mFileManager(aFileManager), > > + mId(-1), > > + mCachedBlob(nsnull) { } > > Nit: shift these left 2 spaces. fixed > > @@ +58,5 @@ > > + void AddRef(); > > + void Release(); > > + void UpdateDBRefs(PRInt32 aDelta); > > + void ClearDBRefs(); > > + void UpdateSliceRefs(PRInt32 aDelta); > > Once we have a common refcount function then each of the above could be > rewritten: > > void AddRef() > { > UpdateReferences(mRefCnt, 1); > } > > ... > > void ClearDBRefs() > { > UpdateReferences(mDBRefCnt, 0, true); > } > fixed > @@ +60,5 @@ > > + void UpdateDBRefs(PRInt32 aDelta); > > + void ClearDBRefs(); > > + void UpdateSliceRefs(PRInt32 aDelta); > > + > > + FileManager* Manager() > > Nit: Make this a const method. fixed > > @@ +65,5 @@ > > + { > > + return mFileManager; > > + } > > + > > + PRInt64 Id() > > Same here. fixed > > @@ +72,5 @@ > > + } > > + > > + nsresult CacheBlob(nsIDOMBlob* aBlob); > > + > > + void NoticeBlobDestruction(); > removed > Like I said above, I hope we can remove these. > > ::: dom/indexedDB/FileManager.cpp > @@ +61,5 @@ > > + static_cast<nsTArray<FileInfo*>*>(aUserArg); > > + > > + if (!array->AppendElement(aValue)) { > > + NS_WARNING("Out of memory!"); > > + return PL_DHASH_STOP; > > This can't happen, remove. fixed > > @@ +73,5 @@ > > +NS_IMPL_THREADSAFE_ADDREF(FileManager) > > +NS_IMPL_THREADSAFE_RELEASE(FileManager) > > + > > +nsresult > > +FileManager::SetDirectory(nsIFile* aDirectory) > > Which thread is this supposed to be called on? It's doing IO so you should > at least assert that it's not on the main thread. fixed > > @@ +90,5 @@ > > + rv = aDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755); > > + NS_ENSURE_SUCCESS(rv, rv); > > + } > > + > > + mDirectory = aDirectory; > > I don't think we should store this. nsIFiles aren't threadsafe, really, and > we're going to call Clone on this one from several different threads later > on. I think you should just store the path and make new nsIFile objects from > that when you're calling Clone now. fixed (storing only path now) > > @@ +99,5 @@ > > +nsresult > > +FileManager::Load(mozIStorageConnection* aConnection) > > +{ > > + NS_ENSURE_TRUE(mFileInfos.Init(), NS_ERROR_OUT_OF_MEMORY); > > + NS_ENSURE_TRUE(mCachedBlobIndex.Init(), NS_ERROR_OUT_OF_MEMORY); > > Yeah, these are fallible, so sadly you do have to check here. > > @@ +108,5 @@ > > + "FROM file" > > + ), getter_AddRefs(stmt)); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + nsTArray<PRInt64> noRefArray; > > nsAutoTArray, maybe 10 elements? We're not expecting anything here unless we > crashed or rolled back a transaction, right? > actually, the noRefArray is now useless it was needed when rows with zero refcount weren't automatically removed in the same transaction anyway, I added an assertion there at least > @@ +126,5 @@ > > + continue; > > + } > > + > > + nsRefPtr<FileInfo> fileInfo = new FileInfo(this); > > + NS_ENSURE_TRUE(fileInfo, NS_ERROR_OUT_OF_MEMORY); > > new is infallible, no need to check here. fixed > > @@ +131,5 @@ > > + > > + fileInfo->mId = id; > > + fileInfo->mDBRefCnt = refcount; > > + > > + if (!mFileInfos.Put(fileInfo->mId, fileInfo)) { > > Just use id here. fixed > > @@ +136,5 @@ > > + NS_WARNING("Out of memory?"); > > + return NS_ERROR_OUT_OF_MEMORY; > > + } > > + > > + mLastFileId = PR_MAX(fileInfo->mId, mLastFileId); > > id again, and use NS_MAX, not PR_MAX. fixed > > @@ +139,5 @@ > > + > > + mLastFileId = PR_MAX(fileInfo->mId, mLastFileId); > > + } > > + > > + if (noRefArray.Length() > 0) { > > Nit: Prefer !noRefArray.IsEmpty() code removed, fixed elsewhere I think > > @@ +143,5 @@ > > + if (noRefArray.Length() > 0) { > > + nsCAutoString ids; > > + > > + for (PRUint32 index = 0; index < noRefArray.Length(); index++) { > > + PRInt64 id = noRefArray.ElementAt(index); > > Make this const PRInt64& fixed > > @@ +149,5 @@ > > + nsCOMPtr<nsIFile> file = GetFileForId(id); > > + NS_ENSURE_TRUE(file, NS_ERROR_FAILURE); > > + > > + rv = file->Remove(false); > > + NS_ENSURE_SUCCESS(rv, rv); > > I don't think you want to return if Remove fails. What happens if the file > is gone, for instance? Warn only, I think. this has changed in new patch, anyway I changed it to a warning > > @@ +159,5 @@ > > + } > > + > > + nsCAutoString query; > > + query = NS_LITERAL_CSTRING("DELETE FROM file WHERE id IN (") + ids + > > + NS_LITERAL_CSTRING(")"); > > Hm, I'm a little worried about this. If you remove the files first and then > execute the query what happens if it fails for some reason? I think it might > be safer to execute the query first and then remove the files only after > that succeeded. as I mentioned, this has been removed completely > > @@ +211,5 @@ > > + > > +nsresult > > +FileManager::Invalidate() > > +{ > > + if (PR_ATOMIC_SET(&mInvalidated, 1)) { > > This is unnecessary, you're going to grab the mutex in a sec anyway, just > use a bool and set it to false inside the lock. fixed > > @@ +215,5 @@ > > + if (PR_ATOMIC_SET(&mInvalidated, 1)) { > > + NS_ERROR("Invalidate more than once?!"); > > + } > > + > > + nsTArray<FileInfo*> fileInfos; > > This should be a nsAutoTArray, give it maybe 20 elements? fixed > > @@ +218,5 @@ > > + > > + nsTArray<FileInfo*> fileInfos; > > + { > > + MutexAutoLock lock(mMutex); > > + mFileInfos.EnumerateRead(EnumerateToTArray, &fileInfos); > > First call fileInfos.SetCapacity(mFileInfos.Count()) to avoid lots of > allocations. fixed, nice optimization > > @@ +283,5 @@ > > + nsAutoString id; > > + id.AppendInt(aId); > > + > > + nsCOMPtr<nsIFile> file; > > + nsresult rv = mDirectory->Clone(getter_AddRefs(file)); > > See above, I don't think it's safe to call clone() here. fixed to use paths > > ::: dom/indexedDB/FileManager.h > @@ +44,5 @@ > > +#include "nsDataHashtable.h" > > +#include "mozilla/Mutex.h" > > +#include "nsIFile.h" > > +#include "nsIDOMFile.h" > > +#include "IndexedDatabase.h" > > Nit: Here's the ordering scheme I've tried to maintain in indexeddb code, > please reorder these here and elsewhere. > > 1. IndexedDatabase.h > 2. Interfaces > 3. Other moz headers > 4. Other indexeddb class headers > 5. Forward declarations of other classes. > fixed everywhere I hope > @@ +57,5 @@ > > +public: > > + FileManager(const nsACString& aOrigin, > > + const nsAString& aDatabaseName) > > + : mOrigin(aOrigin), > > + mDatabaseName(aDatabaseName), > > Nit: Move these left two spaces. fixed > > @@ +61,5 @@ > > + mDatabaseName(aDatabaseName), > > + mLoaded(false), > > + mInvalidated(0), > > + mLastFileId(0), > > + mMutex("FileManager::mMutex") { } > > Nit: {} on its own line. fixed > > @@ +63,5 @@ > > + mInvalidated(0), > > + mLastFileId(0), > > + mMutex("FileManager::mMutex") { } > > + > > + ~FileManager() { } > > Nit: {} on its own line. fixed > > @@ +66,5 @@ > > + > > + ~FileManager() { } > > + > > + NS_IMETHOD_(nsrefcnt) AddRef(); > > + NS_IMETHOD_(nsrefcnt) Release(); > > These can use khuey's INLINE_THREADSAFE version fixed > > @@ +68,5 @@ > > + > > + NS_IMETHOD_(nsrefcnt) AddRef(); > > + NS_IMETHOD_(nsrefcnt) Release(); > > + > > + nsACString& Origin() > > Nit: const fixed > > @@ +73,5 @@ > > + { > > + return mOrigin; > > + } > > + > > + nsAString& DatabaseName() > > Nit: const fixed > > @@ +78,5 @@ > > + { > > + return mDatabaseName; > > + } > > + > > + bool Loaded() > > Nit: const fixed > > @@ +83,5 @@ > > + { > > + return mLoaded; > > + } > > + > > + bool Invalidated() > > Nit: const fixed > > @@ +88,5 @@ > > + { > > + return !!mInvalidated; > > + } > > + > > + nsresult SetDirectory(nsIFile* aDirectory); > > All these methods need some comment saying which thread they can be called > from (or some blanket statement like "all can be called from any thread" or > something). Event better is to assert the correct thread in the beginning of > each method if possible. fixed, added the assertions > > @@ +108,5 @@ > > + > > + nsCString mOrigin; > > + nsString mDatabaseName; > > + > > + bool mLoaded; > > Nit: In general we try to stick bool and other < 4byte things last so that > the compiler packs the members efficiently (at least we hope so!). fixed > > @@ +109,5 @@ > > + nsCString mOrigin; > > + nsString mDatabaseName; > > + > > + bool mLoaded; > > + PRInt32 mInvalidated; > > Make this bool too once you've used the lock instead of atomic ops. fixed > > @@ +121,5 @@ > > + > > + // File infos indexed by blob > > + nsDataHashtable<nsVoidPtrHashKey, FileInfo*> mCachedBlobIndex; > > + > > + mozilla::Mutex mMutex; > > This needs a comment explaining what it protects. fixed (added a comment) > > ::: dom/indexedDB/IDBCursor.h > @@ +40,5 @@ > > #ifndef mozilla_dom_indexeddb_idbcursor_h__ > > #define mozilla_dom_indexeddb_idbcursor_h__ > > > > +#include "FileManager.h" > > +#include "mozilla/dom/indexedDB/DatabaseInfo.h" > > These are public headers so they need their export path here. fixed (changed FileManager.h to mozilla/dome/indexedDB/FileManager.h) > > ::: dom/indexedDB/IDBDatabase.h > @@ +144,5 @@ > > > > void EnterSetVersionTransaction(); > > void ExitSetVersionTransaction(); > > > > + FileManager* Manager() > > Nit: const fixed > > ::: dom/indexedDB/IDBIndex.cpp > @@ +834,5 @@ > > + nsnull > > + }; > > + > > + bool result = IDBObjectStore::DeserializeValue(aCx, mCloneBuffer, aVal, > > + &callbacks, &mCloneReadInfo); > > These callbacks should be automatic inside IDBObjectStore::DeserializeValue, > so you don't have to remember to provide the callbacks each time. fixed (also SerializeValue()) > > ::: dom/indexedDB/IDBObjectStore.cpp > @@ +42,5 @@ > > #include "nsIJSContextStack.h" > > > > #include "jsclone.h" > > #include "mozilla/storage.h" > > +#include "mozIStorageVirtualFile.h" > > Nit: Please move this up with the other interface include. the include has been removed > > @@ +51,5 @@ > > #include "nsServiceManagerUtils.h" > > #include "nsThreadUtils.h" > > +#include "mozilla/dom/StructuredCloneTags.h" > > +#include "nsDOMFile.h" > > +#include "nsNetUtil.h" > > Nit, please alphabetize. fixed > > @@ +60,5 @@ > > #include "IDBIndex.h" > > #include "IDBKeyRange.h" > > #include "IDBTransaction.h" > > #include "DatabaseInfo.h" > > +#include "FileManager.h" > > Nit: Here too :) fixed > > @@ +730,5 @@ > > + if (isNull) { > > + return NS_OK; > > + } > > + > > + nsAutoString ids; > > nsString fixed > > @@ +734,5 @@ > > + nsAutoString ids; > > + rv = aStatement->GetString(aIndex, ids); > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + > > + nsTArray<PRInt64> array; > > nsAutoTArray fixed > > @@ +739,5 @@ > > + rv = ConvertFileIdsToArray(ids, array); > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + > > + for (PRUint32 i = 0; i < array.Length(); i++) { > > + PRInt64 id = array.ElementAt(i); > > const PRInt64& fixed > > @@ +870,5 @@ > > + } > > + nchars = SwapBytes(nchars); > > + > > + char* buffer = > > + static_cast<char*>(nsMemory::Alloc(nchars + 1)); > > Instead of doing this let's just do: > > nsCString name; > char* buffer = name.BeginWriting(nchars + 1); > > Then you can avoid the adopting bit (though I think you have to manually > call SetLength after you're done). fixed, there's now an inline function for it I also added the type and size to serialization/deserialization > > @@ +897,5 @@ > > + > > + jsval wrappedBlob; > > + nsresult rv = > > + nsContentUtils::WrapNative(aCx, JS_GetGlobalForScopeChain(aCx), blob, > > + &NS_GET_IID(nsIDOMBlob), &wrappedBlob); > > This is always going to give you a Blob wrapper, you'll want a File wrapper > for nsIDOMFiles. > fixed > @@ +945,5 @@ > > + PRUint32 index = cloneWriteInfo->blobs.Length(); > > + > > + nsCOMPtr<nsIDOMFile> file = do_QueryInterface(supports); > > + if (file) { > > + nsCAutoString name; > > nsCString. fixed > > @@ +974,5 @@ > > + return JS_FALSE; > > +} > > + > > +nsresult > > +IDBObjectStore::ConvertFileIdsToArray(const nsAString& aFileIds, > > Hm, please use nsCharSeparatedTokenizer instead of rolling your own. fixed > > @@ +1794,5 @@ > > + > > + nsAutoString fileIds; > > + > > + for (PRUint32 index = 0; index < mCloneWriteInfo.blobs.Length(); index++) { > > + nsCOMPtr<nsIDOMBlob> domBlob = mCloneWriteInfo.blobs[index]; > > You want nsCOMPtr<nsIDOMBlob>& here so that you don't call addref/release in > the loop. fixed > > @@ +1796,5 @@ > > + > > + for (PRUint32 index = 0; index < mCloneWriteInfo.blobs.Length(); index++) { > > + nsCOMPtr<nsIDOMBlob> domBlob = mCloneWriteInfo.blobs[index]; > > + > > + nsRefPtr<FileManager> fileManager = mDatabase->Manager(); > > This should be lifted outside the loop. fixed > > @@ +1802,5 @@ > > + nsRefPtr<FileInfo> fileInfo; > > + PRInt64 id = -1; > > + > > + // Check first if it is a blob created from this db > > + rv = domBlob->GetFileInfo(getter_AddRefs(fileInfo)); > > See comments about GetFileInfo above, I think we should scrap this hash > lookup and hold the file infos for every database in the domBlob. fixed > > @@ +1875,5 @@ > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + > > + // Insert it into the database > > + stmt = mTransaction->GetCachedStatement(NS_LITERAL_CSTRING( > > + "INSERT INTO file(id, refcount) " > > This shouldn't be necessary. If you add something to object_data or > ai_object_data with non-empty file_ids column then your insert/update > trigger will fire and you should get an entry in the updatefunction hash for > this file id and then you'll write the entry with the correct refcount at > commit time. fixed (removed entire block) > > ::: dom/indexedDB/IDBTransaction.cpp > @@ +293,5 @@ > > + NS_ENSURE_TRUE(function, NS_ERROR_OUT_OF_MEMORY); > > + > > + rv = connection->CreateFunction( > > + NS_LITERAL_CSTRING("update_refcount"), 2, function); > > + NS_ENSURE_SUCCESS(rv, false); > > Returning false here is wrong. rv. fixed (also other checks in that block) > > @@ +308,5 @@ > > > > rv = stmt->Execute(); > > NS_ENSURE_SUCCESS(rv, false); > > > > + function.swap(mFunction); > > We need a better name here. mUpdateFileRefcountFunction? fixed > > @@ +951,5 @@ > > return NS_OK; > > } > > + > > +UpdateRefcountFunction::UpdateRefcountFunction(FileManager* aFileManager) > > + : mFileManager(aFileManager) > > Nit: Move left two spaces fixed > > @@ +953,5 @@ > > + > > +UpdateRefcountFunction::UpdateRefcountFunction(FileManager* aFileManager) > > + : mFileManager(aFileManager) > > +{ > > + mFileInfoEntries.Init(); > > This can fail in low mem. Please add a separate Init method and call this > there so that you can handle this failure. fixed > > @@ +956,5 @@ > > +{ > > + mFileInfoEntries.Init(); > > +} > > + > > +NS_IMPL_THREADSAFE_ISUPPORTS1(UpdateRefcountFunction, mozIStorageFunction) > > This doesn't need to be threadsafe, does it? Everything should happen on the > transaction thread. tried it, but I had revert it back, I got the assertions about thread safety :( > > @@ +974,5 @@ > > + PRInt32 type1; > > + aValues->GetTypeOfIndex(0, &type1); > > + > > + PRInt32 type2; > > + aValues->GetTypeOfIndex(1, &type2); > > Since you're not checking the return value here I'd initialize type1 and > type2 to mozIStorageValueArray::VALUE_TYPE_NULL. fixed > > @@ +978,5 @@ > > + aValues->GetTypeOfIndex(1, &type2); > > + > > + if (type1 == mozIStorageValueArray::VALUE_TYPE_NULL && > > + type2 == mozIStorageValueArray::VALUE_TYPE_NULL) { > > + NS_WARNING("Shouldn't be called!"); > > You should assert this. fixed > > @@ +982,5 @@ > > + NS_WARNING("Shouldn't be called!"); > > + } > > +#endif > > + > > + rv = ProcessValue(aValues, 0, -1); > > How about you use true/false here instead of 1/-1? Or maybe make an enum > eDecrement & eIncrement? fixed > > @@ +992,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +UpdateRefcountFunction::UpdateDatabase(mozIStorageConnection* aConnection) > > This can be inlined in the header. fixed > > @@ +994,5 @@ > > + > > +nsresult > > +UpdateRefcountFunction::UpdateDatabase(mozIStorageConnection* aConnection) > > +{ > > + mFileInfoEntries.EnumerateRead(DatabaseUpdateFunction, aConnection); > > Hm, you're not propagating errors here. You need to make a smarter closure > arg here so that you know when something goes wrong. fixed, added a new private class for that > > @@ +1000,5 @@ > > + return NS_OK; > > +} > > + > > +void > > +UpdateRefcountFunction::UpdateFileInfos() > > This can be inlined too. fixed > > @@ +1016,5 @@ > > + if (type == mozIStorageValueArray::VALUE_TYPE_NULL) { > > + return NS_OK; > > + } > > + > > + nsAutoString ids; > > nsString. fixed > > @@ +1050,5 @@ > > +UpdateRefcountFunction::DatabaseUpdateFunction(const PRUint64& aKey, > > + FileInfoEntry* aValue, > > + void* aUserArg) > > +{ > > + // We don't ignore zero deltas intentionally, we need to execute such updates > > If you don't do the thing above where you intentionally write 0 rows then > you can ignore deltas of 0 (which seems much better to me). fixed > > @@ +1063,5 @@ > > + id.AppendInt(aKey); > > + > > + nsCAutoString query; > > + query = NS_LITERAL_CSTRING("UPDATE file SET refcount = refcount + ") + delta + > > + NS_LITERAL_CSTRING(" WHERE id = ") + id; > > Since this query is potentially going to run lots of times you should use a > cached query and bind the arguments (delta and id) to it each time. That > will speed this up significantly. You'll need to pass the IDBTransaction as > part of the closure, and make sure you use a statement scoper helper. fixed, but I couldn't use IDBTransaction for that, it has null mConnection at that point, so new statements can't be created > > ::: dom/indexedDB/IDBTransaction.h > @@ +292,5 @@ > > + FileInfoUpdateFunction(const PRUint64& aKey, > > + FileInfoEntry* aValue, > > + void* aUserArg); > > + > > + nsRefPtr<FileManager> mFileManager; > > Does this need to be a strong ref? Is doesn't seem possible for the > transaction or this updateRefCountFunction to outlive its FileManager. fixed (changed to simple weak ref) > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +635,5 @@ > > correctThread, > > "Running on the wrong thread!"); > > } > > #endif > > + *aDirectory = nsnull; > > This shouldn't be needed, we will never set an invalid directory without > returning an error code. fixed > > @@ +658,5 @@ > > + NS_ENSURE_SUCCESS(rv, rv); > > + } > > + > > + nsTArray<nsRefPtr<FileManager> >* array; > > + if (mFileManagers.Get(aOrigin, &array)) { > > If you just want to check that an entry exists you can pass null for the > second arg here and lose the 'array' variable. fixed > > @@ +710,5 @@ > > + rv = file->IsDirectory(&isDirectory); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + if (isDirectory) { > > + continue; > > This isn't right, we need to add all the files in the subfolder to the quota > management system. Otherwise a db could add unlimited files. well, these directories would be added to quota by the file manager anyway I reworked this code to collect all the subdirs and then verify if they belong to file managers any unknown subdir is considered to be an error (as we talked) > > @@ +713,5 @@ > > + if (isDirectory) { > > + continue; > > + } > > + > > + nsAutoString leafName; > > Please just use nsString here (and below). > > nsAutoString is really only useful if you plan on doing multiple > modifications that could result in several allocations. ok, fixed > > @@ +717,5 @@ > > + nsAutoString leafName; > > + rv = file->GetLeafName(leafName); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + nsAutoString fileDirectoryName; > > Nit: This is the database base filename... Let's rename to dbBaseFilename fixed > > @@ +720,5 @@ > > + > > + nsAutoString fileDirectoryName; > > + if (!GetBaseFilename(leafName, fileDirectoryName)) { > > + NS_WARNING("Unknown file found!"); > > + continue; > > I think we should include any file under this directory in quota management, > so if we find some random file I think we should count it. > reworked to collect unknown files and then verify if they disappear after db and file manager initialization there's an exception for journal files > @@ +732,5 @@ > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + nsCOMPtr<mozIStorageConnection> connection; > > + rv = ss->OpenDatabaseWithVFS(file, NS_LITERAL_CSTRING("quota"), > > + getter_AddRefs(connection)); > > Let's please not do this. We have one place right now where connections are > created and that's really important. Here, for instance, you're not checking > schema version, so if we ever change something in the database table this > could fail. You're also having to duplicate the VFS code, and I'd like to > avoid that too. > > I think we need to rearchitect this opening procedure. Rather than having > this live in the IndexedDatabaseManager it should instead be part of > OpenDatabaseHelper. Maybe IndexedDatabaseManager just needs to hold a > boolean flag indicating whether or not an origin has been initialized. fixed, CreateDatabaseConnection (OpenDatabaseHelper.cpp) is now a static method > > @@ +759,5 @@ > > + NS_ERROR("Database has no name!"); > > + return NS_ERROR_UNEXPECTED; > > + } > > + > > + nsAutoString databaseName; > > nsString fixed > > @@ +763,5 @@ > > + nsAutoString databaseName; > > + rv = stmt->GetString(0, databaseName); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + nsRefPtr<FileManager> fileManager = new FileManager(aOrigin, databaseName); > > Hm, I'd rather we did this lazily. It's hard to imagine that a database > needs to interact with all its files, especially at the same time. Load() has been split to InitDirectory() and Load() InitDirectory() is called during origin initialization, Load() only when a database is really being opened > > @@ +818,5 @@ > > + if (!fileManager) { > > + fileManager = new FileManager(aOrigin, aDatabaseName); > > + > > + if (!array->AppendElement(fileManager)) { > > + NS_WARNING("Out of memory?"); > > This is infallible. fixed > > @@ +827,5 @@ > > + return fileManager.forget(); > > +} > > + > > +void > > +IndexedDatabaseManager::InvalidateFileManagersFor(const nsACString& aOrigin) > > Nit: Either "InvalidateAllFileManagers" or "InvalidateFileManagersForOrigin" fixed > > @@ +840,5 @@ > > + } > > +} > > + > > +void > > +IndexedDatabaseManager::InvalidateFileManagerFor(const nsACString& aOrigin, > > Nit: Just "InvalidateFileManager" fixed > > @@ +857,5 @@ > > + break; > > + } > > + } > > + > > + if (array->IsEmpty()) { > > Nit: This check can go inside the loop where you actually remove an element. fixed > > @@ +1217,5 @@ > > // Call the callback unless we were canceled. > > if (!mCanceled) { > > + PRUint64 usage = mUsage; > > + // Watch for overflow! > > + if (NS_UNLIKELY((LL_MAXINT - usage) <= mFileUsage)) { > > Can we make this an inline function? You do something similar below in > GetUsageForDirectory. > > These NS_(UN)LIKELY things are semi-useless and make the code harder to > read. Let's avoid them. fixed both > > @@ +1265,5 @@ > > + > > +nsresult > > +IndexedDatabaseManager::AsyncUsageRunnable::GetUsageForDirectory( > > + nsIFile* aDirectory, > > + PRUint64* aUsage) > > Hm. I think this should be a static function, and I don't think its worth > separating out db sizes and file sizes. They all count towards the same > quota and we don't provide the web pages any API to determine this. hmm, I didn't changed it to a static function and I'm also keeping the separate file usage. I need it for testing (see test_file_os_delete.html) > > @@ +1295,3 @@ > > > > + if (isDirectory) { > > + rv = GetUsageForDirectory(file, &mFileUsage); > > Hm, this means that any subdirectory of the files directory will be counted > also. I think we should a) track our recursion depth here to make sure that > we never go beyond one subdirectory, and b) issue a warning here since that > should never happen. > fixed, there's now only one level of recursion and also a warning for subdirs in files directory > @@ +1443,5 @@ > > + NS_ENSURE_TRUE(ss, NS_ERROR_FAILURE); > > + > > + nsCOMPtr<mozIStorageVirtualFile> virtualFile; > > + nsresult rv = ss->GetVirtualFile(NS_LITERAL_CSTRING("quota"), > > + getter_AddRefs(virtualFile)); > > I'm not sure why this is necessary... Can't you just delete the file and > then call UpdateQuota? fixed, we now use the all new SQLite API > > @@ +1449,5 @@ > > + NS_WARNING("Failed to get virtual file!"); > > + return NS_ERROR_FAILURE; > > + } > > + > > + rv = virtualFile->Init(mFile); > > We shouldn't mess with the file that we got from the main thread. Instead I > think you should just store the path in the runnable and then create a new > file object here. removed > > ::: dom/indexedDB/IndexedDatabaseManager.h > @@ +291,5 @@ > > + NS_ASSERTION(mFile, "Null file!"); > > + } > > + > > + private: > > + nsCOMPtr<nsIFile> mFile; > > This may be released on the IO thread which is bad news. fixed (only path is now stored) > > ::: dom/indexedDB/OpenDatabaseHelper.cpp > @@ +85,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +CreateTables6(mozIStorageConnection* aDBConn) > > We should have a better name here... CreateFileTables? fixed > > @@ +90,5 @@ > > +{ > > + // Table `file` > > + nsresult rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > > + "CREATE TABLE file (" > > + "id INTEGER, " > > Make this INTEGER PRIMARY KEY, then remove the PRIMARY KEY line below. fixed > > @@ +465,5 @@ > > return NS_OK; > > } > > > > nsresult > > +UpgradeSchemaFrom5To6(mozIStorageConnection* aConnection) > > All of this has changed a bit, sorry :( :) > > @@ +1435,5 @@ > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + > > + nsCOMPtr<nsIFile> dbFile; > > + rv = directory->Clone(getter_AddRefs(dbFile)); > > + NS_ENSURE_SUCCESS(rv, rv); > > Can't return rv here, it's not going to be an INDEXEDDB_ error code. fixed > > @@ +1473,5 @@ > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + > > + if (exists) { > > + // FIXME: use virtual files! > > + rv = fileDirectory->Remove(true); > > I guess this will need to be fixed before we check this in. fixed, it uses the new SQLite API > > ::: storage/public/Makefile.in > @@ +70,5 @@ > > mozIStorageBaseStatement.idl \ > > mozIStorageAsyncStatement.idl \ > > mozIStorageServiceQuotaManagement.idl \ > > mozIStorageVacuumParticipant.idl \ > > + mozIStorageVirtualFile.idl \ > > Nit: Use spaces, not tabs. this interfaces has been removed > > ::: storage/public/mozIStorageServiceQuotaManagement.idl > @@ -127,5 @@ > > * exists then its size information will be added or refreshed. If the > > * file does not exist then the file will be removed from tracking > > * under the quota system. > > */ > > - void updateQutoaInformationForFile(in nsIFile aFile); > > Eek! Glad you caught that. ok > > ::: storage/public/mozIStorageVirtualFile.idl > @@ +39,5 @@ > > + > > +interface nsIFile; > > + > > +[scriptable, uuid(3ab22a71-c7cf-465d-afe6-108f9de619a5)] > > +interface mozIStorageVirtualFile : nsISupports > > This interface needs lots of comments explaining what it's used for. > > @@ +42,5 @@ > > +[scriptable, uuid(3ab22a71-c7cf-465d-afe6-108f9de619a5)] > > +interface mozIStorageVirtualFile : nsISupports > > +{ > > + > > + void init(in nsIFile aFile); > > Why have separate Init and Open methods? Please combine unless I'm missing > something. > > @@ +48,5 @@ > > + void open(); > > + > > + void delete(); > > + > > + void close(); > > Must delete always be called if open() succeeds? Please comment. > > @@ +54,5 @@ > > + void write(in string aBuf, > > + in long aCount, > > + in long long aOffset); > > + > > + void sync(); > > Must this be called before close()? Please comment. > > @@ +56,5 @@ > > + in long long aOffset); > > + > > + void sync(); > > + > > + long long size(); > > unsigned? And make this a readonly attribute, not a function. the interface has been removed > > ::: storage/src/mozStorageService.cpp > @@ +648,5 @@ > > +NS_IMETHODIMP > > +Service::GetVirtualFile(const nsACString &aVFSName, > > + mozIStorageVirtualFile **aResult) > > +{ > > + *aResult = nsnull; > > Nit: Move this into the |!vfs| block so that we only set *aResult once. > > @@ +651,5 @@ > > +{ > > + *aResult = nsnull; > > + > > + sqlite3_vfs* vfs = nsnull; > > + if (aVFSName.EqualsLiteral("default")) { > > Please use sqlite3_vfs_find() here. We don't want to hardcode anything like > this. > > ::: storage/src/mozStorageVirtualFile.cpp > @@ +51,5 @@ > > + > > +VirtualFile::~VirtualFile() > > +{ > > + if (mFd) { > > + nsresult rv = Sync(); > > Please don't call virtual methods in destructors. I think it's sufficient to > assert that mFd is null here and force users to call close themselves. > > @@ +87,5 @@ > > + NS_ENSURE_SUCCESS(rv, rv); > > + } > > + > > + nsCAutoString path; > > + rv = mFile->GetNativePath(path); > > This isn't what you want... You want GetPath followed by a > ConvertUTF16toUTF8 since SQLite deals with UTF8 paths. > > @@ +97,5 @@ > > + } > > + > > + int outFlags = 0; > > + int rc = mVFS->xOpen(mVFS, PromiseFlatCString(path).get(), fd, > > + SQLITE_OPEN_MAIN_DB|SQLITE_OPEN_CREATE|SQLITE_OPEN_READWRITE, &outFlags); > > From the docs: > > "SQLite guarantees that the zFilename parameter to xOpen is either a NULL > pointer or string obtained from xFullPathname()" > > I think you need to do the same... And this seems fragile. I'll check with > the SQLite devs to see if there's a better way to do this sort of thing > without having to duplicate a lot of SQLite's internal logic. > > ::: storage/src/mozStorageVirtualFile.h > @@ +58,5 @@ > > +private: > > + sqlite3_vfs* mVFS; > > + sqlite3_file* mFd; > > + > > + nsCOMPtr<nsIFile> mFile; > > I don't think you really want to hold the nsIFile here... Can't you just > hold the path as a string? That seems to be all you ever use, and you > wouldn't have to convert it every time. all this code has been removed, we now use the new SQLite API which does pretty much the same

This patch should be incorporated in SQLite dev tree

additional fixes and improvements in the patch v0.4: - removed |#include "nsNetUtil.h"| from IDBObjectStore.cpp - added binding of fileIds to the third block (where BindBlobByName() is called) in AddHelper:DoDatabaseWork() - removed |NS_ASSERTION(!aName.IsEmpty(), "Bad argument!")| from GetDatabaseFilename() - changed |AFTER UPDATE ON object_data| to |AFTER UPDATE OF file_ids ON object_data| - added allowUnlimitedQuota()/resetUnlimitedQuota() to helpers.js - fixed slices to keep file infos alive too The cleanup of files directory is now implemented using a virtual table (filesystem) There are 3 implementations of FileInfo - FileInfo16/32/64. The file id never changes so we can save some bytes this way.

Comment on attachment 581014 [details] [diff] [review] interdiff v0.3 - v0.4 Review of attachment 581014 [details] [diff] [review]: ----------------------------------------------------------------- I haven't finished everything but I figured I'd get these comments up sooner rather than later. ::: /Users/varga/Sources/Mozilla5/mozilla-central/content/base/public/nsDOMFile.h @@ +80,1 @@ > nsDOMFileBase(const nsAString& aName, const nsAString& aContentType, Nit, remove extra line you added here. @@ +136,5 @@ > > PRUint64 mStart; > PRUint64 mLength; > > + nsTArray<nsRefPtr<indexedDB::FileInfo> > mFileInfos; Nit, add comment saying this is always protected with mutex. ::: /Users/varga/Sources/Mozilla5/mozilla-central/db/sqlite3/src/Makefile.in @@ +90,5 @@ > endif > > EXPORTS = \ > sqlite3.h \ > + test_quota.h \ Hm, I don't think we should export this. We can add db/sqlite3/src to LOCAL_INCLUDES in the indexeddb makefile instead. ::: /Users/varga/Sources/Mozilla5/mozilla-central/storage/src/FileSystemModule.cpp @@ +39,5 @@ > +#include "nsISimpleEnumerator.h" > +#include "nsIFile.h" > +#include "nsILocalFile.h" > + > +#include "FileSystemModule.h" Nit: always include your header first to make sure your header builds on its own. @@ +44,5 @@ > + > +namespace mozilla { > +namespace storage { > + > +namespace { Nit: move the anonymous stuff out of mozilla::storage @@ +46,5 @@ > +namespace storage { > + > +namespace { > + > +struct VirtualTable Honestly, I'd prefer you inherit mBase rather than keep it as the first member. That would be cleaner and safer in my opinion. @@ +50,5 @@ > +struct VirtualTable > +{ > + VirtualTable() > + { > + memset(&mBase, 0, sizeof(sqlite3_vtab)); sizeof(mBase) @@ +61,5 @@ > +{ > + VirtualTableCursor() > + : mRowId(0), mEof(true) > + { > + memset(&mBase, 0, sizeof(sqlite3_vtab_cursor)); sizeof(mBase) @@ +68,5 @@ > + sqlite3_vtab_cursor mBase; > + > + nsCOMPtr<nsIFile> mDirectory; > + nsCOMPtr<nsISimpleEnumerator> mEntries; > + nsCOMPtr<nsIFile> mCurrentFile; I don't think you need mDirectory or mCurrentFile. All you need is mEntries. Then you can change your GetNextFile to be GetNextFileName since that's all you really care about. @@ +73,5 @@ > + > + nsCString mDirectoryPath; > + nsCString mCurrentFileName; > + > + int mRowId; This needs to be int64 @@ +74,5 @@ > + nsCString mDirectoryPath; > + nsCString mCurrentFileName; > + > + int mRowId; > + bool mEof; I don't think you need this, just test mCurrentFileName.IsVoid()? @@ +77,5 @@ > + int mRowId; > + bool mEof; > +}; > + > +static static isn't needed inside anonymous namespace, here and below. @@ +78,5 @@ > + bool mEof; > +}; > + > +static > +int fsConnect(sqlite3* aDB, void* aAux, int aArgc, const char* const* aArgv, Nit: Here and below I think you should remove the 'fs' prefix. It doesn't really buy you anything as far as I can tell. @@ +83,5 @@ > + sqlite3_vtab** aVtab, char** aErr) > +{ > + VirtualTable* vt = new VirtualTable(); > + > + static const char* ddl = Nit: static const char ddl[]... And what is 'ddl'? How about a nicer name :) @@ +84,5 @@ > +{ > + VirtualTable* vt = new VirtualTable(); > + > + static const char* ddl = > + "CREATE TABLE fs (" Hm, can this conflict with a table name already in the database? Maybe you should make this a little crazier, something like mozilla_virtual_file_system_table? @@ +89,5 @@ > + "name TEXT, " > + "path TEXT" > + ")"; > + > + sqlite3_declare_vtab(aDB, ddl); It looks like this can fail, you should check the result. Also, you'll need to clean up vt if it fails. @@ +91,5 @@ > + ")"; > + > + sqlite3_declare_vtab(aDB, ddl); > + > + *aVtab = &vt->mBase; Hm. Below you're assuming that sqlite3_vtab* and VirtualTable* are identical. I think you should do this: *aVtab = reinterpret_cast<sqlite3_vtab*>(vt); @@ +99,5 @@ > + > +static > +int fsDisconnect(sqlite3_vtab* aVtab ) > +{ > + VirtualTable* vt = (VirtualTable*)aVtab; reinterpret_cast please @@ +107,5 @@ > + return SQLITE_OK; > +} > + > +static > +int fsBestIndex(sqlite3_vtab* aVtab, sqlite3_index_info* aInfo) The docs say: "The xBestIndex method must therefore only consider constraints that have an aConstraint[].usable flag which is true." I think we should make sure to check that. @@ +112,5 @@ > +{ > + for(int i = 0; i < aInfo->nConstraint; i++) { > + if (aInfo->aConstraint[i].iColumn == 1) { > + if (aInfo->aConstraint[i].op & SQLITE_INDEX_CONSTRAINT_EQ) { > + aInfo->aConstraintUsage[i].argvIndex = 1; This could use a comment or five ;) So you're not considering name at all (only path), which is fine for now, but I think you should add some TODO comment saying that this could be much smarter for a single file, seeing as this is a generic-ish module you're adding. @@ +126,5 @@ > +int fsOpen(sqlite3_vtab* aVtab, sqlite3_vtab_cursor** aCursor) > +{ > + VirtualTableCursor* cursor = new VirtualTableCursor(); > + > + *aCursor = (sqlite3_vtab_cursor*)cursor; reinterpret_cast @@ +134,5 @@ > + > +static > +int fsClose(sqlite3_vtab_cursor* aCursor) > +{ > + VirtualTableCursor* cursor = (VirtualTableCursor*)aCursor; Here too. @@ +141,5 @@ > + > + return SQLITE_OK; > +} > + > +nsresult GetNextFile(nsISimpleEnumerator* aEntries, nsIFile** aFile) So, I think this could probably be moved to be a member function on VirtualTableCursor, right? And I think instead of handing out an nsIFile you should just hand out a filename. If !hasMore then you can simply set the string to void. @@ +166,5 @@ > + > +int fsFilter(sqlite3_vtab_cursor* aCursor, int aIdxNum, const char* aIdxStr, > + int aArgc, sqlite3_value** aArgv) > +{ > + VirtualTableCursor* cursor = (VirtualTableCursor*)aCursor; reinterpret_cast here and in all the other methods below. @@ +168,5 @@ > + int aArgc, sqlite3_value** aArgv) > +{ > + VirtualTableCursor* cursor = (VirtualTableCursor*)aCursor; > + > + if(aArgc > 0) { Nit: In general I'm much happier with early exit rather than lots of extra indentation. @@ +172,5 @@ > + if(aArgc > 0) { > + nsCOMPtr<nsILocalFile> directory = > + do_CreateInstance(NS_LOCAL_FILE_CONTRACTID); > + if (!directory) { > + return SQLITE_NOMEM; NS_ENSURE_SUCCESS, here and everywhere else in this method. @@ +232,5 @@ > + > +static > +int fsEof(sqlite3_vtab_cursor* aCursor) > +{ > + return ((VirtualTableCursor*)aCursor)->mEof ? 1 : 0; mCurrentFilename.IsVoid() @@ +242,5 @@ > +{ > + VirtualTableCursor* cursor = (VirtualTableCursor*)aCursor; > + > + nsresult rv; > + switch(aColumnIndex) { Nit: space before ( @@ +249,5 @@ > + if (cursor->mCurrentFileName.IsEmpty()) { > + nsString name; > + rv = cursor->mCurrentFile->GetLeafName(name); > + if (NS_FAILED(rv)) { > + return SQLITE_ERROR; NS_ENSURE_SUCCESS @@ +251,5 @@ > + rv = cursor->mCurrentFile->GetLeafName(name); > + if (NS_FAILED(rv)) { > + return SQLITE_ERROR; > + } > + CopyUTF16toUTF8(name, cursor->mCurrentFileName); According to the docs SQLITE_TRANSIENT will cause sqlite to copy whatever you hand it... So don't copy&convert here, just use sqlite3_result_text16 and let sqlite handle the conversion. @@ +273,5 @@ > + cursor->mDirectoryPath.Length(), SQLITE_TRANSIENT); > + break; > + } > + default: > + sqlite3_result_text(aContext, "", 0, SQLITE_STATIC); I think you should add a NS_NOTREACHED here. This should never happen. @@ +316,5 @@ > + NULL, > + NULL > + }; > + > + return sqlite3_create_module(aDB, "filesystem", &fsModule, NULL); nsnull. ::: /Users/varga/Sources/Mozilla5/mozilla-central/storage/src/FileSystemModule.h @@ +37,5 @@ > + > +#ifndef mozilla_storage_FileSystemModule_h > +#define mozilla_storage_FileSystemModule_h > + > +#include "sqlite3.h" I'd just forward declare struct sqlite3 @@ +42,5 @@ > + > +namespace mozilla { > +namespace storage { > + > +NS_HIDDEN_(int) registerFileSystemModule(sqlite3* aDB); Nit: Capitalize 'R'egister ::: /Users/varga/Sources/Mozilla5/mozilla-central/storage/src/mozStorageConnection.cpp @@ +691,5 @@ > mDBConn = nsnull; > return convertResultCode(srv); > } > > + srv = registerFileSystemModule(mDBConn); Hm. We can't do this here, we don't want to touch all sqlite connections in the whole system. We'll need to register these on indexeddb connections only. I think the solution is to modify mozIStorageConnection.idl and add this before the interface is declared: %{C++ struct sqlite3_module; %} [ptr] native SQLiteModulePtr(sqlite3_module); Then in the interface add this: [noscript] void RegisterVirtualTable(in ACString aModuleName, in SQLiteModulePtr aModulePtr); The you can call this manually in OpenDatabaseHelper.cpp:CreateDatabaseConnection. You may have to rework the FileSystemModule.h file a little but this should be more flexible going forward.

I changed nsDOMFileBase::AddFileInfo() impl to: { if (indexedDB::IndexedDatabaseManager::IsClosed()) { NS_ERROR("Shouldn't be called after shutdown!"); return; } nsRefPtr<indexedDB::FileInfo> fileInfo = aFileInfo; MutexAutoLock lock(indexedDB::IndexedDatabaseManager::FileMutex()); NS_ASSERTION(!mFileInfos.Contains(aFileInfo), "Adding the same file info agan?!"); nsRefPtr<indexedDB::FileInfo>* element = mFileInfos.AppendElement(); fileInfo.swap(*element); } this one should be safer

Comment on attachment 581741 [details] [diff] [review] interdiff v0.4 - v0.5 Review of attachment 581741 [details] [diff] [review]: ----------------------------------------------------------------- ::: /Users/varga/Sources/Mozilla5/mozilla-central/content/base/src/nsDOMFile.cpp @@ +339,5 @@ > > + fileInfo = mFileInfos.AppendElement(); > + } > + > + *fileInfo = aFileInfo; I don't think this is right... Merely appending an element to the list while under the lock isn't what you want to do. You want to actually add the fileinfo too. @@ +353,2 @@ > > + PRUint32 startIndex = IsStoredFile() && !IsWholeFile() ? 1 : 0; This needs a bit of explanation... Why do this? ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/FileInfo.cpp @@ +61,5 @@ > void > FileInfo::GetReferences(PRInt32* aRefCnt, PRInt32* aDBRefCnt, > PRInt32* aSliceRefCnt) > { > + if (IndexedDatabaseManager::IsClosed()) { You should probably set the outparams to something, -1 maybe? Leaving them unset seems dangerous. ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/FileInfo.h @@ +69,5 @@ > void AddRef() > { > + if (IndexedDatabaseManager::IsClosed()) { > + NS_AtomicIncrementRefcnt(mRefCnt); > + } else { Nit: Here and below put | else {| on its own line. @@ +90,5 @@ > > void UpdateDBRefs(PRInt32 aDelta) > { > + if (IndexedDatabaseManager::IsClosed()) { > + NS_ERROR("Shouldn't be called after shutdown!"); Hm, how about moving this into UpdateReferences? The you wouldn't have to duplicate all this in each function and you could just call UpdateReferences safely. ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/IDBTransaction.cpp @@ +946,2 @@ > } > + } else { No cuddling please. ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/test/file.js @@ +201,5 @@ > } > > function scheduleGC() > { > +// SpecialPowers.exactGC(window, continueToNextStep); Is this going away? ::: /Users/varga/Sources/Mozilla5/mozilla-central/storage/src/FileSystemModule.cpp @@ +64,5 @@ > > sqlite3_vtab_cursor mBase; > +}; > + > +struct VirtualTable : public VirtualTableBase This is empty so let's kill it entirely. @@ +166,5 @@ > ")"; > > + int rc = sqlite3_declare_vtab(aDB, virtualTableSchema); > + if (rc != SQLITE_OK) { > + delete vt; How about you only allocate if this succeeds? Then you wouldn't have to do this. @@ +253,5 @@ > > + nsDependentCString text( > + reinterpret_cast<const char *>(::sqlite3_value_text(aArgv[0])), > + ::sqlite3_value_bytes(aArgv[0])); > + NS_ConvertUTF8toUTF16 path(text); Looks like you could avoid this and call sqlite3_value_text16, then use nsDependentString.

Comment on attachment 581014 [details] [diff] [review] interdiff v0.3 - v0.4 Review of attachment 581014 [details] [diff] [review]: ----------------------------------------------------------------- ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/FileManager.cpp @@ +99,5 @@ > } > > + nsCOMPtr<mozIStorageStatement> stmt; > + rv = aConnection->CreateStatement(NS_LITERAL_CSTRING( > + "SELECT name, (name IN (SELECT id from FILE)) FROM fs " Nit: "SELECT id FROM file" ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/IndexedDatabaseManager.cpp @@ +679,5 @@ > + // are database files then we need to create file managers for them and also > + // tell SQLite about all of them. > + > + nsAutoTArray<nsString, 20> subdirectories; > + nsAutoTArray<nsCOMPtr<nsIFile> , 20> unknownFiles; I'd like to change these to be hashsets, but we can do that in a followup. @@ +711,4 @@ > continue; > } > > + nsAutoString dbBaseFilename; nsString. @@ +755,3 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + rv = fileManager->InitDirectory(fileManagerDirectory, connection); Hm, can you combine FileManager::Init and FileManager::InitDirectory and just call it Init? @@ +764,5 @@ > } > NS_ENSURE_SUCCESS(rv, rv); > > + for (PRUint32 i = 0; i < subdirectories.Length(); i++) { > + const nsString& subdirectory = subdirectories.ElementAt(i); Nit: Here and below you can just use |array[index]| rather than |array.ElementAt(index)|. @@ +783,5 @@ > + for (PRUint32 i = 0; i < unknownFiles.Length(); i++) { > + nsCOMPtr<nsIFile>& unknownFile = unknownFiles.ElementAt(i); > + > + bool exists; > + rv = unknownFile->Exists(&exists); We don't need this check, do we? It wouldn't be in our list if it didn't exist already. @@ +801,1 @@ > if (!mFileManagers.Put(aOrigin, fileManagers.forget())) { Oh, this won't work... If it fails then we'll leak a FileManager. Just do the forget() after it succeeds below. @@ +930,5 @@ > > if (!fileManager) { > fileManager = new FileManager(aOrigin, aDatabaseName); > > + if (NS_FAILED(fileManager->Init())) { This is the only call to Init() without a followup for InitDirectory(). I think we should change this to GetFileManager(), not GetOrCreate(), and just return null here. Then DOMWindowUtils can deal accordingly. Can do in a followup since this is only going to happen in mochitests. ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/IndexedDatabaseManager.h @@ +380,5 @@ > + > + // Lock protecting nsDOMFileBase::mFileInfos > + mozilla::Mutex mDOMFileMutex; > + > + nsString mVoidString; This is only used in one spot, let's not have it as a member but on the stack in EnsureOriginIsInitialized. ::: /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/OpenDatabaseHelper.cpp @@ +211,5 @@ > )); > NS_ENSURE_SUCCESS(rv, rv); > > + rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "CREATE VIRTUAL TABLE fs USING filesystem;" Hm. I'd rather we didn't do this here. How about we only create this table in IndexedDatabaseManager::EnsureOriginIsInitialized and then call DROP TABLE as soon as we're done looping through FileManager creation. That way we won't hang on to extra tables. on every connection. @@ +1434,5 @@ > + nsCOMPtr<mozIStorageConnection> connection; > + nsresult rv = ss->OpenDatabaseWithVFS(aDBFile, quotaVFSName, > + getter_AddRefs(connection)); > + if (rv == NS_ERROR_FILE_CORRUPTED) { > + if (aName.IsVoid()) { This needs a comment.

Comment on attachment 582110 [details] [diff] [review] patch v0.6a Thanks for this! Looks great.

Woo! Thanks Jan!

Awesome! How do I start using it?

Woo hoo! Thanks!

(In reply to Ben Francis from comment #45) > Awesome! How do I start using it? That's actually very easy, just pass a DOM blob or file to .add() or .put() Just like any other object. The most interesting part is sharing of files: myblob = inputelement.files[0]; indexedDB.open("mydatabase").onsuccess = function(e) { var db = e.result; var trans = db.transaction(["objectstore1", "objectstore2", READ_WRITE); trans.objectStore("objectstore1").put(myblob, "somekey"); trans.objectStore("objectstore2").put(myblob, "otherkey"); } // myblob is stored only once with two references // the file is automatically and asynchronously deleted when the last reference is dropped indexedDB.open("mydatabase").onsuccess = function(e) { var db = e.result; var trans = db.transaction(["aliens", "deadaliens", READ_WRITE); trans.objectStore("aliens").get("guywhogotshot").onsuccess = function(e) { // e.result contains a blob somewhere in the object graph trans.objectStore("deadaliens").put(e.result); } } // again, the file is not copied

And you can even store the Blob inside of a "JSON object". So the following works just fine: objectStore.put({ name: "Santa", age: 400, height: 185, img: myblob}); In other words, Blob and File are just another type supported by indexedDB, along with numbers, strings, bools, arrays, objects etc. They work just like all other types.

(In reply to ben turner [:bent] from comment #44) > Woo! Thanks Jan! thank you for solving all the windows issues!

Sounds good, thanks, I'll have a go on Monday. I don't have to base64 encode audio files any more!

I've added a note to https://developer.mozilla.org/en-US/Firefox/Releases/11#IndexedDB And also added an explicit mention of being able to store files/blobs on the homepage, just so it is completely obvious: https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API