1339081 - QuotaManager: Upgrade storage from 1.0 to 2.0

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P2)

Description

[Jan Varga [:janv]]

This bug is about some fixes and enhancements needed for storage upgrade in bug 1311057 and bug 1320404, including actual storage upgrades implemented in those bugs.
The upgrade can be more efficient if it's done cumulatively. This will also cause only one backwards incompatible change instead of two.

Comment 1

[Jan Varga [:janv]]

Attachment: patch 1

Comment 2

[Jan Varga [:janv]]

Attachment: patch 2

Comment 3

[Jan Varga [:janv]]

Attachment: patch 3

Comment 4

[Jan Varga [:janv]]

Attachment: patch 4

Comment 5

[Jan Varga [:janv]]

Attachment: patch 5

Comment 6

[Jan Varga [:janv]]

Attachment: patch 6

Comment 7

[Jan Varga [:janv]]

Attachment: patch 7

Comment 8

[Jan Varga [:janv]]

Attachment: patch 8

Comment 9

[Jan Varga [:janv]]

Attachment: patch 9

Comment 10

[Jan Varga [:janv]]

Attachment: patch 10

Comment 11

[Jan Varga [:janv]]

Attachment: patch 11

Comment 12

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e07d5551bcea48c08778c07d183c387a2c85bbe0

Comment 13

[Jan Varga [:janv]]

Attachment: Part 1: Create a standalone testing infrastructure with a basic test

Comment 14

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b1f889d3484421cc4ce5df15ce76eb8ec1b3daa

