Treat persistent storage as temporary storage

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: janv, Assigned: janv)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
Default storage work happens in bug 1083927, but to fix bug 1083852, we need something a bit simpler that doesn't add a new storage repository nor support for explicit persistent storage.
The main reason for this is to fix bug 1083852 on trunk and land on aurora ASAP. Full default storage support depends on bug 771288 and bug 942542. Patches for those are quite big and need to be reviewed. Adding a new repository and explicit persistent storage is also quite risky for aurora, so we decided to create an intermediate step.
Comment on attachment 8512116 [details] [diff] [review]
patch

I think it's ready for an initial review.
Attachment #8512116 - Attachment is patch: true
Attachment #8512116 - Flags: review?(bent.mozilla)
Oh, forgot to say that the patch is based on m-c plus the patch for bug 1068787.
Comment on attachment 8512116 [details] [diff] [review]
patch

Luke, can you take a look at asmjscache changes too ?
Attachment #8512116 - Flags: review?(luke)
Comment on attachment 8512116 [details] [diff] [review]
patch

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

Looks great!

Only nits that you can feel free to ignore since I'm not really abreast on current dom style conventions:

I found the name (a|m)HasQuotaPerm and the implied phrase "has quota permissions" to be ambiguous.  Permission to... check quota? ... maintain a quota?  Perhaps there is a more direct abbreviation?  Say, (a|m)HasUnlimStoragePerm?

::: dom/asmjscache/AsmJSCache.cpp
@@ +682,5 @@
> +
> +    // If we are performing install-time caching of an app, we'd like to store
> +    // the cache entry in persistent storage so the entry is never evicted,
> +    // but we need to check that quota is not enforced for the app.
> +    // That justify us in skipping all quota checks when storing the cache entry

*justifies

