1089764 - Treat persistent storage as temporary storage

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

Attachment: patch

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 1

[Jan Varga [:janv]]

Comment on attachment 8512116 [details] [diff] [review]
patch

I think it's ready for an initial review.

Comment 2

[Jan Varga [:janv]]

Oh, forgot to say that the patch is based on m-c plus the patch for bug 1068787.

Comment 3

[Jan Varga [:janv]]

https://tbpl.mozilla.org/?tree=Try&rev=dc305d0e95b6

Comment 4

[Jan Varga [:janv]]

Comment on attachment 8512116 [details] [diff] [review]
patch

Luke, can you take a look at asmjscache changes too ?

Comment 5

[Luke Wagner [: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);

Comment 6

[Jan Varga [:janv]]

Attachment: patch v2

Comment 7

[Jan Varga [:janv]]

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

Comment 8

[Jan Varga [:janv]]

Attachment: patch v3

Fixed a bug in EnsureOriginIsInitialized()

Comment 9

[Jan Varga [:janv]]

Attachment: interdiff

simplification of patch v3 for aurora

Comment 10

[Jan Varga [:janv]]

Attachment: folded patch

patch v3 + simplification folded into one patch

Comment 11

[Jan Varga [:janv]]

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

Comment 12

[Jan Varga [:janv]]

It seems that origin clearing code is not right. I'll fix it tomorrow.

Comment 13

[Jan Varga [:janv]]

Attachment: interdiff

Comment 14

[Jan Varga [:janv]]

Attachment: folded patch

Comment 15

[Jan Varga [:janv]]

https://tbpl.mozilla.org/?tree=Try&rev=38a580f664dd

It now passes on try.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 17

[Jan Varga [:janv]]

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 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 19

[Jan Varga [:janv]]

Updating the summary since the simplified patch doesn't check app status.

Comment 20

[Jan Varga [:janv]]

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

Comment 21

[Jan Varga [:janv]]

(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

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 23

[Jan Varga [:janv]]

Attachment: patch

I think I addressed all review comments.

Comment 24

[Jan Varga [:janv]]

Attachment: interdiff

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8515043 [details] [diff] [review]
patch

I already stamped assuming you would :)

Comment 26

[Jan Varga [:janv]]

https://tbpl.mozilla.org/?tree=Try&rev=1ccd8f4b273d

Comment 27

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45dad11b6580

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/45dad11b6580

Comment 29

[Jan Varga [:janv]]

Backed out for causing bug 1093067 and bug 1093223
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b03757d6c99

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/9b03757d6c99

Comment 31

[Jan Varga [:janv]]

Attachment: additional patch

Comment 32

[Jan Varga [:janv]]

gerard-majax, could you please apply "patch" and "additional patch" and test it with your old profile ?

Comment 33

[Jan Varga [:janv]]

https://tbpl.mozilla.org/?tree=Try&rev=ff47dcd225db

Comment 34

[:gerard-majax]

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

Comment 35

[:gerard-majax]

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 :)

Comment 36

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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);

Comment 37

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2266676732ef

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2266676732ef

Comment 39

[Jan Varga [:janv]]

(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'

Comment 40

[Jan Varga [:janv]]

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.

Comment 41

[Jan Varga [:janv]]

Attachment: patch for aurora

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.

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 43

[Jan Varga [:janv]]

(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

Comment 44

[Andrew Overholt [:overholt]]

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 45

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 46

[Jan Varga [:janv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4cd2a209317