752407 - Thumbnail cache should be created in the Local profile folder, not the Roaming ones

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Danial Horton]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
Build ID: 20120506030520

Steps to reproduce:

Opened a recent nightly with the new thumbnail api/store folder


Actual results:

\Thumbnails\ was created in AppData\Roaming\Mozilla\Firefox\Profiles\4-Nightly\ AppData\Local\Mozilla\Firefox\Profiles\4-Nightly\


Expected results:

As a destroyable cache, this should have been placed in AppData\Local\Mozilla\Firefox\Profiles\4-Nightly\

The current location poses an annoyance to backup tools, as it will be now including alot of uncompressible megabytes  by default in the backup archive.

Comment 1

[Danial Horton]

err, woops, that actual results should have only listed the Roaming target >.<

Comment 2

[Marco Bonardo [:mak]]

I think makes sense considered it's volatile data that can be rebuilt (sort of a cache), though I'm not sure whether it was done on purpose considering that valuable data one may not be willing to lose when migrating (in such a case this may end up being a wontfix)

Comment 3

[Danial Horton]

Backup tools can easily be adapted to transfer local data,

but this has the potential of being a security concern.

Consider the scenario where a user is working on some corporate images and documents in his private office, but the User profiles are stored elsewhere.

In this case, instead of the images being stored on his pc in appdata\Local, they are written back to his user profile on the server

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [:mak] from comment #2)
> I think makes sense considered it's volatile data that can be rebuilt (sort
> of a cache), though I'm not sure whether it was done on purpose considering
> that valuable data one may not be willing to lose when migrating (in such a
> case this may end up being a wontfix)

Thumbnails are considered somewhat volatile and get rebuilt.

So we're currently using "ProfD/thumbnails" as the directory. Should we instead use "LocalAppData/thumbnails"? Is this a Win-only fix then?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Here's how to get the Local Profile dir: https://mxr.mozilla.org/mozilla2.0/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp#1370

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #5)
> Here's how to get the Local Profile dir:
> https://mxr.mozilla.org/mozilla2.0/source/toolkit/components/url-classifier/
> src/nsUrlClassifierDBService.cpp#1370

Thanks!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use the local profile folder to store thumbnails, not the roaming one

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use the local profile folder to store thumbnails, not the roaming one

Correction, forgot a ProfD occurence.

Comment 9

[Marco Bonardo [:mak]]

I just found bug 606575 that looks interesting and likely to be fixed first, cause on first profile creation we'd still put thumbs in the wrong place.

Actually, this patch changes the default folder, though I wonder if we should provide some migration path, so that the old roaming folder gets deleted, some users already have it taking a bunch of space (even 1GB from what I heard) and after this patch will just be a zombie folder and we'll take double the space.  It's true this only went to Nightly testers, though many may not be aware of this change and the need to remove the roaming folder.

Comment 10

[Marco Bonardo [:mak]]

do we have some kind of "versioning" of the thumbnails service storage?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [:mak] from comment #9)
> I just found bug 606575 that looks interesting and likely to be fixed first,
> cause on first profile creation we'd still put thumbs in the wrong place.

Good catch, thanks.

> Actually, this patch changes the default folder, though I wonder if we
> should provide some migration path, so that the old roaming folder gets
> deleted, some users already have it taking a bunch of space (even 1GB from
> what I heard) and after this patch will just be a zombie folder and we'll
> take double the space.  It's true this only went to Nightly testers, though
> many may not be aware of this change and the need to remove the roaming
> folder.

Yes, we should do this.

