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 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()?

(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 on attachment 614337 [details] [diff] [review] patch v1 There is no better way to do this for another couple of weeks

Comment on attachment 614337 [details] [diff] [review] patch v1 I need to look at this some more

Comment on attachment 614337 [details] [diff] [review] patch v1 I don't see where the copy() function is used

(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 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.

(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 :)

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.

(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.

(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.

We could model our cache eviction behavior. if(rand()*20 ==13) nuke_everything()

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?

(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.

Implemented thumbnail removal on history sanitization.

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.

Keeping the old cache backend as fallback.

I don't see any explicit private browsing awareness; is that planned, or am I missing something?

That was fixed in bug 744743 as a preparation for this one.

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).

(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 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.

(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.

(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.

(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.

(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.

(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.

Pushed to fx-team with some test fixes for Windows: https://hg.mozilla.org/integration/fx-team/rev/080fc3a7cdfe

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

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.

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.

(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.

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.

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!

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

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.

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.

(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.

(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.

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 ?

Sorry missed https://bugzilla.mozilla.org/show_bug.cgi?id=726347 problem solved :)

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 ?

(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.

(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.

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 ;) ?

Is there way to disable this "feature" via about:config settings?

(In reply to Mike from comment #48) > Is there way to disable this "feature" via about:config settings? see comment 35

has anyone created a bug to place this directory in its correct LOCAL DATA folder, instead of the main roaming profile?

i searched, didn't find one, so i did bug 752407

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.

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.

(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.

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.

(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.

(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?

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 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.

(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 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.

Backed out: https://hg.mozilla.org/releases/mozilla-aurora/rev/bd1105af153b

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

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

(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.

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.

Backed out from Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3d01c3e9573c

Backed out the backout: https://hg.mozilla.org/releases/mozilla-aurora/rev/4df6a52d0e49 Please see bug 754671 comment #73.

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.