1311057 - Remove IsApp flags from the quota manager and DOM cache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[(no longer active)]

Follow-up from bug 1308075.

Comment 1

[Jan Varga [:janv]]

Attachment: part 1

Comment 2

[Jan Varga [:janv]]

Attachment: part 2

Comment 3

[Jan Varga [:janv]]

Attachment: part 3

Comment 4

[Jan Varga [:janv]]

Attachment: part 4: Move app related data to trash

Comment 5

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #4)
> Created attachment 8809737 [details] [diff] [review]
> part 4: Move app related data to trash

Now that support for mozApps has been removed, do you guys think we can move all app related data managed by quota manager (asmjscache, dom cache, indexedDB) to a newly created <profile>/storage/trash ?
So such dirs will still take space on the disk, but they will not participate in default/temporary storage.

Comment 6

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

I'm not sure there's a benefit to keeping the data around.  2 Questions:

1) Didn't WebRT store installed app data in a different profile for desktop?  (I'm not sure for Android.)  That is, would the trash/ code path even activate in the user's actual profiles?  Needinfo'ing :myk.

2) What's the point of keeping the data around if it's not accessible to the user?  The Web Runtime was retired in desktop in bug 1238079 in Firefox 48 and in Fennec in bug 1235869 in Firefox 47.  I think this means the data will already have been inaccessible.  A "trash/" or "limbo/" directory would have made sense if we had a plan to help users resurrect the data like:
  - A way of re-associating the data with a specific origin by pushing updated magic data from the marketplace.
  - A way of allowing webextensions to pull data out of trash/limbo for use by a WebExtension superseding the original app or being helpful and trying to implement the previous bullet point.
I think it's too late for these things.  As far as the user is concerned, the data is gone and we're not doing them any disk usage or privacy favors by keeping it around.  In terms of privacy, I mean that users will normally expect that if they can't get to data, then the data is gone.

Comment 7

[Jan Varga [:janv]]

I'm not that familiar with Web Runtime that's why I chose the safer way. However if we are convinced that data for principals with appId != nsIScriptSecurityManager::NO_APP_ID can be just deleted then I will rework the patch.

Comment 8

[(no longer active)]