Andrew, would you have time to review this ?
Patches Part 1 - Part 17 (except Part 4)

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Yes, I can review.  The patches either seem small or close enough to stuff that's still in my brain that it shouldn't be a time problem.  I'm assuming Part 6 from bug 1314361 don't need to be reviewed either, since it has r=bholley on that bug already.  (Also part 7 is r=mayhemer, but it's localstorage and I'm recently active there, so I'm gonna peek at it anyways.)

I'm okay to review against your try pushes unless you are using the bzexport extension or something else to automate creating all the attachments.  Please do flag me with needinfo or something like that when you want me to start the review.

Comment 16

[Jan Varga [:janv]]

Ok, thanks, I'll needinfo you soon.

Comment 17

[Jan Varga [:janv]]

Attachment: Part 2: Add a method for testing storage initialization

Comment 18

[Jan Varga [:janv]]

Attachment: Part 3: Split OriginClearOp to make code more readable and cleaner

Comment 19

[Jan Varga [:janv]]

Attachment: Part 4: Add a result attribute to nsIQuotaRequest

Comment 20

[Jan Varga [:janv]]

Attachment: Part 5: Add a method for testing origin initialization

Comment 21

[Jan Varga [:janv]]

Attachment: Part 6: Fix unknown file handling

Comment 22

[Jan Varga [:janv]]

Attachment: Part 7: Implement safe metadata writting using temp files

Comment 23

[Jan Varga [:janv]]

Attachment: Part 8: Rename the upgrade helper to match the method that is using it

Comment 24

[Jan Varga [:janv]]

Attachment: Part 9: Allow origin properties to be checked before they are added to the list for processing in upgrade helpers

Comment 25

[Jan Varga [:janv]]

Attachment: Part 10: Unify GetDirectoryMetadata() in upgrade helpers

Comment 26

[Jan Varga [:janv]]

Attachment: Part 11: Fallback to PR_Now() if GetLastModifiedTime() fails

Comment 27

[Jan Varga [:janv]]

Attachment: Part 12: Add a baseline helper for 1.0 to 2.0 upgrade

Comment 28

[Jan Varga [:janv]]

Attachment: Part 13: Move morgue directory cleanup to new upgrade helper

Comment 29

[Jan Varga [:janv]]

Attachment: Part 14: Remove apps data as part of new upgrade helper (needed for bug 1311057), add a bunch of new tests to verify upgrade methods

Comment 30

[Jan Varga [:janv]]

Attachment: Part 15: Strip obsolete origin attributes as part of new upgrade helper (needed for bug 1314361), original patch by Kris Maglione

Comment 31

[Jan Varga [:janv]]

Attachment: Part 16: Simplify File.createFromNsIFile() usage

Comment 32

[Jan Varga [:janv]]

Attachment: Part 17: Strip obsolete origin attributes in IndexedDB databases (needed for bug 1314361), original patch by Kris Maglione

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841315 [details] [diff] [review]
Part 1: Create a standalone testing infrastructure with a basic test

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

I understand this to be portions of bug 1311057 attachment 8833900 [details] [diff] [review] specifically the addition of dom/quota/test/unit/.  The head.js's main changes appear to be:
- clearStorage, resetStorage are now named clear and reset.  (nit: Although less confusing since storage may mean other things to people, the names are pretty generic.  clearQM or resetQM seem better.  No need to fix; just thoughts for the future.)
- initStorage is moved to part 2 where it's now named init()
- getOriginDir is now the more clearly named getRelativeFile
- getUsage is introduced with a weird usage idiom that really seems like the IDB/QM tests want to switch to using Task.jsm or async/await.

Thank you very much for the comment in test_basics.js explaining how the zip was created, etc.!

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841879 [details] [diff] [review]
Part 2: Add a method for testing storage initialization

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

I understand this to be portions of bug 1311057 attachment 8833900 [details] [diff] [review] minus the test infra that was moved to part 1 and the upgrade logic that's in later parts.

I understand the revision of the test to:
- Help check that the clear op completed, deleting "storage.sqlite".  (It also recursively deletes the "storage" subdirectory, but as we know, that's complicated/error prone on windows, so a reasonable thing not to check.)  This ensures we avoid false positives in the next step.
- Checks that the init op triggered initialization by virtue of the "storage.sqlite" file existing again after the op.

Comment 35

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841882 [details] [diff] [review]
Part 5: Add a method for testing origin initialization

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

Love the helper test op.

Looking at the test, I am reminded it's a little weird that we still create the .metadata files.  You covered the rationale for this in bug 1195930 comment 77.  Bug 1195930 shipped in 49, trunk is 54 and its release will coincide with ESR 45 being retired, so maybe it's time to file a bug for that.  If you want to intentionally delay the removal longer than that, perhaps reference it in a comment by the METADATA_FILE_NAME constant in ActorsParent.cpp? rs=asuth for adding a comment like that.

Comment 36

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841883 [details] [diff] [review]
Part 6: Fix unknown file handling

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

::: dom/quota/ActorsParent.cpp
@@ +6075,5 @@
> +          continue;
> +        }
> +
> +        UNKNOWN_FILE_WARNING(leafName);
> +        if (!initialized) {

Existing code complaint: The "initialized" guards against returning errors here in AddUsage() are confusing without a comment explaining the rationale.  It's great that you're getting full branch coverage in the test by running getUsage with both initialized and !initialized.  It would also be great to explain why these branches matter or that we're just maintaining existing behavior and it's a low-priority todo to eliminate them.

The existing logic really only makes sense if one of the following is true:
- It's an invariant that after you explicitly initialize an origin that the getUsage op won't fail.  Since you only just added the ability to explicitly initialize an origin, this does not seem like an invariant anything should be depending on.
- Once an origin is initialized we expect normal operation of its clients (or some operating-system process doing things as a result of client operation like anti-virus) to result in files/directories showing up randomly.  This seems like a bad thing to support because in the event of a crash, the origin will fail to initialize at next startup.

Code archaeology suggests the attempt to handle both cases existed from the beginning, but not much more.  In http://searchfox.org/mozilla-central/rev/67b05683bd4ad8a31480b2e98d2d3a11660c41d8/dom/quota/QuotaManager.cpp#2042 the initialized guard also controlled invoking MaybeUpgradeOriginDirectory, but otherwise things are largely the same.

::: dom/quota/test/unit/head.js
@@ +5,5 @@
>  
>  var { 'classes': Cc, 'interfaces': Ci, 'utils': Cu } = Components;
>  
>  const NS_OK = 0x0;
> +const NS_ERROR_UNEXPECTED = 0x8000ffff;

nit: I think convention would be to add `results: Cr` to the destructuring assignment above and then either do Cr.NS_ERROR_UNEXPECTED at the usages sites or use it as the RHS here instead of hardcoding the value.  (context: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.results)

Comment 37

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841890 [details] [diff] [review]
Part 12: Add a baseline helper for 1.0 to 2.0 upgrade

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

::: dom/quota/ActorsParent.cpp
@@ +1441,5 @@
>                         nsACString& aOrigin,
>                         Nullable<bool>& aIsApp);
>  
>    nsresult
> +  GetDirectoryMetadata2(nsIFile* aDirectory,

Given that this is almost indistinguishable from QuotaManager::GetDirectoryMetadata2 in name and implementation, I think adding a comment like the following would be helpful.  Or a name change to "GetPriorVersionDirectoryMetadata2" for this one and/or a change for QM's version to "GetCurrentVersionDirectoryMetadata2".

Upgrade helper to load the contents of ".metadata-v2" files from previous schema versions.  Although QuotaManager has a similar GetDirectoryMetadata2 method, it is only intended to read current version ".metadata-v2" files.  And unlike the old ".metadata" files, the ".metadata-v2" format can evolve because our "storage.sqlite" lets us track the overall version of the storage directory.

@@ +4298,2 @@
>  nsresult
>  QuotaManager::UpgradeStorageFrom1_0To2_0(mozIStorageConnection* aConnection)

No change required, but in general I think it'd be handy to have a comment here or elsewhere that states in brief:
- What mutations are applied as part of the upgrade process.
- What downgrade-incompatible changes in the system (which may or may not be part of the upgrade logic here) may have mandated a schema bump, referencing the motivating bug numbers.  If none, say none.

Much of my rationale here is just that for both this change and the last IDB schema change, it seems like a number of logically distinct bugs have all converged to trigger a schema bump at the same time.  It's great that we can do that, but it makes things extra confusing when trying to do archaeology and reason about motivating factors or what happened in what release, etc.  (Relatedly, this bug currently has no dependencies or see also's listed, which is not helping.)

I also want to be clear that I think this and the preceding series of upgrade clean-ups are fantastic.  The new logic is clean, avoids redundancy, has informative logging as things happen, and has the kind of extensive testing that usually one only sees in SQLite! :)

