bkelly, this is the bug that gets rid of nsIOfflineStorage. If everything goes well, you won't need to fix bug 1110487.

Thanks. ETA? :-)

(In reply to Ben Kelly [:bkelly] from comment #2) > Thanks. ETA? :-) https://tbpl.mozilla.org/?tree=Try&rev=b12bee929637 try looks good, we'll see what bent says :)

I'll comment on this tomorrow.

try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e461792bb3c

So this bug is only about directory locks, all other changes in quota manager API, cleanup, etc. are not included. Hopefully it will make reviewing easier. Also, once this lands (along with bug 1125102), the cache implementation for service workers won't need to implement nsIOfflineStorage and just hold directory locks for the lifetime of cache objects. Ben, I addressed one of your initial comments, more details below... > 1. I like the DirectoryLock idea, but I don't think it should have its own > invalidate callback mechanism. Let's leave that on the Client API (that was > FinishUp on our sketch). You can still do the thing where you wait for all > DirectoryLocks to finish. The only benefit of FinishUp is that it feels a bit cleaner from the API point of view. However, here are disadvantages I found so far: - Invalidate callbacks have a pointer to Database objects, so we can invalidate them easily, FinishUp would have to enumerate gLiveDatabaseHashtable/mLiveDatabases and invalidate databases for given origin. - Invalidate callbacks can be rather easily added for FactoryOp objects too, sure we could also add a hashtable for FactoryOp objects just for FinishUp - AbortCloseStoragesForProcess/InvalidateDirectoryLocksForProcess is now very simple, it just goes through all directory locks and invalidate those which belong to given process, I believe we can't use FinishUp(origin) here, because that would abort operations for other processes. We would have to add FinishUp(process) but I don't think that scales well. - ClearStoragesForURI can be quite simple with invalidate callbacks, with FinishUp, we would have to handle the case when the clearing is blocked e.g. by usage calculation which is not a client operation - Clear (clearing of entire repository) can again be simple, with FinishUp we would have to call it for each origin and that would need to enumerate some hashtables, etc. So I think invalidate callbacks are simpler and probably faster than FinishUp. We can figure out a better name if you don't like the name. > 2. Why does DirectoryLock have an Unlock method? I thought the whole point > was to use reference counting rather than explicit calls? Ok, I removed the Unlock method and it seems to work correctly.

This patch is based on latest m-c plus all bent's patches for WAL, WITHOUT ROWID, etc. It also converts bkelly's cache to use directory locks instead of nsIOfflineStorage. IDB/Cache tests pass locally, I'll push to try once bent lands his stuff.

Comment on attachment 8585028 [details] [diff] [review] patch bkelly, can you take a look at dom/cache changes ? A directory lock is basically WaitForOpenAllowed/AllowNextSyncOp and RegisterStorage/UnregisterStorage merged into one object. With you recent patch for canceling quota init runnable, I think quota manager can in theory clear origins sooner through the invalidate callback. This was not possible before since QM could only invalidate offline storages, synchronized ops couldn't be canceled/invalidated.

Comment on attachment 8585028 [details] [diff] [review] patch Review of attachment 8585028 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. Thanks! I would like to fix up the state names to match the new QM call and callbacks, though. I'm not quite sure what you meant about canceling the QuotaInitRunnable. I don't see where the patch does anything special there. Can you clarify? ::: dom/cache/Context.cpp @@ +39,5 @@ > + > + NS_IMETHOD > + Run() override > + { > + mContext->InvalidateAndAllowToClose(); MOZ_ASSERT(NS_IsMainThread()) for clarity please. @@ +188,5 @@ > // | | > +// +-------v-------+ | > +// | OpenDirectory | Resolve(error) | > +// | (Main Thread) +---------------------+ > +// +-------+-------+ | I don't think this diagram matches the code any more. There are still two states: 1) State to call OpenDirectory() 2) State to wait for OpenDirectory() to call back We do want to change the names of the states to match the new operation, though. Can you create STATE_OPEN_DIRECTORY and STATE_WAIT_FOR_DIRECTORY_LOCK or something? And then update the diagram accordingly? @@ +247,2 @@ > // a callback. We will then get executed again on the main thread when > // it is safe to open the quota directory. Please note that OpenDirectory() will call back in DirectoryLockAcquired(), etc. @@ +258,4 @@ > break; > } > // ------------------------------ > case STATE_WAIT_FOR_OPEN_ALLOWED: You remove this state from the comments, but its still in the code. I think we still want a state here to assert on, but we don't need to switch on it Run() anymore. Just do the right thing in DirectoryLockAcquired(), etc. @@ +358,5 @@ > +void > +Context::QuotaInitRunnable::DirectoryLockAcquired(DirectoryLock* aLock) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mState == STATE_WAIT_FOR_OPEN_ALLOWED); See above comment about updating the state name to match the operation that is being done. @@ +367,5 @@ > + aLock->SetInvalidateCallback(callback); > + > + mDirectoryLock = new nsMainThreadPtrHolder<DirectoryLock>(aLock); > + > + Run(); Please just move the logic from Run()'s STATE_WAIT_FOR_OPEN_ALLOWED case into here. @@ +377,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mState == STATE_WAIT_FOR_OPEN_ALLOWED); > + MOZ_ASSERT(!mDirectoryLock); > + > + Run(); Please call resolve->Resolve(NS_ERROR_FAILURE) directly instead of calling back into Run(). ::: dom/cache/Context.h @@ +47,5 @@ > // 1) The Manager will call Context::AllowToClose() when all of the actors > // have removed themselves as listener. This means an idle context with > // no active DOM objects will close gracefully. > // 2) The QuotaManager invalidates the storage area so it can delete the > +// files. In this case the InvalidateCallback calls Cache::Invalidate() Since you are touching this, can you fix my typo? Should be Context::Invalidate() instead of Cache::Invalidate(). ::: dom/quota/QuotaManager.h @@ +232,5 @@ > const nsACString& aASCIIOrigin, > nsIFile** aDirectory) const; > > + already_AddRefed<DirectoryLock> > + OpenDirectory(const OptionalContentId& aOptionalContentParentId, Please add comments documenting the OpenDirectory() API calls.

Comment on attachment 8585028 [details] [diff] [review] patch Canceling review for now, as discussed on irc I still think we need to not have invalidate callbacks on locks.

Ok, I replaced the invalidation callback with AbortOperations() on the client interface.

Comment on attachment 8599439 [details] [diff] [review] patch bkelly, can you take a look at dom/cache changes again ?

Comment on attachment 8599439 [details] [diff] [review] patch Review of attachment 8599439 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks reasonable, but I'd like to understand the changes to ManagerId better. Also, the error paths in the Context directory lock methods are broken. ::: dom/cache/Context.cpp @@ +145,5 @@ > NS_DECL_THREADSAFE_ISUPPORTS > NS_DECL_NSIRUNNABLE > + > + virtual void > + DirectoryLockAcquired(DirectoryLock* aLock) override; Please put the OpenDirectoryListener methods up in the first public: section of the class. Also please add a comment like "// OpenDirectoryListener methods". @@ +161,5 @@ > // | (Orig Thread) +---------------------+ > // +-------+-------+ | > // | | > // +----------v-----------+ | > +// | CallOpenDirectory | Resolve(error) | The new state is just "OpenDirectory" instead of "CallOpenDirectory". Please fix one or the other. @@ +166,5 @@ > // | (Main Thread) +-----------------+ > // +----------+-----------+ | > // | | > +// +-------v-------+ | > +// | OpenDirectory | Resolve(error) | I think this should be WaitForDirectoryLock. @@ +317,5 @@ > + MOZ_ASSERT(!mDirectoryLock); > + > + mDirectoryLock = new nsMainThreadPtrHolder<DirectoryLock>(aLock); > + > + nsRefPtr<SyncResolver> resolver = new SyncResolver(); The use of SyncResolver() is incorrect here. Its only used in Run() because we need to pass it to the Action. In any case, you need to do this logic from the end of Run(): mState = STATE_COMPLETING; MOZ_ALWAYS_TRUE(NS_SUCCEEDED( mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL))); I suppose you could wrap that up in a Complete(nsresult rv) private method. You can then do Complete(NS_ERROR_ABORT), etc. @@ +319,5 @@ > + mDirectoryLock = new nsMainThreadPtrHolder<DirectoryLock>(aLock); > + > + nsRefPtr<SyncResolver> resolver = new SyncResolver(); > + > + if (mCanceled) { NS_WARN_IF @@ +336,5 @@ > + } > +} > + > +void > +Context::QuotaInitRunnable::DirectoryLockFailed() NS_WARNING() or NS_WARN_IF() in this method please. ::: dom/cache/Manager.cpp @@ +372,5 @@ > + } > + > + // Can't call Manager::Abort() while looping through the manager list since > + // Manager::Abort() can end up removing the Manager from the list. > + // So we have to put all matching managers to an array. nsTObserverArray is designed to allow removals while iterating the list. The copy is not necessary. @@ +1909,5 @@ > + NoteClosing(); > + > + // If there is a context, then cancel and only note that we are done after > + // its cleaned up. > + if (mContext) { You can MOZ_ASSERT(mContext) here. The factory should now never have a reference to a Manager without a Context. ::: dom/cache/Manager.h @@ +130,5 @@ > > // Synchronously shutdown from main thread. This spins the event loop. > static void ShutdownAllOnMainThread(); > > + static void AbortOnMainThread(const nsACString& aOrigin); Please add a comment about what this does. Also note that an IsVoid() string means to abort all Managers. ::: dom/cache/ManagerId.cpp @@ +41,3 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > + nsRefPtr<ManagerId> ref = new ManagerId(aPrincipal, jarPrefix + origin); I really dislike calling something an origin if its actually the origin combined with something else. Its terribly confusing for anyone trying to understand the API. Please have ManagerId() take jarPrefix and origin separately. And can you tell me why this change is necessary for this patch at all? It seems unrelated.

In addition to what we talked about in IRC, can you also confirm the jar prefix provides the same uniqueness guarantee for appId and inBrowser flag?

As in, Cache API used with a principal with same origin, but differing only in appId or browserFlag will still be determined to be non-equal in ManagerId.

Yes, ManagerId with jarPrefix + origin will provide the same functionality. See https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#1526 Also, in our storage world, by "origin" we automatically mean jarPrefix + origin, since using jar prefix or extended origin in conversations is just too long.

Comment on attachment 8599439 [details] [diff] [review] patch Review of attachment 8599439 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good! r- because there are some problem, but on the whole this is great. Please please please track any changes as an interdiff so I don't have to re-review this whole thing again ;) I didn't look at anything in dom/cache, bkelly knows better how that should work. ::: dom/asmjscache/AsmJSCache.cpp @@ +929,4 @@ > FileDescriptorHolder::Finish(); > > + if (mDirectoryLock) { > + mDirectoryLock = nullptr; Nit: Just null out unconditionally, no need to check first. @@ +954,2 @@ > > + // XXX The exclusive lock shouldn't be needed for read operations. What's this about? ::: dom/base/nsDOMWindowUtils.cpp @@ +2920,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsDOMWindowUtils::FlushPendingFileDeletions() Nit: Add a 'using mozilla::dom::indexedDB::IndexedDatabaseManager;' here so that you don't have to specify it twice below. ::: dom/indexedDB/ActorsParent.cpp @@ +5513,5 @@ > > +class UnlockDirectoryRunnable final > + : public nsRunnable > +{ > + friend class Database; Hm, why? @@ +5525,5 @@ > + { } > + > +private: > + ~UnlockDirectoryRunnable() > + { } Assert mDirectoryLock is null here? @@ +6408,5 @@ > + // The next step is State_DatabaseOpenPending. > + State_DirectoryWorkOpen, > + > + // Waiting for database open allowed on the main thread. The next step is > + // State_DatabaseWorkOpen. Hrm, why must we wait twice? Is this just to get to the IO thread? @@ +6545,5 @@ > > virtual void > SendResults() = 0; > > + NS_DECL_ISUPPORTS_INHERITED Ugh. @@ +7671,5 @@ > }; > > +// Maps a database id to information about live database actors. > +typedef nsClassHashtable<nsCStringHashKey, DatabaseActorInfo> > + DatabaseActorHashtable; Nit: Please move this back, see below for a better way. @@ +7816,5 @@ > > void > NoteBackgroundThread(nsIEventTarget* aBackgroundThread); > > + NS_INLINE_DECL_REFCOUNTING(indexedDB::QuotaClient, override) Hm, why is this needed? @@ +7896,3 @@ > : public nsRunnable > { > + const DatabaseActorHashtable::EnumReadFunction mEnumReadFunction; I don't think we need to have a member of this type. You know which function to call based on whether mOrigin is empty or not. @@ +11422,5 @@ > +UnlockDirectoryRunnable::Run() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + mDirectoryLock = nullptr; Assert mDirectoryLock before nulling? @@ +15202,4 @@ > // static > +PLDHashOperator > +QuotaClient:: > +AbortOperationsRunnable::MatchOrigin(const nsACString& aKey, AssertIsOnBackgroundThread() here (and in MatchContentParentId()). @@ +15211,5 @@ > + MOZ_ASSERT(aClosure); > + > + auto* closure = static_cast<AbortOperationsRunnable*>(aClosure); > + > + for (uint32_t count = aValue->mLiveDatabases.Length(), index = 0; range-for @@ +15216,5 @@ > + index < count; > + index++) { > + Database* database = aValue->mLiveDatabases[index]; > + if (closure->mOrigin.IsVoid() || closure->mOrigin == database->Origin()) { > + MOZ_ALWAYS_TRUE(closure->mDatabases.AppendElement(database)); Hm, it doesn't make any sense to have an explicitly fallible array that you then assert always succeeds... Either this needs a failure path or you should just make it infallible (I vote for the latter). @@ +15228,5 @@ > +PLDHashOperator > +QuotaClient:: > +AbortOperationsRunnable::MatchContentParentId(const nsACString& aKey, > + DatabaseActorInfo* aValue, > + void* aClosure) Nit: spacing is off here @@ +15236,5 @@ > + MOZ_ASSERT(aClosure); > + > + auto* closure = static_cast<AbortOperationsRunnable*>(aClosure); > + > + for (uint32_t count = aValue->mLiveDatabases.Length(), index = 0; range-for @@ +15260,5 @@ > + if (!gLiveDatabaseHashtable) { > + return NS_OK; > + } > + > + gLiveDatabaseHashtable->EnumerateRead(mEnumReadFunction, this); Just pick the right function based on the value of mOrigin being empty. @@ +15262,5 @@ > + } > + > + gLiveDatabaseHashtable->EnumerateRead(mEnumReadFunction, this); > + > + for (uint32_t count = mDatabases.Length(), index = 0; range-for. @@ +16301,5 @@ > + MOZ_ASSERT(mDirectoryLock); > + > + // See if this FactoryOp needs to wait. > + bool delayed = false; > + for (uint32_t index = gFactoryOps->Length(); index > 0; index--) { gFactoryOps could be null here if the child process crashed or something and that cleaned up the last Factory actor. Need to watch out for that (and handle appropriately) everywhere you do something async. @@ +16724,5 @@ > + return NS_OK; > +} > + > +bool > +FactoryOp::MustWaitFor(const FactoryOp& aExistingOp) Wow, we can simplify this a lot now, right? return aExistingOp.mCommonParams.metadata().persistenceType() == mCommonParams.metadata().persistenceType() && aExistingOp.mOrigin == mOrigin && aExistingOp.mDatabaseId == mDatabaseId; Right? @@ +16947,1 @@ > quotaClient->NoteBackgroundThread(mOwningThread); I think this is in the wrong place now, it should be moved earlier (maybe all the way to FactoryOp::Open()?). Otherwise we could miss the chance to block shutdown. ::: dom/indexedDB/IDBMutableFile.cpp @@ +180,4 @@ > storageId); > > + storageId.Append('*'); > + storageId.Append(NS_ConvertUTF16toUTF8(metadata.name())); Why do you need this? It seems like something that should be part of another patch maybe? ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +810,5 @@ > } > } > > nsresult > IndexedDatabaseManager::AsyncDeleteFile(FileManager* aFileManager, So I don't really understand why you changed this to use a timer... Why not just try to run the delete immediately like the old code did? Most of the time this is only going to be called once on a single file, right? @@ +822,2 @@ > > + nsresult rv = mDeleteTimer->Cancel(); Why would you cancel the old timer? Seems like you should let it keep running, otherwise if someone keeps calling AsyncDeleteFile you'll basically never do the delete. @@ +833,3 @@ > > + nsTArray<int64_t>* array; > + if (!mPendingDeleteInfos.Get(aFileManager, &array)) { This seems a bit overkill... I think mPendingDeleteInfos could just be an nsTArray of some struct that has a (FileManager*, int64_t) pair. @@ +889,5 @@ > return NS_OK; > } > > +nsresult > +IndexedDatabaseManager::FlushPendingFileDeletions() Not sure I understand the point of this function. There's no way to tell when it's done, and it only speeds up an async process by a second, right? (If we do end up needing this for some reason then it needs to assert that it's on the main thread.) @@ +1178,5 @@ > + > + mDirectoryLock = aLock; > + > + QuotaManager* quotaManager = QuotaManager::Get(); > + if (NS_WARN_IF(!quotaManager)) { This should be asserted right? @@ +1284,5 @@ > + } > + > + for (uint32_t count = mFileIds.Length(), index = 0; > + index < count; > + index++) { Nit: range-for ::: dom/ipc/PContent.ipdl @@ +965,5 @@ > returns (int32_t refCnt, int32_t dBRefCnt, int32_t sliceRefCnt, > bool result); > > + // Use only for testing! > + FlushPendingFileDeletions(); Hopefully we don't need this. ::: dom/quota/Client.h @@ +112,5 @@ > ReleaseIOThreadObjects() = 0; > > // Methods which are called on the main thred. > virtual void > + AbortOperations(const nsACString& aOrigin) = 0; Might as well call this AbortOperationsForOrigin ::: dom/quota/OriginScope.h @@ +10,5 @@ > #include "mozilla/dom/quota/QuotaCommon.h" > > BEGIN_QUOTA_NAMESPACE > > +class OriginScope : public nsCString Why did you rename this? ::: dom/quota/QuotaManager.cpp @@ +144,5 @@ > + nsTArray<DirectoryLockImpl*> mBlocking; > + nsTArray<DirectoryLockImpl*> mBlockedOn; > + > + const bool mExclusive; > + const bool mInternal; What does this mean? Maybe needs a better name. @@ +257,5 @@ > + InvalidateBlockedOnLocks() > + { > + MOZ_ASSERT(NS_IsMainThread()); > + > + for (uint32_t count = mBlockedOn.Length(), index = 0; range-for. @@ +273,3 @@ > }; > > +class DirectoryLockImpl::NotifyRunnable final @@ +288,5 @@ > + } > + > +private: > + ~NotifyRunnable() > + { } Assert mOpenListener and mLock are null? @@ +304,5 @@ > +{ > + uint64_t mMinSizeToBeFreed; > + > + mozilla::Mutex& mMutex; > + mozilla::CondVar mCondVar; Nit: mozilla:: not needed here, or anywhere, as long as you're inside the mozilla namespace. @@ +350,5 @@ > }; > > + State mState; > + nsresult mResultCode; > + bool mDirectoryOpenFailed; Do you really need two result variables? @@ +361,1 @@ > { } Assert which thread for all these classes. @@ +361,4 @@ > { } > > + // Reference counted. > + ~OriginOperationBase() Nit: virtual @@ +361,5 @@ > { } > > + // Reference counted. > + ~OriginOperationBase() > + { } Assert mState == State_Complete? @@ +404,5 @@ > + nsresult > + DirectoryWork(); > +}; > + > +class NormalOriginOperationBase Maybe call this "DirectoryLockOperationBase"? @@ +459,5 @@ > + DirectoryLockFailed() override; > + > + // May be overriden to send results before unblocking open. > + virtual void > + SendResults() Make this pure-virtual, it seems like a footgun. @@ +463,5 @@ > + SendResults() > + { } > +}; > + > +class SpecialOriginOperationBase There's only one subclass of this, can you just combine them and not have another base class? In any case "SpecialOriginOperationBase" doesn't tell you anything about why it's special... @@ +1378,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + for (uint32_t count = mBlocking.Length(), index = 0; > + index < count; > + index++) { range-for. @@ +1385,5 @@ > + > + mBlocking.Clear(); > + > + QuotaManager* qm = QuotaManager::Get(); > + MOZ_ASSERT(qm); Hm, is this assertable? After shutdown happens this could still be in the event queue right? I don't see anything that keeps QM alive as long as all these locks. @@ +1392,5 @@ > +} > + > +// static > +bool > +DirectoryLockImpl::MatchOriginScopes(const OriginScope& aOriginScope1, I'm assuming nothing changed in here? Or in MustWaitFor()? @@ +1467,5 @@ > + > + nsRefPtr<NotifyRunnable> runnable = > + new NotifyRunnable(mOpenListener.forget(), this); > + > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToCurrentThread(runnable))); Hm, why do we need to do this on a runnable? Bouncing again on the main thread is expensive. @@ +1485,5 @@ > + mOpenListener->DirectoryLockAcquired(mLock); > + } > + > + QuotaManager* qm = QuotaManager::Get(); > + MOZ_ASSERT(qm); As it is today this code runs asynchronously so I wonder if this is really assertable? @@ +1575,5 @@ > return gShutdown; > } > > +already_AddRefed<DirectoryLock> > +QuotaManager::OpenDirectory(Nullable<PersistenceType> aPersistenceType, Can we assert anything about these args? This is sort of a major entry point into the API so anything we can enforce here seems like a good idea. @@ +1615,5 @@ > + return lock.forget(); > +} > + > +void > +QuotaManager::RegisterDirectoryLock(DirectoryLock* aLock) Assert aLock @@ +1621,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + mDirectoryLocks.AppendElement(aLock); > + > + DirectoryLockImpl* lock = static_cast<DirectoryLockImpl*>(aLock); Hm... How is this safe? Anyone could create a subclass of DirectoryLock, right? @@ +1637,5 @@ > + GetDirectoryLockTable(persistenceType.Value()); > + > + nsTArray<DirectoryLock*>* array; > + if (!directoryLockTable.Get(originScope, &array)) { > + array = new nsTArray<DirectoryLock*>(); I wonder if this shouldn't be nsAutoTArray<1|2> or something to only have a single allocation here... @@ +1655,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + MOZ_ALWAYS_TRUE(mDirectoryLocks.RemoveElement(aLock)); > + > + DirectoryLockImpl* lock = static_cast<DirectoryLockImpl*>(aLock); Same concern @@ +1684,5 @@ > + } > +} > + > +void > +QuotaManager::RemovePendingDirectoryLock(DirectoryLock* aLock) Assert some thread/arg stuff here. @@ +1694,5 @@ > +QuotaManager::CollectOriginsForEviction( > + uint64_t aMinSizeToBeFreed, > + nsTArray<nsRefPtr<DirectoryLock>>& aLocks) > +{ > + MOZ_ASSERT(NS_IsMainThread()); assert some stuff about the arguments? @@ +1700,5 @@ > + class MOZ_STACK_CLASS Closure final > + { > + nsTArray<DirectoryLockImpl*>& mTemporaryStorageLocks; > + nsTArray<DirectoryLockImpl*>& mDefaultStorageLocks; > + nsTArray<OriginInfo*>& mOriginInfos; Nit: Call this mInactiveOriginInfos? @@ +1746,5 @@ > + GetInactiveOriginInfos(nsTArray<nsRefPtr<OriginInfo>>& aOriginInfos, > + nsTArray<DirectoryLockImpl*>& aLocks, > + nsTArray<OriginInfo*>& aInactiveOriginInfos) > + { > + for (uint32_t count = aOriginInfos.Length(), i = 0; range-for @@ +1779,5 @@ > + // Split locks into separate arrays and filter out locks for persistent > + // storage, they can't block us. > + nsTArray<DirectoryLockImpl*> temporaryStorageLocks; > + nsTArray<DirectoryLockImpl*> defaultStorageLocks; > + for (uint32_t count = mDirectoryLocks.Length(), index = 0; range-for @@ +1794,5 @@ > + } else if (persistenceType.Value() == PERSISTENCE_TYPE_TEMPORARY) { > + temporaryStorageLocks.AppendElement(lock); > + } else if (persistenceType.Value() == PERSISTENCE_TYPE_DEFAULT) { > + defaultStorageLocks.AppendElement(lock); > + } else { MOZ_ASSERT(persistenceType.Value() == P_T_PERSISTENT}; @@ +1809,5 @@ > + mGroupInfoPairs.EnumerateRead(Closure::GetInactiveTemporaryStorageOrigins, > + &closure); > + > + // Sort by the origin access time. > + inactiveOrigins.Sort(OriginInfoLRUComparator()); Don't do this, just make sure you use InsertElementSorted() in the enum function. Then they'll be sorted by the time you get here. @@ +1829,5 @@ > + if (sizeToBeFreed >= aMinSizeToBeFreed) { > + // Success, add directory locks for these origins, so any other > + // operations for them will be delayed (until origin eviction is finalized). > + > + for(uint32_t count = inactiveOrigins.Length(), index = 0; range-for @@ +2201,4 @@ > } > + }; > + > + nsAutoTArray<nsTHashtable<nsCStringHashKey>, Client::TYPE_MAX> origins; This seems a little excessive... nsAutoTArray<nsAutoPtr<nsTHashtable<nsCString>>, Client::TYPE_MAX> would be better ;) Then you can lazily create them if you need. Seems like the common case will be that there are 0 or 1 clients that have active locks, so allocating a bunch of hashtables is probably wasteful. @@ +2201,5 @@ > } > + }; > + > + nsAutoTArray<nsTHashtable<nsCStringHashKey>, Client::TYPE_MAX> origins; > + origins.AppendElements(Client::TYPE_MAX); SetLength()? @@ +2207,5 @@ > + auto* lock = static_cast<DirectoryLockImpl*>(aLock); > + const nsTArray<DirectoryLockImpl*>& blockedOnLocks = > + lock->GetBlockedOnLocks(); > + > + for (uint32_t count = blockedOnLocks.Length(), index = 0; range-for @@ +2226,5 @@ > + origins[clientType.Value()].PutEntry(originScope); > + } > + } > + > + for (uint32_t index = 0; index < Client::TYPE_MAX; index++) { range-for, e.g.: |for (uint32_t index : MakeRange(Client::TYPE_MAX)) {| @@ +2811,3 @@ > QuotaManager::GetClient(Client::Type aClientType) > { > + return mClients.SafeElementAt(aClientType); Hm, I don't remember why we thought this was a good idea... You should just MOZ_ASSERT that it's a valid type and call ElementAt I think... @@ +3277,5 @@ > "during shutdown and will be aborted!"); > > + // Abort all operations. > + for (nsRefPtr<Client>& client : mClients) { > + client->AbortOperations(NullCString()); Weird, is NullCString supposed to mean something different from EmptyCString? @@ +3534,5 @@ > doomedOrigins.AppendElement(OriginParams(persistenceType, origin, isApp)); > } > } > > for (uint32_t index = 0; index < doomedOrigins.Length(); index++) { range-for @@ +3794,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mState == State_Initial); > + > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this))); NS_DispatchToCurrentThread... but why do we need the extra event loop bounce? @@ +3827,5 @@ > + /* aInternal */ true, > + this); > + > + if (mExclusive) { > + static_cast<DirectoryLockImpl*>(lock.get())->InvalidateBlockedOnLocks(); Looks scary until the code gets changed to prevent subclasses. @@ +3844,5 @@ > + > + SendResults(); > + > + if (mDirectoryLock) { > + mDirectoryLock = nullptr; Just always null without checking. @@ +3855,5 @@ > +NormalOriginOperationBase::DirectoryLockAcquired(DirectoryLock* aLock) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mState == State_DirectoryOpenPending); > + MOZ_ASSERT(!mDirectoryLock); MOZ_ASSERT(aLock) ? @@ +3882,5 @@ > + MOZ_ASSERT(mState == State_Initial); > + > + mState = State_DirectoryOpenPending; > + > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this))); Same concern here. @@ +3899,5 @@ > + > +nsresult > +SpecialOriginOperationBase::Open() > +{ > + MOZ_CRASH("Shouldn't get here!"); Why bother overriding this then? @@ +3908,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mState == State_UnblockingOpen); > + > + for (uint32_t count = mLocks.Length(), index = 0; range-for. @@ +4110,5 @@ > + > + // Call the callback unless we were canceled. > + if (!mUsageInfo.Canceled()) { > + if (NS_FAILED(mResultCode)) { > + mUsageInfo.ResetUsage(); Wait, if something failed we just tell the page there's 0 usage? Seems like we need a followup to do something better here. @@ +4196,5 @@ > + return; > + } > + > + nsCOMPtr<nsISimpleEnumerator> entries; > + if (NS_FAILED(directory->GetDirectoryEntries(getter_AddRefs(entries))) || NS_WARN_IF(NS_FAILED(...)) @@ +4229,5 @@ > + } > + > + if (!isDirectory) { > + if (!leafName.EqualsLiteral(DSSTORE_FILE_NAME)) { > + NS_WARNING("Something in the IndexedDB directory that doesn't belong!"); This doesn't make sense... Warning and continuing will cause you to fail the Remove() below, and sleep a bunch too. Need something better here, probably just bail out and put a message with the file name in the error console. @@ +4261,5 @@ > + PR_Sleep(PR_MillisecondsToInterval(200)); > + } > + > + if (NS_FAILED(rv)) { > + NS_WARNING("Failed to remove directory, giving up!"); Hm, but you're not giving up. You're pretending that it succeeded below, which could lead to all sort of craziness later. This needs to be tightened up. @@ +4270,5 @@ > + } > + > + aQuotaManager->OriginClearCompleted(aPersistenceType, origin, isApp); > + } > + Need an NS_WARNING if NS_FAILED(rv) here. @@ +4300,5 @@ > + > + PROFILER_LABEL("Quota", "FinalizeOriginEvictionOp::DoDirectoryWork", > + js::ProfileEntry::Category::OTHER); > + > + for (uint32_t count = mLocks.Length(), index = 0; range-for. ::: dom/quota/QuotaManager.h @@ +56,5 @@ > +}; > + > +// nsISupports is needed for nsMainThreadPtrHandle<DirectoryLock> > +class NS_NO_VTABLE DirectoryLock > + : public nsISupports We need a followup bug to make nsMainThreadPtrHandle not require nsISupports, and a XXX RemoveMe comment here that references that bug. Ah, maybe bug 1164581 @@ +60,5 @@ > + : public nsISupports > +{ > +public: > + // XXX These four methods can go away once QuotaObject.cpp is merged with > + // QuotaManager.cpp When/where does this happen? @@ +74,5 @@ > + virtual const Nullable<bool>& > + GetIsApp() const = 0; > + > +protected: > + virtual ~DirectoryLock() Nit: protected default constructor too. @@ +208,2 @@ > void > + RegisterDirectoryLock(DirectoryLock* aLock); This seems like something that should be private. OpenDirectory should hand out an already-registered DirectoryLock, and the DirectoryLock destructor should call Unregister as a friend, right? @@ +227,5 @@ > const nsACString& aASCIIOrigin, > nsIFile** aDirectory) const; > > + already_AddRefed<DirectoryLock> > + OpenDirectory(Nullable<PersistenceType> aPersistenceType, It's not clear which of these two methods someone should call, or what the difference is between them. ::: dom/quota/QuotaObject.cpp @@ +195,5 @@ > MutexAutoUnlock autoUnlock(quotaManager->mQuotaMutex); > > + for (uint32_t count = locks.Length(), index = 0; > + index < count; > + index++) { Nit: Use range-for here, and elsewhere if you're already having to change stuff: for (nsRefPtr<DirectoryLock>& lock : locks) { // use |lock| }

Attaching this separately.

(In reply to Ben Kelly [PTO till June 8][:bkelly] from comment #9) > Comment on attachment 8585028 [details] [diff] [review] > patch > > Review of attachment 8585028 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall looks good. Thanks! > > I would like to fix up the state names to match the new QM call and > callbacks, though. Ok. > > I'm not quite sure what you meant about canceling the QuotaInitRunnable. I > don't see where the patch does anything special there. Can you clarify? Never mind, invalidation callbacks are gone. > > ::: dom/cache/Context.cpp > @@ +39,5 @@ > > + > > + NS_IMETHOD > > + Run() override > > + { > > + mContext->InvalidateAndAllowToClose(); > > MOZ_ASSERT(NS_IsMainThread()) for clarity please. This code doesn't exist anymore. > > @@ +188,5 @@ > > // | | > > +// +-------v-------+ | > > +// | OpenDirectory | Resolve(error) | > > +// | (Main Thread) +---------------------+ > > +// +-------+-------+ | > > I don't think this diagram matches the code any more. There are still two > states: > > 1) State to call OpenDirectory() > 2) State to wait for OpenDirectory() to call back > > We do want to change the names of the states to match the new operation, > though. > > Can you create STATE_OPEN_DIRECTORY and STATE_WAIT_FOR_DIRECTORY_LOCK or > something? And then update the diagram accordingly? Ok. > > @@ +247,2 @@ > > // a callback. We will then get executed again on the main thread when > > // it is safe to open the quota directory. > > Please note that OpenDirectory() will call back in DirectoryLockAcquired(), > etc. Ok. > > @@ +258,4 @@ > > break; > > } > > // ------------------------------ > > case STATE_WAIT_FOR_OPEN_ALLOWED: > > You remove this state from the comments, but its still in the code. > > I think we still want a state here to assert on, but we don't need to switch > on it Run() anymore. Just do the right thing in DirectoryLockAcquired(), > etc. I added |case STATE_WAIT_FOR_DIRECTORY_LOCK:| before |default:|. > > @@ +358,5 @@ > > +void > > +Context::QuotaInitRunnable::DirectoryLockAcquired(DirectoryLock* aLock) > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + MOZ_ASSERT(mState == STATE_WAIT_FOR_OPEN_ALLOWED); > > See above comment about updating the state name to match the operation that > is being done. Yeah, it's now |STATE_WAIT_FOR_DIRECTORY_LOCK|. > > @@ +367,5 @@ > > + aLock->SetInvalidateCallback(callback); > > + > > + mDirectoryLock = new nsMainThreadPtrHolder<DirectoryLock>(aLock); > > + > > + Run(); > > Please just move the logic from Run()'s STATE_WAIT_FOR_OPEN_ALLOWED case > into here. Ok. > > @@ +377,5 @@ > > + MOZ_ASSERT(NS_IsMainThread()); > > + MOZ_ASSERT(mState == STATE_WAIT_FOR_OPEN_ALLOWED); > > + MOZ_ASSERT(!mDirectoryLock); > > + > > + Run(); > > Please call resolve->Resolve(NS_ERROR_FAILURE) directly instead of calling > back into Run(). Ok. > > ::: dom/cache/Context.h > @@ +47,5 @@ > > // 1) The Manager will call Context::AllowToClose() when all of the actors > > // have removed themselves as listener. This means an idle context with > > // no active DOM objects will close gracefully. > > // 2) The QuotaManager invalidates the storage area so it can delete the > > +// files. In this case the InvalidateCallback calls Cache::Invalidate() > > Since you are touching this, can you fix my typo? Should be > Context::Invalidate() instead of Cache::Invalidate(). > This changed too, QM now calls |Client::AbortOperations()|. > ::: dom/quota/QuotaManager.h > @@ +232,5 @@ > > const nsACString& aASCIIOrigin, > > nsIFile** aDirectory) const; > > > > + already_AddRefed<DirectoryLock> > > + OpenDirectory(const OptionalContentId& aOptionalContentParentId, > > Please add comments documenting the OpenDirectory() API calls. Ok.

(In reply to Ben Kelly [PTO till June 8][:bkelly] from comment #14) > Comment on attachment 8599439 [details] [diff] [review] > patch > > Review of attachment 8599439 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall looks reasonable, but I'd like to understand the changes to > ManagerId better. Also, the error paths in the Context directory lock > methods are broken. > > ::: dom/cache/Context.cpp > @@ +145,5 @@ > > NS_DECL_THREADSAFE_ISUPPORTS > > NS_DECL_NSIRUNNABLE > > + > > + virtual void > > + DirectoryLockAcquired(DirectoryLock* aLock) override; > > Please put the OpenDirectoryListener methods up in the first public: section > of the class. Also please add a comment like "// OpenDirectoryListener > methods". Ok, but you also inherit from nsIRunnable, so I thought |OpenDirectoryListener| methods should go after |NS_DECL_NSIRUNNABLE| > > @@ +161,5 @@ > > // | (Orig Thread) +---------------------+ > > // +-------+-------+ | > > // | | > > // +----------v-----------+ | > > +// | CallOpenDirectory | Resolve(error) | > > The new state is just "OpenDirectory" instead of "CallOpenDirectory". > Please fix one or the other. Ah, it should match the enums, ok. I somehow confused it with method names. > > @@ +166,5 @@ > > // | (Main Thread) +-----------------+ > > // +----------+-----------+ | > > // | | > > +// +-------v-------+ | > > +// | OpenDirectory | Resolve(error) | > > I think this should be WaitForDirectoryLock. Ok. > > @@ +317,5 @@ > > + MOZ_ASSERT(!mDirectoryLock); > > + > > + mDirectoryLock = new nsMainThreadPtrHolder<DirectoryLock>(aLock); > > + > > + nsRefPtr<SyncResolver> resolver = new SyncResolver(); > > The use of SyncResolver() is incorrect here. Its only used in Run() because > we need to pass it to the Action. > > In any case, you need to do this logic from the end of Run(): > > mState = STATE_COMPLETING; > MOZ_ALWAYS_TRUE(NS_SUCCEEDED( > mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL))); > > I suppose you could wrap that up in a Complete(nsresult rv) private method. > You can then do Complete(NS_ERROR_ABORT), etc. Ok. > > @@ +319,5 @@ > > + mDirectoryLock = new nsMainThreadPtrHolder<DirectoryLock>(aLock); > > + > > + nsRefPtr<SyncResolver> resolver = new SyncResolver(); > > + > > + if (mCanceled) { > > NS_WARN_IF Are you sure ? It's just |if (mCanceled) {| elsewhere. > > @@ +336,5 @@ > > + } > > +} > > + > > +void > > +Context::QuotaInitRunnable::DirectoryLockFailed() > > NS_WARNING() or NS_WARN_IF() in this method please. Ok. > > ::: dom/cache/Manager.cpp > @@ +372,5 @@ > > + } > > + > > + // Can't call Manager::Abort() while looping through the manager list since > > + // Manager::Abort() can end up removing the Manager from the list. > > + // So we have to put all matching managers to an array. > > nsTObserverArray is designed to allow removals while iterating the list. > The copy is not necessary. I'm pretty sure I hit an assertion when I tried to do it directly in the |while| loop. Anyway, I don't hit it anymore. > > @@ +1909,5 @@ > > + NoteClosing(); > > + > > + // If there is a context, then cancel and only note that we are done after > > + // its cleaned up. > > + if (mContext) { > > You can MOZ_ASSERT(mContext) here. The factory should now never have a > reference to a Manager without a Context. Ok, can we do the same in Manager::Shutdown() ? > > ::: dom/cache/Manager.h > @@ +130,5 @@ > > > > // Synchronously shutdown from main thread. This spins the event loop. > > static void ShutdownAllOnMainThread(); > > > > + static void AbortOnMainThread(const nsACString& aOrigin); > > Please add a comment about what this does. Also note that an IsVoid() > string means to abort all Managers. Ok. > > ::: dom/cache/ManagerId.cpp > @@ +41,3 @@ > > if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > > > > + nsRefPtr<ManagerId> ref = new ManagerId(aPrincipal, jarPrefix + origin); > > I really dislike calling something an origin if its actually the origin > combined with something else. Its terribly confusing for anyone trying to > understand the API. > > Please have ManagerId() take jarPrefix and origin separately. Ok, I also renamed origin to extendedOrigin in ManagerId. > > And can you tell me why this change is necessary for this patch at all? It > seems unrelated. We already discussed about this.

(In reply to Ben Turner <vacation until 6.8> (use the needinfo flag!) from comment #18) > ::: dom/asmjscache/AsmJSCache.cpp > @@ +954,2 @@ > > > > + // XXX The exclusive lock shouldn't be needed for read operations. > > What's this about? WaitForOpenAllowed() always gives you exclusive access, so if two pages try to load cached asm.js for the same origin, QM will allow only one at a time to do it. However if you pass aExclusive=false to OpenDirectory(), those two pages will both get directory locks from QM, so they can load asm.js simultaneously in theory (the load will end up on the same IO thread currently, but that's a different issue). That's why I think asmjscache could pass aExclusive=false for mOpenMode=eOpenForRead. I didn't want to change it in this patch. I'll file a new bug for it. Bug 1168734. Anyway, as of today we have to keep the exclusive lock since even reads need to update a metadata file (asmjscache specific metadata file). > ::: dom/indexedDB/ActorsParent.cpp > @@ +5513,5 @@ > > > > +class UnlockDirectoryRunnable final > > + : public nsRunnable > > +{ > > + friend class Database; > > Hm, why? Forgot to remove it, done. > > @@ +5525,5 @@ > > + { } > > + > > +private: > > + ~UnlockDirectoryRunnable() > > + { } > > Assert mDirectoryLock is null here? Done. > > @@ +6408,5 @@ > > + // The next step is State_DatabaseOpenPending. > > + State_DirectoryWorkOpen, > > + > > + // Waiting for database open allowed on the main thread. The next step is > > + // State_DatabaseWorkOpen. > > Hrm, why must we wait twice? Is this just to get to the IO thread? Yes, dispatches to the IO thread can only happen from the owning thread (main thread in this case). > > @@ +6545,5 @@ > > > > virtual void > > SendResults() = 0; > > > > + NS_DECL_ISUPPORTS_INHERITED > > Ugh. ? > @@ +7816,5 @@ > > > > void > > NoteBackgroundThread(nsIEventTarget* aBackgroundThread); > > > > + NS_INLINE_DECL_REFCOUNTING(indexedDB::QuotaClient, override) > > Hm, why is this needed? That line doesn't exist anymore. > @@ +11422,5 @@ > > +UnlockDirectoryRunnable::Run() > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + mDirectoryLock = nullptr; > > Assert mDirectoryLock before nulling? Ok > @@ +16724,5 @@ > > + return NS_OK; > > +} > > + > > +bool > > +FactoryOp::MustWaitFor(const FactoryOp& aExistingOp) > > Wow, we can simplify this a lot now, right? > > return aExistingOp.mCommonParams.metadata().persistenceType() == > mCommonParams.metadata().persistenceType() && > aExistingOp.mOrigin == mOrigin && > aExistingOp.mDatabaseId == mDatabaseId; > > Right? Right. I thought I could sacrifice performance here and rather have explicitly commented conditions. Like we have in MaybeCommitOrAbort() in ActorsParent.cpp. > > @@ +16947,1 @@ > > quotaClient->NoteBackgroundThread(mOwningThread); > > I think this is in the wrong place now, it should be moved earlier (maybe > all the way to FactoryOp::Open()?). Otherwise we could miss the chance to > block shutdown. Ok, moved to Open(). > > ::: dom/indexedDB/IDBMutableFile.cpp > @@ +180,4 @@ > > storageId); > > > > + storageId.Append('*'); > > + storageId.Append(NS_ConvertUTF16toUTF8(metadata.name())); > > Why do you need this? It seems like something that should be part of another > patch maybe? QuotaManager::GetStorageId() no longer appends db name, so IDB must do it here and in ActorsParent.cpp > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +810,5 @@ > > } > > } > > > > nsresult > > IndexedDatabaseManager::AsyncDeleteFile(FileManager* aFileManager, > > So I don't really understand why you changed this to use a timer... Why not > just try to run the delete immediately like the old code did? Most of the > time this is only going to be called once on a single file, right? No! There are cases when stored files are deleted multiple at once. Take a look at dom/indexedDB/test/test_file_replace.html When the test finishes, GC runs and IndexedDatabaseManager::AsyncDeleteFile() is called 100 times in a row. So far it was ok, but now the operation uses a directory lock, so we would need to get 100 directory locks and do all the thread hoping 100 times. > > @@ +822,2 @@ > > > > + nsresult rv = mDeleteTimer->Cancel(); > > Why would you cancel the old timer? Seems like you should let it keep > running, otherwise if someone keeps calling AsyncDeleteFile you'll basically > never do the delete. The idea is to group as many file deletions as possible. After the last AsyncDeleteFile() is called the timer will fire after a second. Someone would have to continuously call the method every second or even more frequently to never do the delete. That's very unlikely, don't you think ? > > @@ +833,3 @@ > > > > + nsTArray<int64_t>* array; > > + if (!mPendingDeleteInfos.Get(aFileManager, &array)) { > > This seems a bit overkill... I think mPendingDeleteInfos could just be an > nsTArray of some struct that has a (FileManager*, int64_t) pair. There's always one FileManager per origin, and I'd like to have an array of file ids per origin so I can request just one directory lock for it. > > @@ +889,5 @@ > > return NS_OK; > > } > > > > +nsresult > > +IndexedDatabaseManager::FlushPendingFileDeletions() > > Not sure I understand the point of this function. There's no way to tell > when it's done, and it only speeds up an async process by a second, right? Originally I had this: setTimeout(continueToNextStepSync, 2000); instead of: flushPendingFileDeletions(); in test_file_os_delete.html but the test fails with "Test attempted to use a flaky timeout value 2000" I could set |SimpleTest._flakyTimeoutIsOK = true| but I'm not sure if it's the best solution. The fact that there's no callback is not a problem. The flushing happens on the same thread as getUsage(), so I just need to wait for the following getUsage() to finish. > > (If we do end up needing this for some reason then it needs to assert that > it's on the main thread.) > > @@ +1178,5 @@ > > + > > + mDirectoryLock = aLock; > > + > > + QuotaManager* quotaManager = QuotaManager::Get(); > > + if (NS_WARN_IF(!quotaManager)) { > > This should be asserted right? Right. > ::: dom/quota/Client.h > @@ +112,5 @@ > > ReleaseIOThreadObjects() = 0; > > > > // Methods which are called on the main thred. > > virtual void > > + AbortOperations(const nsACString& aOrigin) = 0; > > Might as well call this AbortOperationsForOrigin That is also used to abort all operations (when passed string is null/void). > > ::: dom/quota/OriginScope.h > @@ +10,5 @@ > > #include "mozilla/dom/quota/QuotaCommon.h" > > > > BEGIN_QUOTA_NAMESPACE > > > > +class OriginScope : public nsCString > > Why did you rename this? It's no longer origin or pattern, but origin or pattern or null and the original name seemed too long. > > ::: dom/quota/QuotaManager.cpp > @@ +144,5 @@ > > + nsTArray<DirectoryLockImpl*> mBlocking; > > + nsTArray<DirectoryLockImpl*> mBlockedOn; > > + > > + const bool mExclusive; > > + const bool mInternal; > > What does this mean? Maybe needs a better name. Added a comment. > @@ +350,5 @@ > > }; > > > > + State mState; > > + nsresult mResultCode; > > + bool mDirectoryOpenFailed; > > Do you really need two result variables? Ok, I removed mDirectoryOpenFailed and reworked the code a bit to not call Run() from DirectoryLockAcquired/DirectoryLockFailed. > > @@ +361,1 @@ > > { } > > Assert which thread for all these classes. Ok, but can't do it in OriginOperationBase::OriginOperationBase > @@ +404,5 @@ > > + nsresult > > + DirectoryWork(); > > +}; > > + > > +class NormalOriginOperationBase > > Maybe call this "DirectoryLockOperationBase"? Hm, the special "FinalizeOriginEvictionOp" is also about directory locks. I would keep the "normal" prefix since it's a base class for ops that run through all the states and the "special" one skips the first state and can also run on the IO thread directly. > > @@ +463,5 @@ > > + SendResults() > > + { } > > +}; > > + > > +class SpecialOriginOperationBase > > There's only one subclass of this, can you just combine them and not have > another base class? Ok > > In any case "SpecialOriginOperationBase" doesn't tell you anything about why > it's special... > > @@ +1385,5 @@ > > + > > + mBlocking.Clear(); > > + > > + QuotaManager* qm = QuotaManager::Get(); > > + MOZ_ASSERT(qm); > > Hm, is this assertable? After shutdown happens this could still be in the > event queue right? I don't see anything that keeps QM alive as long as all > these locks. Oh man, ok, thanks for pointing this out. I had to do quite a bit of refactoring to fix this. I created a directory lock manager that can be invalidated at shutdown, but still referenced by directory lock objects. > > @@ +1392,5 @@ > > +} > > + > > +// static > > +bool > > +DirectoryLockImpl::MatchOriginScopes(const OriginScope& aOriginScope1, > > I'm assuming nothing changed in here? Or in MustWaitFor()? MatchOriginScopes() is just factored out code from MustWaitFor(), so it can be used by origin eviction code. MustWaitFor() changed a bit, some checks were removed/added/reordered. > > @@ +1467,5 @@ > > + > > + nsRefPtr<NotifyRunnable> runnable = > > + new NotifyRunnable(mOpenListener.forget(), this); > > + > > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToCurrentThread(runnable))); > > Hm, why do we need to do this on a runnable? Bouncing again on the main > thread is expensive. Removed the notify runnable. > > @@ +1485,5 @@ > > + mOpenListener->DirectoryLockAcquired(mLock); > > + } > > + > > + QuotaManager* qm = QuotaManager::Get(); > > + MOZ_ASSERT(qm); > > As it is today this code runs asynchronously so I wonder if this is really > assertable? Hopefully solved by creating the directory lock manager. > > @@ +1575,5 @@ > > return gShutdown; > > } > > > > +already_AddRefed<DirectoryLock> > > +QuotaManager::OpenDirectory(Nullable<PersistenceType> aPersistenceType, > > Can we assert anything about these args? This is sort of a major entry point > into the API so anything we can enforce here seems like a good idea. > Ok, I duplicated what we have in DirectoryLockImpl ctor. > @@ +1621,5 @@ > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + mDirectoryLocks.AppendElement(aLock); > > + > > + DirectoryLockImpl* lock = static_cast<DirectoryLockImpl*>(aLock); > > Hm... How is this safe? Anyone could create a subclass of DirectoryLock, > right? I temporarily moved DirectoryLockImpl out of the anonymous namespace, it's now under QuotaManger. This allowed me to remove these static casts. I have a patch for extracting implementation details out of QuotaManager.h, once that lands DirectoryLockImpl will be moved back to anonymous namespace. Also, RegisterDirectoryLock() is now private. > > @@ +1637,5 @@ > > + GetDirectoryLockTable(persistenceType.Value()); > > + > > + nsTArray<DirectoryLock*>* array; > > + if (!directoryLockTable.Get(originScope, &array)) { > > + array = new nsTArray<DirectoryLock*>(); > > I wonder if this shouldn't be nsAutoTArray<1|2> or something to only have a > single allocation here... > You mean nsAutoTArray<DirectoryLock*, 1> or nsAutoTArray<DirectoryLock*, 2> ? Is it worth it ? > @@ +1655,5 @@ > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + MOZ_ALWAYS_TRUE(mDirectoryLocks.RemoveElement(aLock)); > > + > > + DirectoryLockImpl* lock = static_cast<DirectoryLockImpl*>(aLock); > > Same concern The static cast is now gone > @@ +1694,5 @@ > > +QuotaManager::CollectOriginsForEviction( > > + uint64_t aMinSizeToBeFreed, > > + nsTArray<nsRefPtr<DirectoryLock>>& aLocks) > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > assert some stuff about the arguments? Ok > > @@ +1700,5 @@ > > + class MOZ_STACK_CLASS Closure final > > + { > > + nsTArray<DirectoryLockImpl*>& mTemporaryStorageLocks; > > + nsTArray<DirectoryLockImpl*>& mDefaultStorageLocks; > > + nsTArray<OriginInfo*>& mOriginInfos; > > Nit: Call this mInactiveOriginInfos? Ok > @@ +2201,5 @@ > > } > > + }; > > + > > + nsAutoTArray<nsTHashtable<nsCStringHashKey>, Client::TYPE_MAX> origins; > > + origins.AppendElements(Client::TYPE_MAX); > > SetLength()? Ok > @@ +2226,5 @@ > > + origins[clientType.Value()].PutEntry(originScope); > > + } > > + } > > + > > + for (uint32_t index = 0; index < Client::TYPE_MAX; index++) { > > range-for, e.g.: |for (uint32_t index : MakeRange(Client::TYPE_MAX)) {| this seems to work: |for (uint32_t index : MakeRange(uint32_t(Client::TYPE_MAX))) {| > @@ +3277,5 @@ > > "during shutdown and will be aborted!"); > > > > + // Abort all operations. > > + for (nsRefPtr<Client>& client : mClients) { > > + client->AbortOperations(NullCString()); > > Weird, is NullCString supposed to mean something different from EmptyCString? Yes, NullCString means abort all operations, I didn't want to add another method to the client interface and hoped that AbortOperationsForProcess will get removed at some point. > @@ +3794,5 @@ > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + MOZ_ASSERT(mState == State_Initial); > > + > > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this))); > > NS_DispatchToCurrentThread... but why do we need the extra event loop bounce? Hm, SavaOriginAccessTimeOp was using that, but that can be changed to RunImmediately() too so I removed Dispatch() all together. > > @@ +3827,5 @@ > > + /* aInternal */ true, > > + this); > > + > > + if (mExclusive) { > > + static_cast<DirectoryLockImpl*>(lock.get())->InvalidateBlockedOnLocks(); > > Looks scary until the code gets changed to prevent subclasses. The static_cast is now gone. > @@ +3855,5 @@ > > +NormalOriginOperationBase::DirectoryLockAcquired(DirectoryLock* aLock) > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + MOZ_ASSERT(mState == State_DirectoryOpenPending); > > + MOZ_ASSERT(!mDirectoryLock); > > MOZ_ASSERT(aLock) ? Ok > > @@ +3882,5 @@ > > + MOZ_ASSERT(mState == State_Initial); > > + > > + mState = State_DirectoryOpenPending; > > + > > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this))); > > Same concern here. In this case, we are not on the main thread and we need to get there. > > @@ +3899,5 @@ > > + > > +nsresult > > +SpecialOriginOperationBase::Open() > > +{ > > + MOZ_CRASH("Shouldn't get here!"); > > Why bother overriding this then? Open() is pure virtual in OriginOperationBase and FinalizedOriginEvictionOp (SpecialOriginOperationBase before) always gets already acquired locks in the constructor, so Open() shouldn't be called. > @@ +4110,5 @@ > > + > > + // Call the callback unless we were canceled. > > + if (!mUsageInfo.Canceled()) { > > + if (NS_FAILED(mResultCode)) { > > + mUsageInfo.ResetUsage(); > > Wait, if something failed we just tell the page there's 0 usage? Seems like > we need a followup to do something better here. Yes, we just report zero usage. Filed bug 1170019. > @@ +4229,5 @@ > > + } > > + > > + if (!isDirectory) { > > + if (!leafName.EqualsLiteral(DSSTORE_FILE_NAME)) { > > + NS_WARNING("Something in the IndexedDB directory that doesn't belong!"); > > This doesn't make sense... Warning and continuing will cause you to fail the > Remove() below, and sleep a bunch too. Need something better here, probably > just bail out and put a message with the file name in the error console. I don't understand, this is just a case when we find a file (not directory) under <profile>/storage/permanent|temporary|default But it's true that when we initialize repositories, a similar case is handled differently, we return NS_ERROR_UNEXPECTED. For now, I changed the warning as you suggested. > > @@ +4261,5 @@ > > + PR_Sleep(PR_MillisecondsToInterval(200)); > > + } > > + > > + if (NS_FAILED(rv)) { > > + NS_WARNING("Failed to remove directory, giving up!"); > > Hm, but you're not giving up. You're pretending that it succeeded below, > which could lead to all sort of craziness later. This needs to be tightened > up. Actually, I haven't changed anything here, the original patch shuffled it probably. However the latest patch does a better job and this part isn't there anymore. If you still want to fix this somehow, let me know. > > @@ +4270,5 @@ > > + } > > + > > + aQuotaManager->OriginClearCompleted(aPersistenceType, origin, isApp); > > + } > > + > > Need an NS_WARNING if NS_FAILED(rv) here. See above. > ::: dom/quota/QuotaManager.h > @@ +56,5 @@ > > +}; > > + > > +// nsISupports is needed for nsMainThreadPtrHandle<DirectoryLock> > > +class NS_NO_VTABLE DirectoryLock > > + : public nsISupports > > We need a followup bug to make nsMainThreadPtrHandle not require > nsISupports, and a XXX RemoveMe comment here that references that bug. > > Ah, maybe bug 1164581 Ok, thanks. > > @@ +60,5 @@ > > + : public nsISupports > > +{ > > +public: > > + // XXX These four methods can go away once QuotaObject.cpp is merged with > > + // QuotaManager.cpp > > When/where does this happen? Bug 1170021, I updated the comment. > @@ +208,2 @@ > > void > > + RegisterDirectoryLock(DirectoryLock* aLock); > > This seems like something that should be private. OpenDirectory should hand > out an already-registered DirectoryLock, and the DirectoryLock destructor > should call Unregister as a friend, right? > Yeah, RegisterDirectoryLock, UnregisterDirectoryLock, RemovePendingDirectoryLock are now private. > @@ +227,5 @@ > > const nsACString& aASCIIOrigin, > > nsIFile** aDirectory) const; > > > > + already_AddRefed<DirectoryLock> > > + OpenDirectory(Nullable<PersistenceType> aPersistenceType, > > It's not clear which of these two methods someone should call, or what the > difference is between them. The one that returns a lock should be private, but NormalOriginOperationBase::Open needs to call it and I really don't want to move that class out of anonymous namespace. I filed bug 1170279 for extracting implementation details out of QuotaManager.h, that will allow us to remove the "private" OpenDirectory() method from QuotaManager.h The other "public" OpenDirectory() now has some comments. Actually, the new directory lock manager allowed me to remove the "private" OpenDirectory() method from QuotaManager.

Comment on attachment 8610218 [details] [diff] [review] Idle maintenance converted to use directory locks Review of attachment 8610218 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsParent.cpp @@ +16430,5 @@ > + aMaintenanceInfo.mGroup, > + aMaintenanceInfo.mOrigin, > + aMaintenanceInfo.mIsApp, > + Client::IDB, > + /* aExclusive */ true, I actually had to change this to |/* aExclusive */ false|, but we need to talk about this whole thing again. The problem with requesting exclusive access here is that there are some permanent services which keep open IDB databases. So when we get the idle notification, there is a directory lock which prevents a new exclusive lock from being acquired. The current code which uses WaitForOpenAllowed() isn't safe either. At the time we call WaitForOpenAllowed(), there can be IDB database which already passed the opening procedure involving WaitForOpenAllowed() and they are now "protected" by calling RegisterStorage(). But we don't check these storage objects here ...

Try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=56fc30118005

> > ::: dom/cache/Manager.cpp > > @@ +372,5 @@ > > > + } > > > + > > > + // Can't call Manager::Abort() while looping through the manager list since > > > + // Manager::Abort() can end up removing the Manager from the list. > > > + // So we have to put all matching managers to an array. > > > > nsTObserverArray is designed to allow removals while iterating the list. > > The copy is not necessary. > > I'm pretty sure I hit an assertion when I tried to do it directly in the |while| > loop. Anyway, I don't hit it anymore. This was the assertion: [451] ###!!! ASSERTION: iterators outlasting array: 'mIterators == nullptr', file ../../dist/include/nsTObserverArray.h, line 55 #01: mozilla::dom::cache::Context::~Context() (nsRefPtr.h:231, in XUL) #02: mozilla::dom::cache::Context::Release() (mozalloc.h:210, in XUL) #03: mozilla::dom::cache::Manager::Factory::AbortOnBackgroundThread(nsACString_internal const&) (Manager.cpp:366, in XUL) #04: mozilla::dom::cache::Manager::Factory::AbortRunnable::Run() (Manager.cpp:426, in XUL) #05: nsThread::ProcessNextEvent(bool, bool*) (nsCOMPtr.h:389, in XUL) #06: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:265, in XUL) #07: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (MessagePump.cpp:368, in XUL) #08: MessageLoop::Run() (message_loop.cc:517, in XUL) #09: nsThread::ThreadFunc(void*) (nsThread.cpp:366, in XUL) #10: _pt_root (ptthread.c:215, in libnss3.dylib) #11: _pthread_body (in libsystem_pthread.dylib) + 131 #12: _pthread_body (in libsystem_pthread.dylib) + 0

Comment on attachment 8614573 [details] [diff] [review] interdiff Review of attachment 8614573 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/QuotaManager.h @@ +60,1 @@ > class NS_NO_VTABLE DirectoryLock Hrm, I'm still not thrilled about this. Can we try this setup instead? I think this will mean that 1) nobody but QuotaManager can create subclasses of DirectoryLock or create instances of DirectoryLock on their own, and 2) you can always static_cast<> back and forth between DirectoryLock and DirectoryLockImpl without danger. In the header: class QuotaManager { // ... public: class DirectoryLock; already_AddRefed<DirectoryLock> FunctionThatReturnsLock(); private: class DirectoryLockImpl; }; class QuotaManager::DirectoryLock : public nsISupports { friend class DirectoryLockImpl; public: /* non-virtual */ const Nullable<PersistenceType>& GetPersistenceType() const; // ... private: DirectoryLock(); ~DirectoryLock(); }; Then in the cpp: const Nullable<PersistenceType>& QuotaManager::DirectoryLock::GetPersistenceType() const { // ... // Maybe static_cast<const DirectoryLockImpl*>(this) if needed. } class QuotaManager::DirectoryLockImpl : public QuotaManager::DirectoryLock { // ... NS_DECL_ISUPPORTS };

Comment on attachment 8610218 [details] [diff] [review] Idle maintenance converted to use directory locks Review of attachment 8610218 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsParent.cpp @@ +8033,5 @@ > // will call PerformIdleMaintenanceOnDatabase. > void > + ScheduleIdleMaintenance(uint32_t aRunId, > + const nsACString& aKey, > + MultipleMaintenanceInfo& aMaintenanceInfo); Nit: |const MultipleMaintenanceInfo&| here @@ +17229,5 @@ > +#ifdef DEBUG > + maintenanceInfo->mDatabasePaths.Clear(); > +#endif > + > + mQuotaClient->mMaintenanceInfoHashtable->Remove(mKey); MOZ_ALWAYS_TRUE @@ +17250,5 @@ > + mQuotaClient->mMaintenanceInfoHashtable->Get(mKey, &maintenanceInfo)); > + MOZ_ASSERT(maintenanceInfo); > + MOZ_ASSERT(!maintenanceInfo->mDirectoryLock); > + > + mQuotaClient->mMaintenanceInfoHashtable->Remove(mKey); MOZ_ALWAYS_TRUE, then you don't have to do the other assertions/DebugOnly thing above

Comment on attachment 8614573 [details] [diff] [review] interdiff Review of attachment 8614573 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: dom/quota/QuotaManager.cpp @@ +278,5 @@ > NS_DECL_ISUPPORTS > }; > > +class QuotaManager::DirectoryLockManager final > + : public RefCountedObject Why does this need to be refcounted? @@ +300,5 @@ > + DirectoryLockManager() > + : mInvalidated(false) > + { } > + > + NS_INLINE_DECL_REFCOUNTING(DirectoryLockManager) private dtor? @@ +364,5 @@ > + aTemporaryStorageLocks.AppendElement(lock); > + } else if (persistenceType.Value() == PERSISTENCE_TYPE_DEFAULT) { > + aDefaultStorageLocks.AppendElement(lock); > + } else { > + MOZ_ASSERT(persistenceType.Value() == PERSISTENCE_TYPE_PERSISTENT); Nit: Maybe also comment that persistent origins don't need to be collected ever so that's why you're not doing anything. @@ +367,5 @@ > + } else { > + MOZ_ASSERT(persistenceType.Value() == PERSISTENCE_TYPE_PERSISTENT); > + } > + } > +} Nit: indent @@ +1553,5 @@ > + > + if (aOriginScope2.IsNull() || aOriginScope1.IsNull()) { > + match = true; > + } > + else if (aOriginScope2.IsOrigin()) { Nit: Might as well cuddle all these else statements @@ +1573,5 @@ > + if (!match) { > + return false; > + } > + > + return true; |return match;| here? @@ +1659,5 @@ > MOZ_ASSERT_IF(!aInternal, aOpenListener); > + > + nsRefPtr<DirectoryLockImpl> lock = > + new DirectoryLockImpl(this, aPersistenceType, aGroup, aOriginScope, aIsApp, > + aClientType, aExclusive, aInternal, aOpenListener); Nit: One arg per line would be easier to read. Like you do below. @@ +1750,5 @@ > + directoryLockTable.Remove(originScope); > + > + if (!mInvalidated) { > + QuotaManager* qm = QuotaManager::Get(); > + MOZ_ASSERT(qm); Can this happen after shutdown? @@ +1903,5 @@ > uint64_t aMinSizeToBeFreed, > nsTArray<nsRefPtr<DirectoryLock>>& aLocks) > { > MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(aMinSizeToBeFreed > 0); Nit: No need for |> 0| @@ +2028,5 @@ > if (sizeToBeFreed >= aMinSizeToBeFreed) { > // Success, add directory locks for these origins, so any other > // operations for them will be delayed (until origin eviction is finalized). > > + for(OriginInfo* originInfo : inactiveOrigins) { Nit: Space between |for(| @@ +4554,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mState == State_UnblockingOpen); > + > + for (nsRefPtr<DirectoryLock>& lock : mLocks) { > + lock = nullptr; Maybe |mLocks.Clear();| instead?

Comment on attachment 8614573 [details] [diff] [review] interdiff Er, didn't mean to r+, because I think we should rework the DirectoryLock thing. But this looks mostly great!

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #28) > Comment on attachment 8614573 [details] [diff] [review] > interdiff > > Review of attachment 8614573 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/quota/QuotaManager.h > @@ +60,1 @@ > > class NS_NO_VTABLE DirectoryLock > > Hrm, I'm still not thrilled about this. Can we try this setup instead? I > think this will mean that 1) nobody but QuotaManager can create subclasses > of DirectoryLock or create instances of DirectoryLock on their own, and 2) > you can always static_cast<> back and forth between DirectoryLock and > DirectoryLockImpl without danger. > Ok, I did the change and it compiles on mac (will check other platforms).

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #29) > @@ +17229,5 @@ > > +#ifdef DEBUG > > + maintenanceInfo->mDatabasePaths.Clear(); > > +#endif > > + > > + mQuotaClient->mMaintenanceInfoHashtable->Remove(mKey); > > MOZ_ALWAYS_TRUE This is a hash table (not an array) so Remove() returns void. > > @@ +17250,5 @@ > > + mQuotaClient->mMaintenanceInfoHashtable->Get(mKey, &maintenanceInfo)); > > + MOZ_ASSERT(maintenanceInfo); > > + MOZ_ASSERT(!maintenanceInfo->mDirectoryLock); > > + > > + mQuotaClient->mMaintenanceInfoHashtable->Remove(mKey); > > MOZ_ALWAYS_TRUE, then you don't have to do the other assertions/DebugOnly > thing above See above.

Ok, the QuotaManager and DirectoryLockManager merge is the last thing that blocks this from landing. Will try to fix that between work week sessions. (In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #30) > Comment on attachment 8614573 [details] [diff] [review] > interdiff > > Review of attachment 8614573 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good! > > ::: dom/quota/QuotaManager.cpp > @@ +278,5 @@ > > NS_DECL_ISUPPORTS > > }; > > > > +class QuotaManager::DirectoryLockManager final > > + : public RefCountedObject > > Why does this need to be refcounted? We discussed about this on IRC and it seems I should try to merge QuotaManager and DirectoryLockManager. > > @@ +300,5 @@ > > + DirectoryLockManager() > > + : mInvalidated(false) > > + { } > > + > > + NS_INLINE_DECL_REFCOUNTING(DirectoryLockManager) > > private dtor? Yeah, it is. It wouldn't compile otherwise. > @@ +1750,5 @@ > > + directoryLockTable.Remove(originScope); > > + > > + if (!mInvalidated) { > > + QuotaManager* qm = QuotaManager::Get(); > > + MOZ_ASSERT(qm); > > Can this happen after shutdown? Hm, it seems it can, I changed the check to |if (!mInvalidated && !QuotaManager::IsShuttingDown()) {|

Try looks good, per bkelly's request I'm going to land this right after the uplift on Monday.

Thanks!

The "SendResults" overrides here were missing the 'override' annotation, which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+). When inbound reopens, I'll push a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2.

Pushed 1 more followup ^^, to add 'override' to QuotaClient::GetDirectoryLockListener refcounting methods as well. (They're overriding pure virtual declarations of those methods, declared on grandparent-superclass "RefCountedObject", which is in QuotaManager.h.)

dholbert: sorry and thanks :)

https://hg.mozilla.org/mozilla-central/rev/c9b3704bd0ed https://hg.mozilla.org/mozilla-central/rev/e93fd5c79235 https://hg.mozilla.org/mozilla-central/rev/335268dfcd2c