897811 - pref browser.pagethumbnails.capturing_disabled;true broken by bug 870100 landing

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Following the landing of bug 870100 the pref setting no longer is affective in blocking thumbnails from being displayed.

STR: 
add hidden pref browser.pagethumbnails.capturing_disabled
Set to 'true' 

Note that thumbnails are still refreshed and displayed following the landing of bug 870100

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I'm not really sure why we have both browser.pagethumbnails.capturing_disabled and browser.pageThumbs.enabled. Seems like the former prevents some work that the latter doesn't (adding a bunch of observers in gBrowserThumbnails.init), but they're otherwise mostly the same. Seems like we should remove one or the other?

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

Indeed - 
added the apparently hidden pref: browser.pageThumbs.enabled set to 'false' and no thumbs are displayed 

pref: browser.pagethumbnails.capturing_disabled is set currently to: 'true' 

So seems that one or the other could control everything for sure..

A quick test of setting browser.pagethumbnails.capturing_disabled to 'false' 
does not re-enable display of thumbs, so appears that perhaps the new hidden 
pref: browser.pageThumbs.enabled set to 'false' will control everything ?

Further testing of just toggling browser.pageThumbs.enabled will enable/disable thumbs display in a NewTab. 

No coder here, but seems that the older pref: browser.pagethumbnails.capturing_disabled 
could indeed be removed ?

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 897811-remove-browser.pagethumbnails.capturing_disabled.preference.patch

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> I'm not really sure why we have both
> browser.pagethumbnails.capturing_disabled and browser.pageThumbs.enabled.
> Seems like the former prevents some work that the latter doesn't (adding a
> bunch of observers in gBrowserThumbnails.init)

I think we should remove that one that "prevents some work" for 2 reasons:

* It prevents that work, but also means that Fx needs to be restarted after the preference is toggled.  It would be possible to fix that, but ...

* It's a hidden preference, so a tiny minority of users will ever set it.  It's not worth avoiding this extra work for such users, especially if it means even more code so the pref can be toggled and have it honored without a restart.

This patch removes the browser.pagethumbnails.capturing_disabled preference (it only appears in one place) and leaves the browser.pageThumbs.enabled preference alone.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

hrm - on the downside, a quick google for that pref name shows quite a number of hits.  We can obviously update MDN - maybe doing that and ensuring the old pref name is still there means users are still likely to find it via google?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 784803 [details] [diff] [review]
897811-remove-browser.pagethumbnails.capturing_disabled.preference.patch

How complicated would it be to go the other way and keep browser.pagethumbnails.capturing_disabled instead?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 897811-remove-browser.pageThumbs.enabled-preference.patch

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> Comment on attachment 784803 [details] [diff] [review]
> 897811-remove-browser.pagethumbnails.capturing_disabled.preference.patch
> 
> How complicated would it be to go the other way and keep
> browser.pagethumbnails.capturing_disabled instead?

Not hard - I think we'd just still remove the check in the patch I uploaded (ie, in the init() function) and rename the other few occurrences of the pref to this.

OTOH though, looking at the google results, it is almost as though browser.pagethumbnails.capturing_disabled is the pref to mean "disable the capture of sensitive pages" - which we have since fixed - so in some ways we are doing the users a favour as we will be automatically turning on thumbnails now they are "safe" ;)

But yeah - that is a stretch :)  So here is one that keeps only browser.pagethumbnails.capturing_disabled.  Take your pick :)

Try for this one at https://tbpl.mozilla.org/?tree=Try&rev=9900a351c38c

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: 897811-remove-browser.pageThumbs.enabled-preference.patch

Sorry about the previous rushed and obviously wrong patch.  Try for this one at https://tbpl.mozilla.org/?tree=Try&rev=a308683f023b

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 785464 [details] [diff] [review]
897811-remove-browser.pageThumbs.enabled-preference.patch

I would really like to know why those tests need to disable the service too. Maybe lump that in with bug 896912?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b487df56bcfe

I think we should uplift this to 25 so people who have already set the browser.pagethumbnails.capturing_disabled continue to work as expected in that version.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 785464 [details] [diff] [review]
897811-remove-browser.pageThumbs.enabled-preference.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 870100
User impact if declined: Users who previously set a hidden preference to disable thumbnails will see thumbnails start to appear.
Testing completed (on m-c, etc.): Only on inbound at the moment, but fairly low risk and easy to manage.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Jim Jeffery not reading bug-mail 1/2/11]

