Closed Bug 1083927 Opened 10 years ago Closed 10 years ago

IndexedDB: Subdomain Quota Management

Categories

(Core :: Storage: IndexedDB, defect)

Product:
Core
Component:
Storage: IndexedDB
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: bent.mozilla, Assigned: janv)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

file listing of data/local/storage
(deleted), text/plain
Details
file listing of <profile>/storage (Firefox Desktop)
(deleted), text/plain
Details
patch v3
(deleted), patch
ehsan.akhgari
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
interdiff
(deleted), patch
Details | Diff | Splinter Review 
From sicking:

* For databases created using indexedDB.open("db", version)
  store the data in the "storage/default/<origin>/idb" folder
* For databases created using indexedDB.open("db", { version: version, storage: "temporary" })
  store the data in the "storage/temporary/<origin>/idb" folder
* For databases created using indexedDB.open("db", { version: version, storage: "persistent" })
  store the data in the "storage/persist/<origin>/idb" folder
* Delete all data in indexedDB data in "storage/temporary/*/" but not in "storage/temporary/*/idb".
  It's very unlikely that there will be any.
* Migrate any indexedDB data from "storage/persistent/<origin>/" to "storage/default/<origin>/idb".
* Make sure that we create .manifest files for all "storage/default/<origin>/",
  "storage/temporary/<origin>/" and "storage/persist/<origin>/" folders. Including when doing the
   migration in the previous bullet point.
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #0)
> From sicking:
> * Delete all data in indexedDB data in "storage/temporary/*/" but not in "storage/temporary/*/idb".
>   It's very unlikely that there will be any.

Not sure, why we need to do this, the only quota client is asmjscache which currently stores stuff under storage/temporary.

> * Migrate any indexedDB data from "storage/persistent/<origin>/" to "storage/default/<origin>/idb".

Yes, but I actually just renamed storage/persistent to storage/default.