(In reply to Marco Bonardo [:mak] from comment #10)
> do we have some kind of "versioning" of the thumbnails service storage?

Not yet, as per our conversation on IRC we should add a simple preference that holds the current version number of the thumbnail storage.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 627138 [details] [diff] [review]
use the local profile folder to store thumbnails, not the roaming one

Need to address comment #9.

Comment 13

[Raymond Lee [:raymondlee]]

(In reply to Tim Taubert [:ttaubert] from comment #12)
> Comment on attachment 627138 [details] [diff] [review]
> use the local profile folder to store thumbnails, not the roaming one
> 
> Need to address comment #9.

Does it mean we should migrate/move the thumbnails dir from the roaming folder to the local profile one if it exists or simply remove the thumbnails dir in the roaming folder?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Raymond Lee [:raymondlee] from comment #13)
> Does it mean we should migrate/move the thumbnails dir from the roaming
> folder to the local profile one if it exists or simply remove the thumbnails
> dir in the roaming folder?

As this is for Nightly testers only I'd say we should just remove the thumbnails folder in the roaming profile. We should not try to copy it as the roaming profile might be located on some network drive and this could take quite a long time. It's basically just to clean up things.

Comment 15

[Matthew N. [:MattN]]

Marking tracking-firefox15 since deploying Firefox 15 at an enterprise with networked profiles could quickly fill the server's hard drive with thumbnails.

Comment 16

[Jo Hermans]

If the thumbnail cache is limited to a real tiny cache (depending on how the expiration will be done in bug 754671, for example when only the top-9 are stored), it might not matter anymore if it's stored in Local or Roaming. Storing it in Local can also be considered a disadvantage if you log in on a different computer, and there's no cache anymore. Just saying ...

Comment 17

[Gian-Carlo Pascutto [:gcp]]

I agree that the reasoning in comment 15 for making this bug tracking doesn't make much sense. The problem is bug 754671 not getting fixed and/or bug 744388 not being backed out.

If those get fixed, this bug is mostly a non-issue. If those don't get fixed, this bug will do little to prevent the problem, you'll just have huge (multi-gigabyte) and useless Local profile dirs instead.

Comment 18

[Matthew N. [:MattN]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #17)
> I agree that the reasoning in comment 15 for making this bug tracking
> doesn't make much sense. The problem is bug 754671 not getting fixed and/or
> bug 744388 not being backed out.

I'm not sure how you think the reasoning doesn't make sense.  We can't assume that bug 754671 will get fixed in 15 and depending on the fix it may not solve the problem of large roaming thumbnails folders. For example, a cap of 100MB still adds up (in terms of storage and bandwidth) when there are many users on a server.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

>For example, a cap of 100MB still adds up (in terms of storage and bandwidth) when 
>there are many users on a server.

Maybe I'm misunderstanding how much data the feature needs to store to work. From the description it didn't sound like something that needs that amount of space *when working correctly*.

>We can't assume that bug 754671 will get fixed in 15

That's right. With it Firefox has unbounded fast profile growth for all users. It doesn't matter if its in Local or Roaming - both are very bad. 

You're making the band-aid a tracking bug for 15, implying that we'd consider shipping 15 with the bug. I don't think that is the right thing to do.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

We'll track both bugs; IMO we should fix them both.

Comment 21

[Marco Bonardo [:mak]]

Regardless what we do with the size cap, I don't think we should use the roaming folder for volatile and non-dataloss-prone stuff, the roaming profile should stay as slim as possible, any cached data should go to the local profile.

Comment 22

[Marco Bonardo [:mak]]

Also, this has never been intended to band-aid the size issue, just to put things to their correct place.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use the local profile folder to store thumbnails, not the roaming one

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 630903 [details] [diff] [review]
use the local profile folder to store thumbnails, not the roaming one

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

::: browser/components/thumbnails/PageThumbs.jsm
@@ +328,5 @@
> +  },
> +
> +  migrate: function Migrator_migrate() {
> +    while (this.currentVersion < LATEST_STORAGE_VERSION)
> +      this.migrateToNextVersion();

please always brace loops

@@ +335,5 @@
> +  migrateToNextVersion: function Migrator_migrateToNextVersion() {
> +    let version = this.currentVersion;
> +
> +    if (version == 0)
> +      this.removeThumbnailsFromRoamingProfile();

I think you may want to manage downgrades... so if a user goes back to a version using roaming profile, you should migrate him again once he upgrades again.

You may always set the version to latest, even on downgrade (that means reducing it), on upgrade the migration goes again through the steps, that should be built taking into account the fact they may be applied to a profile already migrated (this checking exists and such things).

Btw, in this case likely doesn't matter much cause this never made a release, so you may handle that in a followup or when needed.

@@ +339,5 @@
> +      this.removeThumbnailsFromRoamingProfile();
> +  },
> +
> +  removeThumbnailsFromRoamingProfile:
> +    function Migrator_removeThumbnailsFromRoamingProfile() {

don't indent this, just keep it at same level as the label and avoid the next newline

@@ +353,5 @@
> +      }
> +    }
> +
> +    // Bump the version number.
> +    this.currentVersion = 1;

I think from a code point of view would be better to do this inside migrateToNextVersion, the migrator helper itself should not take care of bumping the version, it's not its task, and is somehow hiding the fact.
What we do usually is something like

if (version < 1)
  doSomething
if (version < 2)
  doSomethingElse
...
setVersion(LATEST)

that basically would merge migrate and migrateToNextVersion to one method.
Though you may also just do, in migrateToNextVersion:

if (version < 1) {
  doSomething
  bump
} else if (version < 2) {
  doSomethingElse
  bump
}

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use the local profile folder to store thumbnails, not the roaming one

(In reply to Marco Bonardo [:mak] from comment #24)
> I think you may want to manage downgrades... so if a user goes back to a
> version using roaming profile, you should migrate him again once he upgrades
> again.

Yes, good point. Setting version now always to the latest. The first migration can handle an already migrated profile and we'll take it into account for further migrations.

> I think from a code point of view would be better to do this inside
> migrateToNextVersion, the migrator helper itself should not take care of
> bumping the version, it's not its task, and is somehow hiding the fact.
> What we do usually is something like
> 
> if (version < 1)
>   doSomething
> if (version < 2)
>   doSomethingElse
> ...
> setVersion(LATEST)
> 
> that basically would merge migrate and migrateToNextVersion to one method.

I implemented exactly this. It's nicer and easier to read.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/960b6d4ea73b

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/960b6d4ea73b

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Marking Fx 15 as unaffected because bug 744388 has been backed out on Aurora.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/439a4b36aab8