@@ +690,5 @@
> +
> +    MOZ_ASSERT_IF(mWriteParams.mInstalled, mIsApp);
> +
> +    if (mWriteParams.mInstalled && !QuotaManager::IsQuotaEnforced(
> +          quota::PERSISTENCE_TYPE_PERSISTENT, mOrigin, mIsApp, mHasQuotaPerm)) {

Is having the function call be farther to the right than the first arg really the style?  (It's a bit ugly :)

@@ +707,5 @@
> +  // only be stored in temporary storage.)
> +
> +  MOZ_ASSERT_IF(mPersistence != quota::PERSISTENCE_TYPE_INVALID, mIsApp);
> +  MOZ_ASSERT_IF(mPersistence != quota::PERSISTENCE_TYPE_INVALID,
> +                mPersistence == quota::PERSISTENCE_TYPE_PERSISTENT);

I think it'd read a tiny bit better as
  MOZ_ASSERT_IF(mPersistence != quota::PERSISTENCE_TYPE_INVALID,
                mIsApp && mPersistence == quota::PERSISTENCE_TYPE_PERSISTENT);
Attachment #8512116 - Flags: review?(luke) → review+
Posted patch patch v2 (obsolete) — Splinter Review
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Attachment #8512813 - Flags: review?(bent.mozilla)
Attachment #8512116 - Flags: review?(bent.mozilla)
(In reply to Luke Wagner [:luke] from comment #5)
> Comment on attachment 8512116 [details] [diff] [review]

Thanks for quick review, I think I addressed all your comments in the patch v2
The patch also contains a fix for file:/// handling in the origin parser.
Posted patch patch v3 (obsolete) — Splinter Review
Fixed a bug in EnsureOriginIsInitialized()
Attachment #8512813 - Attachment is obsolete: true
Attachment #8512813 - Flags: review?(bent.mozilla)
Attachment #8513588 - Flags: review?(bent.mozilla)
Posted patch interdiff (obsolete) — Splinter Review
simplification of patch v3 for aurora
Posted patch folded patch (obsolete) — Splinter Review
patch v3 + simplification folded into one patch
bent asked me to simplify the patch because the origin parser and metadata creation seems too risky for aurora.

The simplified patch adds a temporary hack (just for aurora):
After getting group (eTLD+1) and origin string by calling GetInfoFromPrincipal, origin string is sanitized (e.g. ':' replaced with '+') and the value is also assigned to group. So each origin has its own group (but only for persistent storage treated as temporary).
This way we don't have to parse already stored origins on disk and get group, etc. on the main thread.

The patch is currently on try:
https://tbpl.mozilla.org/?tree=Try&rev=4c2a541f5991
It seems that origin clearing code is not right. I'll fix it tomorrow.
Posted patch interdiff (obsolete) — Splinter Review
Attachment #8513629 - Attachment is obsolete: true
Posted patch folded patch (obsolete) — Splinter Review
Attachment #8513631 - Attachment is obsolete: true
Comment on attachment 8512813 [details] [diff] [review]
patch v2

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

I had these comments lying around from a previous version of the patch

::: dom/asmjscache/AsmJSCache.cpp
@@ +667,5 @@
>    State mState;
> +
> +  bool mIsApp;
> +  bool mHasUnlimStoragePerm;
> +  bool mEnforcingQuota;

Can these be const?

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +538,3 @@
>  
> +  aValue->InvalidateAllFileManagers();
> +  return PL_DHASH_REMOVE;

Now that there's no condition why not just do PL_DHASH_NEXT here and then call Clear() on the table after you call Enumerate() (could even make it EnumerateRead then)

@@ +563,2 @@
>  
> +  info->InvalidateAndRemoveFileManagers(aPersistenceType);

No need to pass an arg here anymore, right?

@@ +565,2 @@
>  
> +  if (!info->HasFileManagers()) {

This condition should always be true now right? No need to test, just assert.

::: dom/indexedDB/test/mochitest.ini
@@ +147,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
>  [test_deleteDatabase_interactions.html]
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
>  [test_disabled_quota_prompt.html]
> +# Test temporarily disabled.

Hrm. Why?

::: dom/quota/QuotaManager.cpp
@@ +636,5 @@
> +
> +  int64_t mTimestamp;
> +  nsCString mGroup;
> +  nsCString mOrigin;
> +  bool mIsApp;

Can't you tell this from the appId?

@@ +664,5 @@
> +  };
> +
> +  const nsString mOrigin;
> +
> +  Nullable<uint32_t> mAppId;

Can't this be just a plain uint32_t that is set to NO_APP_ID?

@@ +665,5 @@
> +
> +  const nsString mOrigin;
> +
> +  Nullable<uint32_t> mAppId;
> +  Nullable<bool> mInMozBrowser;

This can just be a plain bool.

@@ +681,5 @@
> +  OriginParser(const nsAString& aOrigin)
> +    : mOrigin(aOrigin)
> +    , mAppId()
> +    , mInMozBrowser()
> +    , mPort()

These don't have to be explicitly initialized, do they?

@@ +742,5 @@
> +  OriginKey(PersistenceType aPersistenceType,
> +            const nsACString& aOrigin)
> +  {
> +    PersistenceTypeToText(aPersistenceType, *this);
> +    Append(NS_LITERAL_CSTRING(":"));

Append(':')

@@ +829,5 @@
> +GetLastModifiedTime(nsIFile* aFile, int64_t* aTimestamp)
> +{
> +  AssertIsOnIOThread();
> +  MOZ_ASSERT(aFile);
> +  MOZ_ASSERT(aTimestamp);

You need to also assert |*aTimestamp == INT64_MIN| on the first call somehow... Otherwise this will return garbage.

@@ +865,5 @@
> +      return rv;
> +    }
> +
> +    nsCOMPtr<nsIFile> file = do_QueryInterface(entry);
> +    if (NS_WARN_IF(!file)) {

Just MOZ_ASSERT this

@@ +1171,5 @@
> +                      const nsACString& aOrigin,
> +                      bool aIsApp)
> +{
> +  if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT && (aIsApp ||
> +        aOrigin.Equals("chrome") || aOrigin.Equals("moz-safe-about:home"))) {

This can't be duplicated with the stuff in OriginInfo. We need something better. Probably a static function on QuotaManager.

@@ +1678,5 @@
> +
> +  LiveStorageIndex& liveStorageIndex = GetLiveStorageIndex(aStorage->Type());
> +
> +  nsTArray<nsIOfflineStorage*>* array;
> +  if (liveStorageIndex.Get(origin, &array) && array->RemoveElement(aStorage)) {

|array->RemoveElement(aStorage)| should always return true, right? I'd MOZ_ALWAYS_TRUE that.

@@ +1990,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    nsCOMPtr<nsIFile> childDirectory = do_QueryInterface(entry);

MOZ_ASSERT

@@ +2013,5 @@
> +          leafName.EqualsLiteral(DSSTORE_FILE_NAME)) {
> +        continue;
> +      }
> +
> +      NS_WARNING("Something in the repository that doesn't belong!");

Might be good to put the leafName in the warning.

::: dom/quota/QuotaManager.h
@@ +64,5 @@
> +  , mIsApp(aIsApp)
> +  { }
> +
> +  PersistenceType mPersistenceType;
> +  nsCString mOrigin;

This packs better if origin is first.

@@ +93,5 @@
>    typedef void
>    (*WaitingOnStoragesCallback)(nsTArray<nsCOMPtr<nsIOfflineStorage> >&, void*);
>  
> +  typedef nsClassHashtable<nsCStringHashKey,
> +                           nsTArray<nsIOfflineStorage*>> LiveStorageIndex;

"Index"? How about "Table" or "Map"

::: dom/quota/QuotaObject.cpp
@@ +296,5 @@
> +bool
> +OriginInfo::IsTreatedAsPersistent()
> +{
> +  if (mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_PERSISTENT && (mIsApp ||
> +      mOrigin.Equals("chrome") || mOrigin.Equals("moz-safe-about:home"))) {

Whitelisting origins like this isn't really a great solution... Is there nothing else we can do here?

Nit: just do:

  return cond1 &&
         (cond2 ||
          cond3 ||
          cond4);

@@ +421,5 @@
> +  uint64_t usage = 0;
> +
> +  if (mPersistenceType == PERSISTENCE_TYPE_TEMPORARY) {
> +    usage = mUsage;
> +  } else {

Hrm, why wouldn't you always loop through the other origininfos looking for temporary usage?

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

Seems smart to MOZ_ASSERT(mUsage >= originInfo->mUsage) before subtracting here.

@@ +476,2 @@
>  
> +      quotaManager->mTemporaryStorageUsage -= originInfo->mUsage;

And similar here.

::: dom/quota/QuotaObject.h
@@ +94,5 @@
> +  bool
> +  IsTreatedAsPersistent();
> +
> +  bool
> +  IsTreatedAsTemporary()

Both methods can be const

@@ +136,5 @@
>    nsCString mOrigin;
>    uint64_t mLimit;
>    uint64_t mUsage;
>    int64_t mAccessTime;
> +  bool mIsApp;

These can all be const right?
Comment on attachment 8514263 [details] [diff] [review]
folded patch

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

::: dom/quota/QuotaObject.cpp
@@ +177,3 @@
>    // Temporary storage has a hard limit for group usage (20 % of the global
>    // limit).
> +  if (groupUsage > quotaManager->GetGroupLimit()) {

bah, this should be:
  if (groupUsage + delta > quotaManager->GetGroupLimit()) {

@@ +244,5 @@
> +    if (complementaryGroupInfo) {
> +      groupUsage += complementaryGroupInfo->LockedGetTemporaryUsage();
> +    }
> +
> +    if (groupUsage > quotaManager->GetGroupLimit()) {

dtto
Comment on attachment 8514263 [details] [diff] [review]
folded patch

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

So long term we need to stop having two kinds of metadata files. It's weird :)

::: dom/quota/QuotaManager.cpp
@@ +625,5 @@
>    aOrigin.ReplaceChar(kReplaceChars, '+');
>  }
>  
>  nsresult
> +GetLastModifiedTime(nsIFile* aFile, int64_t* aTimestamp)

This function is tiny and only called in one place... Maybe just remove it?

@@ +632,5 @@
> +  MOZ_ASSERT(aFile);
> +  MOZ_ASSERT(aTimestamp);
> +
> +  bool isDirectory;
> +  nsresult rv = aFile->IsDirectory(&isDirectory);

Just #ifdef DEBUG this

@@ +852,5 @@
> +    return rv;
> +  }
> +
> +  int64_t fileSize;
> +  rv = metadataFile->GetFileSize(&fileSize);

You haven't checked to make sure that this file exists yet, right? Couldn't this return NS_ERROR_FILE_NOT_FOUND?

@@ +857,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  if (fileSize == sizeof(uint64_t)) {

This needs a comment about why this case is special.

@@ +859,5 @@
> +  }
> +
> +  if (fileSize == sizeof(uint64_t)) {
> +    nsCOMPtr<nsIBinaryInputStream> binaryStream;
> +    rv = GetDirectoryMetadataInputStream(aDirectory,

This is just going to clone the file again...

::: dom/quota/QuotaManager.h
@@ +356,5 @@
> +  // XXX This is temporary, we don't have real .metadata files for persistent
> +  //     storage yet, so we have to use sanitized origin strings and create
> +  //     separate group for each origin.
> +  static void
> +  UpdateInfoForPersistenceType(PersistenceType aPersistenceType,

This function seems unnecessary, and likely a footgun. Every time it's called it's being called immediately after a QuotaManager::GetInfoForFoo(), so why not just remove this function and do the fixup in those other functions?

::: dom/quota/QuotaObject.cpp
@@ +177,3 @@
>    // Temporary storage has a hard limit for group usage (20 % of the global
>    // limit).
> +  if (groupUsage > quotaManager->GetGroupLimit()) {

As you noted, this is wrong.

@@ +490,5 @@
> +    OriginInfo* originInfo = mOriginInfos[index - 1];
> +    if (originInfo->IsTreatedAsTemporary()) {
> +      mUsage -= originInfo->mUsage;
> +
> +      QuotaManager* quotaManager = QuotaManager::Get();

Do this outside the loop
Attachment #8514263 - Flags: review?(bent.mozilla) → review+
Updating the summary since the simplified patch doesn't check app status.
Summary: Treat persistent storage as default storage → Treat persistent storage as temporary storage
Blocks: 1083852
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #18)
> Comment on attachment 8514263 [details] [diff] [review]
> folded patch
> 
> Review of attachment 8514263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So long term we need to stop having two kinds of metadata files. It's weird
> :)

Sure, originally I just wanted to delete and recreate the empty metadata file for persistent storage to refresh its last modified time. But that doesn't work because we need to store the result of already called PR_Now() and I couldn't find an API in gecko that does something like "touch -t".
Implementation of "touch -t" just for this bug would be an overkill in my opinion (or calling "touch -t" using nsIFile::Launch).

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +625,5 @@
> >    aOrigin.ReplaceChar(kReplaceChars, '+');
> >  }
> >  
> >  nsresult
> > +GetLastModifiedTime(nsIFile* aFile, int64_t* aTimestamp)
> 
> This function is tiny and only called in one place... Maybe just remove it?

Ok.

> 
> @@ +632,5 @@
> > +  MOZ_ASSERT(aFile);
> > +  MOZ_ASSERT(aTimestamp);
> > +
> > +  bool isDirectory;
> > +  nsresult rv = aFile->IsDirectory(&isDirectory);
> 
> Just #ifdef DEBUG this

Moved to GetDirectoryMetadataLastModifiedTime(), but it's not #ifdef DEBUG
It returns NS_ERROR_UNEXPECTED if someone manually created a directory named ".metadata".

> 
> @@ +852,5 @@
> > +    return rv;
> > +  }
> > +
> > +  int64_t fileSize;
> > +  rv = metadataFile->GetFileSize(&fileSize);
> 
> You haven't checked to make sure that this file exists yet, right? Couldn't
> this return NS_ERROR_FILE_NOT_FOUND?

I added Exists() check.
It returns NS_ERROR_UNEXPECTED if someone manually removed the metadata file.

> 
> @@ +857,5 @@
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return rv;
> > +  }
> > +
> > +  if (fileSize == sizeof(uint64_t)) {
> 
> This needs a comment about why this case is special.

Ok, added a comment.

> 
> @@ +859,5 @@
> > +  }
> > +
> > +  if (fileSize == sizeof(uint64_t)) {
> > +    nsCOMPtr<nsIBinaryInputStream> binaryStream;
> > +    rv = GetDirectoryMetadataInputStream(aDirectory,
> 
> This is just going to clone the file again...

Fixed.

> 
> ::: dom/quota/QuotaManager.h
> @@ +356,5 @@
> > +  // XXX This is temporary, we don't have real .metadata files for persistent
> > +  //     storage yet, so we have to use sanitized origin strings and create
> > +  //     separate group for each origin.
> > +  static void
> > +  UpdateInfoForPersistenceType(PersistenceType aPersistenceType,
> 
> This function seems unnecessary, and likely a footgun. Every time it's
> called it's being called immediately after a QuotaManager::GetInfoForFoo(),
> so why not just remove this function and do the fixup in those other
> functions?

Yeah, that was my approach too, but for that I would need to pass persistence type to
QuotaManager::GetInfoForFoo() and AsmJSCache.cpp knows what persistence type to use
only after getting info by calling current QuotaManager::GetInfoFromPrincipal().

But ok, I'll rework it as you suggested.
It will still look a bit hacky though.

> 
> ::: dom/quota/QuotaObject.cpp
> @@ +177,3 @@
> >    // Temporary storage has a hard limit for group usage (20 % of the global
> >    // limit).
> > +  if (groupUsage > quotaManager->GetGroupLimit()) {
> 
> As you noted, this is wrong.

Fixed.

> 
> @@ +490,5 @@
> > +    OriginInfo* originInfo = mOriginInfos[index - 1];
> > +    if (originInfo->IsTreatedAsTemporary()) {
> > +      mUsage -= originInfo->mUsage;
> > +
> > +      QuotaManager* quotaManager = QuotaManager::Get();
> 
> Do this outside the loop

Done.
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #16)
> Comment on attachment 8512813 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8512813 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I had these comments lying around from a previous version of the patch

Ok, for now, I'll address only those that apply to the simplified patch

> 
> ::: dom/asmjscache/AsmJSCache.cpp
> @@ +667,5 @@
> >    State mState;
> > +
> > +  bool mIsApp;
> > +  bool mHasUnlimStoragePerm;
> > +  bool mEnforcingQuota;
> 
> Can these be const?

no, real initialization of these doesn't happen in the constructor.

> ::: dom/indexedDB/test/mochitest.ini
> @@ +147,5 @@
> >  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> >  [test_deleteDatabase_interactions.html]
> >  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> >  [test_disabled_quota_prompt.html]
> > +# Test temporarily disabled.
> 
> Hrm. Why?

There's now nothing to test, the only persistent storage is chrome or about:home.
Chrome doesn't track quota at all, only about:home could reach 50MB in theory.

> ::: dom/quota/QuotaManager.cpp
> @@ +1171,5 @@
> > +                      const nsACString& aOrigin,
> > +                      bool aIsApp)
> > +{
> > +  if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT && (aIsApp ||
> > +        aOrigin.Equals("chrome") || aOrigin.Equals("moz-safe-about:home"))) {
> 
> This can't be duplicated with the stuff in OriginInfo. We need something
> better. Probably a static function on QuotaManager.
> 

Ok, I plan to do various optimizations like expanding OriginParams to include:
PersistenceType mPersistenceType;
const nsACString& mGroup;
const nsACString& mOrigin;
bool mIsApp;

convert it to a class and define IsTreatedAsPersistent() on the class
That flag can be cached, etc.

OriginParams will be used all over, instead of passing 4 arguments
But that's indeed too much for this patch.

> @@ +1678,5 @@
> > +
> > +  LiveStorageIndex& liveStorageIndex = GetLiveStorageIndex(aStorage->Type());
> > +
> > +  nsTArray<nsIOfflineStorage*>* array;
> > +  if (liveStorageIndex.Get(origin, &array) && array->RemoveElement(aStorage)) {
> 
> |array->RemoveElement(aStorage)| should always return true, right? I'd
> MOZ_ALWAYS_TRUE that.

Ok, but I had to convert the hash table lookup too.

> 
> @@ +1990,5 @@
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      return rv;
> > +    }
> > +
> > +    nsCOMPtr<nsIFile> childDirectory = do_QueryInterface(entry);
> 
> MOZ_ASSERT

Ok.

> 
> @@ +2013,5 @@
> > +          leafName.EqualsLiteral(DSSTORE_FILE_NAME)) {
> > +        continue;
> > +      }
> > +
> > +      NS_WARNING("Something in the repository that doesn't belong!");
> 
> Might be good to put the leafName in the warning.

Ok.

> 
> ::: dom/quota/QuotaManager.h
> @@ +64,5 @@
> > +  , mIsApp(aIsApp)
> > +  { }
> > +
> > +  PersistenceType mPersistenceType;
> > +  nsCString mOrigin;
> 
> This packs better if origin is first.

Ok.

> 
> @@ +93,5 @@
> >    typedef void
> >    (*WaitingOnStoragesCallback)(nsTArray<nsCOMPtr<nsIOfflineStorage> >&, void*);
> >  
> > +  typedef nsClassHashtable<nsCStringHashKey,
> > +                           nsTArray<nsIOfflineStorage*>> LiveStorageIndex;
> 
> "Index"? How about "Table" or "Map"

Ok, "Table" sounds good.

> 
> ::: dom/quota/QuotaObject.cpp
> @@ +296,5 @@
> > +bool
> > +OriginInfo::IsTreatedAsPersistent()
> > +{
> > +  if (mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_PERSISTENT && (mIsApp ||
> > +      mOrigin.Equals("chrome") || mOrigin.Equals("moz-safe-about:home"))) {
> 
> Whitelisting origins like this isn't really a great solution... Is there
> nothing else we can do here?

Proper default storage won't need this. Those origins will be moved to explicit persistent
storage repository during repository upgrade. The condition will look like this:

if (persistenceType == PERSISTENCE_TYPE_PERSISTENT ||
    (persistenceType == PERSISTENCE_TYPE_DEFAULT && isApp) {
  return true;
}

However, explicit persistent storage will probably enable the first prompt, so we will have
to do something with about:home. Maybe just preset the unlimited storage permission for
about:home somehow.

> 
> Nit: just do:
> 
>   return cond1 &&
>          (cond2 ||
>           cond3 ||
>           cond4);

Ok.

> 
> @@ +421,5 @@
> > +  uint64_t usage = 0;
> > +
> > +  if (mPersistenceType == PERSISTENCE_TYPE_TEMPORARY) {
> > +    usage = mUsage;
> > +  } else {
> 
> Hrm, why wouldn't you always loop through the other origininfos looking for
> temporary usage?

Ok, just in case temporary is treated as persistent :)
Fixed LockedGetTemporaryUsage() and also LockedGetTemporaryOriginInfos()

> 
> @@ +468,5 @@
> >  
> >    for (uint32_t index = mOriginInfos.Length(); index > 0; index--) {
> > +    OriginInfo* originInfo = mOriginInfos[index - 1];
> > +    if (originInfo->IsTreatedAsTemporary()) {
> > +      mUsage -= originInfo->mUsage;
> 
> Seems smart to MOZ_ASSERT(mUsage >= originInfo->mUsage) before subtracting
> here.

Ok.

> 
> @@ +476,2 @@
> >  
> > +      quotaManager->mTemporaryStorageUsage -= originInfo->mUsage;
> 
> And similar here.

Ok.

> 
> ::: dom/quota/QuotaObject.h
> @@ +94,5 @@
> > +  bool
> > +  IsTreatedAsPersistent();
> > +
> > +  bool
> > +  IsTreatedAsTemporary()
> 
> Both methods can be const

Ok.

> 
> @@ +136,5 @@
> >    nsCString mOrigin;
> >    uint64_t mLimit;
> >    uint64_t mUsage;
> >    int64_t mAccessTime;
> > +  bool mIsApp;
> 
> These can all be const right?

only mOrigin and mLimit
(In reply to Jan Varga [:janv] from comment #20)
> I couldn't find an API in gecko that does something like "touch -t".

nsIFile has a setter for lastModifiedDate.
Posted patch patchSplinter Review
I think I addressed all review comments.
Attachment #8512116 - Attachment is obsolete: true
Attachment #8513588 - Attachment is obsolete: true
Attachment #8514261 - Attachment is obsolete: true
Attachment #8514263 - Attachment is obsolete: true
Attachment #8515043 - Flags: review?(bent.mozilla)
Posted patch interdiffSplinter Review
Comment on attachment 8515043 [details] [diff] [review]
patch

I already stamped assuming you would :)
Attachment #8515043 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/45dad11b6580
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1093067
Depends on: 1093223
Backed out for causing bug 1093067 and bug 1093223
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b03757d6c99
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla36 → ---
gerard-majax, could you please apply "patch" and "additional patch" and test it with your old profile ?
Attachment #8516247 - Flags: review?(bent.mozilla)
(In reply to Jan Varga [:janv] from comment #32)
> gerard-majax, could you please apply "patch" and "additional patch" and test
> it with your old profile ?

Sure, I'm rebuilding my Flame, and I'll apply those.
Flags: needinfo?(lissyx+mozillians)
Comment on attachment 8516247 [details] [diff] [review]
additional patch

With this additionnal patch, I don't see the bunch of UnknownErr messages in logcat, and my device properly boots with a profile that previously exposed the bad issue :)
Flags: needinfo?(lissyx+mozillians)
Attachment #8516247 - Flags: feedback+
Comment on attachment 8516247 [details] [diff] [review]
additional patch

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

Ok

::: dom/quota/QuotaManager.cpp
@@ +630,5 @@
>  
> +bool
> +IsApp(const nsCString& aOrigin)
> +{
> +  int32_t pos = aOrigin.Find("+f+");

It seems smarter to first check that the first char is a digit [0-9], otherwise you scan the whole string for all non-apps.

@@ +632,5 @@
> +IsApp(const nsCString& aOrigin)
> +{
> +  int32_t pos = aOrigin.Find("+f+");
> +  if (pos != kNotFound) {
> +    nsCString appIdStr(Substring(aOrigin, 0, pos));

I don't think this extra nsCString isn't needed. Can you just do:

  int32_t appId = Substring(aOrigin, 0, pos).ToInteger(&rv);
Attachment #8516247 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/2266676732ef
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #36)
> Comment on attachment 8516247 [details] [diff] [review]
> additional patch
> 
> Review of attachment 8516247 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok
> 
> ::: dom/quota/QuotaManager.cpp
> @@ +630,5 @@
> >  
> > +bool
> > +IsApp(const nsCString& aOrigin)
> > +{
> > +  int32_t pos = aOrigin.Find("+f+");
> 
> It seems smarter to first check that the first char is a digit [0-9],
> otherwise you scan the whole string for all non-apps.

Ok, added the extra check.

> 
> @@ +632,5 @@
> > +IsApp(const nsCString& aOrigin)
> > +{
> > +  int32_t pos = aOrigin.Find("+f+");
> > +  if (pos != kNotFound) {
> > +    nsCString appIdStr(Substring(aOrigin, 0, pos));
> 
> I don't think this extra nsCString isn't needed. Can you just do:
> 
>   int32_t appId = Substring(aOrigin, 0, pos).ToInteger(&rv);

Unfortunately that produces: error: no member named 'ToInteger' in 'nsDependentCSubstring'
Ok, so if the patch sticks on m-c, we plan to get an approval for aurora and land it there in 10-14 days. The uplift to beta is in 20 days.
We also aim to finish/review/land full default storage support before current m-c is uplifted to aurora. So the intermediate solution would only appear in FF 35.
Approval Request Comment
[Feature/regressing bug #]: Humble Bundle games get QuotaExceededError at 50mb, no UI prompt, bug 1083852.
[User impact if declined]: Humble Bundle games stop working (after they store 50mb of data), but other sites can have similar problems. Basically, we currently allow just 50mb of data to be stored for persistent storage.
[Describe test coverage new/current, TBPL]: The fix landed on m-c on Nov 4th, 2014. The origin eviction test for temporary storage now also tests persistent storage.
[Risks and why]: To be honest this patch is quite big and changes the way how we treat persistent storage. However other options like implementing the 50mb prompt for PBackground or reverting entire IndexedDB on PBackground are even more risky.
[String/UUID change made/needed]: No string/UUID changes.
Attachment #8523204 - Flags: approval-mozilla-aurora?
Comment on attachment 8523204 [details] [diff] [review]
patch for aurora

Do we have any idea how many users this is actually affecting?  I'm not clear on the reason to take risk in Aurora this late in the cycle and would prefer to just let this ride the trains.
Flags: needinfo?(Jan.Varga)
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #42)
> Comment on attachment 8523204 [details] [diff] [review]
> patch for aurora
> 
> Do we have any idea how many users this is actually affecting?  I'm not
> clear on the reason to take risk in Aurora this late in the cycle and would
> prefer to just let this ride the trains.

As far as I know, breaking Humble Bundle is a big deal, since we're promoting it, etc.
luke, overholt or bent know more about it
Flags: needinfo?(Jan.Varga)
I think the risk of not taking the patch here is greater than letting the regression surface to users when it hits release.  Waiting this late in the Aurora cycle was in part due to not wanting to destabilize Aurora around Firefox's 10th anniversary.
Comment on attachment 8523204 [details] [diff] [review]
patch for aurora

OK, well we do have an extra week of Aurora for 35 so let's get it in.
Attachment #8523204 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.