Since the thumbnails were moved from the cache to the file system, they have become much more visible. At this moment, I have 130MB of images in 1983 files. That's after *only* 10 days. It's not a new issue, but previously they were hiding among tens of thousands of entries in the 1GB cache, and cleared when that cache got cleared (automatically or manually). They could also have been pushed out of that cache by other content, but that's not happening anymore. They could also have been deleted by the regular expiration algorithm, but since the current history can keep entries for more than many months, that's not an option either. Also note that they are questions about privacy (there's no reason to store images if they're never going to be displayed again). I do understand that the thumbnails do not only contain the 9 images that have to present on the new-tab page, but that it also contains the images that are needed in panorama. You can also not write an algorithm that would only remember the top 9 sites, and discard the rest. So what can be our options ? - at startup, discard all except the top-N sites ? In case people are removing tabs from the new-tab page, we need N to be larger than 9. - discard images that are older than X days ? Maybe skip their deletion if they're still in the top-N list. - if a tab is deleted, we should also delete the thumbnail, except if they're in the top-N list - there could be also reasons why some sites are not stored in the cache, like mentioned in bug 754608.

From comment 54 of bug 744388 [https://bugzilla.mozilla.org/show_bug.cgi?id=744388#c53] 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. Images should be discarded on browser shutdown and not start-up (so as to prevent any start-up lag). So on next browser start, only 9 thumbnails would be present in the thumbnails folder under profile directory. In fact, any more thumbnails are not even required as if the user removes any tab from the new tab page grid, its thumbnail will be automatically generated when the user visits that page again.

In an extension, I was storing the images in places in a private field. I also only saved screenshots of pages that are in the top 20 most-visited list (top 20 and not top 9, so that I'm sure to have a screenshot at the moment when the page's rank rises above 10 and it enters the newtab page). This already seriously limits the number of thumbs to 20-25. I would refresh the thumbnail once a day, whenever the user visits the page anyway. To delete thumbs of pages that sink in rank, I was setting expireStorageAfter = Ci.nsIAnnotationService.EXPIRE_WEEKS; . Together with the refresh, that ensured that we only keep a small number of thumbs.

We should only store 9 screenshots for thumbnails for privacy and security reasons.

(In reply to Jo Hermans from comment #0) > So what can be our options ? Here's my suggestion: * Keep thumbnails for new-tab entries, and for some N additional potential new-tab sites (in case the user removes some entries). * Keep thumbnails for open and recently navigated-away/closed tabs. When a tab is removed from the navigation/undo-close-tab cache, also remove the thumbnail (provided it isn't a reserved new-tab thumb). Of course, this could just be simplified to remove the thumb when the tab is closed. * On shutdown, if the user has selected to clear the cache, also clear the entire thumbnail cache. * On startup, after the session has been restored, asynchronously evict all eligible thumbs. Since the cache should be kept fairly clean while running, this should not amount to much disk I/O. Another matter that I think needs to be addressed is the thumbnail size its self. They're being rendered at 640x400. Even with a very large screen (1920x1200), the about:newtab thumbs are only 480x270. The tab-drag thumbs only look to be ~350x250. The panorama tabs can be scaled ridiculously huge, but I'd much rather see those limited to a sane size (and maybe deal with some image up-scaling) rather than have enormous thumbs saved to disk. Although, if the cache is change to only ever contain a limited number of entries (i.e. the set of open tabs and about:newtab entries), then the physical size issue becomes somewhat moot.

I am wondering why in this bug is mentioned 9 folders? In my thumbnails folder is 16 folders each containing 16 folders... "Total space occupied: 1 149 360 956 bytes in 7 322 file(s), in 272 directories" says Total Commander...

Not 9 folder, but 9 images. The about:newtab page has 9 thumbnails in a 3x3 grid.

Here's a working patch. So the idea is the following: every 10 minutes (or whatever we want to set it to) the thumbnail service checks its storage directory for files that can be expired. It does this by sending a notification called "browser:cleanup-thumbnails". The subject is an array that expects to be filled with URLs we want to keep. This way the newtab page can specify the URLs it is currently showing. All Panorama does (if activated/used) is provide the URLs of all tabs that are currently opened. The thumbnail service then iterates through all the files in the directory and compares their names and deletes them if they're not in the list and older than one day. Other notes: 1) The storage version is bumped to "2" and wipes the whole thumbnails storage. This is because I removed the file-cache-like sub-directories. The files are just stored under $profile/thumbnails now. Not sure why I thought this was necessary but we clearly don't want to have lots of thumbnails in here. 2) Links.getLinks() in NewTabUtils provides a synchronous fallback to retrieve links. It is highly theoretical that a user hasn't opened a single blank tab (with about:newtab enabled) so the link cache *should* be populated. If it is not, then this is a fallback to get all the links currently shown in the newtab page so that their thumbnails won't get lost. 3) In a second iteration we can and should update this code to use the new asynchronous file API that does all this cleanup and removal in a worker. Yoric said he's willing to help us with that.

(In reply to Tim Taubert [:ttaubert] from comment #9) > All Panorama does (if activated/used) is > provide the URLs of all tabs that are currently opened. Can we do this regardless of panorama? There's a good chance that add-ons will utilize the service for open tabs. Requiring them all to observe browser:cleanup-thumbnails seems suboptimal.

10 minutes seems to small of a time gap, and as thumbnails are deleted synchronously, deleting them every 10 minutes (or not) might be too heavy. Every hour seems a good option.

(In reply to Dão Gottwald [:dao] from comment #10) > (In reply to Tim Taubert [:ttaubert] from comment #9) > > All Panorama does (if activated/used) is > > provide the URLs of all tabs that are currently opened. > > Can we do this regardless of panorama? There's a good chance that add-ons > will utilize the service for open tabs. Requiring them all to observe > browser:cleanup-thumbnails seems suboptimal. Sure, I'll just move that to browser-thumbnails.js. (In reply to Girish Sharma [:Optimizer] from comment #11) > 10 minutes seems to small of a time gap, and as thumbnails are deleted > synchronously, deleting them every 10 minutes (or not) might be too heavy. > Every hour seems a good option. I agree but we'd need some smaller initial delay, because not everyone might keep their browser open for an hour. Also deleting every 10 mins should take less time than deleting bigger chunks every hour. We can always change that afterwards if it proves to be annoying.

(In reply to Tim Taubert [:ttaubert] from comment #12) > we'd need some smaller initial delay, because not everyone might > keep their browser open for an hour. There are special timers that take care of this, like the one used for the periodic software update check.

(In reply to Girish Sharma [:Optimizer] from comment #11) > 10 minutes seems to small of a time gap, and as thumbnails are deleted > synchronously, deleting them every 10 minutes (or not) might be too heavy. > Every hour seems a good option. Note that removal should be asynchronous pretty soon, see bug 753768.

(In reply to Dão Gottwald [:dao] from comment #13) > (In reply to Tim Taubert [:ttaubert] from comment #12) > > we'd need some smaller initial delay, because not everyone might > > keep their browser open for an hour. > > There are special timers that take care of this, like the one used for the > periodic software update check. There is nsIUpdateTimerManager which does exactly this (implemented with plain nsITimers) but seems very specific to mozapps/update. Should we use that? http://dxr.lanedo.com/mozilla-central/toolkit/mozapps/update/nsUpdateTimerManager.js.html

ok, ok guys... but why store more thumbnails than what's in use (9 or more according to future setting)? and next this scheduler will be in some way heavy on hdds, because of making a new thread etc. Shouldn't we store only used and visible thumbnails like Speed/Fast Dial add-ons? This will be the lightest way on memory usage. What do you think about it?

We now have an interval of one hour. The initial delay is 10 minutes. I didn't use nsIUpdateTimerManager because that seems too tied to the update service and re-initializing the timer is no big deal.

(In reply to Virtual_ManPL from comment #16) > but why store more thumbnails than what's in use (9 or more according to > future setting)? > and next this scheduler will be in some way heavy on hdds, because of making > a new thread etc. The thumbnail service is used by Panorama as well and we'd like to have more features use it instead of every component implementing its own thumbnailing mechanisms. So we can't just throw away every thumbnail that's not "visible" anymore.

AFAIK the only thing update-specific about nsIUpdateTimerManager is where it lives in the source tree (and its name, I guess :/). Assuming its functionality is suitable for this, I don't see any reasons not to use it.

What mechanism do we use for add-on or blocklist update checks? Maybe some places maintenance jobs use such a timer as well...

How many files are likely to stay in this directory with expiration involved? I'm a bit concerned about having to hit the filesystem and walk through everything. That's not exactly cheap, especially with lots of files.

(In reply to Justin Dolske [:Dolske] from comment #21) > How many files are likely to stay in this directory with expiration involved? I have no idea, tbh. At least 34 + the number of open tabs and then all the pages we took screenshots for over the day. > I'm a bit concerned about having to hit the filesystem and walk through > everything. That's not exactly cheap, especially with lots of files. Would it help to split this work into chunks, using setTimeout()? Until we have a worker thread that's able to help us with this?

(In reply to Dão Gottwald [:dao] from comment #20) > What mechanism do we use for add-on or blocklist update checks? Maybe some > places maintenance jobs use such a timer as well... We do use nsIUpdateTimerManager, finding those uses was really hard as they are registered with the category in a manifest: toolkit/components/search/toolkitsearch.manifest toolkit/mozapps/extensions/extensions.manifest I'll update the patch to use it as well.

Using nsIUpdateTimerManager.

Comment on attachment 631191 [details] [diff] [review] patch v3 >+ observe: function Thumbnails_observe(aSubject, aTopic, aData) { >+ if (aTopic == "browser:cleanup-thumbnails") { >+ aSubject.QueryInterface(Ci.nsIMutableArray); >+ >+ for (let tab of gBrowser.tabs) { for (let browser of gBrowser.browsers) { >+ let url = Cc["@mozilla.org/supports-string;1"] >+ .createInstance(Ci.nsISupportsString); >+ url.data = tab.linkedBrowser.currentURI.spec; url.data = browser.currentURI.spec; > migrateToNextVersion: function Migrator_migrateToNextVersion() { > let version = this.currentVersion; > > if (version == 0) > this.removeThumbnailsFromRoamingProfile(); >+ else if (version == 1) >+ this.clearThumbnailsFolder(); > }, I'd prefer if 'migrateToNextVersion' bumped the version or if 'migrate' did it in the loop. >+ getLinksSynchronously: function PlacesProvider_getLinksSynchronously() { This wouldn't be needed if PageThumbs.jsm wasn't using the observer service but had a list of functions that would call back asynchronously, would it?

Ah, I didn't realize that this was using nsIFile APIs on the main thread for the iteration/clearing. I don't think that's going to be suitable for landing, even as a stop gap solution. Unfortunately I'm not sure what better options we have now that this is on Aurora...

That's an easy one, back out bug 744388 ASAP as was asked (and promised...) in that bug several times already? :-P

This patch is now using the async file API that Yoric just landed (bug 747876). It lazily spawns its own worker (for now as we'd be the first client, later there should be a global worker) and removes files asynchronously. Also, this patch uses the async directory traversal API from bug 764436. The only thing we don't have yet are async stat calls which means that this patch does not yet always keep files younger than one day. The list of URLs to keep thumbnails for is now collected by calling filters that will call back asynchronously which removes the need for a 'getLinksSynchronously' method and we can omit some nsI* casting and XPCOM calls. Stills needs a test.

Comment on attachment 634026 [details] [diff] [review] patch v4 >+ filter: function Thumbnails_filter(aCallback) { >+ aCallback(Array.map(gBrowser.browsers, function (aBrowser) { >+ return aBrowser.currentURI.spec; >+ })); >+ }, [browser.currentURI.spec for (browser of gBrowser.browsers)] >+ filter: function ExpirationFilter_filter(aCallback) { >+ if (!AllPages.enabled) { >+ aCallback([]); >+ return; >+ } You could probably return false here and let PageThumbs handle this instead of calling back with an empty array.

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

(In reply to Tim Taubert [:ttaubert] from comment #22) > > I'm a bit concerned about having to hit the filesystem and walk through > > everything. That's not exactly cheap, especially with lots of files. > > Would it help to split this work into chunks, using setTimeout()? Until we > have a worker thread that's able to help us with this? Even with a worker thread you may still want to process it in chunks, so that there's less impact on other disk IO that might want to happen during that time.

Addressed the feedback from comment #29 and added chunked processing. I talked to Gavin and we're planning to merge this back to Aurora. We'll not wait for bug 766194 any further and will implement this part as a follow-up. While with this patch we might possibly expire thumbnails that we *could* need (that are younger than a day) we will have a solid storage for all other thumbnails that we're sure we need.

(In reply to Ben Bucksch (:BenB) from comment #34) > https://wiki.mozilla.org/Privacy/Reviews/New_Tab#Principle:_Limited_Data Right, it's also a privacy goal. Thanks for adding that. (In reply to Tim Taubert [:ttaubert] from comment #33) > I talked to Gavin and we're planning to merge this back to Aurora. We'll not > wait for bug 766194 any further and will implement this part as a follow-up. It's not only that we'll not wait for bug 766194, it will most probably not be backported so it's not available in Aurora.

Comment on attachment 643842 [details] [diff] [review] patch v5 >+ filter: function Thumbnails_filter(aCallback) { rename this to filterForThumbnailExpiration or something similarly self-explanatory? >+let PageThumbsWorker = { >+ postMessage: function Worker_postMessage(message, callback) { >+ this._callbacks.push(callback); >+ this._worker.postMessage(message); >+ }, >+ handleEvent: function Worker_handleEvent(aEvent) { >+ let callback = this._callbacks.shift(); >+ if (callback) >+ callback(aEvent.data); > } I don't see how this ensures that the correct callback is picked. >+ filter: function ExpirationFilter_filter(aCallback) { >+ if (!AllPages.enabled) >+ return false; >+ >+ Links.populateCache(function () { >+ let urls = []; >+ >+ // Add all URLs to the list that we want to keep thumbnails for. >+ for (let link of Links.getLinks().slice(0, 25)) { >+ if (link && link.url) >+ urls.push(link.url); >+ } >+ >+ aCallback(urls); >+ }); >+ } "return true;" at the end of the function

(In reply to Dão Gottwald [:dao] from comment #36) > >+ filter: function Thumbnails_filter(aCallback) { > > rename this to filterForThumbnailExpiration or something similarly > self-explanatory? Done. > I don't see how this ensures that the correct callback is picked. The OS.File API itself works synchronously because we have no cross-platform support for async file IO. The worker thread receives a message and blocks until the operation has finished. That's why we can use a FIFO pipe to store the callbacks. > "return true;" at the end of the function Not sure why I made it return a boolean value to indicate a filter is disabled. The code's easier if this just calls the callback with an empty array - so that's what the patch does now.

Comment on attachment 644886 [details] [diff] [review] patch v6 > wipe: function Storage_wipe() { > try { >- FileUtils.getDir("ProfLD", [THUMBNAIL_DIRECTORY]).remove(true); >+ this.getDirectory().remove(true); > } catch (e) { > /* The file might not exist or we're not permitted to remove it. */ > } > }, This looks like it will now create the directory before removing it. >+ clearThumbnailsFolder: function Migrator_clearThumbnailsFolder() { >+ try { >+ FileUtils.getDir("ProfLD", [THUMBNAIL_DIRECTORY]).remove(true); >+ } catch (e) { >+ // The directory might not exist or we're not permitted to remove it. >+ } >+ } Does getDir itself throw when the directory doesn't exist?

Comment on attachment 644886 [details] [diff] [review] patch v6 >+ function filterCallback(aURLs) { >+ urls = urls.concat(aURLs); >+ if (--filtersToWaitFor == 0) >+ expire(); >+ } >+ >+ for (let filter of this._filters) { >+ if (typeof filter == "function") >+ filter(filterCallback) >+ else if ("filterForThumbnailExpiration" in filter) >+ filter.filterForThumbnailExpiration(filterCallback); >+ } You should either a) just not test for filterForThumbnailExpiration's existence or b) throw an exception if it doesn't exist or c) ensure that filtersToWaitFor is decremented and expire is called.

(In reply to Dão Gottwald [:dao] from comment #38) > > wipe: function Storage_wipe() { > > try { > >- FileUtils.getDir("ProfLD", [THUMBNAIL_DIRECTORY]).remove(true); > >+ this.getDirectory().remove(true); > > } catch (e) { > > /* The file might not exist or we're not permitted to remove it. */ > > } > > }, > > This looks like it will now create the directory before removing it. Right, that's not what we want if we're about to wipe the directory. I'll add an argument to getDirectory() to disable directory creation. > Does getDir itself throw when the directory doesn't exist? No, it returns an nsIFile instance where .exists() returns false. remove() throws in case of permission errors or if the directory doesn't exist. (In reply to Dão Gottwald [:dao] from comment #39) > You should either a) just not test for filterForThumbnailExpiration's > existence or b) throw an exception if it doesn't exist or c) ensure that > filtersToWaitFor is decremented and expire is called. Good catch, thanks. I think we should just go with (a) and this will report an error if the object does not have the expected method.

All issues addressed.

I'm permanently hitting bug 737244 with debug builds on try :( https://tbpl.mozilla.org/?tree=Try&rev=a366582689c4 Seems to be a crash/bug in the memory reporter for workers.

> > Does getDir itself throw when the directory doesn't exist? > > No, it returns an nsIFile instance where .exists() returns false. remove() > throws in case of permission errors or if the directory doesn't exist. Then I'd prefer if you moved the this.getDirectory calls out of the try/catch blocks, as we don't expect exceptions from them.

All feedback addressed. Found the reason for crashing debug builds - will be fixed by bug 779445. I *think* Dão is on vacation. Gavin, could you please take a look at this? We can of course ask someone else as well.

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=d0ab054846d1

Comment on attachment 647898 [details] [diff] [review] patch v8 >+ clearThumbnailsFolder: function Migrator_clearThumbnailsFolder() { >+ let dir = FileUtils.getDir("ProfLD", [THUMBNAIL_DIRECTORY]); >+ try { >+ dir.remove(true); This seems like it will cause the problem as bug 767411. I guess if we ship it in 16, it won't actually affect anyone, for the same reasons I listed in bug 767411 comment 1? Even still, it would be nice to avoid this huge hit for Nightly users. Can we easily switch this to an async task using OS.File? PageThumbsStorage.wipe has the same problem, and looks like it's called when history is cleared - I think we need to fix that too, in a tracking16+ bug. bumping the review request to felipe for the rest of the review

Comment on attachment 647898 [details] [diff] [review] patch v8 Review of attachment 647898 [details] [diff] [review]: ----------------------------------------------------------------- My understanding is with the chunked removal, this will only remove 50 thumbnails per hour. Is this the intended behavior? I don't know what's the expected number of thumbnails being saved to know if the removal can catch up with the thumbnail growth in this case (as this opens the chance to grow infinitely). ::: browser/components/thumbnails/PageThumbs.jsm @@ +316,2 @@ > try { > + dir.remove(true); every nsIFile.remove() needs to have .followLinks = false set, otherwise it will follow symlinks/.lnk files that could have been dropped in that folder and remove their contents (instead of just removing the symlinks) @@ +431,5 @@ > + filter.filterForThumbnailExpiration(filterCallback); > + } > + }, > + > + expireThumbnails: function Expiration_expireThumbnails(aURLsToKeep) { this function could live in the worker to avoid the roundtrip of getFilesInDirectory + 1 postMessage for each removeFile. But I believe you put it in this file to keep the expiration logic all here? If the postMessages are not expensive then that's fine ::: browser/components/thumbnails/PageThumbsWorker.js @@ +26,5 @@ > + self.postMessage({result: false, detail: "message not understood"}); > + return; > + } > + > + self.postMessage({result: result}); I think it would be clearer to write this function as a switch statement where the default case is the "message not understood" error, so that you only use 1 postMessage (although the result obj will be a bit more cumbersome) @@ +34,5 @@ > + let iter = new OS.File.DirectoryIterator(msg.path); > + let entries = []; > + > + for (let entry in iter) { > + if (!entry.isDir && !entry.isSpecialDir && !entry.isLink) where are these getters defined? I couldn't find them. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/osfile_win_front.jsm#563 defines isSymLink for example, and no isSpecialDir that I can see ::: browser/modules/NewTabUtils.jsm @@ +621,5 @@ > + if (!this._initialized) { > + this._initialized = true; > + ExpirationFilter.init(); > + Telemetry.init(); > + } is it not necessary to uninit the expiration filter in this file? I'm guessing no because they have the same lifetime (i.e. until shutdown)

(In reply to :Felipe Gomes from comment #47) > Comment on attachment 647898 [details] [diff] [review] > patch v8 > > Review of attachment 647898 [details] [diff] [review]: > ----------------------------------------------------------------- > > My understanding is with the chunked removal, this will only remove 50 > thumbnails per hour. Is this the intended behavior? I don't know what's the > expected number of thumbnails being saved to know if the removal can catch > up with the thumbnail growth in this case (as this opens the chance to grow > infinitely). I have not checked the code, but I have the intuition that we should rather maintain some data structure of least-recently modified thumbnails (map + circular linked list?) and cut it (by half?) whenever it passes some threshold.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #46) > >+ clearThumbnailsFolder: function Migrator_clearThumbnailsFolder() { > >+ let dir = FileUtils.getDir("ProfLD", [THUMBNAIL_DIRECTORY]); > >+ try { > >+ dir.remove(true); > > This seems like it will cause the problem as bug 767411. I guess if we ship > it in 16, it won't actually affect anyone, for the same reasons I listed in > bug 767411 comment 1? Even still, it would be nice to avoid this huge hit > for Nightly users. Can we easily switch this to an async task using OS.File? Right, that will only affect Nightly/Aurora users. The problem is that there's no way to remove a directory asynchronously, yet - bug 761138. OTOH migrating the storage directory might cause interesting effects if we're still in the transition and the thumbnail service captures thumbnails. > PageThumbsStorage.wipe has the same problem, and looks like it's called when > history is cleared - I think we need to fix that too, in a tracking16+ bug. I also thought about doing this but until now, the process of clearing your history is synchronous. You click the button and after that the history got cleared. We could fix that as a follow-up if we want to change that. (In reply to :Felipe Gomes from comment #47) > My understanding is with the chunked removal, this will only remove 50 > thumbnails per hour. Is this the intended behavior? I don't know what's the > expected number of thumbnails being saved to know if the removal can catch > up with the thumbnail growth in this case (as this opens the chance to grow > infinitely). Yes, good point. I took Yoric's proposal and now cut the list of files to remove in half if that half is bigger than EXPIRATION_MIN_CHUNK_SIZE (=50 by default). > ::: browser/components/thumbnails/PageThumbs.jsm > @@ +316,2 @@ > > try { > > + dir.remove(true); > > every nsIFile.remove() needs to have .followLinks = false set, otherwise it > will follow symlinks/.lnk files that could have been dropped in that folder > and remove their contents (instead of just removing the symlinks) Fixed. > @@ +431,5 @@ > > + filter.filterForThumbnailExpiration(filterCallback); > > + } > > + }, > > + > > + expireThumbnails: function Expiration_expireThumbnails(aURLsToKeep) { > > this function could live in the worker to avoid the roundtrip of > getFilesInDirectory + 1 postMessage for each removeFile. But I believe you > put it in this file to keep the expiration logic all here? If the > postMessages are not expensive then that's fine That's right it could live in the worker but I kept it here to keep the logic together. I modified the code anyway to take a removeFiles() method. I don't know the overhead of postMessage() but we call it now only twice for every expiration round. > ::: browser/components/thumbnails/PageThumbsWorker.js > @@ +26,5 @@ > > + self.postMessage({result: false, detail: "message not understood"}); > > + return; > > + } > > + > > + self.postMessage({result: result}); > > I think it would be clearer to write this function as a switch statement > where the default case is the "message not understood" error, so that you > only use 1 postMessage (although the result obj will be a bit more > cumbersome) Done. > @@ +34,5 @@ > > + let iter = new OS.File.DirectoryIterator(msg.path); > > + let entries = []; > > + > > + for (let entry in iter) { > > + if (!entry.isDir && !entry.isSpecialDir && !entry.isLink) > > where are these getters defined? I couldn't find them. > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/ > osfile_win_front.jsm#563 defines isSymLink for example, and no isSpecialDir > that I can see Yeah... The API changed in the meantime. Fixed. > ::: browser/modules/NewTabUtils.jsm > @@ +621,5 @@ > > + if (!this._initialized) { > > + this._initialized = true; > > + ExpirationFilter.init(); > > + Telemetry.init(); > > + } > > is it not necessary to uninit the expiration filter in this file? I'm > guessing no because they have the same lifetime (i.e. until shutdown) It's not necessary because both things are live until the browsers shuts down.

Comment on attachment 652986 [details] [diff] [review] patch v9 Review of attachment 652986 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/thumbnails/PageThumbsWorker.js @@ +48,5 @@ > + }, > + > + removeFiles: function Worker_removeFiles(msg) { > + for (let file of msg.paths) { > + OS.File.remove(file); Is there any chance of this throwing? if so we should wrap a try catch to make sure we don't end up skipping postMessage with the return value, which would make the callback queue on the other thread to get out of sync

(In reply to :Felipe Gomes from comment #50) > > + removeFiles: function Worker_removeFiles(msg) { > > + for (let file of msg.paths) { > > + OS.File.remove(file); > > Is there any chance of this throwing? if so we should wrap a try catch to > make sure we don't end up skipping postMessage with the return value, which > would make the callback queue on the other thread to get out of sync Good point, this throws indeed. Will wrap it in a try/catch and continue with the next file on error.

Same as v9 with some slight modifications as the Aurora codebase is a little different.

Comment on attachment 653906 [details] [diff] [review] backport for Aurora Will update the patch.

I talked to felipe and before this patch gets into the next Nightly we decided to rename the old thumbnails directory instead of removing it synchronously (that would cause bug 767411). We'll handle (or maybe not) removing the directory asynchronously in a follow-up bug. This will leave Aurora/Nightly users with the big old thumbnail directory for now but that's better than Firefox freezing at startup while removing gigs of images.

Updated the backport to rename the thumbnail folder as well.

Comment on attachment 654012 [details] [diff] [review] follow-up to rename old thumbnail folder instead of removing it Review of attachment 654012 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/thumbnails/PageThumbs.jsm @@ +394,2 @@ > try { > + dir.moveTo(null, dir.leafName + "-1"); "-old" maybe? so it's easier for people to confidently manually remove the old folder if they want

(In reply to :Felipe Gomes from comment #57) > > try { > > + dir.moveTo(null, dir.leafName + "-1"); > > "-old" maybe? so it's easier for people to confidently manually remove the > old folder if they want Yeah, that's better, will do.

Comment on attachment 654016 [details] [diff] [review] backport for Aurora Review of attachment 654016 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/thumbnails/PageThumbs.jsm @@ +14,5 @@ > const PREF_STORAGE_VERSION = "browser.pagethumbnails.storage_version"; > +const LATEST_STORAGE_VERSION = 2; > + > +const EXPIRATION_MIN_CHUNK_SIZE = 50; > +const EXPIRATION_INTERVAL_SECS = 10;//3600; leftover change?

(In reply to :Felipe Gomes from comment #60) > > +const EXPIRATION_MIN_CHUNK_SIZE = 50; > > +const EXPIRATION_INTERVAL_SECS = 10;//3600; > > leftover change? Oops :/ Will fix and change to "-old" as well.

Comment on attachment 654016 [details] [diff] [review] backport for Aurora [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 744388 User impact if declined: thumbnail storage keeps growing Testing completed (on m-c, etc.): included in tomorrow's Nightly (probably) Risk to taking this patch (and alternatives if risky): risk is middle I'd say but it would fix a long-standing issue with thumbnail disappearing from the new tab page because the alternative would be to back out bug 744388 from Aurora again. String or UUID changes made by this patch: none

(In reply to Tim Taubert [:ttaubert] from comment #55) > We'll handle (or maybe not) removing the directory asynchronously in a > follow-up bug. This will leave Aurora/Nightly users with the big old > thumbnail directory for now but that's better than Firefox freezing at > startup while removing gigs of images. Please do handle it. Leaving gigabytes of garbage behind doesn't seem reasonable at all, as 1) that space can be very valuable depending on how much free space is left, 2) it makes backing up profiles more cumbersome (which nightly users will do more likely than others) and 3) it's a privacy leak.

As far as I understand, the issue of async removal is clearly not specific to this bug or even to thumbnails (e.g. cache, cookies). Perhaps we should have a general-purpose API to handle "safe" asynchronous directory removal. Also, this could be useful on Android, as the temporary directory is something of a hack.

(In reply to Tim Taubert [:ttaubert] from comment #62) > Risk to taking this patch (and alternatives if risky): risk is middle I'd > say but it would fix a long-standing issue with thumbnail disappearing from > the new tab page because the alternative would be to back out bug 744388 > from Aurora again. I don't think the feedback about missing thumbnails has been negative enough to warrant taking a medium-risk forward fix. Let's back out bug 744388 instead.

Sorry, the feedback about large thumbnail directories* Let's let things stand as they were in 15 for one more cycle.

Comment on attachment 654016 [details] [diff] [review] backport for Aurora I'm tripping over myself and need to review further. I'm going to discuss with ttaubert and leave this nominated.

Comment on attachment 654016 [details] [diff] [review] backport for Aurora Spoke with Tim, my comment 65 contains the correct justification. We'll back out instead given the medium risk of the new async IO lib and our proximity to release.

(In reply to Dão Gottwald [:dao] from comment #63) > (In reply to Tim Taubert [:ttaubert] from comment #55) > > We'll handle (or maybe not) removing the directory asynchronously in a > > follow-up bug. This will leave Aurora/Nightly users with the big old > > thumbnail directory for now but that's better than Firefox freezing at > > startup while removing gigs of images. > > Please do handle it. Leaving gigabytes of garbage behind doesn't seem > reasonable at all, as 1) that space can be very valuable depending on how > much free space is left, 2) it makes backing up profiles more cumbersome > (which nightly users will do more likely than others) and 3) it's a privacy > leak. Yes, those are all very valid points. I'll handle removing the leftover thumbnails in bug 784868.

https://hg.mozilla.org/mozilla-central/rev/327883b4f2fe https://hg.mozilla.org/mozilla-central/rev/8e30c456cb6e

Comment on attachment 652986 [details] [diff] [review] patch v9 (In reply to Alex Keybl [:akeybl] from comment #65) > I don't think the feedback about missing thumbnails has been negative enough > to warrant taking a medium-risk forward fix. Let's back out bug 744388 > instead. I don't think we can delay bug 744388 any further, the "thumbnails disappearing" issue has been a major problem for New Tab users (see e.g. bug 756881 comment 22, numerous posts on SUMO, etc.). I don't think the risk assessment here is accurate - it's true that we're using a new and "unproven" API, but there's no reason to expect it will cause problems, and we can always re-evaluate this decision later on in the beta cycle if we discover serious issues (doing a backout then is straighforward).

Funny. It never stopped Mozilla before from checking in bug 744388, despite that there was no expiration method, no size-limit, obvious security issues and it was using a roaming profile. And now when it's possible to fix this, there's a risk with an unproven API ?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #71) > I don't think the risk assessment here is accurate - it's true that we're > using a new and "unproven" API, but there's no reason to expect it will > cause problems, and we can always re-evaluate this decision later on in the > beta cycle if we discover serious issues (doing a backout then is > straighforward). Given Gavin's concerns, we discussed this as part of a larger group at today's Channel Meeting. The medium risk evaluation was originally made with an expectation that we would not necessarily find new regressions and the prevalence of missing thumbnails was not great. Gavin believes that the medium risk evaluation is correct initially, but that we'll be able to find regressions over the 6 week cycle and backout if necessary. The fact that it's taken us a couple of cycles is not a matter of severity, but rather a fact of the difficulty in technical implementation. We checked with Support and they have heard enough data to warrant taking the forward fix. Given that, let's land this and re-land bug 744388 on Aurora 16.