Initial patch coming ...
(In reply to Jan Varga [:janv] from comment #1)
> Not sure, why we need to do this, the only quota client is asmjscache which
> currently stores stuff under storage/temporary.
> 

There is of course IDB, so two quota clients.
Attached patch initial patch (obsolete) (deleted) — — Splinter Review 

    
    

    
  
Also, I'm not sure what we should do with the artificial urls indexeddb:// which are used by addon sdk.
Should we move them to explicit persistent storage during the upgrade ?
(we would also need to update all consumers to use explicit persistent storage)

    
    

    
  
(In reply to Jan Varga [:janv] from comment #1)
> (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #0)
> > From sicking:
> > * Delete all data in indexedDB data in "storage/temporary/*/" but not in "storage/temporary/*/idb".
> >   It's very unlikely that there will be any.
> 
> Not sure, why we need to do this, the only quota client is asmjscache which
> currently stores stuff under storage/temporary.

If it's less than 5 lines of code, then sure, move the asmjscache data to the new storage/temp folder. But otherwise it doesn't seem worth it.

    
    

    
  
Ryan, do you have any opinion on indexeddb:// origins ?
See comment 4.

    
    

    
  
(In reply to Jan Varga [:janv] from comment #4)
> Also, I'm not sure what we should do with the artificial urls indexeddb://
> which are used by addon sdk.
> Should we move them to explicit persistent storage during the upgrade ?
> (we would also need to update all consumers to use explicit persistent
> storage)

Is there a difference in functionality depending on whether they are moved or not?

Does explicit persistent storage behave differently than whatever other bucket ("default", I guess?) they would be in if not moved?

From your patch, it looks like requesting "explicit persistent" moves the storage to a new "permanent" directory (though that does not seem to be mentioned in comment 0)?  It looks like the existing "persistent" directory would go away entirely, so the choices are between "default" and "permanent" now?
Flags: needinfo?(Jan.Varga)

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] from comment #7)
> (In reply to Jan Varga [:janv] from comment #4)
> > Also, I'm not sure what we should do with the artificial urls indexeddb://
> > which are used by addon sdk.
> > Should we move them to explicit persistent storage during the upgrade ?
> > (we would also need to update all consumers to use explicit persistent
> > storage)
> 
> Is there a difference in functionality depending on whether they are moved
> or not?

storage/default is treated as persistent storage for apps and as temporary storage otherwise.
So for example, if indexeddb+++fx-devtools is not moved to storage/permanent, quota manager can delete indexeddb++fx-devtools if temporary storage is full and indexeddb++fx-devtools is one of the least recently used origins.

> 
> Does explicit persistent storage behave differently than whatever other
> bucket ("default", I guess?) they would be in if not moved?
> 
> From your patch, it looks like requesting "explicit persistent" moves the
> storage to a new "permanent" directory (though that does not seem to be
> mentioned in comment 0)?  It looks like the existing "persistent" directory
> would go away entirely, so the choices are between "default" and "permanent"
> now?

Yeah, the upgrade is easier when we just stop using storage/persistent.

The choices are:

Explicit persistent storage:
indexedDB.open(NAME, { version: VERSION, storage: "persistent" });
This goes to storage/permanent

Default storage:
indexedDB.open(NAME, VERSION);
or
indexedDB.open(NAME, { version: VERSION, storage: "default" });
This goes to storage/default

Explicit temporary storage:
indexedDB.open(NAME, { version: VERSION, storage: "temporary" });
This goes to storage/temporary

All three storage repositories are independent, so you can have three databases with the same name.
Flags: needinfo?(Jan.Varga)

    
    

    
  
(In reply to Jan Varga [:janv] from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] from comment #7)
> > (In reply to Jan Varga [:janv] from comment #4)
> > > Also, I'm not sure what we should do with the artificial urls indexeddb://
> > > which are used by addon sdk.
> > > Should we move them to explicit persistent storage during the upgrade ?
> > > (we would also need to update all consumers to use explicit persistent
> > > storage)
> > 
> > Is there a difference in functionality depending on whether they are moved
> > or not?
> 
> storage/default is treated as persistent storage for apps and as temporary
> storage otherwise.
> So for example, if indexeddb+++fx-devtools is not moved to
> storage/permanent, quota manager can delete indexeddb++fx-devtools if
> temporary storage is full and indexeddb++fx-devtools is one of the least
> recently used origins.

Okay, well that decides it then, you should move SDK consumers to explicit persistent storage.

SDK consumers would generally expect the same treatment as browser chrome code (like about:home, which you've changed to explicit persistent already).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Fixed remaining XXX and added a test for the upgrade.

        Attachment #8523862 -
        Attachment is obsolete: true

        Attachment #8524738 -
        Flags: review?(bent.mozilla)

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] from comment #9)
> (In reply to Jan Varga [:janv] from comment #8)
> > (In reply to J. Ryan Stinnett [:jryans] from comment #7)
> > > (In reply to Jan Varga [:janv] from comment #4)
> > > > Also, I'm not sure what we should do with the artificial urls indexeddb://
> > > > which are used by addon sdk.
> > > > Should we move them to explicit persistent storage during the upgrade ?
> > > > (we would also need to update all consumers to use explicit persistent
> > > > storage)
> > > 
> > > Is there a difference in functionality depending on whether they are moved
> > > or not?
> > 
> > storage/default is treated as persistent storage for apps and as temporary
> > storage otherwise.
> > So for example, if indexeddb+++fx-devtools is not moved to
> > storage/permanent, quota manager can delete indexeddb++fx-devtools if
> > temporary storage is full and indexeddb++fx-devtools is one of the least
> > recently used origins.
> 
> Okay, well that decides it then, you should move SDK consumers to explicit
> persistent storage.
> 
> SDK consumers would generally expect the same treatment as browser chrome
> code (like about:home, which you've changed to explicit persistent already).

Ok, I found just one consumer in mozilla-central: browser/devtools/app-manager/app-projects.js

I'm not sure how we should proceed with other SDK consumers if we move all indexeddb:// origins to explicit persistent storage. If they don't update their code to pass { version: VERSION, storage: "persistent" } along with the move then they will lose all previously stored data.
On the other hand if we don't move all indexeddb:// then they will have to do it on their own which is quite painful.

Myk, I don't know much about the SDK, would it possible to just announce that all non m-c consumers of sdk/indexed-db have to update their code to require persistent storage explicitly ?
We plan to move indexeddb:// origins to explicit persistent storage in FF 36.
Flags: needinfo?(myk)

    
    

    
  
Alexandre, could you please test the attached patch with your old profile. It works ok on my flame, but my profile is not so big as yours. Thanks.
Flags: needinfo?(lissyx+mozillians)

    
    

    
  

      
      
      
      

        Attached file
          
        file listing of data/local/storage (deleted)
        — 
      

    


  
Jonas, just to be sure, here's a listing of data/local/storage after it was upgraded for default storage. Can you take a look ?
Flags: needinfo?(jonas)

    
    

    
  


  
And this is how it looks in Firefox Desktop.

    
    

    
  
(In reply to Jan Varga [:janv] from comment #12)
> (In reply to J. Ryan Stinnett [:jryans] from comment #9)
> > Okay, well that decides it then, you should move SDK consumers to explicit
> > persistent storage.
> > 
> > SDK consumers would generally expect the same treatment as browser chrome
> > code (like about:home, which you've changed to explicit persistent already).
> 
> Ok, I found just one consumer in mozilla-central:
> browser/devtools/app-manager/app-projects.js
> 
> I'm not sure how we should proceed with other SDK consumers if we move all
> indexeddb:// origins to explicit persistent storage. If they don't update
> their code to pass { version: VERSION, storage: "persistent" } along with
> the move then they will lose all previously stored data.
> On the other hand if we don't move all indexeddb:// then they will have to
> do it on their own which is quite painful.
> 
> Myk, I don't know much about the SDK, would it possible to just announce
> that all non m-c consumers of sdk/indexed-db have to update their code to
> require persistent storage explicitly ?
> We plan to move indexeddb:// origins to explicit persistent storage in FF 36.

SDK consumers should all go through a wrapper module[1].  I believe you could update that module itself, and then SDK consumers wouldn't need any changes.

[1]: http://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/indexed-db.js#39-44

    
    

    
  
(In reply to Jan Varga [:janv] from comment #13)
> Alexandre, could you please test the attached patch with your old profile.
> It works ok on my flame, but my profile is not so big as yours. Thanks.

What should I test ? Just applying the patch, and booting with it is enough?
Flags: needinfo?(lissyx+mozillians) → needinfo?(Jan.Varga)

    
    

    
  
(In reply to Alexandre LISSY :gerard-majax from comment #17)
> (In reply to Jan Varga [:janv] from comment #13)
> > Alexandre, could you please test the attached patch with your old profile.
> > It works ok on my flame, but my profile is not so big as yours. Thanks.
> 
> What should I test ? Just applying the patch, and booting with it is enough?

Yes, booting with it should be enough.
Flags: needinfo?(Jan.Varga)

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] from comment #16)
> SDK consumers should all go through a wrapper module[1].  I believe you
> could update that module itself, and then SDK consumers wouldn't need any
> changes.
> 
> [1]:
> http://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/
> indexed-db.js#39-44

Ok, I'll try to do it in the wrapper somehow.

    
    

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

> Myk, I don't know much about the SDK, would it possible to just announce
> that all non m-c consumers of sdk/indexed-db have to update their code to
> require persistent storage explicitly ?
> We plan to move indexeddb:// origins to explicit persistent storage in FF 36.

I'm not actually involved in SDK development any longer, so I'm not the right person to answer this question!


(In reply to J. Ryan Stinnett [:jryans] from comment #16)

> SDK consumers should all go through a wrapper module[1].  I believe you
> could update that module itself, and then SDK consumers wouldn't need any
> changes.

That sounds right to me, but requesting needinfo from Dave Townsend for confirmation (or redirection to another member of his SDK development team).
Flags: needinfo?(myk) → needinfo?(dtownsend+bugmail)

    
    

    
  
Only comment I have is that usually we've used the term "persistent" rather than "permanent". I suspect that if we get a spec which exposes this stuff it'll use "persistent" as term. But it's obviously not a big deal if the spec and our directory structure don't use the exact same terminology.
Flags: needinfo?(jonas)

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #21)
> Only comment I have is that usually we've used the term "persistent" rather
> than "permanent". I suspect that if we get a spec which exposes this stuff
> it'll use "persistent" as term. But it's obviously not a big deal if the
> spec and our directory structure don't use the exact same terminology.

Yeah, originally I wanted to use "persistant" for the directory name which is a common misspelling of "persistent", but bent didn't like it :) I got inspired by "network" vs "netwerk" :)
It would be possible to keep the original name, but it's not worth in my opinion because it complicates code (needs to be crash resistant) for almost no benefit and typical users nor developers won't notice it at all.

    
    

    
  
Comment on attachment 8524738 [details] [diff] [review]
patch v1

Restored my very old profile on my Flame with this patch applied, it booted properly.

        Attachment #8524738 -
        Flags: feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Enhanced indexeddb:// handling.

        Attachment #8524738 -
        Attachment is obsolete: true

        Attachment #8524738 -
        Flags: review?(bent.mozilla)

        Attachment #8526125 -
        Flags: review?(bent.mozilla)
Flags: needinfo?(dtownsend+bugmail) → needinfo?(evold)

    
    

    
  
(In reply to Myk Melez [:myk] [@mykmelez] from comment #20)
> > SDK consumers should all go through a wrapper module[1].  I believe you
> > could update that module itself, and then SDK consumers wouldn't need any
> > changes.
> 
> That sounds right to me, but requesting needinfo from Dave Townsend for
> confirmation (or redirection to another member of his SDK development team).

patch v2 changes indexed-db.js module to use explicit persistent storage by default

    
    

    
  
Comment 16 sounds alright to me, does this sound alright to you Irakli?
Flags: needinfo?(evold) → needinfo?(rFobic)

    
    

    
  
Comment on attachment 8526125 [details] [diff] [review]
patch v2

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

All the browser/ stuff needs review from someone on the browser team. I'd suggest :dao or :gavin, and please explain to them our small time window.

I'm not quite done with the QuotaManager changes yet.

::: addon-sdk/source/lib/sdk/indexed-db.js
@@ +46,5 @@
> +  } else {
> +    args = [principal].concat(toArray(args));
> +  }
> +  if (args.length == 2) {
> +    args.push({ storage: "persistent" });

This is wrong, there is a two arg version of deleteDatabase. Are there tests for this stuff?

::: browser/base/content/pageinfo/pageInfo.xul
@@ +274,2 @@
>            </vbox>
> +          <button id="indexedDBClear" label="&permClearStorage;"

This looks like it needs to be reverted...

::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
@@ +20,5 @@
>  [include:xpcshell-shared.ini]
>  
>  [test_blob_file_backed.js]
>  [test_bug1056939.js]
> +[test_bug1083927.js]

Can this be named something better?

::: gfx/skia/trunk/include/core/SkCanvas.h
@@ +362,5 @@
>                       offscreen when restore() is called
>          @param flags  LayerFlags
>          @return The value to pass to restoreToCount() to balance this save()
>      */
> +//    SK_ATTR_EXTERNALLY_DEPRECATED("SaveFlags use is deprecated")

???

    
    

    
  
Comment on attachment 8526125 [details] [diff] [review]
patch v2

Dao or Gavin, could you please take a look at browser/ changes in this patch.
We removed the first prompt for IndexedDB in bug 758357. We are now enabling it again for explicitly persistent storage. The other change in browser/ is that about home needs to request explicit persistent storage to not participate in temporary storage (otherwise data for about:gome could be theoretically deleted).
We need to land this patch before m-c is uplifted to Aurora, because current Aurora contains a simplified (a bit hacky) version of default storage and we promised to ship it in Firefox 35 only.
Sorry for requesting this review just a few days before the uplift, but the situation is quite challenging since we had to land the simplified version on trunk first, then request approval for aurora and land it there and bent is also working hard to deliver IndexedDB on workers before the uplift.

        Attachment #8526125 -
        Flags: review?(gavin.sharp)

        Attachment #8526125 -
        Flags: review?(dao)

    
    

    
  
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #27)
> Comment on attachment 8526125 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8526125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All the browser/ stuff needs review from someone on the browser team. I'd
> suggest :dao or :gavin, and please explain to them our small time window.

Done.

> 
> I'm not quite done with the QuotaManager changes yet.
> 
> ::: addon-sdk/source/lib/sdk/indexed-db.js
> @@ +46,5 @@
> > +  } else {
> > +    args = [principal].concat(toArray(args));
> > +  }
> > +  if (args.length == 2) {
> > +    args.push({ storage: "persistent" });
> 
> This is wrong, there is a two arg version of deleteDatabase. Are there tests
> for this stuff?

I'll add some tests, but I'm not aware of a two arg version of deleteDatabase, will ping you on IRC.

> 
> ::: browser/base/content/pageinfo/pageInfo.xul
> @@ +274,2 @@
> >            </vbox>
> > +          <button id="indexedDBClear" label="&permClearStorage;"
> 
> This looks like it needs to be reverted...

Oh, I overlooked that.

> 
> ::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
> @@ +20,5 @@
> >  [include:xpcshell-shared.ini]
> >  
> >  [test_blob_file_backed.js]
> >  [test_bug1056939.js]
> > +[test_bug1083927.js]
> 
> Can this be named something better?

Ok, renamed to test_defaultStorageUpgrade.js

> 
> ::: gfx/skia/trunk/include/core/SkCanvas.h
> @@ +362,5 @@
> >                       offscreen when restore() is called
> >          @param flags  LayerFlags
> >          @return The value to pass to restoreToCount() to balance this save()
> >      */
> > +//    SK_ATTR_EXTERNALLY_DEPRECATED("SaveFlags use is deprecated")
> 
> ???

Oops, forgot to remove it.

    
    

    
  
Comment on attachment 8526125 [details] [diff] [review]
patch v2

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

This looks ok to me!

::: addon-sdk/source/lib/sdk/indexed-db.js
@@ +46,5 @@
> +  } else {
> +    args = [principal].concat(toArray(args));
> +  }
> +  if (args.length == 2) {
> +    args.push({ storage: "persistent" });

This is wrong, there is a two arg version of deleteDatabase. Are there tests for this stuff?

::: browser/base/content/pageinfo/pageInfo.xul
@@ +274,2 @@
>            </vbox>
> +          <button id="indexedDBClear" label="&permClearStorage;"

This looks like it needs to be reverted...

::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
@@ +20,5 @@
>  [include:xpcshell-shared.ini]
>  
>  [test_blob_file_backed.js]
>  [test_bug1056939.js]
> +[test_bug1083927.js]

Can this be named something better?

::: dom/quota/QuotaManager.cpp
@@ +594,5 @@
> +
> +  nsCOMPtr<nsIFile> mDirectory;
> +  mozilla::Mutex& mMutex;
> +  mozilla::CondVar mCondVar;
> +  nsresult mResultCode;

Maybe mMainThreadResultCode?

@@ +626,5 @@
> +  NS_IMETHOD
> +  Run();
> +};
> +
> +struct StorageDirectoryHelper::OriginProps

This struct looks like it packs pretty inefficiently. Can you reorder the members to pack better?

@@ +639,5 @@
> +
> +  Type mType;
> +  nsCString mSpec;
> +  uint32_t mAppId;
> +  bool mInMozBrowser;

No constructor to initialize your POD members?

@@ +685,5 @@
> +  bool mMaybeDriveLetter;
> +  bool mError;
> +
> +public:
> +  OriginParser(const nsAString& aOrigin)

This needs explicit.

@@ +706,5 @@
> +    MOZ_ASSERT(aAppId);
> +    MOZ_ASSERT(!mError);
> +
> +    if (mAppId.IsNull()) {
> +      *aAppId = nsIScriptSecurityManager::NO_APP_ID;

Does mAppId really need to be a Nullable<> since you have the NO_APP_ID value?

@@ +719,5 @@
> +    MOZ_ASSERT(aInMozBrowser);
> +    MOZ_ASSERT(!mError);
> +
> +    if (mInMozBrowser.IsNull()) {
> +      *aInMozBrowser = false;

Same here... Why Nullable<> ?

@@ +726,5 @@
> +    }
> +  }
> +
> +  void
> +  GetSpec(nsCString& aSpec);

Why are these not nsACString& ?

@@ +733,5 @@
> +  GetResult(nsCString& aResult);
> +
> +private:
> +  void
> +  HandleSchema(nsAutoString& aSchema);

I think these should all take |const nsDependentSubstring&| to avoid stack copying.

@@ +745,5 @@
> +
> +class OriginKey : public nsAutoCString
> +{
> +public:
> +  explicit

Not needed for multi-arg constructors.

@@ +811,5 @@
> +  return false;
> +}
> +
> +bool
> +ParseOriginString(const nsAString& aOrigin,

This is needless indirection. Just make this a static functiona and pass the args directly into the OriginParser constructor to avoid all the GetFoo() calls below.

@@ +838,5 @@
> +bool
> +IsPersistentOriginWhitelisted(const nsACString& aOrigin)
> +{
> +  if (aOrigin.Equals(kChromeOrigin) ||
> +      aOrigin.Equals(kAboutHomeOrigin) ||

Both of these should be EqualsLiteral.

@@ +847,5 @@
> +  return false;
> +}
> +
> +nsresult
> +InitStoragePath(nsIFile* aBaseDir,

Maybe call this CloneStoragePath?

@@ +1399,5 @@
>      rv = baseDir->GetPath(mStoragePath);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = InitStoragePath(baseDir,
> +                         NS_LITERAL_STRING("persistent"),

These should be const's at the top of the file, and then you don't have to ask for leafName in some spots below.

@@ +2451,5 @@
>                                          nsIFile** aDirectory)
>  {
>    AssertIsOnIOThread();
>  
> +  nsresult rv = InitializeStorageArea();

How about we call this something like MaybeUpgradeStorageArea?

@@ +2529,5 @@
> +      uint64_t ts;
> +      rv = stream->Read64(&ts);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      timestamp = ts;

Maybe assert that this is <= PR_Now

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

Just MOZ_ASSERT

@@ +4998,5 @@
> +        originProps->mType = OriginProps::eChrome;
> +        originProps->mSpec = kChromeOrigin;
> +
> +        int64_t timestamp;
> +        rv = GetLastModifiedTime(originDir, &timestamp);

Why do we care about a timestamp on persistent chrome? Can't we just set to 0 or something?

@@ +5005,5 @@
> +        }
> +
> +        originProps->mTimestamp = timestamp;
> +      } else {
> +        NS_WARNING("chrome in temporary storage directory?!");

This can happen, can't it? If chrome explicitly asks for storage:persistent?

@@ +5018,5 @@
> +      }
> +
> +      OriginProps* originProps = mOriginProps.AppendElement();
> +      originProps->mDirectory = originDir;
> +      originProps->mType = OriginProps::eContent;

What about about:home?

@@ +5041,5 @@
> +  }
> +
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this)));
> +
> +  mozilla::MutexAutoLock autolock(mMutex);

You have to scope this lock better, you're doing lots of I/O with it still locked. This will cause any other thread trying to grab the quota mutex to block on your I/O!

Additional question: Why are you using the quota mutex here? Is there some other synchronization going on here that could possibly interfere with other quota shared state?

And another: You're using the quota mutex on the main thread after some amount of delay from when the QuotaManager dispatched this runnable initially. Events could have run in between, so what guarantees that the QuotaManager and its mutex are still alive when you get back to the main thread?

@@ +5115,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +StorageDirectoryHelper::RunOnMainThread()

Maybe MOZ_ASSERT(!mOriginProps.IsEmpty()) here, and of course MOZ_ASSERT(NS_IsMainThread())

@@ +5212,5 @@
> +
> +bool
> +OriginParser::Parse()
> +{
> +  nsCharSeparatedTokenizerTemplate<TokenizerIgnoreNothing>

Is there a reason you're using the utf16 version of this tokenizer? I see you converting all the path components to UTF8, why not just convert the whole string first in one go and then use nsCCharSeparatedTokenizerTemplate here?

@@ +5217,5 @@
> +    tokenizer(mOrigin, '+');
> +
> +  nsAutoString token;
> +  while (tokenizer.hasMoreTokens() && !mError) {
> +    token = tokenizer.nextToken();

This copies the string into the stack each time... Can't you just pass the result of nextToken() directly into HandleToken?

@@ +5228,5 @@
> +  }
> +
> +  if (tokenizer.separatorAfterCurrentToken()) {
> +    nsAutoString emptyString;
> +    HandleToken(emptyString);

Can't you just make a HandleEmptyToken() function to avoid a new object here?

@@ +5235,5 @@
> +  if (mError) {
> +    return false;
> +  }
> +
> +  return true;

MOZ_ASSERT(mState == eComplete)

@@ +5245,5 @@
> +  MOZ_ASSERT(!mError);
> +
> +  nsAutoCString spec;
> +
> +  spec.Append(mSchema);

Just pass mSchema as the constructor arg for spec.

@@ +5247,5 @@
> +  nsAutoCString spec;
> +
> +  spec.Append(mSchema);
> +  if (mSchemaType == eFile) {
> +    spec.Append(NS_LITERAL_CSTRING("://"));

AppendLiteral()

@@ +5263,5 @@
> +
> +  if (mSchemaType == eMozSafeAbout) {
> +    spec.Append(':');
> +  } else {
> +    spec.Append(NS_LITERAL_CSTRING("://"));

AppendLiteral

@@ +5308,5 @@
> +  if (!aToken.EqualsLiteral("http") &&
> +      !aToken.EqualsLiteral("https") &&
> +      !aToken.EqualsLiteral("indexeddb") &&
> +      !aToken.EqualsLiteral("file") &&
> +      !aToken.EqualsLiteral("moz-safe-about") &&

We expect this to be more common than file, and maybe indexeddb, so move it up the list.

@@ +5316,5 @@
> +  }
> +
> +  mSchema = NS_ConvertUTF16toUTF8(aToken);
> +
> +  if (mSchema.Equals("file")) {

These should all be EqualsLiteral... Though you already called that above. Can you try to make just one strcmp call per schema here?

@@ +5341,5 @@
> +  mState = eExpectingPathnameComponent;
> +}
> +
> +void
> +OriginParser::HandleToken(nsAutoString& aToken)

MOZ_ASSERT(!aToken.IsEmpty()) after you add a HandleEmptyToken function

@@ +5346,5 @@
> +{
> +  switch (mState) {
> +    case eExpectingAppIdOrSchema: {
> +      nsresult rv;
> +      uint32_t appId = aToken.ToInteger(&rv);

Rather than this I think it would be smarter to check IsAsciiDigit(aToken.First()). Then you'd know if you were dealing with app id or schema without first trying to printf.

@@ +5353,5 @@
> +        mState = eExpectingInMozBrowser;
> +        return;
> +      }
> +
> +      return HandleSchema(aToken);

HandleSchema returns void, move the return to the next line.

Same for several other places below.

@@ +5358,5 @@
> +    }
> +
> +    case eExpectingInMozBrowser: {
> +      if (aToken.Length() != 1) {
> +        mError = true;

You should add a NS_WARNING to all these spots where you set mError.

@@ +5427,5 @@
> +
> +      return;
> +    }
> +
> +    case eExpectingPort: {

MOZ_ASSERT(mSchemaType == eNone) here?

@@ +5429,5 @@
> +    }
> +
> +    case eExpectingPort: {
> +      nsresult rv;
> +      uint32_t port = aToken.ToInteger(&rv);

This won't work with nsDependentSubstring so you'll have to assign to a stack string first. But only for this one case.

@@ +5465,5 @@
> +      if (mMaybeDriveLetter && aToken.IsEmpty()) {
> +        MOZ_ASSERT(mPathnameComponents.Length() == 1);
> +
> +        nsCString& pathnameComponent = mPathnameComponents[0];
> +        pathnameComponent = pathnameComponent + NS_LITERAL_CSTRING(":");

pathnameComponent.Append(':')

::: dom/quota/QuotaManager.h
@@ +583,5 @@
>    nsAutoTArray<nsRefPtr<Client>, Client::TYPE_MAX> mClients;
>  
>    nsString mIndexedDBPath;
>    nsString mStoragePath;
>    nsString mPersistentStoragePath;

Why is this needed now?

::: gfx/skia/trunk/include/core/SkCanvas.h
@@ +362,5 @@
>                       offscreen when restore() is called
>          @param flags  LayerFlags
>          @return The value to pass to restoreToCount() to balance this save()
>      */
> +//    SK_ATTR_EXTERNALLY_DEPRECATED("SaveFlags use is deprecated")

???

        Attachment #8526125 -
        Flags: review?(bent.mozilla) → review+

    
    

    
  

      
      
      
      

        Attached file
          
        defaultStorageUpgrade_profile.zip (used to be bug1083927.zip) (obsolete) (deleted)
        — 
      

    


  

    
    

    
  
Ok, thanks, I'll try to address it ASAP.

    
    

    
  
Comment on attachment 8526125 [details] [diff] [review]
patch v2

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

::: browser/base/content/abouthome/aboutHome.js
@@ +225,5 @@
>      gSnippetsMapCallbacks.length = 0;
>    }
>  
> +  let openRequest = indexedDB.open(DATABASE_NAME, {version: DATABASE_VERSION,
> +                                                   storage: DATABASE_STORAGE});

Jan, why is this change needed? Why doesn't the 'default' storage type for about:home automatically get treated as persistent storage?

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #33)
> Comment on attachment 8526125 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8526125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/abouthome/aboutHome.js
> @@ +225,5 @@
> >      gSnippetsMapCallbacks.length = 0;
> >    }
> >  
> > +  let openRequest = indexedDB.open(DATABASE_NAME, {version: DATABASE_VERSION,
> > +                                                   storage: DATABASE_STORAGE});
> 
> Jan, why is this change needed? Why doesn't the 'default' storage type for
> about:home automatically get treated as persistent storage?

Discussed about this on IRC and we concluded that about:home is not chrome so it should request persistent storage explicitly.

    
    

    
  
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #26)
> Comment 16 sounds alright to me, does this sound alright to you Irakli?

Yes that sound right to me as well.
Flags: needinfo?(rFobic)

    
    

    
  
Comment on attachment 8526125 [details] [diff] [review]
patch v2

>diff --git a/browser/base/content/abouthome/aboutHome.js b/browser/base/content/abouthome/aboutHome.js

This change looks fine.

>diff --git a/browser/base/content/pageinfo/pageInfo.xul b/browser/base/content/pageinfo/pageInfo.xul

>-            <label id="indexedDBStatus" control="indexedDBClear" hidden="true"/>
>+            <label id="indexedDBStatus" control="indexedDBClear"/>
>           </vbox>
>-          <button id="indexedDBClear" label="&permClearStorage;" hidden="true"
>+          <button id="indexedDBClear" label="&permClearStorage;"

I really don't understand these changes, they seem to break the default button state.

>diff --git a/browser/base/content/pageinfo/permissions.js b/browser/base/content/pageinfo/permissions.js

I don't understand the rationale behind these changes.

>diff --git a/browser/modules/SitePermissions.jsm b/browser/modules/SitePermissions.jsm

>   "indexedDB": {
>-    states: [ SitePermissions.ALLOW, SitePermissions.UNKNOWN, SitePermissions.BLOCK ],

>     onChange: function (aURI, aState) {
>-      if (aState == SitePermissions.ALLOW || aState == SitePermissions.BLOCK)
>+      if (aState == SitePermissions.BLOCK)
>         Services.perms.remove(aURI.host, "indexedDB-unlimited");
>     }
>   },

The changes in this file look like mostly a reversion of https://hg.mozilla.org/mozilla-central/rev/600aee747c40, but with these two additional changes thrown in. I do not understand them either.

Can you explain?

        Attachment #8526125 -
        Flags: review?(gavin.sharp)

        Attachment #8526125 -
        Flags: review?(dao)

        Attachment #8526125 -
        Flags: review-

    
    

    
  
Is there anything in this patch that could affect Fennec? We were using indexedDB for our Reader code in Fx35, but removed it in Fx36. That's the only chrome code I know of that used indexedDB.

Is there any WebApp installation or permission code that may need to change?

    
    

    
  
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #36)
> Comment on attachment 8526125 [details] [diff] [review]
> patch v2
> 
> >diff --git a/browser/base/content/abouthome/aboutHome.js b/browser/base/content/abouthome/aboutHome.js
> 
> This change looks fine.
> 
> >diff --git a/browser/base/content/pageinfo/pageInfo.xul b/browser/base/content/pageinfo/pageInfo.xul
> 
> >-            <label id="indexedDBStatus" control="indexedDBClear" hidden="true"/>
> >+            <label id="indexedDBStatus" control="indexedDBClear"/>
> >           </vbox>
> >-          <button id="indexedDBClear" label="&permClearStorage;" hidden="true"
> >+          <button id="indexedDBClear" label="&permClearStorage;"
> 
> I really don't understand these changes, they seem to break the default
> button state.

Well, I don't think they break the default state, the original patch for disabling the first prompt added the |hidden="true"| and the patch in this bug just reverted that.
Anyway, I removed this change from the patch in this bug (as suggested by bent too) and thing should be fine now.

> 
> >diff --git a/browser/base/content/pageinfo/permissions.js b/browser/base/content/pageinfo/permissions.js
> 
> I don't understand the rationale behind these changes.

The "indexedDB" permission was temporarily removed (well for about two years), because we needed to disable the first prompt before unprefixing indexedDB (window.mozIndexedDB -> window.indexedDB).
Now we're switching the default storage for indexedDB to a new repository called "default storage", but at the same time we are introducing an explicit persistent storage which must be requested explicitly by passing { storage: "persistent"} to indexedDB.open()
Explicit persistent storage needs to be protected by the first prompt and the first prompt uses the "indexedDB" permission setting to remember decisions made by the user.
The changes in permissions.js just try to restore the state when the first prompt was enabled.

The first prompt was disabled in bug 758357, but some other changes landed in the meantime which touched the same code, so it's not so easy to verify that the patch in this bug just reverts the patch in bug 758357.

> 
> >diff --git a/browser/modules/SitePermissions.jsm b/browser/modules/SitePermissions.jsm
> 
> >   "indexedDB": {
> >-    states: [ SitePermissions.ALLOW, SitePermissions.UNKNOWN, SitePermissions.BLOCK ],
> 
> >     onChange: function (aURI, aState) {
> >-      if (aState == SitePermissions.ALLOW || aState == SitePermissions.BLOCK)
> >+      if (aState == SitePermissions.BLOCK)
> >         Services.perms.remove(aURI.host, "indexedDB-unlimited");
> >     }
> >   },
> 
> The changes in this file look like mostly a reversion of
> https://hg.mozilla.org/mozilla-central/rev/600aee747c40, but with these two
> additional changes thrown in. I do not understand them either.
> 
> Can you explain?

Well, yes, but it's more about reverting changes made in bug 758357.
getStateLabel() was added just for indexedDB because it's current handling of the permission is a bit odd.
See this comment:
http://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/PermissionRequestBase.cpp#105

The "states" property is not needed anymore, because for explicit persistent storage we want to use the default order in which UNKNOWN_ACTION (prompt) is the default.

Let me know if you need more information.

    
    

    
  
(In reply to Mark Finkle (:mfinkle) from comment #37)
> Is there anything in this patch that could affect Fennec? We were using
> indexedDB for our Reader code in Fx35, but removed it in Fx36. That's the
> only chrome code I know of that used indexedDB.
> 
> Is there any WebApp installation or permission code that may need to change?

AFAIK, there are some scripts especially for B2G that put stuff into profiles directly and in this bug we're changing the current directory structure:

<profile>
  storage
    persistent
    temporary

to:

<profile>
  storage
    permanent
    temporary
    default

So some scripts will have to updated.

The other thing is that indexedDB.open() will use the new repository "default storage" by default, that is, if you call it without passing { storage: "persistent|temporary" } as the second argument.
Data in the default storage is treated as temporary for principals with appStatus==APP_STATUS_NOT_INSTALLED) and as persistent otherwise (essentially for apps).
So if something like about:home which is a part of browser's UI, but running as a web page (untrusted content), is using IndexedDB without requesting explicit persistent storage, it will be treated as temporary. That's probably not what you want for data stored by the UI.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3 (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8526125 -
        Attachment is obsolete: true

        Attachment #8528464 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        interdiff (deleted)
        — 
          — Splinter Review
      

    


  

    
    

    
  
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #30)
> Comment on attachment 8526125 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8526125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks ok to me!
> 
> ::: addon-sdk/source/lib/sdk/indexed-db.js
> @@ +46,5 @@
> > +  } else {
> > +    args = [principal].concat(toArray(args));
> > +  }
> > +  if (args.length == 2) {
> > +    args.push({ storage: "persistent" });
> 
> This is wrong, there is a two arg version of deleteDatabase. Are there tests
> for this stuff?

We talked about this on IRC, args contain the principal too so that branch is actually for the one arg version of deleteDatabase. Anyway I improved the test for indexed-db.js module to test all possible combinations hopefully.

> 
> ::: browser/base/content/pageinfo/pageInfo.xul
> @@ +274,2 @@
> >            </vbox>
> > +          <button id="indexedDBClear" label="&permClearStorage;"
> 
> This looks like it needs to be reverted...

Done

> 
> ::: dom/indexedDB/test/unit/xpcshell-parent-process.ini
> @@ +20,5 @@
> >  [include:xpcshell-shared.ini]
> >  
> >  [test_blob_file_backed.js]
> >  [test_bug1056939.js]
> > +[test_bug1083927.js]
> 
> Can this be named something better?

Yeah, test_defaultStorageUpgrade.js and defaultStorageUpgrade_profile.zip

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +594,5 @@
> > +
> > +  nsCOMPtr<nsIFile> mDirectory;
> > +  mozilla::Mutex& mMutex;
> > +  mozilla::CondVar mCondVar;
> > +  nsresult mResultCode;
> 
> Maybe mMainThreadResultCode?

Ok

> 
> @@ +626,5 @@
> > +  NS_IMETHOD
> > +  Run();
> > +};
> > +
> > +struct StorageDirectoryHelper::OriginProps
> 
> This struct looks like it packs pretty inefficiently. Can you reorder the
> members to pack better?

Fixed

> 
> @@ +639,5 @@
> > +
> > +  Type mType;
> > +  nsCString mSpec;
> > +  uint32_t mAppId;
> > +  bool mInMozBrowser;
> 
> No constructor to initialize your POD members?

Added

> 
> @@ +685,5 @@
> > +  bool mMaybeDriveLetter;
> > +  bool mError;
> > +
> > +public:
> > +  OriginParser(const nsAString& aOrigin)
> 
> This needs explicit.

Done

> 
> @@ +706,5 @@
> > +    MOZ_ASSERT(aAppId);
> > +    MOZ_ASSERT(!mError);
> > +
> > +    if (mAppId.IsNull()) {
> > +      *aAppId = nsIScriptSecurityManager::NO_APP_ID;
> 
> Does mAppId really need to be a Nullable<> since you have the NO_APP_ID
> value?

Ok, you convinced me to drop the Nullable.

> 
> @@ +719,5 @@
> > +    MOZ_ASSERT(aInMozBrowser);
> > +    MOZ_ASSERT(!mError);
> > +
> > +    if (mInMozBrowser.IsNull()) {
> > +      *aInMozBrowser = false;
> 
> Same here... Why Nullable<> ?

Removed.

> 
> @@ +726,5 @@
> > +    }
> > +  }
> > +
> > +  void
> > +  GetSpec(nsCString& aSpec);
> 
> Why are these not nsACString& ?

Fixed.

> 
> @@ +733,5 @@
> > +  GetResult(nsCString& aResult);
> > +
> > +private:
> > +  void
> > +  HandleSchema(nsAutoString& aSchema);
> 
> I think these should all take |const nsDependentSubstring&| to avoid stack
> copying.

Ok.

> 
> @@ +745,5 @@
> > +
> > +class OriginKey : public nsAutoCString
> > +{
> > +public:
> > +  explicit
> 
> Not needed for multi-arg constructors.

Ok.

> 
> @@ +811,5 @@
> > +  return false;
> > +}
> > +
> > +bool
> > +ParseOriginString(const nsAString& aOrigin,
> 
> This is needless indirection. Just make this a static functiona and pass the
> args directly into the OriginParser constructor to avoid all the GetFoo()
> calls below.

Ok, but I'm passing arguments to Parse(), not the constructor.

> 
> @@ +838,5 @@
> > +bool
> > +IsPersistentOriginWhitelisted(const nsACString& aOrigin)
> > +{
> > +  if (aOrigin.Equals(kChromeOrigin) ||
> > +      aOrigin.Equals(kAboutHomeOrigin) ||
> 
> Both of these should be EqualsLiteral.

Ok.

> 
> @@ +847,5 @@
> > +  return false;
> > +}
> > +
> > +nsresult
> > +InitStoragePath(nsIFile* aBaseDir,
> 
> Maybe call this CloneStoragePath?

Ok.

> 
> @@ +1399,5 @@
> >      rv = baseDir->GetPath(mStoragePath);
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +    rv = InitStoragePath(baseDir,
> > +                         NS_LITERAL_STRING("persistent"),
> 
> These should be const's at the top of the file, and then you don't have to
> ask for leafName in some spots below.

Fixed

> 
> @@ +2451,5 @@
> >                                          nsIFile** aDirectory)
> >  {
> >    AssertIsOnIOThread();
> >  
> > +  nsresult rv = InitializeStorageArea();
> 
> How about we call this something like MaybeUpgradeStorageArea?

Ok

> 
> @@ +2529,5 @@
> > +      uint64_t ts;
> > +      rv = stream->Read64(&ts);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      timestamp = ts;
> 
> Maybe assert that this is <= PR_Now

Ok

> 
> @@ +4961,5 @@
> > +      return rv;
> > +    }
> > +
> > +    nsCOMPtr<nsIFile> originDir = do_QueryInterface(entry);
> > +    if (NS_WARN_IF(!originDir)) {
> 
> Just MOZ_ASSERT

Ok

> 
> @@ +4998,5 @@
> > +        originProps->mType = OriginProps::eChrome;
> > +        originProps->mSpec = kChromeOrigin;
> > +
> > +        int64_t timestamp;
> > +        rv = GetLastModifiedTime(originDir, &timestamp);
> 
> Why do we care about a timestamp on persistent chrome? Can't we just set to
> 0 or something?

Yeah, and we can just rely on the constructor.

> 
> @@ +5005,5 @@
> > +        }
> > +
> > +        originProps->mTimestamp = timestamp;
> > +      } else {
> > +        NS_WARNING("chrome in temporary storage directory?!");
> 
> This can happen, can't it? If chrome explicitly asks for storage:persistent?

This is actually a case when "chrome" appears in storage/temporary and that shouldn't happen, because we don't allow default/temporary storage for chrome currently.

> 
> @@ +5018,5 @@
> > +      }
> > +
> > +      OriginProps* originProps = mOriginProps.AppendElement();
> > +      originProps->mDirectory = originDir;
> > +      originProps->mType = OriginProps::eContent;
> 
> What about about:home?

The distinction between eChrome/eContent is there just to be able to call GetInfoForChrome() for eChrome in StorageDirectoryHelper::RunOnMainThread().

eAboutHome would be only helpful for IsPersistentOriginWhitelisted() to not strcmp the origin string, but I'm not sure if it's worth it.

> 
> @@ +5041,5 @@
> > +  }
> > +
> > +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this)));
> > +
> > +  mozilla::MutexAutoLock autolock(mMutex);
> 
> You have to scope this lock better, you're doing lots of I/O with it still
> locked. This will cause any other thread trying to grab the quota mutex to
> block on your I/O!
> 
> Additional question: Why are you using the quota mutex here? Is there some
> other synchronization going on here that could possibly interfere with other
> quota shared state?
> 
> And another: You're using the quota mutex on the main thread after some
> amount of delay from when the QuotaManager dispatched this runnable
> initially. Events could have run in between, so what guarantees that the
> QuotaManager and its mutex are still alive when you get back to the main
> thread?

Addressed all three I think.

> 
> @@ +5115,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +nsresult
> > +StorageDirectoryHelper::RunOnMainThread()
> 
> Maybe MOZ_ASSERT(!mOriginProps.IsEmpty()) here, and of course
> MOZ_ASSERT(NS_IsMainThread())

Done

> 
> @@ +5212,5 @@
> > +
> > +bool
> > +OriginParser::Parse()
> > +{
> > +  nsCharSeparatedTokenizerTemplate<TokenizerIgnoreNothing>
> 
> Is there a reason you're using the utf16 version of this tokenizer? I see
> you converting all the path components to UTF8, why not just convert the
> whole string first in one go and then use nsCCharSeparatedTokenizerTemplate
> here?

Ok, changed the parser to take an nsCString.

> 
> @@ +5217,5 @@
> > +    tokenizer(mOrigin, '+');
> > +
> > +  nsAutoString token;
> > +  while (tokenizer.hasMoreTokens() && !mError) {
> > +    token = tokenizer.nextToken();
> 
> This copies the string into the stack each time... Can't you just pass the
> result of nextToken() directly into HandleToken?

Added a stack ref that shouldn't copy, I now need to add the token to a handled token list.

> 
> @@ +5228,5 @@
> > +  }
> > +
> > +  if (tokenizer.separatorAfterCurrentToken()) {
> > +    nsAutoString emptyString;
> > +    HandleToken(emptyString);
> 
> Can't you just make a HandleEmptyToken() function to avoid a new object here?

I added HandleTrailingSeparator().

> 
> @@ +5235,5 @@
> > +  if (mError) {
> > +    return false;
> > +  }
> > +
> > +  return true;
> 
> MOZ_ASSERT(mState == eComplete)

Ok, but I had to modify the parser a bit to achieve this, because I now need to know if there are more tokens before setting next state.

> 
> @@ +5245,5 @@
> > +  MOZ_ASSERT(!mError);
> > +
> > +  nsAutoCString spec;
> > +
> > +  spec.Append(mSchema);
> 
> Just pass mSchema as the constructor arg for spec.

Ok.

> 
> @@ +5247,5 @@
> > +  nsAutoCString spec;
> > +
> > +  spec.Append(mSchema);
> > +  if (mSchemaType == eFile) {
> > +    spec.Append(NS_LITERAL_CSTRING("://"));
> 
> AppendLiteral()

Ok.

> 
> @@ +5263,5 @@
> > +
> > +  if (mSchemaType == eMozSafeAbout) {
> > +    spec.Append(':');
> > +  } else {
> > +    spec.Append(NS_LITERAL_CSTRING("://"));
> 
> AppendLiteral

Ok.

> 
> @@ +5308,5 @@
> > +  if (!aToken.EqualsLiteral("http") &&
> > +      !aToken.EqualsLiteral("https") &&
> > +      !aToken.EqualsLiteral("indexeddb") &&
> > +      !aToken.EqualsLiteral("file") &&
> > +      !aToken.EqualsLiteral("moz-safe-about") &&
> 
> We expect this to be more common than file, and maybe indexeddb, so move it
> up the list.

Done.

> 
> @@ +5316,5 @@
> > +  }
> > +
> > +  mSchema = NS_ConvertUTF16toUTF8(aToken);
> > +
> > +  if (mSchema.Equals("file")) {
> 
> These should all be EqualsLiteral... Though you already called that above.
> Can you try to make just one strcmp call per schema here?

Yeah, the code is now super efficient :)

> 
> @@ +5341,5 @@
> > +  mState = eExpectingPathnameComponent;
> > +}
> > +
> > +void
> > +OriginParser::HandleToken(nsAutoString& aToken)
> 
> MOZ_ASSERT(!aToken.IsEmpty()) after you add a HandleEmptyToken function

I didn't do exactly the same thing, but I added more checks for empty tokens.
I also added a better reporting for the error case (printing exact line in the code and all already handled tokens) so eventual bug reporting and debugging should be much easier.

Here's an example:
0:01.05 PROCESS_OUTPUT: Thread-1 (pid:1735) "origin: file++++1++"
0:01.05 PROCESS_OUTPUT: Thread-1 (pid:1735) "[1735] WARNING: Expected a pathname component (not an empty string)!: file /Users/varga/Sources/Mozilla/dom/quota/QuotaManager.cpp, line 5559"
0:01.05 PROCESS_OUTPUT: Thread-1 (pid:1735) "[1735] WARNING: Origin 'file++++1++' failed to parse, handled tokens: 'file', '', '', '', '1': file /Users/varga/Sources/Mozilla/dom/quota/QuotaManager.cpp, line 5250"

> 
> @@ +5346,5 @@
> > +{
> > +  switch (mState) {
> > +    case eExpectingAppIdOrSchema: {
> > +      nsresult rv;
> > +      uint32_t appId = aToken.ToInteger(&rv);
> 
> Rather than this I think it would be smarter to check
> IsAsciiDigit(aToken.First()). Then you'd know if you were dealing with app
> id or schema without first trying to printf.

Ok.

> 
> @@ +5353,5 @@
> > +        mState = eExpectingInMozBrowser;
> > +        return;
> > +      }
> > +
> > +      return HandleSchema(aToken);
> 
> HandleSchema returns void, move the return to the next line.
> 
> Same for several other places below.

Ok

> 
> @@ +5358,5 @@
> > +    }
> > +
> > +    case eExpectingInMozBrowser: {
> > +      if (aToken.Length() != 1) {
> > +        mError = true;
> 
> You should add a NS_WARNING to all these spots where you set mError.

Ok, actually I added a QM_WARNING which behaves like printf

> 
> @@ +5427,5 @@
> > +
> > +      return;
> > +    }
> > +
> > +    case eExpectingPort: {
> 
> MOZ_ASSERT(mSchemaType == eNone) here?

Ok

> 
> @@ +5429,5 @@
> > +    }
> > +
> > +    case eExpectingPort: {
> > +      nsresult rv;
> > +      uint32_t port = aToken.ToInteger(&rv);
> 
> This won't work with nsDependentSubstring so you'll have to assign to a
> stack string first. But only for this one case.

Ok, there is one more place like this.

> 
> @@ +5465,5 @@
> > +      if (mMaybeDriveLetter && aToken.IsEmpty()) {
> > +        MOZ_ASSERT(mPathnameComponents.Length() == 1);
> > +
> > +        nsCString& pathnameComponent = mPathnameComponents[0];
> > +        pathnameComponent = pathnameComponent + NS_LITERAL_CSTRING(":");
> 
> pathnameComponent.Append(':')

Ok.

> 
> ::: dom/quota/QuotaManager.h
> @@ +583,5 @@
> >    nsAutoTArray<nsRefPtr<Client>, Client::TYPE_MAX> mClients;
> >  
> >    nsString mIndexedDBPath;
> >    nsString mStoragePath;
> >    nsString mPersistentStoragePath;
> 
> Why is this needed now?

Removed.

> 
> ::: gfx/skia/trunk/include/core/SkCanvas.h
> @@ +362,5 @@
> >                       offscreen when restore() is called
> >          @param flags  LayerFlags
> >          @return The value to pass to restoreToCount() to balance this save()
> >      */
> > +//    SK_ATTR_EXTERNALLY_DEPRECATED("SaveFlags use is deprecated")
> 
> ???

Reverted this change.

    
    

    
  
Comment on attachment 8529867 [details] [diff] [review]
patch v3

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

r=me on the browser bits.

        Attachment #8529867 -
        Flags: review+

    
    

    
  
So, testing on try is the last thing that needs to be done before landing on mozilla-inbound.
But given the current problems with try, I'm not sure if we will have enough time to do it before the uplift.

    
  
Blocks: 1020196

    
    

    
  
Try looks ok:
https://tbpl.mozilla.org/?tree=Try&rev=5cfee7e4b069
https://tbpl.mozilla.org/?tree=Try&rev=50b58c012264

So I'm going to try to land it on mozilla-inbound, not sure if it gets merged to m-c before the uplift to aurora.
Also, if there are too many regressions not caught by tests, we can back it out either on m-c or aurora I guess. I'm ok with that.
However it would be nice if we had this fixed in Firefox 36 and the simplified version of default storage would ship in Firefox 35 only.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/840a80f401a6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

    
    

    
  
Hey did either of you two actually run the add-on sdk test changes?

They appear to be failing, and it looks to me like I will have to revert this in order to do an uplift.

https://tbpl.mozilla.org/?tree=Jetpack&rev=ca58332bce3b
Flags: needinfo?(ehsan.akhgari)
Flags: needinfo?(Jan.Varga)

    
    

    
  
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #49)
> Hey did either of you two actually run the add-on sdk test changes?
> 
> They appear to be failing, and it looks to me like I will have to revert
> this in order to do an uplift.
> 
> https://tbpl.mozilla.org/?tree=Jetpack&rev=ca58332bce3b

Are these tests any different from those run on m-c/inbound/try ?
Flags: needinfo?(Jan.Varga)

    
    

    
  
Comment on attachment 8529867 [details] [diff] [review]
patch v3

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

::: testing/mochitest/jetpack-package-harness.js
@@ +119,5 @@
>    Services.prefs.setBoolPref("testing.jetpackTestHarness.running", true);
>  
> +  // Need to set this very early, otherwise the false value gets cached in
> +  // DOM bindings code.
> +  Services.prefs.setBoolPref("dom.indexedDB.experimental", true);

I suspect that jetpack-package-harness.js is separate from the one in m-c.

The test on the JetPack tree is failing because the "storage" property is not defined. The "dom.indexedDB.experimental" pref needs to set very early, otherwise DOM bindings code gets "false" and the storage property is not set.

    
    

    
  
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #49)
> Hey did either of you two actually run the add-on sdk test changes?
> 
> They appear to be failing, and it looks to me like I will have to revert
> this in order to do an uplift.
> 
> https://tbpl.mozilla.org/?tree=Jetpack&rev=ca58332bce3b

Of course I ran add-on sdk tests locally and also on try.
Flags: needinfo?(ehsan.akhgari)

    
    

    
  
(In reply to Jan Varga [:janv] from comment #52)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #49)
> > Hey did either of you two actually run the add-on sdk test changes?
> > 
> > They appear to be failing, and it looks to me like I will have to revert
> > this in order to do an uplift.
> > 
> > https://tbpl.mozilla.org/?tree=Jetpack&rev=ca58332bce3b
> 
> Of course I ran add-on sdk tests locally and also on try.

I see three try links in this bug, and I don't see one tgat includes the jetpack test suite, could you please point that one out to me.
Flags: needinfo?(Jan.Varga)

    
    

    
  
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #53)
> (In reply to Jan Varga [:janv] from comment #52)
> > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #49)
> > > Hey did either of you two actually run the add-on sdk test changes?
> > > 
> > > They appear to be failing, and it looks to me like I will have to revert
> > > this in order to do an uplift.
> > > 
> > > https://tbpl.mozilla.org/?tree=Jetpack&rev=ca58332bce3b
> > 
> > Of course I ran add-on sdk tests locally and also on try.
> 
> I see three try links in this bug, and I don't see one tgat includes the
> jetpack test suite, could you please point that one out to me.

ie: point out the try link which includes the jetpack test suite results

    
    

    
  
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/7d5922e71fa8b932bb5cd38a5f70fb70547ba9a7
Revert "Bug 1083927 - IndexedDB: Subdomain Quota Management; r=bent,ehsan"

This reverts commit 7422d43ba91e22cd166edeb2eea3c7713c67bf4b.

    
    

    
  
(In reply to [github robot] from comment #55)
> Commit pushed to master at https://github.com/mozilla/addon-sdk
> 
> https://github.com/mozilla/addon-sdk/commit/
> 7d5922e71fa8b932bb5cd38a5f70fb70547ba9a7
> Revert "Bug 1083927 - IndexedDB: Subdomain Quota Management; r=bent,ehsan"
> 
> This reverts commit 7422d43ba91e22cd166edeb2eea3c7713c67bf4b.

I am concerned that landing this revert of SDK changes means that API consumers (so, WebIDE for example) will see data disappear (again).

Are you planning to land this in m-c immediately?

    
    

    
  
Erik, please reply here if do you merge the SDK including the revert, so I can be sure to re-test WebIDE.
Flags: needinfo?(evold)
Depends on: 1110067
Depends on: 1110585

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] from comment #56)
> (In reply to [github robot] from comment #55)
> > Commit pushed to master at https://github.com/mozilla/addon-sdk
> > 
> > https://github.com/mozilla/addon-sdk/commit/
> > 7d5922e71fa8b932bb5cd38a5f70fb70547ba9a7
> > Revert "Bug 1083927 - IndexedDB: Subdomain Quota Management; r=bent,ehsan"
> > 
> > This reverts commit 7422d43ba91e22cd166edeb2eea3c7713c67bf4b.
> 
> I am concerned that landing this revert of SDK changes means that API
> consumers (so, WebIDE for example) will see data disappear (again).
> 
> Are you planning to land this in m-c immediately?

I'll figure out something before landing this.  I hope the test just needs to be corrected and it's not more work than that.
Flags: needinfo?(evold)
Flags: needinfo?(evold)

    
    

    
  
I've written this test:


    exports.testOpenWithoutPrincipal = function(assert, done) {
      let request = indexedDB.open("test", {
        storage: "temporary",
        version: 5
      });

      request.onsuccess = function(event) {
        let db = event.target.result;
        //db.createObjectStore("Foasdfjk", { });
        assert.equal(db.name, "test", "the db.name is test");
        assert.equal(db.storage, "temporary", "the storage is temporary");
        done();
      }
    }

which is failing when I run it, but when I read https://github.com/mozilla/gecko-dev/blob/master/dom/indexedDB/test/test_persistenceType.html#L52  it seems like the test that I wrote should pass.. any idea what the issue is here?

At the moment, I always experience db.storage == undefined, which is why the tests that Jan wrote are failing.
Flags: needinfo?(evold) → needinfo?(ehsan)

    
    

    
  
gotta reopen this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Please do not reopen bugs that have not been backed out.

To answer your question, you need to set the dom.indexedDB.experimental pref to true in your test.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(ehsan)
Resolution: --- → FIXED

    
    

    
  
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #61)
> Please do not reopen bugs that have not been backed out.
> 
> To answer your question, you need to set the dom.indexedDB.experimental pref
> to true in your test.

Yeah, that's what I tried to explain in comment 51.
Flags: needinfo?(Jan.Varga)

    
    

    
  
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #53)
> (In reply to Jan Varga [:janv] from comment #52)
> > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #49)
> > > Hey did either of you two actually run the add-on sdk test changes?
> > > 
> > > They appear to be failing, and it looks to me like I will have to revert
> > > this in order to do an uplift.
> > > 
> > > https://tbpl.mozilla.org/?tree=Jetpack&rev=ca58332bce3b
> > 
> > Of course I ran add-on sdk tests locally and also on try.
> 
> I see three try links in this bug, and I don't see one tgat includes the
> jetpack test suite, could you please point that one out to me.

That's strange, I pushed to try with "try: -b do -p all -u all -t none" and it passed.
It also passed on mozilla-inbound and mozilla-central.

    
    

    
  
(In reply to Jan Varga [:janv] from comment #63)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #53)
> > (In reply to Jan Varga [:janv] from comment #52)
> > > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #49)
> > > > Hey did either of you two actually run the add-on sdk test changes?
> > > > 
> > > > They appear to be failing, and it looks to me like I will have to revert
> > > > this in order to do an uplift.
> > > > 
> > > > https://tbpl.mozilla.org/?tree=Jetpack&rev=ca58332bce3b
> > > 
> > > Of course I ran add-on sdk tests locally and also on try.
> > 
> > I see three try links in this bug, and I don't see one tgat includes the
> > jetpack test suite, could you please point that one out to me.
> 
> That's strange, I pushed to try with "try: -b do -p all -u all -t none" and
> it passed.
> It also passed on mozilla-inbound and mozilla-central.

I don't think the jetpack suite is included in this suite atm :(

    
    

    
  
(In reply to Jan Varga [:janv] from comment #62)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #61)
> > Please do not reopen bugs that have not been backed out.
> > 
> > To answer your question, you need to set the dom.indexedDB.experimental pref
> > to true in your test.
> 
> Yeah, that's what I tried to explain in comment 51.

Ah I'm sorry I missed that.

    
    

    
  
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/6081c14d0096b73c280cb331d2137f90cd42c02a
Revert "Revert "Bug 1083927 - IndexedDB: Subdomain Quota Management; r=bent,ehsan""

This reverts commit 7d5922e71fa8b932bb5cd38a5f70fb70547ba9a7.

https://github.com/mozilla/addon-sdk/commit/6fe1370a75ed6a8187a2591626b6e2e5293c53d3
Bug 1083927 setting dom.indexedDB.experimental to true for tests

https://github.com/mozilla/addon-sdk/commit/a9b82a271be9ea265d977423f67c0cc3e43fc0bb
Merge pull request #1797 from erikvold/1083927

Bug 1083927 - IndexedDB: Subdomain Quota Management r=erikvold

    
    

    
  
Thanks for the help Ehsan and Jan, I think this is working now with the pref change, sorry for the trouble.

    
    

    
  
Comment on attachment 8529867 [details] [diff] [review]
patch v3

Approval Request Comment

[Feature/regressing bug #]: This is a followup for bug 1089764. It implements proper default storage (bug 1089764 contained a simplified and a bit hacky version of it).

[User impact if declined]: We promised to ship the simplified version of default storage in Firefox 35 only.

[Describe test coverage new/current, TBPL]: The fix landed on m-c on Nov 28th, 2014, just a few hours after the uplift to Aurora. The most risky part of the patch - origin parser is covered by an xpcshell test.

[Risks and why]: The patch is not simple, but it's been on trunk for a while, there are some small regressions which will have to be fixed on aurora too.

[String/UUID change made/needed]: No string/UUID changes.

        Attachment #8529867 -
        Flags: approval-mozilla-aurora?

        Attachment #8529867 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Ok, thanks. I'll land on aurora once it opens.

    
  
Depends on: 1110010

    
  
Depends on: 1119462

    
    

    
  
We need to get the docs updated with the new rules for how much storage can be used in each mode and when prompts will be displayed. Jan, can you post the most up-to-date rules here?
Flags: needinfo?(Jan.Varga)
Keywords: dev-doc-needed

    
    

    
  
Ping - it would be really useful to be able to publish this info, as https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API#Storage_limits has been criticized a few times for not being correct.

I've published some info I've found out from various places at https://bugzilla.mozilla.org/show_bug.cgi?id=1156671, but I don't really know if it is correct.

    
    

    
  
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #72)
> Ping - it would be really useful to be able to publish this info, as
> https://developer.mozilla.org/en-US/docs/Web/API/
> IndexedDB_API#Storage_limits has been criticized a few times for not being
> correct.
> 
> I've published some info I've found out from various places at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1156671, but I don't really
> know if it is correct.

I put together some info for you, see my comment in bug 1156671.
Flags: needinfo?(Jan.Varga)

    
    

    
  
Documentation on this published at :

https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Browser_storage_limits_and_eviction_criteria

This could definitely use a tech review.

When we are good to go, I will reach out to other browser vendors and ask them to start adding more info to cover their products too.
Keywords: dev-doc-needed → dev-doc-complete

    
    

    
  
Thanks Chris for the new documentation page, it will be very helpful to web application developers.

You might also need to update the documentation about IDBFactory.open(): https://developer.mozilla.org/en-US/docs/Web/API/IDBFactory/open.

The "Parameters > options (version and storage)" section is a bit outdated now. It says "permanent" is the default mode, and the "Note" section could be replaced by a simple link to the new page you've added.

    
    

    
  
(In reply to Maxime RETY from comment #75)
> You might also need to update the documentation about IDBFactory.open():
> https://developer.mozilla.org/en-US/docs/Web/API/IDBFactory/open.
> 
> The "Parameters > options (version and storage)" section is a bit outdated
> now. It says "permanent" is the default mode, and the "Note" section could
> be replaced by a simple link to the new page you've added.

Very good call - thanks Maxime! I have updated it:

https://developer.mozilla.org/en-US/docs/Web/API/IDBFactory/open

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: