Last Comment Bug 752407 - Thumbnail cache should be created in the Local profile folder, not the Roaming ones
: Thumbnail cache should be created in the Local profile folder, not the Roamin...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 16
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 606575 767411
Blocks: 744388 754671
  Show dependency treegraph
 
Reported: 2012-05-06 22:32 PDT by Danial Horton
Modified: 2012-08-17 19:16 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
unaffected


Attachments
use the local profile folder to store thumbnails, not the roaming one (886 bytes, patch)
2012-05-25 01:50 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
use the local profile folder to store thumbnails, not the roaming one (1.51 KB, patch)
2012-05-25 01:52 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
use the local profile folder to store thumbnails, not the roaming one (4.25 KB, patch)
2012-06-07 02:44 PDT, Tim Taubert [:ttaubert]
mak77: review+
Details | Diff | Splinter Review
use the local profile folder to store thumbnails, not the roaming one (4.07 KB, patch)
2012-06-18 03:16 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review

Description Danial Horton 2012-05-06 22:32:34 PDT
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 2012-05-06 22:33:56 PDT
err, woops, that actual results should have only listed the Roaming target >.<
Comment 2 Marco Bonardo [::mak] 2012-05-07 13:10:57 PDT
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 2012-05-07 14:38:42 PDT
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] 2012-05-22 05:38:13 PDT
(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] 2012-05-25 01:29:04 PDT
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] 2012-05-25 01:47:33 PDT
(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] 2012-05-25 01:50:37 PDT
Created attachment 627136 [details] [diff] [review]
use the local profile folder to store thumbnails, not the roaming one
Comment 8 Tim Taubert [:ttaubert] 2012-05-25 01:52:44 PDT
Created attachment 627138 [details] [diff] [review]
use the local profile folder to store thumbnails, not the roaming one

Correction, forgot a ProfD occurence.
Comment 9 Marco Bonardo [::mak] 2012-05-25 05:23:29 PDT
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] 2012-05-25 05:24:20 PDT
do we have some kind of "versioning" of the thumbnails service storage?
Comment 11 Tim Taubert [:ttaubert] 2012-05-29 01:55:48 PDT
(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] 2012-05-30 04:35:43 PDT
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] 2012-05-31 01:27:21 PDT
(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] 2012-05-31 01:55:07 PDT
(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] 2012-06-02 22:38:12 PDT
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 2012-06-03 03:57:25 PDT
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] 2012-06-03 04:10:58 PDT
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] 2012-06-03 23:08:20 PDT
(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] 2012-06-03 23:57:07 PDT
>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: gavin@gavinsharp.com] 2012-06-04 18:44:08 PDT
We'll track both bugs; IMO we should fix them both.
Comment 21 Marco Bonardo [::mak] 2012-06-05 06:31:09 PDT
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] 2012-06-05 06:32:27 PDT
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] 2012-06-07 02:44:16 PDT
Created attachment 630903 [details] [diff] [review]
use the local profile folder to store thumbnails, not the roaming one
Comment 24 Marco Bonardo [::mak] 2012-06-15 03:40:46 PDT
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] 2012-06-18 03:16:35 PDT
Created attachment 633978 [details] [diff] [review]
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] 2012-06-18 07:06:56 PDT
https://hg.mozilla.org/integration/fx-team/rev/960b6d4ea73b
Comment 27 Tim Taubert [:ttaubert] 2012-06-19 06:15:15 PDT
https://hg.mozilla.org/mozilla-central/rev/960b6d4ea73b
Comment 28 Tim Taubert [:ttaubert] 2012-06-19 14:57:03 PDT
Marking Fx 15 as unaffected because bug 744388 has been backed out on Aurora.
Comment 29 Ed Morley [:emorley] 2012-06-20 02:21:31 PDT
https://hg.mozilla.org/mozilla-central/rev/439a4b36aab8

Note You need to log in before you can comment on or make changes to this bug.