So, I'm confused..
which pref was removed ?
browser.pagethumbnails.capturing_disabled 
browser.pageThumbs.enabled

It appeared from a quick look that the top pref was 'removed' in favor in the 2nd pref

Sorry, didn't read the actual patch.

I 'reset' the top pref 'browser.pagethumnails.capturing_disabled and restarted and the pref was gone, as I suspected would happen with the removal of the pref.

However, with browser.pagthumbe.enabled set to 'false' I'm now seeing thumbs again, and now readding the browser.pagethumbnails.capturing_disabled and setting to 'true' I'm still getting thumbs even after repeated deletions of the 
J:\Users\username\AppData\Local\Mozilla\Firefox\Profiles\default.xxxxxxx thumbnails 
folder.

I can't seem to turn them off at all now...

Comment 13

[Jim Jeffery not reading bug-mail 1/2/11]

both pref's retained and 'hidden' ?  

I have:
browser.pageThumbs.enabled;false
browser.pagethumbnails.capturing_disabled;true
browser.pagethumbnails.storage_version;3

and after deleting the thumbnails folder - I still see thumbs...

Comment 14

[:Gavin Sharp [email: [e-mail]]]

browser.pageThumbs.enabled was removed and is now ignored by Firefox.

browser.pagethumbnails.capturing_disabled is the pref you can use to disable thumbnail collection in general.

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

Thanks Gavin

removed (reset - browser.pagethumbs.enabled)
deleted cache folder
deleted thumbnails folder 

Both with Firefox Closed - restarted - and still getting thumbs. :(

Comment 16

[Jim Jeffery not reading bug-mail 1/2/11]

Re-opening, 
pref: browser.pagethumbnails.capturing_disabled no longer blocks thumbnails with this patch landing.

Tested on latest m-c hourly based on cset: 
https://hg.mozilla.org/mozilla-central/rev/79b5c74ef97b

I deleted cache and thumbnails folders several times and thumbs keep on coming back.
Removed via add/remove and reinstalled from a fresh download of the build with above cset, and still seeing thumbs.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Jim Jeffery not reading bug-mail 1/2/11]

I'm now totally confused, went back to a 7/24/13 build prior to the landing of but 870100, and with the pref browser.pagethumbnails.capturing_disabled = True 

I'm still getting thumbs...where before I certain that was not case.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Attachment: bug-897811-followup.patch

Pretty spectacular screwup by me.  The new pref's meaning is negated: "browser.pageThumbs.enabled" -> "browser.pagethumbnails.capturing_disabled" - but the values only negated in one place - they should all have been.

At least the default case (ie, no pref existing) is OK - so only when there was a value for browser.pagethumbnails.capturing_disabled did things go wrong.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug
User impact if declined: Thumbnails may appear when they shouldn't, and may not when they should.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Lower than taking the other one was ;)
String or IDL/UUID changes made by this patch: None

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 787216 [details] [diff] [review]
bug-897811-followup.patch

doh!

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Thanks for catching this one, Jim!

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #21)
> Thanks for catching this one, Jim!

Your Welcome.  Thanks for the quick turn-around.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9642207875

Comment 24

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/58e2271ca9d0

Comment 25

[Boris Zbarsky [:bzbarsky]]

Backed out of inbound for b-c failures.  https://tbpl.mozilla.org/php/getParsedLog.php?id=26290181&tree=Mozilla-Inbound is a typical one: https://hg.mozilla.org/integration/mozilla-inbound/rev/574c96f701d9

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Attachment: ab9642207875

I neglected to fix head.js in the thumbnails dir as I mistakenly thought that was the only thing I didn't screw up last time.  This patch fixes that too.

https://tbpl.mozilla.org/?tree=Try&rev=6ecfcf7f9e58

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Backed out on Aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/3318c44609da

Comment 28

[Mark Hammond [:markh] [:mhammond]]

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

I won't ask for Aurora approval until it sticks this time ;)

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/0f21776e01b9

Comment 30

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 787344 [details] [diff] [review]
ab9642207875

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Where do I start with this.  tl;dr, this bug.
User impact if declined: Thumbnails not disabled when user sets a pref that should disable them.
Testing completed (on m-c, etc.): Finally stuck on m-c.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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