@@ +6913,5 @@
> +    return rv;
> +  }
> +
> +  nsCString suffix;
> +  rv = binaryStream->ReadCString(suffix);

Aside: AFAICT, we don't actually need to be writing the suffix to disk in ".metadata-v2" since both the origin and the group already include the suffix.  I think at the beginning of bug 1195930 and friends the logic was using GetOrigin() which was what became GetOriginNoSuffix() but then the QM logic switched over to GetOrigin() (with suffix), which is good, but which left us with the unused suffix.

Do you agree with this analysis?  (Note that I don't think we should change the behavior.  But if you wanted to add a comment somewhere, I wouldn't complain ;)

Comment 38

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841894 [details] [diff] [review]
Part 16: Simplify File.createFromNsIFile() usage

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

I look forward to these tests using a test driver that can handle being yielded a promise :)

Comment 39

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841895 [details] [diff] [review]
Part 17: Strip obsolete origin attributes in IndexedDB databases (needed for bug 1314361), original patch by Kris Maglione

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

Test-wise, I don't think the test is strong enough as-is.  Specifically, it looks like the test assumes opening the database would fail if the upgrade fails, but OpenDatabaseOp::LoadDatabaseInformation's enforcement is just an NS_WARNING:
  if (mOrigin != origin) {
    NS_WARNING("Origins don't match!");
  }

For the test to work, the open needs to return an error instead.  Alternately, it could be made into an assert.  The downside to doing this is that I suppose it would cause :bent's long-unsupported https://github.com/benturner/idbbrowser extension to work even less.  And it could stymie users trying to debug things by manually copying databases around.  However, we do have working IDB-in-devtools these days, and the scrambled filenames IDB uses already make it largely intractable to copy things between origins.


Restating my understanding of the patch: This adds logic similar to the localstorage "Bug 1314361 - Part 7: Strip addonId from existing DOMStorage origin attributes" patch, stripping the addonid by virtue of "Bug 1314361 - Part 6: Remove the addonId origin attribute." removing the mAddonId case from OriginAttributes::CreateSuffix.

::: dom/indexedDB/test/unit/test_obsoleteOriginAttributesUpgrade.js
@@ +13,5 @@
> +
> +  clearAllDatabases(continueToNextStepSync);
> +  yield;
> +
> +  installPackagedProfile("obsoleteOriginAttributes_profile");

I think a comment mentioning that the origin directory in the zip file is "moz-extension+++8ea6d31b-917c-431f-a204-15b95e904d4f^addonId=indexedDB-test%40kmaglione.mozilla.com" would help make it more clear what's being tested.

Comment 40

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841892 [details] [diff] [review]
Part 14: Remove apps data as part of new upgrade helper (needed for bug 1311057), add a bunch of new tests to verify upgrade methods

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