I think removing any app related data makes sense, since as things stand, there's no way to use apps in Gecko on mozilla-central any more.  However since we're talking about removing user data, I'd like to get a confirmation from Myk as well before we proceed.  He's needinfo'ed here already!

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to Andrew Sutherland [:asuth] from comment #6)
> 1) Didn't WebRT store installed app data in a different profile for desktop?
> (I'm not sure for Android.)  That is, would the trash/ code path even
> activate in the user's actual profiles?  Needinfo'ing :myk.

Yes.  On both Android and desktop, the web runtime stored app data in a separate profile for each app (except for meta-data about the app that was managed by the DOMAppRegistry).  On desktop, it actually stored the data in a separate profile within a separate data directory (i.e. for each app, a separate subdirectory of ~/AppData/Roaming/ on Windows and ~/Library/Application Support/ on Mac).

(In reply to :Ehsan Akhgari from comment #8)
> I think removing any app related data makes sense, since as things stand,
> there's no way to use apps in Gecko on mozilla-central any more.  However
> since we're talking about removing user data, I'd like to get a confirmation
> from Myk as well before we proceed.  He's needinfo'ed here already!

It's hard for me to give this confirmation, since I don't know what data would actually be removed (if any).  But I can confirm that the data of "open web apps" (i.e. apps that were run via the web runtime) lived in separate profiles on Android and desktop, so deleting app data from the user's Firefox profile won't affect it.

Indeed, we planned to (but never proceeded to) delete the profiles on Android in bug 1242787, which is consistent with the uninstall behavior of Android apps.  Whereas we planned to (and did) leave the profiles on desktop, which is consistent with uninstall behavior there (for better or worse).

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Are these patches close to ready to merge? I'm working on the private browsing IDB stuff and will need to add PB flags to quite a few of the same places that IsApp was. If I can do that on top of these patches (they don't need to land ASAP, IDB PB is still a ways off so I'll just merge them into my dev branch at a point before my patches), it'll save us a lot of conflict unhappiness.

Comment 11

[Jan Varga [:janv]]

Yes, I want to get reviews and land this right after I get back home, this Thursday if everything goes well.

Comment 12

[(no longer active)]

(In reply to Kyle Machulis [:qdot] from comment #10)
> Are these patches close to ready to merge? I'm working on the private
> browsing IDB stuff and will need to add PB flags to quite a few of the same
> places that IsApp was.

Please don't do that.

Is there a principal available where you would like to check the PB status?  If yes, you can use nsIPrincipal::PrivateBrowsingId() > 0 to check whether you're in PB mode.  If not, you need to get a principal (or the OriginAttributes of a principal) down to that place.  If you end up passing boolean flags around, you're definitely doing something wrong.  :-)

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to :Ehsan Akhgari from comment #12) 
> Is there a principal available where you would like to check the PB status? 
> If yes, you can use nsIPrincipal::PrivateBrowsingId() > 0 to check whether
> you're in PB mode.  If not, you need to get a principal (or the
> OriginAttributes of a principal) down to that place.  If you end up passing
> boolean flags around, you're definitely doing something wrong.  :-)

Basically, there's places where we store origin properties (OriginProps, etc) where it'd be nice to have the private browsing info, so I can do things like file cleanup using mostly existing code. I suppose I can just have that store OriginAttributes on construction, instead of just the PrivateBrowsing bool.

Comment 14

[(no longer active)]

(In reply to Kyle Machulis [:qdot] from comment #13)
> (In reply to :Ehsan Akhgari from comment #12) 
> > Is there a principal available where you would like to check the PB status? 
> > If yes, you can use nsIPrincipal::PrivateBrowsingId() > 0 to check whether
> > you're in PB mode.  If not, you need to get a principal (or the
> > OriginAttributes of a principal) down to that place.  If you end up passing
> > boolean flags around, you're definitely doing something wrong.  :-)
> 
> Basically, there's places where we store origin properties (OriginProps,
> etc) where it'd be nice to have the private browsing info, so I can do
> things like file cleanup using mostly existing code. I suppose I can just
> have that store OriginAttributes on construction, instead of just the
> PrivateBrowsing bool.

Better than that, you don't need to do anything whatsoever!  Just call OriginProps::mAttrs::PrivateBrowsingId() when you need to know whether you should honor PB.  :-)

Comment 15

[Jan Varga [:janv]]

Attachment: Part 1: Remove support for packaged apps from asmjscache

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8825175 [details] [diff] [review]
Part 1: Remove support for packaged apps from asmjscache

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

Cool

Comment 17

[Jan Varga [:janv]]

Attachment: Part 2: Remove isApp from quota manager and its clients

Please take a look at the cleanup patch too, I split it to make it easier to review.

Comment 18

[Jan Varga [:janv]]

Attachment: Part 3: Code simplification and cleanup after isApp removal

Comment 19

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

Comment on attachment 8825205 [details] [diff] [review]
Part 3: Code simplification and cleanup after isApp removal

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

Much simpler now!  Hooray for IsApp and its annoying persistence-upgrade semantics going away!

Comment 20

[Jan Varga [:janv]]

Attachment: Part 4: Delete app related data

Comment 21

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

Comment on attachment 8825568 [details] [diff] [review]
Part 4: Delete app related data

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

Restating my understanding of the test: Using a pre-packaged profile with an origin without appid and one with appid:
- verify that the appid profile directory got purged by the database needing to be created from scratch.  (There's slightly non-obvious fallthrough here, but it's safe because grabEventAndContinueHandler synchronously dispatches to the generator, so the manipulations of the request event handlers are sound.)
- verify that the non-appid profile directory did not get blown away (and implicitly that the profile unpacked okay; good sanity checking).

And the fix itself is that on the 1.0-to-2.0 upgrade that we blow anything away with an appid found per the origin parser.

I have some questions here relating to the appid/isolated-browser flags because I'm assuming they're doomed and if I knew the answers earlier in the week, I no longer know them now.  Feel free to ignore my questions if they're moot.  (There were a variety of other patches/related bugs in discussion, but they're not linked on this bug and I trust you to be aware of those even if I'm not.)

::: dom/indexedDB/test/unit/test_removeAppsUpgrade.js
@@ +25,5 @@
> +  function openDatabase(params) {
> +    let uri = ios.newURI(params.url, null, null);
> +    let principal =
> +      ssm.createCodebasePrincipal(uri,
> +                                  {appId: params.appId || ssm.NO_APPID,

Similarly to the ActorsParent case, it seems like eventually these arguments will go away and this might start throwing.  I think a comment/link explaining what needs to happen when that happens would be useful.  It's less obvious what the right thing to do is here since the inability to express a deprecated origin in code inhibits the ability for detecting whether it's actually gone or not.  Does this this test actually want to just be poking the quota manager so that it upgrades the profile and then walking the storage subdir and asserting there's no directory with the name "https+++developer.cdn.mozilla.net^appId=1007&inBrowser=1" anywhere?

::: dom/quota/ActorsParent.cpp
@@ +4203,5 @@
> +      if (NS_WARN_IF(!result)) {
> +        return NS_ERROR_FAILURE;
> +      }
> +
> +      if (attrs.mAppId == nsIScriptSecurityManager::NO_APP_ID ||

What's the plan when PrincipalOriginAttributes no longer has mAppId?  It seems like it's probably appropriate to describe or link to the plan here for whoever ends up breaking this code.  Feel free to just name-check the relevant bug/patch here if you already have the fix and it's imminently landing.

(I presume the fix is just for the OriginParser to gain an extra argument like RetiredOriginAttributes and/or just a boolean flag that indicates the directory is obsolete, and that becomes the basis of deleting the directory.)

@@ +4208,5 @@
> +          attrs.mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> +        continue;
> +      }
> +
> +      QM_WARNING("Deleting %s directory!",

I think a slightly more verbose string like "Deleting obsolete %s directory that is no longer a legal origin!" may help make this more obvious to anyone running afoul of this and wondering what is up.

Comment 22

[Jan Varga [:janv]]

Yeah appId is going away which means extra work in quota manager and tests to keep upgrades working.

Comment 23

[Kris Maglione [:kmag]]

Can we add some assertions to prevent creating storage for origins with appIds after the migration? That should allow us to remove the appId origin attribute later without the need for another migration or schema bump. We'll just need to update the existing migration code to not rely on the mAppId property.

Comment 24

[Jan Varga [:janv]]

Attachment: Part 4: Delete app related data

Ok, so the test doesn't depend on appId stuff anymore. I actually created a separate test suite for quota manager and updated the test to not depend on IDB. For that I had to add a testing method to nsIQuotaManagerService to be able to initialize storage from the test.

Comment 25

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

Comment on attachment 8831812 [details] [diff] [review]
Part 4: Delete app related data

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

I love the test!  The explicit QM init is great and the test check is very clean and straightforward.

Restating stuff for my benefit:
- InitOp extends QuotaRequestBase which sets mNeedsQuotaManagerInit so it is guaranteed to trigger initialization.  This is verified by the newly introduced AssertStorageIsInitialized() which is the only thing the op actually does.  And init() will actually re-init after a call to nsIQuotaManagerService because QuotaManager::ResetOrClearCompleted() which is called the the ResetOrClearOp does `mStorageInitialized = false;`.

::: dom/quota/test/head.js
@@ +142,5 @@
> +
> +  return dirService.get("ProfD", Ci.nsIFile);
> +}
> +
> +function getOriginDir(path)

This looks new/different from indexedDB/unit/xpcshell-head-parent-process.js... given my recent file delimiter screw-up, I think it's worth adding a comment along the lines of the following to save people from having to worry about that or reminding them that the delimiter can be an issue:

Given a "/"-delimited path relative to the profile directory, return an nsIFile representing the path.  This does not test for the existence of the file or parent directories.  It is safe even on Windows where the directory separator is not "/", but make sure you're not passing in a "\"-delimited path.

::: dom/quota/test/test_removeAppsUpgrade.js
@@ +64,5 @@
> +  checkOriginDirectories();
> +
> +  info("Initializing storage");
> +
> +  initStorage(continueToNextStepSync);

I don't understand why you're resetting QM or re-initializing it, at least not without checking for other side-effects.  The only way these last two calls to checkOriginDirectories could fail is if the dead origin comes back to life (very unlikely), or the safe origin gets nuked (also seems unlikely).

Were you planning to re-installPackage("removeAppsUpgrade"); after the upgrade/reset to re-establish the app-origin directory and then re-init to make sure the app-origin dir stays there to verify that once QM thinks it has upgraded to v2.0 it doesn't do this obsolete origin sweep a second time?  That's what makes the most sense to me unless you meant to have the requests be able to provide failure results (they can't currently), or make sure no assertions trigger.  If you're depending on assertions, I think it's appropriate to add comments to that effect.  Or if it's something else I missed, please also do add comments even if it's just "paranoia: make sure nothing explodes the first time we initialize *after* an upgrade".  Otherwise I think it makes sense to remove these checks.

Comment 26

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #25)
> I love the test!  The explicit QM init is great and the test check is very
> clean and straightforward.

Thanks :)

> 
> Restating stuff for my benefit:
> - InitOp extends QuotaRequestBase which sets mNeedsQuotaManagerInit so it is
> guaranteed to trigger initialization.  This is verified by the newly
> introduced AssertStorageIsInitialized() which is the only thing the op
> actually does.  And init() will actually re-init after a call to
> nsIQuotaManagerService because QuotaManager::ResetOrClearCompleted() which
> is called the the ResetOrClearOp does `mStorageInitialized = false;`.

Exactly.

> ::: dom/quota/test/head.js
> @@ +142,5 @@
> > +
> > +  return dirService.get("ProfD", Ci.nsIFile);
> > +}
> > +
> > +function getOriginDir(path)
> 
> This looks new/different from
> indexedDB/unit/xpcshell-head-parent-process.js... given my recent file

Hm, I just copied it from installPackagedProfile(), I don't think it's different.
Anyway, I'll add the comment you suggested below

> delimiter screw-up, I think it's worth adding a comment along the lines of
> the following to save people from having to worry about that or reminding
> them that the delimiter can be an issue:
> 
> Given a "/"-delimited path relative to the profile directory, return an
> nsIFile representing the path.  This does not test for the existence of the
> file or parent directories.  It is safe even on Windows where the directory
> separator is not "/", but make sure you're not passing in a "\"-delimited
> path.
> 
> ::: dom/quota/test/test_removeAppsUpgrade.js
> @@ +64,5 @@
> > +  checkOriginDirectories();
> > +
> > +  info("Initializing storage");
> > +
> > +  initStorage(continueToNextStepSync);
> 
> I don't understand why you're resetting QM or re-initializing it, at least
> not without checking for other side-effects.  The only way these last two
> calls to checkOriginDirectories could fail is if the dead origin comes back
> to life (very unlikely), or the safe origin gets nuked (also seems unlikely).
> 
> Were you planning to re-installPackage("removeAppsUpgrade"); after the
> upgrade/reset to re-establish the app-origin directory and then re-init to
> make sure the app-origin dir stays there to verify that once QM thinks it
> has upgraded to v2.0 it doesn't do this obsolete origin sweep a second time?
> That's what makes the most sense to me unless you meant to have the requests
> be able to provide failure results (they can't currently), or make sure no
> assertions trigger.  If you're depending on assertions, I think it's
> appropriate to add comments to that effect.  Or if it's something else I
> missed, please also do add comments even if it's just "paranoia: make sure
> nothing explodes the first time we initialize *after* an upgrade". 
> Otherwise I think it makes sense to remove these checks.

Hm, you are right, the reset() doesn't provide any real benefit in the test right now.
I'll remove it. It will be even nicer and cleaner.

Comment 27

[Jan Varga [:janv]]

Attachment: Part 4: Delete app related data

Ok, this is it.

Comment 28

[Jan Varga [:janv]]

This is ready to land, but we want to land it together with bug 1314361 to create less confusion around storage major version changes.

Comment 29

[Jan Varga [:janv]]

Attachment: Part 4: Delete app related data

Moved new xpcshell test and supporting infrastructure from dom/quota/test to dom/quota/test/unit

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a467cae4dde6
Part 1: Remove support for packaged apps from asmjscache; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb4ec26174e
Part 2: Remove isApp from quota manager and its clients; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c5d9e6c47f
Part 3: Code simplification and cleanup after isApp removal; r=asuth

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a467cae4dde6
https://hg.mozilla.org/mozilla-central/rev/efb4ec26174e
https://hg.mozilla.org/mozilla-central/rev/e5c5d9e6c47f