Closed
Bug 1287701
Opened 9 years ago
Closed 8 years ago
Expose persisted() method to StorageManager
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
DUPLICATE
of bug 1286717
People
(Reporter: shawnjohnjr, Assigned: tt)
References
(Blocks 1 open bug)
Details
(Whiteboard: [storage-v1])
Attachments
(2 files, 6 obsolete files)
45.10 KB,
patch
|
janv
:
feedback+
|
Details | Diff | Splinter Review |
22.92 KB,
patch
|
Details | Diff | Splinter Review |
Expose persisted() method to StorageManager
Assignee | ||
Comment 2•9 years ago
|
||
Step 1: Add a flag into originInfo and adjust the mechanism for removing non-persistent storage.
Assignee | ||
Comment 3•9 years ago
|
||
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.
Attachment #8779242 -
Attachment is obsolete: true
Attachment #8780385 -
Flags: feedback?(jvarga)
Comment 4•9 years ago
|
||
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
Attachment #8780385 -
Flags: feedback?(jvarga) → feedback+
Assignee | ||
Comment 5•9 years ago
|
||
(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•9 years ago
|
||
(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
Assignee | ||
Comment 7•9 years ago
|
||
(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•9 years ago
|
||
(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().
Assignee | ||
Comment 9•9 years ago
|
||
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).
Attachment #8780385 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
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!
Attachment #8781488 -
Flags: feedback?(jvarga)
Assignee | ||
Comment 11•9 years ago
|
||
inter-diff patch
Updated•9 years ago
|
Attachment #8781489 -
Attachment is patch: true
Comment 12•9 years ago
|
||
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().
Attachment #8781488 -
Flags: feedback?(jvarga) → feedback+
Assignee | ||
Comment 13•8 years ago
|
||
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.
Attachment #8781488 -
Attachment is obsolete: true
Attachment #8782786 -
Flags: feedback?(jvarga)
Assignee | ||
Comment 14•8 years ago
|
||
inter-diff patch for WIP patch
Attachment #8781489 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8782787 -
Attachment is patch: true
Comment 15•8 years ago
|
||
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.
Attachment #8782786 -
Flags: feedback?(jvarga) → feedback+
Assignee | ||
Comment 16•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
And don't worry about IsFirstPromptRequired() and IsQuotaEnforced(), we'll solve it later.
Assignee | ||
Comment 19•8 years ago
|
||
Rewrite IsTreatedAsPersistent() by checking hash table and provide another function (QuotaManager::IsPersistent()) for checking IsPersistent flag by checking metadata.
Attachment #8782786 -
Attachment is obsolete: true
Attachment #8785127 -
Flags: feedback?(jvarga)
Assignee | ||
Comment 20•8 years ago
|
||
interdiff patch
Attachment #8782787 -
Attachment is obsolete: true
Comment 21•8 years ago
|
||
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.
Attachment #8785127 -
Flags: feedback?(jvarga) → feedback+
Assignee | ||
Comment 22•8 years ago
|
||
(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.
Updated•8 years ago
|
Whiteboard: storage-v1
Reporter | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 24•8 years ago
|
||
Since we need to merge and remove duplicate code, it's not easy to have persisted() method in a separate bug.
Updated•8 years ago
|
Whiteboard: storage-v1 → [storage-v1]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•