Update FF 62 to know about upcoming new quota client

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: janv, Assigned: janv)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

11 months ago
This is a pre-patch for upcoming new local storage implementation.
See bug 1286798 comment 141.
I'll request a beta approval for this, since the merge day is today.
Assignee

Updated

11 months ago
Blocks: 1286798
Assignee

Updated

11 months ago
Assignee: nobody → jvarga
Assignee

Updated

11 months ago
Status: NEW → ASSIGNED
Assignee

Comment 1

11 months ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8987457 - Flags: review?(bugmail)
Assignee

Comment 2

11 months ago
Posted patch patchSplinter Review
sorry, attached old patch
Attachment #8987457 - Attachment is obsolete: true
Attachment #8987457 - Flags: review?(bugmail)
Attachment #8987459 - Flags: review?(bugmail)
Assignee

Updated

11 months ago
Attachment #8987459 - Attachment is patch: true
Comment on attachment 8987459 [details] [diff] [review]
patch

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

2 minor enhancements proposed for thoroughness in ActorsParent.cpp.  I've manually tested this and it works well.  Thanks for undertaking this!

## Restating

The next-gen version:
- The next-gen patches on bug 1286798 leave the following additional data on disk beyond the updated state in webappsstore.sqlite when all goes as planned:
  - A "ls" QuotaManager client that stores its data in "data.sqlite", with directories looking like PROFILE/storage/default/origin/ls/data.sqlite.
  - A "ls-archive.sqlite" file stored at PROFILE/storage/ls-archive.sqlite.
- The next-gen patches also have the following transient storage:
  - A "ls-archive-tmp.sqlite" at PROFILE/storage/ls-archive-tmp.sqlite that is the result of copying webappsstore.sqlite and later renamed to ls-archive-tmp.sqlite.
- The next-gen patches do not bump any schema versions.

QuotaManager in general:
- Freaks out when initializing as it relates to the above when it:
  - Finds an unknown client: https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/dom/quota/ActorsParent.cpp#4340
- Does not freak out as it relates to the above when:
  - There is extra stuff in the base directory of storage/ when there aren't upgrades involved because the directory's contents are not enumerated; explicit subdirs are just created: https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/dom/quota/ActorsParent.cpp#3585

This upgrade deals with the potential problems by:
- Deleting all the possible "ls" client directories.  (Which only happens under storage/default/, see specific comments below.)
- Deleting the ls-archive.sqlite file in the root.  There could also be a temporary file, so I'm requesting we cover that below too.
- The deletion is invoked post-database-creation/upgrade.  In an actual upgrade/fresh profile, the LS logic will still run, but will have no effect.  In the downgrade from 63 we're attempting to compensate for, the versions won't have changed, so the LS check will run and normalize things if necessary.  In a more dramatic downgrade to patched-62, say after we turn off the duplicative old-style-localstorage support, we'll either have bumped the QM major schema version, in which case we'll just fail to init QM, or we'll wipe the LS from the future and have no old webappsstore.sqlite around anymore.  Neither of those is great, but we're not magic, and we can control when we take those actions.

The really nice test includes a zipped profile that contains both the ls-archive.sqlite file and an "ls" client directory with file and verifies that the profile is expected, then triggers initialization and verifies the file from the future go away.

::: dom/quota/ActorsParent.cpp
@@ +4828,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +QuotaManager::MaybeRemoveLocalStorageData()

As I understand this method right now, it's behavior can be characterized as:
- Always check for LS_ARCHIVE_FILE_NAME.
- Delete the file if it exists.
- Always check if storage/default/ exists, bail if it does not.
- Iterate over the contents of storage/default/.  If there's a directory, check if it has an "ls" client sub-directory.  If it does, recursively remove that directory.

What's notable is that:
1. We're only checking storage/default's origins, not storage/permanent, not storage/temporary.
2. There's no fast-path.  We run this check every time at startup.

For the 2nd, I think we can and should optimize this.  The pseudo-code is:
- Check for "LS_ARCHIVE_FILE_NAME".
- If it's not there, there's no cleanup to do, return immediately.
- Do the origin sweep, deleting the "ls" client directories.
- Once that sweep has completed, delete the LS_ARCHIVE_FILE_NAME.

I think it would also be nice to handle the crash case where "ls-archive-tmp.sqlite" is present and delete that.  That would mean we're back up to 2 fstat's because we'd check for the temp file and then the real file, but that's safer for privacy reasons.