The originalSchema logic is somewhat awkward.  Here's the rationale as I understand it:
- UpgradeStorageFrom0_0To1_0Helper and its even-older-upgrade-helper CreateOrUpgradeDirectoryMetadataHelper both subclass StorageDirectoryHelper and depend on OriginProps::Init to not fail or they would break.
- Although we know the eventual result of an app schema is to be deleted by UpgradeStorageFrom1_0To2_0Helper, it's preferable to not touch the older upgrade logic both to avoid breaking it and to avoid making it more complex.
- The spec has to be hacked to be "http://" instead of "app://" because StorageDirectoryHelper::RunOnMainThread creates a URI, and because the app scheme is gone, that simply won't work.  That's also why OriginParser::MaybeUpdateSchema is invoked right after to restore the original schema.

I'm signing off on this because pretty much any solution is awkward, but when it comes time to address bug 1320404, I'd suggest trying to clean things up along these lines:
- Introduce an OriginProps Type of eObsoleteDelete.
- Alter OriginParser::ParseOrigin and OriginParser::Parse to return an enum of { InvalidOrigin, ObsoleteOrigin, ValidOrigin }.  Stop mangling the scheme.  Do expose the scheme since the upgrade logic does need to figure out if the obsolescence is supposed to be dealt with in its upgrade script or not.  (Alternately, some other mechanism like aObsoleteReason or aObsoleteVersion could be exposed.)
- Have OriginProps::Init set its mType to eObsoleteDelete if the parse returned obsolete.
- Have EnsureOriginIsInitializedInternal return failure if the Parse result is not ValidOrigin.
- Have StorageDirectoryHelper::RunOnMainThread special-case eObsoleteDelete so a URI doesn't need to be created.  This may turn out to be trickier/uglier than I'm assuming, proving this patch's approach right ;)


Related nit: For URL terminology, it's scheme, not schema.


Restating other stuff:
- The IndexedDB migration test cases have had their appId-using origins removed.
- The new quota manager tests (which are derived from their idb predecessors) assume responsibility for those cases (and more!), although the net result is the directories disappear, so who can say which upgrade function deletes them? ;)


Kudos on the tests here.  The IDB tests they're derived from were already pretty nice, but these new ones are very thorough, self-documenting where possible, and have very much improved comments over what was in the IDB variants.  Plus, I think you've even further improved the test coverage, which is saying something!

Comment 41

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8841893 [details] [diff] [review]
Part 15: Strip obsolete origin attributes as part of new upgrade helper (needed for bug 1314361), original patch by Kris Maglione

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

Typo nit: "striped" should be "stripped".  ("striped" is the past participle of "stripe", not "strip".)

Comment 42

[Andrew Sutherland [:asuth] (he/him)]

(All reviews completed.  Kudos again on all the great work, :janv!  Also everyone else helping to rid of us the addon id's and app id's!)

Comment 43

[Jan Varga [:janv]]

You did amazing job as usual, thanks!

I'll go through the patches again and try to incorporate suggested changes, if the changes are too big I'll re-request review, but I don't expect there will be many new review requests.

Comment 44

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #33)
> - clearStorage, resetStorage are now named clear and reset.  (nit: Although
> less confusing since storage may mean other things to people, the names are
> pretty generic.  clearQM or resetQM seem better.  No need to fix; just
> thoughts for the future.)

Well, I tried multiple "consistent" approaches to the naming and decided to use simple names if possible. I don't plan to add client oriented tests to dom/quota/test, so these generic names shouldn't clash with anything (at least, I hope so).

> - getUsage is introduced with a weird usage idiom that really seems like the
> IDB/QM tests want to switch to using Task.jsm or async/await.

Yeah that would be nice, I like async/await, but since I don't want to mix different "async" approaches, I'll stick with generators for now.

Comment 45

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #34)
> - Help check that the clear op completed, deleting "storage.sqlite".  (It
> also recursively deletes the "storage" subdirectory, but as we know, that's
> complicated/error prone on windows, so a reasonable thing not to check.) 

Yeah, but only if there are open files which is not the case in this test.

Comment 46

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #35)
> that.  If you want to intentionally delay the removal longer than that,
> perhaps reference it in a comment by the METADATA_FILE_NAME constant in
> ActorsParent.cpp? rs=asuth for adding a comment like that.

Ok, filed bug 1343576 and added the comment.

Comment 47

[Jan Varga [:janv]]

Attachment: Part 18: Rationalize IndexedDB origin initialization code (preparation for sharing with a new upgrade method)

Comment 48

[Jan Varga [:janv]]

Attachment: Part 19: Rename file manager directories as part of new upgrade helper

Comment 49

[Jan Varga [:janv]]

Comment on attachment 8843575 [details] [diff] [review]
Part 18: Rationalize IndexedDB origin initialization code (preparation for sharing with a new upgrade method)

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

