This is a followup for bug 787804. See https://bugzilla.mozilla.org/show_bug.cgi?id=787804#c13 especially this part: @@ +103,5 @@ > + } > + > + int64_t newUsage = mOriginInfo->mUsage - mSize + end; > + if (newUsage > mOriginInfo->mLimit) { > + if (!indexedDB::IndexedDatabaseManager::QuotaIsLifted()) { We need a followup to move all this machinery into the QuotaManager. I'm really nervous about us using two mutexes for this (mQuotaMutex as well as the one in IndexedDatabaseManager), so we should do this next.

Comment on attachment 693346 [details] [diff] [review] patch v0.2 Review of attachment 693346 [details] [diff] [review]: ----------------------------------------------------------------- Lots of nits (and most not your fault) but this looks great! ::: dom/indexedDB/IDBTransaction.cpp @@ +894,5 @@ > mAbortCode = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR; > } > > if (mConnection) { > + QuotaManager::SetCurrentWindow(database->GetOwner()); Can you add an assertion that 'database->GetOwner()' is non-null before calling? That should have been there already... ::: dom/indexedDB/CheckQuotaHelper.cpp @@ +21,1 @@ > #include "mozilla/Services.h" Nit: Can you move the mozilla/ headers before the ns* headers? @@ +23,1 @@ > #define PERMISSION_INDEXEDDB_UNLIMITED "indexedDB-unlimited" We need a followup (not urgent) to change these permissions and topics to not mention indexedDB. ::: dom/quota/QuotaManager.cpp @@ +147,5 @@ > quotaManager->mOriginInfos.Remove(origin); > > + // Some other thread could increase the size without blocking (increasing > + // the origin usage without hitting the limit), but no more than this one. > + NS_ASSERTION(mSize < end, "This shouldn't happen!"); Shouldn't this be <= ? @@ +219,5 @@ > +nsresult > +QuotaManager::Init() > +{ > + mOriginInfos.Init(); > + mCheckQuotaHelpers.Init(); Nit: Since these are infallible you should move them below the fallible stuff. @@ +333,5 @@ > return GetQuotaObject(aOrigin, file); > } > + > +void > +QuotaManager::SetCurrentWindowInternal(nsPIDOMWindow* aWindow) Can you add an assertion here that mCurrentWindowIndex != BAD_TLS_INDEX? @@ +344,5 @@ > + else { > + // We cannot assert PR_GetThreadPrivate(mCurrentWindowIndex) here > + // because we cannot distinguish between the thread private became > + // null and that it was set to null on the first place, > + // because we didn't have a window. Nit: Since we're here we should fix this comment: "We cannot assert PR_GetThreadPrivate(mCurrentWindowIndex) here because there are some cases where we did not already have a window." @@ +360,5 @@ > + MutexAutoLock autoLock(mQuotaMutex); > + > + mCheckQuotaHelpers.Get(aWindow, getter_AddRefs(helper)); > + > + if (helper) { Nit: Combine this with the Get call above: if (mCheckQuotaHelpers.Get(aWindow, getter_AddRefs(helper))) { helper->Cancel(); } @@ +368,5 @@ > + > +bool > +QuotaManager::LockedQuotaIsLifted() > +{ > + nsPIDOMWindow* window = nullptr; Nit: Move this down to where you assign, and only assign once. @@ +370,5 @@ > +QuotaManager::LockedQuotaIsLifted() > +{ > + nsPIDOMWindow* window = nullptr; > + nsRefPtr<CheckQuotaHelper> helper; > + bool createdHelper = false; Nit: Move these down to where they're first used. @@ +373,5 @@ > + nsRefPtr<CheckQuotaHelper> helper; > + bool createdHelper = false; > + > + window = > + static_cast<nsPIDOMWindow*>(PR_GetThreadPrivate(mCurrentWindowIndex)); Nit: Can you assert that mCurrentWindowIndex != BAD_TLS_INDEX at the top of the function? @@ +376,5 @@ > + window = > + static_cast<nsPIDOMWindow*>(PR_GetThreadPrivate(mCurrentWindowIndex)); > + > + // IDB is supported outside of Windows, but we don't init quota for it, so we > + // should not get here without a Window. Nit: Let's fix this comment: "Quota is not enforced in chrome contexts (e.g. for components and JSMs) so we must have a window here." @@ +379,5 @@ > + // IDB is supported outside of Windows, but we don't init quota for it, so we > + // should not get here without a Window. > + NS_ASSERTION(window, "Why don't we have a Window here?"); > + > + mQuotaMutex.AssertCurrentThreadOwns(); Nit: Move this to the top of the function. @@ +383,5 @@ > + mQuotaMutex.AssertCurrentThreadOwns(); > + > + mCheckQuotaHelpers.Get(window, getter_AddRefs(helper)); > + > + if (!helper) { Nit: Combine this with the Get call above: if (!mCheckQuotaHelpers.Get(window, getter_AddRefs(helper))) { // ... } ::: dom/quota/QuotaManager.h @@ +12,5 @@ > #include "mozilla/Mutex.h" > #include "nsDataHashtable.h" > #include "nsRefPtrHashtable.h" > #include "nsThreadUtils.h" > +#include "xpcpublic.h" This is a pretty hefty header just for a -1 #define, let's just un-inline the QuotaManager constructor/destructor and keep this in the cpp? @@ +128,5 @@ > const nsAString& aPath); > > + // Set the Window that the current thread is doing operations for. > + // The caller is responsible for ensuring that aWindow is held alive. > + static inline void Nit: No need for the 'inline' here or below... Even though it was already there ;) @@ +134,5 @@ > + { > + QuotaManager* quotaManager = Get(); > + NS_ASSERTION(quotaManager, "Must have a manager here!"); > + > + return quotaManager->SetCurrentWindowInternal(aWindow); Nit: No need for 'return'... Even though it was already there ;) @@ +138,5 @@ > + return quotaManager->SetCurrentWindowInternal(aWindow); > + } > + > + static inline void > + CancelPromptsForWindow(nsPIDOMWindow* aWindow) While you're here you might as well assert aWindow. @@ +160,5 @@ > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > } > > + nsresult > + Init(); Nit: There are no real nsresults here, let's just make this return bool for now. @@ +182,5 @@ > + nsRefPtrHashtable<nsPtrHashKey<nsPIDOMWindow>, > + CheckQuotaHelper> mCheckQuotaHelpers; > + > + // TLS storage index for the current thread's window. > + unsigned mCurrentWindowIndex; Nit: unsigned int. Dunno why that isn't there now...

(In reply to ben turner [:bent] from comment #3) > Comment on attachment 693346 [details] [diff] [review] > patch v0.2 > > Review of attachment 693346 [details] [diff] [review]: > ----------------------------------------------------------------- > > Lots of nits (and most not your fault) but this looks great! > > ::: dom/indexedDB/IDBTransaction.cpp > @@ +894,5 @@ > > mAbortCode = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR; > > } > > > > if (mConnection) { > > + QuotaManager::SetCurrentWindow(database->GetOwner()); > > Can you add an assertion that 'database->GetOwner()' is non-null before > calling? That should have been there already... no, we can't assert it, since database->GetOwner() returns null for databases with the chrome privilege and that's not an error > > ::: dom/indexedDB/CheckQuotaHelper.cpp > @@ +21,1 @@ > > #include "mozilla/Services.h" > > Nit: Can you move the mozilla/ headers before the ns* headers? sure > > @@ +23,1 @@ > > #define PERMISSION_INDEXEDDB_UNLIMITED "indexedDB-unlimited" > > We need a followup (not urgent) to change these permissions and topics to > not mention indexedDB. will do > > ::: dom/quota/QuotaManager.cpp > @@ +147,5 @@ > > quotaManager->mOriginInfos.Remove(origin); > > > > + // Some other thread could increase the size without blocking (increasing > > + // the origin usage without hitting the limit), but no more than this one. > > + NS_ASSERTION(mSize < end, "This shouldn't happen!"); > > Shouldn't this be <= ? probably not, we discussed about it on IRC leaving the condition as it is for now > > @@ +219,5 @@ > > +nsresult > > +QuotaManager::Init() > > +{ > > + mOriginInfos.Init(); > > + mCheckQuotaHelpers.Init(); > > Nit: Since these are infallible you should move them below the fallible > stuff. ok > > @@ +333,5 @@ > > return GetQuotaObject(aOrigin, file); > > } > > + > > +void > > +QuotaManager::SetCurrentWindowInternal(nsPIDOMWindow* aWindow) > > Can you add an assertion here that mCurrentWindowIndex != BAD_TLS_INDEX? sure, done > > @@ +344,5 @@ > > + else { > > + // We cannot assert PR_GetThreadPrivate(mCurrentWindowIndex) here > > + // because we cannot distinguish between the thread private became > > + // null and that it was set to null on the first place, > > + // because we didn't have a window. > > Nit: Since we're here we should fix this comment: > > "We cannot assert PR_GetThreadPrivate(mCurrentWindowIndex) here because > there are some cases where we did not already have a window." ok, done > > @@ +360,5 @@ > > + MutexAutoLock autoLock(mQuotaMutex); > > + > > + mCheckQuotaHelpers.Get(aWindow, getter_AddRefs(helper)); > > + > > + if (helper) { > > Nit: Combine this with the Get call above: > > if (mCheckQuotaHelpers.Get(aWindow, getter_AddRefs(helper))) { > helper->Cancel(); > } done > > @@ +368,5 @@ > > + > > +bool > > +QuotaManager::LockedQuotaIsLifted() > > +{ > > + nsPIDOMWindow* window = nullptr; > > Nit: Move this down to where you assign, and only assign once. done > > @@ +370,5 @@ > > +QuotaManager::LockedQuotaIsLifted() > > +{ > > + nsPIDOMWindow* window = nullptr; > > + nsRefPtr<CheckQuotaHelper> helper; > > + bool createdHelper = false; > > Nit: Move these down to where they're first used. done > > @@ +373,5 @@ > > + nsRefPtr<CheckQuotaHelper> helper; > > + bool createdHelper = false; > > + > > + window = > > + static_cast<nsPIDOMWindow*>(PR_GetThreadPrivate(mCurrentWindowIndex)); > > Nit: Can you assert that mCurrentWindowIndex != BAD_TLS_INDEX at the top of > the function? sure, done > > @@ +376,5 @@ > > + window = > > + static_cast<nsPIDOMWindow*>(PR_GetThreadPrivate(mCurrentWindowIndex)); > > + > > + // IDB is supported outside of Windows, but we don't init quota for it, so we > > + // should not get here without a Window. > > Nit: Let's fix this comment: > > "Quota is not enforced in chrome contexts (e.g. for components and JSMs) > so we must have a window here." done > > @@ +379,5 @@ > > + // IDB is supported outside of Windows, but we don't init quota for it, so we > > + // should not get here without a Window. > > + NS_ASSERTION(window, "Why don't we have a Window here?"); > > + > > + mQuotaMutex.AssertCurrentThreadOwns(); > > Nit: Move this to the top of the function. done > > @@ +383,5 @@ > > + mQuotaMutex.AssertCurrentThreadOwns(); > > + > > + mCheckQuotaHelpers.Get(window, getter_AddRefs(helper)); > > + > > + if (!helper) { > > Nit: Combine this with the Get call above: > > if (!mCheckQuotaHelpers.Get(window, getter_AddRefs(helper))) { > // ... > } done > > ::: dom/quota/QuotaManager.h > @@ +12,5 @@ > > #include "mozilla/Mutex.h" > > #include "nsDataHashtable.h" > > #include "nsRefPtrHashtable.h" > > #include "nsThreadUtils.h" > > +#include "xpcpublic.h" > > This is a pretty hefty header just for a -1 #define, let's just un-inline > the QuotaManager constructor/destructor and keep this in the cpp? done > > @@ +128,5 @@ > > const nsAString& aPath); > > > > + // Set the Window that the current thread is doing operations for. > > + // The caller is responsible for ensuring that aWindow is held alive. > > + static inline void > > Nit: No need for the 'inline' here or below... Even though it was already > there ;) ok, removed the inline > > @@ +134,5 @@ > > + { > > + QuotaManager* quotaManager = Get(); > > + NS_ASSERTION(quotaManager, "Must have a manager here!"); > > + > > + return quotaManager->SetCurrentWindowInternal(aWindow); > > Nit: No need for 'return'... Even though it was already there ;) ok, fixed > > @@ +138,5 @@ > > + return quotaManager->SetCurrentWindowInternal(aWindow); > > + } > > + > > + static inline void > > + CancelPromptsForWindow(nsPIDOMWindow* aWindow) > > While you're here you might as well assert aWindow. ok, added > > @@ +160,5 @@ > > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > } > > > > + nsresult > > + Init(); > > Nit: There are no real nsresults here, let's just make this return bool for > now. done > > @@ +182,5 @@ > > + nsRefPtrHashtable<nsPtrHashKey<nsPIDOMWindow>, > > + CheckQuotaHelper> mCheckQuotaHelpers; > > + > > + // TLS storage index for the current thread's window. > > + unsigned mCurrentWindowIndex; > > Nit: unsigned int. Dunno why that isn't there now... fixed