1287701 - Expose persisted() method to StorageManager

Status: RESOLVED DUPLICATE of bug 1286717

(Core :: DOM: Core & HTML, defect, P2)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Expose persisted() method to StorageManager

Comment 1

[Tom Tung [:tt, :ttung]]

I would like to take this bug.

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: WIP-v1

Step 1: Add a flag into originInfo and adjust the mechanism for removing non-persistent storage.

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: Add/Store a persistent flag in QuotaManager

Hi Jan,

In this patch, I add a "isPersistent" flag in QuotaManager and store it into metadata. Could you give me some feedback to see whether the direction of this patch is correct or not? 

Besides, I'll leave the dynamic change "isPersistent" flag into persist() attribute(bug 1286717) so we don't need to care about owning locks issue in this bug. By the way, I'll start to implement webidl part once bug 1267941 is done thus I could avoid the patch conflict problem.

Comment 4

[Jan Varga [:janv]]

Comment on attachment 8780385 [details] [diff] [review]
Add/Store a persistent flag in QuotaManager

Review of attachment 8780385 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: dom/quota/ActorsParent.cpp
@@ +5006,5 @@
>              OriginInfo* originInfo = originInfos[i];
>  
> +            if (originInfo->Persisted()) {
> +              break;
> +            }

Hm, we might rather want to destroy OriginInfo once the origin is persisted.

@@ +7683,5 @@
>    MOZ_ASSERT(mOriginProps.Length() == 1);
>  
>    OriginProps& originProps = mOriginProps[0];
>  
> +  // We don't have any approach to restore isPersistent, so assume it is false.

Yes, this is true

::: dom/quota/QuotaManager.h
@@ +468,5 @@
>                     const nsACString& aOrigin,
>                     bool aIsApp,
>                     int64_t aAccessTime,
> +                   nsIFile* aDirectory,
> +                   bool isPersistnet);

aDirectory is an output argument here so it should be the last one

Comment 5

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #4)
Hi Jan,

Thanks for giving feedback so quickly.

> Comment on attachment 8780385 [details] [diff] [review]
> Add/Store a persistent flag in QuotaManager
> 
> Review of attachment 8780385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +5006,5 @@
> >              OriginInfo* originInfo = originInfos[i];
> >  
> > +            if (originInfo->Persisted()) {
> > +              break;
> > +            }
> 
> Hm, we might rather want to destroy OriginInfo once the origin is persisted.

Sure, but only when user grant to destroy. So I'll do the checking condition just before we are going to destroy it and file a follow-up bug for sending delete request(asking for granted) to user?  

> ::: dom/quota/QuotaManager.h
> @@ +468,5 @@
> >                     const nsACString& aOrigin,
> >                     bool aIsApp,
> >                     int64_t aAccessTime,
> > +                   nsIFile* aDirectory,
> > +                   bool isPersistnet);
> 
> aDirectory is an output argument here so it should be the last one

Sure, I'll address that in next patch.

Comment 6

[Jan Varga [:janv]]

