'QuotaExceedError' is not thrown for a persisted website

VERIFIED FIXED

Status

()

VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: icrisan, Assigned: tt)

Tracking

(Blocks: 1 bug)

55 Branch
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [storage-v1] )

Attachments

(6 attachments, 11 obsolete attachments)

2.72 KB, application/zip
Details
1.11 KB, patch
Details | Diff | Splinter Review
2.58 KB, patch
Details | Diff | Splinter Review
1.17 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
3.63 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Created attachment 8876669 [details]
storageTest.zip

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.
(Assignee)

Comment 1

2 years ago
(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.
(Assignee)

Comment 2

2 years ago
I'd like to put it under dom::QM instead of dom::indexedDB, since the problem happens when the website is persisted.
Component: DOM: IndexedDB → DOM: Quota Manager
(Assignee)

Comment 3

2 years ago
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
(Assignee)

Comment 4

2 years ago
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?
(Assignee)

Updated

2 years ago
Blocks: 1254428
Whiteboard: [storage-v1]
(Assignee)

Comment 5

2 years ago
(In reply to Tom Tung [:tt] from comment #4)
ni :janv for comment #4
Flags: needinfo?(jvarga)

Comment 6

2 years ago
Yeah, I'll take a look.
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
Created attachment 8877098 [details] [diff] [review]
solution-b.patch
(Assignee)

Comment 9

2 years ago
Created attachment 8877099 [details] [diff] [review]
patch for ensuring originInfo is created
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.
(Assignee)

Comment 11

2 years ago
(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.
Flags: needinfo?(jvarga)
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.
(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.
(Assignee)

Comment 15

2 years ago
(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.
(Assignee)

Comment 17

2 years ago
(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...
(Assignee)

Comment 19

2 years ago
Created attachment 8877534 [details] [diff] [review]
Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist().

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.
Attachment #8877098 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8877535 [details] [diff] [review]
Bug 1372116 - P2: Init quota for origin directlly when the metadata is just created to save time for traversing directory and algin the format.

Patch to align the format in EnsureOriginIsInitialized()
(Assignee)

Comment 21

2 years ago
Created attachment 8877536 [details] [diff] [review]
Bug 1372116 - P3: Test to verify persist() do create originInfo after tempory storage is initialized.

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.
(Assignee)

Comment 23

2 years ago
(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.
(Assignee)

Comment 24

2 years ago
Created attachment 8877839 [details] [diff] [review]
Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist().

Update the patch to change the "else" and the indent back to make the patch easier to read.
Attachment #8877534 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
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!
Attachment #8877839 - Flags: review?(jvarga)
(Assignee)

Comment 26

2 years ago
Created attachment 8877845 [details] [diff] [review]
Bug 1372116 - P2: Init quota for origin directlly when the metadata is just created to save time for traversing directory.

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.
Attachment #8877535 - Attachment is obsolete: true
Attachment #8877845 - Flags: review?(jvarga)
(Assignee)

Comment 27

2 years ago
Created attachment 8877847 [details] [diff] [review]
Bug 1372116 - P3: Test to verify persist() do create originInfo when the temporary storage has been initialized.

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!
Attachment #8877536 - Attachment is obsolete: true
Attachment #8877847 - Flags: review?(jvarga)
(Assignee)

Comment 28

2 years ago
Created attachment 8877859 [details] [diff] [review]
Add a debug check to ensure we have created originInfo after the temporary storage is initialized.

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
Attachment #8877859 - Flags: feedback?(jvarga)
(Assignee)

Comment 29

2 years ago
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.
(Assignee)

Comment 30

2 years ago
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.
Attachment #8877859 - Flags: feedback?(jvarga)
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 ?
Attachment #8877839 - Flags: review?(jvarga)

Updated

2 years ago
Attachment #8877845 - Flags: review?(jvarga) → review+
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.
Attachment #8877847 - Flags: review?(jvarga)
(Assignee)

Comment 33

2 years ago
(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()) {
 ...
}
(Assignee)

Comment 35

2 years ago
(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.
(Assignee)

Comment 36

2 years ago
Created attachment 8882185 [details] [diff] [review]
Bug 1372116 - P1 - v2: Create originInfo after creating metadata if the temporay storage has been initialized for persist().

Address comment
Attachment #8877839 - Attachment is obsolete: true
(Assignee)

Comment 37

2 years ago
Created attachment 8882189 [details] [diff] [review]
Bug 1372116 - P1 - v2.1: Create originInfo after creating metadata if the temporay storage has been initialized for persist().

Remove the assertion because it has a chance that the temporary storage is initialized but originInfo hasn't been created.
Attachment #8882185 - Attachment is obsolete: true
(Assignee)

Comment 38

2 years ago
Created attachment 8882191 [details] [diff] [review]
Bug 1372116 - P1 - v2.1: Create originInfo after creating metadata if the temporay storage has been initialized for persist().
Attachment #8882189 - Attachment is obsolete: true
(Assignee)

Comment 39

2 years ago
Created attachment 8882194 [details] [diff] [review]
[Final] Bug 1372116 - P2: Init quota for origin directlly when the metadata is just created to save time for traversing directory. r=janv
Attachment #8877845 - Attachment is obsolete: true
Attachment #8882194 - Flags: review+
(Assignee)

Comment 40

2 years ago
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!
Attachment #8882191 - Flags: review?(jvarga)
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 ..."
Attachment #8882191 - Flags: review?(jvarga) → review+
(Assignee)

Comment 42

2 years ago
Created attachment 8882203 [details] [diff] [review]
[Final] Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). r=janv

Thanks for the review! I addressed the comment!
Attachment #8882191 - Attachment is obsolete: true
Attachment #8882203 - Flags: review+
(Assignee)

Updated

2 years ago
Blocks: 1361330
(Assignee)

Comment 44

2 years ago
Created attachment 8883211 [details] [diff] [review]
Bug 1372116 - P3 - v2: Test to verify persist() do create originInfo when the temporary storage has been initialized.

Rewrite test based on patches in Bug 1361330
Attachment #8877847 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
No longer blocks: 1361330
Depends on: 1361330
(Assignee)

Updated

a year ago
Blocks: 1376444
(Assignee)

Comment 45

a year ago
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.
Flags: needinfo?(jvarga)
Yes, go ahead and land it.
Flags: needinfo?(jvarga)
(Assignee)

Updated

a year ago
Attachment #8882194 - Attachment description: Bug 1372116 - P2: Init quota for origin directlly when the metadata is just created to save time for traversing directory. r=janv → [Final] Bug 1372116 - P2: Init quota for origin directlly when the metadata is just created to save time for traversing directory. r=janv
(Assignee)

Updated

a year ago
Attachment #8882203 - Attachment description: Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). r=janv → [Final] Bug 1372116 - P1: Create originInfo after creating metadata if the temporay storage has been initialized for persist(). r=janv
(Assignee)

Comment 47

a year ago
Hi Sheriff,

Please only push P1 Attachment #8882194 [details] [diff] and P2 Attachment #8882203 [details] [diff]. Thanks!
Status: NEW → ASSIGNED
Keywords: checkin-needed, leave-open
(Assignee)

Comment 48

a year ago
> 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]

Comment 49

a year ago
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
Keywords: checkin-needed
(Assignee)

Comment 50

a year ago
(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 :)
Flags: needinfo?(icrisan)
(Reporter)

Comment 51

a year ago
Hi Tom,

Sure :), I will test again on tomorrow's Nightly.
Flags: needinfo?(icrisan)
(Reporter)

Comment 53

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

Comment 54

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

Thanks!
(Assignee)

Updated

a year ago
Blocks: 1389378
(Assignee)

Comment 55

a year ago
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
Attachment #8883211 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
No longer depends on: 1361330
(Assignee)

Comment 56

a year ago
Close this bug, since I moved the only not landed patch to bug 1389378.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.