::: dom/indexedDB/ActorsParent.cpp
@@ -17889,5 @@
>      }
>  
>      if (isDirectory) {
> -      if (!StringEndsWith(leafName, filesSuffix) ||
> -          !validSubdirs.GetEntry(leafName)) {

This doesn't buy us much, we can just add all subdirs to the subdirsToProcess array.

@@ -18079,5 @@
>      }
>    }
>  
> -  // We have to do this after file manager initialization.
> -  if (!unknownFiles.IsEmpty()) {

Delaying this is not useful anymore, since we check all types of files, so they can't disappear during file manager initialization which can open the database.

Comment 50

[Jan Varga [:janv]]

Comment on attachment 8843576 [details] [diff] [review]
Part 19: Rename file manager directories as part of new upgrade helper

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

::: dom/quota/Client.h
@@ +90,5 @@
>    }
>  
>    // Methods which are called on the IO thred.
>    virtual nsresult
> +  UpgradeStorageFrom1_0To2_0(nsIFile* aDirectory)

We could also add something like UpgradeStorage(nsIFile* aDirectory, int32_t aOldVersion, int32_t aNewVersion), but I'm not sure if it's worth the additional complexity.

Comment 51

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #36)
> The existing logic really only makes sense if one of the following is true:
> - It's an invariant that after you explicitly initialize an origin that the
> getUsage op won't fail.  Since you only just added the ability to explicitly
> initialize an origin, this does not seem like an invariant anything should
> be depending on.
> - Once an origin is initialized we expect normal operation of its clients
> (or some operating-system process doing things as a result of client
> operation like anti-virus) to result in files/directories showing up
> randomly.  This seems like a bad thing to support because in the event of a
> crash, the origin will fail to initialize at next startup.
> 
> Code archaeology suggests the attempt to handle both cases existed from the
> beginning, but not much more.

I don't remember why I decided to do that this way. Maybe I wanted to let developers
to add temporary files into origin directories while getUsage() would still work.

I added this comment to the code:
We are maintaining existing behavior here (failing if the origin is
not yet initialized or just continuing otherwise).
This can possibly be used by developers to add temporary backups into
origin directories without losing get usage functionality.

> > +const NS_ERROR_UNEXPECTED = 0x8000ffff;
> 
> nit: I think convention would be to add `results: Cr` to the destructuring
> assignment above and then either do Cr.NS_ERROR_UNEXPECTED at the usages
> sites or use it as the RHS here instead of hardcoding the value.  (context:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/
> Language_Bindings/Components.results)

Ah yes, I fixed it.

Comment 52

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #37)
> Given that this is almost indistinguishable from
> QuotaManager::GetDirectoryMetadata2 in name and implementation, I think
> adding a comment like the following would be helpful.  Or a name change to

Ok, I added the comment, thanks for providing it.

> @@ +4298,2 @@
> >  nsresult
> >  QuotaManager::UpgradeStorageFrom1_0To2_0(mozIStorageConnection* aConnection)
> 
> No change required, but in general I think it'd be handy to have a comment
> here or elsewhere that states in brief:
> - What mutations are applied as part of the upgrade process.
> - What downgrade-incompatible changes in the system (which may or may not be
> part of the upgrade logic here) may have mandated a schema bump, referencing
> the motivating bug numbers.  If none, say none.

Ok, I'll add comments for that

> (Relatedly, this bug currently has no dependencies or see also's listed,
> which is not helping.)

I added correct dependencies.

> Aside: AFAICT, we don't actually need to be writing the suffix to disk in
> ".metadata-v2" since both the origin and the group already include the
> suffix.  I think at the beginning of bug 1195930 and friends the logic was
> using GetOrigin() which was what became GetOriginNoSuffix() but then the QM
> logic switched over to GetOrigin() (with suffix), which is good, but which
> left us with the unused suffix.
> 
> Do you agree with this analysis?  (Note that I don't think we should change
> the behavior.  But if you wanted to add a comment somewhere, I wouldn't
> complain ;)

Yeah, the suffix isn't used right now, but we might need it in future. It's
a bit of redundancy we can live with given how painful is to upgrade metadata
files. I'll add a comment for this too.
Anyway, there are also some unused reserved data blocks.

Comment 53

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #52)
> > No change required, but in general I think it'd be handy to have a comment
> > here or elsewhere that states in brief:
> > - What mutations are applied as part of the upgrade process.
> > - What downgrade-incompatible changes in the system (which may or may not be
> > part of the upgrade logic here) may have mandated a schema bump, referencing
> > the motivating bug numbers.  If none, say none.
> 
> Ok, I'll add comments for that