(In reply to Tom Tung [:tt] from comment #5)
> > ::: dom/quota/ActorsParent.cpp
> > @@ +5006,5 @@
> > >              OriginInfo* originInfo = originInfos[i];
> > >  
> > > +            if (originInfo->Persisted()) {
> > > +              break;
> > > +            }
> > 
> > Hm, we might rather want to destroy OriginInfo once the origin is persisted.
> 
> Sure, but only when user grant to destroy. So I'll do the checking condition
> just before we are going to destroy it and file a follow-up bug for sending
> delete request(asking for granted) to user?  
> 

I'm talking about destroying OriginInfo memory object, not about clearing the origin on disk.
If you take a look at temporary storage initialization code, you'll see that we don't init quota (which creates OriginInfo) for origin directories with isApp == true

Comment 7

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #6)
> I'm talking about destroying OriginInfo memory object, not about clearing
> the origin on disk.
> If you take a look at temporary storage initialization code, you'll see that
> we don't init quota (which creates OriginInfo) for origin directories with
> isApp == true

Oh I see what you meant but I still don't get it. I'm worried about if we destroy all the persistent originInfo memory object, how do we know a incoming storage is persistent? I mean we can do so for isApp because we can get its value before calling EnsureOriginIsInitialized(), but we can get isPersistent's value only after calling InitializeRepository() if we don't have originInfo to record them. 
Should we read it from metadata each time when we call EnsureOriginIsInitialized()?

Comment 8

[Jan Varga [:janv]]

(In reply to Tom Tung [:tt] from comment #7)
> (In reply to Jan Varga [:janv] from comment #6)
> > I'm talking about destroying OriginInfo memory object, not about clearing
> > the origin on disk.
> > If you take a look at temporary storage initialization code, you'll see that
> > we don't init quota (which creates OriginInfo) for origin directories with
> > isApp == true
> 
> Oh I see what you meant but I still don't get it. I'm worried about if we
> destroy all the persistent originInfo memory object, how do we know a
> incoming storage is persistent? I mean we can do so for isApp because we can
> get its value before calling EnsureOriginIsInitialized(), but we can get
> isPersistent's value only after calling InitializeRepository() if we don't
> have originInfo to record them. 
> Should we read it from metadata each time when we call
> EnsureOriginIsInitialized()?

Yes, initial patch can just load metadata each time, but we can add a hash table or something later to cache the persistent flag info. We don't want to use OriginInfo for that since OriginInfo is used to determine which origins can be evicted and that can cause problems.
Also, when an origin is persisted, it shouldn't take up space in temporary storage, that can be easily done by destroying OriginInfo using RemoveQuotaForOrigin().

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: [WIP v2] Add/Store a IsPersisent flag into QuotaManger

Hi Jan,

In this patch, I 
    1. Remove the IsPersistent flag in QuotaInfo since QuotaInfo should be destroyed when persist() is called.
    2. Add a flag to IsTreatedAsPesistent() function and modify code to initial the flag before calling that function.

Next step: create a new hash table or use current table to record the persistent flag.

BTW, I would like to leave changing IsPersistent flag to true (destroying OriginInfo using RemoveQuotaForOrigin()) to bug 1286717 (persist attribute).

Comment 10

[Tom Tung [:tt, :ttung]]

Comment on attachment 8781488 [details] [diff] [review]
[WIP v2] Add/Store a IsPersisent flag into QuotaManger

Hi Jan,

Could you give me some feedback about this patch? Thanks!

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-wip.txt

inter-diff patch

Comment 12

[Jan Varga [:janv]]

Comment on attachment 8781488 [details] [diff] [review]
[WIP v2] Add/Store a IsPersisent flag into QuotaManger

Review of attachment 8781488 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/ActorsParent.cpp
@@ +4513,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  bool isPersistent = false;
> +  if (!created) {
> +    GetIsPersistent(directory, &isPersistent);

This should be done earlier, right after GetDirectoryForOrigin().

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: [WIP v3] Add/Store a IsPersisent flag into QuotaManger

Hi Jan,

I add somethings more in this patch. Could you give me some feedback? Thanks!
In this patch, 
    1. I check the flag earlier.
    2. I add a hash table to track the origin (is persistent or not) in PERSISTENCE_TYPE_DEFAULT 
    3. I implement the path from QuotaManagerService to QuotaManager by using current PBackground.

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-wip.txt

inter-diff patch for WIP patch

Comment 15

[Jan Varga [:janv]]

Comment on attachment 8782786 [details] [diff] [review]
[WIP v3] Add/Store a IsPersisent flag into QuotaManger

Review of attachment 8782786 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: dom/quota/ActorsParent.cpp
@@ +4539,5 @@
> +      isPersistent = false;
> +    }
> +
> +    mPersistentBoxes.Put(aOrigin, isPersistent);
> +  }

I propose to keep all the logic in IsTreatedAsPersistent(). I also propose to remove aIsApp from IsTreatedAsPersistent(). We will have to load metadata file anyway to get persisted flag, so we can also get isApp from the file.
The method should also use the hashtable to cache this information.

Comment 16

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #15)
> I propose to keep all the logic in IsTreatedAsPersistent(). I also propose
> to remove aIsApp from IsTreatedAsPersistent(). We will have to load metadata
> file anyway to get persisted flag, so we can also get isApp from the file.
> The method should also use the hashtable to cache this information.

I'm working on it but I have an question about implementing these logic in IsTreatedAsPersistent(). 
If we need to read the metadata in IsTreatedAsPersistent(), we may need to make IsTreatedAsPersistent() return nsresult. However, [1] and [2] return boolean and it means we may need to disregard nsresult which return from IsTrearedAsTemporary()? Currently, I cannot find out a good way to deal with that.

[1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#4786
[2] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#4799

Comment 17

[Jan Varga [:janv]]

I think only first call for IsTreatedAsPersistent() can fail, because we might not have the cached value yet. So you can split the method into two versions:

nsresult IsTreatedAsPersistent(bool* result);
- this will load metadata from the disk if there's no cached value and put it into the hash table
- then it can just call |bool IsTreatedAsPersistent()|

bool IsTreatedAsPersistent();
- this will assert that we have an entry in the hash table for it
- and finally, it will evaluate all the variables

Comment 18

[Jan Varga [:janv]]

And don't worry about IsFirstPromptRequired() and IsQuotaEnforced(), we'll solve it later.

Comment 19

[Tom Tung [:tt, :ttung]]

Attachment: [WIP v4] Add/Store a IsPersisent flag into QuotaManger.

Rewrite IsTreatedAsPersistent() by checking hash table and provide another function (QuotaManager::IsPersistent()) for checking IsPersistent flag by checking metadata.

Comment 20

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-wip.txt

interdiff patch

Comment 21

[Jan Varga [:janv]]

Comment on attachment 8785127 [details] [diff] [review]
[WIP v4] Add/Store a IsPersisent flag into QuotaManger.

Review of attachment 8785127 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah looks good.

Comment 22

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #21)
> Comment on attachment 8785127 [details] [diff] [review]
> [WIP v4] Add/Store a IsPersisent flag into QuotaManger.
> 
> Review of attachment 8785127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah looks good.

Thanks! I'll do the rest part for QuotaManager and QuotaManagerService in bug 1298329. This bug will be left for exposing persisted() attribute from QuotaManagerService to webidl.

Comment 24

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Since we need to merge and remove duplicate code, it's not easy to have persisted() method in a separate bug.