For the 1st, I understand that:
- QM-enabled next-gen localStorage only uses PERSISTENCE_TYPE_DEFAULT, so we only need to look in default.  To have things under temporary/, like the asmjscache, it would need to pass PERSISTENCE_TYPE_TEMPORARY, but it does not.  To have things under permanent/ it would need to pass PERSISTENCE_TYPE_PERSISTENT, but it does not.  (And there's no magic upgrade to persistent for being the system principal; that's something that clients like IndexedDB do based on consulting the principal themselves.  The next-gen LocalStorage implementation notably just refuses to provide any LocalStorage instances to the System Principal outright, but that's not actually a change because GenerateOriginKey in current firefox already refuses to provide LocalStorage for anything that lacks a URI associated with the origin, as is the case with the system principal.)
Attachment #8987459 - Flags: review?(bugmail) → review+
Assignee

Comment 4

11 months ago
(In reply to Andrew Sutherland [:asuth] from comment #3)
> 2. There's no fast-path.  We run this check every time at startup.
> 
> For the 2nd, I think we can and should optimize this.  The pseudo-code is:
> - Check for "LS_ARCHIVE_FILE_NAME".
> - If it's not there, there's no cleanup to do, return immediately.
> - Do the origin sweep, deleting the "ls" client directories.
> - Once that sweep has completed, delete the LS_ARCHIVE_FILE_NAME.

Hm, so in the next-gen bug, you want to propose to always create LS_ARCHIVE_FILE_NAME, even when there's no webappsstore.sqlite to copy from ?
Flags: needinfo?(bugmail)
Yes, although I hadn't realized that file could possibly not exist.  At least in my testing from a fresh profile, I ended up with an empty "ls-archive.sqlite" database because all of the contents got migrated.  It seems more consistent and easier to reason about to always have that file than to sometimes not have the file for an extremely rare edge-case, especially if we don't remove it when it becomes empty.

The alternatives that jump out at me would be to a) do something "clever" with the QM schema minor version numbers or b) to have the "ls" client cleanup logic happen during the already-existing QM initialization sweep.  I suppose there is an option c) of keeping things the way they are and hoping the OS caches the FS info sufficiently.  However, it still seems hygienically safer to only do the wipe once per downgrade.
Flags: needinfo?(bugmail)
Assignee

Comment 6

11 months ago
(In reply to Andrew Sutherland [:asuth] from comment #5)
> Yes, although I hadn't realized that file could possibly not exist.  At
> least in my testing from a fresh profile, I ended up with an empty
> "ls-archive.sqlite" database because all of the contents got migrated.  It
> seems more consistent and easier to reason about to always have that file
> than to sometimes not have the file for an extremely rare edge-case,
> especially if we don't remove it when it becomes empty.

Ok, I agree. I actually like this, I just wanted to make sure I fully understand your intention.

> The alternatives that jump out at me would be to a) do something "clever"
> with the QM schema minor version numbers or b) to have the "ls" client
> cleanup logic happen during the already-existing QM initialization sweep.  I
> suppose there is an option c) of keeping things the way they are and hoping
> the OS caches the FS info sufficiently.  However, it still seems
> hygienically safer to only do the wipe once per downgrade.

I think all this would be too complicated to do and maintain.
Assignee

Comment 7

11 months ago
Posted patch interdiffSplinter Review
Assignee

Comment 8

11 months ago
Posted patch patch v2Splinter Review
Attachment #8988568 - Flags: review?(bugmail)
Comment on attachment 8988568 [details] [diff] [review]
patch v2

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

Looks great and glad to have the extra test coverage for the temp file case too!
Attachment #8988568 - Flags: review?(bugmail) → review+

Comment 10

11 months ago
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb385983de8
Update FF 62 to know about upcoming new quota client; r=asuth

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0cb385983de8
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(In reply to Jan Varga [:janv] from comment #0)
> This is a pre-patch for upcoming new local storage implementation.
> See bug 1286798 comment 141.
> I'll request a beta approval for this, since the merge day is today.

Hi Jan, do you still intend to request a beta approval for this patch?
Flags: needinfo?(jvarga)
Assignee

Comment 13

11 months ago
Comment on attachment 8988568 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/Bug causing the regression]: It's not a regression, it's a pre-patch for upcoming bug 1286798.
[User impact if declined]: Users which switch channels using the same profile would run into problems. See bug 1286798 comment 141.
[Is this code covered by automated tests?]: Yes, there's an xpcshell-test for it.
[Has the fix been verified in Nightly?]: Yes, the patch landed 4 days ago.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No, it's very low risk.
[Why is the change risky/not risky?]: The patch is quite simple, fix was tested by the author of the patch, the reviewer did his own independent testing, and we see no regressions in nightly.
[String changes made/needed]: None
Flags: needinfo?(jvarga)
Attachment #8988568 - Flags: approval-mozilla-beta?
Assignee

Comment 14

11 months ago
(In reply to Pascal Chevrel:pascalc from comment #12)
> (In reply to Jan Varga [:janv] from comment #0)
> > This is a pre-patch for upcoming new local storage implementation.
> > See bug 1286798 comment 141.
> > I'll request a beta approval for this, since the merge day is today.
> 
> Hi Jan, do you still intend to request a beta approval for this patch?

Hi Pascal, yes I do, see previous comment. Thanks.
Comment on attachment 8988568 [details] [diff] [review]
patch v2

Patch to support profile migration (supporting a feature aimed at 63)
Let's uplift for beta 7.
Attachment #8988568 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.