Last Comment Bug 744388 - [Page Thumbnails] implement a custom storage, don't use the file cache
: [Page Thumbnails] implement a custom storage, don't use the file cache
Status: VERIFIED FIXED
[Snappy:P1]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 11 votes (vote)
: Firefox 16
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 764681 (view as bug list)
Depends on: 744743 752407 752409 754671 762094
Blocks: 742594 743563 658913 716949 724408 726267 730042 731157 734952 736724 739069 740287 744780 753768
  Show dependency treegraph
 
Reported: 2012-04-11 07:10 PDT by Tim Taubert [:ttaubert]
Modified: 2012-12-21 13:43 PST (History)
43 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled
+
verified


Attachments
patch v1 (20.82 KB, patch)
2012-04-12 04:59 PDT, Tim Taubert [:ttaubert]
taras.mozilla: feedback+
Details | Diff | Splinter Review
patch v2 (20.28 KB, patch)
2012-04-18 06:45 PDT, Tim Taubert [:ttaubert]
dietrich: feedback+
Details | Diff | Splinter Review
patch v3 (25.74 KB, patch)
2012-04-24 11:20 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v4 (16.32 KB, patch)
2012-04-25 08:40 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v5 (18.41 KB, patch)
2012-04-25 09:16 PDT, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
Backout for Aurora (Fx 15) (20.00 KB, patch)
2012-06-14 03:51 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-04-11 07:10:42 PDT
We ran into a lot of problems with the choice of the file cache as the current thumbnail storage. Thumbnails get lost in a lot of occasions and the file cache turns out not be as performant as we thought.

We're going to build a custom storage that stores thumbnails as separate files. This way we don't need to implement a custom channel to deliver those.

We need to take care of removing those files ourselves when cleaning the history and we must not capture thumbnails in private browsing mode.
Comment 1 Tim Taubert [:ttaubert] 2012-04-12 04:59:52 PDT
Created attachment 614337 [details] [diff] [review]
patch v1
Comment 2 Tim Taubert [:ttaubert] 2012-04-12 05:15:01 PDT
Comment on attachment 614337 [details] [diff] [review]
patch v1

Taras, I'd like to get feedback from you regarding some implementation details and performance parts of this patch.

>+++ b/browser/components/thumbnails/PageThumbs.jsm
>+  write: function Storage_write(aURL, aDataStream, aCallback) {
>+    let file = this.getFileForURL(aURL);
>+    let flags = FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE |
>+                FileUtils.MODE_TRUNCATE;
>+    let ostream = Cc["@mozilla.org/network/safe-file-output-stream;1"]
>+                    .createInstance(Ci.nsIFileOutputStream);
>+    ostream.init(file, flags, 384, ostream.DEFER_OPEN);
>+    NetUtil.asyncCopy(aDataStream, ostream, function (aResult) {
>+      aCallback(Components.isSuccessCode(aResult));
>     });
>   },

Is that the best way to asynchronously write a file? I got that part from the session store service.

>+  copy: function Storage_copy(aSourceURL, aTargetURL) {
>+    let sourceFile = this.getFileForURL(aSourceURL);
>+    let targetFile = this.getFileForURL(aTargetURL);
>+    sourceFile.copyTo(targetFile.parent, targetFile.leafName);
>   },

It's not in the startup path but is there maybe a better way of doing this?

>+  _calculateMD5Hash: function Storage_calculateMD5Hash(aValue) {
>+    let hash = gCryptoHash;
>+    let value = gUnicodeConverter.convertToByteArray(aValue);
>+
>+    hash.init(hash.MD5);
>+    hash.update(value, value.length);
>+    return this._convertToHexString(hash.finish(false));
>+  },

