The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 25

Status

()

Firefox
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jim Jeffery not reading bug-mail 1/2/11, Assigned: markh)

Tracking

({regression})

Trunk
Firefox 26
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 fixed, firefox26 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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

Comment 3

4 years ago
Created attachment 784803 [details] [diff] [review]
897811-remove-browser.pagethumbnails.capturing_disabled.preference.patch

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #784803 - Flags: review?(gavin.sharp)
(Assignee)

Updated

4 years ago
Assignee: nobody → mhammond
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 4

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

Comment 6

4 years ago
Created attachment 785292 [details] [diff] [review]
897811-remove-browser.pageThumbs.enabled-preference.patch

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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
Attachment #785292 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

4 years ago
Created attachment 785464 [details] [diff] [review]
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
Attachment #784803 - Attachment is obsolete: true
Attachment #785292 - Attachment is obsolete: true
Attachment #784803 - Flags: review?(gavin.sharp)
Attachment #785292 - Flags: review?(gavin.sharp)
Attachment #785464 - Flags: review?(gavin.sharp)
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?
Attachment #785464 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 9

4 years ago
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.
Status: NEW → ASSIGNED
status-firefox25: --- → affected
(Assignee)

Comment 10

4 years ago
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
Attachment #785464 - Flags: approval-mozilla-aurora?
Attachment #785464 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/b487df56bcfe
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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...
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...
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.
Thanks Gavin

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

Both with Firefox Closed - restarted - and still getting thumbs. :(
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/releases/mozilla-aurora/rev/fd829b4478e3
status-firefox26: --- → affected
Target Milestone: Firefox 26 → ---
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.
(Assignee)

Comment 19

4 years ago
Created attachment 787216 [details] [diff] [review]
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
Attachment #787216 - Flags: review?(gavin.sharp)
Attachment #787216 - Flags: approval-mozilla-aurora?
Comment on attachment 787216 [details] [diff] [review]
bug-897811-followup.patch

doh!
Attachment #787216 - Flags: review?(gavin.sharp)
Attachment #787216 - Flags: review+
Attachment #787216 - Flags: approval-mozilla-aurora?
Attachment #787216 - Flags: approval-mozilla-aurora+
Thanks for catching this one, Jim!
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #21)
> Thanks for catching this one, Jim!

Your Welcome.  Thanks for the quick turn-around.
(Assignee)

Comment 23

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9642207875
(Assignee)

Comment 24

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/58e2271ca9d0
status-firefox25: affected → fixed
status-firefox26: affected → fixed
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
(Assignee)

Updated

4 years ago
status-firefox25: fixed → affected
status-firefox26: fixed → affected
(Assignee)

Comment 26

4 years ago
Created attachment 787344 [details] [diff] [review]
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
Attachment #787216 - Attachment is obsolete: true
Attachment #787344 - Flags: review?(gavin.sharp)
(Assignee)

Comment 27

4 years ago
Backed out on Aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/3318c44609da
Attachment #787344 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 28

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

I won't ask for Aurora approval until it sticks this time ;)
https://hg.mozilla.org/mozilla-central/rev/0f21776e01b9
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
(Assignee)

Comment 30

4 years ago
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
Attachment #787344 - Flags: approval-mozilla-aurora?
Attachment #787344 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
status-firefox26: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/e0ea75b63407
status-firefox25: affected → fixed
You need to log in before you can comment on or make changes to this bug.