The default bug view has changed. See this FAQ.

[Page Thumbnails] implement a custom storage, don't use the file cache

VERIFIED FIXED in Firefox 16

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 disabled, firefox16+ verified)

Details

(Whiteboard: [Snappy:P1])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 734952
(Assignee)

Updated

5 years ago
Blocks: 730042
(Assignee)

Updated

5 years ago
Blocks: 724408
(Assignee)

Updated

5 years ago
Blocks: 739069
(Assignee)

Updated

5 years ago
Blocks: 740287
(Assignee)

Updated

5 years ago
Blocks: 726267
(Assignee)

Updated

5 years ago
Blocks: 716949
(Assignee)

Updated

5 years ago
Depends on: 744743
(Assignee)

Comment 1

5 years ago
Created attachment 614337 [details] [diff] [review]
patch v1
(Assignee)

Comment 2

5 years ago
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()?
Attachment #614337 - Flags: feedback?(taras.mozilla)
(Assignee)

Updated

5 years ago
Blocks: 736724
(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.

Updated

5 years ago
Whiteboard: [Snappy] → [Snappy:P1]

Comment 4

5 years ago
Comment on attachment 614337 [details] [diff] [review]
patch v1

There is no better way to do this for another couple of weeks
Attachment #614337 - Flags: feedback?(taras.mozilla)

Comment 5

5 years ago
Comment on attachment 614337 [details] [diff] [review]
patch v1

I need to look at this some more
Attachment #614337 - Flags: feedback?(taras.mozilla)

Comment 6

5 years ago
Comment on attachment 614337 [details] [diff] [review]
patch v1

I don't see where the copy() function is used
(Assignee)

Comment 7

5 years ago
(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

5 years ago
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.
Attachment #614337 - Flags: feedback?(taras.mozilla) → feedback+

Comment 9

5 years ago
(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 :)
(Assignee)

Comment 10

5 years ago
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.
Attachment #614337 - Attachment is obsolete: true
Attachment #616102 - Flags: feedback?(dietrich)
(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.
(Assignee)

Comment 12

5 years ago
(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

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

Updated

5 years ago
Blocks: 744780
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?
Attachment #616102 - Flags: feedback?(dietrich) → feedback+
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
Created attachment 617959 [details] [diff] [review]
patch v3

Implemented thumbnail removal on history sanitization.
Attachment #616102 - Attachment is obsolete: true
Attachment #617959 - Flags: review?(dietrich)
(Assignee)

Updated

5 years ago
Blocks: 731157
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.
Attachment #617959 - Flags: review?(dietrich)
(Assignee)

Comment 18

5 years ago
Created attachment 618285 [details] [diff] [review]
patch v4

Keeping the old cache backend as fallback.
Attachment #617959 - Attachment is obsolete: true
Attachment #618285 - Flags: review?(dietrich)
I don't see any explicit private browsing awareness; is that planned, or am I missing something?
(Assignee)

Comment 20

5 years ago
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).
(Assignee)

Comment 22

5 years ago
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.
Attachment #618285 - Attachment is obsolete: true
Attachment #618285 - Flags: review?(dietrich)
Attachment #618318 - Flags: review?(dietrich)
(Assignee)

Updated

5 years ago
Blocks: 658913
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.
Attachment #618318 - Flags: review?(dietrich) → review+
(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.
(Assignee)

Comment 26

5 years ago
(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.
(Assignee)

Comment 27

5 years ago
(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.
(Assignee)

Comment 28

5 years ago
(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.
(Assignee)

Comment 29

5 years ago
Pushed to fx-team with some test fixes for Windows:

https://hg.mozilla.org/integration/fx-team/rev/080fc3a7cdfe
Whiteboard: [Snappy:P1] → [Snappy:P1][fixed-in-fx-team]
Target Milestone: --- → Firefox 15
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
Whiteboard: [Snappy:P1][fixed-in-fx-team] → [Snappy:P1]
Target Milestone: Firefox 15 → ---
(Assignee)

Comment 31

5 years ago
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.
Whiteboard: [Snappy:P1] → [Snappy:P1][fixed-in-fx-team]
Target Milestone: --- → Firefox 15
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e1edaf3a8883
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P1][fixed-in-fx-team] → [Snappy:P1]
Blocks: 743563

Comment 33

5 years ago
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

5 years ago
(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!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
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

5 years ago
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.
(Assignee)

Comment 40

5 years ago
(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.

Comment 42

5 years ago
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

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

Comment 44

5 years ago
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 ?
(Assignee)

Comment 45

5 years ago
(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.
(Assignee)

Comment 46

5 years ago
(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

5 years ago
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

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

Comment 49

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

see comment 35

Comment 50

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

Comment 51

5 years ago
i searched, didn't find one, so i did

bug 752407

Updated

5 years ago
Depends on: 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.
Status: RESOLVED → VERIFIED
Depends on: 752409
Blocks: 753768

Updated

5 years ago
Depends on: 754671
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.
(Assignee)

Comment 54

5 years ago
(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.
(Assignee)

Comment 56

5 years ago
(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?
(Assignee)

Updated

5 years ago
Depends on: 762094
Duplicate of this bug: 762523

Updated

5 years ago
Duplicate of this bug: 764681
(Assignee)

Comment 60

5 years ago
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.
Attachment #633094 - Flags: review?(dao)
Attachment #633094 - Flags: approval-mozilla-aurora?
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.
Attachment #633094 - Flags: review?(dao) → review+
(Assignee)

Comment 62

5 years ago
(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.
Attachment #633094 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 64

5 years ago
Backed out:

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd1105af153b
status-firefox15: --- → disabled
Target Milestone: Firefox 15 → Firefox 16
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
(Assignee)

Comment 67

5 years ago
(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.
tracking-firefox16: --- → +
(Assignee)

Comment 69

5 years ago
Backed out from Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d01c3e9573c
status-firefox16: --- → disabled
(Assignee)

Comment 70

5 years ago
Backed out the backout:

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

Please see bug 754671 comment #73.
status-firefox16: disabled → fixed
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.
status-firefox16: fixed → verified

Updated

5 years ago
Depends on: 802220
No longer depends on: 802220
You need to log in before you can comment on or make changes to this bug.