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

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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: [e-mail]]]

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

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

Comment on attachment 614337 [details] [diff] [review]
patch v1

I need to look at this some more

Comment 6

[(dormant account)]

Comment on attachment 614337 [details] [diff] [review]
patch v1

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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

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

(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] (inactive)]

Attachment: 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]]

(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] (inactive)]

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

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

Comment 14

[Dietrich Ayala (:dietrich)]

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] (inactive)]

(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] (inactive)]

Attachment: patch v3

Implemented thumbnail removal on history sanitization.

Comment 17

[Dietrich Ayala (:dietrich)]

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] (inactive)]

Attachment: patch v4

Keeping the old cache backend as fallback.

Comment 19

[Josh Matthews [:jdm]]

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

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 21

[Dão Gottwald [:dao]]

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] (inactive)]

Attachment: 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)]

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

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

(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] (inactive)]

(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] (inactive)]

(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] (inactive)]

(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] (inactive)]

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

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

Comment 30

[Ed Morley [:emorley]]

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] (inactive)]

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] (inactive)]

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

Comment 33

[SpeciesX]

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]

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

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

[(no longer active)]

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

[(no longer active)]

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

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]

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] (inactive)]

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

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

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]

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

Comment 44

[CruNcher]

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] (inactive)]

(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] (inactive)]

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

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]

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

Comment 49

[Jo Hermans]

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

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]

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

bug 752407

Comment 52

[Virgil Dicu [:virgil] [QA]]

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

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] (inactive)]

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

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] (inactive)]

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

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

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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]]

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] (inactive)]

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

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] (inactive)]

Backed out:

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

Comment 65

[Ryan VanderMeulen [:RyanVM]]

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

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] (inactive)]

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

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] (inactive)]

Backed out from Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d01c3e9573c

Comment 70

[Tim Taubert [:ttaubert] (inactive)]

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

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.