bug 1165217 changed the publicly exposed interface to use origin attributes, but the internal implementation still directly uses appId and isInBrowserElement.

This require some non-trivial changes in quota manager. I would rather do it myself.

(In reply to Jan Varga [:janv] from comment #1) > This require some non-trivial changes in quota manager. I would rather do it > myself. Thanks :) I looked into trying it when I wrote the original patch, but I didn't feel comfortable making the changes, as I didn't completely understand the internals.

Mark this blocks Bug 1168777 as QuotaManager should also use clear-origin-data. Jan, are you still working on this? It has been two months since your last comment.

(In reply to Jan Varga [:janv] from comment #1) > This require some non-trivial changes in quota manager. I would rather do it > myself. Hi, Jan could you be more specifically on the 'non-trivial' change? I could help to take this bug if you don't have time. :)

(In reply to Yoshi Huang[:allstars.chh] from comment #4) > (In reply to Jan Varga [:janv] from comment #1) > > This require some non-trivial changes in quota manager. I would rather do it > > myself. > > Hi, Jan > could you be more specifically on the 'non-trivial' change? > I could help to take this bug if you don't have time. :) We need to upgrade whole directory structure under <profile>/storage to match new schema. Origin directories are now named: appId+inMozBrowser+originNoSuffix They need to be renamed to: origin (with origin suffix) This upgrade needs to be crash proof (restore a mid-done upgrade, etc.) For that we also need a new xpcshell-test to make sure the upgrade works correctly and to catch possible problems in future. Origin directories contain .metadata files and these contain the old originNoSuffix (prefixed with appId/inMozBrowser). Other than that a .metadata contains a group (nsIPrincipal::GetDomain), prefixed with jarPrefix. So all .metadata files need to be upgraded too (safely, you must always assume that browser can crash anytime and we must be able to restore from that state, too much stuff depends on this). There is also code for origin deletion specified by principal or by appId/browserOnly. All this stuff needs to be updated.

Thanks for the detailed information, I'll work on this.

(In reply to Jan Varga [:janv] from comment #5) > (In reply to Yoshi Huang[:allstars.chh] from comment #4) > > (In reply to Jan Varga [:janv] from comment #1) > > > This require some non-trivial changes in quota manager. I would rather do it > > > myself. > > > > Hi, Jan > > could you be more specifically on the 'non-trivial' change? > > I could help to take this bug if you don't have time. :) > > We need to upgrade whole directory structure under <profile>/storage to > match new schema. > Origin directories are now named: appId+inMozBrowser+originNoSuffix > They need to be renamed to: origin (with origin suffix) > Hi Janv Just want to confirm first, for the directory name, do you also imply that origin suffix should come after the origin, i,e, it should be looked like https+++developer.cdn.mozilla.net+appId=1007&inBrowser=1 Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #7) > (In reply to Jan Varga [:janv] from comment #5) > > (In reply to Yoshi Huang[:allstars.chh] from comment #4) > > > (In reply to Jan Varga [:janv] from comment #1) > > > > This require some non-trivial changes in quota manager. I would rather do it > > > > myself. > > > > > > Hi, Jan > > > could you be more specifically on the 'non-trivial' change? > > > I could help to take this bug if you don't have time. :) > > > > We need to upgrade whole directory structure under <profile>/storage to > > match new schema. > > Origin directories are now named: appId+inMozBrowser+originNoSuffix > > They need to be renamed to: origin (with origin suffix) > > > Hi Janv > Just want to confirm first, for the directory name, do you also imply that > origin suffix should come after the origin, i,e, it should be looked like > https+++developer.cdn.mozilla.net+appId=1007&inBrowser=1 > > Thanks I think so, AFAIK the idea is to remove appId and inBrowser logic from storage API implementations, so in our case, quota manager should just use nsIPrincipal::GetOrigin() if possible. I also think that .metadata files should contain a new column: origin attributes.

Bad news, the origin is also stored in sqlite databases which back up indexedDB, SQL table "database", column "origin".

I found the OriginClearOP runs on IO thread, however when handling clear-origin-data, the OriginAttributesPattern inheriting Dictionary should run on main thread. Will consider to dispatch a runnable to main thread to run OriginAttributesPattern.Match, but it may be more costly.

(In reply to Yoshi Huang[:allstars.chh] from comment #12) > I found the OriginClearOP runs on IO thread, however when handling > clear-origin-data, the OriginAttributesPattern inheriting Dictionary should > run on main thread. > > Will consider to dispatch a runnable to main thread to run > OriginAttributesPattern.Match, but it may be more costly. You mean for every directory entry ? Could you try to run OriginAttributesPattern.Init on the main thread and then OriginAttributesPattern.Matches directly on IO thread ? I don't see why Matches would have to run on the main thread. Hm, for some reason I can't ni? bholley.

This is my current WIP, didn't address Janv's latest comment yet. also it still has several TODOs

(In reply to Jan Varga [:janv] from comment #13) > Hm, for some reason I can't ni? bholley.

Move OriginAttributesPattern.Init to main thread (in OriginClearOp::DoInitOnMainThread) And since OriginScope only stores Origin now, I remove OriginScope.h and replace all OriginScopes to nsCString. So far this patch will still crash in https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#21013

(In reply to Yoshi Huang[:allstars.chh] from comment #17) > Created attachment 8705083 [details] [diff] [review] > WIP - Part 2: handling clear-origin-data v2 > > Move OriginAttributesPattern.Init to main thread (in > OriginClearOp::DoInitOnMainThread) > > And since OriginScope only stores Origin now, I remove OriginScope.h and > replace all OriginScopes to nsCString. > I'm afraid this is not so simple as it may look. The clearing basically consist of locking areas specified by origin string, appId, browserOnly and then by looping over directory entries and removing ones that match. If you just replace OriginScope with nsCString then locking won't work correctly. In the new world OriginScope must reflect OriginAttributesPattern somehow. Just to explain the locking a bit more, let's have these origins stored on disk: 1000+n+http+++www1.example.com 1000+n+http+++www2.example.com 1000+n+http+++www3.example.com So, all of these belong to appId 1000, say the first one is currently still is use (there's a live IndexeDB database object). Now we get a request to delete everything for appId 1000. We need to get an exclusive lock for appId 1000 (for all three www1, www2, www3), but since www1.example.com is still in use, we won't get the lock until www1.example.com is finished (we actually speed up this process by aborting all running operations for www1.example.com, so the lock for it is released as soon as possible). If you don't do this properly, you will start deleting files that may be still open by sqlite and that would be really bad.

OriginAttributesPattern.Matches should be fine to run on background threads as long as you are *sure* nothing on the main thread, or any other thread, is modifying the OriginAttributesPattern struct. What makes you sure that no one on the main thread isn't modifying the struct at the same time?

Comment on attachment 8710945 [details] [diff] [review] WIP - Part 2: handling clear-origin-data v3 Review of attachment 8710945 [details] [diff] [review]: ----------------------------------------------------------------- Hi, Janv Really thanks to your comments, and sorry I've been working on other bugs and caused some delay of this bug. Yeah, the removal of OriginScope seems to make things worser and I gave up the idea. So I try to add OriginAttributesPattern into OriginScope, so far the try looks good, therefore would like to ask for your feedback first. Also I haven't addressed your previous comment 5 "This upgrade needs to be crash proof (restore a mid-done upgrade, etc.) For that we also need a new xpcshell-test to make sure the upgrade works correctly and to catch possible problems in future." For the test I'll start from modifying dom/indexedDB/test/unit/defaultStorageUpgrade_profile.zip first, and will have a another patch for it. However I am not quite sure about "mid-done upgrade" and "catch possible problems in future" Do we have some tests already ? or if not, could you give me more details on this ? Thanks

Comment on attachment 8704099 [details] [diff] [review] WIP - Part 1: use origin instead of GetJarPrefix Review of attachment 8704099 [details] [diff] [review]: ----------------------------------------------------------------- This replaces GetJarPrefix with Origin.

ping janv? Although b2g gets lower priority now however this bug will also used by ContentualIdentity (bug 1191418) Thanks

Jan told me he's been looking at this and just hasn't said anything here yet.

(In reply to Andrew Overholt [:overholt] from comment #25) > Jan told me he's been looking at this and just hasn't said anything here yet. Oh, thanks. I've heard that b2g-specific patches won't get reviewed so I was a little bit worried, as this feature could also benefit the platform and not b2g only. If this is already in janv's queue then it's totally fine, no push. Thanks

Hi Janv Sorry for bothering you again, I know you've been busy. But my feedback? has been sent for over 2 months but without any response. I know you said you'd like to do it by yourself from Comment 1, if you still think that would be easier, feel free to take this bug back to you. Or if you don't have time to review this (actually it's a feedback), do you mind I find other peers to do this? I sent feedback? instead of r? is that I am not quite familier with this code, in order to make a r? I'll have to write more tests for this, but I am not sure I am in the right direction and will waste too much time on this. As I mentioned in Comment 24 now Contextual Identity project will have dependency on this bug, and I know it will still take a while to make this bug landed, I'd like to speed up on this. Thanks

Ok, sorry, I'll give you my feedback today. Anyway, we can't land this without the upgrade I mentioned in comment 5 ? Do you plan to implement it ?

(In reply to Jan Varga [:janv] from comment #28) > Ok, sorry, I'll give you my feedback today. > Anyway, we can't land this without the upgrade I mentioned in comment 5 ? > Do you plan to implement it ? Yes, see my previous Comment 22. I'd like to know in more details about "mid-done upgrade" and "catch possible problems in future" Do we have some tests already ? or if not, could you give me more details on this ? Thanks

Comment on attachment 8704099 [details] [diff] [review] WIP - Part 1: use origin instead of GetJarPrefix Review of attachment 8704099 [details] [diff] [review]: ----------------------------------------------------------------- QuotaManager::GetInfoFromPrincipal() changes look good, but could you please describe changes in StorageDirectoryHelper::ProcessOriginDirectories() ? I don't quite understand what the new |else if (!suffix.IsEmpty())| block should/is doing. Please note, the the StorageDirectoryHelper is currently used for moving specific origin directories during a storage area upgrade. It's also used for restoring missing/broken ".metadata" files.

(In reply to Yoshi Huang[:allstars.chh] from comment #29) > (In reply to Jan Varga [:janv] from comment #28) > > Ok, sorry, I'll give you my feedback today. > > Anyway, we can't land this without the upgrade I mentioned in comment 5 ? > > Do you plan to implement it ? > > Yes, see my previous Comment 22. > I'd like to know in more details about > "mid-done upgrade" and "catch possible problems in future" > Do we have some tests already ? or if not, could you give me more details on > this ? There are already some tests, for example for "default" storage upgrade, but this bug will need a new test, existing storage upgrade tests can be used as a templete for the new one. Each upgrade of storage area must be done in a way that even when the app crashes during any stage of the upgrade, we must be able to recover and finish the upgrade as if nothing had happened. That's what I meant by mid-done upgrade. A new test should contain a saved storage area (before the upgrade) and test if everything works correctly after the upgrade. That should catch any problems in future when the code is changed again. The upgrade roughly consists of: - renaming directories - updating .metadata files (they contain origin and group) - updating IndexedDB databases (they contain origin) The question is if we should do everything at one time or something can be done lazily.

(In reply to Jan Varga [:janv] from comment #30) > Comment on attachment 8704099 [details] [diff] [review] > WIP - Part 1: use origin instead of GetJarPrefix > > Review of attachment 8704099 [details] [diff] [review]: > ----------------------------------------------------------------- > > QuotaManager::GetInfoFromPrincipal() changes look good, but could you please > describe changes in StorageDirectoryHelper::ProcessOriginDirectories() ? > I don't quite understand what the new |else if (!suffix.IsEmpty())| block > should/is doing. > Please note, the the StorageDirectoryHelper is currently used for moving > specific origin directories during a storage area upgrade. It's also used > for restoring missing/broken ".metadata" files. I think the tests pass with your changes just because there's no storage/temporary/appId+inBrowser+++www.example.com in those saved storage areas.

Comment on attachment 8710945 [details] [diff] [review] WIP - Part 2: handling clear-origin-data v3 Thanks, I'll adress your comments first and start writing some tests first.

Hi Jan Sorry for my late reply. (In reply to Jan Varga [:janv] from comment #30) > Comment on attachment 8704099 [details] [diff] [review] > WIP - Part 1: use origin instead of GetJarPrefix > > Review of attachment 8704099 [details] [diff] [review]: > ----------------------------------------------------------------- > > but could you please > describe changes in StorageDirectoryHelper::ProcessOriginDirectories() ? > I don't quite understand what the new |else if (!suffix.IsEmpty())| block > should/is doing. !suffix.IsEmpty means the origin has non-default-origin-attributes For example, in the test it uses defaultStorageUpgrade_profile.zip, two of the entries with non-empty origin suffix are 1007+f+app+++system.gaiamobile.org/ 1007+t+https+++developer.cdn.mozilla.net/ And the code in |else if (!suffix.IsEmpty())| will convert them to app+++system.gaiamobile.org^appId=1007 (whose nsIPrincipal.origin is app://system.gaiamobile.org^appId=1007 https+++developer.cdn.mozilla.net^appId=1007&inBrowser=1 (whose nsIPrincipal.origin is https://developer.cdn.mozilla.net^appId=1007&inBrowser=1) (In reply to Jan Varga [:janv] from comment #32) > > I think the tests pass with your changes just because there's no > storage/temporary/appId+inBrowser+++www.example.com in those saved storage > areas. Yeah, the test (defaultStorageUpgrade_profile.zip) doesn't have entries with non-empty origin suffix in storage/temporary/, however it has two entries in storage/persistent/, which tests my change in ActorParent.cpp.

Just discussed with Janv on irc, he'd like to take this bug back, but needs confirmation with Andrew. We'd like to see this feature landed before 6/6 (Firefox 49 branched to Aurora) for Contextual Identity as I mentioned in Comment 24

WFM

Since Andrew is okay with this, I'll reset the assignee. Jan, thanks for your help on this. :)

(In reply to Yoshi Huang[:allstars.chh] from comment #37) > Since Andrew is okay with this, I'll reset the assignee. > Jan, thanks for your help on this. :) Thanks for your initial patches!

Jan, are you planning on taking this bug and completing before the 6/6 merge? If so, can you assign yourself? Thank you!

(In reply to Jan Varga [:janv] from comment #5) > (In reply to Yoshi Huang[:allstars.chh] from comment #4) > > (In reply to Jan Varga [:janv] from comment #1) > > > This require some non-trivial changes in quota manager. I would rather do it > > > myself. > > > > Hi, Jan > > could you be more specifically on the 'non-trivial' change? > > I could help to take this bug if you don't have time. :) > > We need to upgrade whole directory structure under <profile>/storage to > match new schema. > Origin directories are now named: appId+inMozBrowser+originNoSuffix > They need to be renamed to: origin (with origin suffix) > > This upgrade needs to be crash proof (restore a mid-done upgrade, etc.) > For that we also need a new xpcshell-test to make sure the upgrade works > correctly and to catch possible problems in future. > Origin directories contain .metadata files and these contain the old > originNoSuffix (prefixed with appId/inMozBrowser). > Other than that a .metadata contains a group (nsIPrincipal::GetDomain), > prefixed with jarPrefix. > So all .metadata files need to be upgraded too (safely, you must always > assume that browser can crash anytime and we must be able to restore from > that state, too much stuff depends on this). > > There is also code for origin deletion specified by principal or by > appId/browserOnly. All this stuff needs to be updated. I have this mostly implemented, I still need to update the origin parser to be able to handle origin attributes and a few things here and there.

(In reply to Jan Varga [:janv] from comment #10) > Bad news, the origin is also stored in sqlite databases which back up > indexedDB, SQL table "database", column "origin". I've fixed this by adding a new schema upgrade for IndexedDB databases, so it's not done as part of the main storage directory upgrade which upgrades directory metadata files and renamed origin directories. It's done lazily when a database is actually being opened. The origin column isn't actually used anywhere.

Comment on attachment 8710945 [details] [diff] [review] WIP - Part 2: handling clear-origin-data v3 Review of attachment 8710945 [details] [diff] [review]: ----------------------------------------------------------------- I should be able to use some chunks of this patch. Anyway there's one thing ... ::: dom/quota/ActorsParent.cpp @@ +2051,2 @@ > } else { > + MOZ_CRASH("Yoshi: When will this happen?"); I think this can easily happen when there are two directory locks for the same pattern. For example: lock1: appId=1 lock2: appId=1 In this case we need to have method that compares those two patterns. If there's already an exclusive lock for appId=1, we have to delay new locks that look like: "appId=1", "appId=1&addonId=1", etc.

Hi Jan, Do you know if the work you have done here will also separate the DOM Cache? Per bkelly, it uses the QuotaManger to get the principal - https://dxr.mozilla.org/mozilla-central/source/dom/cache/ManagerId.cpp#30 Thank you!

(In reply to Tanvi Vyas out 5/16-5/17 [:tanvi] from comment #48) > Hi Jan, > > Do you know if the work you have done here will also separate the DOM Cache? > Per bkelly, it uses the QuotaManger to get the principal - > https://dxr.mozilla.org/mozilla-central/source/dom/cache/ManagerId.cpp#30 > > Thank you! Yeah, DOM cache will be separated too

Bug 1238576 disabled these tests because they used navigator.mozApps We need them to test clear origin changes in this bug. See also my comment https://bugzilla.mozilla.org/show_bug.cgi?id=1238576#c18 The code added to SpecialPowers is copied from Webapps.jsm Kyle, feel free to delegate this and other reviews for this bug to Andrew Sutherland.

This is done separately to make it easier to review Part 2. The code move is split into a remove and add patch since a combined patch "optimizes" the move by moving other code.

try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaaa544ff38b

Comment on attachment 8757122 [details] [diff] [review] Part 1: Re-enable tests for clearing origin data Review of attachment 8757122 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/specialpowers/content/specialpowersAPI.js @@ +2086,5 @@ > unwrapIfWrapped(mo).observe(unwrapIfWrapped(node), > {nativeAnonymousChildList, subtree}); > }, > + > + clearPrivateData: function(appId, browserOnly) { Maybe this should be named clearAppPrivateData or something else that makes it clear this is app-specific and not a general origin thing? (It seems like this name was inherited from the Webapps.jsm (prefixed) method of the same name; clearBrowserData invoked on a mozApp traverses the message manager and then _clearPrivateData gets called.)

(In reply to Andrew Sutherland [:asuth] from comment #62) > Comment on attachment 8757122 [details] [diff] [review] > Part 1: Re-enable tests for clearing origin data > > Review of attachment 8757122 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/specialpowers/content/specialpowersAPI.js > @@ +2086,5 @@ > > unwrapIfWrapped(mo).observe(unwrapIfWrapped(node), > > {nativeAnonymousChildList, subtree}); > > }, > > + > > + clearPrivateData: function(appId, browserOnly) { > > Maybe this should be named clearAppPrivateData or something else that makes > it clear this is app-specific and not a general origin thing? (It seems > like this name was inherited from the Webapps.jsm (prefixed) method of the > same name; clearBrowserData invoked on a mozApp traverses the message > manager and then _clearPrivateData gets called.) Yeah, good idea, I'll change it.

Comment on attachment 8757898 [details] [diff] [review] Part 8: Fixed support for origin clearing, reworked internal origin patterns to use OriginAttributesPattern Review of attachment 8757898 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.h @@ +177,5 @@ > > return true; > } > + > + bool Matches(const OriginAttributesPattern& aOther) const Jonas, can you review this new method in BasePrincipal.h ? We need to compare two patterns in QuotaManager implementation. For example, there can be a clear operation running for appId=1 and a new one is requested for appId=1&userContextId=1 I can provide more examples if you want.

For part 5 and its introduction of the storage.sqlite database, it seems like the "storage" table used to store the kStorageVersion (distinct from the kSQLiteSchemaVersion which is stored in the PRAGMA user_version but mozStorage calls SchemaVersion) exists because we expect that on upgrades there are two logically distinct steps: * 1: Upgrade the actual database schema * (a lot of potentially long-running upgrade stuff happens, during which time we may crash) * 2: Mark us as having completed upgrading our directory tree representations in the database so we don't have to do it again. It definitely makes sense to avoid attempting to apply the database schema changes multiple times, but it seems simpler to just assign a distinct schema version to each of these. So version 1 would be having created any tables, then version 2 would be having applied the upgrades to the directory tree. This establishes a single ordering and avoids needing to worry about situations in the future where the "storage" table's version is nonsensically inconsistent with the schema version. Using "storage" would make more sense under IDB database semantics where changing the schema version is a bigger deal, but as far as SQLite is concerned, it's not a particularly big deal despite the version living in the file header. (There's a normal VDBE op that does a normal btree thing, etc.) In the current case, since the "storage" table is the only table, that would mean the only schema version bump needed would be to 1 from 0 once the upgrade sweep has completed. Having said that, I'm not particularly opposed to the current approach. But if it's kept, it'd be great to get some comments that explain (briefly) why it is the way it is. It might also be worth enhancing the schema so that there is an (enforced) unique key so that when we're dealing with the version there's only one row that can ever be talked about. Right now it's possible to have multiple rows in there (which is nonsensical), but something one could worry about since there's just an insert and the schema doesn't allow for an upsert. Example-wise: sqlite> create table storage (version INTEGER NOT NULL DEFAULT 0); sqlite> create table bstorage (name STRING UNIQUE, version INTEGER NOT NULL DEFAULT 0); sqlite> INSERT OR REPLACE INTO bstorage (name, version) VALUES ("main", 1); sqlite> INSERT OR REPLACE INTO bstorage (name, version) VALUES ("main", 2); sqlite> select * from bstorage; main|2 sqlite> INSERT OR REPLACE INTO storage (version) VALUES (1); sqlite> INSERT OR REPLACE INTO storage (version) VALUES (2); sqlite> select * from storage; 1 2 What's best probably depends on whether you have longer-term plans to have that table avoid the need for file-system scanning.

Not a bad idea, I'll try to merge those versions locally and see how it behaves.

(In reply to Jan Varga [:janv] from comment #46) > (In reply to Jan Varga [:janv] from comment #10) > > Bad news, the origin is also stored in sqlite databases which back up > > indexedDB, SQL table "database", column "origin". > > I've fixed this by adding a new schema upgrade for IndexedDB databases, so > it's not done as part of the main storage directory upgrade which upgrades > directory metadata files and renamed origin directories. It's done lazily > when a database is actually being opened. The origin column isn't actually > used anywhere. I don't understand why we're keeping it around and updating it in Part 6 given that it's unused. Is it because removing a column from a SQLite table is a hassle? The patch does add an NS_WARNING, but that seems like a half-hearted justification for keeping it around. If there's a rationale like "it's handy when people zip up their IDB files to have the origin in there in plaintext" or "really weird, unlikely threat model we plan to address some day", it would be great to have a brief comment inline with the "CREATE TABLE" schema to this end. Or even if it's just "we thought we'd need this, but we didn't, and we haven't gotten around to dropping it yet".

The "origin" column was added not so long ago. I don' exactly know how bent wanted to use it. I put just a warning there because there's a corner case when origins don't match: It happens in dom/indexedDB/test/unit/test_defaultStorageUpgrade.js file:///Users/joe/c++/index.html and file:///Users/joe/c///index.html both end up in: file++++Users+joe+c+++index.html likewise file:///+/index.html and file://///index.html end up in: file++++++index.html Now when I think about it, I could just do the sanitize thing and compare them sanitized ... and return NS_ERROR_FILE_CORRUPTED when they don't match.

Comment on attachment 8757892 [details] [diff] [review] Part 6: A new schema upgrade for IndexedDB databases Review of attachment 8757892 [details] [diff] [review]: ----------------------------------------------------------------- r=asuth but please also consider the points from comment 67. ::: dom/indexedDB/ActorsParent.cpp @@ +4582,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > } else { > // This logic needs to change next time we change the schema! meta: Is this comment saying make sure you update the assert when changing the schema, or is there an overdue change to this logic?

Yeah, the assert and version must be in sync, the assert should remind you to update the block below it.

(In reply to Jan Varga [:janv] from comment #68) > I put just a warning there because there's a corner case when origins don't > match: Interesting... > Now when I think about it, I could just do the sanitize thing and compare > them sanitized ... > and return NS_ERROR_FILE_CORRUPTED when they don't match. ...but is this a use-case we're really trying to support? My impression was that we don't really care about supporting file:// URLs and that they're a nightmare for origin enforcement and windows path limitation things. In general I'd expect if devtools ever veers back into trying to do a WebIDE thing they'd want to just automatically expose specific file roots via real http localhost exposure (albeit possibly with extra security so only Firefox can connect to the internal webserver we spin up). Which is to say, unless we're expecting this collision case to happen in cases we care about (and I don't think we should let that happen), it seems better to let the file URL's just accidentally collide without maintaining extra logic/etc. Also, returning corrupted doesn't seem like it would work much better for the file:// users since their two "origins" would potentially keep nuking each other in the cases where this occurred which seems more confusing than getting a unified database. But absolutely your && :bent's call.

Yeah, file:// URLs have been a pain in the ass :) Maybe the warning is enough for now.

(In reply to Jan Varga [:janv] from comment #68) > The "origin" column was added not so long ago. I don' exactly know how bent > wanted to use it. Two reasons. First, we can ensure that we don't have collisions in the origin hash we use for the path because when we open the db we can make sure that the origins exactly match. Second, chrome code crawling through the idb directory can figure out the origin of every db without having to reverse-engineer our hash scheme.

Ok, thanks for the info, makes sense.

Comment on attachment 8757898 [details] [diff] [review] Part 8: Fixed support for origin clearing, reworked internal origin patterns to use OriginAttributesPattern Review of attachment 8757898 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.h @@ +177,5 @@ > > return true; > } > + > + bool Matches(const OriginAttributesPattern& aOther) const non-required suggestion: Would it make more sense to call this Overlaps() given that's the terminology you use in ActorsParent.cpp? (Or Intersects?) I think it logically makes more sense since WasPassed() is checked on both. Granted, OriginAttributesPatternDictionary does not define defaults whereas OriginAttributesDictionary does, so you fundamentally have to, but the situations are different enough that it seems weird to reuse "Matches". Counter-argument: It's a small nuance, there's an argument for the consistency with the other name, and requires more than trivial search/replace in your patch. ::: dom/quota/ActorsParent.cpp @@ +6360,5 @@ > } > > + // Skip the origin directory if it doesn't match the pattern. > + if (!originScope.MatchesOrigin(OriginScope::FromOrigin( > + NS_ConvertUTF16toUTF8(leafName)))) { unimportant nit: indentation

(In reply to Andrew Sutherland [:asuth] from comment #75) > non-required suggestion: Would it make more sense to call this Overlaps() > given that's the terminology you use in ActorsParent.cpp? (Or Intersects?) Ok, but what about Matches(), MatchesPattern() and MatchesOrigin() in OriginScope.h ? I guess those should be renamed too.

A note why I decided to keep the old .metadata files and create new ones .metadata-v2. Since we don't have a versioning for <profile>/storage, old FF will try to use already upgraded profiles. That can lead to unexpected behavior. Fortunately, we are very strict about directories and files under <profile>/storage. So when we add .metadata-v2 old FF versions will evaluate it as an unknown/unexpected file. This itself seems like sufficient and that .metadata isn't needed anymore. However, old FF checks for existence of the .metadata file and if it doesn't exist then it creates an empty one and moves all files in that directory under the idb/ subdir. So the .metadata-v2 would be moved under idb/. I think that looks too messy, I rather decided to keep both for now. We can remove .metadata files ones the new versioning gets used enough (1 - 1.5 years I guess).

Thanks for the note! I was assuming it was something along those lines. I have to admit I was a bit surprised how upset QuotaManager::InitializeOrigin gets about the presence of the .metadata-v2 file, however. https://dxr.mozilla.org/mozilla-central/source/dom/quota/ActorsParent.cpp#3542 causes initializing the origin to fail, breaking all quota manager clients (indexeddb, dom/cache and thereby serviceworkers I expect, asmjscache) for all existing origins once you've run a Firefox with this patch set once. This is potentially more notable than previous version transitions for consumers like IndexedDB and Cache since most other migrations seemed to just upgrade specific origins on-demand rather than all of them at Firefox startup. (And since we only automatically load pinned tabs by default, with the rest only being loaded-on-demand.) It's really unfortunate that bug 1246615 has not gotten any traction, because it sorta sucks that the only errors we will log when things go awry is "UnknownError" / NS_ERROR_UNEXPECTED. Hopefully this was already known and you've discussed it with :overholt and fancy people, because this seems like a prepare-for-pitchforks scenario. I expect a mail will need to be sent to dev-platform around landing time so people know that a trap-door transition has occurred.

Yeah, having a UI for the incompatibility would be optimal, but the situation after these patches will still be much better than it was before. Before we ran into problems that didn't look related to storage / quota manager / indexedDB. So it took many hours to debug it and finally identify it. I already mentioned that fixing this bug is not trivial. I could in theory use existing .metadata files and just append stuff to it (group and origin would be stored twice), but we also need to rename origin directories. I think it would be a nightmare long term to maintain such code. I believe the transition from appId/inBrowser to originAttribute is itself quite a big change that justifies us to not support using already upgraded profiles by Aurora or something older. I mentioned it to :overholt during development. Sure, I'll send an email to dev-platform so people will know. Btw, thanks for quick reviews, hopefully I'll be able to land it this weekend before Monday's uplift.

Comment on attachment 8757891 [details] [diff] [review] Part 5: Core changes, support for storage directory versioning, new metadata v2 files, origin suffix added to API methods, origin attributes used across the implementation, more upgrade tests Review of attachment 8757891 [details] [diff] [review]: ----------------------------------------------------------------- Hooray for this upgrade logic being a lot more straightforward than the Part 2 logic! These extra tests are great. I'm a little confused about the permutations though: * idbSubdirUpgrade1: indexedDB/ORIGIN/IDBFILES, no .metadata, allegedly Fx21 * idbSubdirUpgrade2: storage/persistent/ORIGIN/IDBFILES, no .metadata, allegedly Fx21 * storagePersistentUpgrade: indexedDB/ORIGIN/idb/IDBFILES, yes .metadata, allegedly Fx25 It seems like one of these had to be manually messed with (unless it was a legacy thing related to whether NS_APP_INDEXEDDB_PARENT_DIR or NS_APP_USER_PROFILE_50_DIR was used?)? The answer doesn't really matter, but I'm interested in understanding. ::: dom/quota/ActorsParent.cpp @@ +4327,5 @@ > + } > + > + if (storageVersion > kStorageVersion) { > + NS_WARNING("Unable to initialize storage, version is too high!"); > + return NS_ERROR_FAILURE; This will eventually become another very dramatic transition if we don't improve things because if we see a future version the quota manager breaks and thereby its clients break too. Unless we end up getting fancy on this bug, I think this will want its own spin-off bug where there's a plan/discussion on how to make things less bad going forward, even if it's just depending on bug 1246615 to save us.

Comment on attachment 8757895 [details] [diff] [review] Part 7: New test for metadata v2 restoring (also tests userContextId origin attribute) Review of attachment 8757895 [details] [diff] [review]: ----------------------------------------------------------------- Are you doing the .metadata truncations by hand? It doesn't need to be addressed in this patch/friends, but I think it would be handy to have a direct explanation or indirect-by-way-of-helper-script-you-used to make it more obvious what's actually being tested.

Comment on attachment 8758188 [details] [diff] [review] Part 9: Remove old directories (fixes bug 1273717) Review of attachment 8758188 [details] [diff] [review]: ----------------------------------------------------------------- Woooo!

(In reply to Andrew Sutherland [:asuth] from comment #80) > These extra tests are great. I'm a little confused about the permutations > though: > * idbSubdirUpgrade1: indexedDB/ORIGIN/IDBFILES, no .metadata, allegedly Fx21 > * idbSubdirUpgrade2: storage/persistent/ORIGIN/IDBFILES, no .metadata, > allegedly Fx21 > * storagePersistentUpgrade: indexedDB/ORIGIN/idb/IDBFILES, yes .metadata, > allegedly Fx25 > > It seems like one of these had to be manually messed with (unless it was a > legacy thing related to whether NS_APP_INDEXEDDB_PARENT_DIR or > NS_APP_USER_PROFILE_50_DIR was used?)? The answer doesn't really matter, > but I'm interested in understanding. No, it's not about NS_APP_... FF 21 was the last version which use just: <profile>/indexedDB/ORIGIN/IDBFILES (no "idb" subdir) FF 25 was the release before we renamed indexedDB to storage/persistent (and also started using idb subdirs) FF 36 was the release before we introduced storage/default and renamed storage/persistent to storage/permanent idbSubdirUpgrade2_profile.zip is there becase some origins could make it into storage/persistent without creating "idb" subdirs an empty .metadata was created when an origin was "upgraded" by creating the "idb" subdir storagePersistentUpgrade_profile.zip is the case when the idb subdir was created and .metadata exists > > ::: dom/quota/ActorsParent.cpp > @@ +4327,5 @@ > > + } > > + > > + if (storageVersion > kStorageVersion) { > > + NS_WARNING("Unable to initialize storage, version is too high!"); > > + return NS_ERROR_FAILURE; > > This will eventually become another very dramatic transition if we don't > improve things because if we see a future version the quota manager breaks > and thereby its clients break too. Unless we end up getting fancy on this > bug, I think this will want its own spin-off bug where there's a > plan/discussion on how to make things less bad going forward, even if it's > just depending on bug 1246615 to save us. Well, hopefully, we won't do such big changes under <profile>/storage very often. I also considered to introduce major and minor version. Minor version change would be backwards compatible, do you think it's worth ?

(In reply to Andrew Sutherland [:asuth] from comment #81) > Comment on attachment 8757895 [details] [diff] [review] > Part 7: New test for metadata v2 restoring (also tests userContextId origin > attribute) > > Review of attachment 8757895 [details] [diff] [review]: > ----------------------------------------------------------------- > > Are you doing the .metadata truncations by hand? It doesn't need to be > addressed in this patch/friends, but I think it would be handy to have a > direct explanation or indirect-by-way-of-helper-script-you-used to make it > more obvious what's actually being tested. Yes, manually, by hand :) I have a simple documentation for it it in my notes, I'll add it to the tests.

Wow, thanks! I emailed :sicking about the dom peer review for Part 8

(In reply to Jan Varga [:janv] from comment #83) > Well, hopefully, we won't do such big changes under <profile>/storage very > often. > I also considered to introduce major and minor version. Minor version change > would be backwards compatible, do you think it's worth ? Yes. Anything we can do to give us latitude in the future to avoid creating more situations where transitioning from version n to n+1 is dramatically one-way for the profile would be fantastic. To maximize this latitude, I suggest using a scheme where we reserve a bit of the version to let older versions indicate when they've messed with the profile. Bit-mask wise: [28 bits: major, 3 bits: minor, 1 bit: not messed with, left 0 on minor=0]. So this release could be major version 1, minor 0: (1 << 4) | (0 << 1) | 0. Next release we do a minor rev so we go to (1 << 4) | (1 << 1) | 1. This release sees that database, sees the major is still the same, but it pulls the low bit down to 0 so the "next" release can see that the database got messed with. Then it's at the discretion of that release whether it actually needs to re-run some upgrade logic or whether it was impossible for the prior version to damage things. This is slightly inspired by a somewhat over-complicated hand-wavey approach we used for the Thunderbird global database. You can read the doc block which included the motivation at: https://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/datastore.js#695 It's very relevant to note that that code was written long before the rapid release model was a thing, so Thunderbird was assuming releases could easily be 3 months - 2 years apart, etc. But we don't have to do that right now. Ideally it happens in a timely fashion so there's at least 3 releases between when it goes in and when we actually have to bump the schema rev.

Uh, and that specific scheme is arbitrarily based on IndexedDB's versioning approach. I'm not sure what the best choice is. What I do know is that if we feel like we need to play more complicated encoding games with the schema than that, then maybe it is easier to just use something like your "storage" table with a column minGeckoVersionThatTouchedDatabase or minSchemaVersionThatTouchedDatabase.

I think, it's better to do this now. If we don't make it before Monday, I'll request aurora approval ...

Comment on attachment 8760022 [details] [diff] [review] Part 11: Unified SQLite database schema and storage version, major and minor version support Review of attachment 8760022 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Note that I am unable to audit the changes in metadata2Restore_profile.zip because the (binary) patch does not apply cleanly on top of what is already there. I'm assuming the changes were to add additional/documentation and/or source materials for the test based on what you said, and I'm okay without double-checking. If I end up doing more reviews for you in the future that involve binary hunks, it might be useful to publish your branch or patch-series in an accessible location. I think using reviewboard does this automatically, or if you use mq, you can revision control it and publish it to a user-account, or if you use git and git-cinnabar (like I do) you can fork https://github.com/xeonchen/gecko to enable publishing your branches to github without feeling bad about uploading gecko's history in its near-entirety, etc. ::: dom/quota/ActorsParent.cpp @@ +4267,1 @@ > rv = UpgradeStorageFrom1To2(connection); nit: It's somewhat awkward to be mixing commenting-out code with your "#if 0" idiom for including schema-upgrade test code. It looks like you used commenting here for syntax consistency reasons. Doesn't need to be changed, just a note. (And I certainly do very much appreciate your including the code that helps me understand how you test and plan for this scenario!)

Thanks for the review! I'll reply later ... try looks good too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36a2c1e05b75 So, I'm going to land it on inbound.

The patch doesn't apply cleanly because I addressed your previous comments and updated relevant patches. The only change I did in metadata2Restore.zip in part 11 is that it contains updated storage.sqlite (no storage table, version stored in user_schema). Yeah, I was forced to user /* */ there because #if #endif couldn't be used nicely. We usually don't land commented out code, but this case is kinda special :)

Pushed by jvarga@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3acbafcc10d Part 1: Re-enable tests for clearing origin data; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/ab62788c946c Part 2: Refactor StorageDirectoryHelper to a base class and two separate final classes, improve correctness of default storage upgrade and add support for resource urls; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/47147d38a9d8 Part 3: Move code to correct place - remove; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/1e03e4b3efaf Part 4: Move code to correct place - add; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/e345136f5a86 Part 5: Core changes, support for storage directory versioning, new metadata v2 files, origin suffix added to API methods, origin attributes used across the implementation, more upgrade tests; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2fd77579c9 Part 6: A new schema upgrade for IndexedDB databases; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2d9e54215f Part 7: New test for metadata v2 restoring (also tests userContextId origin attribute); r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/804d10cab665 Part 8: Fixed support for origin clearing, reworked internal origin patterns to use OriginAttributesPattern; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/9b32c8b9be79 Part 9: Remove old directories (fixes bug 1273717); r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/06132a28bab6 Part 10: Use extra durability mode for storage.sqlite; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/8a44fc10f73b Part 11: Unified SQLite database schema and storage version, major and minor version support; r=asuth

Comment on attachment 8757898 [details] [diff] [review] Part 8: Fixed support for origin clearing, reworked internal origin patterns to use OriginAttributesPattern Review of attachment 8757898 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/OriginScope.h @@ +139,5 @@ > + if (IsOrigin()) { > + match = mOrigin.Equals(aOther.mOrigin); > + } else if (IsPattern()) { > + match = mPattern.Matches(aOther.mOriginAttributes); > + } else { We should assert IsNull() here, and also in MatchesPattern() and Matches() I'll file a follow-up.

Comment on attachment 8757898 [details] [diff] [review] Part 8: Fixed support for origin clearing, reworked internal origin patterns to use OriginAttributesPattern r=jst

I don't understand what it means to call .Matches() on two patterns. When does that function return true? Does .matches perform a "A is subset of B" operation, a "A overlaps B" operation, a "A equals B" operation, or something else? For example, what does { addonId: 1 }.Matches({ userContextId: 2}) return? Why? Also, when is this functionality needed? If someone first clears {appId:1} and then clears {appId:1, userContextId=1} why wouldn't we simply first clear all data where appId is 1, and then clear all data where appId is 1 and where userContextId is 1? The second operation wouldn't result in any data deleted, since it's a subset of the first operation, but that seems ok. Doing deletes is going to be a rare operation, so I don't see a need to optimize and detect when a clear can be optimized away due to an ongoing clear being a superset.

(In reply to Jonas Sicking (:sicking) from comment #95) > I don't understand what it means to call .Matches() on two patterns. When > does that function return true? > > Does .matches perform a "A is subset of B" operation, a "A overlaps B" > operation, a "A equals B" operation, or something else? A overlaps B, I actually renamed the method to Overlaps() as :asuth suggested > > For example, what does { addonId: 1 }.Matches({ userContextId: 2}) return? > Why? It returns true. There can be an origin directory with origin attributes: ^addonId=1&userContextId=2 Both patterns { addonId: 1 } and { userContextId: 2 } matches that origin directory. It looks a bit strange, but both patterns overlap from this point of view. Here's a case when they wouldn't overlap: { addonId: 1, userContextId: 2 } { addonId: 2, userContextId: 2 } So two patterns don't overlap when an origin attribute is defined on both and values don't equal. > > Also, when is this functionality needed? > > If someone first clears {appId:1} and then clears {appId:1, userContextId=1} > why wouldn't we simply first clear all data where appId is 1, and then clear > all data where appId is 1 and where userContextId is 1? The second operation > wouldn't result in any data deleted, since it's a subset of the first > operation, but that seems ok. > > Doing deletes is going to be a rare operation, so I don't see a need to > optimize and detect when a clear can be optimized away due to an ongoing > clear being a superset. This is not about this kind of optimization, it's about locking an origin or a set of origins. The locking is needed to prevent exclusive access to origin directories. For example if an origin or a set is being cleared we don't want to allow indexedDB to create a new database and vice-versa when there are open connections for an indexedDB database we don't want to allow a clear operation to start.

https://hg.mozilla.org/mozilla-central/rev/f3acbafcc10d https://hg.mozilla.org/mozilla-central/rev/ab62788c946c https://hg.mozilla.org/mozilla-central/rev/47147d38a9d8 https://hg.mozilla.org/mozilla-central/rev/1e03e4b3efaf https://hg.mozilla.org/mozilla-central/rev/e345136f5a86 https://hg.mozilla.org/mozilla-central/rev/bc2fd77579c9 https://hg.mozilla.org/mozilla-central/rev/9d2d9e54215f https://hg.mozilla.org/mozilla-central/rev/804d10cab665 https://hg.mozilla.org/mozilla-central/rev/9b32c8b9be79 https://hg.mozilla.org/mozilla-central/rev/06132a28bab6 https://hg.mozilla.org/mozilla-central/rev/8a44fc10f73b

(In reply to Jan Varga [:janv] from comment #96) > The locking is needed to prevent exclusive access to origin directories. s/exclusive/concurrent

Cool, that makes sense. "Overlaps" is a much better name. Might be worth leaving a comment like "returns true if it's possible to construct an OriginAttributes struct which matches both patterns." Also note that for now, this function will be super conservative. While { addonId: 1 }.Matches({ userContextId: 2}) will return true, we never create an OriginAttribute which has both addonId and userContextId set (Assertions to enforce that is about to be added). Hence we'll never have data which matches both those patterns. But we can add such optimizations later. Probably should add said assertions first if nothing else.