Some example code: var fileHandle = db.mozCreateFileHandle("file.txt", "text"); var request = db.transaction(["test"], IDBTransaction.READ_WRITE) .objectStore("test").add(fileHandle, key); db.transaction(["test"]).objectStore("test").get(key).onsuccess = function(event) { var fileHandle = event.target.result; var lockedFile = fileHandle.open("w"); lockedFile.position = 123; lockedFile.read(1).onsuccess = function(event) { var value = event.target.result; lockedFile.write(value.toUpperCase()); } }

What is the use case here?

I'll send a WIP patch today (w/o TransactionThreadPool changes since there is still no consensus on how to process IDBLockedFile objects)

The use case is to be able to do basic file-writing. Currently you can't write to a file, you can only construct new ones. This means that if you have a 10MB file and want to append 10 bytes to it, or write 10 bytes in the middle of it, you'll have to write a full 10MB of data to disk.

FileHandle API proposal: http://lists.w3.org/Archives/Public/public-webapps/2012JanMar/0886.html

TODO: - cleanup and optimization - self review - more automatic tests fastRead(), fastWrite(), etc. not yet supported, not sure if we need those in the initial version

Jan: An alternative to the fast* methods might be to simply expose a .close() method on the lock. Calling .close() wouldn't affect any already scheduled requests, but would prevent any new requests from being placed (.read/.write would throw if called on a closed lock). That way the IO thread knows when the last request is run and can start processing requests from the next lock immediately. Let me know what you think.

I'll think about it

Ben, could you negotiate these changes with sqlite developers ?

(In reply to Jonas Sicking (:sicking) from comment #11) > Jan: An alternative to the fast* methods might be to simply expose a > .close() method on the lock. Calling .close() wouldn't affect any already > scheduled requests, but would prevent any new requests from being placed > (.read/.write would throw if called on a closed lock). That way the IO > thread knows when the last request is run and can start processing requests > from the next lock immediately. > > Let me know what you think. Ok, I experimented with this a bit ... Unfortunately, the .close() method doesn't help much (from the point of view of current design). A file (file stream) must be (flushed and) closed before the next lock (locked file) opens the file. Flushing must run on the IO thread and checking for the closed flag on the main thread. So we would end up with several round-trips anyway. I think we can merge a file operation with (flushing and) closing the file if we know that there's only one request in advance. This info could be passed in several ways: 1. FileHandle.open(mode, oneShot = true) 2. LockedFile.write(data, last = true) 3. shadow file operations on the FileHandle interface - this looks quite nice Anyway, I'll think about it more.

(In reply to Jan Varga [:janv] from comment #14) > 3. shadow file operations on the FileHandle interface - this looks quite nice ok, I experimentally added FileHandle.write(in long location, jsval data) and it seems to work fine: - fclose() is called right after fwrite() - no need to wait for the request success handler - no need to to close the file using separate helper (additional IO thread <-> main thread round-trip) so when a write is done (on IO thread) it still needs to post an event to main thread which then releases the lock, but entire processing should be much faster then the full (multi request) locked file also, the "fast" version also provides a simplified API (besides the performance advantage) and we should be able to share real implementation with LockedFile.write(data)

We have decided (Jonas and I) to get reviews for the current patch (the fast methods or .close() method will not be available for now) Anyway, I need to do some cleanup and better error checking Will send a patch for review ASAP.

bah, it seems there's a bug in gnulib (on linux) http://lists.gnu.org/archive/html/bug-gnulib/2011-08/msg00073.html calling a flush after each request solves the problem, but I don't want to degrade performance on all platforms, so I'm trying to find a linux only fix

So just a drive-by comment: We shouldn't need MemoryInputStream. When we are passed a JS-string we need to UTF8 encode anyway, and so you can just UTF8 encode into a normal XPCOM nsCString, and then use NS_NewCStringInputStream. When you are handed an ArrayBuffer you can just use NS_NewByteInputStream and pass it NS_ASSIGNMENT_COPY (we need to copy the data since ArrayBuffer's can be mutated).

(In reply to Jan Varga [:janv] from comment #17) > bah, it seems there's a bug in gnulib (on linux) > http://lists.gnu.org/archive/html/bug-gnulib/2011-08/msg00073.html > > calling a flush after each request solves the problem, but I don't want to > degrade performance on all platforms, so I'm trying to find a linux only fix Never mind, it's a different problem... DOM read/write requests are dispatched to a thread pool, so each request can end up on a different thread. For some reason we have to call fflush/fpurge after each such DOM write/read request on linux.

Yeah, it's now working fine on all platforms (including native fennec)

(In reply to Jonas Sicking (:sicking) from comment #18) > So just a drive-by comment: We shouldn't need MemoryInputStream. When we are > passed a JS-string we need to UTF8 encode anyway, and so you can just UTF8 > encode into a normal XPCOM nsCString, and then use NS_NewCStringInputStream. > When you are handed an ArrayBuffer you can just use NS_NewByteInputStream > and pass it NS_ASSIGNMENT_COPY (we need to copy the data since ArrayBuffer's > can be mutated). ok, it works anyway, I added SetUnicodeData(const nsAString&) to nsStringStream impl, so we can avoid the extra memory allocation and UTF8 encode directly into nsStringStream internal member

I fixed quite serious leak, will attach a new patch today

more tests

Comment on attachment 617475 [details] [diff] [review] patch v0.4 Review of attachment 617475 [details] [diff] [review]: ----------------------------------------------------------------- Not done, just giving a snapshot. ::: content/base/public/nsDOMFile.h @@ +76,5 @@ > public: > typedef mozilla::dom::indexedDB::FileInfo FileInfo; > > + nsDOMFile(const nsAString& aName, const nsAString& aContentType, > + PRUint64 aLength) This is always going to be inherited now, right? Let's make the constructors/destructor protected. @@ +151,5 @@ > // Protected by IndexedDatabaseManager::FileMutex() > nsTArray<nsRefPtr<FileInfo> > mFileInfos; > }; > > +class nsDOMFileBase : public nsDOMFile It's weird that the "Base" class is not actually the base class. I think we should reverse this. @@ +158,5 @@ > + nsDOMFileBase(const nsAString& aName, const nsAString& aContentType, > + PRUint64 aLength) > + : nsDOMFile(aName, aContentType, aLength) > + { > + } Nit: { } is fine, here and below. ::: content/base/src/nsContentUtils.cpp @@ +350,5 @@ > NS_DECL_NSICHANNELEVENTSINK > NS_DECL_NSIINTERFACEREQUESTOR > }; > > +class nsCharsetDetectionObserver : public nsICharsetDetectionObserver This could just be in an anonymous namespace, without the ns prefix. @@ +3554,5 @@ > + nsICharsetDetectionObserver) > + > +NS_IMETHODIMP > +nsCharsetDetectionObserver::Notify(const char *aCharset, > + nsDetectionConfident aConf) You could just inline this. @@ +3567,5 @@ > + nsACString &aCharset) > +{ > + // First try the universal charset detector > + nsCOMPtr<nsICharsetDetector> detector > + = do_CreateInstance(NS_CHARSET_DETECTOR_CONTRACTID_BASE Nit: = on prev line @@ +3575,5 @@ > + const nsAdoptingCString& detectorName = > + Preferences::GetLocalizedCString("intl.charset.detector"); > + if (!detectorName.IsEmpty()) { > + nsCAutoString detectorContractID; > + detectorContractID.AssignLiteral(NS_CHARSET_DETECTOR_CONTRACTID_BASE); Nit: You can pass this in the constructor. @@ +3585,5 @@ > + nsresult rv; > + > + // The charset detector doesn't work for empty (null) aData. Testing > + // aDataLen instead of aData so that we catch potential errors. > + if (detector && aDataLen != 0) { Nit: (detector && aDataLen) @@ +3589,5 @@ > + if (detector && aDataLen != 0) { > + nsRefPtr<nsCharsetDetectionObserver> observer = > + new nsCharsetDetectionObserver(); > + > + detector->Init(observer); Looks like this can fail. ::: content/base/src/nsDOMFile.cpp @@ +340,5 @@ > // copied again. That's why we have to ignore the first file info. > + // Snapshots are handled in a similar way (they have to be copied). > + PRUint32 startIndex = 0; > + if (IsStoredFile() && (!IsWholeFile() || IsSnapshot())) { > + startIndex = 1; Nit: Avoid the double assignment. @@ +430,5 @@ > + > +//////////////////////////////////////////////////////////////////////////// > +// nsDOMFileBaseCC implementation > + > +NS_IMPL_CYCLE_COLLECTION_CLASS(nsDOMFileBaseCC) NS_IMPL_CYCLE_COLLECTION_0 for these @@ +440,5 @@ > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END > + > +// XXX > +//DOMCI_DATA(File, nsDOMFileBaseCC) > +//DOMCI_DATA(Blob, nsDOMFileBaseCC) I think it's safe to remove these. ::: dom/base/DOMRequest.cpp @@ +104,5 @@ > { > NS_ABORT_IF_FALSE(!mDone, "Already fired success/error"); > > mDone = true; > + if (!JSVAL_IS_VOID(aResult)) { I think you want JSVAL_IS_GCTHING @@ +126,5 @@ > > void > +DOMRequest::FireError(nsresult aError, bool aBubble, bool aCancelable) > +{ > + NS_ABORT_IF_FALSE(!mDone, "Already fired success/error"); Also assert !mError ::: dom/base/DOMRequest.h @@ +29,5 @@ > nsDOMEventTargetHelper) > > void FireSuccess(jsval aResult); > + void FireError(const nsAString& aError, > + bool aBubble = false, bool aCancelable = false); These are always going to be false, right? Let's lose the optional args. @@ +45,5 @@ > > +protected: > + void FireEvent(const nsAString& aType, bool aBubble, bool aCancelable); > + > + virtual void RootResultVal() Not your code, I know, but can we move these to the cpp file? @@ +59,5 @@ > + NS_DROP_JS_OBJECTS(this, DOMRequest); > + mRooted = false; > + } > + > + jsval mResult; I'd move these back to the top, just add a protected: block there. ::: dom/base/nsDOMClassInfo.cpp @@ +1582,5 @@ > > NS_DEFINE_CLASSINFO_DATA(IDBFactory, nsDOMGenericSH, > DOM_DEFAULT_SCRIPTABLE_FLAGS) > + NS_DEFINE_CLASSINFO_DATA_WITH_NAME(IDBFileHandle, FileHandle, nsDOMGenericSH, > + DOM_DEFAULT_SCRIPTABLE_FLAGS) Hm, FileHandle is an event target, right? @@ +1650,5 @@ > NS_DEFINE_CLASSINFO_DATA(DOMRequest, nsEventTargetSH, > EVENTTARGET_SCRIPTABLE_FLAGS) > + > + NS_DEFINE_CLASSINFO_DATA_WITH_NAME(DOMFileHandle, FileHandle, nsDOMGenericSH, > + DOM_DEFAULT_SCRIPTABLE_FLAGS) Here too. @@ +4293,5 @@ > DOM_CLASSINFO_MAP_BEGIN(IDBFactory, nsIIDBFactory) > DOM_CLASSINFO_MAP_ENTRY(nsIIDBFactory) > DOM_CLASSINFO_MAP_END > > + DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(IDBFileHandle, nsIDOMFileHandle) I'm unclear what this actually does. What's your intention here? I'm assuming that you don't want "FileHandle" to be available in the global scope? If so then I think this won't do it. ::: dom/base/nsDOMWindowUtils.cpp @@ +2179,5 @@ > + NS_ASSERTION(initializer, "what?"); > + > + jsval args[2] = { aBlobParts, aParameters }; > + > + rv = initializer->Initialize(nsnull, aCx, nsnull, aOptionalArgCount, args); I think we should figure out how to call this correctly. Need the window and the wrapped object. Passing null here seems like a footgun. @@ +2229,5 @@ > + > + if (!JSVAL_IS_PRIMITIVE(aFile)) { > + JSObject* obj = JSVAL_TO_OBJECT(aFile); > + > + nsISupports *nativeObj = Nit: * before the space. ::: dom/file/AsyncHelper.cpp @@ +15,5 @@ > + > +NS_IMPL_THREADSAFE_ISUPPORTS2(AsyncHelper, nsIRunnable, nsIRequest) > + > +AsyncHelper::AsyncHelper() > +: mFirstTime(true) mErrorCode is not initialized. And please inline in the header. @@ +20,5 @@ > +{ > +} > + > +nsresult > +AsyncHelper::Init(nsIFileStream* aStream) This can be a constructor arg, no need for the Init method. @@ +44,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +AsyncHelper::Run() So this function runs three times... You could use NS_NewRunnableMethod for the Start/StopRequest calls, and leave Run to the background thread? Might make things more sane. @@ +48,5 @@ > +AsyncHelper::Run() > +{ > + if (NS_IsMainThread()) { > + if (mFirstTime) { > + mObserver->OnStartRequest(this, nsnull); You should be passing mCtxt here. Also, from the IDL: "An exception thrown from onStartRequest has the side-effect of causing the request to be canceled." If you don't want to do that then you should loudly assert here so that we'll know if there's a problem. @@ +52,5 @@ > + mObserver->OnStartRequest(this, nsnull); > + mFirstTime = false; > + } > + else { > + mObserver->OnStopRequest(this, nsnull, mErrorCode); And pass mCtxt here. Also, I think you need to explicitly drop mObserver and mCtxt (and all other main-thread things) here, otherwise you may lose the race with another thread and release them on the wrong thread. You could have a ReleaseMainThreadObjects here like we do in IDB. @@ +58,5 @@ > + > + return NS_OK; > + } > + > + nsresult rv = NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL); Lose the 'nsresult rv', it'll just warn in non-debug builds. Just do: if (NS_FAILED(NS_Dispatch(...))) { NS_WARNING(...); } @@ +76,5 @@ > + > +NS_IMETHODIMP > +AsyncHelper::GetName(nsACString& aName) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; Add a MOZ_NOT_REACHED to all of these? ::: dom/file/AsyncHelper.h @@ +9,5 @@ > + > +#include "FileCommon.h" > + > +#include "nsIFileStream.h" > +#include "nsIRequest.h" Should #include "nsIRunnable.h" here too. @@ +29,5 @@ > + NS_DECL_NSIRUNNABLE > + NS_DECL_NSIREQUEST > + > + AsyncHelper(); > + virtual ~AsyncHelper() Constructor/destructor should be protected. ::: dom/file/nsIDOMFileHandle.idl @@ +34,5 @@ > + nsIDOMLockedFile > + open([optional /* "readonly" */] in DOMString mode); > + > + nsIDOMFileRequest > + getFile(); readonly attribute? @@ +38,5 @@ > + getFile(); > + > + [notxpcom] > + long long > + getFileId(); Here too. @@ +42,5 @@ > + getFileId(); > + > + [notxpcom] > + FileInfo > + getFileInfo(); And here. ::: dom/file/nsIDOMFileRequest.idl @@ +9,5 @@ > +interface nsIDOMEventListener; > +interface nsIDOMLockedFile; > + > +[scriptable, builtinclass, uuid(fe06b66e-fede-4d44-ab3a-403f60d6b593)] > +interface nsIDOMFileRequest : nsISupports This should inherit nsIDOMRequest, right? @@ +11,5 @@ > + > +[scriptable, builtinclass, uuid(fe06b66e-fede-4d44-ab3a-403f60d6b593)] > +interface nsIDOMFileRequest : nsISupports > +{ > + readonly attribute nsIDOMLockedFile lockedFile; How about just "file"? ::: dom/file/nsIDOMLockedFile.idl @@ +16,5 @@ > + boolean lastModified; > +}; > + > +[scriptable, builtinclass, uuid(63055eeb-cc19-468b-bafa-7b7961796340)] > +interface nsIDOMLockedFile : nsISupports Do we also need a 'delete' function? @@ +25,5 @@ > + readonly attribute DOMString mode; > + > + readonly attribute boolean open; > + > + attribute unsigned long long location; Is this supposed to be [noscript]? @@ +37,5 @@ > + nsIDOMFileRequest > + readAsArrayBuffer(in long size); > + > + nsIDOMFileRequest > + readAsText(in long size, [optional] in DOMString encoding); Nit: encoding on its own line @@ +48,5 @@ > + nsIDOMFileRequest > + append(in jsval value); > + > + nsIDOMFileRequest > + truncate(); Should this take a size argument (maybe optional)? @@ +51,5 @@ > + nsIDOMFileRequest > + truncate(); > + > + nsIDOMFileRequest > + sync(); "flush"? Something better here. ::: dom/file/nsIFileStorage.h @@ +10,5 @@ > +#include "nsISupports.h" > + > +#define NS_FILESTORAGE_IID \ > +{0xbba9c2ff, 0x85c9, 0x47c1, \ > + { 0xaf, 0xce, 0x0a, 0x7e, 0x6f, 0x21, 0x50, 0x95 } } Nit: Indent both lines @@ +12,5 @@ > +#define NS_FILESTORAGE_IID \ > +{0xbba9c2ff, 0x85c9, 0x47c1, \ > + { 0xaf, 0xce, 0x0a, 0x7e, 0x6f, 0x21, 0x50, 0x95 } } > + > +class nsPIDOMWindow; This looks unnecessary. @@ +14,5 @@ > + { 0xaf, 0xce, 0x0a, 0x7e, 0x6f, 0x21, 0x50, 0x95 } } > + > +class nsPIDOMWindow; > + > +class nsIFileStorage : public nsISupports { Nit: { on its own line @@ +16,5 @@ > +class nsPIDOMWindow; > + > +class nsIFileStorage : public nsISupports { > +public: > + Nit: Nix extra line. ::: dom/file/nsIFileStream.idl @@ +15,5 @@ > +[scriptable, uuid(ebbbb779-92a3-4b2a-b7cf-6efbe904c453)] > +interface nsIFileStream : nsISupports > +{ > + /** > + * @param file file to read from or strem to Nit: stream @@ +20,5 @@ > + * @param mode file open mode (see fopen documentation) > + * @param behaviorFlags flags specifying various behaviors of the class > + * (see enumerations in the class) > + */ > + void init(in nsIFile file, in AString mode, in long behaviorFlags); Nit: each arg on its own line. Also, let's just call the last param 'flags'? @@ +47,5 @@ > + * happen when this flag is not set would happen during the > + * first read. Also, the file is not locked when Init is called, > + * so it might be deleted before we try to read from it. > + */ > + const long DEFER_OPEN = 1<<0; Nit: 1 << 0. Also, I usually like to see this above the method it's used in, and with a name that indicates which parameter it goes with. So if we rename the param to 'flags' then I would want this to be FLAGS_DEFER_OPEN. @@ +52,5 @@ > + > + /** > + * Get file size; > + */ > + long long getSize(); readonly attribute @@ +57,5 @@ > + > + /** > + * Get file last modified time. > + */ > + long long getLastModified(); here too. @@ +62,5 @@ > + > + /** > + * Flush and sync. > + */ > + void sync(); Same comment re: better name here. ::: dom/indexedDB/IndexedDatabaseManager.h @@ +318,5 @@ > class WaitForTransactionsToFinishRunnable : public nsIRunnable > { > public: > + WaitForTransactionsToFinishRunnable(SynchronizedOp* aOp, > + PRUint32 aCountdown = 1) Let's do this without the default arg. @@ +342,5 @@ > + public: > + WaitForLockedFilesToFinishRunnable() > + : mStillBusy(true) > + { > + } Nit: { } @@ +347,5 @@ > + > + NS_DECL_ISUPPORTS > + NS_DECL_NSIRUNNABLE > + > + bool mStillBusy; Should be private. ::: dom/indexedDB/nsIIDBFileHandle.idl @@ +10,5 @@ > + > +[scriptable, builtinclass, uuid(7b05f6bb-26b0-4c12-a9a1-e31dd933deb8)] > +interface nsIIDBFileHandle : nsISupports > +{ > + readonly attribute nsIIDBDatabase db; "database"? ::: dom/indexedDB/test/helpers.js @@ +14,4 @@ > { > allowIndexedDB(); > + if (unlimitedQuota) { > + denyUnlimitedQuota(); Er... backwards? Need to rename unlimitedQuota. ::: dom/interfaces/base/nsIDOMWindowUtils.idl @@ +1034,5 @@ > boolean checkAndClearPaintedState(in nsIDOMElement aElement); > > + [optional_argc, implicit_jscontext] > + nsIDOMFile getFile(in DOMString aName, [optional] in jsval aBlobParts, > + [optional] in jsval aParameters); Nit: These need comments @@ +1044,2 @@ > /* > + * Get internal id of the stored blob, file or file handle. Nit: /** ::: js/xpconnect/src/dom_quickstubs.qsconf @@ +472,5 @@ > 'nsIDOMDOMError.*', > + > + # dom/file > + 'nsIDOMFileHandle.*', > + '-nsIDOMFileHandle.getFileId', Hm, do you have to do this for unscriptable methods? ::: xpcom/io/nsStreamUtils.cpp @@ +768,5 @@ > + PRUint32 offset, > + PRUint32 count, > + PRUint32 *countRead) > +{ > + char* fromBuf = static_cast<char*>(closure); const char? ::: xpcom/io/nsStreamUtils.h @@ +213,5 @@ > const char *aFromSegment, PRUint32 aToOffset, > PRUint32 aCount, PRUint32 *aWriteCount); > > +extern NS_METHOD > +NS_CopySegmentToBuffer(nsIOutputStream *aOutputStream, void *aClosure, Need comment. ::: xpcom/io/nsStringStream.cpp @@ +438,5 @@ > +{ > + NS_PRECONDITION(aStreamResult, "null out ptr"); > + > + nsStringInputStream* stream = new nsStringInputStream(); > + if (! stream) Nit: lose extra space @@ +441,5 @@ > + nsStringInputStream* stream = new nsStringInputStream(); > + if (! stream) > + return NS_ERROR_OUT_OF_MEMORY; > + > + NS_ADDREF(stream); Boo. I know this is old code, but let's use a refptr and forget() here. ::: xpcom/io/nsStringStream.h @@ +105,5 @@ > NS_NewCStringInputStream(nsIInputStream** aStreamResult, > const nsACString& aStringToRead); > > +extern nsresult > +NS_NewCStringInputStream(nsIInputStream** aStreamResult, Needs a comment (does it convert to UTF8, treat as raw buffer, etc.)

(In reply to ben turner [:bent] from comment #27) > Comment on attachment 617475 [details] [diff] [review] > patch v0.4 > > Review of attachment 617475 [details] [diff] [review]: > ----------------------------------------------------------------- > > Not done, just giving a snapshot. > > ::: content/base/public/nsDOMFile.h > @@ +76,5 @@ > > public: > > typedef mozilla::dom::indexedDB::FileInfo FileInfo; > > > > + nsDOMFile(const nsAString& aName, const nsAString& aContentType, > > + PRUint64 aLength) > > This is always going to be inherited now, right? Let's make the > constructors/destructor protected. done > > @@ +151,5 @@ > > // Protected by IndexedDatabaseManager::FileMutex() > > nsTArray<nsRefPtr<FileInfo> > mFileInfos; > > }; > > > > +class nsDOMFileBase : public nsDOMFile > > It's weird that the "Base" class is not actually the base class. I think we > should reverse this. done > > @@ +158,5 @@ > > + nsDOMFileBase(const nsAString& aName, const nsAString& aContentType, > > + PRUint64 aLength) > > + : nsDOMFile(aName, aContentType, aLength) > > + { > > + } > > Nit: { } is fine, here and below. done > > ::: content/base/src/nsContentUtils.cpp > @@ +350,5 @@ > > NS_DECL_NSICHANNELEVENTSINK > > NS_DECL_NSIINTERFACEREQUESTOR > > }; > > > > +class nsCharsetDetectionObserver : public nsICharsetDetectionObserver > > This could just be in an anonymous namespace, without the ns prefix. done > > @@ +3554,5 @@ > > + nsICharsetDetectionObserver) > > + > > +NS_IMETHODIMP > > +nsCharsetDetectionObserver::Notify(const char *aCharset, > > + nsDetectionConfident aConf) > > You could just inline this. done > > @@ +3567,5 @@ > > + nsACString &aCharset) > > +{ > > + // First try the universal charset detector > > + nsCOMPtr<nsICharsetDetector> detector > > + = do_CreateInstance(NS_CHARSET_DETECTOR_CONTRACTID_BASE > > Nit: = on prev line done > > @@ +3575,5 @@ > > + const nsAdoptingCString& detectorName = > > + Preferences::GetLocalizedCString("intl.charset.detector"); > > + if (!detectorName.IsEmpty()) { > > + nsCAutoString detectorContractID; > > + detectorContractID.AssignLiteral(NS_CHARSET_DETECTOR_CONTRACTID_BASE); > > Nit: You can pass this in the constructor. done > > @@ +3585,5 @@ > > + nsresult rv; > > + > > + // The charset detector doesn't work for empty (null) aData. Testing > > + // aDataLen instead of aData so that we catch potential errors. > > + if (detector && aDataLen != 0) { > > Nit: (detector && aDataLen) done > > @@ +3589,5 @@ > > + if (detector && aDataLen != 0) { > > + nsRefPtr<nsCharsetDetectionObserver> observer = > > + new nsCharsetDetectionObserver(); > > + > > + detector->Init(observer); > > Looks like this can fail. fixed > > ::: content/base/src/nsDOMFile.cpp > @@ +340,5 @@ > > // copied again. That's why we have to ignore the first file info. > > + // Snapshots are handled in a similar way (they have to be copied). > > + PRUint32 startIndex = 0; > > + if (IsStoredFile() && (!IsWholeFile() || IsSnapshot())) { > > + startIndex = 1; > > Nit: Avoid the double assignment. fixed > > @@ +430,5 @@ > > + > > +//////////////////////////////////////////////////////////////////////////// > > +// nsDOMFileBaseCC implementation > > + > > +NS_IMPL_CYCLE_COLLECTION_CLASS(nsDOMFileBaseCC) > > NS_IMPL_CYCLE_COLLECTION_0 for these fixed > > @@ +440,5 @@ > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END > > + > > +// XXX > > +//DOMCI_DATA(File, nsDOMFileBaseCC) > > +//DOMCI_DATA(Blob, nsDOMFileBaseCC) > > I think it's safe to remove these. done > > ::: dom/base/DOMRequest.cpp > @@ +104,5 @@ > > { > > NS_ABORT_IF_FALSE(!mDone, "Already fired success/error"); > > > > mDone = true; > > + if (!JSVAL_IS_VOID(aResult)) { > > I think you want JSVAL_IS_GCTHING done > > @@ +126,5 @@ > > > > void > > +DOMRequest::FireError(nsresult aError, bool aBubble, bool aCancelable) > > +{ > > + NS_ABORT_IF_FALSE(!mDone, "Already fired success/error"); > > Also assert !mError convereted the !mDone check to NS_ASSERTION() and added some more also in FireSuccess() > > ::: dom/base/DOMRequest.h > @@ +29,5 @@ > > nsDOMEventTargetHelper) > > > > void FireSuccess(jsval aResult); > > + void FireError(const nsAString& aError, > > + bool aBubble = false, bool aCancelable = false); > > These are always going to be false, right? Let's lose the optional args. fixed > > @@ +45,5 @@ > > > > +protected: > > + void FireEvent(const nsAString& aType, bool aBubble, bool aCancelable); > > + > > + virtual void RootResultVal() > > Not your code, I know, but can we move these to the cpp file? sure > > @@ +59,5 @@ > > + NS_DROP_JS_OBJECTS(this, DOMRequest); > > + mRooted = false; > > + } > > + > > + jsval mResult; > > I'd move these back to the top, just add a protected: block there. done > > ::: dom/base/nsDOMClassInfo.cpp > @@ +1582,5 @@ > > > > NS_DEFINE_CLASSINFO_DATA(IDBFactory, nsDOMGenericSH, > > DOM_DEFAULT_SCRIPTABLE_FLAGS) > > + NS_DEFINE_CLASSINFO_DATA_WITH_NAME(IDBFileHandle, FileHandle, nsDOMGenericSH, > > + DOM_DEFAULT_SCRIPTABLE_FLAGS) > > Hm, FileHandle is an event target, right? ah, forgot to do it for LockedFile too, fixed > > @@ +1650,5 @@ > > NS_DEFINE_CLASSINFO_DATA(DOMRequest, nsEventTargetSH, > > EVENTTARGET_SCRIPTABLE_FLAGS) > > + > > + NS_DEFINE_CLASSINFO_DATA_WITH_NAME(DOMFileHandle, FileHandle, nsDOMGenericSH, > > + DOM_DEFAULT_SCRIPTABLE_FLAGS) > > Here too. fixed > > @@ +4293,5 @@ > > DOM_CLASSINFO_MAP_BEGIN(IDBFactory, nsIIDBFactory) > > DOM_CLASSINFO_MAP_ENTRY(nsIIDBFactory) > > DOM_CLASSINFO_MAP_END > > > > + DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(IDBFileHandle, nsIDOMFileHandle) > > I'm unclear what this actually does. What's your intention here? I'm > assuming that you don't want "FileHandle" to be available in the global > scope? If so then I think this won't do it. Well, the _NO_CLASS_IF fixed an assertion that I was getting (in nsDOMClassInfo.cpp) However, it seems it now works even w/o the _NO_CLASS_IF > > ::: dom/base/nsDOMWindowUtils.cpp > @@ +2179,5 @@ > > + NS_ASSERTION(initializer, "what?"); > > + > > + jsval args[2] = { aBlobParts, aParameters }; > > + > > + rv = initializer->Initialize(nsnull, aCx, nsnull, aOptionalArgCount, args); > > I think we should figure out how to call this correctly. Need the window and > the wrapped object. Passing null here seems like a footgun. the same is done in mozJSComponentLoader.cpp, static JSBool File(JSContext *cx, unsigned argc, jsval *vp) ... rv = initializer->Initialize(nsnull, cx, nsnull, argc, JS_ARGV(cx, vp)) I know, there's no window object in JS components Anyway, the 1st and 3rd arguments is completely ignored in: NS_IMETHODIMP nsDOMMultipartFile::Initialize(nsISupports* aOwner, JSContext* aCx, JSObject* aObj, PRUint32 aArgc, jsval* aArgv) { return InitInternal(aCx, aArgc, aArgv, GetXPConnectNative); } > > @@ +2229,5 @@ > > + > > + if (!JSVAL_IS_PRIMITIVE(aFile)) { > > + JSObject* obj = JSVAL_TO_OBJECT(aFile); > > + > > + nsISupports *nativeObj = > > Nit: * before the space. fixed > > ::: dom/file/AsyncHelper.cpp > @@ +15,5 @@ > > + > > +NS_IMPL_THREADSAFE_ISUPPORTS2(AsyncHelper, nsIRunnable, nsIRequest) > > + > > +AsyncHelper::AsyncHelper() > > +: mFirstTime(true) > > mErrorCode is not initialized. fixed (defaults to NS_OK) > > And please inline in the header. done > > @@ +20,5 @@ > > +{ > > +} > > + > > +nsresult > > +AsyncHelper::Init(nsIFileStream* aStream) > > This can be a constructor arg, no need for the Init method. well, the Init() and AsyncFoo() seems like a convention in the necko but I'm ok with moving it to the ctor done > > @@ +44,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +AsyncHelper::Run() > > So this function runs three times... You could use NS_NewRunnableMethod for > the Start/StopRequest calls, and leave Run to the background thread? Might > make things more sane. actually, I forgot that there's nsRequestObserverProxy fixed > > @@ +48,5 @@ > > +AsyncHelper::Run() > > +{ > > + if (NS_IsMainThread()) { > > + if (mFirstTime) { > > + mObserver->OnStartRequest(this, nsnull); > > You should be passing mCtxt here. fixed > > Also, from the IDL: > > "An exception thrown from onStartRequest has the side-effect of causing > the request to be canceled." > > If you don't want to do that then you should loudly assert here so that > we'll know if there's a problem. fixed by using the nsRequestObserverProxy > > @@ +52,5 @@ > > + mObserver->OnStartRequest(this, nsnull); > > + mFirstTime = false; > > + } > > + else { > > + mObserver->OnStopRequest(this, nsnull, mErrorCode); > > And pass mCtxt here. fixed > > Also, I think you need to explicitly drop mObserver and mCtxt (and all other > main-thread things) here, otherwise you may lose the race with another > thread and release them on the wrong thread. You could have a > ReleaseMainThreadObjects here like we do in IDB. mStream is threadsafe (FileStream.cpp in dom/file and dom/indexedDB) mObserver is threadsafe too (FileHelper.cpp) and mCtxt is not used at all not sure if it's worth to dispatch a new runnable and call ReleaseMainThreadObjects() > > @@ +58,5 @@ > > + > > + return NS_OK; > > + } > > + > > + nsresult rv = NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL); > > Lose the 'nsresult rv', it'll just warn in non-debug builds. Just do: not needed anymore > > if (NS_FAILED(NS_Dispatch(...))) { > NS_WARNING(...); > } > > @@ +76,5 @@ > > + > > +NS_IMETHODIMP > > +AsyncHelper::GetName(nsACString& aName) > > +{ > > + return NS_ERROR_NOT_IMPLEMENTED; > > Add a MOZ_NOT_REACHED to all of these? done > > ::: dom/file/AsyncHelper.h > @@ +9,5 @@ > > + > > +#include "FileCommon.h" > > + > > +#include "nsIFileStream.h" > > +#include "nsIRequest.h" > > Should #include "nsIRunnable.h" here too. done > > @@ +29,5 @@ > > + NS_DECL_NSIRUNNABLE > > + NS_DECL_NSIREQUEST > > + > > + AsyncHelper(); > > + virtual ~AsyncHelper() > > Constructor/destructor should be protected. done > > ::: dom/file/nsIDOMFileHandle.idl > @@ +34,5 @@ > > + nsIDOMLockedFile > > + open([optional /* "readonly" */] in DOMString mode); > > + > > + nsIDOMFileRequest > > + getFile(); > > readonly attribute? hmm, doesn't look good to me would be ok, if we returned the file synchronously > > @@ +38,5 @@ > > + getFile(); > > + > > + [notxpcom] > > + long long > > + getFileId(); > > Here too. [notxpcom] readonly attribute long long fileId; translates into: /* [notxpcom] readonly attribute long long fileId; */ NS_IMETHOD GetFileId(PRInt64 *aFileId) = 0; but I need: [notxpcom] long long getFileId(); which translates into: /* [notxpcom] long long getFileId (); */ NS_IMETHOD_(PRInt64) GetFileId(void) = 0; > > @@ +42,5 @@ > > + getFileId(); > > + > > + [notxpcom] > > + FileInfo > > + getFileInfo(); > > And here. see my comment above > > ::: dom/file/nsIDOMFileRequest.idl > @@ +9,5 @@ > > +interface nsIDOMEventListener; > > +interface nsIDOMLockedFile; > > + > > +[scriptable, builtinclass, uuid(fe06b66e-fede-4d44-ab3a-403f60d6b593)] > > +interface nsIDOMFileRequest : nsISupports > > This should inherit nsIDOMRequest, right? yeah, fixed I had to add a new macro NS_FORWARD_NSIDOMEVENTTARGET_NOPREHANDLEEVENT to nsDOMEventTargetHelper.h > > @@ +11,5 @@ > > + > > +[scriptable, builtinclass, uuid(fe06b66e-fede-4d44-ab3a-403f60d6b593)] > > +interface nsIDOMFileRequest : nsISupports > > +{ > > + readonly attribute nsIDOMLockedFile lockedFile; > > How about just "file"? well, I would rather keep the prefix if possible > > ::: dom/file/nsIDOMLockedFile.idl > @@ +16,5 @@ > > + boolean lastModified; > > +}; > > + > > +[scriptable, builtinclass, uuid(63055eeb-cc19-468b-bafa-7b7961796340)] > > +interface nsIDOMLockedFile : nsISupports > > Do we also need a 'delete' function? not sure what you mean here the file will get deleted automatically (unless it was added to an objectstore) that's actually very neat feature > > @@ +25,5 @@ > > + readonly attribute DOMString mode; > > + > > + readonly attribute boolean open; > > + > > + attribute unsigned long long location; > > Is this supposed to be [noscript]? I talked to Jonas, mode and location should definitely not be [noscript] and we also think that we can expose the internal method IsOpen() via an attribute, the .open doesn't sound right I renamed it to .active > > @@ +37,5 @@ > > + nsIDOMFileRequest > > + readAsArrayBuffer(in long size); > > + > > + nsIDOMFileRequest > > + readAsText(in long size, [optional] in DOMString encoding); > > Nit: encoding on its own line fixed > > @@ +48,5 @@ > > + nsIDOMFileRequest > > + append(in jsval value); > > + > > + nsIDOMFileRequest > > + truncate(); > > Should this take a size argument (maybe optional)? no, the current location will be used to truncate the file to specific size > > @@ +51,5 @@ > > + nsIDOMFileRequest > > + truncate(); > > + > > + nsIDOMFileRequest > > + sync(); > > "flush"? Something better here. commit ? > > ::: dom/file/nsIFileStorage.h > @@ +10,5 @@ > > +#include "nsISupports.h" > > + > > +#define NS_FILESTORAGE_IID \ > > +{0xbba9c2ff, 0x85c9, 0x47c1, \ > > + { 0xaf, 0xce, 0x0a, 0x7e, 0x6f, 0x21, 0x50, 0x95 } } > > Nit: Indent both lines fixed > > @@ +12,5 @@ > > +#define NS_FILESTORAGE_IID \ > > +{0xbba9c2ff, 0x85c9, 0x47c1, \ > > + { 0xaf, 0xce, 0x0a, 0x7e, 0x6f, 0x21, 0x50, 0x95 } } > > + > > +class nsPIDOMWindow; > > This looks unnecessary. yeah, removed > > @@ +14,5 @@ > > + { 0xaf, 0xce, 0x0a, 0x7e, 0x6f, 0x21, 0x50, 0x95 } } > > + > > +class nsPIDOMWindow; > > + > > +class nsIFileStorage : public nsISupports { > > Nit: { on its own line done > > @@ +16,5 @@ > > +class nsPIDOMWindow; > > + > > +class nsIFileStorage : public nsISupports { > > +public: > > + > > Nit: Nix extra line. removed > > ::: dom/file/nsIFileStream.idl > @@ +15,5 @@ > > +[scriptable, uuid(ebbbb779-92a3-4b2a-b7cf-6efbe904c453)] > > +interface nsIFileStream : nsISupports > > +{ > > + /** > > + * @param file file to read from or strem to > > Nit: stream fixed the typo > > @@ +20,5 @@ > > + * @param mode file open mode (see fopen documentation) > > + * @param behaviorFlags flags specifying various behaviors of the class > > + * (see enumerations in the class) > > + */ > > + void init(in nsIFile file, in AString mode, in long behaviorFlags); > > Nit: each arg on its own line. Also, let's just call the last param 'flags'? ok, fixed > > @@ +47,5 @@ > > + * happen when this flag is not set would happen during the > > + * first read. Also, the file is not locked when Init is called, > > + * so it might be deleted before we try to read from it. > > + */ > > + const long DEFER_OPEN = 1<<0; > > Nit: 1 << 0. fixed > > Also, I usually like to see this above the method it's used in, and with a > name that indicates which parameter it goes with. So if we rename the param > to 'flags' then I would want this to be FLAGS_DEFER_OPEN. ok, fixed > > @@ +52,5 @@ > > + > > + /** > > + * Get file size; > > + */ > > + long long getSize(); > > readonly attribute done > > @@ +57,5 @@ > > + > > + /** > > + * Get file last modified time. > > + */ > > + long long getLastModified(); > > here too. done > > @@ +62,5 @@ > > + > > + /** > > + * Flush and sync. > > + */ > > + void sync(); > > Same comment re: better name here. I renamed it to Commit() and added a better comment > > ::: dom/indexedDB/IndexedDatabaseManager.h > @@ +318,5 @@ > > class WaitForTransactionsToFinishRunnable : public nsIRunnable > > { > > public: > > + WaitForTransactionsToFinishRunnable(SynchronizedOp* aOp, > > + PRUint32 aCountdown = 1) > > Let's do this without the default arg. ok, done > > @@ +342,5 @@ > > + public: > > + WaitForLockedFilesToFinishRunnable() > > + : mStillBusy(true) > > + { > > + } > > Nit: { } fixed > > @@ +347,5 @@ > > + > > + NS_DECL_ISUPPORTS > > + NS_DECL_NSIRUNNABLE > > + > > + bool mStillBusy; > > Should be private. fixed > > ::: dom/indexedDB/nsIIDBFileHandle.idl > @@ +10,5 @@ > > + > > +[scriptable, builtinclass, uuid(7b05f6bb-26b0-4c12-a9a1-e31dd933deb8)] > > +interface nsIIDBFileHandle : nsISupports > > +{ > > + readonly attribute nsIIDBDatabase db; > > "database"? ok > > ::: dom/indexedDB/test/helpers.js > @@ +14,4 @@ > > { > > allowIndexedDB(); > > + if (unlimitedQuota) { > > + denyUnlimitedQuota(); > > Er... backwards? Need to rename unlimitedQuota. yeah, renamed unlimitedQuota to limitedQuota > > ::: dom/interfaces/base/nsIDOMWindowUtils.idl > @@ +1034,5 @@ > > boolean checkAndClearPaintedState(in nsIDOMElement aElement); > > > > + [optional_argc, implicit_jscontext] > > + nsIDOMFile getFile(in DOMString aName, [optional] in jsval aBlobParts, > > + [optional] in jsval aParameters); > > Nit: These need comments done > > @@ +1044,2 @@ > > /* > > + * Get internal id of the stored blob, file or file handle. > > Nit: /** fixed > > ::: js/xpconnect/src/dom_quickstubs.qsconf > @@ +472,5 @@ > > 'nsIDOMDOMError.*', > > + > > + # dom/file > > + 'nsIDOMFileHandle.*', > > + '-nsIDOMFileHandle.getFileId', > > Hm, do you have to do this for unscriptable methods? no, only for [notxpcom] this is the error message: Method nsIDOMFileHandle.getFileId: notxpcom methods are not supported. make[1]: *** [dom_quickstubs.cpp] Error 1 > > ::: xpcom/io/nsStreamUtils.cpp > @@ +768,5 @@ > > + PRUint32 offset, > > + PRUint32 count, > > + PRUint32 *countRead) > > +{ > > + char* fromBuf = static_cast<char*>(closure); > > const char? ok > > ::: xpcom/io/nsStreamUtils.h > @@ +213,5 @@ > > const char *aFromSegment, PRUint32 aToOffset, > > PRUint32 aCount, PRUint32 *aWriteCount); > > > > +extern NS_METHOD > > +NS_CopySegmentToBuffer(nsIOutputStream *aOutputStream, void *aClosure, > > Need comment. done > > ::: xpcom/io/nsStringStream.cpp > @@ +438,5 @@ > > +{ > > + NS_PRECONDITION(aStreamResult, "null out ptr"); > > + > > + nsStringInputStream* stream = new nsStringInputStream(); > > + if (! stream) > > Nit: lose extra space ok > > @@ +441,5 @@ > > + nsStringInputStream* stream = new nsStringInputStream(); > > + if (! stream) > > + return NS_ERROR_OUT_OF_MEMORY; > > + > > + NS_ADDREF(stream); > > Boo. I know this is old code, but let's use a refptr and forget() here. ok, fixed > > ::: xpcom/io/nsStringStream.h > @@ +105,5 @@ > > NS_NewCStringInputStream(nsIInputStream** aStreamResult, > > const nsACString& aStringToRead); > > > > +extern nsresult > > +NS_NewCStringInputStream(nsIInputStream** aStreamResult, > > Needs a comment (does it convert to UTF8, treat as raw buffer, etc.) actually, we don't need this removed, along with SetUnicodeData()

For what it's worth, I'm not a fan of "commit" since that's often used for databases and is paired with a "rollback" function. I'm fine with either sync() or flush() as I believe both are commonly used in file APIs for the functionality that we have here.

(In reply to Jonas Sicking (:sicking) from comment #31) > For what it's worth, I'm not a fan of "commit" since that's often used for > databases and is paired with a "rollback" function. > > I'm fine with either sync() or flush() as I believe both are commonly used > in file APIs for the functionality that we have here. so, I propose: nsIDOMLockedFile::Flush() nsIOutputStream::Flush() - this method will call fflush() and fsync() nsIFileStream::FlushBuffers() - this method will only call fflush(), needed only internally to flush buffers on the same thread on which wrote some data

Comment on attachment 617475 [details] [diff] [review] patch v0.4 Review of attachment 617475 [details] [diff] [review]: ----------------------------------------------------------------- Still not quite done, just have the IndexedDB changes left. ::: dom/file/DOMFile.cpp @@ +39,5 @@ > + return NS_OK; > +} > + > +already_AddRefed<nsIDOMBlob> > +DOMFile::CreateSlice(PRUint64 aStart, PRUint64 aLength, Can you asset main thread here? @@ +48,5 @@ > + return t.forget(); > +} > + > +NS_IMETHODIMP > +DOMFile::GetMozFullPathInternal(nsAString &aFilename) Can you assert main thread here? ::: dom/file/DOMFile.h @@ +14,5 @@ > +#include "LockedFile.h" > + > +BEGIN_FILE_NAMESPACE > + > +class DOMFile : public nsDOMFileBaseCC Maybe just File? @@ +35,5 @@ > + > + // Create as a stored file > + DOMFile(const nsAString& aName, const nsAString& aContentType, > + PRUint64 aLength, nsIFile* aFile, FileInfo* aFileInfo, > + LockedFile* aLockedFile) Nit: reorder last two args so it's the same as the other. @@ +45,5 @@ > + NS_ASSERTION(mLockedFile, "Null locked file!"); > + mFileInfos.AppendElement(aFileInfo); > + } > + > + virtual ~DOMFile() protected. @@ +49,5 @@ > + virtual ~DOMFile() > + { } > + > + // Overrides > + NS_IMETHOD GetMozFullPathInternal(nsAString& aFullPath); Nit: Name on next line, and for the next one. @@ +55,5 @@ > + > +protected: > + // Create slice > + DOMFile(const DOMFile* aOther, PRUint64 aStart, PRUint64 aLength, > + const nsAString& aContentType) This one should go in the cpp I think. @@ +67,5 @@ > + if (mStoredFile) { > + FileInfo* fileInfo; > + > + if (!mozilla::dom::indexedDB::IndexedDatabaseManager::IsClosed()) { > + mozilla::dom::indexedDB::IndexedDatabaseManager::FileMutex().Lock(); You should use an autolock @@ +87,5 @@ > + virtual already_AddRefed<nsIDOMBlob> > + CreateSlice(PRUint64 aStart, PRUint64 aLength, > + const nsAString& aContentType); > + > + virtual bool IsStoredFile() Nit: IsStoredFile on next line. And the others below. @@ +105,5 @@ > + > +private: > + nsCOMPtr<nsIFile> mFile; > + bool mWholeFile; > + bool mStoredFile; Nit: In general i'd put bools at the end so they pack nicely. ::: dom/file/DOMFileHandle.h @@ +17,5 @@ > +{ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DOMFileHandle, FileHandle) You could use the _NO_UNLINK version of this macro, I guess. Not convinced it makes things more clear, so whatever you'd like. @@ +21,5 @@ > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DOMFileHandle, FileHandle) > + > +#if 0 > + static already_AddRefed<DOMFileHandle> > + Create(nsDOMDeviceStorage* aDeviceStorage, Eh? ::: dom/file/FileHandle.cpp @@ +37,5 @@ > + void ReleaseMainThreadObjects() > + { > + mFileHandle = nsnull; > + > + FileHelper::ReleaseMainThreadObjects(); Probably want to call MetadataHelper::ReleaseMainThreadObjects. Even if that doesn't exist it might some day in the future. @@ +58,5 @@ > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(FileHandle, > + nsDOMEventTargetHelper) > + // Don't unlink mFileStorage! Is this important? @@ +105,5 @@ > + > + LockedFile::Mode mode = LockedFile::READ_ONLY; > + if (aOptionalArgCount) { > + if (aMode.EqualsLiteral("readwrite")) { > + mode = LockedFile::READ_WRITE; Nit: avoid double assign. @@ +129,5 @@ > + LockedFile::Create(this, LockedFile::READ_ONLY, LockedFile::PARALLEL); > + NS_ENSURE_TRUE(lockedFile, NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR); > + > + nsRefPtr<FileRequest> fileRequest = > + FileRequest::Create(GetOwner(), lockedFile); nsDOMEventTargetHelper no longer holds a strong ref to its owner, so you should null-check. @@ +147,5 @@ > + > +NS_IMETHODIMP_(PRInt64) > +FileHandle::GetFileId() > +{ > + return -1; Want to warn here? @@ +153,5 @@ > + > +NS_IMETHODIMP_(mozilla::dom::indexedDB::FileInfo*) > +FileHandle::GetFileInfo() > +{ > + return nsnull; And here? ::: dom/file/FileHandle.h @@ +33,5 @@ > +{ > + friend class FileService; > + friend class LockedFile; > + friend class FinishHelper; > + friend class FileHelper; I think Windows (MSVC) barfs on these. Need to also forward declare them above. @@ +39,5 @@ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_NSIDOMFILEHANDLE > + > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED( Don't think you need the SCRIPT_HOLDER thing here, or the TRACE macros in the cpp. You're inheriting, but not extending, so just use the regular INHERITED macros. @@ +43,5 @@ > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED( > + FileHandle, > + nsDOMEventTargetHelper) > + > + const nsAString& Name() const Nit: Function names on following line. ::: dom/file/FileHelper.cpp @@ +32,5 @@ > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > +} > + > +FileHelper::~FileHelper() Looks like you don't have any code dealing with mListener or mRequest? @@ +34,5 @@ > +} > + > +FileHelper::~FileHelper() > +{ > + if (!NS_IsMainThread()) { Hm... This really shouldn't happen, right? ReleaseMainThreadObjects should be all you need to keep these objects from releasing on the wrong thread. Maybe put a warning or assertion in here? @@ +45,5 @@ > + FileRequest* fileRequest; > + mFileRequest.forget(&fileRequest); > + > + nsCOMPtr<nsIThread> mainThread; > + NS_GetMainThread(getter_AddRefs(mainThread)); No need to do this if all the pointers are null. @@ +65,5 @@ > + } > +} > + > +NS_IMPL_THREADSAFE_ISUPPORTS1(FileHelper, > + nsIRequestObserver) Nit: this can probably fit on one line @@ +70,5 @@ > + > +nsresult > +FileHelper::Enqueue() > +{ > + FileService* service = FileService::GetOrCreate(); GetOrCreate could fail, right? @@ +99,5 @@ > + > + rv = DoAsyncRun(stream); > + NS_ENSURE_SUCCESS(rv, rv); > + > + mListener = aListener; Hm, do you want to have this set before calling DoAsyncRun? You'd need to null it back out if DoAsyncRun fails, but might be nicer for subclasses. @@ +114,5 @@ > +NS_IMETHODIMP > +FileHelper::OnStopRequest(nsIRequest* aRequest, nsISupports* aCtxt, > + nsresult aStatus) > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); Add this to other methods too? Helps make code readable and prevents accidental bugs. @@ +131,5 @@ > +{ > + if (mLockedFile->IsAborted()) { > + NS_ASSERTION(mRequest, "Should have a request!\n"); > + > + nsresult rv = mRequest->Cancel(NS_ERROR_NOT_AVAILABLE); Usually we use NS_BINDING_ABORTED to signal aborts. @@ +147,5 @@ > + > +void > +FileHelper::OnStreamClose() > +{ > + Finish(); Inline @@ +153,5 @@ > + > +void > +FileHelper::OnStreamDestroy() > +{ > + Finish(); Inline @@ +176,5 @@ > + return NS_OK; > +} > + > +void > +FileHelper::ReleaseMainThreadObjects() What about mListener and mRequest? Seems better to put these all in one place. @@ +215,5 @@ > + gCurrentLockedFile = oldLockedFile; > + > + mLockedFile->OnRequestFinished(); > + > + mListener->OnFileHelperComplete(this); So... There's three things happening here: mFileRequest->NotifyHelperCompleted(this); mLockedFile->OnRequestFinished(); mListener->OnFileHelperComplete(this); Is there any way that this could be simplified? @@ +223,5 @@ > + > + NS_ASSERTION(!(mFileStorage || mLockedFile || mFileRequest), "Subclass " > + "didn't call FileHelper::ReleaseMainThreadObjects!"); > + > + mFinished = true; It's not possible for anything in this function to spin an event loop or otherwise call back into Finish() is it? If it's possible then you should move this to right after you check it above. ::: dom/file/FileHelper.h @@ +36,5 @@ > + */ > +class FileHelper : public nsIRequestObserver > +{ > + friend class FileRequest; > + friend class FileOutputStreamWrapper; Forward declare ::: dom/file/FileRequest.cpp @@ +54,5 @@ > + > +void > +FileRequest::OnProgress(PRUint64 aProgress, PRUint64 aProgressMax) > +{ > + FireProgressEvent(aProgress, aProgressMax); Inline. @@ +81,5 @@ > + JSAutoRequest ar(cx); > + JSAutoEnterCompartment ac; > + if (ac.enter(cx, global)) { > + jsval result; > + aFileHelper->GetSuccessResult(cx, &result); This can fail, in which case you probably want to fire an error instead @@ +145,5 @@ > + nsRefPtr<nsDOMProgressEvent> event = new nsDOMProgressEvent(nsnull, nsnull); > + nsresult rv = event->InitProgressEvent(NS_LITERAL_STRING("progress"), > + false, false, false, aLoaded, aTotal); > + if (NS_FAILED(rv)) { > + return; I'd say you still want a warning, so NS_ENSURE_SUCCESS(rv,) will do what you want. @@ +150,5 @@ > + } > + > + rv = event->SetTrusted(true); > + if (NS_FAILED(rv)) { > + return; Here too. @@ +154,5 @@ > + return; > + } > + > + bool dummy; > + DispatchEvent(static_cast<nsIDOMProgressEvent*>(event), &dummy); And you probably want it here too. ::: dom/file/FileRequest.h @@ +24,5 @@ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_NSIDOMFILEREQUEST > + NS_FORWARD_NSIDOMDOMREQUEST(DOMRequest::) > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(FileRequest, You don't need the SCRIPT_HOLDER stuff @@ +44,5 @@ > + ~FileRequest(); > + > + void FireProgressEvent(PRUint64 aLoaded, PRUint64 aTotal); > + > + virtual void RootResultVal() Since this is virtual I'd un-inline it ::: dom/file/FileService.cpp @@ +56,5 @@ > + NS_ENSURE_SUCCESS(rv, nsnull); > + > + gInstance = service.forget(); > + > + ClearOnShutdown(&gInstance); I'm not really happy using this... It comes after the "xpcom-shutdown" and "xpcom-shutdown-threads" notification, and it will release tons of things (runnables, other xpcom objects, maybe dom objects?). Those released objects may expect XPCOM to still be around, or at the very least to continue processing events. Plus, before deleting I'd hope we could call WaitForAllStoragesToComplete or something. If you think it's impossible for that to happen then we should at the very least assert loudly. I think leaking until late in shutdown is totally possible though. Seems like we should use the observer service here. @@ +112,5 @@ > + > + if (!(modeIsWrite ? lockedForWriting : lockedForReading)) { > + modeIsWrite ? fileStorageInfo->LockFileForWriting(fileName) > + : fileStorageInfo->LockFileForReading(fileName); > + } This is a little too cute for my taste. Can you expand this? @@ +147,5 @@ > + > + fileStorageInfo->RemoveLockedFileQueueFor(aLockedFile); > + > + if (!fileStorageInfo->HasRunningLockedFiles()) { > + mFileStorageInfos.Remove(storageId); Might want an #ifdef DEBUG block here to set storageId to null since it is dead now. @@ +184,5 @@ > + if (!mFileStorageInfos.Get(aFileStorage->StorageId(), &fileStorageInfo)) { > + return; > + } > + > + nsAutoTArray<nsRefPtr<LockedFile>, 50> lockedFiles; 50 seems a little large here... @@ +212,5 @@ > +FileService::MaybeFireCallback(PRUint32 aCallbackIndex) > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + StoragesCompleteCallback& callback = mCompleteCallbacks[aCallbackIndex]; Hm... Can any of these callbacks cause mCompleteCallbacks to be modified? That would be very bad given the way you're doing this here. @@ +229,5 @@ > + } > +} > + > +inline > +FileService::LockedFileQueue::LockedFileQueue(LockedFile* aLockedFile) You should add the 'inline' keyword to the header. For all the rest of these too. @@ +243,5 @@ > + > + if (mLockedFile->mRequestMode == LockedFile::PARALLEL) { > + nsresult rv = aFileHelper->AsyncRun(this); > + if (NS_FAILED(rv)) { > + NS_WARNING("Failed to asynchronously run helper!"); Hm, why not propagate this back to the caller? @@ +261,5 @@ > + > + mQueue.RemoveElementAt(index); > + } > + else { > + mCurrentHelper = nsnull; Assert that mCurrentHelper == aFileHelper @@ +268,5 @@ > + } > +} > + > +void > +FileService::LockedFileQueue::ProcessQueue() I think this should return an nsresult too. @@ +289,5 @@ > +{ > + nsRefPtr<LockedFileQueue>* lockedFileQueue = > + mLockedFileQueues.AppendElement(); > + *lockedFileQueue = new LockedFileQueue(aLockedFile); > + return *lockedFileQueue; Up to you, but I find 'return lockedFileQueue->get()' to be more readable. @@ +316,5 @@ > + return info; > +} > + > +void > +FileService::FileStorageInfo::RemoveLockedFileQueueFor(LockedFile* aLockedFile) How about a comment on why this loop is more complicated than it seems like it ought to be (rebuilding lock lists because multiple lockedfile objects may lock same file name). @@ +369,5 @@ > + > +inline bool > +FileService::FileStorageInfo::HasRunningLockedFiles() > +{ > + return mLockedFileQueues.Length() > 0; Put this in the header? Nit: !IsEmpty() seems more readable. ::: dom/file/FileService.h @@ +28,5 @@ > + // Returns a non-owning reference! > + static FileService* Get(); > + > + nsresult Enqueue(LockedFile* aLockedFile, > + FileHelper* aFileHelper = nsnull); Nit: Let's lose the optional arg @@ +46,5 @@ > + { > + friend class FileService; > + > + public: > + LockedFileQueue(LockedFile* aLockedFile); Is this only created by the FileService? If so hide this in private, FileService is a friend already. @@ +48,5 @@ > + > + public: > + LockedFileQueue(LockedFile* aLockedFile); > + > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(LockedFileQueue) Hm. This is only safe because your base class has a pure virtual addref/release (NS_METHOD vs. NS_IMETHOD mismatch with this macro). I think you should manually declare here: NS_IMETHOD_(nsrefcnt) AddRef(); NS_IMETHOD_(nsrefcnt) Release(); Then just use the standard NS_IMPL_THREADSAFE_ADDREF/RELEASE macros in the cpp. @@ +58,5 @@ > + private: > + void ProcessQueue(); > + > + nsRefPtr<LockedFile> mLockedFile; > + nsAutoTArray<nsRefPtr<FileHelper>, 10> mQueue; Hm... Where'd 10 come from? @@ +75,5 @@ > + > + public: > + LockedFileQueue* CreateLockedFileQueue(LockedFile* aLockedFile); > + > + LockedFileQueue* GetLockedFileQueueFor(LockedFile* aLockedFile); Nit: Lose the "For", it's redundant. In several other methods below as well. @@ +100,5 @@ > + { > + mFilesWriting.AppendElement(aFileName); > + } > + > + bool IsFileLockedForReading(const nsAString& aFileName) Hm. These are O(n) searches... Possible that that could get slow, right? ::: dom/file/FileStream.cpp @@ +21,5 @@ > +#endif > + > +USING_FILE_NAMESPACE > + > +FileStream::FileStream() Looks like this could go in the header. @@ +28,5 @@ > + mFile(nsnull) > +{ > +} > + > +FileStream::~FileStream() This too. @@ +68,5 @@ > + default: > + whence = -1; > + } > + > + int rc = fseek(mFile, aOffset, whence); Hm. I really think we should use 64bit length apis here... Hate to say it, but we've solved this problem already. We have necko's file streams, which only do reading or writing, but maybe we could reuse that somehow? Otherwise, we have NSPR stuff that gets this right across platforms, and we should use it I think. @@ +92,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +FileStream::SetEOF() You could skip the first three chunks of this function by just calling Tell @@ +129,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +FileStream::Available(PRUint32* aResult) Could skip early stuff by calling Tell @@ +167,5 @@ > + if (!mFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + int bytesRead = fread(aBuf, 1, aCount, mFile); All the docs I have say this returns a size_t @@ +180,5 @@ > +NS_IMETHODIMP > +FileStream::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure, > + PRUint32 aCount, PRUint32* aResult) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; NS_NOTREACHED too, here and below. @@ +200,5 @@ > + if (!mFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + int cnt = fwrite(aBuf, 1, aCount, mFile); This returns size_t @@ +240,5 @@ > +} > + > +NS_IMETHODIMP > +FileStream::Init(nsIFile* aFile, const nsAString& aMode, > + PRInt32 aBehaviorFlags) Nit: Indentation is weird here. @@ +242,5 @@ > +NS_IMETHODIMP > +FileStream::Init(nsIFile* aFile, const nsAString& aMode, > + PRInt32 aBehaviorFlags) > +{ > + NS_ENSURE_TRUE(!mFile, NS_ERROR_ALREADY_INITIALIZED); This will never be exposed to script, right? So let's just ASSERT, not ENSURE. For both of these. @@ +269,5 @@ > + if (!mFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + struct stat buffer; Nit: no need for 'struct' @@ +274,5 @@ > + int rc = fstat(fileno(mFile), &buffer); > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > + > + PRInt64 s; > + LL_I2L(s, buffer.st_size); We don't use the LL_ macros any more. @@ +290,5 @@ > + if (!mFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + struct stat buffer; Nit: no need for 'struct'. @@ +295,5 @@ > + int rc = fstat(fileno(mFile), &buffer); > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > + > + PRInt64 s, s2us; > + LL_I2L(s, buffer.st_mtime); No need for these. @@ +304,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +FileStream::Sync() Replace first few chunks with Flush() @@ +327,5 @@ > + return NS_OK; > +} > + > +void > +FileStream::CleanUpOpen() This can be inlined in the header. @@ +353,5 @@ > + return NS_OK; > +} > + > +nsresult > +FileStream::DoPendingOpen() Put this in the header. ::: dom/file/FileStream.h @@ +28,5 @@ > + NS_DECL_NSISEEKABLESTREAM > + NS_DECL_NSIFILESTREAM > + > + // nsIInputStream > + NS_IMETHOD Close(void); Nit: nix the void @@ +29,5 @@ > + NS_DECL_NSIFILESTREAM > + > + // nsIInputStream > + NS_IMETHOD Close(void); > + NS_IMETHOD Available(PRUint32 *_retval); Nit: Here and below, * on the left. @@ +33,5 @@ > + NS_IMETHOD Available(PRUint32 *_retval); > + NS_IMETHOD Read(char *aBuf, PRUint32 aCount, PRUint32 *_retval); > + NS_IMETHOD ReadSegments(nsWriteSegmentFun aWriter, void *aClosure, > + PRUint32 aCount, PRUint32 *_retval); > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); Nit: No need for NS_OUTPARAM. below too. @@ +36,5 @@ > + PRUint32 aCount, PRUint32 *_retval); > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > + > + // nsIOutputStream > + //NS_IMETHOD Close(void); Nit: Rather than this just put something about Close() being identical to nsIInputStream. IsNonBlocking too. @@ +46,5 @@ > + PRUint32 aCount, PRUint32 *_retval); > + //NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > + > + FileStream(); > + virtual ~FileStream(); Nit: protected destructor ::: dom/file/FileStreamWrappers.cpp @@ +77,5 @@ > + mOffset(aOffset), > + mLimit(aLimit), > + mFlags(aFlags), > + mFirstTime(true) > +{ Nit: Assert non-null filestread/filehelper @@ +98,5 @@ > + } > + } > +} > + > +NS_IMPL_THREADSAFE_ADDREF(FileStreamWrapper) You can just use NS_IMPL_THREADSAFE_ISUPPORTS0 to replace all of these macros. @@ +112,5 @@ > + PRUint64 aLimit, > + PRUint32 aFlags) > +: FileStreamWrapper(aFileStream, aFileHelper, aOffset, aLimit, aFlags) > +{ > + mInputStream = do_QueryInterface(mFileStream); Hm... Seems like you need to assert this or make an init method. @@ +115,5 @@ > +{ > + mInputStream = do_QueryInterface(mFileStream); > +} > + > +NS_IMPL_ADDREF_INHERITED(FileInputStreamWrapper, FileStreamWrapper) NS_IMPL_ISUPPORTS_INHERITED1 @@ +166,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + } > + } > + > + mOffset = 0; Hm... I don't understand why we do this. Shouldn't we keep this around? @@ +169,5 @@ > + > + mOffset = 0; > + } > + > + PRUint32 max = mLimit - mOffset; This needs to be 64 bit, and signed (seems possible for mOffset to be larger than mLimit, right?). Then check <= below. @@ +216,5 @@ > + FileHelper* aFileHelper, > + PRUint64 aOffset, > + PRUint64 aLimit, > + PRUint32 aFlags) > +: FileStreamWrapper(aFileStream, aFileHelper, aOffset, aLimit, aFlags) You need an #ifdef DEBUG here to mark mThread null @@ +218,5 @@ > + PRUint64 aLimit, > + PRUint32 aFlags) > +: FileStreamWrapper(aFileStream, aFileHelper, aOffset, aLimit, aFlags) > +{ > + mOutputStream = do_QueryInterface(mFileStream); Assert or init method. @@ +221,5 @@ > +{ > + mOutputStream = do_QueryInterface(mFileStream); > +} > + > +NS_IMPL_ADDREF_INHERITED(FileOutputStreamWrapper, FileStreamWrapper) NS_IMPL_ISUPPORTS_INHERITED1 @@ +237,5 @@ > + nsresult rv; > + > + if (!mFirstTime) { > + // We must flush the stream on the same thread on which we wrote some data. > + nsCOMPtr<nsIOutputStream> ostream = do_QueryInterface(mFileStream); Wait, you already have this, right? (mOutputStream) @@ +245,5 @@ > + } > + > +#ifdef DEBUG > + void* thread = PR_GetCurrentThread(); > + NS_ASSERTION(thread == mThread, "Unsetting thread locals on wrong thread!"); Just assert PR_GetCurrentThread() == mThread, lose the stack var and the #ifdef DEBUG @@ +262,5 @@ > + > + mOffset = 0; > + mLimit = 0; > + > + return NS_OK; If that flush failed don't you want to return that here? @@ +279,5 @@ > + mThread = PR_GetCurrentThread(); > +#endif > + mFileHelper->mFileStorage->SetThreadLocals(); > + > + nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mFileStream); I'd QI mOutputStream, not mFileStream @@ +285,5 @@ > + if (mOffset == LL_MAXUINT) { > + seekable->Seek(nsISeekableStream::NS_SEEK_END, 0); > + } > + else { > + seekable->Seek(nsISeekableStream::NS_SEEK_SET, mOffset); You should propagate any errors here. @@ +292,5 @@ > + > + mOffset = 0; > + } > + > + PRUint32 max = mLimit - mOffset; Same problem here, use PRInt64, ensure that max can't be negative. @@ +304,5 @@ > + } > + > + nsresult rv = mOutputStream->Write(aBuf, aCount, _retval); > + > + if (NS_SUCCEEDED(rv)) { NS_ENSURE_SUCCESS here would be nicer. @@ +321,5 @@ > + > +NS_IMETHODIMP > +FileOutputStreamWrapper::Flush() > +{ > + return NS_OK; Hm... Why wouldn't we call flush on mOutputStream? @@ +349,5 @@ > + > +NS_IMPL_THREADSAFE_ISUPPORTS1(ProgressRunnable, nsIRunnable) > + > +NS_IMETHODIMP > +ProgressRunnable::Run() Assert main thread for these 3 runnables ::: dom/file/FileStreamWrappers.h @@ +33,5 @@ > + > + enum { > + NOTIFY_PROGRESS = 1 << 0, > + NOTIFY_CLOSE = 1 << 1, > + NOTIFY_DESTROY = 1 << 1 Er... This is intentional? Maybe NOTIFY_DESTROY = NOTIFY_CLOSE to make that clear. @@ +56,5 @@ > + FileInputStreamWrapper(nsIFileStream* aFileStream, > + FileHelper* aFileHelper, > + PRUint64 aOffset, > + PRUint64 aLimit, > + PRUint32 aFlags = 0); Nit: Require that last arg. @@ +58,5 @@ > + PRUint64 aOffset, > + PRUint64 aLimit, > + PRUint32 aFlags = 0); > + > + virtual ~FileInputStreamWrapper() Nit: Protected @@ +76,5 @@ > + FileOutputStreamWrapper(nsIFileStream* aFileStream, > + FileHelper* aFileHelper, > + PRUint64 aOffset, > + PRUint64 aLimit, > + PRUint32 aFlags = 0); Nit: Require that last arg @@ +80,5 @@ > + PRUint32 aFlags = 0); > + > + virtual ~FileOutputStreamWrapper() > + { > + } Nit: Protected, and { } @@ +85,5 @@ > + > +private: > + nsCOMPtr<nsIOutputStream> mOutputStream; > +#ifdef DEBUG > + void* mThread; How about 'mWriteThread'? Or something that indicates that it's the thread where you actually write things, not confused with creating/owning thread. ::: dom/file/LockedFile.cpp @@ +41,5 @@ > +public: > + ReadHelper(LockedFile* aLockedFile, > + FileRequest* aFileRequest, > + PRInt64 aLocation, > + PRInt64 aSize) These can't really be negative, right? In DoAsyncRun you pass the size into an unsigned arg, so if they can actually be negative then I think we need some changes below. @@ +45,5 @@ > + PRInt64 aSize) > + : FileHelper(aLockedFile, aFileRequest), > + mLocation(aLocation), mSize(aSize) > + { > + mStream = new MemoryOutputStream(aSize); Hm, MemoryOutputStream takes a PRInt32! @@ +50,5 @@ > + } > + > + virtual ~ReadHelper() > + { > + } Nit { }, in several other places below too. Although, you don't really need them, right? @@ +73,5 @@ > + PRInt64 aSize, > + const nsAString& aEncoding) > + : ReadHelper(aLockedFile, aFileRequest, aLocation, aSize) > + { > + CopyUTF16toUTF8(aEncoding, mEncoding); Technically you could avoid doing this until after you check against IsEmpty on the other thread. @@ +108,5 @@ > + nsresult DoAsyncRun(nsIFileStream* aStream); > + > +private: > + PRInt64 mLocation; > + nsCOMPtr<nsIInputStream> mStream; This probably needs to be released on the main thread, right? @@ +184,5 @@ > + nsCOMPtr<nsIInputStream> mStream; > +}; > + > +already_AddRefed<nsDOMEvent> > +CreateGenericEvent(const nsAString& aType, bool aBubbles, bool aCancelable) This isn't really needed now that we have nsContentUtils::DispatchTrustedEvent. Let's switch to using that. @@ +206,5 @@ > + RequestMode aRequestMode) > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + nsRefPtr<LockedFile> lockedFile(new LockedFile()); Nit: use = @@ +226,5 @@ > + > + NS_ASSERTION(depth, "This should never be 0!"); > + lockedFile->mCreatedRecursionDepth = depth - 1; > + > + rv = thread->AddObserver(lockedFile); Hm. IndexedDB is no longer going to use this, see bug 672667. This code will have similar trouble with native event handlers, so we'll need to change it to use the new method. @@ +231,5 @@ > + NS_ENSURE_SUCCESS(rv, nsnull); > + > + lockedFile->mCreating = true; > + > + FileService* service = FileService::GetOrCreate(); This can fail... @@ +244,5 @@ > + mLocation(0), > + mPendingRequests(0), > + mCreatedRecursionDepth(0), > + mAborted(false), > + mCreating(false) Missing mRequestMode @@ +308,5 @@ > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + if (!mPendingRequests) { > + NS_ASSERTION(mReadyState == INITIAL, > + "Reusing a transaction!"); Nit: Not a transaction ;) @@ +342,5 @@ > + nsCOMPtr<nsIFileStream> stream = mFileHandle->CreateStream(); > + > + nsString streamMode; > + if (mMode == READ_WRITE) { > + streamMode = NS_LITERAL_STRING("r+b"); Use AssignLiteral here and below. @@ +364,5 @@ > + > + if (mStream && mFileHandle->mFileStorage->IsStorageInvalidated()) { > + return NS_ERROR_NOT_AVAILABLE; > + } > + else { Nit: else after a return @@ +366,5 @@ > + return NS_ERROR_NOT_AVAILABLE; > + } > + else { > + nsCOMPtr<nsIFileStream> stream; > + nsresult rv = CreateStream(getter_AddRefs(stream)); Hm... It's possible to have mStream and !IsStorageInvalidated, right? in that case looks like you're always making a new stream. Don't you want to only create if one doesn't exist already? @@ +383,5 @@ > +LockedFile::GenerateFileRequest() > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + return FileRequest::Create(GetOwner(), > + this); Nit: This can fit on one line. @@ +419,5 @@ > + > +NS_IMETHODIMP > +LockedFile::GetFileHandle(nsIDOMFileHandle** aFileHandle) > +{ > + nsRefPtr<FileHandle> result = mFileHandle; Nit: Use an nsCOMPtr here (has the added benefit of asserting QI correctness). @@ +429,5 @@ > +LockedFile::GetMode(nsAString& aMode) > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + switch(mMode) { Nit: space before ( @@ +434,5 @@ > + case READ_ONLY: > + aMode.AssignLiteral("readonly"); > + break; > + case READ_WRITE: > + aMode.AssignLiteral("readwrite"); Nit: default: NS_NOTREACHED @@ +446,5 @@ > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + *aOpen = IsOpen(); > + Nit: lose extra line @@ +481,5 @@ > + } > + > + nsRefPtr<MetadataParameters> params = new MetadataParameters(); > + > + if (!JSVAL_IS_VOID(aParameters) && !JSVAL_IS_NULL(aParameters)) { Is this necessary (I would hope the other code does this)? Otherwise you probably want !JSVAL_IS_PRIMITIVE. @@ +504,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +LockedFile::ReadAsArrayBuffer(PRInt32 aSize, Hm, you don't do any sanity checking on aSize. It could be negative or 0 here. I added a comment to the IDL, i think this should become PRUint64, but if it's 0 you can either throw or do a shortcut and never go to the necko thread. @@ +530,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +LockedFile::ReadAsText(PRInt32 aSize, Same comments here. @@ +704,5 @@ > + return NS_OK; > +} > + > +nsresult > +LockedFile::OpenInputStream(bool aWholeFile, PRUint64 aStart, PRUint64 aLength, This is a little weird... You don't really need to queue anything here. You know that you're in parallel mode, and all that this twisty windy code path ends up doing is calling CreateStream and then wrapping it. I think it's much easier to understand if you just do that here and lose the OpenStreamHelper entirely. @@ +759,5 @@ > + rv = helper->Enqueue(); > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR); > + > + if (!aAppend) { > + mLocation += inputLength; Wait, so all the other methods move mLocation, but Append doesn't? I would think that mLocation should be LL_MAXUINT now. @@ +767,5 @@ > + return NS_OK; > +} > + > +nsresult > +LockedFile::GetInputStream(const jsval& aValue, JSContext* aCx, This should be static, and maybe not a member at all. And we could rename it to something like GetInputStreamForJSVal @@ +779,5 @@ > + if (JS_IsArrayBufferObject(obj, aCx)) { > + PRUint32 length = JS_GetArrayBufferByteLength(obj, aCx); > + > + rv = NS_NewByteInputStream(aInputStream, > + (char*)JS_GetArrayBufferData(obj, aCx), length, reinterpret_cast @@ +780,5 @@ > + PRUint32 length = JS_GetArrayBufferByteLength(obj, aCx); > + > + rv = NS_NewByteInputStream(aInputStream, > + (char*)JS_GetArrayBufferData(obj, aCx), length, > + NS_ASSIGNMENT_COPY); Ugh, i hate this... But I don't see any way around it. @@ +794,5 @@ > + if (blob) { > + rv = blob->GetInternalStream(aInputStream); > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = blob->GetSize(aInputLength); I think you should switch the order here, in case something fails @@ +814,5 @@ > + } > + > + nsDependentJSString str; > + if (!str.init(aCx, jsstr)) { > + return NS_ERROR_XPC_BAD_CONVERT_JS; This isn't really right, you want to swap with !jsstr above (this one should be NS_ERROR_FAILURE, that one should be BAD_CONVERT) @@ +851,5 @@ > +} > + > +FinishHelper::FinishHelper(LockedFile* aLockedFile) > +: mLockedFile(aLockedFile), > + mAborted(!!aLockedFile->mAborted) !! is unnecessary here. @@ +856,5 @@ > +{ > + mStream.swap(aLockedFile->mStream); > +} > + > +FinishHelper::~FinishHelper() Move to header. @@ +880,5 @@ > + } > + else { > + event = CreateGenericEvent(NS_LITERAL_STRING("complete"), false, false); > + } > + NS_ENSURE_TRUE(event, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); Heh. Busted ;) @@ +913,5 @@ > + > +nsresult > +ReadHelper::DoAsyncRun(nsIFileStream* aStream) > +{ > + NS_PRECONDITION(aStream, "Passed a null stream!"); NS_ASSERTION please @@ +923,5 @@ > + > + nsCOMPtr<nsIAsyncStreamCopier> copier; > + nsresult rv = > + NS_NewAsyncStreamCopier(getter_AddRefs(copier), istream, mStream, nsnull, > + false, true, STREAM_COPY_BLOCK_SIZE, true, true); Hm, by passing null for the target argument you're going to have a getService call every time you do a request. How about you get the transportservice and cache it in your fileservice? Nit: You could leave off the last two args if you want. Below too. @@ +941,5 @@ > +{ > + JSObject *arrayBuffer; > + nsresult rv = > + nsContentUtils::CreateArrayBuffer(aCx, mStream->Data(), &arrayBuffer); > + NS_ENSURE_SUCCESS(rv, rv); Hm, you could have read a really big chunk of stuff into memory, could you go ahead and mStream->Close() before returning here? That way if you leak or hang around longer than you expect at least you won't be holding that data. @@ +949,5 @@ > + return NS_OK; > +} > + > +nsresult > +ReadTextHelper::GetSuccessResult(JSContext* aCx, Same here with calling mStream->Close() before returning. @@ +975,5 @@ > + tmpString); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (!xpc::StringToJsval(aCx, tmpString, aVal)) { > + return NS_ERROR_FAILURE; Nit: Add a warning @@ +984,5 @@ > + > +nsresult > +WriteHelper::DoAsyncRun(nsIFileStream* aStream) > +{ > + NS_PRECONDITION(aStream, "Passed a null stream!"); NS_ASSERTION ::: dom/file/LockedFile.h @@ +35,5 @@ > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_NSIDOMLOCKEDFILE > + NS_DECL_NSITHREADOBSERVER > + > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED( Unnecessary SCRIPT_HOLDER here @@ +68,5 @@ > + // nsIDOMEventTarget > + virtual nsresult PreHandleEvent(nsEventChainPreVisitor& aVisitor); > + > + void OnNewRequest(); > + void OnRequestFinished(); Looks like these are only called by FileHelper, which is a friend. Make them private? @@ +70,5 @@ > + > + void OnNewRequest(); > + void OnRequestFinished(); > + > + already_AddRefed<FileRequest> GenerateFileRequest(); This is only ever called in LockedFile methods, so make it private. Also need to mark inline here. @@ +95,5 @@ > + LockedFile(); > + ~LockedFile(); > + > + nsresult WriteOrAppend(const jsval& aValue, > + JSContext* aCx, Nit: Above you have args going all the way to 80 chars, here you have each on its own line. Let's be consistent. @@ +129,5 @@ > + NS_DECL_ISUPPORTS > + NS_DECL_NSIRUNNABLE > + > + FinishHelper(LockedFile* aLockedFile); > + ~FinishHelper(); This can be private. ::: dom/file/MemoryStreams.cpp @@ +13,5 @@ > +MemoryOutputStream::MemoryOutputStream(PRInt32 aSize) > +: mOffset(0) > +{ > + char* dummy; > + mData.GetMutableData(&dummy, aSize); This can fail. Need to check, sadly, so probably want an Init method or static Create that does both. @@ +16,5 @@ > + char* dummy; > + mData.GetMutableData(&dummy, aSize); > +} > + > +MemoryOutputStream::~MemoryOutputStream() This can go in the header @@ +20,5 @@ > +MemoryOutputStream::~MemoryOutputStream() > +{ > +} > + > +NS_IMPL_THREADSAFE_ADDREF(MemoryOutputStream) NS_IMPL_THREADSAFE_ISUPPORTS1 @@ +36,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +MemoryOutputStream::Write(const char *aBuf, PRUint32 aCount, PRUint32 *_retval) Nit: * on the left @@ +38,5 @@ > + > +NS_IMETHODIMP > +MemoryOutputStream::Write(const char *aBuf, PRUint32 aCount, PRUint32 *_retval) > +{ > + return WriteSegments(NS_CopySegmentToBuffer, (char*)aBuf, aCount, _retval); Er... Why are you un-consting aBuf? @@ +58,5 @@ > +NS_IMETHODIMP > +MemoryOutputStream::WriteSegments(nsReadSegmentFun aReader, void* aClosure, > + PRUint32 aCount, PRUint32* _retval) > +{ > + PRUint32 maxCount = mData.Length() - mOffset; mOffset could be greater than length, right? ::: dom/file/MemoryStreams.h @@ +18,5 @@ > +public: > + NS_DECL_ISUPPORTS > + NS_DECL_NSIOUTPUTSTREAM > + > + MemoryOutputStream(PRInt32 aSize); Make sure we figure out what to do with this being a signed 32 bit integer. @@ +19,5 @@ > + NS_DECL_ISUPPORTS > + NS_DECL_NSIOUTPUTSTREAM > + > + MemoryOutputStream(PRInt32 aSize); > + virtual ~MemoryOutputStream(); Private, and inlined. ::: dom/file/MetadataHelper.cpp @@ +12,5 @@ > + > +nsresult > +MetadataParameters::Init(JSContext* aCx, const jsval* aVal) > +{ > + return mParams.Init(aCx, aVal); You could inline this. @@ +37,5 @@ > + JSObject* obj = JS_NewObject(aCx, nsnull, nsnull, nsnull); > + NS_ENSURE_TRUE(obj, NS_ERROR_OUT_OF_MEMORY); > + > + if (mParams->mParams.size && > + !JS_DefineProperty(aCx, obj, "size", INT_TO_JSVAL(mParams->mSize), This is kind of confusing... You are checking 'mParams->mParams.size', but then using 'mParams->mSize' Can you just nix the mParams->mParams member entirely and carry bools on the MetadataHelper ("wantsSize", "hasSize", something like that)? Also, mSize is a PRInt64... A few things here. 1. Why signed? What would negative mean? 2. You can't use INT_TO_JSVAL for that. You need to compare it to JSVAL_INT_MAX and convert to a double if it's bigger. The you want to setNumber() on a jsval. @@ +42,5 @@ > + nsnull, nsnull, JSPROP_ENUMERATE)) { > + return NS_ERROR_FAILURE; > + } > + > + if (mParams->mParams.lastModified) { Same weirdness here. @@ +44,5 @@ > + } > + > + if (mParams->mParams.lastModified) { > + double msec = mParams->mLastModified / PR_USEC_PER_MSEC; > + JSObject *date = JS_NewDateObjectMsec(aCx, msec); This can fail. ::: dom/file/MetadataHelper.h @@ +19,5 @@ > +BEGIN_FILE_NAMESPACE > + > +class MetadataParameters > +{ > + friend class MetadataHelper; Forward declare. @@ +26,5 @@ > + > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MetadataParameters) > + > + nsresult Init(JSContext* aCx, const jsval* aVal); Nit: Names on next lines again @@ +34,5 @@ > + return mSize; > + } > + > +private: > + mozilla::dom::DOMFileMetadataParameters mParams; You're already in mozilla::dom here right? I bet you can remove this. @@ +60,5 @@ > + { > + public: > + AsyncMetadataGetter(MetadataParameters* aParams) > + : mParams(aParams) > + { } Nit: add an extra line here. ::: dom/file/nsIDOMLockedFile.idl @@ +34,5 @@ > + [optional /* none */] in jsval parameters); > + > + [implicit_jscontext] > + nsIDOMFileRequest > + readAsArrayBuffer(in long size); These should be 'unsigned long long' @@ +37,5 @@ > + nsIDOMFileRequest > + readAsArrayBuffer(in long size); > + > + nsIDOMFileRequest > + readAsText(in long size, [optional] in DOMString encoding); 'unsigned long long'

(In reply to ben turner [:bent] from comment #33) >@@ +68,5 @@ >> + default: >> + whence = -1; >> + } >> + >> + int rc = fseek(mFile, aOffset, whence); > >Hm. I really think we should use 64bit length apis here... Hate to say it, but >we've solved this problem already. We have necko's file streams, which only do >reading or writing, but maybe we could reuse that somehow? Otherwise, we have >NSPR stuff that gets this right across platforms, and we should use it I think. Originally, I didn't want to touch necko ... anyway, here's a patch that refactors necko file stream impl and adds a new read/write stream I also added an interface to get metadata from the stream directly (no need to open the file again) The patch passes on try: https://tbpl.mozilla.org/?tree=Try&rev=e6c18f02c5f1

I also refactored filehandle implementation to use necko file streams for device storage filehandles and IDB specific streams for IDB filehandles. These two stream implementations can't share any code, since necko uses direct calls like open(), write() and IDB must use stdio like functions provided by SQLite (to handle quota) I hope this is acceptable. So, I completely removed dom/file/FileStream.h/cpp, dom/indexedDB/FileStream.h/cpp is still there and I renamed the dom/file/nsIFileStream.idl to dom/indexedDB/nsIStandardFileStream.idl nsIStandardFileStream is also reused in other IDB code (for storing blobs/files) FileHandle::CreateStream() changed to CreateStream(nsIFile* aFile, bool aReadOnly) So stream initialization is done in DOMFileHandle or IDBFileHandle (used to be in LockedFile)

Comment on attachment 617475 [details] [diff] [review] patch v0.4 Review of attachment 617475 [details] [diff] [review]: ----------------------------------------------------------------- This is the last! ::: dom/file/FileHandle.cpp @@ +50,5 @@ > +NS_IMPL_CYCLE_COLLECTION_CLASS(FileHandle) > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(FileHandle, > + nsDOMEventTargetHelper) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mFileStorage, You don't need the _AMBIGUOUS here ::: dom/indexedDB/FileStream.cpp @@ +14,5 @@ > + > +FileStream::FileStream() > +: mBehaviorFlags(0), > + mDeferredOpen(false), > + mQuotaFile(nsnull) Looks like constructor/destructor could be in the header. @@ +34,5 @@ > + NS_INTERFACE_MAP_ENTRY(nsIFileStream) > +NS_INTERFACE_MAP_END > + > +NS_IMETHODIMP > +FileStream::Seek(PRInt32 aWhence, PRInt64 aOffset) In each of these methods it might be wise to assert that we are not doing any io on the main thread. @@ +55,5 @@ > + case nsISeekableStream::NS_SEEK_END: > + whence = SEEK_END; > + break; > + default: > + whence = -1; Hm... What should happen here? Should we bail out? @@ +58,5 @@ > + default: > + whence = -1; > + } > + > + int rc = sqlite3_quota_fseek(mQuotaFile, aOffset, whence); This will truncate aOffset. Need to NS_ENSURE that aOffset < PR_INT32_MAX, and we need to make sure we don't deal with bigger sizes anywhere else in this code. We probably should file a bug to update this all to handle 64bit file sizes, and we need to get the sqlite guys to change their api to do it. @@ +74,5 @@ > + if (!mQuotaFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + int rc = sqlite3_quota_ftell(mQuotaFile); long is the return type. @@ +82,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +FileStream::SetEOF() Just call Tell() in the beginning to skip the first three code chunks. @@ +109,5 @@ > + > + if (mQuotaFile) { > + int rc = sqlite3_quota_fclose(mQuotaFile); > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > + mQuotaFile = nsnull; I think you want to set mQuotaFile to null here regardless of whether close fails, right? @@ +116,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +FileStream::Available(PRUint32* aResult) Use Tell() again @@ +134,5 @@ > + > + int end = sqlite3_quota_ftell(mQuotaFile); > + NS_ENSURE_TRUE(end >= 0, NS_BASE_STREAM_OSERROR); > + > + int result = end - cur; long here. And ensure that end >= cur, just in case something wacky happens. @@ +154,5 @@ > + if (!mQuotaFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + int bytesRead = sqlite3_quota_fread(aBuf, 1, aCount, mQuotaFile); This returns size_t, which is unsigned. @@ +177,5 @@ > + *aNonBlocking = false; > + return NS_OK; > +} > + > + Nit: remove extra line @@ +179,5 @@ > +} > + > + > +NS_IMETHODIMP > +FileStream::Write(const char *aBuf, PRUint32 aCount, PRUint32 *aResult) Nit: * on left @@ +188,5 @@ > + if (!mQuotaFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + int cnt = sqlite3_quota_fwrite(aBuf, 1, aCount, mQuotaFile); this returns size_t @@ +200,5 @@ > + > +NS_IMETHODIMP > +FileStream::Flush() > +{ > + nsresult rv = DoPendingOpen(); Do you actually need to do this here? If someone calls Flush() before you've opened the file then I think you could just return NS_OK immediately. @@ +225,5 @@ > +{ > + return NS_ERROR_NOT_IMPLEMENTED; > +} > + > + Nit: Remove extra line. @@ +257,5 @@ > + if (!mQuotaFile) { > + return NS_BASE_STREAM_CLOSED; > + } > + > + PRInt64 rc = sqlite3_quota_file_size(mQuotaFile); So... if rc == -1 then the file isn't under quota management. That should be impossible, right? I think you should just assert that rather than checking in release builds. @@ +279,5 @@ > + int rc = sqlite3_quota_file_mtime(mQuotaFile, &mtime); > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > + > + PRInt64 s, s2us; > + LL_I2L(s, mtime); No need to use these macros any longer. @@ +288,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +FileStream::Sync() Same here, early return if file not opened yet. @@ +304,5 @@ > + return NS_OK; > +} > + > +void > +FileStream::CleanUpOpen() You could move this to the header. @@ +331,5 @@ > + return NS_OK; > +} > + > +nsresult > +FileStream::DoPendingOpen() This could be inlined in the header too. ::: dom/indexedDB/FileStream.h @@ +30,5 @@ > + NS_DECL_NSIFILESTREAM > + > + // nsIInputStream > + NS_IMETHOD Close(void); > + NS_IMETHOD Available(PRUint32 *_retval); Nit: * on left, names on next line. @@ +34,5 @@ > + NS_IMETHOD Available(PRUint32 *_retval); > + NS_IMETHOD Read(char *aBuf, PRUint32 aCount, PRUint32 *_retval); > + NS_IMETHOD ReadSegments(nsWriteSegmentFun aWriter, void *aClosure, > + PRUint32 aCount, PRUint32 *_retval); > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); Nit: No NS_OUTPARAM @@ +37,5 @@ > + PRUint32 aCount, PRUint32 *_retval); > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > + > + // nsIOutputStream > + //NS_IMETHOD Close(void); Nit: comment that Close/IsNonBlocking is same as nsIInputStream @@ +47,5 @@ > + PRUint32 aCount, PRUint32 *_retval); > + //NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > + > + FileStream(); > + virtual ~FileStream(); Protected. ::: dom/indexedDB/IDBDatabase.cpp @@ +155,5 @@ > + const nsAString& aType) > + : AsyncConnectionHelper(aDatabase, aRequest), > + mName(aName), mType(aType) > + { > + } Nit: { } @@ +159,5 @@ > + } > + > + ~CreateFileHelper() > + { > + } Here too @@ +171,5 @@ > + nsString mName; > + nsString mType; > + > + // Out-params. > + nsRefPtr<FileInfo> mFileInfo; These should be released via ReleaseMainThreadObjects @@ +314,5 @@ > IDBDatabase::OnUnlink() > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > +// NS_ASSERTION(!GetOwner() && !GetScriptOwner(), > +// "Should have been cleared already!"); Nit: Uncomment (I've got those fixed in another bug) @@ +737,5 @@ > } > > +// nsIFileStorage > +inline nsISupports* > +IDBDatabase::StorageId() These are all virtual so they won't be inlined. Let's remove that inline from each, and remove this comment too. @@ +757,5 @@ > + > +inline void > +IDBDatabase::SetThreadLocals() > +{ > + IndexedDatabaseManager::SetCurrentWindow(GetOwner()); Assert GetOwner first here, it can become null. @@ +881,5 @@ > + mFileInfo = fileManager->GetNewFileInfo(); > + NS_ENSURE_TRUE(mFileInfo, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + nsCOMPtr<nsIFile> directory = fileManager->GetDirectory(); > + NS_ENSURE_TRUE(directory, nsnull); Oops, these need NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR. One more below. ::: dom/indexedDB/IDBFileHandle.cpp @@ +45,5 @@ > + > + nsRefPtr<FileInfo> fileInfo(aFileInfo); > + NS_ASSERTION(fileInfo, "Null pointer!"); > + > + nsRefPtr<IDBFileHandle> newFile(new IDBFileHandle); Nit: =, and make sure to call with () @@ +53,5 @@ > + newFile->mFileStorage = aDatabase; > + newFile->mName = aName; > + newFile->mType = aType; > + > + newFile->mFile = ::GetFile(fileInfo); Nit: Rather than :: why not just have a more descriptive name for your local function? @@ +101,5 @@ > +IDBFileHandle::GetDb(nsIIDBDatabase** aDB) > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + nsCOMPtr<nsIIDBDatabase> database = do_QueryInterface(mFileStorage); Assert that this always succeeds. ::: dom/indexedDB/IDBFileHandle.h @@ +33,5 @@ > + return mFileInfo; > + } > + > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(IDBFileHandle, > + FileHandle) This is unndeeded. You're not traversing or unlinking anything. ::: dom/indexedDB/IDBObjectStore.cpp @@ +818,5 @@ > + > + for (PRUint32 i = 0; i < array.Length(); i++) { > + const PRInt64& id = array.ElementAt(i); > + > + nsRefPtr<FileInfo> fileInfo = fileManager->GetFileInfo(id); Just use array[i] instead of using an alias here. @@ +953,5 @@ > uint32_t aData, > void* aClosure) > { > + if (aTag == SCTAG_DOM_FILEHANDLE || > + aTag == SCTAG_DOM_BLOB || aTag == SCTAG_DOM_FILE) { Nit: the first two should fit on the same line, right? @@ +1168,5 @@ > + nsCOMPtr<nsIDOMFileHandle> fileHandle = do_QueryInterface(supports); > + if (fileHandle) { > + nsRefPtr<FileInfo> fileInfo = fileHandle->GetFileInfo(); > + if (!fileInfo) { > + return false; These fail points could use warnings. ::: dom/indexedDB/IDBTransaction.cpp @@ +478,3 @@ > { > + if (!mCreatedFileInfos.Put(aBlob, aFileInfo)) { > + NS_WARNING("Out of memory?"); Need to let caller know. ::: dom/indexedDB/IDBTransaction.h @@ +174,5 @@ > GetOrCreateObjectStore(const nsAString& aName, > ObjectStoreInfo* aObjectStoreInfo); > > + already_AddRefed<FileInfo> GetFileInfo(nsIDOMBlob* aBlob); > + void AddFileInfo(nsIDOMBlob* aBlob, FileInfo* aFileInfo); This can fail, needs to propagate its result. ::: dom/indexedDB/IndexedDatabase.h @@ +71,2 @@ > > struct StructuredCloneReadInfo { Now need a constructor to null-init mDatabase. @@ +72,5 @@ > struct StructuredCloneReadInfo { > void Swap(StructuredCloneReadInfo& aCloneReadInfo) { > mCloneBuffer.swap(aCloneReadInfo.mCloneBuffer); > mFileInfos.SwapElements(aCloneReadInfo.mFileInfos); > + mDatabase = aCloneReadInfo.mDatabase; This doesn't swap :) @@ +88,3 @@ > }; > > struct StructuredCloneWriteInfo { Now need a constructor to null-init mTransaction. @@ +90,5 @@ > struct StructuredCloneWriteInfo { > void Swap(StructuredCloneWriteInfo& aCloneWriteInfo) { > mCloneBuffer.swap(aCloneWriteInfo.mCloneBuffer); > + mFiles.SwapElements(aCloneWriteInfo.mFiles); > + mTransaction = aCloneWriteInfo.mTransaction; This doesn't swap either. ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +541,2 @@ > TransactionThreadPool* pool = TransactionThreadPool::Get(); > + if (!(service || pool)) { Nit: !service && !pool @@ +546,5 @@ > for (PRUint32 index = 0; index < liveDatabases.Length(); index++) { > IDBDatabase*& database = liveDatabases[index]; > if (database->GetOwner() == aWindow && > + (service && service->HasLockedFilesForStorage(database) || > + pool && pool->HasTransactionsForDatabase(database))) { This needs more parenthesis around the two args to || @@ +590,5 @@ > + count ? new WaitForTransactionsToFinishRunnable(op, count) > + : new WaitForTransactionsToFinishRunnable(op); > + > + if (!count) { > + runnable->Run(); This should never happen. Assert count == 1 or 2. @@ +598,3 @@ > > + if (service) { > + nsAutoTArray<nsCOMPtr<nsIFileStorage>, 1> array; Here and below don't use nsAutoTArray, the elements are about to be swapped and that means coyping, so we're just wasting stack space. Old code was wrong. @@ +1261,5 @@ > + // If the necko service (thread pool) gets the shutdown notification > + // first then the sync loop won't be processed at all, otherwise it will > + // lock the main thread until all IDB file storages are finished. > + > + nsAutoTArray<nsCOMPtr<nsIFileStorage>, 50> liveDatabases; Don't use nsAutoTArray here. Do call SetCapacity first though, with mLiveDatabases.Count(). @@ +1273,5 @@ > + if (!service->WaitForAllStoragesToComplete(liveDatabases, runnable)) { > + NS_WARNING("Failed to wait for databases to complete!"); > + } > + > + nsIThread *thread = NS_GetCurrentThread(); Nit: * on left @@ +1274,5 @@ > + NS_WARNING("Failed to wait for databases to complete!"); > + } > + > + nsIThread *thread = NS_GetCurrentThread(); > + while (runnable->mStillBusy) { Nit: I'd prefer a simple method IsBusy() to directly accessing the member. @@ +1276,5 @@ > + > + nsIThread *thread = NS_GetCurrentThread(); > + while (runnable->mStillBusy) { > + if (!NS_ProcessNextEvent(thread)) { > + NS_WARNING("Something bad happened!"); NS_ERROR("Failed to process next event!");

Comment on attachment 621317 [details] [diff] [review] patch for necko Review of attachment 621317 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good! ::: netwerk/base/public/nsIFileStreams.idl @@ +222,5 @@ > + * A file stream that allows you to get some metadata like size and > + * last modified time. > + */ > +[scriptable, uuid(07f679e4-9601-4bd1-b510-cd3852edb881)] > +interface nsIFileMetadataStream : nsISupports This isn't really a stream... ::: netwerk/base/public/nsNetUtil.h @@ +953,5 @@ > do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID, &rv); > if (NS_SUCCEEDED(rv)) { > rv = in->Init(file, ioFlags, perm, behaviorFlags); > if (NS_SUCCEEDED(rv)) > + rv = CallQueryInterface(in, result); This is wrong, no need to QI. I think nsCOMPtr.forget can handle this now though @@ +991,5 @@ > do_CreateInstance(NS_LOCALFILEOUTPUTSTREAM_CONTRACTID, &rv); > if (NS_SUCCEEDED(rv)) { > rv = out->Init(file, ioFlags, perm, behaviorFlags); > if (NS_SUCCEEDED(rv)) > + rv = CallQueryInterface(out, result); Here too. @@ +1010,5 @@ > do_CreateInstance(NS_SAFELOCALFILEOUTPUTSTREAM_CONTRACTID, &rv); > if (NS_SUCCEEDED(rv)) { > rv = out->Init(file, ioFlags, perm, behaviorFlags); > if (NS_SUCCEEDED(rv)) > + rv = CallQueryInterface(out, result); And here. @@ +1028,5 @@ > + do_CreateInstance(NS_LOCALFILESTREAM_CONTRACTID, &rv); > + if (NS_SUCCEEDED(rv)) { > + rv = stream->Init(file, ioFlags, perm, behaviorFlags); > + if (NS_SUCCEEDED(rv)) > + rv = CallQueryInterface(stream, result); This can definitely use forget() ::: netwerk/base/src/nsFileStreams.cpp @@ +351,5 @@ > NS_LOCALFILEINPUTSTREAM_CID) > > NS_INTERFACE_MAP_BEGIN(nsFileInputStream) > +// NS_INTERFACE_MAP_ENTRY(nsFileStream) > +// NS_INTERFACE_MAP_ENTRY(nsIInputStream) Hm. @@ +845,5 @@ > + if (stream == nsnull) > + return NS_ERROR_OUT_OF_MEMORY; > + NS_ADDREF(stream); > + nsresult rv = stream->QueryInterface(aIID, aResult); > + NS_RELEASE(stream); Blegh. nsRefPtr and CallQueryInterface! @@ +904,5 @@ > + if (!mFD) { > + return NS_BASE_STREAM_CLOSED; > + } > + > +#if 0 ? ::: netwerk/base/src/nsFileStreams.h @@ +152,5 @@ > + NS_IMETHOD Available(PRUint32* _retval) > + { > + return nsFileStreamBase::Available(_retval); > + } > + NS_IMETHOD Read(char* aBuf, PRUint32 aCount, PRUint32* _retval) I'd move this to the cpp @@ +184,4 @@ > NS_IMETHOD Seek(PRInt32 aWhence, PRInt64 aOffset); > > + nsFileInputStream() > + : nsFileStreamBase() This isn't really needed. @@ -208,5 @@ > public nsIFileOutputStream > { > public: > NS_DECL_ISUPPORTS_INHERITED > - NS_DECL_NSIOUTPUTSTREAM I think here you can just use NS_FORWARD_NSIOUTPUTSTREAM since you're not doing anything special. @@ +291,4 @@ > NS_DECL_NSIFILEOUTPUTSTREAM > > + nsFileOutputStream() > + : nsFileStreamBase() Unneeded @@ +295,5 @@ > + { } > + > + virtual ~nsFileOutputStream() > + { > + nsFileOutputStream::Close(); Simple Close() here is sufficient. @@ +343,5 @@ > + NS_DECL_NSIFILESTREAM > + NS_DECL_NSIFILEMETADATASTREAM > + > + nsFileStream() : nsFileStreamBase() {} > + virtual ~nsFileStream() { nsFileStream::Close(); } Simple Close() is enough here.

(In reply to ben turner [:bent] from comment #37) > Comment on attachment 621317 [details] [diff] [review] > patch for necko > > Review of attachment 621317 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks pretty good! > > ::: netwerk/base/public/nsIFileStreams.idl > @@ +222,5 @@ > > + * A file stream that allows you to get some metadata like size and > > + * last modified time. > > + */ > > +[scriptable, uuid(07f679e4-9601-4bd1-b510-cd3852edb881)] > > +interface nsIFileMetadataStream : nsISupports > > This isn't really a stream... renamed to nsIFileMetadata > > ::: netwerk/base/public/nsNetUtil.h > @@ +953,5 @@ > > do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID, &rv); > > if (NS_SUCCEEDED(rv)) { > > rv = in->Init(file, ioFlags, perm, behaviorFlags); > > if (NS_SUCCEEDED(rv)) > > + rv = CallQueryInterface(in, result); > > This is wrong, no need to QI. I think nsCOMPtr.forget can handle this now > though well, the .forget compiles, but it doesn't work correctly without QI, I get a lot of assertions: ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../../dist/include/nsCOMPtr.h, line 532 nsCOMPtr<nsIInputStream>::Assert_NoQueryNeeded()+0x00000084 nsGetterAddRefs<nsIInputStream>::~nsGetterAddRefs()+0x0000005F nsGetterAddRefs<nsIInputStream>::~nsGetterAddRefs()+0x00000015 _ZN7mozillaL12openPrefFileEP7nsIFile+0x00000095 _ZN7mozillaL23pref_InitInitialObjectsEv+0x00000709 mozilla::Preferences::Init()+0x000000ED ... I think it now works (w/o the necko patch) only because nsIInputStream and nsIFileInputStream is supported by the same C++ class Anyway, I found out that my patch would cause all file input streams to QI to nsIOutputStream and file output streams to QI to nsIInputStream. That's undesirable, so I fixed it, and it also fixed the assertion. btw, can the fact that the base class implement both input and output methods expose any security problems ? > > @@ +991,5 @@ > > do_CreateInstance(NS_LOCALFILEOUTPUTSTREAM_CONTRACTID, &rv); > > if (NS_SUCCEEDED(rv)) { > > rv = out->Init(file, ioFlags, perm, behaviorFlags); > > if (NS_SUCCEEDED(rv)) > > + rv = CallQueryInterface(out, result); > > Here too. see above comment > > @@ +1010,5 @@ > > do_CreateInstance(NS_SAFELOCALFILEOUTPUTSTREAM_CONTRACTID, &rv); > > if (NS_SUCCEEDED(rv)) { > > rv = out->Init(file, ioFlags, perm, behaviorFlags); > > if (NS_SUCCEEDED(rv)) > > + rv = CallQueryInterface(out, result); > > And here. see above comment > > @@ +1028,5 @@ > > + do_CreateInstance(NS_LOCALFILESTREAM_CONTRACTID, &rv); > > + if (NS_SUCCEEDED(rv)) { > > + rv = stream->Init(file, ioFlags, perm, behaviorFlags); > > + if (NS_SUCCEEDED(rv)) > > + rv = CallQueryInterface(stream, result); > > This can definitely use forget() yeah, fixed > > ::: netwerk/base/src/nsFileStreams.cpp > @@ +351,5 @@ > > NS_LOCALFILEINPUTSTREAM_CID) > > > > NS_INTERFACE_MAP_BEGIN(nsFileInputStream) > > +// NS_INTERFACE_MAP_ENTRY(nsFileStream) > > +// NS_INTERFACE_MAP_ENTRY(nsIInputStream) > > Hm. removed > > @@ +845,5 @@ > > + if (stream == nsnull) > > + return NS_ERROR_OUT_OF_MEMORY; > > + NS_ADDREF(stream); > > + nsresult rv = stream->QueryInterface(aIID, aResult); > > + NS_RELEASE(stream); > > Blegh. nsRefPtr and CallQueryInterface! removed altogether added NS_GENERIC_FACTORY_CONSTRUCTOR(nsFileStream) instead > > @@ +904,5 @@ > > + if (!mFD) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > +#if 0 > > ? reimplemented using PR_GetOpenFileInfo64() > > ::: netwerk/base/src/nsFileStreams.h > @@ +152,5 @@ > > + NS_IMETHOD Available(PRUint32* _retval) > > + { > > + return nsFileStreamBase::Available(_retval); > > + } > > + NS_IMETHOD Read(char* aBuf, PRUint32 aCount, PRUint32* _retval) > > I'd move this to the cpp ok > > @@ +184,4 @@ > > NS_IMETHOD Seek(PRInt32 aWhence, PRInt64 aOffset); > > > > + nsFileInputStream() > > + : nsFileStreamBase() > > This isn't really needed. removed |: nsFileStreamBase()| > > @@ -208,5 @@ > > public nsIFileOutputStream > > { > > public: > > NS_DECL_ISUPPORTS_INHERITED > > - NS_DECL_NSIOUTPUTSTREAM > > I think here you can just use NS_FORWARD_NSIOUTPUTSTREAM since you're not > doing anything special. ok > > @@ +291,4 @@ > > NS_DECL_NSIFILEOUTPUTSTREAM > > > > + nsFileOutputStream() > > + : nsFileStreamBase() > > Unneeded removed both lines > > @@ +295,5 @@ > > + { } > > + > > + virtual ~nsFileOutputStream() > > + { > > + nsFileOutputStream::Close(); > > Simple Close() here is sufficient. ok > > @@ +343,5 @@ > > + NS_DECL_NSIFILESTREAM > > + NS_DECL_NSIFILEMETADATASTREAM > > + > > + nsFileStream() : nsFileStreamBase() {} > > + virtual ~nsFileStream() { nsFileStream::Close(); } > > Simple Close() is enough here. ok, removed also the ctor

(In reply to Jan Varga [:janv] from comment #38) > Anyway, I found out that my patch would cause all file input streams to QI to > nsIOutputStream and file output streams to QI to nsIInputStream. That's > undesirable, so > I fixed it, and it also fixed the assertion. Yeah, maybe using the _CONDITIONAL QI macros would work here. You could have two bools in the base class that say whether it is in/out/in+out and then you could set those bools in the right classes' constructors. Something like that? > btw, can the fact that the base class implement both input and output methods > expose any security problems ? If you make it so that they don't QI when they shouldn't then I don't believe so.

(In reply to ben turner [:bent] from comment #39) > (In reply to Jan Varga [:janv] from comment #38) > > Anyway, I found out that my patch would cause all file input streams to QI to > > nsIOutputStream and file output streams to QI to nsIInputStream. That's > > undesirable, so > > I fixed it, and it also fixed the assertion. > > Yeah, maybe using the _CONDITIONAL QI macros would work here. You could have > two bools in the base class that say whether it is in/out/in+out and then > you could set those bools in the right classes' constructors. Something like > that? That would be nice, but for example, nsIInputStream and nsIFileInputStream must be both supported by nsFileInputStream C++ class, if we want to avoid the CallQueryInterface() in NS_NewLocalFileInputStream() So the base class defines both implementations (nsIInputStream and nsIOutputStream), but QueryInterface() in the base class supports only nsISeekableStream. > > > btw, can the fact that the base class implement both input and output methods > > expose any security problems ? > > If you make it so that they don't QI when they shouldn't then I don't > believe so. ok

(In reply to ben turner [:bent] from comment #36) > Comment on attachment 617475 [details] [diff] [review] > patch v0.4 > > Review of attachment 617475 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is the last! > > ::: dom/file/FileHandle.cpp > @@ +50,5 @@ > > +NS_IMPL_CYCLE_COLLECTION_CLASS(FileHandle) > > + > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(FileHandle, > > + nsDOMEventTargetHelper) > > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mFileStorage, > > You don't need the _AMBIGUOUS here fixed > > ::: dom/indexedDB/FileStream.cpp > @@ +14,5 @@ > > + > > +FileStream::FileStream() > > +: mBehaviorFlags(0), > > + mDeferredOpen(false), > > + mQuotaFile(nsnull) > > Looks like constructor/destructor could be in the header. fixed > > @@ +34,5 @@ > > + NS_INTERFACE_MAP_ENTRY(nsIFileStream) > > +NS_INTERFACE_MAP_END > > + > > +NS_IMETHODIMP > > +FileStream::Seek(PRInt32 aWhence, PRInt64 aOffset) > > In each of these methods it might be wise to assert that we are not doing > any io on the main thread. ok, done I discovered some main thread IO :) It should be all fixed now. > > @@ +55,5 @@ > > + case nsISeekableStream::NS_SEEK_END: > > + whence = SEEK_END; > > + break; > > + default: > > + whence = -1; > > Hm... What should happen here? Should we bail out? ok, replaced with |return NS_ERROR_INVALID_ARG;| > > @@ +58,5 @@ > > + default: > > + whence = -1; > > + } > > + > > + int rc = sqlite3_quota_fseek(mQuotaFile, aOffset, whence); > > This will truncate aOffset. Need to NS_ENSURE that aOffset < PR_INT32_MAX, > and we need to make sure we don't deal with bigger sizes anywhere else in > this code. added a check: // TODO: Add support for 64 bit file sizes, bug 752431 NS_ENSURE_TRUE(aOffset < PR_INT32_MAX, NS_ERROR_INVALID_ARG); > > We probably should file a bug to update this all to handle 64bit file sizes, > and we need to get the sqlite guys to change their api to do it. filed bug 752431 > > @@ +74,5 @@ > > + if (!mQuotaFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + int rc = sqlite3_quota_ftell(mQuotaFile); > > long is the return type. fixed > > @@ +82,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +FileStream::SetEOF() > > Just call Tell() in the beginning to skip the first three code chunks. done > > @@ +109,5 @@ > > + > > + if (mQuotaFile) { > > + int rc = sqlite3_quota_fclose(mQuotaFile); > > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > > + mQuotaFile = nsnull; > > I think you want to set mQuotaFile to null here regardless of whether close > fails, right? right, fixed > > @@ +116,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +FileStream::Available(PRUint32* aResult) > > Use Tell() again I added a new function sqlite3_quota_file_available() to test_quota.h/cpp So SQLite guys can optimize the implementation and possibly provide sqlite3_quota_file_available64() too. No need to use Tell() > > @@ +134,5 @@ > > + > > + int end = sqlite3_quota_ftell(mQuotaFile); > > + NS_ENSURE_TRUE(end >= 0, NS_BASE_STREAM_OSERROR); > > + > > + int result = end - cur; > > long here. And ensure that end >= cur, just in case something wacky happens. this now lives in test_quota.c > > @@ +154,5 @@ > > + if (!mQuotaFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + int bytesRead = sqlite3_quota_fread(aBuf, 1, aCount, mQuotaFile); > > This returns size_t, which is unsigned. fixed, I had to add sqlite3_quota_ferror() to test_quota.h Otherwise it's not possibile to distinguish between an error and the EOF > > @@ +177,5 @@ > > + *aNonBlocking = false; > > + return NS_OK; > > +} > > + > > + > > Nit: remove extra line done > > @@ +179,5 @@ > > +} > > + > > + > > +NS_IMETHODIMP > > +FileStream::Write(const char *aBuf, PRUint32 aCount, PRUint32 *aResult) > > Nit: * on left fixed > > @@ +188,5 @@ > > + if (!mQuotaFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + int cnt = sqlite3_quota_fwrite(aBuf, 1, aCount, mQuotaFile); > > this returns size_t fixed > > @@ +200,5 @@ > > + > > +NS_IMETHODIMP > > +FileStream::Flush() > > +{ > > + nsresult rv = DoPendingOpen(); > > Do you actually need to do this here? If someone calls Flush() before you've > opened the file then I think you could just return NS_OK immediately. Hmm, you are right. However, the necko file stream impl does open the file, should I change it anyway ? > > @@ +225,5 @@ > > +{ > > + return NS_ERROR_NOT_IMPLEMENTED; > > +} > > + > > + > > Nit: Remove extra line. done > > @@ +257,5 @@ > > + if (!mQuotaFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + PRInt64 rc = sqlite3_quota_file_size(mQuotaFile); > > So... if rc == -1 then the file isn't under quota management. That should be > impossible, right? I think you should just assert that rather than checking > in release builds. ok, fixed > > @@ +279,5 @@ > > + int rc = sqlite3_quota_file_mtime(mQuotaFile, &mtime); > > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > > + > > + PRInt64 s, s2us; > > + LL_I2L(s, mtime); > > No need to use these macros any longer. Yeah, I actually changed the interface to return milliseconds (instead of microseconds) So, no need to multiply here. > > @@ +288,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +FileStream::Sync() > > Same here, early return if file not opened yet. See my comment about FileStream::Flush() > > @@ +304,5 @@ > > + return NS_OK; > > +} > > + > > +void > > +FileStream::CleanUpOpen() > > You could move this to the header. done > > @@ +331,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +FileStream::DoPendingOpen() > > This could be inlined in the header too. done > > ::: dom/indexedDB/FileStream.h > @@ +30,5 @@ > > + NS_DECL_NSIFILESTREAM > > + > > + // nsIInputStream > > + NS_IMETHOD Close(void); > > + NS_IMETHOD Available(PRUint32 *_retval); > > Nit: * on left, names on next line. fixed > > @@ +34,5 @@ > > + NS_IMETHOD Available(PRUint32 *_retval); > > + NS_IMETHOD Read(char *aBuf, PRUint32 aCount, PRUint32 *_retval); > > + NS_IMETHOD ReadSegments(nsWriteSegmentFun aWriter, void *aClosure, > > + PRUint32 aCount, PRUint32 *_retval); > > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > > Nit: No NS_OUTPARAM fixed > > @@ +37,5 @@ > > + PRUint32 aCount, PRUint32 *_retval); > > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > > + > > + // nsIOutputStream > > + //NS_IMETHOD Close(void); > > Nit: comment that Close/IsNonBlocking is same as nsIInputStream done > > @@ +47,5 @@ > > + PRUint32 aCount, PRUint32 *_retval); > > + //NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > > + > > + FileStream(); > > + virtual ~FileStream(); > > Protected. do you mean to add a static Create() method and move ctor/dtor to the protected section ? > > ::: dom/indexedDB/IDBDatabase.cpp > @@ +155,5 @@ > > + const nsAString& aType) > > + : AsyncConnectionHelper(aDatabase, aRequest), > > + mName(aName), mType(aType) > > + { > > + } > > Nit: { } fixed > > @@ +159,5 @@ > > + } > > + > > + ~CreateFileHelper() > > + { > > + } > > Here too fixed > > @@ +171,5 @@ > > + nsString mName; > > + nsString mType; > > + > > + // Out-params. > > + nsRefPtr<FileInfo> mFileInfo; > > These should be released via ReleaseMainThreadObjects ok, fixed > > @@ +314,5 @@ > > IDBDatabase::OnUnlink() > > { > > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > +// NS_ASSERTION(!GetOwner() && !GetScriptOwner(), > > +// "Should have been cleared already!"); > > Nit: Uncomment (I've got those fixed in another bug) ok, will do > > @@ +737,5 @@ > > } > > > > +// nsIFileStorage > > +inline nsISupports* > > +IDBDatabase::StorageId() > > These are all virtual so they won't be inlined. Let's remove that inline > from each, and remove this comment too. ok, done > > @@ +757,5 @@ > > + > > +inline void > > +IDBDatabase::SetThreadLocals() > > +{ > > + IndexedDatabaseManager::SetCurrentWindow(GetOwner()); > > Assert GetOwner first here, it can become null. done > > @@ +881,5 @@ > > + mFileInfo = fileManager->GetNewFileInfo(); > > + NS_ENSURE_TRUE(mFileInfo, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + > > + nsCOMPtr<nsIFile> directory = fileManager->GetDirectory(); > > + NS_ENSURE_TRUE(directory, nsnull); > > Oops, these need NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR. One more below. done > > ::: dom/indexedDB/IDBFileHandle.cpp > @@ +45,5 @@ > > + > > + nsRefPtr<FileInfo> fileInfo(aFileInfo); > > + NS_ASSERTION(fileInfo, "Null pointer!"); > > + > > + nsRefPtr<IDBFileHandle> newFile(new IDBFileHandle); > > Nit: =, and make sure to call with () fixed > > @@ +53,5 @@ > > + newFile->mFileStorage = aDatabase; > > + newFile->mName = aName; > > + newFile->mType = aType; > > + > > + newFile->mFile = ::GetFile(fileInfo); > > Nit: Rather than :: why not just have a more descriptive name for your local > function? GetFileFor() ? GetFileForFileInfo() looks weird, too many F :) > > @@ +101,5 @@ > > +IDBFileHandle::GetDb(nsIIDBDatabase** aDB) > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + > > + nsCOMPtr<nsIIDBDatabase> database = do_QueryInterface(mFileStorage); > > Assert that this always succeeds. done > > ::: dom/indexedDB/IDBFileHandle.h > @@ +33,5 @@ > > + return mFileInfo; > > + } > > + > > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(IDBFileHandle, > > + FileHandle) > > This is unndeeded. You're not traversing or unlinking anything. ok, removed > > ::: dom/indexedDB/IDBObjectStore.cpp > @@ +818,5 @@ > > + > > + for (PRUint32 i = 0; i < array.Length(); i++) { > > + const PRInt64& id = array.ElementAt(i); > > + > > + nsRefPtr<FileInfo> fileInfo = fileManager->GetFileInfo(id); > > Just use array[i] instead of using an alias here. ok, fixed > > @@ +953,5 @@ > > uint32_t aData, > > void* aClosure) > > { > > + if (aTag == SCTAG_DOM_FILEHANDLE || > > + aTag == SCTAG_DOM_BLOB || aTag == SCTAG_DOM_FILE) { > > Nit: the first two should fit on the same line, right? fixed > > @@ +1168,5 @@ > > + nsCOMPtr<nsIDOMFileHandle> fileHandle = do_QueryInterface(supports); > > + if (fileHandle) { > > + nsRefPtr<FileInfo> fileInfo = fileHandle->GetFileInfo(); > > + if (!fileInfo) { > > + return false; > > These fail points could use warnings. done > > ::: dom/indexedDB/IDBTransaction.cpp > @@ +478,3 @@ > > { > > + if (!mCreatedFileInfos.Put(aBlob, aFileInfo)) { > > + NS_WARNING("Out of memory?"); > > Need to let caller know. fixed > > ::: dom/indexedDB/IDBTransaction.h > @@ +174,5 @@ > > GetOrCreateObjectStore(const nsAString& aName, > > ObjectStoreInfo* aObjectStoreInfo); > > > > + already_AddRefed<FileInfo> GetFileInfo(nsIDOMBlob* aBlob); > > + void AddFileInfo(nsIDOMBlob* aBlob, FileInfo* aFileInfo); > > This can fail, needs to propagate its result. fixed > > ::: dom/indexedDB/IndexedDatabase.h > @@ +71,2 @@ > > > > struct StructuredCloneReadInfo { > > Now need a constructor to null-init mDatabase. ok, but I had to separate these structs into StructuredCloneInfo.h fixed > > @@ +72,5 @@ > > struct StructuredCloneReadInfo { > > void Swap(StructuredCloneReadInfo& aCloneReadInfo) { > > mCloneBuffer.swap(aCloneReadInfo.mCloneBuffer); > > mFileInfos.SwapElements(aCloneReadInfo.mFileInfos); > > + mDatabase = aCloneReadInfo.mDatabase; > > This doesn't swap :) fixed > > @@ +88,3 @@ > > }; > > > > struct StructuredCloneWriteInfo { > > Now need a constructor to null-init mTransaction. fixed > > @@ +90,5 @@ > > struct StructuredCloneWriteInfo { > > void Swap(StructuredCloneWriteInfo& aCloneWriteInfo) { > > mCloneBuffer.swap(aCloneWriteInfo.mCloneBuffer); > > + mFiles.SwapElements(aCloneWriteInfo.mFiles); > > + mTransaction = aCloneWriteInfo.mTransaction; > > This doesn't swap either. fixed > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +541,2 @@ > > TransactionThreadPool* pool = TransactionThreadPool::Get(); > > + if (!(service || pool)) { > > Nit: !service && !pool fixed > > @@ +546,5 @@ > > for (PRUint32 index = 0; index < liveDatabases.Length(); index++) { > > IDBDatabase*& database = liveDatabases[index]; > > if (database->GetOwner() == aWindow && > > + (service && service->HasLockedFilesForStorage(database) || > > + pool && pool->HasTransactionsForDatabase(database))) { > > This needs more parenthesis around the two args to || yeah, fixed the warning > > @@ +590,5 @@ > > + count ? new WaitForTransactionsToFinishRunnable(op, count) > > + : new WaitForTransactionsToFinishRunnable(op); > > + > > + if (!count) { > > + runnable->Run(); > > This should never happen. Assert count == 1 or 2. Hmm, I think I added this on purpose. In theory, after app startup, there can be an opened database without any requests processed so far. Then this database is to be deleted. Note, that the delete request is processed on the IO thread, not transaction thread pool. So I think, TransactionThreadPool::Get() can return null here. > > @@ +598,3 @@ > > > > + if (service) { > > + nsAutoTArray<nsCOMPtr<nsIFileStorage>, 1> array; > > Here and below don't use nsAutoTArray, the elements are about to be swapped > and that means coyping, so we're just wasting stack space. Old code was > wrong. ok, fixed > > @@ +1261,5 @@ > > + // If the necko service (thread pool) gets the shutdown notification > > + // first then the sync loop won't be processed at all, otherwise it will > > + // lock the main thread until all IDB file storages are finished. > > + > > + nsAutoTArray<nsCOMPtr<nsIFileStorage>, 50> liveDatabases; > > Don't use nsAutoTArray here. Do call SetCapacity first though, with > mLiveDatabases.Count(). ok, fixed > > @@ +1273,5 @@ > > + if (!service->WaitForAllStoragesToComplete(liveDatabases, runnable)) { > > + NS_WARNING("Failed to wait for databases to complete!"); > > + } > > + > > + nsIThread *thread = NS_GetCurrentThread(); > > Nit: * on left fixed > > @@ +1274,5 @@ > > + NS_WARNING("Failed to wait for databases to complete!"); > > + } > > + > > + nsIThread *thread = NS_GetCurrentThread(); > > + while (runnable->mStillBusy) { > > Nit: I'd prefer a simple method IsBusy() to directly accessing the member. ok, fixed > > @@ +1276,5 @@ > > + > > + nsIThread *thread = NS_GetCurrentThread(); > > + while (runnable->mStillBusy) { > > + if (!NS_ProcessNextEvent(thread)) { > > + NS_WARNING("Something bad happened!"); > > NS_ERROR("Failed to process next event!"); fixed

I still need to address the second round of review comments. Anyway, we could try to get reviews for necko changes in the meantime. Basically, we added a new read-write file stream and also the possibility to raise/lower number of stream transport threads. FileService increases (lazily) the max number of threads by one.

(In reply to Jan Varga [:janv] from comment #38) > > ::: netwerk/base/public/nsNetUtil.h > > @@ +953,5 @@ > > > do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID, &rv); > > > if (NS_SUCCEEDED(rv)) { > > > rv = in->Init(file, ioFlags, perm, behaviorFlags); > > > if (NS_SUCCEEDED(rv)) > > > + rv = CallQueryInterface(in, result); > > > > This is wrong, no need to QI. I think nsCOMPtr.forget can handle this now > > though > > well, the .forget compiles, but it doesn't work correctly > without QI, I get a lot of assertions: > > ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', > file ../../../dist/include/nsCOMPtr.h, line 532 > nsCOMPtr<nsIInputStream>::Assert_NoQueryNeeded()+0x00000084 > nsGetterAddRefs<nsIInputStream>::~nsGetterAddRefs()+0x0000005F > nsGetterAddRefs<nsIInputStream>::~nsGetterAddRefs()+0x00000015 > _ZN7mozillaL12openPrefFileEP7nsIFile+0x00000095 > _ZN7mozillaL23pref_InitInitialObjectsEv+0x00000709 > mozilla::Preferences::Init()+0x000000ED > ... If you are getting this assertion that likely means that your object inherits nsIInputStream twice. That is generally something that you want to avoid since it can mean that nsIInputStream* a = ...; nsIInputStream* b = ...; if (a == b) { ... } can test false even if a and b refer to the same object. It's bugs like that that the above assertion is trying to catch. But the assertion won't catch all cases when that can happen and so it's not something that we should rely on. Looking at the patch, it looks like you are making nsFileStreamBase inherit nsIInputStream, and then subclasses of nsFileStreamBase *also* inherit nsIInputStream. You need to make subclasses not directly inherit nsIInputStream, and instead get it from the base class.

Oh, I see, the subclasses inherit nsIFileInputStream and thus indirectly nsIInputStream. Why do you need the base-class to implement nsIInputStream? Can you simply make the base-class have the functions that you've added, but not actually inherit nsIInputStream? That way subclasses can still forward to the baseclass but we don't end up with the problem of inheriting interfaces multiple times.

(In reply to Jonas Sicking (:sicking) from comment #44) > Oh, I see, the subclasses inherit nsIFileInputStream and thus indirectly > nsIInputStream. > > Why do you need the base-class to implement nsIInputStream? Can you simply > make the base-class have the functions that you've added, but not actually > inherit nsIInputStream? That way subclasses can still forward to the > baseclass but we don't end up with the problem of inheriting interfaces > multiple times. I'll try it, we have to forward to the base class anyway

Comment on attachment 621910 [details] [diff] [review] patch v0.6 Review of attachment 621910 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIFileStreams.idl @@ +234,5 @@ > + /** > + * File last modified time in miliseconds from midnight (00:00:00), > + * January 1, 1970 Greenwich Mean Time (GMT). > + */ > + readonly attribute long long lastModified; Why not use PRTime here (and µsec instead of msec)? ::: netwerk/base/public/nsIStreamTransportService.idl @@ +101,5 @@ > + > + /** > + * Raise the maximum number of active threads. > + */ > + [noscript] void raiseThreadLimit(); Why is this noscript? And by how much will it raise the number of threads? It is not clear to me what the use of this functoin is... @@ +104,5 @@ > + */ > + [noscript] void raiseThreadLimit(); > + > + /** > + * Lower the maximum number of active threads. Will this ever lower them to zero? Or is there a lower limit? And what will happen if the # of threads is already at the limit? ::: netwerk/base/src/nsFileStreams.cpp @@ +912,5 @@ > + if (PR_GetOpenFileInfo64(mFD, &info) == PR_FAILURE) { > + return NS_BASE_STREAM_OSERROR; > + } > + > + PRInt64 modTime = PRInt64(info.modifyTime); PRTime? ::: netwerk/base/src/nsStreamTransportService.cpp @@ +553,5 @@ > + PRUint32 threadLimit; > + nsresult rv = mPool->GetThreadLimit(&threadLimit); > + NS_ENSURE_SUCCESS(rv, rv); > + > + NS_ASSERTION(threadLimit > 4, "Badly nested raise/lower thread limit!"); OK, so you should document in the IDL that raiseThreadLimit/lowerThreadLimit should always be paired

(In reply to Jan Varga [:janv] from comment #45) > (In reply to Jonas Sicking (:sicking) from comment #44) > > Oh, I see, the subclasses inherit nsIFileInputStream and thus indirectly > > nsIInputStream. > > > > Why do you need the base-class to implement nsIInputStream? Can you simply > > make the base-class have the functions that you've added, but not actually > > inherit nsIInputStream? That way subclasses can still forward to the > > baseclass but we don't end up with the problem of inheriting interfaces > > multiple times. > > I'll try it, we have to forward to the base class anyway works ok, the changes now look much simpler, thanks for the hint

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #46) > Comment on attachment 621910 [details] [diff] [review] > patch v0.6 > > Review of attachment 621910 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/public/nsIFileStreams.idl > @@ +234,5 @@ > > + /** > > + * File last modified time in miliseconds from midnight (00:00:00), > > + * January 1, 1970 Greenwich Mean Time (GMT). > > + */ > > + readonly attribute long long lastModified; > > Why not use PRTime here (and µsec instead of msec)? I would rather use msec to be consistent with nsIFile > > ::: netwerk/base/public/nsIStreamTransportService.idl > @@ +101,5 @@ > > + > > + /** > > + * Raise the maximum number of active threads. > > + */ > > + [noscript] void raiseThreadLimit(); > > Why is this noscript? And by how much will it raise the number of threads? > It is not clear to me what the use of this functoin is... Added documentation for this > > @@ +104,5 @@ > > + */ > > + [noscript] void raiseThreadLimit(); > > + > > + /** > > + * Lower the maximum number of active threads. > > Will this ever lower them to zero? Or is there a lower limit? And what will > happen if the # of threads is already at the limit? Added documentation for this > > ::: netwerk/base/src/nsFileStreams.cpp > @@ +912,5 @@ > > + if (PR_GetOpenFileInfo64(mFD, &info) == PR_FAILURE) { > > + return NS_BASE_STREAM_OSERROR; > > + } > > + > > + PRInt64 modTime = PRInt64(info.modifyTime); > > PRTime? again, see my comment about nsIFile > > ::: netwerk/base/src/nsStreamTransportService.cpp > @@ +553,5 @@ > > + PRUint32 threadLimit; > > + nsresult rv = mPool->GetThreadLimit(&threadLimit); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + NS_ASSERTION(threadLimit > 4, "Badly nested raise/lower thread limit!"); > > OK, so you should document in the IDL that raiseThreadLimit/lowerThreadLimit > should always be paired /** * Raise the maximum number of active threads by one. * * Calling this method won't create any additional thread synchronously. * It will be only created when it's needed (lazily). * * Used by mozilla::dom::file::FileService to increase the maximum number * of active threads in the thread pool for asynchronous file IO. */ void raiseThreadLimit(); /** * Lower the maximum number of active threads by one. * lowerThreadLimit() should be always paired with raiseThreadLimit(). * * Calling this method won't destroy any already running thread * synchronously. It will be only destroyed when it's done with * currently running event. * * This will never lower the maximum number of active threads beyond * the internal limit. * * @throws NS_ERROR_UNEXPECTED * When trying to lower the maximum number of active threads * beyond the internal limit (for example in the case of badly * nested calls) */ void lowerThreadLimit();

(In reply to ben turner [:bent] from comment #33) > ::: dom/file/nsIDOMLockedFile.idl > @@ +34,5 @@ > > + [optional /* none */] in jsval parameters); > > + > > + [implicit_jscontext] > > + nsIDOMFileRequest > > + readAsArrayBuffer(in long size); > > These should be 'unsigned long long' > > @@ +37,5 @@ > > + nsIDOMFileRequest > > + readAsArrayBuffer(in long size); > > + > > + nsIDOMFileRequest > > + readAsText(in long size, [optional] in DOMString encoding); > > 'unsigned long long' I agree with the unsigned, but do you we really need to provide such big reads ? even nsIInputStream::read(..., count) is "only" unsigned long Current implementation uses a string buffer for reads and buffer.GetMutableData() takes only PRUint32

The fact that a lot of our old necko interfaces use 32bit integers is a big pain and something we're working on fixing. Let's use 64bit numbers everywhere for file handling, otherwise we're bound to run into 4/2GB limits sooner or later.

It's not a problem to handle files sized over 4 GB. I asked if we really need to read chunks sized over 4GB.

readAsArrayBuffer/readAsText asynchronously return a JS array buffer/string the max size of these is limited by uint32_t AFAIK am I missing something ?

It doesn't surprise me if we have various places that are limited to 32bit. However we really should make all filehandling APIs be based around 64bit. It always bites us when we don't, and it's nice to keep file-stuff consistent. That said, I might be talking out of my ass since I'm not entirely sure where in the patch we are talking about.

ok, so is this just about avoiding future FileHandle API changes and we will support only 4GB chunks ? (until strings/array buffers support larger sizes on 64bit platforms)

Ben, it would be great if we had this in SQLite tree too

(In reply to ben turner [:bent] from comment #33) > Comment on attachment 617475 [details] [diff] [review] > patch v0.4 > > Review of attachment 617475 [details] [diff] [review]: > ----------------------------------------------------------------- > > Still not quite done, just have the IndexedDB changes left. > > ::: dom/file/DOMFile.cpp > @@ +39,5 @@ > > + return NS_OK; > > +} > > + > > +already_AddRefed<nsIDOMBlob> > > +DOMFile::CreateSlice(PRUint64 aStart, PRUint64 aLength, > > Can you asset main thread here? ok, done > > @@ +48,5 @@ > > + return t.forget(); > > +} > > + > > +NS_IMETHODIMP > > +DOMFile::GetMozFullPathInternal(nsAString &aFilename) > > Can you assert main thread here? ok, done > > ::: dom/file/DOMFile.h > @@ +14,5 @@ > > +#include "LockedFile.h" > > + > > +BEGIN_FILE_NAMESPACE > > + > > +class DOMFile : public nsDOMFileBaseCC > > Maybe just File? ok, done > > @@ +35,5 @@ > > + > > + // Create as a stored file > > + DOMFile(const nsAString& aName, const nsAString& aContentType, > > + PRUint64 aLength, nsIFile* aFile, FileInfo* aFileInfo, > > + LockedFile* aLockedFile) > > Nit: reorder last two args so it's the same as the other. done > > @@ +45,5 @@ > > + NS_ASSERTION(mLockedFile, "Null locked file!"); > > + mFileInfos.AppendElement(aFileInfo); > > + } > > + > > + virtual ~DOMFile() > > protected. done > > @@ +49,5 @@ > > + virtual ~DOMFile() > > + { } > > + > > + // Overrides > > + NS_IMETHOD GetMozFullPathInternal(nsAString& aFullPath); > > Nit: Name on next line, and for the next one. fixed > > @@ +55,5 @@ > > + > > +protected: > > + // Create slice > > + DOMFile(const DOMFile* aOther, PRUint64 aStart, PRUint64 aLength, > > + const nsAString& aContentType) > > This one should go in the cpp I think. ok, done > > @@ +67,5 @@ > > + if (mStoredFile) { > > + FileInfo* fileInfo; > > + > > + if (!mozilla::dom::indexedDB::IndexedDatabaseManager::IsClosed()) { > > + mozilla::dom::indexedDB::IndexedDatabaseManager::FileMutex().Lock(); > > You should use an autolock ok, done > > @@ +87,5 @@ > > + virtual already_AddRefed<nsIDOMBlob> > > + CreateSlice(PRUint64 aStart, PRUint64 aLength, > > + const nsAString& aContentType); > > + > > + virtual bool IsStoredFile() > > Nit: IsStoredFile on next line. And the others below. fixed > > @@ +105,5 @@ > > + > > +private: > > + nsCOMPtr<nsIFile> mFile; > > + bool mWholeFile; > > + bool mStoredFile; > > Nit: In general i'd put bools at the end so they pack nicely. ok, done > > ::: dom/file/DOMFileHandle.h > @@ +17,5 @@ > > +{ > > +public: > > + NS_DECL_ISUPPORTS_INHERITED > > + > > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DOMFileHandle, FileHandle) > > You could use the _NO_UNLINK version of this macro, I guess. Not convinced > it makes things more clear, so whatever you'd like. actually, we don't need this at all, as you pointed out at IDBFileHandle.h > > @@ +21,5 @@ > > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DOMFileHandle, FileHandle) > > + > > +#if 0 > > + static already_AddRefed<DOMFileHandle> > > + Create(nsDOMDeviceStorage* aDeviceStorage, > > Eh? fixed > > ::: dom/file/FileHandle.cpp > @@ +37,5 @@ > > + void ReleaseMainThreadObjects() > > + { > > + mFileHandle = nsnull; > > + > > + FileHelper::ReleaseMainThreadObjects(); > > Probably want to call MetadataHelper::ReleaseMainThreadObjects. Even if that > doesn't exist it might some day in the future. ok, fixed > > @@ +58,5 @@ > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > + > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(FileHandle, > > + nsDOMEventTargetHelper) > > + // Don't unlink mFileStorage! > > Is this important? no, removed the comment and added: NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mFileStorage) > > @@ +105,5 @@ > > + > > + LockedFile::Mode mode = LockedFile::READ_ONLY; > > + if (aOptionalArgCount) { > > + if (aMode.EqualsLiteral("readwrite")) { > > + mode = LockedFile::READ_WRITE; > > Nit: avoid double assign. ok, fixed > > @@ +129,5 @@ > > + LockedFile::Create(this, LockedFile::READ_ONLY, LockedFile::PARALLEL); > > + NS_ENSURE_TRUE(lockedFile, NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR); > > + > > + nsRefPtr<FileRequest> fileRequest = > > + FileRequest::Create(GetOwner(), lockedFile); > > nsDOMEventTargetHelper no longer holds a strong ref to its owner, so you > should null-check. fixed, also in LockedFile.cpp > > @@ +147,5 @@ > > + > > +NS_IMETHODIMP_(PRInt64) > > +FileHandle::GetFileId() > > +{ > > + return -1; > > Want to warn here? see my comment below > > @@ +153,5 @@ > > + > > +NS_IMETHODIMP_(mozilla::dom::indexedDB::FileInfo*) > > +FileHandle::GetFileInfo() > > +{ > > + return nsnull; > > And here? I don't think so, it should be legitimate to pass non IDB file handles to IDBObjectStore.add() or .put() In that case we just throw, see IDBObjectStore::StructuredCloneWriteCallback() > > ::: dom/file/FileHandle.h > @@ +33,5 @@ > > +{ > > + friend class FileService; > > + friend class LockedFile; > > + friend class FinishHelper; > > + friend class FileHelper; > > I think Windows (MSVC) barfs on these. Need to also forward declare them > above. ok, done > > @@ +39,5 @@ > > +public: > > + NS_DECL_ISUPPORTS_INHERITED > > + NS_DECL_NSIDOMFILEHANDLE > > + > > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED( > > Don't think you need the SCRIPT_HOLDER thing here, or the TRACE macros in > the cpp. You're inheriting, but not extending, so just use the regular > INHERITED macros. ok, removed > > @@ +43,5 @@ > > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED( > > + FileHandle, > > + nsDOMEventTargetHelper) > > + > > + const nsAString& Name() const > > Nit: Function names on following line. fixed > > ::: dom/file/FileHelper.cpp > @@ +32,5 @@ > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > +} > > + > > +FileHelper::~FileHelper() > > Looks like you don't have any code dealing with mListener or mRequest? do you mean that mListener and mRequest are not released via a proxy in the dtor ? they are both thread safe > > @@ +34,5 @@ > > +} > > + > > +FileHelper::~FileHelper() > > +{ > > + if (!NS_IsMainThread()) { > > Hm... This really shouldn't happen, right? ReleaseMainThreadObjects should > be all you need to keep these objects from releasing on the wrong thread. > Maybe put a warning or assertion in here? ok, it seem it's even possible to allow this dtor only on the main thread > > @@ +45,5 @@ > > + FileRequest* fileRequest; > > + mFileRequest.forget(&fileRequest); > > + > > + nsCOMPtr<nsIThread> mainThread; > > + NS_GetMainThread(getter_AddRefs(mainThread)); > > No need to do this if all the pointers are null. this code doesn't exist anymore > > @@ +65,5 @@ > > + } > > +} > > + > > +NS_IMPL_THREADSAFE_ISUPPORTS1(FileHelper, > > + nsIRequestObserver) > > Nit: this can probably fit on one line fixed > > @@ +70,5 @@ > > + > > +nsresult > > +FileHelper::Enqueue() > > +{ > > + FileService* service = FileService::GetOrCreate(); > > GetOrCreate could fail, right? right, fixed > > @@ +99,5 @@ > > + > > + rv = DoAsyncRun(stream); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + mListener = aListener; > > Hm, do you want to have this set before calling DoAsyncRun? You'd need to > null it back out if DoAsyncRun fails, but might be nicer for subclasses. I fixed this in the meantime. If DoAsyncRun fails then Finish() is called, and that clears mListener. > > @@ +114,5 @@ > > +NS_IMETHODIMP > > +FileHelper::OnStopRequest(nsIRequest* aRequest, nsISupports* aCtxt, > > + nsresult aStatus) > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > Add this to other methods too? Helps make code readable and prevents > accidental bugs. ok, added > > @@ +131,5 @@ > > +{ > > + if (mLockedFile->IsAborted()) { > > + NS_ASSERTION(mRequest, "Should have a request!\n"); > > + > > + nsresult rv = mRequest->Cancel(NS_ERROR_NOT_AVAILABLE); > > Usually we use NS_BINDING_ABORTED to signal aborts. ok, fixed > > @@ +147,5 @@ > > + > > +void > > +FileHelper::OnStreamClose() > > +{ > > + Finish(); > > Inline ok, done > > @@ +153,5 @@ > > + > > +void > > +FileHelper::OnStreamDestroy() > > +{ > > + Finish(); > > Inline ok, done > > @@ +176,5 @@ > > + return NS_OK; > > +} > > + > > +void > > +FileHelper::ReleaseMainThreadObjects() > > What about mListener and mRequest? Seems better to put these all in one > place. Well, mListener and mRequest are both thread safe > > @@ +215,5 @@ > > + gCurrentLockedFile = oldLockedFile; > > + > > + mLockedFile->OnRequestFinished(); > > + > > + mListener->OnFileHelperComplete(this); > > So... There's three things happening here: > > mFileRequest->NotifyHelperCompleted(this); > mLockedFile->OnRequestFinished(); > mListener->OnFileHelperComplete(this); > > Is there any way that this could be simplified? Well, I haven't figured out a way to do it nicely > > @@ +223,5 @@ > > + > > + NS_ASSERTION(!(mFileStorage || mLockedFile || mFileRequest), "Subclass " > > + "didn't call FileHelper::ReleaseMainThreadObjects!"); > > + > > + mFinished = true; > > It's not possible for anything in this function to spin an event loop or > otherwise call back into Finish() is it? If it's possible then you should > move this to right after you check it above. There's no early "return" in the body of this method, so it should be safe to move it to the beginning. ok, fixed > > ::: dom/file/FileHelper.h > @@ +36,5 @@ > > + */ > > +class FileHelper : public nsIRequestObserver > > +{ > > + friend class FileRequest; > > + friend class FileOutputStreamWrapper; > > Forward declare ok, fixed > > ::: dom/file/FileRequest.cpp > @@ +54,5 @@ > > + > > +void > > +FileRequest::OnProgress(PRUint64 aProgress, PRUint64 aProgressMax) > > +{ > > + FireProgressEvent(aProgress, aProgressMax); > > Inline. moved to the header > > @@ +81,5 @@ > > + JSAutoRequest ar(cx); > > + JSAutoEnterCompartment ac; > > + if (ac.enter(cx, global)) { > > + jsval result; > > + aFileHelper->GetSuccessResult(cx, &result); > > This can fail, in which case you probably want to fire an error instead yeah, I fixed this in the meantime > > @@ +145,5 @@ > > + nsRefPtr<nsDOMProgressEvent> event = new nsDOMProgressEvent(nsnull, nsnull); > > + nsresult rv = event->InitProgressEvent(NS_LITERAL_STRING("progress"), > > + false, false, false, aLoaded, aTotal); > > + if (NS_FAILED(rv)) { > > + return; > > I'd say you still want a warning, so NS_ENSURE_SUCCESS(rv,) will do what you > want. ok, fixed > > @@ +150,5 @@ > > + } > > + > > + rv = event->SetTrusted(true); > > + if (NS_FAILED(rv)) { > > + return; > > Here too. fixed > > @@ +154,5 @@ > > + return; > > + } > > + > > + bool dummy; > > + DispatchEvent(static_cast<nsIDOMProgressEvent*>(event), &dummy); > > And you probably want it here too. ok, fixed > > ::: dom/file/FileRequest.h > @@ +24,5 @@ > > +public: > > + NS_DECL_ISUPPORTS_INHERITED > > + NS_DECL_NSIDOMFILEREQUEST > > + NS_FORWARD_NSIDOMDOMREQUEST(DOMRequest::) > > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(FileRequest, > > You don't need the SCRIPT_HOLDER stuff ok, removed > > @@ +44,5 @@ > > + ~FileRequest(); > > + > > + void FireProgressEvent(PRUint64 aLoaded, PRUint64 aTotal); > > + > > + virtual void RootResultVal() > > Since this is virtual I'd un-inline it done > > ::: dom/file/FileService.cpp > @@ +56,5 @@ > > + NS_ENSURE_SUCCESS(rv, nsnull); > > + > > + gInstance = service.forget(); > > + > > + ClearOnShutdown(&gInstance); > > I'm not really happy using this... It comes after the "xpcom-shutdown" and > "xpcom-shutdown-threads" notification, and it will release tons of things > (runnables, other xpcom objects, maybe dom objects?). Those released objects > may expect XPCOM to still be around, or at the very least to continue > processing events. Plus, before deleting I'd hope we could call > WaitForAllStoragesToComplete or something. > ok, all done > If you think it's impossible for that to happen then we should at the very > least assert loudly. I think leaking until late in shutdown is totally > possible though. > > Seems like we should use the observer service here. > > @@ +112,5 @@ > > + > > + if (!(modeIsWrite ? lockedForWriting : lockedForReading)) { > > + modeIsWrite ? fileStorageInfo->LockFileForWriting(fileName) > > + : fileStorageInfo->LockFileForReading(fileName); > > + } > > This is a little too cute for my taste. Can you expand this? ok, done > > @@ +147,5 @@ > > + > > + fileStorageInfo->RemoveLockedFileQueueFor(aLockedFile); > > + > > + if (!fileStorageInfo->HasRunningLockedFiles()) { > > + mFileStorageInfos.Remove(storageId); > > Might want an #ifdef DEBUG block here to set storageId to null since it is > dead now. ok, done > > @@ +184,5 @@ > > + if (!mFileStorageInfos.Get(aFileStorage->StorageId(), &fileStorageInfo)) { > > + return; > > + } > > + > > + nsAutoTArray<nsRefPtr<LockedFile>, 50> lockedFiles; > > 50 seems a little large here... 10 ? > > @@ +212,5 @@ > > +FileService::MaybeFireCallback(PRUint32 aCallbackIndex) > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + > > + StoragesCompleteCallback& callback = mCompleteCallbacks[aCallbackIndex]; > > Hm... Can any of these callbacks cause mCompleteCallbacks to be modified? > That would be very bad given the way you're doing this here. TODO > > @@ +229,5 @@ > > + } > > +} > > + > > +inline > > +FileService::LockedFileQueue::LockedFileQueue(LockedFile* aLockedFile) > > You should add the 'inline' keyword to the header. For all the rest of these > too. ok, done > > @@ +243,5 @@ > > + > > + if (mLockedFile->mRequestMode == LockedFile::PARALLEL) { > > + nsresult rv = aFileHelper->AsyncRun(this); > > + if (NS_FAILED(rv)) { > > + NS_WARNING("Failed to asynchronously run helper!"); > > Hm, why not propagate this back to the caller? fixed > > @@ +261,5 @@ > > + > > + mQueue.RemoveElementAt(index); > > + } > > + else { > > + mCurrentHelper = nsnull; > > Assert that mCurrentHelper == aFileHelper done > > @@ +268,5 @@ > > + } > > +} > > + > > +void > > +FileService::LockedFileQueue::ProcessQueue() > > I think this should return an nsresult too. fixed > > @@ +289,5 @@ > > +{ > > + nsRefPtr<LockedFileQueue>* lockedFileQueue = > > + mLockedFileQueues.AppendElement(); > > + *lockedFileQueue = new LockedFileQueue(aLockedFile); > > + return *lockedFileQueue; > > Up to you, but I find 'return lockedFileQueue->get()' to be more readable. ok, fixed > > @@ +316,5 @@ > > + return info; > > +} > > + > > +void > > +FileService::FileStorageInfo::RemoveLockedFileQueueFor(LockedFile* aLockedFile) > > How about a comment on why this loop is more complicated than it seems like > it ought to be (rebuilding lock lists because multiple lockedfile objects > may lock same file name). ok, done > > @@ +369,5 @@ > > + > > +inline bool > > +FileService::FileStorageInfo::HasRunningLockedFiles() > > +{ > > + return mLockedFileQueues.Length() > 0; > > Put this in the header? done > > Nit: !IsEmpty() seems more readable. yeah, replaced with !IsEmpty() > > ::: dom/file/FileService.h > @@ +28,5 @@ > > + // Returns a non-owning reference! > > + static FileService* Get(); > > + > > + nsresult Enqueue(LockedFile* aLockedFile, > > + FileHelper* aFileHelper = nsnull); > > Nit: Let's lose the optional arg ok, fixed > > @@ +46,5 @@ > > + { > > + friend class FileService; > > + > > + public: > > + LockedFileQueue(LockedFile* aLockedFile); > > Is this only created by the FileService? If so hide this in private, > FileService is a friend already. ok, fixed > > @@ +48,5 @@ > > + > > + public: > > + LockedFileQueue(LockedFile* aLockedFile); > > + > > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(LockedFileQueue) > > Hm. This is only safe because your base class has a pure virtual > addref/release (NS_METHOD vs. NS_IMETHOD mismatch with this macro). I think > you should manually declare here: > > NS_IMETHOD_(nsrefcnt) AddRef(); > NS_IMETHOD_(nsrefcnt) Release(); > > Then just use the standard NS_IMPL_THREADSAFE_ADDREF/RELEASE macros in the > cpp. ok, had to declare mRefCnt and NS_DECL_OWNINGTHREAD too > > @@ +58,5 @@ > > + private: > > + void ProcessQueue(); > > + > > + nsRefPtr<LockedFile> mLockedFile; > > + nsAutoTArray<nsRefPtr<FileHelper>, 10> mQueue; > > Hm... Where'd 10 come from? converted to nsTArray > > @@ +75,5 @@ > > + > > + public: > > + LockedFileQueue* CreateLockedFileQueue(LockedFile* aLockedFile); > > + > > + LockedFileQueue* GetLockedFileQueueFor(LockedFile* aLockedFile); > > Nit: Lose the "For", it's redundant. In several other methods below as well. you don't like the for ? :) ok, removed > > @@ +100,5 @@ > > + { > > + mFilesWriting.AppendElement(aFileName); > > + } > > + > > + bool IsFileLockedForReading(const nsAString& aFileName) > > Hm. These are O(n) searches... Possible that that could get slow, right? converted nsTArray to nsTHashtable luckily, nsTHashtable is becoming infallible (bug 734847) > > ::: dom/file/FileStream.cpp > @@ +21,5 @@ > > +#endif > > + > > +USING_FILE_NAMESPACE > > + > > +FileStream::FileStream() > > Looks like this could go in the header. > > @@ +28,5 @@ > > + mFile(nsnull) > > +{ > > +} > > + > > +FileStream::~FileStream() > > This too. > > @@ +68,5 @@ > > + default: > > + whence = -1; > > + } > > + > > + int rc = fseek(mFile, aOffset, whence); > > Hm. I really think we should use 64bit length apis here... Hate to say it, > but we've solved this problem already. We have necko's file streams, which > only do reading or writing, but maybe we could reuse that somehow? > Otherwise, we have NSPR stuff that gets this right across platforms, and we > should use it I think. > > @@ +92,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +FileStream::SetEOF() > > You could skip the first three chunks of this function by just calling Tell > > @@ +129,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +FileStream::Available(PRUint32* aResult) > > Could skip early stuff by calling Tell > > @@ +167,5 @@ > > + if (!mFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + int bytesRead = fread(aBuf, 1, aCount, mFile); > > All the docs I have say this returns a size_t > > @@ +180,5 @@ > > +NS_IMETHODIMP > > +FileStream::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure, > > + PRUint32 aCount, PRUint32* aResult) > > +{ > > + return NS_ERROR_NOT_IMPLEMENTED; > > NS_NOTREACHED too, here and below. > > @@ +200,5 @@ > > + if (!mFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + int cnt = fwrite(aBuf, 1, aCount, mFile); > > This returns size_t > > @@ +240,5 @@ > > +} > > + > > +NS_IMETHODIMP > > +FileStream::Init(nsIFile* aFile, const nsAString& aMode, > > + PRInt32 aBehaviorFlags) > > Nit: Indentation is weird here. > > @@ +242,5 @@ > > +NS_IMETHODIMP > > +FileStream::Init(nsIFile* aFile, const nsAString& aMode, > > + PRInt32 aBehaviorFlags) > > +{ > > + NS_ENSURE_TRUE(!mFile, NS_ERROR_ALREADY_INITIALIZED); > > This will never be exposed to script, right? So let's just ASSERT, not > ENSURE. For both of these. > > @@ +269,5 @@ > > + if (!mFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + struct stat buffer; > > Nit: no need for 'struct' > > @@ +274,5 @@ > > + int rc = fstat(fileno(mFile), &buffer); > > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > > + > > + PRInt64 s; > > + LL_I2L(s, buffer.st_size); > > We don't use the LL_ macros any more. > > @@ +290,5 @@ > > + if (!mFile) { > > + return NS_BASE_STREAM_CLOSED; > > + } > > + > > + struct stat buffer; > > Nit: no need for 'struct'. > > @@ +295,5 @@ > > + int rc = fstat(fileno(mFile), &buffer); > > + NS_ENSURE_TRUE(rc == 0, NS_BASE_STREAM_OSERROR); > > + > > + PRInt64 s, s2us; > > + LL_I2L(s, buffer.st_mtime); > > No need for these. > > @@ +304,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +FileStream::Sync() > > Replace first few chunks with Flush() > > @@ +327,5 @@ > > + return NS_OK; > > +} > > + > > +void > > +FileStream::CleanUpOpen() > > This can be inlined in the header. > > @@ +353,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +FileStream::DoPendingOpen() > > Put this in the header. > > ::: dom/file/FileStream.h > @@ +28,5 @@ > > + NS_DECL_NSISEEKABLESTREAM > > + NS_DECL_NSIFILESTREAM > > + > > + // nsIInputStream > > + NS_IMETHOD Close(void); > > Nit: nix the void > > @@ +29,5 @@ > > + NS_DECL_NSIFILESTREAM > > + > > + // nsIInputStream > > + NS_IMETHOD Close(void); > > + NS_IMETHOD Available(PRUint32 *_retval); > > Nit: Here and below, * on the left. > > @@ +33,5 @@ > > + NS_IMETHOD Available(PRUint32 *_retval); > > + NS_IMETHOD Read(char *aBuf, PRUint32 aCount, PRUint32 *_retval); > > + NS_IMETHOD ReadSegments(nsWriteSegmentFun aWriter, void *aClosure, > > + PRUint32 aCount, PRUint32 *_retval); > > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > > Nit: No need for NS_OUTPARAM. below too. > > @@ +36,5 @@ > > + PRUint32 aCount, PRUint32 *_retval); > > + NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > > + > > + // nsIOutputStream > > + //NS_IMETHOD Close(void); > > Nit: Rather than this just put something about Close() being identical to > nsIInputStream. IsNonBlocking too. > > @@ +46,5 @@ > > + PRUint32 aCount, PRUint32 *_retval); > > + //NS_IMETHOD IsNonBlocking(bool *_retval NS_OUTPARAM); > > + > > + FileStream(); > > + virtual ~FileStream(); > > Nit: protected destructor I dropped dom/file/FileStream.h/cpp entirely DOMFileHandle is now using necko streams Anyway, I tried to apply these comments on dom/indexedDB/FileStream.h/cpp too > > ::: dom/file/FileStreamWrappers.cpp > @@ +77,5 @@ > > + mOffset(aOffset), > > + mLimit(aLimit), > > + mFlags(aFlags), > > + mFirstTime(true) > > +{ > > Nit: Assert non-null filestread/filehelper good idea, fixed > > @@ +98,5 @@ > > + } > > + } > > +} > > + > > +NS_IMPL_THREADSAFE_ADDREF(FileStreamWrapper) > > You can just use NS_IMPL_THREADSAFE_ISUPPORTS0 to replace all of these > macros. ok, fixed > > @@ +112,5 @@ > > + PRUint64 aLimit, > > + PRUint32 aFlags) > > +: FileStreamWrapper(aFileStream, aFileHelper, aOffset, aLimit, aFlags) > > +{ > > + mInputStream = do_QueryInterface(mFileStream); > > Hm... Seems like you need to assert this or make an init method. ok, added an assertion > > @@ +115,5 @@ > > +{ > > + mInputStream = do_QueryInterface(mFileStream); > > +} > > + > > +NS_IMPL_ADDREF_INHERITED(FileInputStreamWrapper, FileStreamWrapper) > > NS_IMPL_ISUPPORTS_INHERITED1 ok, done > > @@ +166,5 @@ > > + NS_ENSURE_SUCCESS(rv, rv); > > + } > > + } > > + > > + mOffset = 0; > > Hm... I don't understand why we do this. Shouldn't we keep this around? no, mOffset becomes relative here (was absolute) > > @@ +169,5 @@ > > + > > + mOffset = 0; > > + } > > + > > + PRUint32 max = mLimit - mOffset; > > This needs to be 64 bit, and signed (seems possible for mOffset to be larger > than mLimit, right?). Then check <= below. I agree with 64 bit, but unsigned mOffset is set to 0 when read()/write() is called first time it is then increased by the amount of bytes read/written > > @@ +216,5 @@ > > + FileHelper* aFileHelper, > > + PRUint64 aOffset, > > + PRUint64 aLimit, > > + PRUint32 aFlags) > > +: FileStreamWrapper(aFileStream, aFileHelper, aOffset, aLimit, aFlags) > > You need an #ifdef DEBUG here to mark mThread null ok, added > > @@ +218,5 @@ > > + PRUint64 aLimit, > > + PRUint32 aFlags) > > +: FileStreamWrapper(aFileStream, aFileHelper, aOffset, aLimit, aFlags) > > +{ > > + mOutputStream = do_QueryInterface(mFileStream); > > Assert or init method. added an assertion > > @@ +221,5 @@ > > +{ > > + mOutputStream = do_QueryInterface(mFileStream); > > +} > > + > > +NS_IMPL_ADDREF_INHERITED(FileOutputStreamWrapper, FileStreamWrapper) > > NS_IMPL_ISUPPORTS_INHERITED1 ok, fixed > > @@ +237,5 @@ > > + nsresult rv; > > + > > + if (!mFirstTime) { > > + // We must flush the stream on the same thread on which we wrote some data. > > + nsCOMPtr<nsIOutputStream> ostream = do_QueryInterface(mFileStream); > > Wait, you already have this, right? (mOutputStream) actually, I changed Flush() to flush buffers and sync here, we need to just flush buffers, so this now QI nsIStandardFileStream and calls FlushBuffers() we don't need this for necko streams > > @@ +245,5 @@ > > + } > > + > > +#ifdef DEBUG > > + void* thread = PR_GetCurrentThread(); > > + NS_ASSERTION(thread == mThread, "Unsetting thread locals on wrong thread!"); > > Just assert PR_GetCurrentThread() == mThread, lose the stack var and the > #ifdef DEBUG ok, fixed > > @@ +262,5 @@ > > + > > + mOffset = 0; > > + mLimit = 0; > > + > > + return NS_OK; > > If that flush failed don't you want to return that here? fixed, had to use double assign to keep code simple > > @@ +279,5 @@ > > + mThread = PR_GetCurrentThread(); > > +#endif > > + mFileHelper->mFileStorage->SetThreadLocals(); > > + > > + nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mFileStream); > > I'd QI mOutputStream, not mFileStream ok, done the same in Read() > > @@ +285,5 @@ > > + if (mOffset == LL_MAXUINT) { > > + seekable->Seek(nsISeekableStream::NS_SEEK_END, 0); > > + } > > + else { > > + seekable->Seek(nsISeekableStream::NS_SEEK_SET, mOffset); > > You should propagate any errors here. ok, fixed > > @@ +292,5 @@ > > + > > + mOffset = 0; > > + } > > + > > + PRUint32 max = mLimit - mOffset; > > Same problem here, use PRInt64, ensure that max can't be negative. see my comment above > > @@ +304,5 @@ > > + } > > + > > + nsresult rv = mOutputStream->Write(aBuf, aCount, _retval); > > + > > + if (NS_SUCCEEDED(rv)) { > > NS_ENSURE_SUCCESS here would be nicer. ok, fixed > > @@ +321,5 @@ > > + > > +NS_IMETHODIMP > > +FileOutputStreamWrapper::Flush() > > +{ > > + return NS_OK; > > Hm... Why wouldn't we call flush on mOutputStream? I want to to have flushing under own control if possible > > @@ +349,5 @@ > > + > > +NS_IMPL_THREADSAFE_ISUPPORTS1(ProgressRunnable, nsIRunnable) > > + > > +NS_IMETHODIMP > > +ProgressRunnable::Run() > > Assert main thread for these 3 runnables ok, done > > ::: dom/file/FileStreamWrappers.h > @@ +33,5 @@ > > + > > + enum { > > + NOTIFY_PROGRESS = 1 << 0, > > + NOTIFY_CLOSE = 1 << 1, > > + NOTIFY_DESTROY = 1 << 1 > > Er... This is intentional? Maybe NOTIFY_DESTROY = NOTIFY_CLOSE to make that > clear. no, it's a typo, I fixed it in the meantime > > @@ +56,5 @@ > > + FileInputStreamWrapper(nsIFileStream* aFileStream, > > + FileHelper* aFileHelper, > > + PRUint64 aOffset, > > + PRUint64 aLimit, > > + PRUint32 aFlags = 0); > > Nit: Require that last arg. ok, done > > @@ +58,5 @@ > > + PRUint64 aOffset, > > + PRUint64 aLimit, > > + PRUint32 aFlags = 0); > > + > > + virtual ~FileInputStreamWrapper() > > Nit: Protected ok, done > > @@ +76,5 @@ > > + FileOutputStreamWrapper(nsIFileStream* aFileStream, > > + FileHelper* aFileHelper, > > + PRUint64 aOffset, > > + PRUint64 aLimit, > > + PRUint32 aFlags = 0); > > Nit: Require that last arg ok, done > > @@ +80,5 @@ > > + PRUint32 aFlags = 0); > > + > > + virtual ~FileOutputStreamWrapper() > > + { > > + } > > Nit: Protected, and { } ok, done > > @@ +85,5 @@ > > + > > +private: > > + nsCOMPtr<nsIOutputStream> mOutputStream; > > +#ifdef DEBUG > > + void* mThread; > > How about 'mWriteThread'? Or something that indicates that it's the thread > where you actually write things, not confused with creating/owning thread. ok, mWriteThread > > ::: dom/file/LockedFile.cpp > @@ +41,5 @@ > > +public: > > + ReadHelper(LockedFile* aLockedFile, > > + FileRequest* aFileRequest, > > + PRInt64 aLocation, > > + PRInt64 aSize) > > These can't really be negative, right? In DoAsyncRun you pass the size into > an unsigned arg, so if they can actually be negative then I think we need > some changes below. fixed > > @@ +45,5 @@ > > + PRInt64 aSize) > > + : FileHelper(aLockedFile, aFileRequest), > > + mLocation(aLocation), mSize(aSize) > > + { > > + mStream = new MemoryOutputStream(aSize); > > Hm, MemoryOutputStream takes a PRInt32! I moved this out of constructor to Init() MemoryOutputStream::Create() now takes a PRUint64 and checks the size: NS_ENSURE_TRUE(aSize <= PR_UINT32_MAX, nsnull); > > @@ +50,5 @@ > > + } > > + > > + virtual ~ReadHelper() > > + { > > + } > > Nit { }, in several other places below too. Although, you don't really need > them, right? right, removed > > @@ +73,5 @@ > > + PRInt64 aSize, > > + const nsAString& aEncoding) > > + : ReadHelper(aLockedFile, aFileRequest, aLocation, aSize) > > + { > > + CopyUTF16toUTF8(aEncoding, mEncoding); > > Technically you could avoid doing this until after you check against IsEmpty > on the other thread. nice, fixed > > @@ +108,5 @@ > > + nsresult DoAsyncRun(nsIFileStream* aStream); > > + > > +private: > > + PRInt64 mLocation; > > + nsCOMPtr<nsIInputStream> mStream; > > This probably needs to be released on the main thread, right? no, all these streams are thread safe AFAIK > > @@ +184,5 @@ > > + nsCOMPtr<nsIInputStream> mStream; > > +}; > > + > > +already_AddRefed<nsDOMEvent> > > +CreateGenericEvent(const nsAString& aType, bool aBubbles, bool aCancelable) > > This isn't really needed now that we have > nsContentUtils::DispatchTrustedEvent. Let's switch to using that. Well, nsContentUtils::DispatchTrustedEvent() is not that new (imported from CVS) and it needs a dom document, since it creates events using doc->createEvent() Do you really want to use it ? > > @@ +206,5 @@ > > + RequestMode aRequestMode) > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + > > + nsRefPtr<LockedFile> lockedFile(new LockedFile()); > > Nit: use = fixed > > @@ +226,5 @@ > > + > > + NS_ASSERTION(depth, "This should never be 0!"); > > + lockedFile->mCreatedRecursionDepth = depth - 1; > > + > > + rv = thread->AddObserver(lockedFile); > > Hm. IndexedDB is no longer going to use this, see bug 672667. This code will > have similar trouble with native event handlers, so we'll need to change it > to use the new method. yeah, I updated it to use the new method > > @@ +231,5 @@ > > + NS_ENSURE_SUCCESS(rv, nsnull); > > + > > + lockedFile->mCreating = true; > > + > > + FileService* service = FileService::GetOrCreate(); > > This can fail... ok, added NS_ENSURE_TRUE() > > @@ +244,5 @@ > > + mLocation(0), > > + mPendingRequests(0), > > + mCreatedRecursionDepth(0), > > + mAborted(false), > > + mCreating(false) > > Missing mRequestMode fixed > > @@ +308,5 @@ > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + if (!mPendingRequests) { > > + NS_ASSERTION(mReadyState == INITIAL, > > + "Reusing a transaction!"); > > Nit: Not a transaction ;) ah, missed that one :) > > @@ +342,5 @@ > > + nsCOMPtr<nsIFileStream> stream = mFileHandle->CreateStream(); > > + > > + nsString streamMode; > > + if (mMode == READ_WRITE) { > > + streamMode = NS_LITERAL_STRING("r+b"); > > Use AssignLiteral here and below. this is now done in IDBFileHandle::CreateStream() fixed > > @@ +364,5 @@ > > + > > + if (mStream && mFileHandle->mFileStorage->IsStorageInvalidated()) { > > + return NS_ERROR_NOT_AVAILABLE; > > + } > > + else { > > Nit: else after a return this code doesn't exist anymore, see my comment below > > @@ +366,5 @@ > > + return NS_ERROR_NOT_AVAILABLE; > > + } > > + else { > > + nsCOMPtr<nsIFileStream> stream; > > + nsresult rv = CreateStream(getter_AddRefs(stream)); > > Hm... It's possible to have mStream and !IsStorageInvalidated, right? in > that case looks like you're always making a new stream. Don't you want to > only create if one doesn't exist already? Yeah, I figured that out and fixed in the meantime However, I had to rework this (parallel streams were released on the main thread!) There's now CreateParallelStream() and GetOrCreateStream() Parallel streams are kept in an array, so they can be closed and released in FinishHelper > > @@ +383,5 @@ > > +LockedFile::GenerateFileRequest() > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + return FileRequest::Create(GetOwner(), > > + this); > > Nit: This can fit on one line. ok, fixed > > @@ +419,5 @@ > > + > > +NS_IMETHODIMP > > +LockedFile::GetFileHandle(nsIDOMFileHandle** aFileHandle) > > +{ > > + nsRefPtr<FileHandle> result = mFileHandle; > > Nit: Use an nsCOMPtr here (has the added benefit of asserting QI > correctness). this doesn't work: nsCOMPtr<nsIDOMFileHandle> result = mFileHandle; result.forget(aFileHandle); nsCOMPtr<nsIDOMFileHandle> result = mFileHandle.get(); result.forget(aFileHandle); or nsCOMPtr<nsIDOMFileHandle> result(mFileHandle); result.forget(aFileHandle); > > @@ +429,5 @@ > > +LockedFile::GetMode(nsAString& aMode) > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + > > + switch(mMode) { > > Nit: space before ( fixed > > @@ +434,5 @@ > > + case READ_ONLY: > > + aMode.AssignLiteral("readonly"); > > + break; > > + case READ_WRITE: > > + aMode.AssignLiteral("readwrite"); > > Nit: default: NS_NOTREACHED fixed > > @@ +446,5 @@ > > +{ > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + > > + *aOpen = IsOpen(); > > + > > Nit: lose extra line ok > > @@ +481,5 @@ > > + } > > + > > + nsRefPtr<MetadataParameters> params = new MetadataParameters(); > > + > > + if (!JSVAL_IS_VOID(aParameters) && !JSVAL_IS_NULL(aParameters)) { > > Is this necessary (I would hope the other code does this)? Otherwise you > probably want !JSVAL_IS_PRIMITIVE. generated code just warns if it's not an object: DOMFileMetadataParameters::Init(JSContext* aCx, const jsval* aVal) { if (!aCx || !aVal) { return NS_OK; } NS_ENSURE_STATE(aVal->isObject()); ... } anyway, I reworked this code a bit aParameters is now only optional, default action is to get all parameters (size and lastModified) .getMetadata() throws if aParameters is not an object or when nothing is requested > > @@ +504,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +LockedFile::ReadAsArrayBuffer(PRInt32 aSize, > > Hm, you don't do any sanity checking on aSize. It could be negative or 0 > here. I added a comment to the IDL, i think this should become PRUint64, but > if it's 0 you can either throw or do a shortcut and never go to the necko > thread. added: if (!aSize) { return NS_ERROR_TYPE_ERR; } > > @@ +530,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +LockedFile::ReadAsText(PRInt32 aSize, > > Same comments here. see above > > @@ +704,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +LockedFile::OpenInputStream(bool aWholeFile, PRUint64 aStart, PRUint64 aLength, > > This is a little weird... You don't really need to queue anything here. You > know that you're in parallel mode, and all that this twisty windy code path > ends up doing is calling CreateStream and then wrapping it. I think it's > much easier to understand if you just do that here and lose the > OpenStreamHelper entirely. well, I would have to call: OnNewRequest(); if (fileHandle->mFileStorage->IsStorageInvalidated()) { return NS_ERROR_NOT_AVAILABLE; } AsyncRun() - if that fails, OnRequestFinished() and the stream wrapper holds the FileHelper and when the stream is done reading it then calls FileHelper::Finish(), to cleanup and call OnRequestFinished I would need to move OnStreamClose() and OnStreamDestroy() to LockedFile and hold lockedfile in the wrapper, LockedFile is not thread safe and the wrapper is thread safe, so LockedFile would need to be released on main thread, etc. I think it's actually simpler and cleaner to use the helper for this. > > @@ +759,5 @@ > > + rv = helper->Enqueue(); > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR); > > + > > + if (!aAppend) { > > + mLocation += inputLength; > > Wait, so all the other methods move mLocation, but Append doesn't? I would > think that mLocation should be LL_MAXUINT now. ok, fixed > > @@ +767,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +LockedFile::GetInputStream(const jsval& aValue, JSContext* aCx, > > This should be static, and maybe not a member at all. And we could rename it > to something like GetInputStreamForJSVal ok, also moved to the anon namespace > > @@ +779,5 @@ > > + if (JS_IsArrayBufferObject(obj, aCx)) { > > + PRUint32 length = JS_GetArrayBufferByteLength(obj, aCx); > > + > > + rv = NS_NewByteInputStream(aInputStream, > > + (char*)JS_GetArrayBufferData(obj, aCx), length, > > reinterpret_cast ok, fixed > > @@ +780,5 @@ > > + PRUint32 length = JS_GetArrayBufferByteLength(obj, aCx); > > + > > + rv = NS_NewByteInputStream(aInputStream, > > + (char*)JS_GetArrayBufferData(obj, aCx), length, > > + NS_ASSIGNMENT_COPY); > > Ugh, i hate this... But I don't see any way around it. yeah, we have to duplicate data here > > @@ +794,5 @@ > > + if (blob) { > > + rv = blob->GetInternalStream(aInputStream); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + rv = blob->GetSize(aInputLength); > > I think you should switch the order here, in case something fails ok, done > > @@ +814,5 @@ > > + } > > + > > + nsDependentJSString str; > > + if (!str.init(aCx, jsstr)) { > > + return NS_ERROR_XPC_BAD_CONVERT_JS; > > This isn't really right, you want to swap with !jsstr above (this one should > be NS_ERROR_FAILURE, that one should be BAD_CONVERT) ok, fixed > > @@ +851,5 @@ > > +} > > + > > +FinishHelper::FinishHelper(LockedFile* aLockedFile) > > +: mLockedFile(aLockedFile), > > + mAborted(!!aLockedFile->mAborted) > > !! is unnecessary here. right > > @@ +856,5 @@ > > +{ > > + mStream.swap(aLockedFile->mStream); > > +} > > + > > +FinishHelper::~FinishHelper() > > Move to header. ok, done > > @@ +880,5 @@ > > + } > > + else { > > + event = CreateGenericEvent(NS_LITERAL_STRING("complete"), false, false); > > + } > > + NS_ENSURE_TRUE(event, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > Heh. Busted ;) :) > > @@ +913,5 @@ > > + > > +nsresult > > +ReadHelper::DoAsyncRun(nsIFileStream* aStream) > > +{ > > + NS_PRECONDITION(aStream, "Passed a null stream!"); > > NS_ASSERTION please ok, fixed > > @@ +923,5 @@ > > + > > + nsCOMPtr<nsIAsyncStreamCopier> copier; > > + nsresult rv = > > + NS_NewAsyncStreamCopier(getter_AddRefs(copier), istream, mStream, nsnull, > > + false, true, STREAM_COPY_BLOCK_SIZE, true, true); > > Hm, by passing null for the target argument you're going to have a > getService call every time you do a request. How about you get the > transportservice and cache it in your fileservice? yeah, I'm now caching the event target in FileService FileService also calls nsIStreamTransportService::RaiseThreadLimit() during initialization That increases the max number of necko threads from 4 to 5. > > Nit: You could leave off the last two args if you want. Below too. ok > > @@ +941,5 @@ > > +{ > > + JSObject *arrayBuffer; > > + nsresult rv = > > + nsContentUtils::CreateArrayBuffer(aCx, mStream->Data(), &arrayBuffer); > > + NS_ENSURE_SUCCESS(rv, rv); > > Hm, you could have read a really big chunk of stuff into memory, could you > go ahead and mStream->Close() before returning here? That way if you leak or > hang around longer than you expect at least you won't be holding that data. do you mean |mStream = nsnull| ? Close() doesn't release anything > > @@ +949,5 @@ > > + return NS_OK; > > +} > > + > > +nsresult > > +ReadTextHelper::GetSuccessResult(JSContext* aCx, > > Same here with calling mStream->Close() before returning. see my comment above > > @@ +975,5 @@ > > + tmpString); > > + NS_ENSURE_SUCCESS(rv, rv); > > + > > + if (!xpc::StringToJsval(aCx, tmpString, aVal)) { > > + return NS_ERROR_FAILURE; > > Nit: Add a warning ok, done > > @@ +984,5 @@ > > + > > +nsresult > > +WriteHelper::DoAsyncRun(nsIFileStream* aStream) > > +{ > > + NS_PRECONDITION(aStream, "Passed a null stream!"); > > NS_ASSERTION ok, fixed > > ::: dom/file/LockedFile.h > @@ +35,5 @@ > > + NS_DECL_ISUPPORTS_INHERITED > > + NS_DECL_NSIDOMLOCKEDFILE > > + NS_DECL_NSITHREADOBSERVER > > + > > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED( > > Unnecessary SCRIPT_HOLDER here ok, removed > > @@ +68,5 @@ > > + // nsIDOMEventTarget > > + virtual nsresult PreHandleEvent(nsEventChainPreVisitor& aVisitor); > > + > > + void OnNewRequest(); > > + void OnRequestFinished(); > > Looks like these are only called by FileHelper, which is a friend. Make them > private? ok, done > > @@ +70,5 @@ > > + > > + void OnNewRequest(); > > + void OnRequestFinished(); > > + > > + already_AddRefed<FileRequest> GenerateFileRequest(); > > This is only ever called in LockedFile methods, so make it private. Also > need to mark inline here. ok, fixed > > @@ +95,5 @@ > > + LockedFile(); > > + ~LockedFile(); > > + > > + nsresult WriteOrAppend(const jsval& aValue, > > + JSContext* aCx, > > Nit: Above you have args going all the way to 80 chars, here you have each > on its own line. Let's be consistent. fixed > > @@ +129,5 @@ > > + NS_DECL_ISUPPORTS > > + NS_DECL_NSIRUNNABLE > > + > > + FinishHelper(LockedFile* aLockedFile); > > + ~FinishHelper(); > > This can be private. ok, but I had to add LockedFile as a friend > > ::: dom/file/MemoryStreams.cpp > @@ +13,5 @@ > > +MemoryOutputStream::MemoryOutputStream(PRInt32 aSize) > > +: mOffset(0) > > +{ > > + char* dummy; > > + mData.GetMutableData(&dummy, aSize); > > This can fail. Need to check, sadly, so probably want an Init method or > static Create that does both. ok, added Create() added Init() to ReadHelper and ReadTextHelper > > @@ +16,5 @@ > > + char* dummy; > > + mData.GetMutableData(&dummy, aSize); > > +} > > + > > +MemoryOutputStream::~MemoryOutputStream() > > This can go in the header ok, done > > @@ +20,5 @@ > > +MemoryOutputStream::~MemoryOutputStream() > > +{ > > +} > > + > > +NS_IMPL_THREADSAFE_ADDREF(MemoryOutputStream) > > NS_IMPL_THREADSAFE_ISUPPORTS1 fixed > > @@ +36,5 @@ > > + return NS_OK; > > +} > > + > > +NS_IMETHODIMP > > +MemoryOutputStream::Write(const char *aBuf, PRUint32 aCount, PRUint32 *_retval) > > Nit: * on the left ok, fixed both > > @@ +38,5 @@ > > + > > +NS_IMETHODIMP > > +MemoryOutputStream::Write(const char *aBuf, PRUint32 aCount, PRUint32 *_retval) > > +{ > > + return WriteSegments(NS_CopySegmentToBuffer, (char*)aBuf, aCount, _retval); > > Er... Why are you un-consting aBuf? because WriterSegments() takes |void*| ? > > @@ +58,5 @@ > > +NS_IMETHODIMP > > +MemoryOutputStream::WriteSegments(nsReadSegmentFun aReader, void* aClosure, > > + PRUint32 aCount, PRUint32* _retval) > > +{ > > + PRUint32 maxCount = mData.Length() - mOffset; > > mOffset could be greater than length, right? only if aReader read more than we asked it to read added additional assertion: NS_ASSERTION(mData.Length() >= mOffset, "Bad stream state!"); > > ::: dom/file/MemoryStreams.h > @@ +18,5 @@ > > +public: > > + NS_DECL_ISUPPORTS > > + NS_DECL_NSIOUTPUTSTREAM > > + > > + MemoryOutputStream(PRInt32 aSize); > > Make sure we figure out what to do with this being a signed 32 bit integer. added |NS_ENSURE_TRUE(aSize <= PR_UINT32_MAX, nsnull);| to Create() so, for now, we will support "only" 32 bit sized chunks > > @@ +19,5 @@ > > + NS_DECL_ISUPPORTS > > + NS_DECL_NSIOUTPUTSTREAM > > + > > + MemoryOutputStream(PRInt32 aSize); > > + virtual ~MemoryOutputStream(); > > Private, and inlined. done > > ::: dom/file/MetadataHelper.cpp > @@ +12,5 @@ > > + > > +nsresult > > +MetadataParameters::Init(JSContext* aCx, const jsval* aVal) > > +{ > > + return mParams.Init(aCx, aVal); > > You could inline this. ok, done > > @@ +37,5 @@ > > + JSObject* obj = JS_NewObject(aCx, nsnull, nsnull, nsnull); > > + NS_ENSURE_TRUE(obj, NS_ERROR_OUT_OF_MEMORY); > > + > > + if (mParams->mParams.size && > > + !JS_DefineProperty(aCx, obj, "size", INT_TO_JSVAL(mParams->mSize), > > This is kind of confusing... You are checking 'mParams->mParams.size', but > then using 'mParams->mSize' Can you just nix the mParams->mParams member > entirely and carry bools on the MetadataHelper ("wantsSize", "hasSize", > something like that)? naming variables is always hard :) I added several new methods to make this code more readable > > Also, mSize is a PRInt64... A few things here. > > 1. Why signed? What would negative mean? changed to unsigned > 2. You can't use INT_TO_JSVAL for that. You need to compare it to > JSVAL_INT_MAX and convert to a double if it's bigger. The you want to > setNumber() on a jsval. fixed > > @@ +42,5 @@ > > + nsnull, nsnull, JSPROP_ENUMERATE)) { > > + return NS_ERROR_FAILURE; > > + } > > + > > + if (mParams->mParams.lastModified) { > > Same weirdness here. see my comment above > > @@ +44,5 @@ > > + } > > + > > + if (mParams->mParams.lastModified) { > > + double msec = mParams->mLastModified / PR_USEC_PER_MSEC; > > + JSObject *date = JS_NewDateObjectMsec(aCx, msec); > > This can fail. fixed > > ::: dom/file/MetadataHelper.h > @@ +19,5 @@ > > +BEGIN_FILE_NAMESPACE > > + > > +class MetadataParameters > > +{ > > + friend class MetadataHelper; > > Forward declare. done > > @@ +26,5 @@ > > + > > +public: > > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MetadataParameters) > > + > > + nsresult Init(JSContext* aCx, const jsval* aVal); > > Nit: Names on next lines again fixed > > @@ +34,5 @@ > > + return mSize; > > + } > > + > > +private: > > + mozilla::dom::DOMFileMetadataParameters mParams; > > You're already in mozilla::dom here right? I bet you can remove this. yeah, removed > > @@ +60,5 @@ > > + { > > + public: > > + AsyncMetadataGetter(MetadataParameters* aParams) > > + : mParams(aParams) > > + { } > > Nit: add an extra line here. done > > ::: dom/file/nsIDOMLockedFile.idl > @@ +34,5 @@ > > + [optional /* none */] in jsval parameters); > > + > > + [implicit_jscontext] > > + nsIDOMFileRequest > > + readAsArrayBuffer(in long size); > > These should be 'unsigned long long' fixed > > @@ +37,5 @@ > > + nsIDOMFileRequest > > + readAsArrayBuffer(in long size); > > + > > + nsIDOMFileRequest > > + readAsText(in long size, [optional] in DOMString encoding); > > 'unsigned long long' fixed

- use infallible hash tables - use fallible string in MemoryOutputStream - use sqlite3_quota_file_truesize() instead of sqlite3_quota_file_size() (for now)

necko changes landed separately

changes: - flush before getting file metadata - added test_getFileId.html (bug 755902) - DeviceStorage landed - m-c SQLite upgrade (contains test_quota changes)

patch based on multi process IDB

notes for patch 0.7.4: - CreateFileHelper now has to implement MaybeSendResponseToChildProcess() and UnpackResponseFromParentProcess() - reverted newly created StructuredCloneInfo.h (ctors for StructuredCloneReadInfo and StructuredCloneWriteInfo moved to IndexedDatabaseInlines.h) - adjusted IndexedDatabase.h changes (modified/added the == operators) - transaction->mCreatedFileInfos.Init() called in parent and child process - the sync loop (wait for IDB databases - file storages to complete) moved under sIsMainProcess check (IndexedDatabaseManager.cpp) not sure, if we need to modify SerializedStructuredCloneWriteInfo/SerializedStructuredCloneReadInfo at this point all IDB tests pass (including test_ipc.html) on my machine

Comment on attachment 627756 [details] [diff] [review] interdiff v0.4 - v0.7.3 Review of attachment 627756 [details] [diff] [review]: ----------------------------------------------------------------- r=me. just some small stuff to consider/fix below, overall it looks great! Thanks for keeping up with all the other IndexedDB changes flying around, and for being so patient :) ::: Mozilla1/src/content/base/src/nsContentUtils.cpp @@ +3580,5 @@ > // No universal charset detector, try the default charset detector > const nsAdoptingCString& detectorName = > Preferences::GetLocalizedCString("intl.charset.detector"); > if (!detectorName.IsEmpty()) { > + nsCAutoString detectorContractID(NS_CHARSET_DETECTOR_CONTRACTID_BASE); This will actually be worse than before, it has to do runtime length checking. The other way looks worse but precomputes the length. ::: Mozilla1/src/db/sqlite3/src/test_quota.c @@ +1252,5 @@ > + FILE* f = p->f; > + long pos1, pos2; > + int rc; > + pos1 = ftell(f); > + if (pos1 < 0) return -1; The sqlite folks use this style it seems: if( rc!=0 ) ::: Mozilla1/src/dom/file/AsyncHelper.cpp @@ +60,5 @@ > > NS_IMETHODIMP > AsyncHelper::GetName(nsACString& aName) > { > + MOZ_NOT_REACHED("Shouldn't be called!"); So... I know I told you to add these, but looking at it now I think I was crazy before. These can just be warnings. Sorry :( ::: Mozilla1/src/dom/file/DOMFileHandle.cpp @@ +48,5 @@ > + nsCOMPtr<nsIInputStream> stream; > + rv = NS_NewLocalFileInputStream(getter_AddRefs(stream), aFile, -1, -1, > + nsIFileInputStream::DEFER_OPEN); > + NS_ENSURE_SUCCESS(rv, nsnull); > + result = stream.forget(); How about you just return stream.forget() here instead of using a temporary nsCOMPtr? Below too. ::: Mozilla1/src/dom/file/File.cpp @@ +22,5 @@ > + > + if (mStoredFile) { > + FileInfo* fileInfo; > + > + using mozilla::dom::indexedDB::IndexedDatabaseManager; Nit: Any reason not to just put this at the top of the file (after headers, of course)? @@ +67,5 @@ > +} > + > +already_AddRefed<nsIDOMBlob> > +File::CreateSlice(PRUint64 aStart, PRUint64 aLength, > + const nsAString& aContentType) Nit: Indent is off here. ::: Mozilla1/src/dom/file/FileService.cpp @@ +71,5 @@ > + } > + > + // Make sure the service is still accessible while any generated callbacks > + // are processed. > + nsresult rv = NS_ProcessPendingEvents(nsnull); Pass 'thread' here. @@ +77,5 @@ > + > + if (!mCompleteCallbacks.IsEmpty()) { > + // Run all callbacks manually now. > + for (PRUint32 index = 0; index < mCompleteCallbacks.Length(); index++) { > + mCompleteCallbacks[index].mCallback->Run(); Hm, can the callbacks running cause mCompleteCallbacks to mutate? This looks dangerous. @@ +82,5 @@ > + } > + mCompleteCallbacks.Clear(); > + > + // And make sure they get processed. > + rv = NS_ProcessPendingEvents(nsnull); Pass 'thread' here. @@ +93,5 @@ > // static > FileService* > FileService::GetOrCreate() > { > + if (!gInstance && !gShutdown) { Maybe warn if called after shutdown? @@ +108,2 @@ > > + rv = obs->AddObserver(service, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false); IndexedDB does this incorrectly, but do you need access to the profile at all times? If so then you should use 'profile-before-change'. Otherwise, if you don't, then this is probably fine. ::: Mozilla1/src/dom/file/FileService.h @@ +116,3 @@ > > + bool > + HasRunningLockedFiles() { Nit: { on next line ::: Mozilla1/src/dom/file/LockedFile.cpp @@ +49,3 @@ > : FileHelper(aLockedFile, aFileRequest), > mLocation(aLocation), mSize(aSize) > + { } Assert non-0 size? @@ +94,5 @@ > nsIInputStream* aStream, > PRUint64 aLength) > : FileHelper(aLockedFile, aFileRequest), > mLocation(aLocation), mStream(aStream), mLength(aLength) > + { } Assert non-0 length? @@ +169,5 @@ > PRUint64 aStart, > PRUint64 aLength) > : FileHelper(aLockedFile, nsnull), > mWholeFile(aWholeFile), mStart(aStart), mLength(aLength) > { } Assert non-0 length? @@ +569,5 @@ > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + // Do nothing if the window is closed > + if (!GetOwner()) { > + return NS_OK; It's a little weird to do this before arg checking. It means sometimes passing bad args results in an exception and sometimes not. I'd move this, and you repeat this pattern in other methods below. Although, it's hard for me to see how GetOwner could return false here since you're being called from JS. Maybe just remove these? @@ +848,5 @@ > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > nsRefPtr<FinishHelper> helper(new FinishHelper(this)); > > + FileService* service = FileService::GetOrCreate(); Get() should be sufficient here right? Then you could assert it. @@ +947,5 @@ > > nsCOMPtr<nsIInputStream> istream = > new FileInputStreamWrapper(aStream, this, mLocation, mSize, flags); > + > + FileService* service = FileService::GetOrCreate(); Get() + assert @@ +1024,5 @@ > > nsCOMPtr<nsIOutputStream> ostream = > new FileOutputStreamWrapper(aStream, this, mLocation, mLength, flags); > > + FileService* service = FileService::GetOrCreate(); Get() + assert ::: Mozilla1/src/dom/file/MemoryStreams.cpp @@ +16,2 @@ > { > + NS_ENSURE_TRUE(aSize <= PR_UINT32_MAX, nsnull); What about 0? ::: Mozilla1/src/dom/file/MetadataHelper.cpp @@ +88,2 @@ > PRInt64 size; > + rv = metadata->GetSize(&size); I'm not sure why this is signed? @@ +96,2 @@ > PRInt64 lastModified; > + rv = metadata->GetLastModified(&lastModified); signed here too? @@ +98,3 @@ > NS_ENSURE_SUCCESS(rv, rv); > > mParams->mLastModified = lastModified; Isn't mParams->mLastModified unsigned? ::: Mozilla1/src/dom/indexedDB/FileStream.cpp @@ +250,5 @@ > } > > + NS_ASSERTION(!NS_IsMainThread(), "Performing sync IO on the main thread!"); > + > + PRInt64 rc = sqlite3_quota_file_truesize(mQuotaFile); Comment here that we'd use sqlite3_quota_file_size but that it seems broken, and please file a followup for us to track that down. @@ +300,4 @@ > nsresult > FileStream::DoOpen() > { > NS_PRECONDITION(!mFilePath.IsEmpty(), "Must have a file path"); NS_ASSERTION ::: Mozilla1/src/dom/indexedDB/IndexedDatabaseManager.cpp @@ +548,5 @@ > } > > for (PRUint32 index = 0; index < liveDatabases.Length(); index++) { > IDBDatabase*& database = liveDatabases[index]; > + if (database->GetOwner() == aWindow && ( Nit: ( on following line. if (1 && ((2 && 3) || (4 && 5))) @@ +591,5 @@ > PRUint32 count = !!service + !!pool; > > nsRefPtr<WaitForTransactionsToFinishRunnable> runnable = > count ? new WaitForTransactionsToFinishRunnable(op, count) > + : new WaitForTransactionsToFinishRunnable(op, 1); NS_MAX(count, 1) to avoid the ? op @@ +1254,5 @@ > // first then the sync loop won't be processed at all, otherwise it will > // lock the main thread until all IDB file storages are finished. > > + nsTArray<nsCOMPtr<nsIFileStorage> > liveDatabases; > + liveDatabases.SetCapacity(mLiveDatabases.Count()); I think you can just pass the capacity as the constructor arg ::: Mozilla1/src/dom/indexedDB/StructuredCloneInfo.h @@ +25,5 @@ > + aData2 = aData1; > + aData1 = temp; > +} > + > +struct StructuredCloneReadInfo { Nit: For all these structs and swap functions please put { on next line

Comment on attachment 629294 [details] [diff] [review] patch v0.7.4 Review of attachment 629294 [details] [diff] [review]: ----------------------------------------------------------------- Can you attach the interdiff to the bug? I looked over all the changes you mentioned and r=me on them (with the little caveat below). Assuming that the interdiff just has the stuff you talked about then I don't need to review it separately, this should all be ready to land! Thanks again! ::: dom/indexedDB/IndexedDatabase.h @@ +91,5 @@ > uint64_t* data; > size_t dataLength; > }; > > +struct StructuredCloneFile { Nit: { on next line. ::: dom/indexedDB/IndexedDatabaseInlines.h @@ +10,5 @@ > > BEGIN_INDEXEDDB_NAMESPACE > > inline > +StructuredCloneWriteInfo::StructuredCloneWriteInfo() Hm, this doesn't need to live here. I only created this file because some of the functions needed other headers that shouldn't be included everywhere. Basically if it compiles inline into IndexedDatabase.h without adding blob/file includes then it should stay there. @@ +34,5 @@ > } > > inline > +StructuredCloneReadInfo::StructuredCloneReadInfo() > +: mDatabase(nsnull) This can move back

(In reply to ben turner [:bent] from comment #66) > Comment on attachment 629294 [details] [diff] [review] > patch v0.7.4 > > Review of attachment 629294 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you attach the interdiff to the bug? I looked over all the changes you > mentioned and r=me on them (with the little caveat below). > > Assuming that the interdiff just has the stuff you talked about then I don't > need to review it separately, this should all be ready to land! > > Thanks again! ok, I'll try (it will contain fixes from the previous comment too) > > ::: dom/indexedDB/IndexedDatabase.h > @@ +91,5 @@ > > uint64_t* data; > > size_t dataLength; > > }; > > > > +struct StructuredCloneFile { > > Nit: { on next line. ok > > ::: dom/indexedDB/IndexedDatabaseInlines.h > @@ +10,5 @@ > > > > BEGIN_INDEXEDDB_NAMESPACE > > > > inline > > +StructuredCloneWriteInfo::StructuredCloneWriteInfo() > > Hm, this doesn't need to live here. I only created this file because some of > the functions needed other headers that shouldn't be included everywhere. > Basically if it compiles inline into IndexedDatabase.h without adding > blob/file includes then it should stay there. > When I do that I get this: /usr/bin/clang++ -o CheckQuotaHelper.o -c -fvisibility=hidden -D_IMPL_NS_LAYOUT -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DEXCLUDE_SKIA_DEPENDENCIES -DOS_MACOSX=1 -DOS_POSIX=1 -I/Users/varga/Sources/Inbound/src/db/sqlite3/src -I/Users/varga/Sources/Inbound/src/xpcom/build -I/Users/varga/Sources/Inbound/src/dom/base -I/Users/varga/Sources/Inbound/src/dom/src/storage -I/Users/varga/Sources/Inbound/src/content/base/src -I/Users/varga/Sources/Inbound/src/content/events/src -I/Users/varga/Sources/Inbound/src/ipc/chromium/src -I/Users/varga/Sources/Inbound/src/ipc/glue -I../../ipc/ipdl/_ipdlheaders -I/Users/varga/Sources/Inbound/src/dom/indexedDB -I. -I../../dist/include -I../../dist/include/nsprpub -I/Users/varga/Sources/Inbound/obj-ff-opt/dist/include/nspr -I/Users/varga/Sources/Inbound/obj-ff-opt/dist/include/nss -fPIC -Qunused-arguments -fno-rtti -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wno-c++0x-extensions -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fshort-wchar -ffunction-sections -fdata-sections -pthread -DNO_X11 -pipe -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer -Qunused-arguments -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/CheckQuotaHelper.pp /Users/varga/Sources/Inbound/src/dom/indexedDB/CheckQuotaHelper.cpp In file included from /Users/varga/Sources/Inbound/src/dom/indexedDB/CheckQuotaHelper.cpp:7: In file included from /Users/varga/Sources/Inbound/src/dom/indexedDB/CheckQuotaHelper.h:11: In file included from /Users/varga/Sources/Inbound/src/dom/indexedDB/IndexedDatabase.h:14: ../../dist/include/nsAutoPtr.h:874:20: error: member access into incomplete type 'mozilla::dom::indexedDB::FileInfo' mRawPtr->Release(); ^ ../../dist/include/nsTArray.h:348:9: note: in instantiation of member function 'nsRefPtr<mozilla::dom::indexedDB::FileInfo>::~nsRefPtr' requested here e->~E(); ^ ../../dist/include/nsTArray.h:1211:20: note: in instantiation of member function 'nsTArrayElementTraits<nsRefPtr<mozilla::dom::indexedDB::FileInfo> >::Destruct' requested here elem_traits::Destruct(iter); ^ ../../dist/include/nsTArray.h:931:5: note: in instantiation of member function 'nsTArray<nsRefPtr<mozilla::dom::indexedDB::FileInfo>, nsTArrayDefaultAllocator>::DestructRange' requested here DestructRange(start, count); ^ ../../dist/include/nsTArray.h:942:5: note: in instantiation of member function 'nsTArray<nsRefPtr<mozilla::dom::indexedDB::FileInfo>, nsTArrayDefaultAllocator>::RemoveElementsAt' requested here RemoveElementsAt(0, Length()); ^ ../../dist/include/nsTArray.h:430:17: note: in instantiation of member function 'nsTArray<nsRefPtr<mozilla::dom::indexedDB::FileInfo>, nsTArrayDefaultAllocator>::Clear' requested here ~nsTArray() { Clear(); } ^ /Users/varga/Sources/Inbound/src/dom/indexedDB/IndexedDatabase.h:51:3: note: in instantiation of member function 'nsTArray<nsRefPtr<mozilla::dom::indexedDB::FileInfo>, nsTArrayDefaultAllocator>::~nsTArray' requested here StructuredCloneReadInfo() ^ /Users/varga/Sources/Inbound/src/dom/indexedDB/IndexedDatabase.h:35:7: note: forward declaration of 'mozilla::dom::indexedDB::FileInfo' class FileInfo; ^ 1 error generated. make[1]: *** [CheckQuotaHelper.o] Error 1 make: *** [default] Error 2 When I include "FileInfo.h" in IndexedDatabase.h I get even more errors ... I think it's not a good idea to include stuff in IndexedDatabase.h that includes IndedexDatabase.h itself The other (not very nice) possibility is to include "FileInfo.h" and stuff in all cpp files where StructuredCloneReadInfo/StructuredCloneWriteInfo is used. So I separated those structs into a standalone file (before your multi process IDB changes) Now, when a separate file already exists (IndexedDatabaseInlines.h) I can use that to do the trick :)

(In reply to Jan Varga [:janv] from comment #67) > So I separated those structs into a standalone file Sounds fine. Sorry that got complicated!

(In reply to ben turner [:bent] from comment #65) > Comment on attachment 627756 [details] [diff] [review] > interdiff v0.4 - v0.7.3 > > Review of attachment 627756 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me. just some small stuff to consider/fix below, overall it looks great! > > Thanks for keeping up with all the other IndexedDB changes flying around, > and for being so patient :) sure :) > > ::: Mozilla1/src/content/base/src/nsContentUtils.cpp > @@ +3580,5 @@ > > // No universal charset detector, try the default charset detector > > const nsAdoptingCString& detectorName = > > Preferences::GetLocalizedCString("intl.charset.detector"); > > if (!detectorName.IsEmpty()) { > > + nsCAutoString detectorContractID(NS_CHARSET_DETECTOR_CONTRACTID_BASE); > > This will actually be worse than before, it has to do runtime length > checking. The other way looks worse but precomputes the length. so, revert back to AssignLiteral() ? > ::: Mozilla1/src/db/sqlite3/src/test_quota.c > @@ +1252,5 @@ > > + FILE* f = p->f; > > + long pos1, pos2; > > + int rc; > > + pos1 = ftell(f); > > + if (pos1 < 0) return -1; > > The sqlite folks use this style it seems: > > if( rc!=0 ) uhm ... "Upon successful completion, ftell() and ftello() return the current offset. Otherwise, -1 is returned and the global variable errno is set to indicate the error." I can only do: if (rc == -1) return rc; > ::: Mozilla1/src/dom/file/AsyncHelper.cpp > @@ +60,5 @@ > > > > NS_IMETHODIMP > > AsyncHelper::GetName(nsACString& aName) > > { > > + MOZ_NOT_REACHED("Shouldn't be called!"); > > So... I know I told you to add these, but looking at it now I think I was > crazy before. These can just be warnings. Sorry :( ok, fixed > ::: Mozilla1/src/dom/file/DOMFileHandle.cpp > @@ +48,5 @@ > > + nsCOMPtr<nsIInputStream> stream; > > + rv = NS_NewLocalFileInputStream(getter_AddRefs(stream), aFile, -1, -1, > > + nsIFileInputStream::DEFER_OPEN); > > + NS_ENSURE_SUCCESS(rv, nsnull); > > + result = stream.forget(); > > How about you just return stream.forget() here instead of using a temporary > nsCOMPtr? Below too. ok, removed the "else" too > > ::: Mozilla1/src/dom/file/File.cpp > @@ +22,5 @@ > > + > > + if (mStoredFile) { > > + FileInfo* fileInfo; > > + > > + using mozilla::dom::indexedDB::IndexedDatabaseManager; > > Nit: Any reason not to just put this at the top of the file (after headers, > of course)? fixed > @@ +67,5 @@ > > +} > > + > > +already_AddRefed<nsIDOMBlob> > > +File::CreateSlice(PRUint64 aStart, PRUint64 aLength, > > + const nsAString& aContentType) > > Nit: Indent is off here. fixed > ::: Mozilla1/src/dom/file/FileService.cpp > @@ +71,5 @@ > > + } > > + > > + // Make sure the service is still accessible while any generated callbacks > > + // are processed. > > + nsresult rv = NS_ProcessPendingEvents(nsnull); > > Pass 'thread' here. I see, NS_ProcessPendingEvents() would have to call NS_GetCurrentThread() again fixed > @@ +77,5 @@ > > + > > + if (!mCompleteCallbacks.IsEmpty()) { > > + // Run all callbacks manually now. > > + for (PRUint32 index = 0; index < mCompleteCallbacks.Length(); index++) { > > + mCompleteCallbacks[index].mCallback->Run(); > > Hm, can the callbacks running cause mCompleteCallbacks to mutate? This looks > dangerous. need to talk about it (on IRC) ... > @@ +82,5 @@ > > + } > > + mCompleteCallbacks.Clear(); > > + > > + // And make sure they get processed. > > + rv = NS_ProcessPendingEvents(nsnull); > > Pass 'thread' here. done > @@ +93,5 @@ > > // static > > FileService* > > FileService::GetOrCreate() > > { > > + if (!gInstance && !gShutdown) { > > Maybe warn if called after shutdown? done > @@ +108,2 @@ > > > > + rv = obs->AddObserver(service, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false); > > IndexedDB does this incorrectly, but do you need access to the profile at > all times? If so then you should use 'profile-before-change'. Otherwise, if > you don't, then this is probably fine. hmm, according to https://bugzilla.mozilla.org/show_bug.cgi?id=662444#c24 we should change it to 'profile-before-change' sorry, I forgot to address the exit(0) change we can also cancel/abort all locked files when we get 'profile-before-change' (if we decide to do it) to speed up the cleanup process > ::: Mozilla1/src/dom/file/FileService.h > @@ +116,3 @@ > > > > + bool > > + HasRunningLockedFiles() { > > Nit: { on next line fixed > ::: Mozilla1/src/dom/file/LockedFile.cpp > @@ +49,3 @@ > > : FileHelper(aLockedFile, aFileRequest), > > mLocation(aLocation), mSize(aSize) > > + { } > > Assert non-0 size? done > @@ +94,5 @@ > > nsIInputStream* aStream, > > PRUint64 aLength) > > : FileHelper(aLockedFile, aFileRequest), > > mLocation(aLocation), mStream(aStream), mLength(aLength) > > + { } > > Assert non-0 length? done added also an early check to LockedFile::WriteOrAppend() if (!inputLength) { return NS_OK; } > @@ +169,5 @@ > > PRUint64 aStart, > > PRUint64 aLength) > > : FileHelper(aLockedFile, nsnull), > > mWholeFile(aWholeFile), mStart(aStart), mLength(aLength) > > { } > > Assert non-0 length? hmm, zero length should be valid here empty blobs/files can be passed to FileReader (and other APIs), no ? > @@ +569,5 @@ > > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > > > + // Do nothing if the window is closed > > + if (!GetOwner()) { > > + return NS_OK; > > It's a little weird to do this before arg checking. It means sometimes > passing bad args results in an exception and sometimes not. I'd move this, > and you repeat this pattern in other methods below. > > Although, it's hard for me to see how GetOwner could return false here since > you're being called from JS. Maybe just remove these? comment 33: " ... @@ +129,5 @@ > + LockedFile::Create(this, LockedFile::READ_ONLY, LockedFile::PARALLEL); > + NS_ENSURE_TRUE(lockedFile, NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR); > + > + nsRefPtr<FileRequest> fileRequest = > + FileRequest::Create(GetOwner(), lockedFile); nsDOMEventTargetHelper no longer holds a strong ref to its owner, so you should null-check. ... " this code is in FileHandle::GetFile(nsIDOMFileRequest** _retval) which is called from JS I suggest to just move |!GetOwner()| after arg and state checking > @@ +848,5 @@ > > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > > > nsRefPtr<FinishHelper> helper(new FinishHelper(this)); > > > > + FileService* service = FileService::GetOrCreate(); > > Get() should be sufficient here right? Then you could assert it. changed to Get(), but we need to talk about it ... > @@ +947,5 @@ > > > > nsCOMPtr<nsIInputStream> istream = > > new FileInputStreamWrapper(aStream, this, mLocation, mSize, flags); > > + > > + FileService* service = FileService::GetOrCreate(); > > Get() + assert fixed > @@ +1024,5 @@ > > > > nsCOMPtr<nsIOutputStream> ostream = > > new FileOutputStreamWrapper(aStream, this, mLocation, mLength, flags); > > > > + FileService* service = FileService::GetOrCreate(); > > Get() + assert fixed > ::: Mozilla1/src/dom/file/MemoryStreams.cpp > @@ +16,2 @@ > > { > > + NS_ENSURE_TRUE(aSize <= PR_UINT32_MAX, nsnull); > > What about 0? added a separate assertion (aSize <= PR_UINT32_MAX might disappear in future) > > ::: Mozilla1/src/dom/file/MetadataHelper.cpp > @@ +88,2 @@ > > PRInt64 size; > > + rv = metadata->GetSize(&size); > > I'm not sure why this is signed? see below > @@ +96,2 @@ > > PRInt64 lastModified; > > + rv = metadata->GetLastModified(&lastModified); > > signed here too? see below > @@ +98,3 @@ > > NS_ENSURE_SUCCESS(rv, rv); > > > > mParams->mLastModified = lastModified; > > Isn't mParams->mLastModified unsigned? actually, I think mLastModified should be signed here struct PRFileInfo64 { ... PROffset64 size PRTime modifyTime; ... } typedef PRInt64 PROffset64; typedef PRInt64 PRTime; PRTime can be negative in theory sqlite3_quota_file_mtime returns a signed int too nsIFileMetadata interface also uses signed ints (to be compatible with nsIFile) anyway, returning negative file size in JS doesn't make sense, so I just added NS_ENSURE_TRUE(size >= 0, NS_ERROR_FAILURE); in MetadataHelper::AsyncMetadataGetter::DoStreamWork() before the conversion from signed to unsigned > ::: Mozilla1/src/dom/indexedDB/FileStream.cpp > @@ +250,5 @@ > > } > > > > + NS_ASSERTION(!NS_IsMainThread(), "Performing sync IO on the main thread!"); > > + > > + PRInt64 rc = sqlite3_quota_file_truesize(mQuotaFile); > > Comment here that we'd use sqlite3_quota_file_size but that it seems broken, > and please file a followup for us to track that down. done > @@ +300,4 @@ > > nsresult > > FileStream::DoOpen() > > { > > NS_PRECONDITION(!mFilePath.IsEmpty(), "Must have a file path"); > > NS_ASSERTION fixed > > ::: Mozilla1/src/dom/indexedDB/IndexedDatabaseManager.cpp > @@ +548,5 @@ > > } > > > > for (PRUint32 index = 0; index < liveDatabases.Length(); index++) { > > IDBDatabase*& database = liveDatabases[index]; > > + if (database->GetOwner() == aWindow && ( > > Nit: ( on following line. > > if (1 && > ((2 && 3) || > (4 && 5))) done > @@ +591,5 @@ > > PRUint32 count = !!service + !!pool; > > > > nsRefPtr<WaitForTransactionsToFinishRunnable> runnable = > > count ? new WaitForTransactionsToFinishRunnable(op, count) > > + : new WaitForTransactionsToFinishRunnable(op, 1); > > NS_MAX(count, 1) to avoid the ? op done > @@ +1254,5 @@ > > // first then the sync loop won't be processed at all, otherwise it will > > // lock the main thread until all IDB file storages are finished. > > > > + nsTArray<nsCOMPtr<nsIFileStorage> > liveDatabases; > > + liveDatabases.SetCapacity(mLiveDatabases.Count()); > > I think you can just pass the capacity as the constructor arg done > ::: Mozilla1/src/dom/indexedDB/StructuredCloneInfo.h > @@ +25,5 @@ > > + aData2 = aData1; > > + aData1 = temp; > > +} > > + > > +struct StructuredCloneReadInfo { > > Nit: For all these structs and swap functions please put { on next line ok, done

(In reply to Jan Varga [:janv] from comment #70) > so, revert back to AssignLiteral() ? Yep > I can only do: > if (rc == -1) return rc; I only meant that they do |if( cond )| style, wasn't suggesting you change to check !=0, sorry. > need to talk about it (on IRC) ... I won't be on irc this weekend but I'll be able to check bugmail. Maybe just jot down your concerns here? > hmm, zero length should be valid here > empty blobs/files can be passed to FileReader (and other APIs), no ? Sounds fine > I suggest to just move |!GetOwner()| after arg and state checking Yeah, sounds good. > changed to Get(), but we need to talk about it ... Let's do that here. > anyway, returning negative file size in JS doesn't make sense, so I just > added > NS_ENSURE_TRUE(size >= 0, NS_ERROR_FAILURE); Cool

(In reply to ben turner [:bent] from comment #72) > > need to talk about it (on IRC) ... > > I won't be on irc this weekend but I'll be able to check bugmail. Maybe just > jot down your concerns here? > ok, I'll try to think about it more ... the mCompleteCallbacks thing seems to be the last one to address did you mean to just make sure that running callbacks can never cause mCompleteCallbacks to mutate or rewrite traversing and running callbacks in a safer way ?

also, this loop traverses mCompleteCallbacks directly for (PRUint32 index = 0; index < mCompleteCallbacks.Length(); index++) { MaybeFireCallback(index); } and MaybeFireCallback() may remove the callback from mCompleteCallbacks ?! it seems we have the same problem in TransactionThreadPool

Yeah, we need to fix this. Consider: for (PRUint32 index = 0; index < mCompleteCallbacks.Length(); index++) { MaybeFireCallback(index); } MaybeFireCallback can remove an element from the array, and then our index is wrong and we will skip the next entry. I think the easiest is to make MaybeFireCallback return a bool that we can test to see if a callback was actually run (and mCompleteCallbacks was modified) so that we can adjust our index value. Can you fix TransactionThreadPool too?

I reworked the callback handling a bit more

Comment on attachment 629555 [details] [diff] [review] patch v0.7.5 Looks great! r=me.

(In reply to ben turner [:bent] from comment #78) > Comment on attachment 629555 [details] [diff] [review] > patch v0.7.5 > > Looks great! r=me. Thanks! I'll land today

Comment on attachment 629555 [details] [diff] [review] patch v0.7.5 >+NS_IMPL_EVENT_HANDLER(FileHandle, abort); >+NS_IMPL_EVENT_HANDLER(FileHandle, error); gcc 4.3.2 chokes on those spurious trailing semicolons.

(In reply to neil@parkwaycc.co.uk from comment #81) > Comment on attachment 629555 [details] [diff] [review] > patch v0.7.5 > > >+NS_IMPL_EVENT_HANDLER(FileHandle, abort); > >+NS_IMPL_EVENT_HANDLER(FileHandle, error); > gcc 4.3.2 chokes on those spurious trailing semicolons. bug 761123

This patch added 6 calls to enablePrivilege to the test suite, which should have been grounds for r-. Jan, can you file a followup bug and fix that?

(In reply to Bobby Holley (:bholley) from comment #84) > This patch added 6 calls to enablePrivilege to the test suite, which should > have been grounds for r-. Jan, can you file a followup bug and fix that? bug 763388, patch coming

(In reply to Jan Varga [:janv] from comment #85) > bug 763388, patch coming Awesome, thanks :-)

FYI: https://wiki.mozilla.org/WebAPI/FileHandleAPI

FYI: bug 770184

:sicking, :janv, :mfuller and I discussed this feature while :janv was in Mountain View last week. The conclusion we reached was that the FileHandle does not exposed significantly more risk than the current file reader / writer APIs do. There was concern about implementation bugs and I will file a followup for our team to look at the changes. Secondly, we decided there should be a review regarding the removal of the indexDB prompt that is scheduled to land at a later date. I will file a bug for that as well. The security review of this feature should be considered complete as of last Friday.

If I understand this patch correctly, we do not have FileHandle support in workers. Is that by design? Should we file a followup for FileHandle in workers? (hint, hint, nudge, nudge: OS.File works in workers).

https://wiki.mozilla.org/WebAPI/FileHandleAPI#ToDo yeah, FileHandle support in workers would be nice to have but I think multi process support is more important, especially for b2g

Filed followup for workers at 786215. I am not exactly sure what multi-process FileHandle should be, though.

There's already a bug for multi process, bug 771288

Documentation: https://developer.mozilla.org/en-US/docs/WebAPI/FileHandle_API Can someone review it just to say if there is a major omission or mistake? Thanks

Thank you for doing this! It looks good at first glance, I'll try to read it again (more carefully) I would probably avoid mentioning the word "transaction" since abort() doesn't rollback at all. Maybe "task", Jonas ?

Yeah, definitely don't use the word "transaction". You could talk about it in terms of opening and closing the file.

Would it be true to say that the set of operations on a LockedFile is done atomically?

according to http://en.wikipedia.org/wiki/ACID#Atomicity, no maybe "Isolated"

Yes, FileHandle only implements the 'I' in ACID http://en.wikipedia.org/wiki/ACID

The documentation on MDN has been updated: - Feel free to take a look a report any mitake :)

The documentation on MDN has been updated: - https://developer.mozilla.org/en-US/docs/WebAPI/FileHandle - https://developer.mozilla.org/en-US/docs/Web/API/FileHandle - https://developer.mozilla.org/en-US/docs/Web/API/LockedFile - https://developer.mozilla.org/en-US/docs/Web/API/FileRequest Feel free to take a look a report any mistake :)