Tom Tung [:tt, :ttung] Assignee
	Description • 8 years ago

I did persisted() attribute alone in bug 1287701 but I would like to do them(persisted/persist) here together.

	Tom Tung [:tt, :ttung] Assignee
	Comment 1 • 8 years ago

Implement a mechanism to persist the origin, a 'isPersistent' flag to distinguish whether is persist or not , a hash table to track the DEFAULT type storage, modify isTreatedAsPersistent() function to enure it run in IO thread and check the hash table rather than isApp flag.

	Jan Varga [:janv]
	Comment 2 • 8 years ago

Tom, do you want me to do full review of this patch or just a feedback ?

	Tom Tung [:tt, :ttung] Assignee
	Comment 3 • 8 years ago

(In reply to Jan Varga [:janv] from comment #2)
> Tom, do you want me to do full review of this patch or just a feedback ?

Just a feedback will be great! Thanks! I sent a review request without considering limiting total quota of persistent storage.

	Tom Tung [:tt, :ttung] Assignee
	Comment 4 • 8 years ago

Comment on attachment 8787077 [details] [diff] [review]
Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService.

Changing review request to feedback since I need to rethink about tracking usage for persistent storage.

	Jan Varga [:janv]
	Comment 5 • 8 years ago

I applied the patch locally, I'll send you what I find ...

	Jan Varga [:janv]
	Comment 6 • 8 years ago

Comment on attachment 8787077 [details] [diff] [review]
Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

Some initial comments.

::: dom/quota/ActorsChild.h
@@ +138,5 @@
>    void
>    HandleResponse();
>  
> +  void
> +  HandleResponse(const PersistResponse& aResponse);

Replace |const PersistResponse& aResponse| with |bool aResponse|.

::: dom/quota/QuotaRequests.cpp
@@ +246,5 @@
>  
>  Request::Request(nsIPrincipal* aPrincipal)
>    : RequestBase(aPrincipal)
>  {
>    AssertIsOnOwningThread();

not your fault, but we can add an asserttion for aPrincipal here too

@@ +255,5 @@
> +  : RequestBase(aPrincipal)
> +  , mCallback(aCallback)
> +  , mIsPersistent(false)
> +{
> +  AssertIsOnOwningThread();

assert aPrincipal and aCallback

@@ +288,5 @@
> +  FireCallback();
> +}
> +
> +NS_IMETHODIMP
> +Request::GetIsPersistent(bool* aIsPersistent)

This should be |GetResult(nsIVariant** aResult)|

@@ +305,1 @@
>  NS_IMPL_CYCLE_COLLECTION_INHERITED(Request, RequestBase, mCallback)

You need to add mVariant here, I think.

::: dom/quota/QuotaRequests.h
@@ +117,5 @@
>  {
>    nsCOMPtr<nsIQuotaCallback> mCallback;
>  
> +  // Persist() & Persisted()
> +  bool mIsPersistent;

nsIVariant

@@ +125,5 @@
>  
>    explicit Request(nsIPrincipal* aPrincipal);
>  
> +  explicit Request(nsIPrincipal* aPrincipal,
> +                   nsIQuotaCallback* aCallback);

|explicit| is not needed if there is more than one argument

@@ +131,5 @@
>    void
>    SetResult();
>  
> +  void
> +  SetResult(bool aIsPersistent);

I think we don't need to SetResult methods, merge them to just one and use nsIVariant as an argument. For the case when a request returns nothing, just call variant->SetAsVoid()

::: dom/quota/nsIQuotaRequests.idl
@@ +35,5 @@
>  
>  [scriptable, uuid(22890e3e-ff25-4372-9684-d901060e2f6c)]
>  interface nsIQuotaRequest : nsIQuotaRequestBase
>  {
> +  readonly attribute bool isPersistent;

nsIQuotaRequest interface is intended for generic requests, so adding a concrete type here is not suitable.
How about |readonly attribute nsIVariant result| ?

	Jan Varga [:janv]
	Comment 7 • 8 years ago

Comment on attachment 8787077 [details] [diff] [review]
Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

I'm a bit confused, if this is just another WIP patch or a complete patch.
I think I gave enough feedback and now it's time to finish the patch, do your best and don't forget to add a test, so I'll be able check all code paths.
Thanks.

::: dom/quota/ActorsChild.cpp
@@ +266,5 @@
>      case RequestResponse::TResetAllResponse:
>        HandleResponse();
>        break;
>  
> +    case RequestResponse::TPersistResponse:

What about TPersistedResponse response, it's not handled here at all.
Do you actually have a test for persist() and isPersisted() ?
It would be probably better to write a simple xpcshell test to call the new methods in nsIQuotaManagerService().
I see too many not handled cases in this patch.

::: dom/quota/ActorsParent.cpp
@@ +1295,5 @@
>  }
>  
>  namespace {
> +// A Hash table and it's used to record PERSISTENCE_TYPE_DEFAULT origins.
> +static nsDataHashtable<nsCStringHashKey, bool> gPersistentBoxes;

I don't think it's a good idea to declare it this way.
I think this can be actually a member of QuotaManager class.

@@ +1586,2 @@
>      return true;
> +  } else if (aPersistenceType == PERSISTENCE_TYPE_TEMPORARY) {

no need for |else if| if you have |return true| above

@@ +1975,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  
> +  MOZ_ASSERT_IF(aIsApp, aIsPersistent);

The assertion should be placed in the beginning of this method

@@ -3222,5 @@
>                                   uint64_t aUsageBytes,
>                                   int64_t aAccessTime)
>  {
>    AssertIsOnIOThread();
> -  MOZ_ASSERT(IsTreatedAsTemporary(aPersistenceType, aIsApp));

Why did you remove this assertion. Do we need to create memory objects for persistent origins ?

@@ +3349,5 @@
>  
>  void
>  QuotaManager::RemoveQuota()
>  {
> +  AssertIsOnIOThread();

add new line here

@@ +6679,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +PersistOp::SendResults()

Why did you override this method ?
It looks almost identical to QuotaRequestBase::SendResults()
And at the same time you also overrode GetResponse() which is called by QuotaRequestBase::SendResults()
Doesn't make sense to me.

This class is used by both requests persist() and isPersisted(), but doesn't look finished to me. I'm actually not sure if it's a good idea to share the same class for both requests, especially when we know that persist() might involve showing the prompt, etc.

::: dom/quota/PQuota.ipdl
@@ +47,5 @@
>  struct ResetAllParams
>  {
>  };
>  
> +struct PersistedParams

maybe GetPersistedParams

@@ +52,5 @@
> +{
> +  PrincipalInfo principalInfo;
> +};
> +
> +struct PersistParams

this sounds too terse to me, what about PersistOriginParams ?

::: dom/quota/PQuotaRequest.ipdl
@@ +23,5 @@
>  struct ResetAllResponse
>  {
>  };
>  
> +struct PersistResponse

thow separate PersistRequest and PersistedRequest, but only one response type ?

::: dom/quota/QuotaManagerService.cpp
@@ +634,5 @@
>  
>  NS_IMETHODIMP
> +QuotaManagerService::CheckIsPersistentForPrincipal(nsIPrincipal* aPrincipal,
> +                                                   nsIQuotaCallback* aCallback,
> +                                                   nsIQuotaRequest** _retval)

I see a lot of code duplication here, we might be able to factor out some parts and share it even with existing methods.

	Tom Tung [:tt, :ttung] Assignee
	Comment 8 • 8 years ago

(In reply to Jan Varga [:janv] from comment #7)
> Comment on attachment 8787077 [details] [diff] [review]
> Bug-1298329-v0-Implement persist/persisted in
> QuotaManager/QuotaManagerService.
> 
> Review of attachment 8787077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a bit confused, if this is just another WIP patch or a complete patch.
> I think I gave enough feedback and now it's time to finish the patch, do
> your best and don't forget to add a test, so I'll be able check all code
> paths.
> Thanks.

Thanks for your feedback! 
I'll look at bug 1296591 first to ensure whether we need to reuse OriginInfo even for persistent storage first.

> ::: dom/quota/ActorsChild.cpp
> @@ +266,5 @@
> >      case RequestResponse::TResetAllResponse:
> >        HandleResponse();
> >        break;
> >  
> > +    case RequestResponse::TPersistResponse:
> 
> What about TPersistedResponse response, it's not handled here at all.
> Do you actually have a test for persist() and isPersisted() ?
> It would be probably better to write a simple xpcshell test to call the new
> methods in nsIQuotaManagerService().
> I see too many not handled cases in this patch.

I plan to shared a response for both since both of them return a bool(success/fail, persistent/not-persistent) and error code. It's okay to separate them.

I did test it but I only write a simple test and do the test in local. I'll provide a test in next patch.

> ::: dom/quota/ActorsParent.cpp
> @@ +1295,5 @@
> >  }
> >  
> >  namespace {
> > +// A Hash table and it's used to record PERSISTENCE_TYPE_DEFAULT origins.
> > +static nsDataHashtable<nsCStringHashKey, bool> gPersistentBoxes;
> 
> I don't think it's a good idea to declare it this way.
> I think this can be actually a member of QuotaManager class.

I was thinking that it can be called in the static function(IsTreatedAsPersistent()) and PersistOp so I declared it as a global variable. It's okay to declare it as a static variable in QuotaManger.

> @@ +1586,2 @@
> >      return true;
> > +  } else if (aPersistenceType == PERSISTENCE_TYPE_TEMPORARY) {
> 
> no need for |else if| if you have |return true| above
> 

I forget to change that. Sorry for this. :(

> @@ +1975,5 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> >      return rv;
> >    }
> >  
> > +  MOZ_ASSERT_IF(aIsApp, aIsPersistent);
> 
> The assertion should be placed in the beginning of this method
> 
Sure

> @@ -3222,5 @@
> >                                   uint64_t aUsageBytes,
> >                                   int64_t aAccessTime)
> >  {
> >    AssertIsOnIOThread();
> > -  MOZ_ASSERT(IsTreatedAsTemporary(aPersistenceType, aIsApp));
> 
> Why did you remove this assertion. Do we need to create memory objects for
> persistent origins ?

No, sorry for that.

> @@ +3349,5 @@
> >  
> >  void
> >  QuotaManager::RemoveQuota()
> >  {
> > +  AssertIsOnIOThread();
> 
> add new line here

Sure.

> @@ +6679,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +void
> > +PersistOp::SendResults()
> 
> Why did you override this method ?
> It looks almost identical to QuotaRequestBase::SendResults()
> And at the same time you also overrode GetResponse() which is called by
> QuotaRequestBase::SendResults()
> Doesn't make sense to me.

I should not override the GetResponse(). But, I need to pass the result(mIsPeristent) so I need to override that. 

> This class is used by both requests persist() and isPersisted(), but doesn't
> look finished to me. I'm actually not sure if it's a good idea to share the
> same class for both requests, especially when we know that persist() might
> involve showing the prompt, etc.

Hmm, I'll redesign the class. I did not consider the case about prompt.
I shared a same class for both requests because both of them need to approach hash table.

> ::: dom/quota/PQuota.ipdl
> @@ +47,5 @@
> >  struct ResetAllParams
> >  {
> >  };
> >  
> > +struct PersistedParams
> 
> maybe GetPersistedParams

OK

> @@ +52,5 @@
> > +{
> > +  PrincipalInfo principalInfo;
> > +};
> > +
> > +struct PersistParams
> 
> this sounds too terse to me, what about PersistOriginParams ?

Sure. It's much better!

> 
> ::: dom/quota/PQuotaRequest.ipdl
> @@ +23,5 @@
> >  struct ResetAllResponse
> >  {
> >  };
> >  
> > +struct PersistResponse
> 
> thow separate PersistRequest and PersistedRequest, but only one response
> type ?

Yeah, but it's okay to separate their resoponse.

> 
> ::: dom/quota/QuotaManagerService.cpp
> @@ +634,5 @@
> >  
> >  NS_IMETHODIMP
> > +QuotaManagerService::CheckIsPersistentForPrincipal(nsIPrincipal* aPrincipal,
> > +                                                   nsIQuotaCallback* aCallback,
> > +                                                   nsIQuotaRequest** _retval)
> 
> I see a lot of code duplication here, we might be able to factor out some
> parts and share it even with existing methods.

Sure, I'll try to do that in next patch!

	Jan Varga [:janv]
	Comment 9 • 8 years ago

(In reply to Tom Tung [:tt] from comment #8)
> > @@ +6679,5 @@
> > > +  return NS_OK;
> > > +}
> > > +
> > > +void
> > > +PersistOp::SendResults()
> > 
> > Why did you override this method ?
> > It looks almost identical to QuotaRequestBase::SendResults()
> > And at the same time you also overrode GetResponse() which is called by
> > QuotaRequestBase::SendResults()
> > Doesn't make sense to me.
> 
> I should not override the GetResponse(). But, I need to pass the
> result(mIsPeristent) so I need to override that. 

Actually, you just need to override GetResponse() and QuotaRequestBase::SendResults() should do everything else for you.

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 8 years ago

Hi Janv,

I address the comment and also provide a xpcshell test for eviction which is similar to |temporary_storage.js|. Besides, I modify the callers for calling QM::IsQuotaEnforced() in IndexedDB and ASMJSCache to ensure the callers is in IOthread. However, I'm not sure the way I did in IndexedDB is good or not. 

Could you help me to review this patch? The interdiff for patch will be provided. Thanks!

	Jan Varga [:janv]
	Comment 12 • 8 years ago

Tom, I think the "bound persistent storage by total quota" will affect this patch greatly.
Do you still need my review/feedback for it ?
I'm going to reply to your questions in bug 1296591, so you should be able to work on that.

	Tom Tung [:tt, :ttung] Assignee
	Comment 13 • 8 years ago

(In reply to Jan Varga [:janv] from comment #12)
> Tom, I think the "bound persistent storage by total quota" will affect this
> patch greatly.
> Do you still need my review/feedback for it ?
> I'm going to reply to your questions in bug 1296591, so you should be able
> to work on that.

Sure, I'll remove the review request and start to work on bug 1296591. After that, I'll rebase this patch and re-send the review request. Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 14 • 8 years ago

Hi Jan,

Could you give me some feedback for this patch? Thanks! I want to ensure the direction is correct, although there still are few things (e.g. test) needs to be done.
  
This patch is mainly for bounding persistent storage by total quota. Besides, I change the name for |mTemporaryStorage*| to |mGlobalStorage*| since they are used by both persistent and non-persistent storage.

	Tom Tung [:tt, :ttung] Assignee
	Comment 15 • 8 years ago

Rebase the patch for implementing persist()/persisted().
In this patch, I remove the hashtable for PersistenceTypeDefault's origin since we can track them by originInfo.

Currently, I have a problem for setting global limit. For example, we have 2G persistent storage usage and somehow a user set his global limit to 1G. I think we can pend or prevent the request/promise for setting limit and pop up a prompt to ask the user to delete his persistent storage.

	Jan Varga [:janv]
	Comment 16 • 8 years ago

I'll take a look at the patch today.

	Jan Varga [:janv]
	Comment 17 • 8 years ago

(In reply to Tom Tung [:tt] from comment #14)
> I change the name for |mTemporaryStorage*| to |mGlobalStorage*|
> since they are used by both persistent and non-persistent storage.

This can be a separate patch, real changes are not so visible with the rename.

	Tom Tung [:tt, :ttung] Assignee
	Comment 18 • 8 years ago

(In reply to Jan Varga [:janv] from comment #17)
> (In reply to Tom Tung [:tt] from comment #14)
> > I change the name for |mTemporaryStorage*| to |mGlobalStorage*|
> > since they are used by both persistent and non-persistent storage.
> 
> This can be a separate patch, real changes are not so visible with the
> rename.

Ok, I'll remove the changes for modify name.

	Tom Tung [:tt, :ttung] Assignee
	Comment 19 • 8 years ago

(In reply to Tom Tung [:tt] from comment #18)
The new patch remove the renaming and add a list for recording persistent origin.

Jan, If you have not read the patch #8797562 yet, please see this one.

	Jan Varga [:janv]
	Comment 20 • 8 years ago

Comment on attachment 8798357 [details] [diff] [review]
[WIP] Bug-1296591-P0-v0.1- Bound persistent storage by total quota.

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

The direction of this patch mostly looks ok.

::: dom/quota/ActorsParent.cpp
@@ +2601,5 @@
>      return false;
>    }
>  
>    AssertNoOverflow(quotaManager->mTemporaryStorageUsage, delta);
> +  uint64_t newStorageUsage = quotaManager->mTemporaryStorageUsage + delta;

this should be part of the renamed patch

@@ +2612,5 @@
>      uint64_t sizeToBeFreed =
>        quotaManager->LockedCollectOriginsForEviction(delta, locks);
>  
>      if (!sizeToBeFreed) {
> +      if (quotaManager->HasPersistentStorage()) {

Isn't this out of scope of this bug ?
We don't have such a prompt yet.

@@ +2686,5 @@
>        return false;
>      }
>  
>      AssertNoOverflow(quotaManager->mTemporaryStorageUsage, delta);
> +    newStorageUsage = quotaManager->mTemporaryStorageUsage + delta;

renaming patch

@@ +5179,5 @@
> +}
> +
> +void
> +OriginInfo::LockedIncreaseGroupUsage(QuotaManager* aQuotaManager)
> +{

It seems to me that this method can share some code with CheckTemporaryStorageLimits().

Anyway, I see bigger problems here. For example you call OriginClearCompleted() here, but that can only run on the quota manager IO thread and this method can run on any IO thread right ?

CheckTemporaryStorageLimits() can be a bit simpler because we know that the origins we touch can't be used in the browser yet, but here you actually can touch origins that are in use.
So you need something like we have in MaybeUpdateSize() which collects a list of
inactive origins.
On the other hand, we could end up in a situation when origins are all active, so we can't delete them, but we need to delete them to satisfy the group limit check.
We can actually abort operations for active origins which should remove them from the active origin list.
All this is quite complex and I don't see why it needs to be part of this patch. When do we need to revert the persisted flag ?
I guess, that will be needed for the new preferences UI, right ?
You either need to fix this method or just remove it for now and do it once it's actually needed.

	Jan Varga [:janv]
	Comment 21 • 8 years ago

(In reply to Tom Tung [:tt] from comment #15)
> Created attachment 8797565 [details] [diff] [review]
> [WIP]  Bug-1298329-P1-v0-Implement persist/persisted in
> QuotaManager/QuotaManagerService.
> 
> Rebase the patch for implementing persist()/persisted().
> In this patch, I remove the hashtable for PersistenceTypeDefault's origin
> since we can track them by originInfo.
> 
> Currently, I have a problem for setting global limit. For example, we have
> 2G persistent storage usage and somehow a user set his global limit to 1G. I
> think we can pend or prevent the request/promise for setting limit and pop
> up a prompt to ask the user to delete his persistent storage.

A user sets the global limit ... you mean once quota manager is already running 
and all initialization of storage is done, the user changes the pref for global limit.
How can that happen ?
We don't react to the pref changes once we are initialized as far ask I know.

	Tom Tung [:tt, :ttung] Assignee
	Comment 22 • 8 years ago

(In reply to Jan Varga [:janv] from comment #20)
Thanks for your feedback!

> Comment on attachment 8798357 [details] [diff] [review]
> [WIP] Bug-1296591-P0-v0.1- Bound persistent storage by total quota.
> 
> Review of attachment 8798357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The direction of this patch mostly looks ok.
> 
> @@ +2612,5 @@
> >      uint64_t sizeToBeFreed =
> >        quotaManager->LockedCollectOriginsForEviction(delta, locks);
> >  
> >      if (!sizeToBeFreed) {
> > +      if (quotaManager->HasPersistentStorage()) {
> 
> Isn't this out of scope of this bug ?
> We don't have such a prompt yet.

Yep, I just what to mark that we should do the check and maybe pop up a prompt here. 

> @@ +5179,5 @@
> > +}
> > +
> > +void
> > +OriginInfo::LockedIncreaseGroupUsage(QuotaManager* aQuotaManager)
> > +{
> 
> It seems to me that this method can share some code with
> CheckTemporaryStorageLimits().
> 
> Anyway, I see bigger problems here. For example you call
> OriginClearCompleted() here, but that can only run on the quota manager IO
> thread and this method can run on any IO thread right ?

Sorry, I cannot get it. Can you tell me more about that? 

I thought this is only called by kind of RevertPersist() function though the QuotaManagerService to QuotaManager and that function should inherit OriginOperationBase like FinalizeOriginEvictionOp. Besides, originInfo only be used in QuotaManager. Hence, I couldn't realize why this method can run on any IO thread.

> CheckTemporaryStorageLimits() can be a bit simpler because we know that the
> origins we touch can't be used in the browser yet, but here you actually can
> touch origins that are in use.
> So you need something like we have in MaybeUpdateSize() which collects a
> list of
> inactive origins.
> On the other hand, we could end up in a situation when origins are all
> active, so we can't delete them, but we need to delete them to satisfy the
> group limit check.
> We can actually abort operations for active origins which should remove them
> from the active origin list.
> All this is quite complex and I don't see why it needs to be part of this
> patch. When do we need to revert the persisted flag ?
> I guess, that will be needed for the new preferences UI, right ?

Yes, you are right. 

> You either need to fix this method or just remove it for now and do it once
> it's actually needed.

Maybe remove it and do it until we need it would be better. I'll file a bug for implementing that.

(In reply to Jan Varga [:janv] from comment #21) 
> > Currently, I have a problem for setting global limit. For example, we have
> > 2G persistent storage usage and somehow a user set his global limit to 1G. I
> > think we can pend or prevent the request/promise for setting limit and pop
> > up a prompt to ask the user to delete his persistent storage.
> 
> A user sets the global limit ... you mean once quota manager is already
> running 
> and all initialization of storage is done, the user changes the pref for
> global limit.
> How can that happen ?
> We don't react to the pref changes once we are initialized as far ask I know.

So we cannot actually do the same scenario as [1]? I found out that possible problem when I was writing the test.

[1] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_temporary_storage.js#198-200

	Tom Tung [:tt, :ttung] Assignee
	Comment 23 • 8 years ago

(In reply to Tom Tung [:tt] from comment #22)
> Maybe remove it and do it until we need it would be better. I'll file a bug
> for implementing that.

Oops, Shawn have already filed bug 1301276 for it.

	Jan Varga [:janv]
	Comment 24 • 8 years ago

(In reply to Tom Tung [:tt] from comment #22)
> > Isn't this out of scope of this bug ?
> > We don't have such a prompt yet.
> 
> Yep, I just what to mark that we should do the check and maybe pop up a
> prompt here. 

But you need to add HasPersistenBoxes() for that, and I don't see any other
caller for that.

> > Anyway, I see bigger problems here. For example you call
> > OriginClearCompleted() here, but that can only run on the quota manager IO
> > thread and this method can run on any IO thread right ?
> 
> Sorry, I cannot get it. Can you tell me more about that? 
> 
> I thought this is only called by kind of RevertPersist() function though the
> QuotaManagerService to QuotaManager and that function should inherit
> OriginOperationBase like FinalizeOriginEvictionOp. Besides, originInfo only
> be used in QuotaManager. Hence, I couldn't realize why this method can run
> on any IO thread.

Ok, but then why you call it LockedSomething and call AssertCurrentThreadOwnsQuotaMutex() ?
All the LockedSomething methods are intended for background IO threads (like IndexedD
transaction pool) and they use the quota mutex to protect access to shared structures.

> > A user sets the global limit ... you mean once quota manager is already
> > running 
> > and all initialization of storage is done, the user changes the pref for
> > global limit.
> > How can that happen ?
> > We don't react to the pref changes once we are initialized as far ask I know.
> 
> So we cannot actually do the same scenario as [1]? I found out that possible
> problem when I was writing the test.

But that's a special scenario intended just for testing, it's an xpcshell test,
not a browser. And after we set the pref, we call resetAllDatabases() which
re-initializes entire storage.

	Tom Tung [:tt, :ttung] Assignee
	Comment 25 • 8 years ago

(In reply to Jan Varga [:janv] from comment #24)
> But you need to add HasPersistenBoxes() for that, and I don't see any other
> caller for that.

You're right. I'll remove that function. Should I also remove the list for recording persistent storage?

> Ok, but then why you call it LockedSomething and call
> AssertCurrentThreadOwnsQuotaMutex() ?
> All the LockedSomething methods are intended for background IO threads (like
> IndexedD
> transaction pool) and they use the quota mutex to protect access to shared
> structures.
> 

Sorry for misleading, I didn't notice that. I did that because we often lock with quota mutex and then access originInfo/groupInfos so I added that assertion and name it with LockedXXX. 

> But that's a special scenario intended just for testing, it's an xpcshell
> test,
> not a browser. And after we set the pref, we call resetAllDatabases() which
> re-initializes entire storage.

Oh, I see. Thank you!

	Jan Varga [:janv]
	Comment 26 • 8 years ago

(In reply to Tom Tung [:tt] from comment #22)
> I thought this is only called by kind of RevertPersist() function though the
> QuotaManagerService to QuotaManager and that function should inherit
> OriginOperationBase like FinalizeOriginEvictionOp. Besides, originInfo only
> be used in QuotaManager.

.. originInfo only be used in QuotaManager ...
So you would have a new operation based on OriginOperationBase and you would be
on the quota IO thread, yeah originInfo should be only used by Quota Manager.
But originInfo represents an origin and by "touch origins that are in use" I meant
that there can be directory locks for that origin. You can't just delete origins
that are protected by a directory lock.

	Jan Varga [:janv]
	Comment 27 • 8 years ago

(In reply to Tom Tung [:tt] from comment #25)
> Sorry for misleading, I didn't notice that. I did that because we often lock
> with quota mutex and then access originInfo/groupInfos so I added that
> assertion and name it with LockedXXX. 

LockedRevertOrigin() actually looks good, but LockedIncreaseGroupUsage() don't
Anyway, let's move that to a separate bug.

	Tom Tung [:tt, :ttung] Assignee
	Comment 28 • 8 years ago

(In reply to Jan Varga [:janv] from comment #26)
> .. originInfo only be used in QuotaManager ...
> So you would have a new operation based on OriginOperationBase and you would
> be
> on the quota IO thread, yeah originInfo should be only used by Quota Manager.
> But originInfo represents an origin and by "touch origins that are in use" I
> meant
> that there can be directory locks for that origin. You can't just delete
> origins
> that are protected by a directory lock.

Oh, I see. If we want to do that strongly, we need to get the lock and abort the operation. Thanks for telling me that!

The similar situation will happen when I persist() an origin as a Quota Client hold the directory lock (e.g. indexedDB db.open, then persist , then db.close).

I have two possible solution: 
1. Change the list for recording persistent origin first and pend the accessing metadata part of persist() until lock releasing. If someone call persisted() between persist() and db.close, it only check the list and will get the correct value.
2. Change the MustWaitFor() so that QuotaManager can get the lock and access the metadata but I am afraid it may break the original lock?

I'll find the better way, implement it and send the patch to review later.

	Tom Tung [:tt, :ttung] Assignee
	Comment 29 • 8 years ago

Patch after addressing the feedback.

	Jan Varga [:janv]
	Comment 30 • 8 years ago

(In reply to Tom Tung [:tt] from comment #28)
> The similar situation will happen when I persist() an origin as a Quota
> Client hold the directory lock (e.g. indexedDB db.open, then persist , then
> db.close).

Sorry, I don't understand. persist() should just modify the metadata file.
And I believe we only ever touch that file on the quota manager IO thread.
Every time we touch it, we open the file and close immediately.

We need to get a directory lock and abort operations only when we need to delete
*all* files, not just the metadata file.

But maybe I overlooked something, please do tell me why you think we need to do that
for persist() too.

	Tom Tung [:tt, :ttung] Assignee
	Comment 31 • 8 years ago

(In reply to Jan Varga [:janv] from comment #30)
> Sorry, I don't understand. persist() should just modify the metadata file.
> And I believe we only ever touch that file on the quota manager IO thread.
> Every time we touch it, we open the file and close immediately.
> 
> We need to get a directory lock and abort operations only when we need to
> delete
> *all* files, not just the metadata file.
> 
> But maybe I overlooked something, please do tell me why you think we need to
> do that
> for persist() too.

You are right. I wrote a test for testing persistent storage should not bound by group limit. During testing, I found some failure and misunderstood them to be locking problem. 

Finally, I found out the reason.
 
The test is similar to test_quotaExceeded_recovery but I persist that origin after getting abort event(reaching limit) [1]. Then, I do objectStore to the same transaction again but I got failed. The reason is that we set the mQuotaExceed when previous objectStore [2] which triggers onabort event.

It is related to bug 1182987 and we do that because we don't have enough space to store. However, we do have enough space after calling persist(). I believe we may have same issue for different quota clients. Should we check the |quota - usage| and cancel that kinds of flag after calling persist? 

Currently, I close that db, reset, open it again and then do rest of test to avoid this problem.

[1] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_quotaExceeded_recovery.js#100
[2] http://searchfox.org/mozilla-central/source/dom/indexedDB/IDBTransaction.cpp#796

	Jan Varga [:janv]
	Comment 32 • 8 years ago

Yes, it's related to bug 1182987. Persist() adds a new corner case for the quota exceeded recovery. We need to handle that somehow.

	Tom Tung [:tt, :ttung] Assignee
	Comment 33 • 8 years ago

Hi Jan,

I think I have done the everything for bound persistent storage by total quota. Could you help me to review this patch?

In this patch,
  - Remove two arguments(Origin, isApp) for IsQuotaEnforced and modify logic inside since we just track every type of storage except the PERSISTENCE_TYPE_PERSISTENT.
  - Modify OriginInfo to make it know itself is persistent or not and add function to the changes for calling persist() 
  - Modify MaybeUpdateSize() by adding condition for not changing groupUsage when originInfo is persistent.
  - Modify CollectOriginsForEviction() to prevent persistent from eviction.
  - Add a list(mPersistentOriginBoxes) to record all the persistent origin 

I'll do the following things in other patch:
- Rename the mTemporaryStorageLimit/mTemporaryStorageUsage/.. since the naming is confused after this patch
- Modify mInitializedOrigins since we only store PERSISTENCE_TYPE_PERSISTENT in this list.

	Tom Tung [:tt, :ttung] Assignee
	Comment 34 • 8 years ago

Almost done except the factoring duplication in QuotaManagerService.cpp and modify the IsFirstPromptRequired().

	Jan Varga [:janv]
	Comment 35 • 8 years ago

Ok, I'll take a look ASAP.

	Tom Tung [:tt, :ttung] Assignee
	Comment 36 • 8 years ago

Basically, this patch is done but I would like to add more tests for it.

Jan, could give me some feedback about these two tests since I am not familiar with indexedDB? I wrote these two tests refer to test_temporary_storage.js and test_quotaExceeded_recovery.js. Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 37 • 8 years ago

(In reply to Jan Varga [:janv] from comment #32)
> Yes, it's related to bug 1182987. Persist() adds a new corner case for the
> quota exceeded recovery. We need to handle that somehow.

I file Bug 1309177 for it.

	Tom Tung [:tt, :ttung] Assignee
	Comment 38 • 8 years ago

Addressed the comment and rebased the patch and I think I've done the everything for persist()/persisted() in QuotaManager. Could you help me to review this patch, Jan? Thanks!!

This patch mainly for providing a idl interface in QuotaManagerService and implement the PersistOrigin()/IsPersistent() in QuotaManager to persist the origin(modify the metadata, originInfo if we have, add it to the list) and check the ispersistent flag (from list and then originInfo and then metadata). Besides, I add two operation class to do the persist()/persisted() operation in QuotaManager which are OriginPersistOp/IsOriginPersistedOp.

	Jan Varga [:janv]
	Comment 39 • 8 years ago

Comment on attachment 8800562 [details] [diff] [review]
Bug-1298329-P1-v1.1-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

Just went quickly through the patch, I remember I suggested to use nsIVariant in QuotaRequests.h/.cpp

::: dom/quota/QuotaRequests.h
@@ +117,5 @@
>  {
>    nsCOMPtr<nsIQuotaCallback> mCallback;
>  
> +  // Persist() & Persisted()
> +  bool mIsPersistent;

I suggested to use nsIVariant for this in comment 6. Is there a reason to not use it ?

@@ +131,5 @@
>    void
>    SetResult();
>  
> +  void
> +  SetResult(bool aIsPersistent);

dtto

	Tom Tung [:tt, :ttung] Assignee
	Comment 40 • 8 years ago

Hi Jan, 

I update the tests please see this patch if you have not read the patch for 8799700. I'll remove the feedback request for that.

In this patch, I create three tests: one for basic test(test_persistent_basic e.g. persist before store anything, ... etc), another one for eviction test(test_persistent_storage), the other one for group limit test(test_groupLimit).

	Tom Tung [:tt, :ttung] Assignee
	Comment 41 • 8 years ago

(In reply to Jan Varga [:janv] from comment #39)
> Comment on attachment 8800562 [details] [diff] [review]
> Bug-1298329-P1-v1.1-Implement persist/persisted in
> QuotaManager/QuotaManagerService.
> 
> Review of attachment 8800562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just went quickly through the patch, I remember I suggested to use
> nsIVariant in QuotaRequests.h/.cpp
> 
> ::: dom/quota/QuotaRequests.h
> @@ +117,5 @@
> >  {
> >    nsCOMPtr<nsIQuotaCallback> mCallback;
> >  
> > +  // Persist() & Persisted()
> > +  bool mIsPersistent;
> 
> I suggested to use nsIVariant for this in comment 6. Is there a reason to
> not use it ?
> 
> @@ +131,5 @@
> >    void
> >    SetResult();
> >  
> > +  void
> > +  SetResult(bool aIsPersistent);
> 
> dtto

Sorry for that, I'll recheck the patch. :(

	Tom Tung [:tt, :ttung] Assignee
	Comment 42 • 8 years ago

Comment on attachment 8800562 [details] [diff] [review]
Bug-1298329-P1-v1.1-Implement persist/persisted in QuotaManager/QuotaManagerService.

I find out that I somehow miss the comment 6 so removing the review request. I'll address them and then send the patch for review later.

	Tom Tung [:tt, :ttung] Assignee
	Comment 43 • 8 years ago

Sorry for overlooking the comment 6. I address them in this patch. Could you help me to review this patch, Jan? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 44 • 8 years ago

Sorry for disturbance!

Rebase the patch due to modify the P1. Basically, this patch do the same thing as I put in comment 40. Change the attachment 8800564 [details] [diff] [review]'s feedback request to this one.

	Jan Varga [:janv]
	Comment 45 • 8 years ago

Ok, I'll start reviewing it.

	Jan Varga [:janv]
	Comment 46 • 8 years ago

Comment on attachment 8801022 [details] [diff] [review]
Bug-1298329-P2-v0.2-Test for persisted() and persist().

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

::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
@@ +36,5 @@
>  [test_database_onclose.js]
>  [test_defaultStorageUpgrade.js]
>  [test_idbSubdirUpgrade.js]
>  [test_globalObjects_ipc.js]
> +[test_groupLimit.js]

Do you really want to add it here ?
The "skip-if" below is for "test_globalObjects_ipc.js"

	Jan Varga [:janv]
	Comment 47 • 8 years ago

Comment on attachment 8799679 [details] [diff] [review]
Bug-1296591-P0-v1.1- Bound persistent storage by total quota.

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

Reviewing this now.

	Tom Tung [:tt, :ttung] Assignee
	Comment 48 • 8 years ago

(In reply to Jan Varga [:janv] from comment #46)
> ::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
> Do you really want to add it here ?
> The "skip-if" below is for "test_globalObjects_ipc.js"

No, it was my misunderstanding. Thanks for pointing it out. I'll fix that in next patch.

	Jan Varga [:janv]
	Comment 49 • 8 years ago

Comment on attachment 8799679 [details] [diff] [review]
Bug-1296591-P0-v1.1- Bound persistent storage by total quota.

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

Some initial comments, more to come ...

::: dom/quota/QuotaManager.h
@@ +518,5 @@
>  
> +  // A list of all the persistent origin under persistence type default. This
> +  // list isn't protected by any mutex but it is only ever written on the IO
> +  // thread.
> +  nsTArray<nsCString> mPersistentOriginBoxes;

1. I would replace the array with a hash table
2. I would call it mPersistedOrigins
3. It's only "touched" on the IO thread
4. I think we don't need it now

The other patch implements IsPersistent(...) method and that checks OriginInfo if it's been persisted or not. So we don't have to read the metadata file from disk and just check the memory structures if they exist. I have all patches applied locally and it seems to me that you update mPersistentOriginBoxes the same way and at same places like you update OriginInfo. So I believe that mPersistentOriginBoxes is redundant/useless until you plan to use it for something else.

	Tom Tung [:tt, :ttung] Assignee
	Comment 50 • 8 years ago

(In reply to Jan Varga [:janv] from comment #49)
> Comment on attachment 8799679 [details] [diff] [review]
> Bug-1296591-P0-v1.1- Bound persistent storage by total quota.
> 
> Review of attachment 8799679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some initial comments, more to come ...
> 
> ::: dom/quota/QuotaManager.h
> @@ +518,5 @@
> 1. I would replace the array with a hash table
> 2. I would call it mPersistedOrigins
> 3. It's only "touched" on the IO thread
> 4. I think we don't need it now
> 
> The other patch implements IsPersistent(...) method and that checks
> OriginInfo if it's been persisted or not. So we don't have to read the
> metadata file from disk and just check the memory structures if they exist.
> I have all patches applied locally and it seems to me that you update
> mPersistentOriginBoxes the same way and at same places like you update
> OriginInfo. So I believe that mPersistentOriginBoxes is redundant/useless
> until you plan to use it for something else.

Yeah, it's redundant right now.
I planed to use |mPersistentOriginBoxes| to pass an array to show all the persistent storage to front-end in preference purpose. I was thinking that it's much better to pass an array than a tree (groupInfopair/groupInfo/originInfo) and I'm afraid that it cause some overhead to parse the tree to an array every time when an user checks persistent storage in preference. 

Could you give me some suggestions about how to do, Jan? Thanks!

	Jan Varga [:janv]
	Comment 51 • 8 years ago

How often someone would go to the storage preferences ?
I think it's ok to generate the array on demand.

	Jan Varga [:janv]
	Comment 52 • 8 years ago

And the array only contains raw origin names. I think preferences will need more information than that. So you would have to "join" array elements with other structures anyway.

	Tom Tung [:tt, :ttung] Assignee
	Comment 53 • 8 years ago

(In reply to Jan Varga [:janv] from comment #51)
(In reply to Jan Varga [:janv] from comment #52)
> And the array only contains raw origin names. I think preferences will need
> more information than that. So you would have to "join" array elements with
> other structures anyway.

Sure, I'll do that in next patch. Thanks for feedback!

	Jan Varga [:janv]
	Comment 54 • 8 years ago

Comment on attachment 8799679 [details] [diff] [review]
Bug-1296591-P0-v1.1- Bound persistent storage by total quota.

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

::: dom/quota/QuotaManager.h
@@ +149,5 @@
>                       const nsACString& aOrigin,
>                       bool aIsApp,
>                       uint64_t aUsageBytes,
> +                     int64_t aAccessTime,
> +                     bool aIsPersistent);

I would call it aPersisted

@@ +385,5 @@
>                          const nsACString& aOrigin,
>                          bool aIsApp);
>  
>    static bool
> +  IsQuotaEnforced(PersistenceType aPersistenceType);

I think is method is now useless. All it does is |aPersistenceType != PERSISTENCE_TYPE_PERSISTENT|. The only caller outside dom/quota is indexeddb and asmjscache. indexeddb needs to call it because it supports the explicit persistent storage, that support will be removed at some point and asmjscache is going away too. I also think that replacing IsQuotaEnforced() with the simple check in dom/quota will make it easier to read the code.

@@ +462,5 @@
>                     const nsACString& aGroup,
>                     const nsACString& aOrigin,
>                     bool aIsApp,
>                     int64_t aAccessTime,
> +                   bool aIsPersistent,

aPersisted

	Jan Varga [:janv]
	Comment 55 • 8 years ago

Comment on attachment 8799679 [details] [diff] [review]
Bug-1296591-P0-v1.1- Bound persistent storage by total quota.

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

I'm still not finished with MaybeUpdateSize() and EnsureOriginIsInitialized(), however, here are some additional comments.

Could you address the comments I sent so far and submit a new P0 patch ?
Thanks

::: dom/asmjscache/AsmJSCache.cpp
@@ +646,5 @@
>  
>      MOZ_ASSERT_IF(mWriteParams.mInstalled, mIsApp);
>  
>      if (mWriteParams.mInstalled &&
> +        !QuotaManager::IsQuotaEnforced(quota::PERSISTENCE_TYPE_PERSISTENT)) {

This IsQuotaEnforced() check was useless even before your patches. It's my fault that I didn't remove it. I see that it's removed in the P1 patch, but I think it should be removed in this patch.

@@ +692,5 @@
>  
>    InitPersistenceType();
>  
>    mEnforcingQuota =
> +    QuotaManager::IsQuotaEnforced(mPersistence);

This now fits on one line.

::: dom/quota/ActorsParent.cpp
@@ +458,5 @@
>    friend class QuotaObject;
>  
>  public:
>    OriginInfo(GroupInfo* aGroupInfo, const nsACString& aOrigin, bool aIsApp,
> +             uint64_t aUsage, int64_t aAccessTime, bool aIsPersistent)

aPersisted

@@ +463,2 @@
>    : mGroupInfo(aGroupInfo), mOrigin(aOrigin), mUsage(aUsage),
> +    mAccessTime(aAccessTime), mIsApp(aIsApp), mIsPersistent(aIsPersistent)

mPersisted(aPersisted)

Also, please add MOZ_ASSERT(aGrouInfo) and then MOZ_ASSERT_IF(aPersisted, aGroupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT)

@@ +474,5 @@
>      return mAccessTime;
>    }
>  
> +  bool
> +  IsPersistent() const

Persisted()

@@ +480,5 @@
> +    return mIsPersistent;
> +  }
> +
> +  void
> +  LockedPersistOrigin();

LockedPersist() and this doesn't have to "public". Move it after LockedUpdateAccessTime().

@@ +512,5 @@
>    const nsCString mOrigin;
>    uint64_t mUsage;
>    int64_t mAccessTime;
>    const bool mIsApp;
> +  bool mIsPersistent;

mPersisted

@@ +589,5 @@
>  };
>  
>  class GroupInfoPair
>  {
> +  friend class OriginInfo;

Is this really needed ?

@@ +2608,5 @@
>        quotaManager->LockedCollectOriginsForEviction(delta, locks);
>  
>      if (!sizeToBeFreed) {
> +      // XXX prompt for asking to delete persistent storage if there is any
> +      // persistent storage.

I think the terminology here should be "persisted origins" instead of "persistent storage".

@@ +5124,5 @@
> +  MOZ_ASSERT(mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT);
> +
> +  mIsPersistent = true;
> +  // Remove Usage from GroupInfo
> +  LockedDecreaseGroupUsage();

I'm not sure if we need a separate method for this. It seems there's only one caller for LockedDecreaseGroupUsage

@@ +5545,5 @@
>                   js::ProfileEntry::Category::OTHER);
>  
>    for (RefPtr<DirectoryLockImpl>& lock : mLocks) {
>      aQuotaManager->OriginClearCompleted(lock->GetPersistenceType().Value(),
> +                                        lock->GetOriginScope().GetOrigin());

It seems this was the only caller of DirectoryLock::GetIsApp(). The method can be removed. The member variable mIsApp in DirectoryLock is not needed too. Also, OpenDirectory(), CreateDirectoryLock() don't need this boolean.

	Jan Varga [:janv]
	Comment 56 • 8 years ago

Comment on attachment 8799679 [details] [diff] [review]
Bug-1296591-P0-v1.1- Bound persistent storage by total quota.

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

::: dom/quota/ActorsParent.cpp
@@ +4534,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    if (IsQuotaEnforced(aPersistenceType)) {
> +      mPersistentOriginBoxes.AppendElement(aOrigin);

This goes away ...

@@ +4536,5 @@
> +    if (IsQuotaEnforced(aPersistenceType)) {
> +      mPersistentOriginBoxes.AppendElement(aOrigin);
> +    }
> +    if (!IsQuotaEnforced(aPersistenceType)) {
> +      mInitializedOrigins.AppendElement(OriginKey(aPersistenceType, aOrigin));

... this should stay, but why is there now the !IsQuotaEnforced(aPersistenceType) check ?

we are here in the "else" branch of "if (IsTreatedAsPersistent(aPersistenceType, aIsApp))"

so this additonal check is useless, no ?

	Jan Varga [:janv]
	Comment 57 • 8 years ago

I wonder if we need to support the isApp flag at all. I know that b2g is gone, they are removing all code related to b2g from the tree. However I'm not if they app model is going to be removed too.
The reviewing would be a bit easier if we could remove support for the isApp from quota manager.

	Jan Varga [:janv]
	Comment 58 • 8 years ago

Fabrice, maybe it's obvious, but I didn't follow discussions about this much. Please see comment 57, what is the current status and plan for near future regarding installed apps ? Basically, we do this in quota manager:
isApp = principal->GetAppStatus() != nsIPrincipal::APP_STATUS_NOT_INSTALLED;

and we treat such principals/origins differently, we don't track quota for them

Thanks

	Jan Varga [:janv]
	Comment 59 • 8 years ago

Ok there are two bugs for dom/apps removal bug 1261019 and bug 1291291.

Ehsan removed mozApps in bug 1261019.

Fabrice commented in bug 1291291:
> jst, do you have someone that can tackle that? That will lead to a lot of
> code removal all over the place, since we won't need the appId and appStatus
> on the principals.

So it doesn't make sense for quota manager to support special handling for installed apps.
I'm going to remove it.

	Tom Tung [:tt, :ttung] Assignee
	Comment 60 • 8 years ago

Thanks for the review!

(In reply to Jan Varga [:janv] from comment #54)
> @@ +385,5 @@
> >                          const nsACString& aOrigin,
> >                          bool aIsApp);
> >  
> >    static bool
> > +  IsQuotaEnforced(PersistenceType aPersistenceType);
> 
> I think is method is now useless. All it does is |aPersistenceType !=
> PERSISTENCE_TYPE_PERSISTENT|. The only caller outside dom/quota is indexeddb
> and asmjscache. indexeddb needs to call it because it supports the explicit
> persistent storage, that support will be removed at some point and
> asmjscache is going away too. I also think that replacing IsQuotaEnforced()
> with the simple check in dom/quota will make it easier to read the code.

I would like to remove the IsQuotaEnforced() even in asmjscache/indexeddb for the same reason(make it easier to read the code) in dom/quota because I find out there have already existed some direct checks for persistence type in asmjscache/indexeddb. 

(In reply to Jan Varga [:janv] from comment #55)
> Comment on attachment 8799679 [details] [diff] [review]
> Bug-1296591-P0-v1.1- Bound persistent storage by total quota.
> 
> Review of attachment 8799679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm still not finished with MaybeUpdateSize() and
> EnsureOriginIsInitialized(), however, here are some additional comments.
> 
> Could you address the comments I sent so far and submit a new P0 patch ?
> Thanks

Sure! Could I also need to update and submit the new P1(re-base due to P0) patch?

> ::: dom/quota/ActorsParent.cpp
> @@ +480,5 @@
> > +    return mIsPersistent;
> > +  }
> > +
> > +  void
> > +  LockedPersistOrigin();
> 
> LockedPersist() and this doesn't have to "public". Move it after
> LockedUpdateAccessTime().

Sure, I did not notice that QuotaManager is the friend class of OriginInfo. 
 
> @@ +589,5 @@
> >  };
> >  
> >  class GroupInfoPair
> >  {
> > +  friend class OriginInfo;
> 
> Is this really needed ?

No, I'll remove it in next patch.

I used to revert the origin type from persistent to best-effort and thus originInfo needed to access groupInfoPair member variable. However, we decided remove that function in this patch and I forgot to remove that.

> @@ +5124,5 @@
> > +  MOZ_ASSERT(mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT);
> > +
> > +  mIsPersistent = true;
> > +  // Remove Usage from GroupInfo
> > +  LockedDecreaseGroupUsage();
> 
> I'm not sure if we need a separate method for this. It seems there's only
> one caller for LockedDecreaseGroupUsage

It's okay to remove that. I separate this function only for being more readable.

> @@ +5545,5 @@
> >                   js::ProfileEntry::Category::OTHER);
> >  
> >    for (RefPtr<DirectoryLockImpl>& lock : mLocks) {
> >      aQuotaManager->OriginClearCompleted(lock->GetPersistenceType().Value(),
> > +                                        lock->GetOriginScope().GetOrigin());
> 
> It seems this was the only caller of DirectoryLock::GetIsApp(). The method
> can be removed. The member variable mIsApp in DirectoryLock is not needed
> too. Also, OpenDirectory(), CreateDirectoryLock() don't need this boolean.

Sure, I'll remove the member variable mIsApp in DirectoryLockImpl.

(In reply to Jan Varga [:janv] from comment #56)
> Comment on attachment 8799679 [details] [diff] [review]
> Bug-1296591-P0-v1.1- Bound persistent storage by total quota.
> 
> Review of attachment 8799679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +4536,5 @@
> > +    if (IsQuotaEnforced(aPersistenceType)) {
> > +      mPersistentOriginBoxes.AppendElement(aOrigin);
> > +    }
> > +    if (!IsQuotaEnforced(aPersistenceType)) {
> > +      mInitializedOrigins.AppendElement(OriginKey(aPersistenceType, aOrigin));
> 
> ... this should stay, but why is there now the
> !IsQuotaEnforced(aPersistenceType) check ?
> 
> we are here in the "else" branch of "if
> (IsTreatedAsPersistent(aPersistenceType, aIsApp))"

Really? I think that it's inside the "if (IsTreatedAsPersistent(aPersistenceType, aIsApp))" so this check is used to ensure we only track PERSISTENCE_TYPE_PERSISTENT origins in mInitializedOrigins (not the PERSISTENCE_TYPE_DEFAULT).

	Jan Varga [:janv]
	Comment 61 • 8 years ago

(In reply to Tom Tung [:tt] from comment #60)
> Thanks for the review!

sure, np

> > Could you address the comments I sent so far and submit a new P0 patch ?
> > Thanks
> 
> Sure! Could I also need to update and submit the new P1(re-base due to P0)
> patch?

Actually, could you wait until I remove appId all together ?

> > It seems this was the only caller of DirectoryLock::GetIsApp(). The method
> > can be removed. The member variable mIsApp in DirectoryLock is not needed
> > too. Also, OpenDirectory(), CreateDirectoryLock() don't need this boolean.
> 
> Sure, I'll remove the member variable mIsApp in DirectoryLockImpl.

I'll do it as part of appId removal.

> > @@ +4536,5 @@
> > > +    if (IsQuotaEnforced(aPersistenceType)) {
> > > +      mPersistentOriginBoxes.AppendElement(aOrigin);
> > > +    }
> > > +    if (!IsQuotaEnforced(aPersistenceType)) {
> > > +      mInitializedOrigins.AppendElement(OriginKey(aPersistenceType, aOrigin));
> > 
> > ... this should stay, but why is there now the
> > !IsQuotaEnforced(aPersistenceType) check ?
> > 
> > we are here in the "else" branch of "if
> > (IsTreatedAsPersistent(aPersistenceType, aIsApp))"
> 
> Really? I think that it's inside the "if
> (IsTreatedAsPersistent(aPersistenceType, aIsApp))" so this check is used to
> ensure we only track PERSISTENCE_TYPE_PERSISTENT origins in
> mInitializedOrigins (not the PERSISTENCE_TYPE_DEFAULT).

Oh, yes, but ...

if (IsQuotaEnforced(aPersistenceType)) {
  mPersistentOriginBoxes.AppendElement(aOrigin);
}
if (!IsQuotaEnforced(aPersistenceType)) {
  mInitializedOrigins.AppendElement(OriginKey(aPersistenceType, aOrigin));
}

That could use a simple "else", doesn't make sense to call the same method twice.

Anyway, in this if (IsTreatedAsPersistent(aPersistenceType, aIsApp))" you call InitializeOrigin(/* aIsPersistent */ true), but then you append to mInitializedOrigins only if !IsQuotaEnforced(aPersistenceType)

This may be technically "correct" but all this look confusing (especially the naming of methods in this new context of persisted origins in default storage) and we should really remove the appId first and rebase your patches on top of that. It will make your and my life easier :)

The P0 patch shouldn't depend on changes in P1 (I'm not 100% sure if there are any dependencies), I'd like to make it a standalone patch since the changes we are doing here are quite risky, we should be very careful.

	Tom Tung [:tt, :ttung] Assignee
	Comment 62 • 8 years ago

(In reply to Jan Varga [:janv] from comment #61)
> Actually, could you wait until I remove appId all together ?

Sure. Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 63 • 8 years ago

WIP patch for P0, addressed comment #49 #54 #55 #56 and #60. 

I'll wait the appId's removal based on comment #61 and re-base this patch again at that time.

	Tom Tung [:tt, :ttung] Assignee
	Comment 64 • 8 years ago

WIP patch, re-base on P0's changes.

(In reply to Jan Varga [:janv] from comment #61)
> The P0 patch shouldn't depend on changes in P1 (I'm not 100% sure if there
> are any dependencies), I'd like to make it a standalone patch since the
> changes we are doing here are quite risky, we should be very careful.

You are right, P0 should not depend on changes in P1 and it should be standalone patch.  

I meant P1 depend on P0 so I need to re-base P1 after addressing the comment to P0. Sorry for making misunderstanding.

	Jan Varga [:janv]
	Comment 65 • 8 years ago

Tom, I submitted some initial patches in bug 1311057, please apply them and re-base your patches. Thanks.

	Tom Tung [:tt, :ttung] Assignee
	Comment 66 • 8 years ago

Re-based after applying initial patches in bug 1311057.

	Tom Tung [:tt, :ttung] Assignee
	Comment 67 • 8 years ago

Re-based after applying initial patches in bug 1311057, too.

	Tom Tung [:tt, :ttung] Assignee
	Comment 68 • 8 years ago

Re-based after applying initial patches in bug 1311057.

(In reply to Jan Varga [:janv] from comment #65)
> Tom, I submitted some initial patches in bug 1311057, please apply them and
> re-base your patches. Thanks.

Sure, I've re-based them. Thanks for your patches!

	Tom Tung [:tt, :ttung] Assignee
	Comment 69 • 8 years ago

Re-base P2 again after applying part 4 in bug 1311057.

	Tom Tung [:tt, :ttung] Assignee
	Comment 70 • 8 years ago

Update the patch for checking whether the origin has already been persisted. If it is, we should not persist it again.

	Tom Tung [:tt, :ttung] Assignee
	Comment 71 • 8 years ago

Comment on attachment 8809720 [details] [diff] [review]
[Rebased Patch] Bug-1296591-P0-v2.1- Bound persistent storage by total quota.

remove WIP title

	Jan Varga [:janv]
	Comment 72 • 8 years ago

Comment on attachment 8809720 [details] [diff] [review]
[Rebased Patch] Bug-1296591-P0-v2.1- Bound persistent storage by total quota.

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

Since, isApp is gone, all storage/default subdirs are quota initialized. So "bound persistent storage by total quota" is not right name for this patch anymore. It's more like "add persisted attribute ..."

I'm a bit worried about changes in QuotaObject::MaybeUpdateSize(), this is very sensitive stuff, the method can be called from multiple threads, there's locking, waiting, relocking, etc. I have to take a look at it one more time before I give final r+.

::: dom/quota/ActorsParent.cpp
@@ +4998,5 @@
> +                       uint64_t aUsage, int64_t aAccessTime, bool aPersisted)
> +  : mGroupInfo(aGroupInfo), mOrigin(aOrigin), mUsage(aUsage),
> +    mAccessTime(aAccessTime), mPersisted(aPersisted)
> +{
> +  MOZ_COUNT_CTOR(OriginInfo);

assertions first, then an empty line and then MOZ_COUNT_CTOR

	Tom Tung [:tt, :ttung] Assignee
	Comment 73 • 8 years ago

Addressed comment #72. Also, to reduce unnecessary group usage computations when the origin has persisted attribute, I rewrite few if-conditions in MaybeUpdateSize(). 

I will provide interdiff patch between this patch(v3) and previous patch(v2.1)

	Tom Tung [:tt, :ttung] Assignee
	Comment 75 • 8 years ago

Updated patch per f2f discussion this afternoon.
In this patch, I mainly do the following things:
  - Add assertion about holding mutex in accesstime and persist's getter function.
  - Integrate if-conditions for checking persisted in MaybeUpdateSize().
  - Add ifdef to hold a QuotaMutex when we remove doomorigins because we add an assert for getter function.

I'll also update the interdiff.

	Tom Tung [:tt, :ttung] Assignee
	Comment 76 • 8 years ago

interdiff patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 77 • 8 years ago

Comment on attachment 8810336 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService.

Change the review flag to the newest patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 78 • 8 years ago

Comment on attachment 8809773 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist().

Change the review flag to the newest patch.

	Jan Varga [:janv]
	Comment 79 • 8 years ago

Comment on attachment 8817118 [details] [diff] [review]
[Rebased Patch] Bug-1296591-P0-v4- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota).

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

::: dom/quota/ActorsParent.cpp
@@ +5158,5 @@
> +{
> +  AssertCurrentThreadOwnsQuotaMutex();
> +  MOZ_ASSERT(mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT);
> +
> +  mPersisted = true;

Nit: add an empty line here

@@ +5235,5 @@
>  
>    for (uint32_t index = mOriginInfos.Length(); index > 0; index--) {
>      OriginInfo* originInfo = mOriginInfos[index - 1];
>  
> +    if (!originInfo->Persisted()) {

I think you forgot to add this check to LockedRemoveOriginInfo(aOrigin).

	Tom Tung [:tt, :ttung] Assignee
	Comment 80 • 8 years ago

(In reply to Jan Varga [:janv] from comment #79)

Thanks for the review! I learn a lot from that.

> Comment on attachment 8817118 [details] [diff] [review]
> [Rebased Patch] Bug-1296591-P0-v4- Add persisted attribute to originInfo and
> bound persisted origins only by total quota (not by group quota).
> 
> Review of attachment 8817118 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +5235,5 @@
> >  
> >    for (uint32_t index = mOriginInfos.Length(); index > 0; index--) {
> >      OriginInfo* originInfo = mOriginInfos[index - 1];
> >  
> > +    if (!originInfo->Persisted()) {
> 
> I think you forgot to add this check to LockedRemoveOriginInfo(aOrigin).

Sorry for forgetting to add this check. I should find it.

	Tom Tung [:tt, :ttung] Assignee
	Comment 81 • 8 years ago

Add dependency since all the patches in these bug are based on Bug 1311057.

	Jan Varga [:janv]
	Comment 83 • 8 years ago

Comment on attachment 8810336 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

Some initial comments...

::: dom/quota/ActorsChild.cpp
@@ +232,5 @@
>    AssertIsOnOwningThread();
>    MOZ_ASSERT(mRequest);
>  
> +  RefPtr<nsVariant> variant = new nsVariant();
> +  variant->SetAsVoid();

Nit: add an empty line here

@@ +243,5 @@
> +  AssertIsOnOwningThread();
> +  MOZ_ASSERT(mRequest);
> +
> +  RefPtr<nsVariant> variant = new nsVariant();
> +  variant->SetAsBool(aResponse);

Nit: add an empty line here

::: dom/quota/QuotaManager.h
@@ +201,5 @@
>                          int64_t* aTimestamp,
>                          nsACString& aSuffix,
>                          nsACString& aGroup,
> +                        nsACString& aOrigin,
> +                        bool* aPersisted);

I don't see why this doesn't follow the order in the metadata file.
aPersisted should go after aTimestamp.

::: dom/quota/nsIQuotaManagerService.idl
@@ +85,5 @@
> +   *        The callback that will be called when the isPersistent flag is
> +   *        available.
> +   */
> +  [must_use] nsIQuotaRequest
> +  checkIsPersistentForPrincipal(in nsIPrincipal aPrincipal,

Pff, the method name sounds rather awkward. What about just "persisted" ?
This way we will be closer to match StorageManager.webidl

@@ +86,5 @@
> +   *        available.
> +   */
> +  [must_use] nsIQuotaRequest
> +  checkIsPersistentForPrincipal(in nsIPrincipal aPrincipal,
> +                                in nsIQuotaCallback aCallback);

I'm not sure if we want to follow getUsageForPrincipal() or clearStoragesForPrincipal(). The former allows to pass a callback, the latter doesn't, but the callback can be easily set on the request which is returned by the method.
My original intention was to use the same pattern as in IDB. A method returns a request and then you can set an event listener/callback. So I think we don't need the 2nd argument here. The same applies for persist() below.

@@ +89,5 @@
> +  checkIsPersistentForPrincipal(in nsIPrincipal aPrincipal,
> +                                in nsIQuotaCallback aCallback);
> +
> +  /**
> +   * Persist the storages stored for given origin.

What about just "Persist given origin."

@@ +95,5 @@
> +   * @param aPrincipal
> +   *        A principal for the origin which we want to persist.
> +   * @param aCallback
> +   *        The callback that will be called when the isPersistent flag is
> +   *        available.

This description is incorrect, right ? I mean the isPersistent flag.

@@ +98,5 @@
> +   *        The callback that will be called when the isPersistent flag is
> +   *        available.
> +   */
> +  [must_use] nsIQuotaRequest
> +  persistStorageForPrincipal(in nsIPrincipal aPrincipal,

I think we should stop copying this *ForPrincipal pattern. What about just "persist(...)" ?

	Jan Varga [:janv]
	Comment 84 • 8 years ago

Comment on attachment 8810336 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

More comments.

::: dom/quota/ActorsChild.cpp
@@ +268,5 @@
>      case RequestResponse::TClearOriginResponse:
>      case RequestResponse::TClearOriginsResponse:
>      case RequestResponse::TClearAllResponse:
>      case RequestResponse::TResetAllResponse:
> +    case RequestResponse::TPersistOriginResponse:

This will become TPersistResponse

@@ +273,4 @@
>        HandleResponse();
>        break;
>  
> +    case RequestResponse::TGetPersistedResponse:

This will become TPersistedResponse

::: dom/quota/ActorsParent.cpp
@@ +4813,5 @@
> +  // Search the originInfo if it has already been created.
> +  {
> +    MutexAutoLock lock(mQuotaMutex);
> +    GroupInfoPair* pair;
> +    if (mGroupInfoPairs.Get(aGroup, &pair)) {

So, if there's no "pair" what then ?

@@ +4889,5 @@
> +  }
> +
> +  // We assumed that directory (metadata) should be created.
> +  if (created) {
> +    return NS_ERROR_UNEXPECTED;

Pff, I don't understand the logic here at all.

::: dom/quota/PQuota.ipdl
@@ +48,5 @@
>  struct ResetAllParams
>  {
>  };
>  
> +struct GetPersistedParams

PersistedParams

@@ +53,5 @@
> +{
> +  PrincipalInfo principalInfo;
> +};
> +
> +struct PersistOriginParams

PersistParams

::: dom/quota/PQuotaRequest.ipdl
@@ +23,5 @@
>  struct ResetAllResponse
>  {
>  };
>  
> +struct GetPersistedResponse

PersistedResponse

@@ +25,5 @@
>  };
>  
> +struct GetPersistedResponse
> +{
> +  bool isPersistent;

just "persisted"

@@ +28,5 @@
> +{
> +  bool isPersistent;
> +};
> +
> +struct PersistOriginResponse

PersistResponse

::: dom/quota/QuotaManager.h
@@ +210,5 @@
>                                     int64_t* aTimestamp,
>                                     nsACString& aSuffix,
>                                     nsACString& aGroup,
> +                                   nsACString& aOrigin,
> +                                   bool* aPersisted);

Fix the order here too

@@ +221,5 @@
>                                     bool aPersistent,
>                                     int64_t* aTimestamp);
>  
> +  nsresult
> +  PersistDirectoryMetadata2(nsIFile* aDirectory);

I don't think we need this method here, there's only one caller anyway.
The code can live in OriginPersistOp.

@@ +348,5 @@
>    GetGroupUsageAndLimit(const nsACString& aGroup,
>                          UsageInfo* aUsageInfo);
>  
> +  // The function for checking the origin is persistent or not. It also
> +  // initialize the static hashtable sPersistentBoxes if it is needed.

sPersistentBoxes doesn't exist anymore, right ?

@@ +350,5 @@
>  
> +  // The function for checking the origin is persistent or not. It also
> +  // initialize the static hashtable sPersistentBoxes if it is needed.
> +  nsresult
> +  Persisted(PersistenceType aPersistenceType,

I think it's useless to have aPersistenceType here.
nsIQuotaManagerService doesn't allow it either and actually persisted()/persist() can only be used for "default" storage.

Hm, I actually think we don't need this method here, neither PersistOrigin(). The code can live in IsOriginPersistedOp and OriginPersistOp.
However you might want to create new methods for the "locked" code in Persist() and PersistOrigin()

	Tom Tung [:tt, :ttung] Assignee
	Comment 85 • 8 years ago

(In reply to Jan Varga [:janv] from comment #83)
Thanks for for the review and giving feedback! 

I'll address them, but I'm not sure few of them. I answer the comment and address my questions below inline.

> ::: dom/quota/QuotaManager.h
> @@ +201,5 @@
> >                          int64_t* aTimestamp,
> >                          nsACString& aSuffix,
> >                          nsACString& aGroup,
> > +                        nsACString& aOrigin,
> > +                        bool* aPersisted);
> 
> I don't see why this doesn't follow the order in the metadata file.
> aPersisted should go after aTimestamp.

I was thinking to add the newest argument into the last argument of function, but it's better to follow the order in the metadata file.
 
> ::: dom/quota/nsIQuotaManagerService.idl
> @@ +86,5 @@
> > +   *        available.
> > +   */
> > +  [must_use] nsIQuotaRequest
> > +  checkIsPersistentForPrincipal(in nsIPrincipal aPrincipal,
> > +                                in nsIQuotaCallback aCallback);
> 
> I'm not sure if we want to follow getUsageForPrincipal() or
> clearStoragesForPrincipal(). The former allows to pass a callback, the
> latter doesn't, but the callback can be easily set on the request which is
> returned by the method.
> My original intention was to use the same pattern as in IDB. A method
> returns a request and then you can set an event listener/callback. So I
> think we don't need the 2nd argument here. The same applies for persist()
> below.

For both two APIs persisted()/persist(), we need to provide the result when the promise is resolved. So, I'll try to return by the method and remove the 2nd argument.

(In reply to Jan Varga [:janv] from comment #84)
> ::: dom/quota/ActorsParent.cpp
> @@ +4813,5 @@
> > +  // Search the originInfo if it has already been created.
> > +  {
> > +    MutexAutoLock lock(mQuotaMutex);
> > +    GroupInfoPair* pair;
> > +    if (mGroupInfoPairs.Get(aGroup, &pair)) {
> 
> So, if there's no "pair" what then ?

I'm trying to early return if we've created the OriginInfo (which means we will have the groupInfoPair as well [1]). I was thinking that someone might call persisted()/persist() before the OriginInfo is created. So, what I do here, is to early return if we have originInfo. If we don't have one, we will search the metadata and create or update the correspond originInfo after searching metadata.

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

> @@ +4889,5 @@
> > +  }
> > +
> > +  // We assumed that directory (metadata) should be created.
> > +  if (created) {
> > +    return NS_ERROR_UNEXPECTED;
> 
> Pff, I don't understand the logic here at all.

I was trying to ensure that we have created the directory (calling EnsureOriginIsInitialized()) before calling PersistOrigin(). Therefore, the directory should be created.

> ::: dom/quota/QuotaManager.h
> @@ +348,5 @@
> >    GetGroupUsageAndLimit(const nsACString& aGroup,
> >                          UsageInfo* aUsageInfo);
> >  
> > +  // The function for checking the origin is persistent or not. It also
> > +  // initialize the static hashtable sPersistentBoxes if it is needed.
> 
> sPersistentBoxes doesn't exist anymore, right ?

Yep, it should be removed.

> @@ +350,5 @@
> >  
> > +  // The function for checking the origin is persistent or not. It also
> > +  // initialize the static hashtable sPersistentBoxes if it is needed.
> > +  nsresult
> > +  Persisted(PersistenceType aPersistenceType,
> 
> I think it's useless to have aPersistenceType here.
> nsIQuotaManagerService doesn't allow it either and actually
> persisted()/persist() can only be used for "default" storage.
> 
> Hm, I actually think we don't need this method here, neither
> PersistOrigin(). The code can live in IsOriginPersistedOp and
> OriginPersistOp.
> However you might want to create new methods for the "locked" code in
> Persist() and PersistOrigin()

Is okay for me to move PersistOrigin(), but I'm afraid we cannot move the Persist().
We use Persisted() to replace IsTreatedAsPersistent() [1] which is called in EnsureOriginIsInitialized(..). Once we do this, I may need to find something else to check whether is persisted or not (not only check originInfo but also directory).

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

	Jan Varga [:janv]
	Comment 86 • 8 years ago

(In reply to Tom Tung [:tt] from comment #85)
> For both two APIs persisted()/persist(), we need to provide the result when
> the promise is resolved. So, I'll try to return by the method and remove the
> 2nd argument.

Just to be sure, you can just remove the 2nd argument and that's it.

> > @@ +4813,5 @@
> > > +  // Search the originInfo if it has already been created.
> > > +  {
> > > +    MutexAutoLock lock(mQuotaMutex);
> > > +    GroupInfoPair* pair;
> > > +    if (mGroupInfoPairs.Get(aGroup, &pair)) {
> > 
> > So, if there's no "pair" what then ?
> 
> I'm trying to early return if we've created the OriginInfo (which means we
> will have the groupInfoPair as well [1]). I was thinking that someone might

It looks messy, for example in the persist() case, you call EnsureOriginIsInitialized() first, so the OriginInfo must have been created, you can add assertions for that if you want.

> call persisted()/persist() before the OriginInfo is created. So, what I do
> here, is to early return if we have originInfo. If we don't have one, we
> will search the metadata and create or update the correspond originInfo
> after searching metadata.

You should extract this block:
  {
    MutexAutoLock lock(mQuotaMutex);
    GroupInfoPair* pair;
    if (mGroupInfoPairs.Get(aGroup, &pair)) {
      RefPtr<GroupInfo> groupInfo = pair->LockedGetGroupInfo(aPersistenceType);
      if (!groupInfo) {
        return NS_ERROR_UNEXPECTED;
      }

      RefPtr<OriginInfo> originInfo = groupInfo->LockedGetOriginInfo(aOrigin);
      if (!originInfo) {
        return NS_ERROR_UNEXPECTED;
      }

      *aResult = originInfo->Persisted();
      return NS_OK;
    }
  }
into a new method and refactor the code around it to call the new method

> > @@ +4889,5 @@
> > > +  }
> > > +
> > > +  // We assumed that directory (metadata) should be created.
> > > +  if (created) {
> > > +    return NS_ERROR_UNEXPECTED;
> > 
> > Pff, I don't understand the logic here at all.
> 
> I was trying to ensure that we have created the directory (calling
> EnsureOriginIsInitialized()) before calling PersistOrigin(). Therefore, the
> directory should be created.

Ok, so here's your code and comments:
// Create directory and metadata if the directory is not existed.
bool created;
rv = EnsureDirectory(directory, &created);
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

// We assumed that directory (metadata) should be created.
if (created) {
  return NS_ERROR_UNEXPECTED;
}

So your first comments says: Create the directory and metadata if it doesn't exist ...
And then you have:
// We assumed that directory (metadata) should be created.
if (created) {
  return NS_ERROR_UNEXPECTED;
}

Does it makes sense to you ? Create the directory and then if it was created return an error ?

Actually, why would you want to call EnsureDirectory() ?
You can just add assertions that the directory do exist.

> 
> > @@ +350,5 @@
> > >  
> > > +  // The function for checking the origin is persistent or not. It also
> > > +  // initialize the static hashtable sPersistentBoxes if it is needed.
> > > +  nsresult
> > > +  Persisted(PersistenceType aPersistenceType,
> > 
> > I think it's useless to have aPersistenceType here.
> > nsIQuotaManagerService doesn't allow it either and actually
> > persisted()/persist() can only be used for "default" storage.
> > 
> > Hm, I actually think we don't need this method here, neither
> > PersistOrigin(). The code can live in IsOriginPersistedOp and
> > OriginPersistOp.
> > However you might want to create new methods for the "locked" code in
> > Persist() and PersistOrigin()
> 
> Is okay for me to move PersistOrigin(), but I'm afraid we cannot move the
> Persist().
> We use Persisted() to replace IsTreatedAsPersistent() [1] which is called in
> EnsureOriginIsInitialized(..). Once we do this, I may need to find something
> else to check whether is persisted or not (not only check originInfo but
> also directory).

So the main question is why you are trying to replace IsTreatedAsPersistent() which became |aPersistenceType == PERSISTENCE_TYPE_PERSISTENT| in the meantime with your new Persisted() method ?
Can you explain ?

	Tom Tung [:tt, :ttung] Assignee
	Comment 87 • 8 years ago

(In reply to Jan Varga [:janv] from comment #86)
> Just to be sure, you can just remove the 2nd argument and that's it.
Sure, thanks!
 
> > > @@ +4813,5 @@
> > I'm trying to early return if we've created the OriginInfo (which means we
> > will have the groupInfoPair as well [1]). I was thinking that someone might
> 
> It looks messy, for example in the persist() case, you call
> EnsureOriginIsInitialized() first, so the OriginInfo must have been created,
> you can add assertions for that if you want.

Oh, that's what I do, I'll change this to assertions. Sorry for not explaining that.

> > call persisted()/persist() before the OriginInfo is created. So, what I do
> > here, is to early return if we have originInfo. If we don't have one, we
> > will search the metadata and create or update the correspond originInfo
> > after searching metadata.
> 
> You should extract this block:
>   {
>     MutexAutoLock lock(mQuotaMutex);
>     GroupInfoPair* pair;
>     if (mGroupInfoPairs.Get(aGroup, &pair)) {
>       RefPtr<GroupInfo> groupInfo =
> pair->LockedGetGroupInfo(aPersistenceType);
>       if (!groupInfo) {
>         return NS_ERROR_UNEXPECTED;
>       }
> 
>       RefPtr<OriginInfo> originInfo =
> groupInfo->LockedGetOriginInfo(aOrigin);
>       if (!originInfo) {
>         return NS_ERROR_UNEXPECTED;
>       }
> 
>       *aResult = originInfo->Persisted();
>       return NS_OK;
>     }
>   }
> into a new method and refactor the code around it to call the new method

I see, thanks for telling me that!

> > > @@ +4889,5 @@
> > I was trying to ensure that we have created the directory (calling
> > EnsureOriginIsInitialized()) before calling PersistOrigin(). Therefore, the
> > directory should be created.
> 
> Ok, so here's your code and comments:
> // Create directory and metadata if the directory is not existed.
> bool created;
> rv = EnsureDirectory(directory, &created);
> if (NS_WARN_IF(NS_FAILED(rv))) {
>   return rv;
> }
> 
> // We assumed that directory (metadata) should be created.
> if (created) {
>   return NS_ERROR_UNEXPECTED;
> }
> 
> So your first comments says: Create the directory and metadata if it doesn't
> exist ...
> And then you have:
> // We assumed that directory (metadata) should be created.
> if (created) {
>   return NS_ERROR_UNEXPECTED;
> }
> 
> Does it makes sense to you ? Create the directory and then if it was created
> return an error ?

Oh, sorry for that. The logic is strange here... 

> Actually, why would you want to call EnsureDirectory() ?
> You can just add assertions that the directory do exist.

You are right, I will do that.

> > 
> > > @@ +350,5 @@
> > > >  
> > > > +  // The function for checking the origin is persistent or not. It also
> > > > +  // initialize the static hashtable sPersistentBoxes if it is needed.
> > > > +  nsresult
> > > > +  Persisted(PersistenceType aPersistenceType,
> > > 
> > > I think it's useless to have aPersistenceType here.
> > > nsIQuotaManagerService doesn't allow it either and actually
> > > persisted()/persist() can only be used for "default" storage.
> > > 
> > > Hm, I actually think we don't need this method here, neither
> > > PersistOrigin(). The code can live in IsOriginPersistedOp and
> > > OriginPersistOp.
> > > However you might want to create new methods for the "locked" code in
> > > Persist() and PersistOrigin()
> > 
> > Is okay for me to move PersistOrigin(), but I'm afraid we cannot move the
> > Persist().
> > We use Persisted() to replace IsTreatedAsPersistent() [1] which is called in
> > EnsureOriginIsInitialized(..). Once we do this, I may need to find something
> > else to check whether is persisted or not (not only check originInfo but
> > also directory).
> 
> So the main question is why you are trying to replace
> IsTreatedAsPersistent() which became |aPersistenceType ==
> PERSISTENCE_TYPE_PERSISTENT| in the meantime with your new Persisted()
> method ?
> Can you explain ?

Sorry, I'm not sure about what you mean here.

When the origin is persisted, we should not have an OriginInfo even if it's type is PERSISTENCE_TYPE_DEFAULT. The logic should be applied to all the IsTreatedAsPersistent(). If we only check |aPersistenceType == PERSISTENCE_TYPE_PERSISTENT|, we cannot distinguish from whether the origin is persisted or not. I mean "persisted" is like "isApp", but sometimes we need to check the directory.

	Jan Varga [:janv]
	Comment 88 • 8 years ago

(In reply to Tom Tung [:tt] from comment #87)
> > So the main question is why you are trying to replace
> > IsTreatedAsPersistent() which became |aPersistenceType ==
> > PERSISTENCE_TYPE_PERSISTENT| in the meantime with your new Persisted()
> > method ?
> > Can you explain ?
> 
> Sorry, I'm not sure about what you mean here.
> 
> When the origin is persisted, we should not have an OriginInfo even if it's
> type is PERSISTENCE_TYPE_DEFAULT. The logic should be applied to all the

Are you sure we don't have it ?
We call InitializeRepository() for origins in default and temporary storage if the initialization didn't happen yet. So if there's an origin directory for the given *persisted* origin, OriginInfo is created and mPersisted flag is set to true.

If the directory doesn't exist we just create a new metadata file with the flag set to false. That's it.

> IsTreatedAsPersistent(). If we only check |aPersistenceType ==
> PERSISTENCE_TYPE_PERSISTENT|, we cannot distinguish from whether the origin
> is persisted or not. I mean "persisted" is like "isApp", but sometimes we
> need to check the directory.

Again, why do we need to distinguish it here ?

	Jan Varga [:janv]
	Comment 89 • 8 years ago

Comment on attachment 8810336 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

More comments.

::: dom/quota/ActorsChild.cpp
@@ +268,5 @@
>      case RequestResponse::TClearOriginResponse:
>      case RequestResponse::TClearOriginsResponse:
>      case RequestResponse::TClearAllResponse:
>      case RequestResponse::TResetAllResponse:
> +    case RequestResponse::TPersistOriginResponse:

This will become TPersistResponse

@@ +273,4 @@
>        HandleResponse();
>        break;
>  
> +    case RequestResponse::TGetPersistedResponse:

This will become TPersistedResponse

::: dom/quota/ActorsParent.cpp
@@ +4813,5 @@
> +  // Search the originInfo if it has already been created.
> +  {
> +    MutexAutoLock lock(mQuotaMutex);
> +    GroupInfoPair* pair;
> +    if (mGroupInfoPairs.Get(aGroup, &pair)) {

So, if there's no "pair" what then ?

@@ +4889,5 @@
> +  }
> +
> +  // We assumed that directory (metadata) should be created.
> +  if (created) {
> +    return NS_ERROR_UNEXPECTED;

Pff, I don't understand the logic here at all.

::: dom/quota/PQuota.ipdl
@@ +48,5 @@
>  struct ResetAllParams
>  {
>  };
>  
> +struct GetPersistedParams

PersistedParams

@@ +53,5 @@
> +{
> +  PrincipalInfo principalInfo;
> +};
> +
> +struct PersistOriginParams

PersistParams

::: dom/quota/PQuotaRequest.ipdl
@@ +23,5 @@
>  struct ResetAllResponse
>  {
>  };
>  
> +struct GetPersistedResponse

PersistedResponse

@@ +25,5 @@
>  };
>  
> +struct GetPersistedResponse
> +{
> +  bool isPersistent;

just "persisted"

@@ +28,5 @@
> +{
> +  bool isPersistent;
> +};
> +
> +struct PersistOriginResponse

PersistResponse

::: dom/quota/QuotaManager.h
@@ +210,5 @@
>                                     int64_t* aTimestamp,
>                                     nsACString& aSuffix,
>                                     nsACString& aGroup,
> +                                   nsACString& aOrigin,
> +                                   bool* aPersisted);

Fix the order here too

@@ +221,5 @@
>                                     bool aPersistent,
>                                     int64_t* aTimestamp);
>  
> +  nsresult
> +  PersistDirectoryMetadata2(nsIFile* aDirectory);

I don't think we need this method here, there's only one caller anyway.
The code can live in OriginPersistOp.

@@ +348,5 @@
>    GetGroupUsageAndLimit(const nsACString& aGroup,
>                          UsageInfo* aUsageInfo);
>  
> +  // The function for checking the origin is persistent or not. It also
> +  // initialize the static hashtable sPersistentBoxes if it is needed.

sPersistentBoxes doesn't exist anymore, right ?

@@ +350,5 @@
>  
> +  // The function for checking the origin is persistent or not. It also
> +  // initialize the static hashtable sPersistentBoxes if it is needed.
> +  nsresult
> +  Persisted(PersistenceType aPersistenceType,

I think it's useless to have aPersistenceType here.
nsIQuotaManagerService doesn't allow it either and actually persisted()/persist() can only be used for "default" storage.

Hm, I actually think we don't need this method here, neither PersistOrigin(). The code can live in IsOriginPersistedOp and OriginPersistOp.
However you might want to create new methods for the "locked" code in Persist() and PersistOrigin()

::: dom/quota/QuotaManagerService.cpp
@@ +60,5 @@
>    gTestingMode = Preferences::GetBool(aPrefName);
>  }
>  
> +nsresult
> +GetPrincipalInfoFromPrincipal(nsIPrincipal* aPrincipal,

What about CheckedPrincipalToPrincipalInfo ?

@@ +73,5 @@
> +
> +  if (aPrincipalInfo.type() != PrincipalInfo::TContentPrincipalInfo &&
> +      aPrincipalInfo.type() != PrincipalInfo::TSystemPrincipalInfo) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

Add an empty line here.

::: dom/quota/QuotaRequests.cpp
@@ +251,5 @@
> +  MOZ_ASSERT(aPrincipal);
> +}
> +
> +Request::Request(nsIPrincipal* aPrincipal,
> +                 nsIQuotaCallback* aCallback)

not needed

@@ +271,2 @@
>  {
>    AssertIsOnOwningThread();

add MOZ_ASSERT(aResult) here

@@ +282,5 @@
> +NS_IMETHODIMP
> +Request::GetResult(nsIVariant** aResult)
> +{
> +  AssertIsOnOwningThread();
> +  NS_ENSURE_ARG_POINTER(aResult);

just MOZ_ASSERT(aResult)

@@ +288,5 @@
> +  if (!mHaveResultOrErrorCode) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  NS_IF_ADDREF(*aResult = mResult);

you can chage this to:
MOZ_ASSERT(mResult);
NS_ADDREF(*aResult = mResult);

::: dom/quota/QuotaRequests.h
@@ +124,5 @@
>    Request();
>  
>    explicit Request(nsIPrincipal* aPrincipal);
>  
> +  Request(nsIPrincipal* aPrincipal, nsIQuotaCallback* aCallback);

This won't be necessary one you remove aCallback argument in nsIQuotaManagerService interface.

	Jan Varga [:janv]
	Comment 90 • 8 years ago

Sorry, bugzilla somehow "restored" comments that have been already sent.
Anyway, there are some new comments starting with "What about CheckedPrincipalToPrincipalInfo ?"

	Jan Varga [:janv]
	Comment 91 • 8 years ago

(In reply to Tom Tung [:tt] from comment #87)
> When the origin is persisted, we should not have an OriginInfo even if it's
> type is PERSISTENCE_TYPE_DEFAULT. The logic should be applied to all the
> IsTreatedAsPersistent(). If we only check |aPersistenceType ==
> PERSISTENCE_TYPE_PERSISTENT|, we cannot distinguish from whether the origin
> is persisted or not. I mean "persisted" is like "isApp", but sometimes we
> need to check the directory.

Yes, persisted is like isApp, but the goal of your P0 patch is to create OriginInfo objects for all origins, even for those with isApp/persisted set to true, no ?

	Jan Varga [:janv]
	Comment 92 • 8 years ago

Comment on attachment 8810336 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

Ok, I think I'm done for now. I'll take a look once you attach a new version of the patch.

::: dom/quota/ActorsParent.cpp
@@ +1124,5 @@
>    virtual void
>    GetResponse(RequestResponse& aResponse) override;
>  };
>  
> +class IsOriginPersistedOp

Just PersistedOp ?

@@ +1138,5 @@
> +public:
> +  explicit IsOriginPersistedOp(const RequestParams& aParams);
> +
> +  virtual bool
> +  Init(Quota* aQuota) override;

I think we don't put "virtual" before the return type anymore if the method is marked as "override".

@@ +1153,5 @@
> +  DoDirectoryWork(QuotaManager* aQuotaManager) override;
> +
> +  virtual void
> +  GetResponse(RequestResponse& aResponse) override;
> +

No need for this empty line.

@@ +1917,5 @@
>  
>  nsresult
>  CreateDirectoryMetadata2(nsIFile* aDirectory, int64_t aTimestamp,
>                           const nsACString& aSuffix, const nsACString& aGroup,
> +                         const nsACString& aOrigin, bool aPersisted)

aPersisted comes after aTimestamp

	Jan Varga [:janv]
	Comment 93 • 8 years ago

Comment on attachment 8809773 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist().

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

I quickly looked over these tests and my first impression is that it's way too complicated.
It might help if you describe each test a bit.

::: dom/indexedDB/test/mochitest.ini
@@ +82,5 @@
>    unit/test_open_objectStore.js
>    unit/test_optionalArguments.js
>    unit/test_overlapping_transactions.js
>    unit/test_persistenceType.js
> +  unit/test_persistent_storage.js

I believe you just forgot to remove this line.

	Tom Tung [:tt, :ttung] Assignee
	Comment 94 • 8 years ago

(In reply to Jan Varga [:janv] from comment #88)
> Are you sure we don't have it ?
 
I think I was mixing it up with my previous proposal... Really sorry for that :(

> We call InitializeRepository() for origins in default and temporary storage
> if the initialization didn't happen yet. So if there's an origin directory
> for the given *persisted* origin, OriginInfo is created and mPersisted flag
> is set to true.
> 
> If the directory doesn't exist we just create a new metadata file with the
> flag set to false. That's it.

Yeah, you are right. So, we do create it even its "persisted" is set to be true, and we'll create a new metadata with setting "persisted" to be false if we don't have one.

> > IsTreatedAsPersistent(). If we only check |aPersistenceType ==
> > PERSISTENCE_TYPE_PERSISTENT|, we cannot distinguish from whether the origin
> > is persisted or not. I mean "persisted" is like "isApp", but sometimes we
> > need to check the directory.
> 
> Again, why do we need to distinguish it here ?

Sorry again, be the same as above, I mix it up with previous proposal which make "persisted" not have a originInfo. I will remove this.

(In reply to Jan Varga [:janv] from comment #91)
> Yes, persisted is like isApp, but the goal of your P0 patch is to create
> OriginInfo objects for all origins, even for those with isApp/persisted set
> to true, no ?

Yes, you are right. Sorry for confusing...

(In reply to Jan Varga [:janv] from comment #93)
> Comment on attachment 8809773 [details] [diff] [review]
> [Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist().
> 
> Review of attachment 8809773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I quickly looked over these tests and my first impression is that it's way
> too complicated.
> It might help if you describe each test a bit.

I'll rewrite each of them to make they easier to understand.

> ::: dom/indexedDB/test/mochitest.ini
> @@ +82,5 @@
> >    unit/test_open_objectStore.js
> >    unit/test_optionalArguments.js
> >    unit/test_overlapping_transactions.js
> >    unit/test_persistenceType.js
> > +  unit/test_persistent_storage.js
> 
> I believe you just forgot to remove this line.

Yep...

	Jan Varga [:janv]
	Comment 95 • 8 years ago

(In reply to Tom Tung [:tt] from comment #94)
> > I quickly looked over these tests and my first impression is that it's way
> > too complicated.
> > It might help if you describe each test a bit.
> 
> I'll rewrite each of them to make they easier to understand.

It might also help if you say which tests you used as a base/template for your new tests.

	Tom Tung [:tt, :ttung] Assignee
	Comment 96 • 8 years ago

Hi Jan,

Thanks for giving so much feedback, I addressed them in this patch. Could you help me to review it again? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 97 • 8 years ago

interdiff patch for P1.

	Tom Tung [:tt, :ttung] Assignee
	Comment 98 • 8 years ago

Hi Jan,

Sorry for implementing testcase too complex but I try to do the same procedures to the persisted origins to ensure they have hit different events as expected. I test the persisted origins under pressure of group limit (test_groupLimit.js) and global limit (test_persistent_storage). Moreover, I test the basic function of persist()/persisted() APIs as well (test_persitent_basic.js).

  In this patch, I implement three testcases and they are listed as following:

test_persistent_basic.js:

  In this testcase, I intended to test basic persist()/persisted() functionalities.
  - Test1: Persist an origin before storing anything and check the persisted() before and after persist().
  - Test2: Store an object into the origin and check if it is still persisted
  - Test3: Clean the origin and check if it is still persisted as well.

test_persistent_storage.js:

  This test is based on [1] and it is mainly test the global limit and eviction. 
  - What I'm going to do here is to keep create database and make it hit the global limit . (Stage 1)
  - Then, I close all of database, persist() them and then I create one more. It should be hit the onerror event since they shared the same global limit. However, there are no any evictions happened since they are all persisted. (Stage 2)
  - Next, cut the limit into half but the QuotaManger should not evict any of storage since they all are persisted. (Stage 3)

[1] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_temporary_storage.js#139

test_groupLimit.js:

  I write this test based on [2]. I create this testcase to make sure QuotaManager won't delete the persisted origin and the group limit should not bound persisted origin. 

  I only create a database and keep filling an object into it until it cannot be written which means reaching group limit. Then I persist() it and write one more object into it again. And, I get oncomplete event as expected.

[2] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_quotaExceeded_recovery.js

	Jan Varga [:janv]
	Comment 99 • 8 years ago

Comment on attachment 8826914 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v2-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

::: dom/quota/ActorsParent.cpp
@@ +4753,5 @@
>    }
>  }
>  
> +nsresult
> +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup,

A typo in the method name.

@@ +4755,5 @@
>  
> +nsresult
> +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup,
> +                                      const nsACString& aOrigin)
> +{

Which thread this method can be called on ?

@@ +4756,5 @@
> +nsresult
> +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup,
> +                                      const nsACString& aOrigin)
> +{
> +  MOZ_ASSERT(mTemporaryStorageInitialized);

Is it safe to access this member variable on this thread ?

@@ +4758,5 @@
> +                                      const nsACString& aOrigin)
> +{
> +  MOZ_ASSERT(mTemporaryStorageInitialized);
> +
> +  MutexAutoLock lock(mQuotaMutex);

So, if you look at all Locked* methods, they don't acquire the lock, they just check that it's been already acquired. Either removed the Locked prefix, or rework the method body.

@@ +4762,5 @@
> +  MutexAutoLock lock(mQuotaMutex);
> +
> +  GroupInfoPair* pair;
> +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> +    return NS_ERROR_UNEXPECTED;

I don't like how you return NS_ERROR_UNEXPECTED here. We need to improve this in next iteration.

@@ +4788,5 @@
> +nsresult
> +QuotaManager::LoackedGetPersistedForOrigin(const nsACString& aGroup,
> +                                           const nsACString& aOrigin,
> +                                           bool* aResult)
> +{

The same comments apply here.

@@ +6656,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  // Figure out which origin

This sounds like an unfinished sentence.

@@ +6677,5 @@
> +                 js::ProfileEntry::Category::OTHER);
> +
> +  // Ensure origin is initialized first.
> +  nsCOMPtr<nsIFile> directory;
> +  nsresult rv = aQuotaManager->EnsureOriginIsInitialized(

So now persisted() creates the directory and metadata if the directory doesn't exist ?
Is it what you want ?

@@ +6734,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  rv = aQuotaManager->LoackedPersistForOrigin(mGroup, mOrigin);

This method should be called after we successfully updated metadata on disk.
Ideally the method would be infallible, so it doesn't return any errors (See my comment about NS_ERROR_UNEXPECTED.
The problem is that you update objects in memory first and then something fails during on disk update. The memory objects should be reverted, but that's quite complicated. It would be better to execute stuff that can fail first and then call infallible methods.

@@ +7875,5 @@
>          return rv;
>        }
>      }
>  
> +    // We don't have any approach to restore aPersisted, so assume it is false.

... , so reset it to false.

::: dom/quota/nsIQuotaManagerService.idl
@@ +75,5 @@
>    [must_use] nsIQuotaRequest
>    reset();
> +
> +  /**
> +   * Check the storages stored in given origin is persisted or not.

Check if given origin is persisted.

@@ +79,5 @@
> +   * Check the storages stored in given origin is persisted or not.
> +   *
> +   * @param aPrincipal
> +   *        A principal for the origin which we want to check is perisitent or
> +   *        not.

A principal for the origin which we want to check.

	Tom Tung [:tt, :ttung] Assignee
	Comment 100 • 8 years ago

(In reply to Jan Varga [:janv] from comment #99)
Thanks for giving feedback so quickly!

> Comment on attachment 8826914 [details] [diff] [review]
> [Rebased Patch] Bug-1298329-P1-v2-Implement persist/persisted in
> QuotaManager/QuotaManagerService.
> 
> Review of attachment 8826914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +4755,5 @@
> >  
> > +nsresult
> > +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup,
> > +                                      const nsACString& aOrigin)
> > +{
> 
> Which thread this method can be called on ?

IO thread I'll add assertion for it.

> @@ +4756,5 @@
> > +nsresult
> > +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup,
> > +                                      const nsACString& aOrigin)
> > +{
> > +  MOZ_ASSERT(mTemporaryStorageInitialized);
> 
> Is it safe to access this member variable on this thread ?

It's in IO thread so it should be safe.

> @@ +4758,5 @@
> > +                                      const nsACString& aOrigin)
> > +{
> > +  MOZ_ASSERT(mTemporaryStorageInitialized);
> > +
> > +  MutexAutoLock lock(mQuotaMutex);
> 
> So, if you look at all Locked* methods, they don't acquire the lock, they
> just check that it's been already acquired. Either removed the Locked
> prefix, or rework the method body.

Sure, I will remove Lock* prefix!

> @@ +4762,5 @@
> > +  MutexAutoLock lock(mQuotaMutex);
> > +
> > +  GroupInfoPair* pair;
> > +  if (!mGroupInfoPairs.Get(aGroup, &pair)) {
> > +    return NS_ERROR_UNEXPECTED;
> 
> I don't like how you return NS_ERROR_UNEXPECTED here. We need to improve
> this in next iteration.

Sure, I'll try to improve it.

> @@ +6656,5 @@
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return rv;
> > +  }
> > +
> > +  // Figure out which origin
> 
> This sounds like an unfinished sentence.

I thought it might be clearer... I'll recover it to "Figure out which origin we're dealing with".

> @@ +6677,5 @@
> > +                 js::ProfileEntry::Category::OTHER);
> > +
> > +  // Ensure origin is initialized first.
> > +  nsCOMPtr<nsIFile> directory;
> > +  nsresult rv = aQuotaManager->EnsureOriginIsInitialized(
> 
> So now persisted() creates the directory and metadata if the directory
> doesn't exist ?
> Is it what you want ?

Sorry, I forget it. I will rewrite it.

> @@ +6734,5 @@
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return rv;
> > +  }
> > +
> > +  rv = aQuotaManager->LoackedPersistForOrigin(mGroup, mOrigin);
> 
> This method should be called after we successfully updated metadata on disk.
> Ideally the method would be infallible, so it doesn't return any errors (See
> my comment about NS_ERROR_UNEXPECTED.

I think this method is fallable, and thus the returning promise can be either true(persisted) or false(best-effort).
[1] https://storage.spec.whatwg.org/#dfnReturnLink-0

> The problem is that you update objects in memory first and then something
> fails during on disk update. The memory objects should be reverted, but
> that's quite complicated. It would be better to execute stuff that can fail
> first and then call infallible methods.

Sorry, I'm not sure how to call infallible methods here. Could you explain more? Thanks! ><

	Tom Tung [:tt, :ttung] Assignee
	Comment 101 • 8 years ago

(In reply to Tom Tung [:tt] from comment #100)
> > This method should be called after we successfully updated metadata on disk.
> > Ideally the method would be infallible, so it doesn't return any errors (See
> > my comment about NS_ERROR_UNEXPECTED.
> 
> I think this method is fallable, and thus the returning promise can be
> either true(persisted) or false(best-effort).
> [1] https://storage.spec.whatwg.org/#dfnReturnLink-0

I just realize that this happen because user can deny the prompt. Sorry for that!

	Jan Varga [:janv]
	Comment 102 • 8 years ago

(In reply to Tom Tung [:tt] from comment #100)
> > @@ +6734,5 @@
> > > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > > +    return rv;
> > > +  }
> > > +
> > > +  rv = aQuotaManager->LoackedPersistForOrigin(mGroup, mOrigin);
> > 
> > This method should be called after we successfully updated metadata on disk.
> > Ideally the method would be infallible, so it doesn't return any errors (See
> > my comment about NS_ERROR_UNEXPECTED.
> 
> I think this method is fallable, and thus the returning promise can be
> either true(persisted) or false(best-effort).
> [1] https://storage.spec.whatwg.org/#dfnReturnLink-0

I meant that LoackedPersistForOrigin() should be infallible.

> 
> > The problem is that you update objects in memory first and then something
> > fails during on disk update. The memory objects should be reverted, but
> > that's quite complicated. It would be better to execute stuff that can fail
> > first and then call infallible methods.
> 
> Sorry, I'm not sure how to call infallible methods here. Could you explain
> more? Thanks! ><

infallible == returning "void" in this context

	Jan Varga [:janv]
	Comment 103 • 8 years ago

(In reply to Tom Tung [:tt] from comment #101)
> (In reply to Tom Tung [:tt] from comment #100)
> > > This method should be called after we successfully updated metadata on disk.
> > > Ideally the method would be infallible, so it doesn't return any errors (See
> > > my comment about NS_ERROR_UNEXPECTED.
> > 
> > I think this method is fallable, and thus the returning promise can be
> > either true(persisted) or false(best-effort).
> > [1] https://storage.spec.whatwg.org/#dfnReturnLink-0
> 
> I just realize that this happen because user can deny the prompt. Sorry for
> that!

No, we don't handle the prompt at this level at all.

	Jan Varga [:janv]
	Comment 104 • 8 years ago

Comment on attachment 8826914 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v2-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

::: dom/quota/ActorsParent.cpp
@@ +1137,5 @@
> +protected:
> +  const RequestParams mParams;
> +  nsCString mSuffix;
> +  nsCString mGroup;
> +  nsCString mOrigin;

I overlooked this in my previous review, why did you decide to ignore mOriginScope and use your own mOrigin ?
Also what about mPersistenceType, don't you need to initialized it ?

@@ +6630,5 @@
> +  AssertIsOnOwningThread();
> +  MOZ_ASSERT(aQuota);
> +
> +  mNeedsMainThreadInit = true;
> +  mNeedsQuotaManagerInit = true;

mNeedsQuotaManagerInit is already set in QuotaRequestBase

@@ +6642,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(GetState() == State_Initializing);
> +  MOZ_ASSERT(mNeedsMainThreadInit);
> +  MOZ_ASSERT(mParams.type() == RequestParams::TPersistedParams ||
> +             mParams.type() == RequestParams::TPersistParams);

I don't think this extra assertion is needed.

@@ +6703,5 @@
> +{
> +  AssertIsOnOwningThread();
> +
> +  PersistedResponse persistedResponse;
> +  persistedResponse.persisted() = mPersisted;

Nit: add an empty line here

@@ +6751,5 @@
> +  }
> +
> +  MOZ_ASSERT(stream);
> +
> +  int64_t timestamp = PR_Now();

You can pass PR_Now() directly to Write64().

@@ +7875,5 @@
>          return rv;
>        }
>      }
>  
> +    // We don't have any approach to restore aPersisted, so assume it is false.

Shouldn't this comment rather go to RestoreDirectoryMetadata2Helper ?

::: dom/quota/QuotaManager.h
@@ +346,5 @@
>                          UsageInfo* aUsageInfo);
>  
> +  nsresult
> +  LoackedPersistForOrigin(const nsACString& aGroup,
> +                          const nsACString& aOrigin);

What about just PersistOrigin() ? and move the declaration and implementation after GetQuotaObject().

@@ +351,5 @@
> +
> +  nsresult
> +  LoackedGetPersistedForOrigin(const nsACString& aGroup,
> +                               const nsACString& aOrigin,
> +                               bool* aResult);

What about just OriginPersisted() ? and move the declaration and implementation after GetQuotaObject().

Also, OriginPersiste() should go before PersistOrigin().

::: dom/quota/QuotaManagerService.cpp
@@ +655,5 @@
> +QuotaManagerService::Persisted(nsIPrincipal* aPrincipal,
> +                               nsIQuotaRequest** _retval)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aPrincipal);

I know it's not in other methods, but I think we should assert _retval too.

@@ +683,5 @@
> +QuotaManagerService::Persist(nsIPrincipal* aPrincipal,
> +                             nsIQuotaRequest** _retval)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aPrincipal);

here too

::: dom/quota/QuotaRequests.cpp
@@ +279,5 @@
> +  if (!mHaveResultOrErrorCode) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  MOZ_ASSERT(mResult);

Nit: add an empty line here

::: dom/quota/nsIQuotaManagerService.idl
@@ +6,5 @@
>  
>  #include "nsISupports.idl"
>  
>  interface nsIPrincipal;
> +interface nsIQuotaCallback;

This can now go away.

	Tom Tung [:tt, :ttung] Assignee
	Comment 105 • 8 years ago

(In reply to Jan Varga [:janv] from comment #104)

Thanks for the review and giving feedback rapidly even in holiday!! 
Sorry for so many problems in my patch. I'll try harder to reduce them.

> ::: dom/quota/ActorsParent.cpp
> @@ +1137,5 @@
> > +protected:
> > +  const RequestParams mParams;
> > +  nsCString mSuffix;
> > +  nsCString mGroup;
> > +  nsCString mOrigin;
> 
> I overlooked this in my previous review, why did you decide to ignore
> mOriginScope and use your own mOrigin ?

I overlooked this member variable. I'll using mOriginScope rather using mOrigin.

> Also what about mPersistenceType, don't you need to initialized it ?

It should be initialized when we call ctor [1] since class PersistedOp is inherited from QuotaRequestBase. 

Besides, should I take care of its value? I mean that I thought we only care about PERSISTENCE_TYPE_DEFAULT. I might be wrong so please correct me if I misunderstand anything.

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

> @@ +6751,5 @@
> > +  }
> > +
> > +  MOZ_ASSERT(stream);
> > +
> > +  int64_t timestamp = PR_Now();
> 
> You can pass PR_Now() directly to Write64().

I saw we did that in [2]. Should I correct as well?

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

> @@ +7875,5 @@
> >          return rv;
> >        }
> >      }
> >  
> > +    // We don't have any approach to restore aPersisted, so assume it is false.
> 
> Shouldn't this comment rather go to RestoreDirectoryMetadata2Helper ?

I'll remove this comment if it confused.

> ::: dom/quota/QuotaManagerService.cpp
> @@ +655,5 @@
> > +QuotaManagerService::Persisted(nsIPrincipal* aPrincipal,
> > +                               nsIQuotaRequest** _retval)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aPrincipal);
> 
> I know it's not in other methods, but I think we should assert _retval too.

I'm thinking to add MOZ_ASSERT(_retval) to the other existing methods in QuotaManagerService.cpp. If you think it is appropriate, I'll do it in next patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 106 • 8 years ago

Addressed comment except few of them that are uncertain for me. And, I listed these in comment #105

	Jan Varga [:janv]
	Comment 107 • 8 years ago

(In reply to Tom Tung [:tt] from comment #105)
> > I overlooked this in my previous review, why did you decide to ignore
> > mOriginScope and use your own mOrigin ?
> 
> I overlooked this member variable. I'll using mOriginScope rather using
> mOrigin.

So you know why it is important ?

> 
> > Also what about mPersistenceType, don't you need to initialized it ?
> 
> It should be initialized when we call ctor [1] since class PersistedOp is
> inherited from QuotaRequestBase. 
> 
> Besides, should I take care of its value? I mean that I thought we only care
> about PERSISTENCE_TYPE_DEFAULT. I might be wrong so please correct me if I
> misunderstand anything.
> 

You should really look at the entire hierarchy of these classes.
With your current patch, mPersistenceType is set to null and mOriginScope is set to null too.
So it means that OpenDirectoryInternal() will lock entire <profile>/storage for no reason.
We need to lock just one origin that is being persisted.

> > You can pass PR_Now() directly to Write64().
> 
> I saw we did that in [2]. Should I correct as well?
> 

The code will always contain bugs, sub-optimal stuff, etc.
If you copy some pattern/code from existing code, it doesn't mean the reviewer will be ok with it, even if he/she has written it.
We don't have to fix the source of the pattern right now. We just don't want to spread it in this patch.


> > Shouldn't this comment rather go to RestoreDirectoryMetadata2Helper ?
> 
> I'll remove this comment if it confused.

Just move it to the other class.

> > I know it's not in other methods, but I think we should assert _retval too.
> 
> I'm thinking to add MOZ_ASSERT(_retval) to the other existing methods in
> QuotaManagerService.cpp. If you think it is appropriate, I'll do it in next
> patch.

Again, we don't have to fix other places in this patch.

	Jan Varga [:janv]
	Comment 108 • 8 years ago

(In reply to Tom Tung [:tt] from comment #105)
> > @@ +6751,5 @@
> > > +  }
> > > +
> > > +  MOZ_ASSERT(stream);
> > > +
> > > +  int64_t timestamp = PR_Now();
> > 
> > You can pass PR_Now() directly to Write64().
> 
> I saw we did that in [2]. Should I correct as well?
> 
> [2]
> http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#3328.

Actually, if you look at that code closely, the variable is passed to a method first and then to a constructor. So we would have to call PR_Now() twice if there was no local "timestamp" variable.
This is elementary stuff.

	Tom Tung [:tt, :ttung] Assignee
	Comment 109 • 8 years ago

(In reply to Jan Varga [:janv] from comment #107)
> > I overlooked this member variable. I'll using mOriginScope rather using
> > mOrigin.
> 
> So you know why it is important ?
> 
> You should really look at the entire hierarchy of these classes.
> With your current patch, mPersistenceType is set to null and mOriginScope is
> set to null too.
> So it means that OpenDirectoryInternal() will lock entire <profile>/storage
> for no reason.
> We need to lock just one origin that is being persisted.
> 

Thanks for telling that! I should notice that they are used to pass into OpenDirectoryInternal(). :(
I will initialize mPersistenceType in PersistedOp::Init() and set mOriginScope in PersistedOp::DoInitOnMainThread().

> The code will always contain bugs, sub-optimal stuff, etc.
> If you copy some pattern/code from existing code, it doesn't mean the
> reviewer will be ok with it, even if he/she has written it.
> We don't have to fix the source of the pattern right now. We just don't want
> to spread it in this patch.

Oh, I see. Thanks for telling me that!
I was thinking to fix them in this patch since they are simple, but it doesn't in the scope this bug.

	Tom Tung [:tt, :ttung] Assignee
	Comment 110 • 8 years ago

Hi Jan,

In this patch, I addressed the comment.

Besides, I change return type for PersistOrigin() from nsresult to void.
The reason is that I think even if we cannot get originInfo, we can correct the "persisted" metadata and the originInfo will be initialize with correct value from correct metadata in next time. 

Moreover, I change return type for OriginPersisted() from nsresult to bool. The reason is that I just need to check the metadata once we cannot get originInfo.

Could you help me to review this patch? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 111 • 8 years ago

interdiff patch for P1

	Jan Varga [:janv]
	Comment 112 • 8 years ago

(In reply to Tom Tung [:tt] from comment #110)
> Besides, I change return type for PersistOrigin() from nsresult to void.
> The reason is that I think even if we cannot get originInfo, we can correct
> the "persisted" metadata and the originInfo will be initialize with correct
> value from correct metadata in next time. 

EnsureOriginIsInitialized() is called first which is expected to create OriginInfo, right ?
So it would be an error if originIfo didn't exist after that call.

However, I just realized that EnsureOriginIsInitialized() is not necessary. It's sufficient that EnsureStorageIsInitialized() is called in OriginOperationBase::DirectoryWork().

> 
> Moreover, I change return type for OriginPersisted() from nsresult to bool.
> The reason is that I just need to check the metadata once we cannot get
> originInfo.

It's not bad, but I think we can do better.

I'll attach a patch.

	Jan Varga [:janv]
	Comment 113 • 8 years ago

Comment on attachment 8817250 [details] [diff] [review]
[Rebased Patch] Bug-1296591-P0-v5- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv.

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

::: dom/quota/ActorsParent.cpp
@@ +5156,5 @@
> +void
> +OriginInfo::LockedPersist()
> +{
> +  AssertCurrentThreadOwnsQuotaMutex();
> +  MOZ_ASSERT(mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT);

We should also assert here that mPersisted is false.

	Jan Varga [:janv]
	Comment 114 • 8 years ago

Comment on attachment 8817250 [details] [diff] [review]
[Rebased Patch] Bug-1296591-P0-v5- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv.

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

::: dom/quota/ActorsParent.cpp
@@ +456,5 @@
>      return mAccessTime;
>    }
>  
> +  bool
> +  Persisted() const

This should be LockedPersisted(). Let's fix AccessTime() elsewhere.

	Jan Varga [:janv]
	Comment 115 • 8 years ago

Comment on attachment 8827091 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v3.1-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

::: dom/quota/ActorsParent.cpp
@@ +1135,5 @@
> +  bool mPersisted;
> +
> +protected:
> +  const RequestParams mParams;
> +  nsCString mSuffix;

mSuffix won't be needed once we remove EnsureOriginIsInitialized() call.

@@ +3451,5 @@
>    return GetQuotaObject(aPersistenceType, aGroup, aOrigin, file);
>  }
>  
> +bool
> +QuotaManager::OriginPersisted(const nsACString& aGroup,

This method along with PersistOrigin contains very similar code. The code should be shared somehow.

@@ +6629,5 @@
> +  MOZ_ASSERT(aQuota);
> +
> +  mNeedsMainThreadInit = true;
> +
> +  mPersistenceType.SetValue(PERSISTENCE_TYPE_DEFAULT);

I would put this above |mNeedsMainThreadInit = true;|

@@ +6670,5 @@
> +nsresult
> +PersistedOp::DoDirectoryWork(QuotaManager* aQuotaManager)
> +{
> +  AssertIsOnIOThread();
> +  MOZ_ASSERT(mParams.type() == RequestParams::TPersistedParams);

I don't think we have to assert this, mParams.type() is not accessed in this method at all.

@@ +6671,5 @@
> +PersistedOp::DoDirectoryWork(QuotaManager* aQuotaManager)
> +{
> +  AssertIsOnIOThread();
> +  MOZ_ASSERT(mParams.type() == RequestParams::TPersistedParams);
> +  MOZ_ASSERT(mPersistenceType.Value() == PERSISTENCE_TYPE_DEFAULT);

But I do think that we need assert !mPersistebceType.IsNull().

We should also assert mOriginScope.IsOrigin()

@@ +6677,5 @@
> +  PROFILER_LABEL("Quota", "PersistedOp::DoDirectoryWork",
> +                 js::ProfileEntry::Category::OTHER);
> +
> +  // Get "persisted" from directory if we cannot find it in originInfo.
> +  if (!aQuotaManager->OriginPersisted(mGroup,

I understand your intention here, but this doesn't read well.
If origin is not persisted, then ...
but we want:
If persisted flag/info is not cached yet, then ...

@@ +6695,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    if(!exists) {

What about switching these branches ?
You don't have to negate "exists" then.

@@ +6696,5 @@
> +      return rv;
> +    }
> +
> +    if(!exists) {
> +      // Metadata have not been created yet.

has

@@ +6703,5 @@
> +      // Get persisted flag
> +      int64_t dummyTimestamp;
> +      nsCString dummySuffix;
> +      nsCString dummyGroup;
> +      nsCString dummyOrigin;

Too many dummies here. We should simplify this somehow.

@@ +6704,5 @@
> +      int64_t dummyTimestamp;
> +      nsCString dummySuffix;
> +      nsCString dummyGroup;
> +      nsCString dummyOrigin;
> +      rv = aQuotaManager->GetDirectoryMetadata2(directory, &dummyTimestamp,

So, what if the metadata file is missing or broken ?
The whole process of getting the persisted flag will fail, right ?
I think we can happily use the *WithRestore method here.

@@ +6755,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  aQuotaManager->PersistOrigin(mGroup, mOriginScope.GetOrigin());

Like I said in one of my previous comments, calling PersistOrigin() first and then updating stuff on disk can lead to inconsistency.
If we fail somewhere in the code that updates stuff on disk, for example GetBinaryOutputStream() fails, then you return from this method - PersistOp::DoDirectoryWork(), but PersistOrigin() already updated OriginInfo.
You could add code that reverts changes done by PersistOrigin() if GetBinaryOutputStream() failed, but it's just easier to execute stuff that can fail and call PersistOrigin() after that.

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

, so reset it *to* false.

	Jan Varga [:janv]
	Comment 116 • 8 years ago

Here are my changes. It builds, but I haven't done any testing, you need to do it yourself.

	Tom Tung [:tt, :ttung] Assignee
	Comment 117 • 8 years ago

(In reply to Jan Varga [:janv] from comment #116)
> Created attachment 8827207 [details] [diff] [review]
> patch
> 
> Here are my changes. It builds, but I haven't done any testing, you need to
> do it yourself.
Hi Jan,

I think we maybe still need EnsureOriginIsInitialize(..) to create directory for metadata if the metadata's directory has not been created yet. 

I tried to persist() origin before storing an object into it and then I got fail while setting stream [1]. The reason is the directory is not exist and make the stream null [2]. I guess the reason is because EnsureStorageIsInitialized() won't create the directory for origin if it is the first time to access. But, I'm not sure about that...

If I'm correct, I have two options for this problem: 
1. Use EnsureOriginIsInitialize() and don't reset mNeedsQuotaManagerInit so that we can avoid calling EnsureStorageIsInitialized() twice. 
2. Calling EnsureDirectory(), CreateDirectoryMetadata() and CreateDirectoryMetadata2() in  PersistOp::DoDirectoryWork() if the directory has not been created yet.

Could you give me some suggests for it and correct me if I'm wrong? Thanks!

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

	Jan Varga [:janv]
	Comment 118 • 8 years ago

(In reply to Tom Tung [:tt] from comment #117)
> If I'm correct, I have two options for this problem: 
> 1. Use EnsureOriginIsInitialize() and don't reset mNeedsQuotaManagerInit so
> that we can avoid calling EnsureStorageIsInitialized() twice. 

I'm not worried about calling it twice, actually that's not the case. if mNeedsQuotaManagerInit is set then EnsureStorageIsInitialized() is called not EnsureOriginIsInitialized().

The problem is that EnsureOriginIsInitialized() will call InitializedRepository() and we don't need that in this case. We just need to store the flag.

> 2. Calling EnsureDirectory(), CreateDirectoryMetadata() and
> CreateDirectoryMetadata2() in  PersistOp::DoDirectoryWork() if the directory
> has not been created yet.

Yes, these methods need to be called if the origin directory doesn't exist. However you should try to share code as usual, so maybe you need to create a new helper method that will be called by EnsureOriginIsInitialized() and PersistOp::DoDirectoryWork()

	Tom Tung [:tt, :ttung] Assignee
	Comment 119 • 8 years ago

Hi Jan,

In this patch, I addressed the comment and did a little changes, thus I listed my changes as following:

- Rewrite function OriginPersisted(), PersistOrigin(), GetDirectoryMetadata2(), GetDirectoryMetadata2WithRestore() as your suggest.

- Implement LockedGetOriginInfo() as your comment

- Improve code in PersistedOp::DoDirectoryWork() as your comment but doing a little changes on last condition.

- Remove assertion for mTemporaryStorageInitialized in PersistOrigin() since we don't call EnsureOriginIsInitialized(). (Maybe add a flag into new function to ensure we initialized origin before calling this function?) 

- Extract EnsureDirectory(), CreateDirectoryMetadata(), CreateDirectoryMetadata2() and InitializeOrigin() from EnsureOriginIsInitialized() and name it as InitializeDirectoryAndOrigin().

- Add Setting mNeedsQuotaManagerInit as true back to make sure calling EnsureStorageIsInitialized() since QuotaRequestBase::Init() is overrided by PersistedOp::Init().

- Keep mSuffix in class PersistedOp since we may need it when creating directory metadata.

- Rewrite class PersistOp and PersistedOp since PersistOp don’t need mPersist flag and PersistedOp don’t need mSuffix string

Could you help me to review this patch? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 121 • 8 years ago

Update this patch since addressing comment #114.

	Jan Varga [:janv]
	Comment 122 • 8 years ago

Tom, can you rebase your tree, P1 doesn't apply well to current m-c.

	Tom Tung [:tt, :ttung] Assignee
	Comment 123 • 8 years ago

(In reply to Jan Varga [:janv] from comment #122)
> Tom, can you rebase your tree, P1 doesn't apply well to current m-c.

I implement it base on your four patches in Bug 1311057. While I tried to update them, I cannot build it because PrincipalOriginAttributes is an unknown type name. I cannot find this type in current m-c but I guess I need to change it to OriginAttributes. So, I will give it a try!

	Tom Tung [:tt, :ttung] Assignee
	Comment 124 • 8 years ago

Rebase the patch to current m-c and the newest patch in RemoveIsApp.

	Tom Tung [:tt, :ttung] Assignee
	Comment 125 • 8 years ago

Rebased patch as well.

	Jan Varga [:janv]
	Comment 126 • 8 years ago

Comment on attachment 8827900 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v4.1-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

Tom, as I said, dig deeper into the code and do more self reviews :)
I'll send comments and my patch which addresses them later.

	Jan Varga [:janv]
	Comment 127 • 8 years ago

Apply this patch on top of your P1 patch. This time I also ran xpcshell tests, including your new tests from patch P2. Anyway, please test my patch on your side too.

	Jan Varga [:janv]
	Comment 128 • 8 years ago

Comment on attachment 8827900 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v4.1-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

::: dom/quota/ActorsParent.cpp
@@ +1179,5 @@
> +public:
> +  explicit PersistedOp(const RequestParams& aParams);
> +
> +  bool
> +  Init(Quota* aQuota) override;

Ok, so you saved mSuffix, on the other hand you introduced a new Init() method which is exactly same as the one in PersistOp(). Then you created a static method for both DoInitOnMainThread() methods. All in all it looks like a mix of C and C++ code.
Can't you just create a new base class that will do initialization for PersistedOp and PersistOp ?

@@ +1227,5 @@
> +  static nsresult
> +  PersistInitOnMainThread(const RequestParams& aParams,
> +                          nsACString* aSuffix,
> +                          nsACString* aGroup,
> +                          OriginScope& aOriginScope);

As I said, why a static method ?

@@ +3515,5 @@
> +
> +  RefPtr<OriginInfo> originInfo = LockedGetOriginInfo(PERSISTENCE_TYPE_DEFAULT,
> +                                                      aGroup,
> +                                                      aOrigin);
> +  if (!originInfo) {

So, we do all these |if (! ...) { return ... }| to keep the indentation sane.
If there's just one condition, you can rather do |if (something)| without the negation.

@@ +3533,5 @@
> +
> +  RefPtr<OriginInfo> originInfo = LockedGetOriginInfo(PERSISTENCE_TYPE_DEFAULT,
> +                                                      aGroup,
> +                                                      aOrigin);
> +  if (!originInfo) {

Again, no need for the negation.

@@ +4654,5 @@
>    }
>  }
>  
>  nsresult
> +QuotaManager::InitializeDirectoryAndOrigin(PersistenceType aPersistenceType,

As I said, this method doesn't help much with code sharing, efficiency and correctness.

@@ +4688,5 @@
> +                                    aSuffix,
> +                                    aGroup,
> +                                    aOrigin);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      } else {

What about this indentation? It looks wrong.

@@ +4827,5 @@
>      }
>    }
>  #endif
>  
> +  rv = InitializeDirectoryAndOrigin(aPersistenceType, directory, aSuffix,

So the first problem here is that you didn't move/use the |#ifndef RELEASE_OR_BETA| code, I'll let you figure out why it's important.

@@ +5135,5 @@
> +QuotaManager::LockedGetOriginInfo(PersistenceType aPersistenceType,
> +                                  const nsACString& aGroup,
> +                                  const nsACString& aOrigin)
> +{
> +  mQuotaMutex.AssertCurrentThreadOwns();

Nit: It's nicer to have all assertions in one section here (no need for the empty line)

@@ +6763,5 @@
> +
> +  mPersistenceType.SetValue(PERSISTENCE_TYPE_DEFAULT);
> +
> +  mNeedsMainThreadInit = true;
> +  mNeedsQuotaManagerInit = true;

So you added |mNeedsQuotaManagerInit = true;| again, despite the fact that it's already set in QuotaRequestBase::Init(). The problem is that you forgot to call QuotaRequestBase::Init() here.

@@ +6776,5 @@
> +  MOZ_ASSERT(GetState() == State_Initializing);
> +  MOZ_ASSERT(mNeedsMainThreadInit);
> +
> +  nsresult rv = PersistOp::PersistInitOnMainThread(mParams, nullptr, &mGroup,
> +                                                   mOriginScope);

Again, this is C++ not C.

@@ +6821,5 @@
> +
> +  if (!exists) {
> +    // The directory has not been created yet.
> +    mPersisted = false;
> +    return  NS_OK;

extra space there

@@ +6829,5 @@
> +  int64_t dummyTimestamp;
> +  rv = aQuotaManager->GetDirectoryMetadata2WithRestore(directory,
> +                                                       /* aPersistent */ false,
> +                                                       &dummyTimestamp,
> +                                                       &persisted.SetValue());

Not sure why you insist on using the Nullable here.

@@ +6942,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  rv = aQuotaManager->InitializeDirectoryAndOrigin(mPersistenceType.Value(),

So here, if the directory doesn't exist yet, you create a metadata file with persisted flag set to true and then you open the same file again and rewrite the flag to true. Can't we just create the file with the flag set to true ?

::: dom/quota/QuotaManager.h
@@ +281,5 @@
> +  InitializeDirectoryAndOrigin(PersistenceType aPersistenceType,
> +                               nsIFile* aDirectory,
> +                               const nsACString& aSuffix,
> +                               const nsACString& aGroup,
> +                               const nsACString& aOrigin);

Extracting the whole block in EnsureOriginIsInitialized to this new method doesn't help much.

	Jan Varga [:janv]
	Comment 129 • 8 years ago

(In reply to Jan Varga [:janv] from comment #128)
> So here, if the directory doesn't exist yet, you create a metadata file with
> persisted flag set to true and then you open the same file again and rewrite
> the flag to true. Can't we just create the file with the flag set to true ?

Correction:
So here, if the directory doesn't exist yet, you create a metadata file with
persisted flag set to *false* and then you open the same file again and rewrite
the flag to true. Can't we just create the file with the flag set to true ?

	Tom Tung [:tt, :ttung] Assignee
	Comment 130 • 8 years ago

(In reply to Jan Varga [:janv] from comment #128)
> ::: dom/quota/ActorsParent.cpp
> @@ +1179,5 @@
> Ok, so you saved mSuffix, on the other hand you introduced a new Init()
> method which is exactly same as the one in PersistOp(). Then you created a
> static method for both DoInitOnMainThread() methods. All in all it looks
> like a mix of C and C++ code.
> Can't you just create a new base class that will do initialization for
> PersistedOp and PersistOp ?

Ok, I'll implement a base class next time.
I have considered about creating a new base class, but I thought it is better to implement a helper function to extract out the same code. And I was wrong.

> @@ +1227,5 @@
> > +  static nsresult
> > +  PersistInitOnMainThread(const RequestParams& aParams,
> > +                          nsACString* aSuffix,
> > +                          nsACString* aGroup,
> > +                          OriginScope& aOriginScope);
> 
> As I said, why a static method ?

As I mentioned above, I was wrong. :(
I was thinking to have a static helper function so that I don't need to care about class's instance. 

> @@ +3515,5 @@
> > +
> > +  RefPtr<OriginInfo> originInfo = LockedGetOriginInfo(PERSISTENCE_TYPE_DEFAULT,
> > +                                                      aGroup,
> > +                                                      aOrigin);
> > +  if (!originInfo) {
> 
> So, we do all these |if (! ...) { return ... }| to keep the indentation sane.
> If there's just one condition, you can rather do |if (something)| without
> the negation.

Thanks, I'll keep it in mind.

> @@ +4688,5 @@
> > +                                    aSuffix,
> > +                                    aGroup,
> > +                                    aOrigin);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +      } else {
> 
> What about this indentation? It looks wrong.

Sorry, it should not happen.

> @@ +4827,5 @@
> >      }
> >    }
> >  #endif
> >  
> > +  rv = InitializeDirectoryAndOrigin(aPersistenceType, directory, aSuffix,
> 
> So the first problem here is that you didn't move/use the |#ifndef
> RELEASE_OR_BETA| code, I'll let you figure out why it's important.

I thought it is a check for preventing from an origin which cannot be parsed. The failure happens only in test cases (fake origin). However, I was wrong, and it really happens in real origin. 

> @@ +6763,5 @@
> > +
> > +  mPersistenceType.SetValue(PERSISTENCE_TYPE_DEFAULT);
> > +
> > +  mNeedsMainThreadInit = true;
> > +  mNeedsQuotaManagerInit = true;
> 
> So you added |mNeedsQuotaManagerInit = true;| again, despite the fact that
> it's already set in QuotaRequestBase::Init(). The problem is that you forgot
> to call QuotaRequestBase::Init() here.

Sorry for not explain clearly or misunderstanding. :(
I tried to express it in comment #119. 
I override QuotaRequestBase::Init() by Persisted::Init() so I need to reset it to true again.

> 
> @@ +6776,5 @@
> > +  MOZ_ASSERT(GetState() == State_Initializing);
> > +  MOZ_ASSERT(mNeedsMainThreadInit);
> > +
> > +  nsresult rv = PersistOp::PersistInitOnMainThread(mParams, nullptr, &mGroup,
> > +                                                   mOriginScope);
> 
> Again, this is C++ not C.

I'll address base-class method next time

> @@ +6829,5 @@
> > +  int64_t dummyTimestamp;
> > +  rv = aQuotaManager->GetDirectoryMetadata2WithRestore(directory,
> > +                                                       /* aPersistent */ false,
> > +                                                       &dummyTimestamp,
> > +                                                       &persisted.SetValue());
> 
> Not sure why you insist on using the Nullable here.

I thought we don't to set "mPersisted" as failure happens, so we need a temporary variable to store it. And it's not necessary to create another boolean since we've already haven one. 

Please correct me if there is any problem here! Thanks!

> ::: dom/quota/QuotaManager.h
> @@ +281,5 @@
> > +  InitializeDirectoryAndOrigin(PersistenceType aPersistenceType,
> > +                               nsIFile* aDirectory,
> > +                               const nsACString& aSuffix,
> > +                               const nsACString& aGroup,
> > +                               const nsACString& aOrigin);
> 
> Extracting the whole block in EnsureOriginIsInitialized to this new method
> doesn't help much.

I'll think about a better approach next time!

(In reply to Jan Varga [:janv] from comment #129)
> Correction:
> So here, if the directory doesn't exist yet, you create a metadata file with
> persisted flag set to *false* and then you open the same file again and
> rewrite
> the flag to true. Can't we just create the file with the flag set to true ?

That's a good idea! I'll do it in next patch!

	Jan Varga [:janv]
	Comment 131 • 8 years ago

(In reply to Tom Tung [:tt] from comment #130)
> > So the first problem here is that you didn't move/use the |#ifndef
> > RELEASE_OR_BETA| code, I'll let you figure out why it's important.
> 
> I thought it is a check for preventing from an origin which cannot be
> parsed. The failure happens only in test cases (fake origin). However, I was
> wrong, and it really happens in real origin. 

It's a guard. Before we create a new origin directory we make sure that it can be parsed
by our origin parser. PersistOp needs to check it too.

> > So you added |mNeedsQuotaManagerInit = true;| again, despite the fact that
> > it's already set in QuotaRequestBase::Init(). The problem is that you forgot
> > to call QuotaRequestBase::Init() here.
> 
> Sorry for not explain clearly or misunderstanding. :(
> I tried to express it in comment #119. 
> I override QuotaRequestBase::Init() by Persisted::Init() so I need to reset
> it to true again.

Overridden methods can call methods which they are overriding, right ?
So if we add something to QuotaRequestBase::Init() it will be automatically executed by a derived class.

> > Not sure why you insist on using the Nullable here.
> 
> I thought we don't to set "mPersisted" as failure happens, so we need a
> temporary variable to store it. And it's not necessary to create another
> boolean since we've already haven one. 
> 
> Please correct me if there is any problem here! Thanks!

Local variables are quite cheap, they are on the stack. Having a new 1-byte variable is not a big deal, easier than passing |&persisted.SetValue()|

> > So here, if the directory doesn't exist yet, you create a metadata file with
> > persisted flag set to *false* and then you open the same file again and
> > rewrite
> > the flag to true. Can't we just create the file with the flag set to true ?
> 
> That's a good idea! I'll do it in next patch!

Just to be sure, you just need to apply my patch and that's it (if you don't find anything problematic during your testing).

	Jan Varga [:janv]
	Comment 132 • 8 years ago

Comment on attachment 8827901 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P2-v2.1-Test for persisted() and persist().

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

These comments are just for test_persistent_base.js

::: dom/indexedDB/test/unit/test_persistent_basic.js
@@ +9,5 @@
> +{
> +  const spec = "http://foo.com";
> +  const principal = getPrincipal(spec);
> +
> +  const name = this.window ? window.location.pathname : "test_persistnet_basic";

There's a typo, *persistent*, actually "persistent" is a bit misleading, we are testing persisted() and persist() here.

@@ +13,5 @@
> +  const name = this.window ? window.location.pathname : "test_persistnet_basic";
> +  const objectStoreName = "foo";
> +
> +  // Store in 1 MB chunks.
> +  const dataSize = 1024 * 1024;

Where is this used ?

@@ +16,5 @@
> +  // Store in 1 MB chunks.
> +  const dataSize = 1024 * 1024;
> +
> +  // Initialize
> +  clearAllDatabases(continueToNextStepSync);

Why do you need to clear everything (all origins) first ?

@@ +34,5 @@
> +  yield undefined;
> +
> +  is(request.result, true, spec + " should be persisted now.");
> +
> +  info("Test 2: Store an object and check whether the origin is persisted.");

Not sure why we have to test this. It would be better to test all possible code paths in PersistedOp and PersistOp.
For example, both can run when an origin has been already initialized or not.
If you really want to be thorough, then you can simulate situations like having the origin directory but not having metadata file or having broken metadata file.

@@ +55,5 @@
> +  let obj = { name: "foo" }
> +
> +  let trans = db.transaction(objectStoreName, "readwrite");
> +  request = trans.objectStore(objectStoreName).add(obj);
> +  request.onerror = errorHandler;

error events propagate, so if you have db.onerror set then you don't need request.onerror

@@ +61,5 @@
> +  trans.oncomplete = function(event) {
> +    testGenerator.next(true);
> +  }
> +  let next = yield undefined;
> +  if (!next) {

So who calls testGenerator.next(false) ?

@@ +70,5 @@
> +  yield undefined;
> +
> +  is(request.result, true, spec + " should still be persisted.");
> +
> +  info("Test 3: Clean the storage and check whether the origin is persisted.");

Not sure about this either, how can object store clearing affect persisted state. It would be better to test that after calling clearStoragesForPrincipal(), the persisted flag is reset to false.

	Jan Varga [:janv]
	Comment 133 • 8 years ago

(In reply to Jan Varga [:janv] from comment #132)
> These comments are just for test_persistent_base.js

I meant test_persistent_basic.js

	Tom Tung [:tt, :ttung] Assignee
	Comment 134 • 8 years ago

(In reply to Jan Varga [:janv] from comment #132)
Thanks for the feedbacks! I'll re-design this test(including test name) in next patch.

> ::: dom/indexedDB/test/unit/test_persistent_basic.js
> @@ +9,5 @@
> > +{
> > + const spec = "http://foo.com";
> > + const principal = getPrincipal(spec);
> > +
> > + const name = this.window ? window.location.pathname : "test_persistnet_basic";
> 
> There's a typo, *persistent*, actually "persistent" is a bit misleading, we
> are testing persisted() and persist() here.

Okay, I'll rename this and the test name to "test_persist_basic"

> @@ +13,5 @@
> > + const name = this.window ? window.location.pathname : "test_persistnet_basic";
> > + const objectStoreName = "foo";
> > +
> > + // Store in 1 MB chunks.
> > + const dataSize = 1024 * 1024;
> 
> Where is this used ?

It shouldn't be here.

> @@ +16,5 @@
> Why do you need to clear everything (all origins) first ?

I thought that it's better to clear all the database before testing things related to metadata and originInfo. I'll remove it in next patch!

> @@ +34,5 @@
> > + yield undefined;
> > +
> > + is(request.result, true, spec + " should be persisted now.");
> > +
> > + info("Test 2: Store an object and check whether the origin is persisted.");
> 
> Not sure why we have to test this. It would be better to test all possible
> code paths in PersistedOp and PersistOp.
> For example, both can run when an origin has been already initialized or not.
> If you really want to be thorough, then you can simulate situations like
> having the origin directory but not having metadata file or having broken
> metadata file.

I'll rewrite this test in next patch. Thanks for the feedback!

> @@ +55,5 @@
> > + let obj = { name: "foo" }
> > +
> > + let trans = db.transaction(objectStoreName, "readwrite");
> > + request = trans.objectStore(objectStoreName).add(obj);
> > + request.onerror = errorHandler;
> 
> error events propagate, so if you have db.onerror set then you don't need
> request.onerror

Oh, I see!
 
> @@ +61,5 @@
> > + trans.oncomplete = function(event) {
> > + testGenerator.next(true);
> > + }
> > + let next = yield undefined;
> > + if (!next) {
> 
> So who calls testGenerator.next(false) ?

No, I'll fix all similar problem here.

> @@ +70,5 @@
> > + yield undefined;
> > +
> > + is(request.result, true, spec + " should still be persisted.");
> > +
> > + info("Test 3: Clean the storage and check whether the origin is persisted.");
> 
> Not sure about this either, how can object store clearing affect persisted
> state. It would be better to test that after calling
> clearStoragesForPrincipal(), the persisted flag is reset to false.

I wanted to make sure the "persisted" state will not change as object store/delete. 
I'll try to rewrite it and think about testing possible code path of PersistOp/PersistOp.

	Tom Tung [:tt, :ttung] Assignee
	Comment 135 • 8 years ago

(In reply to Jan Varga [:janv] from comment #127)
> Created attachment 8828253 [details] [diff] [review]
> patch
> 
> Apply this patch on top of your P1 patch. This time I also ran xpcshell
> tests, including your new tests from patch P2. Anyway, please test my patch
> on your side too.

It also passes all the indexedDB tests on my side. Thanks!

BTW, I consider renaming mTemporaryStorage* (e.g. mTemporaryStorageLimit, ...) to mGlobalStorage* or somethings else since they include persist storages as well. Should I do it in this bug or file a follow-up bug?

	Tom Tung [:tt, :ttung] Assignee
	Comment 136 • 8 years ago

Update patch for testing (P2).
- I rewrite the "test_persistent_basic.js" and rename it as "test_persist_basic.js".
- In "test_persist_basic.js", I test both persist() and persisted() before and after EnsureOriginIsInitialized(). Besides, I also test them when the origin cannot be parsed.

Next: I'll rename "test_persistent_storage.js" later.

	Tom Tung [:tt, :ttung] Assignee
	Comment 137 • 8 years ago

Updated patch again!

	Tom Tung [:tt, :ttung] Assignee
	Comment 138 • 8 years ago

Comment on attachment 8827901 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P2-v2.1-Test for persisted() and persist().

Remove the r? request since I found some similar problems in the other two test cases. I'll fix them and send a review request later. Sorry for that if you are reviewing them.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 139 • 8 years ago

    I'm a bit worried about we add callback into Request instead of registering callback in the beginning. Right now we call persist() and then get nsIQuotaRequest instance, then setCallback(). What if we got callback (ex: onComplete) before we setCallback()? Since we removed callback parameter, do we guarantee callback can't be fired before we register callback?

    RefPtr<nsIQuotaRequest> request;                                        
    rv = CheckIsPersistentForPrincipal(principal, getter_AddRefs(request)); 
    if (NS_SUCCEEDED(rv)) {                                                 
      request->SetCallback(resolver);                                       
    }

	Jan Varga [:janv]
	Comment 140 • 8 years ago

You are still on the same thread, right ? So nothing else can touch the callback.
And all the methods in nsIQuotaManagerService shouldn't synchronously fire any callbacks when they are being executed. Even when a result is ready, a new runnable should be dispatched to current thread to fire the callback asynchronously. You can add comments for this if you want.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 141 • 8 years ago

(In reply to Jan Varga [:janv] from comment #140)
> You are still on the same thread, right ? So nothing else can touch the
> callback.
> And all the methods in nsIQuotaManagerService shouldn't synchronously fire
> any callbacks when they are being executed. Even when a result is ready, a
> new runnable should be dispatched to current thread to fire the callback
> asynchronously. You can add comments for this if you want.
I see, thanks for the explanation.

	Tom Tung [:tt, :ttung] Assignee
	Comment 142 • 8 years ago

Update patch.

	Jan Varga [:janv]
	Comment 143 • 8 years ago

(In reply to Tom Tung [:tt] from comment #135)
> BTW, I consider renaming mTemporaryStorage* (e.g. mTemporaryStorageLimit,
> ...) to mGlobalStorage* or somethings else since they include persist
> storages as well.

It does with the isApp too, it's not a big problem.

> Should I do it in this bug or file a follow-up bug?

Definitely not in this bug and it's really low priority.

	Jan Varga [:janv]
	Comment 144 • 8 years ago

Tom, are you going to attach a new P1 patch ? It should be just the current P1 + my patch, but maybe you had to modify something. We don't have to keep P1 + my patch separate. I'd like to take a look at final P1 one more time.

	Tom Tung [:tt, :ttung] Assignee
	Comment 145 • 8 years ago

(In reply to Jan Varga [:janv] from comment #144)
> Tom, are you going to attach a new P1 patch ? It should be just the current
> P1 + my patch, but maybe you had to modify something. We don't have to keep
> P1 + my patch separate. I'd like to take a look at final P1 one more time.

Got it! I'll attach a new P1 patch tomorrow!

	Tom Tung [:tt, :ttung] Assignee
	Comment 146 • 8 years ago

Hi Jan,

In this patch, I rewrite the test cases. Could you help me to review it? Thanks!

The test_persist.js test the persist() and persisted() before and after calling EnsureOriginIsInitialized().
Besides, it also verifies the persist() will fail when the origin cannot be parsed.

In test_persist_quotaExceeded.js, I test the quota exceeded situations before calling persist() and after calling persist(). 

First, I test two different origins in the same group when the quata is exceeded. 
Before persisting one of them, they shared the same group limit & usage. After that, the best-effort one can store more data since the persisted origin doesn't share the same group usage. Moreover, it can store more because it doesn't be bounded by group limit but by the global limit. 

In the end of the test, I use the other origin which is not in the same group to test eviction won't happen on the persisted origin.

	Tom Tung [:tt, :ttung] Assignee
	Comment 147 • 8 years ago

Update the patch but I will check it again tomorrow!

	Tom Tung [:tt, :ttung] Assignee
	Comment 148 • 8 years ago

Hi Jan,

Except apply your comments & patch, I re-write few function and I list them below inline:

- Make function QuotaManager::GetDirectoryMetadata2(..) accept passing 'nullptr' to aTimestamp as well for removing dummyTimestamp in PersistedOp::DoDirectoryWork() and PersistOp::DoDirectoryWork. However, I modified the assertion to MOZ_ASSERT(aTimestamp || aPersisted) and I'm not sure about whether it is ok or not. Please help me check this part!

- Add comments before passing 'nullptr' to make it more readable.

- Rewrite QuotaManager::LockedGetOriginInfo() to remove negation condition but I'm not sure about this as well. Please help me check this part as well.

Would you mind reviewing this patch again? Thanks! I'll upload the inter-diff patch (between applying your patch and this patch) later.

	Tom Tung [:tt, :ttung] Assignee
	Comment 149 • 8 years ago

inter-diff patch for P1 between my previous r? patch + :janv's patch and current r? patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 150 • 8 years ago

Update patch to make all the Persist* things (e.g. PersistParams) are sorted alphabetically. Sorry for that if you are reviewing it, Jan. :(

	Tom Tung [:tt, :ttung] Assignee
	Comment 151 • 8 years ago

inter-diff patch for P1 between my previous reviewed patch & Jan's patch and current r? patch.

	Jan Varga [:janv]
	Comment 152 • 8 years ago

(In reply to Tom Tung [:tt] from comment #150)
> Created attachment 8832809 [details] [diff] [review]
> [Rebased Patch] Bug-1298329-P1-v5.2-Implement persist/persisted in
> QuotaManager/QuotaManagerService.
> 
> Update patch to make all the Persist* things (e.g. PersistParams) are sorted
> alphabetically. Sorry for that if you are reviewing it, Jan. :(

That union mostly follows the order of methods in nsIQuotaManagerService, I don't think we need to sort them alphabetically here.

	Tom Tung [:tt, :ttung] Assignee
	Comment 153 • 8 years ago

(In reply to Jan Varga [:janv] from comment #152)
> That union mostly follows the order of methods in nsIQuotaManagerService, I
> don't think we need to sort them alphabetically here.

Oh, I see. I will correct them back in next patch! Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 154 • 8 years ago

I changed my mind. I decided to update patch for following the order in nsIQutaManagerService.

	Tom Tung [:tt, :ttung] Assignee
	Comment 155 • 8 years ago

Update inter-diff patch as well

	Jan Varga [:janv]
	Comment 156 • 8 years ago

Comment on attachment 8833142 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P1-v5.3-Implement persist/persisted in QuotaManager/QuotaManagerService.

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

Looks good.

	Jan Varga [:janv]
	Comment 159 • 8 years ago

Comment on attachment 8829825 [details] [diff] [review]
[Rebased Patch] Bug-1298329-P2-v3.2-Test for persisted() and persist().

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

This looks promising, better than the previous patch.
However, I created a new standalone testing infrastructure for quota manager in bug 1339081.
It would be nicer to rewrite the tests for persist/persisted using this new infrastructure.
You can wait a few days until my patches land or you can download them here:
https://hg.mozilla.org/try/pushloghtml?changeset=6b1f889d3484421cc4ce5df15ce76eb8ec1b3daa