Here's how it looks like:

// The upgrade consists of a number of logically distinct bugs that
// intentionally got fixed at the same time to trigger just one major
// version bump.
//
//
// Morgue directory cleanup
// [Feature/Bug]:
// The original bug that added "on demand" morgue cleanup is 1165119.
//
// [Mutations]:
// Morgue directories are removed from all origin directories during the
// upgrade process. Origin initialization and usage calculation doesn't try
// to remove morgue directories anymore.
//
// [Downgrade-incompatible changes]:
// Morgue directories can reappear if user runs an already upgraded profile
// in an older version of Firefox. Morgue directories then prevent current
// Firefox from initializing and using the storage.

Comment 54

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #40)
> I'm signing off on this because pretty much any solution is awkward, but
> when it comes time to address bug 1320404, I'd suggest trying to clean
> things up along these lines:
> - Introduce an OriginProps Type of eObsoleteDelete.
> - Alter OriginParser::ParseOrigin and OriginParser::Parse to return an enum
> of { InvalidOrigin, ObsoleteOrigin, ValidOrigin }.  Stop mangling the
> scheme.  Do expose the scheme since the upgrade logic does need to figure
> out if the obsolescence is supposed to be dealt with in its upgrade script
> or not.  (Alternately, some other mechanism like aObsoleteReason or
> aObsoleteVersion could be exposed.)
> - Have OriginProps::Init set its mType to eObsoleteDelete if the parse
> returned obsolete.
> - Have EnsureOriginIsInitializedInternal return failure if the Parse result
> is not ValidOrigin.
> - Have StorageDirectoryHelper::RunOnMainThread special-case eObsoleteDelete
> so a URI doesn't need to be created.  This may turn out to be
> trickier/uglier than I'm assuming, proving this patch's approach right ;)

Yeah, the current handling is rather hacky, I'm going to try to implement what
you proposed here.

Comment 55

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #40)
> Related nit: For URL terminology, it's scheme, not schema.

A new patch is coming that renames schema to scheme.

Comment 56

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #39)
> Test-wise, I don't think the test is strong enough as-is.  Specifically, it
> looks like the test assumes opening the database would fail if the upgrade
> fails, but OpenDatabaseOp::LoadDatabaseInformation's enforcement is just an
> NS_WARNING:
>   if (mOrigin != origin) {
>     NS_WARNING("Origins don't match!");
>   }
> 
> For the test to work, the open needs to return an error instead. 
> Alternately, it could be made into an assert.  The downside to doing this is
> that I suppose it would cause :bent's long-unsupported
> https://github.com/benturner/idbbrowser extension to work even less.  And it
> could stymie users trying to debug things by manually copying databases
> around.  However, we do have working IDB-in-devtools these days, and the
> scrambled filenames IDB uses already make it largely intractable to copy
> things between origins.

I didn't convert the warning to an assert or error, because there are URIs that
don't give us unique origins if parsed with the origin parser, for example:
file:///Users/joe/c++/index.html
file:///Users/joe/c///index.html

both end up in:
file++++Users+joe+c+++index.html

and when it's parsed, we get:
file:///Users/joe/c///index.html

I think, there can be other cases when file:// ends up with non unique origin.

> I think a comment mentioning that the origin directory in the zip file is
> "moz-extension+++8ea6d31b-917c-431f-a204-15b95e904d4f^addonId=indexedDB-
> test%40kmaglione.mozilla.com" would help make it more clear what's being
> tested.

Ok, I added a comment.

Comment 57

[Jan Varga [:janv]]

Attachment: Part 14: Don't try to upgrade obsolete origins, remove them right after we detect them

Comment 58

[Jan Varga [:janv]]

Attachment: Part 15: Shift code to correct place

Comment 59

[Jan Varga [:janv]]

Attachment: Part 16: Remove apps data as part of new upgrade helper (needed for bug 1311057), add a bunch of new tests to verify upgrade methods

Comment 60

[Jan Varga [:janv]]

Attachment: Part 17: Strip obsolete origin attributes as part of new upgrade helper (needed for bug 1314361), original patch by Kris Maglione

Comment 61

[Jan Varga [:janv]]

Attachment: Part 18: Simplify File.createFromNsIFile() usage

Comment 62

[Jan Varga [:janv]]

Attachment: Part 19: Strip obsolete origin attributes in IndexedDB databases (needed for bug 1314361), original patch by Kris Maglione

Comment 63

