Steps to reproduce: 1. Open a tab using an url from https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt (e.g: https://example.com:443) 2. Set the 'dom.quotaManager.temporaryStorage.fixedLimit' pref to 50000 => the global limit is 50MB 3. Persist the tab from step 1 4. Reach and exceed the global limit by storing data into indexeddb Expected results: I expect to receive 'QuotaExceededError' or something similar. Actual results: No error is thrown even though data is successfully written in indexeddb.

(In reply to Ioana Crisan from comment #0) > Actual results: > No error is thrown even though data is successfully written in indexeddb. After digging more, I found it actually writes into the disk by TelemetryVFS::xWrite(), but not checking quota through QuotaOject::MaybeUpdateSize(). The reason is that quotaObject didn't exsit somehow, so the issue happened. I'll look more into it.

I'd like to put it under dom::QM instead of dom::indexedDB, since the problem happens when the website is persisted.

I guessed I find out the reason. In this test, indexedDB will somehow initialize quota for origin with ERSISTENCE_TYPE_TEMPORARY first (1). Then, persist that origin (2). Final, store things into the origin with PERSISTENCE_TYPE_DEFAULT (3) and increasing its storage without updating (4). (1) mTemporaryStorageInitialized is set to be true, because of first initializing. (2) Then, because the directory for PERSISTENCE_TYPE_DEFAULT is not created yet, we creat the directory for the persisting origin. Besides, we won't created the originInfo, groupInfo for it since they have not existed yet. (3) However, when if quota clients want to use the origin later, we won't initialize the persisted origin since the directory is created and the persistenceType is not PERSISTENCE_TYPE_PERSISTENT [1]. (4) Thus, the quota client won't update its size later because it cannot find the quota object (because the originInfo and groupInfo are not created). Right now, I think the solution is to create the originInfo and groupInfo if they aren't created yet when persisting an origin. I'll consider more tomorrow. [1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5138-5186

Hi Jan, After looking into it more. I list two issues that I found below. Could you help me to confrim them? Thanks! 1. I implemented a method to get group usage (Bug 1281103) and it gets the group usage from runtime in-memory data. Before getting usage, it calls EnsureOriginIsInitialized()[1] to initialize origin. However, it creates the metadata files and originInfo object for PERSISTENCE_TYPE_TEMPORARY. And, I don't think it's necessary now. Actually, it only needs to call InitializeRepository() like [2]. [1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#6969-6972 [2] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5095-5135 2. The other issue is that persist() doesn't create originInfo if the originInfo doesn't exist. It only changes originInfo's persisted attribute when the originInfo is created and modify the metadata & directroy. In the meantime, when we don't call initializeOrigin() for the origin with its directory is created [3] [4]. So, the problem happens when an origin initializes the QM (mTemporaryStorageInitialized is set to true) and we persist the new origin (only create the metadata but not originInfo). Then, if the quota clients access that origin later, it won't created the originInfo/groupInfo for the origin, which means we won't track quota for that origin. There are two solutions for fixing this issue. A: Create in-memory object (originInfo) when persist() B: For the persistenceType is not PERSISTENCE_TYPE_PERSISTENT, call the initializeOrigin() if we cannot get originInfo for that origin rather than checking directory. Thus, it ensure the origin have originInfo. Both A and B can fix this issue, but I think the solution B is better because B created originInfo only when the quota client needs to access that origin. [3] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5138 [4] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5172-5186 I'll fix issue 2 in this bug and file a bug for issue 1. Does that sound good to you?

(In reply to Tom Tung [:tt] from comment #4) ni :janv for comment #4

Yeah, I'll take a look.

(In reply to Tom Tung [:tt] from comment #4) > There are two solutions for fixing this issue. > A: Create in-memory object (originInfo) when persist() > B: For the persistenceType is not PERSISTENCE_TYPE_PERSISTENT, call the > initializeOrigin() if we cannot get originInfo for that origin rather than > checking directory. Thus, it ensure the origin have originInfo. > > Both A and B can fix this issue, but I think the solution B is better > because B created originInfo only when the quota client needs to access that > origin. I found solution B needs to acquire the mutex lock (get originInfo) for almost all the quota client's opertaions. It's better to implement solution A because it saves time for that. But maybe we could have an assertion or |ifdef DEBUG| at the end of EnsureOriginIsInitialized() to prevent from forgetting to create originInfo for the origin.

Comment on attachment 8877098 [details] [diff] [review] solution-b.patch Review of attachment 8877098 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +3930,5 @@ > + > + // if originInfo has not been created yet, create it. > + MOZ_ASSERT(aDirectory); > + > + nsresult rv = InitializeOrigin(PERSISTENCE_TYPE_DEFAULT, So this traverses the directory, but we know that the directory is empty ... Also, InitializeOrigin() will call InitQuotaForOrigin() which locks again and it also does all the hash table lookups again. @@ +7682,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > + int64_t timestamp = PR_Now(); CreateDirectoryMetadataFiles() also calls PR_Now(), right ? So the metadata file and OriginInfo will end up with different values.

(In reply to Jan Varga [:janv] from comment #10) I might be wrong, so please correct me if I was wrong. Thanks! > ::: dom/quota/ActorsParent.cpp > @@ +3930,5 @@ > > + > > + // if originInfo has not been created yet, create it. > > + MOZ_ASSERT(aDirectory); > > + > > + nsresult rv = InitializeOrigin(PERSISTENCE_TYPE_DEFAULT, > > So this traverses the directory, but we know that the directory is empty ... I was thinking that the directory can be not empty. For example: 1. A website used to store things through indexedDB/DOM Cache. 2. Open Firefox and navigate to that website. Then, Quit the Firefox. 3. The website updates its script and calling persist at the first line of its script. 4. Open Firefox and navigate to that website again. In here, the directory is not empty when calling persist(). Thus, if I try to implement this solution(creating originInfo when persist()), I think I need to call this function to calculate overall usage for the origin of that website. > Also, InitializeOrigin() will call InitQuotaForOrigin() which locks again > and it also does all the hash table lookups again. I thought the mutex lock is released here, so it's fine to call InitQuotaForOrigin(). I limited the mutex lock by the "{}". > @@ +7682,5 @@ > > if (NS_WARN_IF(NS_FAILED(rv))) { > > return rv; > > } > > > > + int64_t timestamp = PR_Now(); > > CreateDirectoryMetadataFiles() also calls PR_Now(), right ? > So the metadata file and OriginInfo will end up with different values. That's my fault! I should look deeper. Thanks!

(In reply to Tom Tung [:tt] from comment #11) > > ::: dom/quota/ActorsParent.cpp > > @@ +3930,5 @@ > > > + > > > + // if originInfo has not been created yet, create it. > > > + MOZ_ASSERT(aDirectory); > > > + > > > + nsresult rv = InitializeOrigin(PERSISTENCE_TYPE_DEFAULT, > > > > So this traverses the directory, but we know that the directory is empty ... > > I was thinking that the directory can be not empty. > For example: > > 1. A website used to store things through indexedDB/DOM Cache. > 2. Open Firefox and navigate to that website. Then, Quit the Firefox. > 3. The website updates its script and calling persist at the first line of > its script. > 4. Open Firefox and navigate to that website again. > > In here, the directory is not empty when calling persist(). > > Thus, if I try to implement this solution(creating originInfo when > persist()), I think I need to call this function to calculate overall usage > for the origin of that website. This bug is about the case when temporary storage was already initialized, right ? Anyway, after 4. if temporary storage was not initialized and persist() is called and then a quota client calls EnsureOriginIsInitialized() OriginInfo will be created, so we don't have to fix anything here. If temporary storage was initialized, then OriginInfo must exist, so no problem again. So far, this was all about when the origin directory already exists, if I understood your case correctly. Not let's talk about other case. I think the problem is if the directory doesn't exist yet, right ? Now if temporary storage was not initialized, no problem again, we create the directory in Persist() and it will be scanned and initialized in EnsureOriginIsInitialized by calling InitializeRepository(). If temporary storage was already initialized, we finally get to the real problem. We need to create OriginInfo, because next time EnsureOriginIsInitialized() is called, it thinks the directory is already initialized and no OriginInfo is created. This is the only thing that needs to be fixed in my opinion. > > Also, InitializeOrigin() will call InitQuotaForOrigin() which locks again > > and it also does all the hash table lookups again. > > I thought the mutex lock is released here, so it's fine to call > InitQuotaForOrigin(). I limited the mutex lock by the "{}". Yes, it's released, but we must lock twice which is more expensive. We should use only one lock if possible.

I didn't test this at all, but I believe this is what needs to be done. It would be great if the attached test was rewritten to a standard mochitest or xpcshell-test.

(In reply to Tom Tung [:tt] from comment #4) > 1. I implemented a method to get group usage (Bug 1281103) and it gets the > group usage from runtime in-memory data. Before getting usage, it calls > EnsureOriginIsInitialized()[1] to initialize origin. However, it creates the > metadata files and originInfo object for PERSISTENCE_TYPE_TEMPORARY. And, I > don't think it's necessary now. Actually, it only needs to call > InitializeRepository() like [2]. > > [1] > http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#6969- > 6972 > [2] > http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5095- > 5135 Well, yes, we should fix that in future even though it's harmless.

(In reply to Jan Varga [:janv] from comment #12) > This bug is about the case when temporary storage was already initialized, > right ? > > Anyway, after 4. if temporary storage was not initialized and persist() is > called and then a quota client > calls EnsureOriginIsInitialized() OriginInfo will be created, so we don't > have to fix anything here. > > If temporary storage was initialized, then OriginInfo must exist, so no > problem again. > So far, this was all about when the origin directory already exists, if I > understood your case correctly. > > Not let's talk about other case. I think the problem is if the directory > doesn't exist yet, right ? > Now if temporary storage was not initialized, no problem again, we create > the directory in Persist() and > it will be scanned and initialized in EnsureOriginIsInitialized by calling > InitializeRepository(). > > If temporary storage was already initialized, we finally get to the real > problem. > We need to create OriginInfo, because next time EnsureOriginIsInitialized() > is called, > it thinks the directory is already initialized and no OriginInfo is created. > This is > the only thing that needs to be fixed in my opinion. You're right. I was afraid that not always having the same place to initialize origin reduces readability, so I tried to always initialize all persisting origin when persist(). However, it doesn't worthy to initialize all persisting origin when persist(), because it costs a lot. Besides, I can add comments to avoid that. > Yes, it's released, but we must lock twice which is more expensive. > We should use only one lock if possible. Your patch looks great, it can avoid problem of locking twice! I'd like to add an assertion to ensure originInfo has been created when mTemporaryStorageIsInitialized is set to be true. BTW, I found there is a similar issue which calling InitializeOrigin()[1] while the direcrtory is just created. I'd like to fix this to align the coding style and increase the proformance as well in another patch. [1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5183-5184 (In reply to Jan Varga [:janv] from comment #13) > Created attachment 8877254 [details] [diff] [review] > try this > > I didn't test this at all, but I believe this is what needs to be done. > It would be great if the attached test was rewritten to a standard mochitest > or xpcshell-test. Sure, I'll update a test to verify it.

(In reply to Tom Tung [:tt] from comment #15) > reduces readability, so I tried to always initialize all persisting origin > when persist(). However, it doesn't worthy to initialize all persisting > origin when persist(), because it costs a lot. Besides, I can add comments > to avoid that. "... to initialize all persisting origin when persist()" ? You mean to initialize all client sub directories when we just create a new origin directory ? > BTW, I found there is a similar issue which calling InitializeOrigin()[1] > while the direcrtory is just created. I'd like to fix this to align the > coding style and increase the proformance as well in another patch. > Yeah, I know. So if we change that to InitQuotaForOrigin(), then there won't be a problem with readability I guess.

(In reply to Jan Varga [:janv] from comment #16) > "... to initialize all persisting origin when persist()" ? > You mean to initialize all client sub directories when we just create a new > origin directory ? No, I meant to initialize the origin (create originInfo) when storage.persist() an origin. Just like I did in attachment 8877098 [details] [diff] [review], but it's not a good idea. It may slow down the persist() since it acquired two mutex lock in some cases. Sorry for misleading...

This patch basically addresses attachment 8877254 [details] [diff] [review]. It adds an assertion to avoid that the oirginInfo isn't existed after the temporary storage is initialized. Besides, it returns earlier to reduce the indent.

Patch to align the format in EnsureOriginIsInitialized()

Patch for test

Comment on attachment 8877534 [details] [diff] [review] Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). Review of attachment 8877534 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ -7671,5 @@ > - /* aTimestamp */ nullptr); > - if (NS_WARN_IF(NS_FAILED(rv))) { > - return rv; > - } > - } else { Why did you need to replace the "else" with an early "return". This change shifted indentation and now it's harder to understand what the patch is actually fixing.

(In reply to Jan Varga [:janv] from comment #22) > Why did you need to replace the "else" with an early "return". > This change shifted indentation and now it's harder to understand what the > patch is actually fixing. I thought it's easier to read the overall logic. I'll change it back and send them to review tommorrow if try goes green. Sorry about that.

Update the patch to change the "else" and the indent back to make the patch easier to read.

Comment on attachment 8877839 [details] [diff] [review] Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). Hi Jan, Could you help me to review this patch? Basically, it's the same with the patch which you provided. I add an assertion to catch the similar problems. Thanks!

Update commit message and comment. Hi Jan, Could you help me to review this patch? Thanks! This patch changes to call InitQuotaForOrigin() rather than InitializeOrigin() when directory metadata is just created to save the time for traversing the directory and align the format with P1.

Hi Jan, This patch is for verifying the previous patch works. It references the reporter's test to reproduce the problem. Could you help me to review this patch? Thanks!

Hi Jan, After having this bug, I'm thinking to have a debug check like this. I reckon the non-persistent storage should have their originInfo after initializing repository. So, I add this check and try to run all the existing tests. However, when I'm running the test on indexedDB, the "test_defaultStorageUpgrqde.js" fails. The reason is that there are two different URLs (e.g. file://c++/, file://c///) and they will have the same directory (file+++c+++) but two different origins. It makes QM only create originInfo for the first initialized origin but not for the second initialized origin because QM creates originInfo based on checking if the directory is initialized or not [1]. So, if we have this case in the real world, the second origin won't bound by global and group limits. Do I misunderstand anything or this is an issue we need to fix? [1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5137-5141

So, in today's meeting, Jan suggested that I should move the test(P3) to dom/indexedDB/test, because it uses indexedDB. I could move it back or create another test in dom/quota/test after he finishes a new internal quota client. For adding assertion on debug build, though it's an issue, we are better to not add this into code base for now. We should find a way out to deal with it in the future.

Comment on attachment 8877859 [details] [diff] [review] Add a debug check to ensure we have created originInfo after the temporary storage is initialized. Remove the feedback request per comment 29. Note: I'm thinking to change the debug check to an if-condition. Like: if NO_OriginInfo then Create_OriginInfo. However, it's too expensive. Maybe we'll find an appropriate way in the future.

Comment on attachment 8877839 [details] [diff] [review] Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). Review of attachment 8877839 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +7677,5 @@ > } > + > + // Directory metadata has been successfully created. > + // Create OriginInfo too if temporary storage was already initialized to > + // avoid QM having initialized repository. "Create OriginInfo too if temporary storage was already initialized to avoid QM having initialized repository." This sentence doesn't makes sense to me. @@ +7681,5 @@ > + // avoid QM having initialized repository. > + // After initializing repository, each initialied direcotrires with > + // non-persistent types should have an originInfo to track their usage. > + // If QM have not initialized repository yet, let QM created originInfo > + // later. The wording here is confusing. @@ +7737,5 @@ > } > + > + // Directory metadata has been successfully created/updated, try to update > + // OriginInfo too (it's ok if OriginInfo doesn't exist yet). > + aQuotaManager->PersistOrigin(mGroup, mOriginScope.GetOrigin()); So in my patch ("Try this") I did it this way: // Directory metadata has been successfully updated. // Update OriginInfo too if temporary storage was already initialized. if (aQuotaManager->IsTemporaryStorageInitialized()) { aQuotaManager->PersistOrigin(mGroup, mOriginScope.GetOrigin()); } Do you think it's wrong ?

Comment on attachment 8877847 [details] [diff] [review] Bug 1372116 - P3: Test to verify persist() do create originInfo when the temporary storage has been initialized. Review of attachment 8877847 [details] [diff] [review]: ----------------------------------------------------------------- This should use simpledb.

(In reply to Jan Varga [:janv] from comment #31) > // Directory metadata has been successfully updated. > // Update OriginInfo too if temporary storage was already initialized. > if (aQuotaManager->IsTemporaryStorageInitialized()) { > aQuotaManager->PersistOrigin(mGroup, mOriginScope.GetOrigin()); > } > > Do you think it's wrong ? No, but I thought it might be better if I could provide information. However, it confuses you, so I'll change the comment back. Sorry about that.

(In reply to Tom Tung [:tt] from comment #33) > (In reply to Jan Varga [:janv] from comment #31) > > // Directory metadata has been successfully updated. > > // Update OriginInfo too if temporary storage was already initialized. > > if (aQuotaManager->IsTemporaryStorageInitialized()) { > > aQuotaManager->PersistOrigin(mGroup, mOriginScope.GetOrigin()); > > } > > > > Do you think it's wrong ? > > No, but I thought it might be better if I could provide information. > However, it confuses you, so I'll change the comment back. Sorry about that. You are talking about the comment, but there's also the additional "if": if (aQuotaManager->IsTemporaryStorageInitialized()) { ... }

(In reply to Jan Varga [:janv] from comment #34) > You are talking about the comment, but there's also the additional "if": > if (aQuotaManager->IsTemporaryStorageInitialized()) { > ... > } Oh, I should add it back. It confused me because it check IsTemporaryStorageInitialized() and still check the originInfo in PersistOrigin(). However after storage meeting, I realized IsTemporaryStorageInitialized() doesn't guarantee we have originInfo(). Besides, it saves time for acquiring a mutex lock.

Address comment

Remove the assertion because it has a chance that the temporary storage is initialized but originInfo hasn't been created.

Comment on attachment 8882191 [details] [diff] [review] Bug 1372116 - P1 - v2.1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). Hi Jan, Could you help me to review this again? I addressed your comment and removed the assertion in this patch. Thanks!

Comment on attachment 8882191 [details] [diff] [review] Bug 1372116 - P1 - v2.1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). Review of attachment 8882191 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +7647,5 @@ > MOZ_ASSERT(mOriginScope.IsOrigin()); > > AUTO_PROFILER_LABEL("PersistOp::DoDirectoryWork", OTHER); > > + // Update directory metadata on disk first. Then, update the originInfo Nit: "... Then, update the originInfo ..." should be: "... Then, create/update the originInfo ..."

Thanks for the review! I addressed the comment!

Rewrite test based on patches in Bug 1361330

Hi Jan, This bug seems to solve at least two knowing issues (this bug & bug 1376444) reported by SV. Could I land patch P1 & P2 before P3(test) getting r+? Thanks! Note: I'll send P3(test) to review after bug 1361330 is landed.

Yes, go ahead and land it.

Hi Sheriff, Please only push P1 Attachment #8882194 [details] [diff] and P2 Attachment #8882203 [details] [diff]. Thanks!

> Please only push P1 Attachment #8882194 [details] [diff] [diff] and P2 Attachment > #8882203 [details] [diff]. Thanks! P1 Attachment #8882203 [details] [diff] and P2 Attachment #8882194 [details] [diff]

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfcb0aeb065 P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/76538de6b9b2 P2: Init quota for origin directlly when the metadata is just created to save time for traversing directory. r=janv

(In reply to Pulsebot from comment #49) Thanks! Hi Ioana, Would you mind test it again on tomorrow Nighty? Thanks! I couldn't reproduce it after applying my patch. It'd be nice if you can ensure this really fix the issue :)

Hi Tom, Sure :), I will test again on tomorrow's Nightly.

https://hg.mozilla.org/mozilla-central/rev/7bfcb0aeb065 https://hg.mozilla.org/mozilla-central/rev/76538de6b9b2

I could not reproduce the issue using the latest Nightly build(Build ID: 20170714100217).

(In reply to Ioana Crisan from comment #53) > I could not reproduce the issue using the latest Nightly build(Build ID: > 20170714100217). Thanks!

Comment on attachment 8883211 [details] [diff] [review] Bug 1372116 - P3 - v2: Test to verify persist() do create originInfo when the temporary storage has been initialized. Move this to bug 1389378

Close this bug, since I moved the only not landed patch to bug 1389378.

Reproduced the issue on Nightly 2017-06-12 using the testpage: https://bug1376444.bmoattachments.org/attachment.cgi?id=8881384 Verified fixed on 57.0a1 (2017-08-25) Win 7, Win 10, Ubuntu 14.04, OS X 10.12.6.

Removing leave-open keyword from resolved bugs, per :sylvestre.