In order to determine a suitable file name for the thumbnail I'm calculating the MD5 hash of the given URL. Do you think that's too costly and we should rather use something like nsDiskCache::Hash()?
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-12 11:17:12 PDT
(In reply to Tim Taubert [:ttaubert] from comment #2)
> >+  copy: function Storage_copy(aSourceURL, aTargetURL) {
> >+    let sourceFile = this.getFileForURL(aSourceURL);
> >+    let targetFile = this.getFileForURL(aTargetURL);
> >+    sourceFile.copyTo(targetFile.parent, targetFile.leafName);
> >   },
> 
> It's not in the startup path but is there maybe a better way of doing this?

copyTo is sync :( Bug 716174 offers an alternative that's probably better, but it isn't ideal.
Comment 4 (dormant account) 2012-04-12 14:15:52 PDT
Comment on attachment 614337 [details] [diff] [review]
patch v1

There is no better way to do this for another couple of weeks
Comment 5 (dormant account) 2012-04-12 16:05:34 PDT
Comment on attachment 614337 [details] [diff] [review]
patch v1

I need to look at this some more
Comment 6 (dormant account) 2012-04-16 15:04:48 PDT
Comment on attachment 614337 [details] [diff] [review]
patch v1

I don't see where the copy() function is used
Comment 7 Tim Taubert [:ttaubert] 2012-04-16 15:07:48 PDT
(In reply to Taras Glek (:taras) from comment #6)
> I don't see where the copy() function is used

With the patch applied, PageThumbs.jsm:150.
Comment 8 (dormant account) 2012-04-16 15:17:16 PDT
Comment on attachment 614337 [details] [diff] [review]
patch v1

+    Services.telemetry.getHistogramById("FX_THUMBNAILS_HIT_OR_MISS")
+      .add(file.exists());
Don't do this. This causes a stat on main thread. Instead check return value of newChannelFromURI.
I wonder if we can load the file from disk using DEFER_OPEN + file io, since newChannelFromURI will do some combo of stat()/open().

Otherwise, as far as I can tell this patch is written as well(with regard to io) as the current api allows.
Comment 9 (dormant account) 2012-04-16 15:28:49 PDT
(In reply to Tim Taubert [:ttaubert] from comment #7)
> (In reply to Taras Glek (:taras) from comment #6)
> > I don't see where the copy() function is used
> 
> With the patch applied, PageThumbs.jsm:150.

cool. With the new file api we should be able to use hardlinks there :)
Comment 10 Tim Taubert [:ttaubert] 2012-04-18 06:45:54 PDT
Created attachment 616102 [details] [diff] [review]
patch v2

I'm still not sure how to implement removing thumbnails when the user sanitizes their history. We could maybe use a nsINavHistoryObserver and implement onDeleteVisits() and onDeleteURI() to check if there's a thumbnail for the given URI and remove it. Alas, onDeleteVisits() is also called when visits expire so that might not be a good way to do it.

As a second thought we could hook into browser/base/content/sanitize.js and call something like PageThumbs.removeAllThumbnails() or PageThumbs.removeThumbnailsByTimeframe(start, end) when "history" is sanitized. For removeThumbnailsByTimeframe() we'd need to iterate through all files in the thumbnail directory and check if their last modification time is in the time frame.
Comment 11 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-18 07:00:53 PDT
(In reply to Tim Taubert [:ttaubert] from comment #10)
> Created attachment 616102 [details] [diff] [review]
> patch v2
> 
> I'm still not sure how to implement removing thumbnails when the user
> sanitizes their history. We could maybe use a nsINavHistoryObserver and
> implement onDeleteVisits() and onDeleteURI() to check if there's a thumbnail
> for the given URI and remove it. Alas, onDeleteVisits() is also called when
> visits expire so that might not be a good way to do it.

you don't need onDeleteVisits, just onDeleteURI and onClearHistory.
Comment 12 Tim Taubert [:ttaubert] 2012-04-18 07:09:33 PDT
(In reply to Marco Bonardo [:mak] from comment #11)
> you don't need onDeleteVisits, just onDeleteURI and onClearHistory.

Ok, so using a nsINavHistoryObserver would work, too. The only thing we couldn't implement with that is when someone sanitizes a specific time range. Let's say I visited somepage.com yesterday and 30 mins ago. Today I looked at very strange things under the same URL somepage.com (because AJAX or iframe or whatever) that we took screenshots of. When sanitizing the last hour we remove today's visit to this page but we'd still keep the "content" (as an image) the user saw. Not sure if that's a relevant use case.
Comment 13 (dormant account) 2012-04-18 10:23:38 PDT
We could model our cache eviction behavior. if(rand()*20 ==13) nuke_everything()
Comment 14 Dietrich Ayala (:dietrich) 2012-04-23 09:50:29 PDT
Comment on attachment 616102 [details] [diff] [review]
patch v2

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

f+ on the approach. can update to proper async file IO later when available. needs per-URI sanitization per our IRL chat.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +227,5 @@
>  
> +  copy: function Storage_copy(aSourceURL, aTargetURL) {
> +    let sourceFile = this.getFileForURL(aSourceURL);
> +    let targetFile = this.getFileForURL(aTargetURL);
> +    sourceFile.copyTo(targetFile.parent, targetFile.leafName);

does this use NetUtils.asyncCopy in the background or are these nsIFile?
Comment 15 Tim Taubert [:ttaubert] 2012-04-23 09:59:46 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #14)
> does this use NetUtils.asyncCopy in the background or are these nsIFile?

No, those are nsIFiles and that's a sync copy afaik. Should I switch to asyncCopy (like bug 716174) as Gavin said? We could also handle that as a follow-up when FileUtils.copyFile() is actually implemented.
Comment 16 Tim Taubert [:ttaubert] 2012-04-24 11:20:45 PDT
Created attachment 617959 [details] [diff] [review]
patch v3

Implemented thumbnail removal on history sanitization.
Comment 17 Dietrich Ayala (:dietrich) 2012-04-25 07:33:41 PDT
Comment on attachment 617959 [details] [diff] [review]
patch v3

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

per conversation, need to leave cache as fallback for a release to avoid empty new tab page and panorama.
Comment 18 Tim Taubert [:ttaubert] 2012-04-25 08:40:27 PDT
Created attachment 618285 [details] [diff] [review]
patch v4

Keeping the old cache backend as fallback.
Comment 19 Josh Matthews [:jdm] 2012-04-25 08:42:53 PDT
I don't see any explicit private browsing awareness; is that planned, or am I missing something?
Comment 20 Tim Taubert [:ttaubert] 2012-04-25 08:43:55 PDT
That was fixed in bug 744743 as a preparation for this one.
Comment 21 Dão Gottwald [:dao] 2012-04-25 08:48:46 PDT
Comment on attachment 618285 [details] [diff] [review]
patch v4

>--- a/browser/base/content/browser-thumbnails.js
>+++ b/browser/base/content/browser-thumbnails.js
>@@ -34,18 +34,19 @@ let gBrowserThumbnails = {
>     gBrowser.addTabsProgressListener(this);
> 
>     this._tabEvents.forEach(function (aEvent) {
>       gBrowser.tabContainer.addEventListener(aEvent, this, false);
>     }, this);
> 
>     this._timeouts = new WeakMap();
> 
>-    XPCOMUtils.defineLazyModuleGetter(this, "_pageThumbs",
>-      "resource:///modules/PageThumbs.jsm", "PageThumbs");
>+    let tmp = {};
>+    Cu.import("resource:///modules/PageThumbs.jsm", tmp);
>+    this._pageThumbs = tmp.PageThumbs;

>--- a/browser/components/thumbnails/PageThumbs.jsm
>+++ b/browser/components/thumbnails/PageThumbs.jsm

>+PlacesUtils.history.addObserver(PageThumbsHistoryObserver, false);

This is a design flaw -- importing the module shouldn't silently do stuff. You should add an init method for this (and probably call it from nsBrowserGlue rather than from browser windows).
Comment 22 Tim Taubert [:ttaubert] 2012-04-25 09:16:45 PDT
Created attachment 618318 [details] [diff] [review]
patch v5

(In reply to Dão Gottwald [:dao] from comment #21)
> This is a design flaw -- importing the module shouldn't silently do stuff.
> You should add an init method for this (and probably call it from
> nsBrowserGlue rather than from browser windows).

Fixed. Thanks for pointing me to nsBrowserGlue, wasn't aware of that.
Comment 23 Dietrich Ayala (:dietrich) 2012-04-27 07:20:27 PDT
Comment on attachment 618318 [details] [diff] [review]
patch v5

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

per talk IRL, we should support thumbnails during private browsing mode, and cleared after. but that can be a followup.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +238,5 @@
> +
> +  copy: function Storage_copy(aSourceURL, aTargetURL) {
> +    let sourceFile = this.getFileForURL(aSourceURL);
> +    let targetFile = this.getFileForURL(aTargetURL);
> +    sourceFile.copyTo(targetFile.parent, targetFile.leafName);

does copyTo throw? if so, should catch and report errors.

@@ +253,5 @@
> +  wipe: function Storage_wipe() {
> +    try {
> +      FileUtils.getDir("ProfD", [THUMBNAIL_DIRECTORY]).remove(true);
> +    } catch (e) {
> +      /* The file might not exist or we're not permitted to remove it. */

should use Components.utils.reportError here, and above.
Comment 24 Dão Gottwald [:dao] 2012-04-27 07:24:31 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #23)
> per talk IRL, we should support thumbnails during private browsing mode, and
> cleared after. but that can be a followup.

We should access existing thumbnails but not store new ones. Nothing should be written to the disk in private browsing mode.
Comment 25 Dão Gottwald [:dao] 2012-04-27 07:25:49 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #23)
> > +  wipe: function Storage_wipe() {
> > +    try {
> > +      FileUtils.getDir("ProfD", [THUMBNAIL_DIRECTORY]).remove(true);
> > +    } catch (e) {
> > +      /* The file might not exist or we're not permitted to remove it. */
> 
> should use Components.utils.reportError here, and above.

It looks like the exception is expected, according to the comment. In that case we shouldn't spam the console.
Comment 26 Tim Taubert [:ttaubert] 2012-04-27 07:26:35 PDT
(In reply to Dão Gottwald [:dao] from comment #24)
> (In reply to Dietrich Ayala (:dietrich) from comment #23)
> > per talk IRL, we should support thumbnails during private browsing mode, and
> > cleared after. but that can be a followup.
> 
> We should access existing thumbnails but not store new ones. Nothing should
> be written to the disk in private browsing mode.

Let's flesh out the details in a follow-up bug. I agree, we shouldn't write to disk in pb mode and maybe just use some temporary storage.
Comment 27 Tim Taubert [:ttaubert] 2012-04-30 18:21:51 PDT
(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to Dietrich Ayala (:dietrich) from comment #23)
> > > +  wipe: function Storage_wipe() {
> > > +    try {
> > > +      FileUtils.getDir("ProfD", [THUMBNAIL_DIRECTORY]).remove(true);
> > > +    } catch (e) {
> > > +      /* The file might not exist or we're not permitted to remove it. */
> > 
> > should use Components.utils.reportError here, and above.
> 
> It looks like the exception is expected, according to the comment. In that
> case we shouldn't spam the console.

Yes, there could be an exception with wrong file/dir permissions - even if it's only for a single file in the whole thumbnails path. It's not a fatal error and we probably shouldn't bother the user.
Comment 28 Tim Taubert [:ttaubert] 2012-04-30 18:25:52 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #23)
> > +  copy: function Storage_copy(aSourceURL, aTargetURL) {
> > +    let sourceFile = this.getFileForURL(aSourceURL);
> > +    let targetFile = this.getFileForURL(aTargetURL);
> > +    sourceFile.copyTo(targetFile.parent, targetFile.leafName);
> 
> does copyTo throw? if so, should catch and report errors.

It's supposed to throw when the file exists (per nsIFile docs) but it doesn't - I added it to the test to ensure it's working. I'd rather not throw here for the same reason as noted in comment #27.
Comment 29 Tim Taubert [:ttaubert] 2012-05-01 04:53:20 PDT
Pushed to fx-team with some test fixes for Windows:

https://hg.mozilla.org/integration/fx-team/rev/080fc3a7cdfe
Comment 30 Ed Morley [:emorley] 2012-05-01 08:37:28 PDT
Unfortunately backed out for xpcshell failures:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=080fc3a7cdfe
eg https://tbpl.mozilla.org/php/getParsedLog.php?id=11357682&tree=Fx-Team

{
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/browser/components/places/tests/unit/test_browserGlue_shutdown.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/browser/components/places/tests/unit/test_browserGlue_shutdown.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST 1: Export to bookmarks.html if autoExportHTML is true.
TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/browser/components/places/tests/unit/test_browserGlue_shutdown.js | [null : 65] true == true

TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryService.removeObserver]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/PageThumbs.jsm :: PageThumbs_uninit :: line 81"  data: no]
resource:///modules/webappsUI.jsm:23: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
<<<<<<<
}

https://hg.mozilla.org/integration/fx-team/rev/23caa5d559ae
Comment 31 Tim Taubert [:ttaubert] 2012-05-01 14:03:24 PDT
Pushed to fx-team again:

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

Fixed xpcshell bustage by adding an 'initialized' flag to make uninit() a no-op if PageThumbs hasn't been initialized, yet.
Comment 32 Tim Taubert [:ttaubert] 2012-05-02 06:48:00 PDT
https://hg.mozilla.org/mozilla-central/rev/e1edaf3a8883
Comment 33 SpeciesX 2012-05-02 09:05:15 PDT
Is it really necessary creating a thumbnail of every visited site? I doesn't use the new tab feature, but i have in less than half hour already more than 60 thumbnails.
Comment 34 yamiravko 2012-05-02 10:56:26 PDT
(In reply to SpeciesX from comment #33)
> Is it really necessary creating a thumbnail of every visited site? I doesn't
> use the new tab feature, but i have in less than half hour already more than
> 60 thumbnails.

I agree.  I think there should be an config setting to disable thumbnailing.
Comment 35 Jim Jeffery not reading bug-mail 1/2/11 2012-05-02 11:53:03 PDT
https://bugzilla.mozilla.org/show_bug.cgi?id=726347#c18
add the pref in about:config
browser.pagethumbnails.capturing_disabled as a boolean entry set to 'true'

(In reply to yamiravko from comment #34)
> (In reply to SpeciesX from comment #33)
> > Is it really necessary creating a thumbnail of every visited site? I doesn't
> > use the new tab feature, but i have in less than half hour already more than
> > 60 thumbnails.
> 
> I agree.  I think there should be an config setting to disable thumbnailing.
Comment 36 :Ehsan Akhgari 2012-05-02 12:43:45 PDT
Backed out the latest fx-team merge whole-sale because of Ts regressions:

https://hg.mozilla.org/mozilla-central/rev/b0200dab0ccc

Please reland only after investigating and fixing the regression.  Thanks!
Comment 37 :Ehsan Akhgari 2012-05-02 13:14:43 PDT
I messed up and backed out the wrong range, sorry about that.  I backed out my backout, so this is still on mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/dec5b367c421
Comment 38 Girish Sharma [:Optimizer] 2012-05-03 03:04:46 PDT
I had to forcefully shut down Windows while Nightly was running, and when I restarted, the New Tab Page thumbnails were gone.
Does this bug not fix this situation? 
I mean when the browser looses thumbnails on force shutdown or crash.
Comment 39 Jo Hermans 2012-05-03 03:14:41 PDT
It should, but the patch isn't available yet in the current nightly (2012-05-02). It will be in the 2012-05-03 version, unless someone tries to back it out again.
Comment 40 Tim Taubert [:ttaubert] 2012-05-03 03:15:30 PDT
(In reply to Girish Sharma [:Optimizer] from comment #38)
> I had to forcefully shut down Windows while Nightly was running, and when I
> restarted, the New Tab Page thumbnails were gone.
> Does this bug not fix this situation? 
> I mean when the browser looses thumbnails on force shutdown or crash.

Yes, that's exactly one of the things this bug fixes.
Comment 41 Girish Sharma [:Optimizer] 2012-05-03 03:16:14 PDT
(In reply to Jo Hermans from comment #39)
> It should, but the patch isn't available yet in the current nightly
> (2012-05-02). It will be in the 2012-05-03 version, unless someone tries to
> back it out again.

Oh I see, sorry for the inconvenience.
Comment 42 CruNcher 2012-05-03 09:37:34 PDT
Will Extensions be automaticaly redirected to these tthumbnails place (where is it ?) and how about handling this for users that generaly dont want anything cached except into memory currently it's forced for those due to bug https://bugzilla.mozilla.org/show_bug.cgi?id=724408
couldn't we get a own about:config setting for the New Tab thumbnails ?
Comment 43 CruNcher 2012-05-03 09:46:03 PDT
Sorry missed https://bugzilla.mozilla.org/show_bug.cgi?id=726347 problem solved :)
Comment 44 CruNcher 2012-05-03 09:55:08 PDT
Also whats about users that might would like this but just temporarily also in their normal profiles for the current session only :) (memory cached thumbnails) without using private browsing mode ?
Comment 45 Tim Taubert [:ttaubert] 2012-05-03 09:58:58 PDT
(In reply to CruNcher from comment #44)
> Also whats about users that might would like this but just temporarily also
> in their normal profiles for the current session only :) (memory cached
> thumbnails) without using private browsing mode ?

If you want all your data to exist temporarily, only (in memory) then you should pick the private browsing mode. We don't support thumbnails in pb mode at the moment but we will in the future.

If you want your thumbnails to be cleared on shutdown you could enable clearing your history on shutdown. This will remove thumbnails for the corresponding pages as well.
Comment 46 Tim Taubert [:ttaubert] 2012-05-03 10:00:03 PDT
(In reply to CruNcher from comment #42)
> Will Extensions be automaticaly redirected to these tthumbnails place (where
> is it ?)

Extensions will hopefully use the thumbnail service in the future. It's still not too mature though and we need to catch up with the documentation. Thumbnails are saved in the "thumbnails" directory in your profile directory.
Comment 47 CruNcher 2012-05-03 10:16:36 PDT
What about letting the user decide what model he want's to use ?
think about it like this enable browser.pagethumbnails.capturing_disabled by default though pre cache them for his current session in memory so he still can use that feature and then inform the user when he closes Firefox about this Thumbnail feature and how he want's to use it (multiple choice selection) if he wants to continue using it permanent and how (saving on exit (he has a lot of memory available),saving on the go directly (he want's push it more to IO) and on the next restart if thumbnails are found load them :) ?

Though i guess the UI guys would jump in circles pushing so much cognitive work to the user ;) ?
Comment 48 Mike 2012-05-05 05:34:37 PDT
Is there way to disable this "feature" via about:config settings?
Comment 49 Jo Hermans 2012-05-05 12:49:29 PDT
(In reply to Mike from comment #48)
> Is there way to disable this "feature" via about:config settings?

see comment 35
Comment 50 Danial Horton 2012-05-06 22:25:26 PDT
has anyone created a bug to place this directory in its correct LOCAL DATA folder, instead of the main roaming profile?
Comment 51 Danial Horton 2012-05-06 22:36:32 PDT
i searched, didn't find one, so i did

bug 752407
Comment 52 Virgil Dicu [:virgil] [QA] 2012-05-08 08:26:36 PDT
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/15.0 Firefox/15.0a1

Verified on Ubuntu 12.04, Mac OS 10.6, Windows 7.

Works great. All thumbnails for visited sites were displayed as expected.
Comment 53 Girish Sharma [:Optimizer] 2012-05-15 00:52:10 PDT
Due to this feature, the thumbnails folder in my profile directory is of the size range of 350 MB.

Why does each of the 9 folders in the thumbnails folder contain more 9 folders ? Please someone explain.

IMO, Firefox should delete old thumbnails to reduce the size of the folder.

Maybe on each Firefox shutdown, the thumbnails that are not being shown on the new tab page should be deleted. This way, there would be only 9 thumbnails when the browser is closed.
Comment 54 Tim Taubert [:ttaubert] 2012-05-15 01:05:34 PDT
(In reply to Girish Sharma [:Optimizer] from comment #53)
> Due to this feature, the thumbnails folder in my profile directory is of the
> size range of 350 MB.
> 
> IMO, Firefox should delete old thumbnails to reduce the size of the folder.

Please see bug 754671.
Comment 55 Gian-Carlo Pascutto [:gcp] 2012-05-25 01:32:40 PDT
I understood a fix for bug 754671 won't be available soon, so can we consider backing this out again, and relanding when that is fixed?

Unbounded growth of this store is not good. I'm sure many users already have >1G by now.
Comment 56 Tim Taubert [:ttaubert] 2012-05-25 01:34:27 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #55)
> I understood a fix for bug 754671 won't be available soon, so can we
> consider backing this out again, and relanding when that is fixed?

I'd be fine with doing that after the next merge if we didn't come up with a fix until then. So I'd suggest backing it out for Aurora.
Comment 57 Dão Gottwald [:dao] 2012-06-05 15:10:03 PDT
(In reply to Tim Taubert [:ttaubert] from comment #56)
> (In reply to Gian-Carlo Pascutto (:gcp) from comment #55)
> > I understood a fix for bug 754671 won't be available soon, so can we
> > consider backing this out again, and relanding when that is fixed?
> 
> I'd be fine with doing that after the next merge if we didn't come up with a
> fix until then. So I'd suggest backing it out for Aurora.

Can this be backed out now?
Comment 58 Virgil Dicu [:virgil] [QA] 2012-06-11 02:45:32 PDT
*** Bug 762523 has been marked as a duplicate of this bug. ***
Comment 59 Jo Hermans 2012-06-14 03:49:09 PDT
*** Bug 764681 has been marked as a duplicate of this bug. ***
Comment 60 Tim Taubert [:ttaubert] 2012-06-14 03:51:13 PDT
Created attachment 633094 [details] [diff] [review]
Backout for Aurora (Fx 15)

We currently can't fix bug 754671 easily without introducing additional problems. We're waiting on async file IO to land but this will most definitely not be backported to Aurora so we need to back this out.
Comment 61 Dão Gottwald [:dao] 2012-06-14 03:55:53 PDT
Comment on attachment 633094 [details] [diff] [review]
Backout for Aurora (Fx 15)

I assume this is basically just a straight backout of the original patch and subsequent patches related to the custom storage.
Comment 62 Tim Taubert [:ttaubert] 2012-06-14 03:57:33 PDT
(In reply to Dão Gottwald [:dao] from comment #61)
> I assume this is basically just a straight backout of the original patch and
> subsequent patches related to the custom storage.

Yes.
Comment 63 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-15 15:57:23 PDT
Comment on attachment 633094 [details] [diff] [review]
Backout for Aurora (Fx 15)

[Triage Comment]
Please go ahead and land this backout on Aurora (15) and update status flags accordingly.
Comment 64 Tim Taubert [:ttaubert] 2012-06-15 16:08:59 PDT
Backed out:

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd1105af153b
Comment 65 Ryan VanderMeulen [:RyanVM] 2012-06-15 18:19:08 PDT
Please test your patches on Try before landing on Aurora. I had to disable browser_thumbnails_bug753755.js due to it being perma-orange after your landing. If it's not the right fix, please backout and fix the test.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d20406146672

https://tbpl.mozilla.org/php/getParsedLog.php?id=12715794&tree=Mozilla-Aurora
TEST-START | chrome://mochitests/content/browser/browser/components/thumbnails/test/browser_thumbnails_bug753755.js
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/thumbnails/test/browser_thumbnails_bug753755.js | Exception thrown at chrome://mochitests/content/browser/browser/components/thumbnails/test/browser_thumbnails_bug753755.js:10 - ReferenceError: PageThumbsStorage is not defined
INFO TEST-END | chrome://mochitests/content/browser/browser/components/thumbnails/test/browser_thumbnails_bug753755.js | finished in 7ms
Comment 66 Ryan VanderMeulen [:RyanVM] 2012-06-15 18:22:00 PDT
Also, Aurora is not inbound. Don't land on it if you aren't planning to watch the tree afterwards.
https://wiki.mozilla.org/Tree_Rules#mozilla-aurora
Comment 67 Tim Taubert [:ttaubert] 2012-06-16 01:43:00 PDT
(In reply to Ryan VanderMeulen from comment #65)
> Please test your patches on Try before landing on Aurora. I had to disable
> browser_thumbnails_bug753755.js due to it being perma-orange after your
> landing. If it's not the right fix, please backout and fix the test.

That's the right fix. I actually pushed to try but I think I may have pushed an older version...

(In reply to Ryan VanderMeulen from comment #66)
> Also, Aurora is not inbound. Don't land on it if you aren't planning to
> watch the tree afterwards.

Sorry, I forgot about it :/ I usually watch my pushes as I'm not using m-i.
Comment 68 Alex Keybl [:akeybl] 2012-08-22 16:10:48 PDT
We're going to perform a backout again due to bug 754671, and the medium risk fix that we'd rather not take. Tracking for FF16 given that.
Comment 69 Tim Taubert [:ttaubert] 2012-08-22 16:25:42 PDT
Backed out from Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d01c3e9573c
Comment 70 Tim Taubert [:ttaubert] 2012-08-23 15:03:19 PDT
Backed out the backout:

https://hg.mozilla.org/releases/mozilla-aurora/rev/4df6a52d0e49

Please see bug 754671 comment #73.
Comment 71 Virgil Dicu [:virgil] [QA] 2012-09-11 07:03:53 PDT
Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0

No thumbnail recovery problems with the new thumbnail storage on 16 beta 2. Verified Mac OS 10.7, Ubuntu 12.04, Windows 7. 

Will verify size of thumbnail storage separately.

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