[Jan Varga [:janv]]

Attachment: Part 20: Rationalize IndexedDB origin initialization code (preparation for sharing with a new upgrade method)

Comment 64

[Jan Varga [:janv]]

Attachment: Part 21: Rename file manager directories as part of new upgrade helper

Comment 65

[Jan Varga [:janv]]

Attachment: Part 22: Rename "schema" to "scheme" where appropriate

Comment 66

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8843715 [details] [diff] [review]
Part 14: Don't try to upgrade obsolete origins, remove them right after we detect them

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

Love the cleanup!

Comment 67

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8843716 [details] [diff] [review]
Part 15: Shift code to correct place

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

Your efforts to isolate changes that the diff algorithm performs badly on are very much appreciated!

Comment 68

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jan Varga [:janv] from comment #53)
> Here's how it looks like:
> 
> // The upgrade consists of a number of logically distinct bugs that
> // intentionally got fixed at the same time to trigger just one major
> // version bump.

These comments and the convention you're adopting here are fantastic.  Hopefully they also prove to be time-savers when analyzing reported bugs!

Comment 69

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jan Varga [:janv] from comment #56)
> (In reply to Andrew Sutherland [:asuth] from comment #39)
> > Test-wise, I don't think the test is strong enough as-is.  Specifically, it
...
> I didn't convert the warning to an assert or error, because there are URIs
> that
> don't give us unique origins if parsed with the origin parser, for example:
> file:///Users/joe/c++/index.html
> file:///Users/joe/c///index.html
> 
> both end up in:
> file++++Users+joe+c+++index.html

My concern with (now) part 19 is that if you removed the IDB ActorsParent.cpp changes from the patch, then the test will still pass.  As it stands, the test is another test of the QM logic.  Verifying the IDB behavior depends on someone manually running the test and ensuring that NS_WARNING output doesn't show.

How about exposing QM's SanitizeOriginString directly or as a helper like (a non-argument-mutating) "AreOriginsStoredTogetherOnDisk(a, b)" and converting LoadDatabaseInformation to use whichever to return an error if the sanitized origins don't match?

If you're worried the additional enforcement might run up against violations in user installations, maybe an IDB_WARNING could be added and the test could assert that the warning text isn't logged to the (browser) console?  (I don't think the NS_WARNING is easily detected by the test itself?)

Alternately, add a comment documenting that a human really needs to verify the NS_WARNING output doesn't happen.  I trust you to have verified manually that the SQL function works and don't really expect this functionality to regress, and I think it's great to have the verification test checked into the tree, but if it's checked in, I think it should be clearly labeled.

Comment 70

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8843721 [details] [diff] [review]
Part 20: Rationalize IndexedDB origin initialization code (preparation for sharing with a new upgrade method)

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

::: dom/indexedDB/ActorsParent.cpp
@@ +17910,5 @@
> +      nsString path;
> +      MOZ_ALWAYS_SUCCEEDS(file->GetPath(path));
> +      MOZ_ASSERT(!path.IsEmpty());
> +
> +      nsPrintfCString warning(R"(Refusing to open databases for "%s" because )"

Neat!

Comment 71

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8843722 [details] [diff] [review]
Part 21: Rename file manager directories as part of new upgrade helper

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

(In reply to Jan Varga [:janv] from comment #50)
> > +  UpgradeStorageFrom1_0To2_0(nsIFile* aDirectory)
> 
> We could also add something like UpgradeStorage(nsIFile* aDirectory, int32_t
> aOldVersion, int32_t aNewVersion), but I'm not sure if it's worth the
> additional complexity.

I definitely think you made the right call here.  The code is going to be written as discrete version upgrades anyways, and anyone investigating the code using current tooling like searchfox/DXR will benefit from the explicit method names.

Also, great cleanup here and on those that fed into it.

Comment 72

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #69)
> My concern with (now) part 19 is that if you removed the IDB
> ActorsParent.cpp changes from the patch, then the test will still pass.  As
> it stands, the test is another test of the QM logic.  Verifying the IDB
> behavior depends on someone manually running the test and ensuring that
> NS_WARNING output doesn't show.

Oh, you are right. I'll try to automatize this somehow in an additional patch.

Anyway thanks for quick reviews!

Comment 73

[Jan Varga [:janv]]

Attachment: Part 23: Treat it as an error if the origin stored in the database doesn't match the origin used to open the database

Comment 74

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=548d3504546aed40f3f37348bf6dc36e1d98f2ea

Comment 75

[Jan Varga [:janv]]

Ok, thanks. I'm going to land this finally!

Comment 76

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95580390d302
Part 1: Create a standalone testing infrastructure with a basic test; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c88cea1dfbc4
Part 2: Add a method for testing storage initialization; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce9c2480cd9
Part 3: Split OriginClearOp to make code more readable and cleaner; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4c339ac82b
Part 4: Add a result attribute to nsIQuotaRequest; r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/d105ab94f5bc
Part 5: Add a method for testing origin initialization; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/691e3bc4f183
Part 6: Fix unknown file handling; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/be36f7ad8f15
Part 7: Implement safe metadata writting using temp files; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/3830a1f5acdb
Part 8: Rename the upgrade helper to match the method that is using it; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/54064fdef8a0
Part 9: Allow origin properties to be checked before they are added to the list for processing in upgrade helpers; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b6895403af
Part 10: Unify GetDirectoryMetadata() in upgrade helpers; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8972b262292d
Part 11: Fallback to PR_Now() if GetLastModifiedTime() fails; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/53717af6cc12
Part 12: Add a baseline helper for 1.0 to 2.0 upgrade; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c4db7bae51
Part 13: Move morgue directory cleanup to new upgrade helper; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fa25bc31d1
Part 14: Don't try to upgrade obsolete origins, remove them right after we detect them; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/de6a57b28704
Part 15: Shift code to correct place; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d13de25b640
Part 16: Remove apps data as part of new upgrade helper (needed for bug 1311057), add a bunch of new tests to verify upgrade methods; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/83d8f200c087
Part 17: Strip obsolete origin attributes as part of new upgrade helper (needed for bug 1314361), original patch by Kris Maglione; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5740cdbe500d
Part 18: Simplify File.createFromNsIFile() usage; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa58383f87b5
Part 19: Strip obsolete origin attributes in IndexedDB databases (needed for bug 1314361), original patch by Kris Maglione; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77f0b972eb2
Part 20: Rationalize IndexedDB origin initialization code (preparation for sharing with a new upgrade method); r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8c440c6a96
Part 21: Rename file manager directories as part of new upgrade helper; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/164b54df9312
Part 22: Rename "schema" to "scheme" where appropriate; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1812fe0216
Part 23: Treat it as an error if the origin stored in the database doesn't match the origin used to open the database; r=asuth

Comment 77

[Jan Varga [:janv]]

Needinfo myself to send a heads up to dev-platform once this sticks and is merged to m-c

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95580390d302
https://hg.mozilla.org/mozilla-central/rev/c88cea1dfbc4
https://hg.mozilla.org/mozilla-central/rev/cce9c2480cd9
https://hg.mozilla.org/mozilla-central/rev/1f4c339ac82b
https://hg.mozilla.org/mozilla-central/rev/d105ab94f5bc
https://hg.mozilla.org/mozilla-central/rev/691e3bc4f183
https://hg.mozilla.org/mozilla-central/rev/be36f7ad8f15
https://hg.mozilla.org/mozilla-central/rev/3830a1f5acdb
https://hg.mozilla.org/mozilla-central/rev/54064fdef8a0
https://hg.mozilla.org/mozilla-central/rev/e8b6895403af
https://hg.mozilla.org/mozilla-central/rev/8972b262292d
https://hg.mozilla.org/mozilla-central/rev/53717af6cc12
https://hg.mozilla.org/mozilla-central/rev/c6c4db7bae51
https://hg.mozilla.org/mozilla-central/rev/a9fa25bc31d1
https://hg.mozilla.org/mozilla-central/rev/de6a57b28704
https://hg.mozilla.org/mozilla-central/rev/5d13de25b640
https://hg.mozilla.org/mozilla-central/rev/83d8f200c087
https://hg.mozilla.org/mozilla-central/rev/5740cdbe500d
https://hg.mozilla.org/mozilla-central/rev/aa58383f87b5
https://hg.mozilla.org/mozilla-central/rev/b77f0b972eb2
https://hg.mozilla.org/mozilla-central/rev/bb8c440c6a96
https://hg.mozilla.org/mozilla-central/rev/164b54df9312
https://hg.mozilla.org/mozilla-central/rev/5a1812fe0216

Comment 79

[Andrew Overholt [:overholt]]

(In reply to Jan Varga [:janv] from comment #77)
> Needinfo myself to send a heads up to dev-platform once this sticks and is
> merged to m-c

Jan wrote an email: https://groups.google.com/d/msg/mozilla.dev.platform/wt1Jzzd6GdI/IU_jsao6BAAJ