Ciprian Georgiu, Desktop QA Reporter
	Description • 6 years ago

[Affected versions]:
- latest Nightly 65.0a1
- Beta 64.0b5

[Affected platforms]:
- Windows 10 x64
- macOS 10.13
- Ubuntu 16.04 x64

[Preconditions]:
- enable “DisableFirefoxScreenshots” policy via polices.json or GPO

[Steps to reproduce]:
1. Launch Firefox.
2. Access any webpage and take a screenshot via context menu.

[Expected result]:
- Firefox screenshots is disabled.

[Actual result]:
- Firefox screenshots is enabled.

[Regression range]:
- Last good revision: e18f1acd0782f6333ee892c7ebc9711d9a5e550a
  First bad revision: 70a5ef5872f895009bfdf2958616ca0ecece807f
  Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e18f1acd0782f6333ee892c7ebc9711d9a5e550a&tochange=70a5ef5872f895009bfdf2958616ca0ecece807f

[Additional notes]:
- In order to reproduce this issue more easily I have attached the policies.json file

	Stephen A Pohl [:spohl]
	Comment 1 • 6 years ago

This shouldn't block bug 1445943 since this issue is also present with JSON policies. Bug 1445943 is simply adding support for policies via `defaults write` and configuration profiles but does not change the way policies are being applied by Firefox.

	Hector Zhao [:hectorz]
	Comment 2 • 6 years ago

I think bug 1488971 + bug 1498410 combined caused this bug.

IIUC, in bug 1488971, monitor of pref change was moved outside of extension (and delayed to window restored), but the check on startup was still in bootstrap.js [1], and that check was then removed as part of bug 1498410.

[1]: https://hg.mozilla.org/mozilla-central/rev/3113a83b3455#l3.48

	Mike Kaply [:mkaply]
	Comment 3 • 6 years ago

Jared, can you take a look at this please?

Note that pocket moved to a WebExtension but was still able to honor the pref. Not sure how.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 4 • 6 years ago

The addons team asked that we move pref-based management into the addons code, and out of the webextension. Maybe aswan has ideas.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 5 • 6 years ago

To clarify, I can take a look as well, but aswan may have better ideas

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 6 • 6 years ago

> Note that pocket moved to a WebExtension but was still able to honor the pref. Not sure how.

I don't see pocket in the list of webextensions in browser/extensions[1], so maybe their code was just merged into the tree.

Are there any known issues with webcompat-reporter (assuming it's exposed via an enterprise policy)? It seems to be using almost the same nsBrowserGlue check as screenshots[2].


[1] https://searchfox.org/mozilla-central/source/browser/extensions
[2] https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1457-1469

	Andrew Swan [:aswan]
	Comment 7 • 6 years ago

pocket is not a webextension.
The code that monitors that pref is apparently only checking for runtime changes to the pref, if the value flips before the observer is attached (and I assume policies are applied early during startup?) then nothing will ever notice.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 8 • 6 years ago

MozReview-Commit-ID: FGUaKbXhit2

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 9 • 6 years ago

Do we want to get this patch uplifted to 64?

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 10 • 6 years ago

Comment on attachment 9023033 [details]
Bug 1503893 - Ensure Screenshots disable pref is checked at startup; r?aswan

Hey, could you take a look at this patch? It's marked critical, and I suspect will be targeting uplift into 64

	Mike Kaply [:mkaply]
	Comment 11 • 6 years ago

Yes, please.

	Andrew Swan [:aswan]
	Comment 12 • 6 years ago

I think the needinfo was just to remind me that this was also on Phabricator?
If I misunderstood and there's something else please re-flag me

	Pulsebot
	Comment 13 • 6 years ago

Pushed by jhirsch@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ecf239647a4
Ensure Screenshots disable pref is checked at startup; r=aswan

	:Felipe Gomes (needinfo for replies!)
	Comment 14 • 6 years ago

I wonder why the test at https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser/disable_fxscreenshots/ never failed. Is it the case that if this pref was set on a fresh profile (before the first run of Firefox Screenshots), it would work correctly? (because that's how this test behaves)

	Cristina Coroiu [:ccoroiu]
	Comment 15 • 6 years ago
bugherder

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 16 • 6 years ago

Comment on attachment 9023033 [details]
Bug 1503893 - Ensure Screenshots disable pref is checked at startup; r?aswan

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1488971

User impact if declined: Firefox Screenshots will not be disabled by the enterprise group policy

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: Set the 'DisableFirefoxScreenshots' policy, start / restart Firefox, verify none of the Screenshots UI is visible (Library menu item, page action item, context menu item). Visit screenshots.firefox.com/shots and verify no shots are shown.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is small and only affects Screenshots by checking a pref's state at startup, rather than only listening for the pref to be changed.

String changes made/needed: None

	Ciprian Georgiu, Desktop QA Reporter
	Comment 17 • 6 years ago

This bug is no longer reproducible on latest Nightly 65.0a1 (2018-11-15) across platforms: Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64.

Firefox Screenshots is successfully disabled (from Library, Context Menu and Page Actions) after the policy is applied.

Needinfo myself as a reminder, to verify this on Beta once the patch is approved and uplifted.

	Julien Cristau [:jcristau] (back April 22)
	Comment 18 • 6 years ago

Comment on attachment 9023033 [details]
Bug 1503893 - Ensure Screenshots disable pref is checked at startup; r?aswan

fix regression with screenshots and enterprise policies, approved for 64.0b10

	Noemi Erli[:noemi_erli]
	Comment 19 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4546853c31c1

	Ciprian Georgiu, Desktop QA Reporter
	Comment 20 • 6 years ago

This issue is also verified fixed on 64.0b10 running Windows 10 x64, macOS 10.12 and Ubuntu 16.04 x64.