1429160 - Policy: Disable built-in PDF viewer

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

Disable PDF.js

Pref to look: pdfjs.disabled

Comment 1

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1429160 - Create enterprise policy to disable PDFjs

Review commit: https://reviewboard.mozilla.org/r/225914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225914/

Comment 2

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8956975 [details]
Bug 1429160 - Create enterprise policy to disable PDFjs

https://reviewboard.mozilla.org/r/225914/#review231876

LGTM, but you'll need to submit a PR to https://github.com/mozilla/pdf.js, since I believe all pdfjs changes are supposed to go through there.

Comment 3

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8956975 [details]
Bug 1429160 - Create enterprise policy to disable PDFjs

https://reviewboard.mozilla.org/r/225914/#review231876

To clarify, I submit the PR to the GitHub page and do not check it into mozilla-central. Is that right?

Comment 4

[Alex Thayer [:alexical] (she/her)]

(In reply to Kirk Steuber [:bytesized] from comment #3)
> Comment on attachment 8956975 [details]
> Bug 1429160 - Create enterprise policy to disable PDFjs
> 
> https://reviewboard.mozilla.org/r/225914/#review231876
> 
> To clarify, I submit the PR to the GitHub page and do not check it into
> mozilla-central. Is that right?

I'm not sure of the exact procedure - I know that I messed it up though when I made those changes, which created some extra work for the maintainers.

I think bdahl is still a good contact for it though. Brendan, should he just submit the PR for the PDFjs.jsm changes and leave it to be merged in, or should he land it in tree and also a PR, or...?

Comment 5

[Brendan Dahl [:bdahl]]

This looks like it adds a second way to disable pdfjs. Are we not able to use the existing pref "pdfjs.disabled"?

Comment 6

[Alex Thayer [:alexical] (she/her)]

(In reply to Brendan Dahl [:bdahl] from comment #5)
> This looks like it adds a second way to disable pdfjs. Are we not able to
> use the existing pref "pdfjs.disabled"?

Paraphrasing from IRC w/ Kirk: the time at which the policy engine sets prefs causes it to happen before we set up the listeners on pdfjs.disabled, meaning we don't end up overriding our cache pref introduced in bug 1389443. Looking back at the patch - Kirk, did you try just setting both the pdfjs.disabled pref and the pdfjs.enabledCache.state pref, instead of introducing code into PDFjs.jsm?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Kirk is on PTO, so he can explain in more details, but he had mentioned to me that the pdf.js code caches the disabled state (based on the pref + other conditions) somewhere else. The pref change observer is created later than the policies code runs, so it doesn't pick up this change to invalidate the cache.. So activating/deactivating this policy would only take effect after a second restart, which is confusing

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8956975 [details]
Bug 1429160 - Create enterprise policy to disable PDFjs

https://reviewboard.mozilla.org/r/225914/#review233512

::: browser/components/enterprisepolicies/schemas/policies-schema.json:146
(Diff revision 1)
>        "first_available": "60.0",
>  
>        "type": "boolean"
>      },
>  
> +    "DisablePDFjs": {

Instead of referencing PDF.js in the name of the policy, this policy should be called DisableBuiltinPDFViewer

::: browser/components/enterprisepolicies/schemas/policies-schema.json:147
(Diff revision 1)
>  
>        "type": "boolean"
>      },
>  
> +    "DisablePDFjs": {
> +      "description": "Disables PDFjs, which displays PDFs within Firefox.",

According to the github repo, the proper spelling for the name is PDF.js

Comment 9

[Robin Steuber (they/them) [:bytesized]]

Brendan- Let me explain my reasoning for adding a mechanism for disabling PDF.js:

I originally implemented this by setting |pdfjs.disabled| but, as mentioned above, I discovered that this took an additional restart to take effect. This happens because the policy code runs before PDFjs.jsm is loaded, so the pref observer that it sets up has not been connected yet and the cache ends up being trusted until Firefox restarts. This can be avoided by editing the cache directly by setting |pdfjs.enabledCache.state| to false. However, the reverse problem remains: disabling the policy requires one restart for Firefox to pick up the new policies and a second restart for PDF.js to refresh the cache and realize that it is no longer disabled.

Currently there is no mechanism for running code when a policy is disabled, so overriding the cache again would not be trivial. Even if there was such a mechanism, it doesn't really make sense to override the cache that way since there may be other factors preventing PDF.js from being enabled.

I considered a number of options to work around this, such as changing PDFjs.jsm to expect possible changes to |pdfjs.disabled| before the JSM has been loaded. However, doing such a thing seems like it would be pretty difficult without re-introducing the issue of main thread IO during startup. Ultimately it seemed much simpler to introduce an un-cached disable mechanism. Since |Services.policies.isAllowed| is meant specifically for that use, it seemed appropriate.

Does that sound ok to you? If so, what is the proper mechanism to go about merging the patch to make this change?

Comment 10

[Brendan Dahl [:bdahl]]

Sounds reasonable. You can land your changes here and I'll upstream them to https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/content/PdfJs.jsm (or if you want to upstream yourself that's fine too).

Comment 11

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8956975 [details]
Bug 1429160 - Create enterprise policy to disable PDFjs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225914/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7833a7b4a73f
Create enterprise policy to disable PDFjs r=dthayer,Felipe

Comment 13

[Brendan Dahl [:bdahl]]

As for upstreaming, don't worry about it. We've decided to remove this Firefox specific code from the github repo.

Comment 14

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 7833a7b4a73f (bug 1429160) for browser-chrome failures at enterprisepolicies/tests/browser/browser_policies_sorted_alphabetically.js

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7833a7b4a73fe070ab4dc0f77f949b32d4a025d4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168318170&repo=autoland&lineNumber=3215

Backout: https://hg.mozilla.org/integration/autoland/rev/ebbd21637fac5e6146c82acc8a37d742b03ce8ca

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f09470b87745
Create enterprise policy to disable PDF.js. r=dthayer,felipe

Comment 16

[Tiberius Oros[:tiberius_oros]]

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

Comment 17

[Robin Steuber (they/them) [:bytesized]]

Reopened- I believe that we will want to uplift this to 60.

Comment 18

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8956975 [details]
Bug 1429160 - Create enterprise policy to disable PDFjs

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policy Engine
[User impact if declined]: The Policy Engine planned for ESR60 will lack the functionality to disable PDF.js
[Is this code covered by automated tests?]: Test is included.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Verification to be done by Abe Masresha
[List of other uplifts needed for the feature/fix]: 
[Is the change risky?]: Minimal Risk
[Why is the change risky/not risky?]: Change will have no effect unless the policy engine is active and has active policies.
[String changes made/needed]: None

Comment 19

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Enterprise Policies for ESR

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8956975 [details]
Bug 1429160 - Create enterprise policy to disable PDFjs

enterprise policy for pdfjs, beta60+

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ac6479eca3f

Comment 22

[Abe - QA (:Abe_LV)]

We tested this on Nightly using JSON and ADMX policy formats and it is verified as fixed.
The built-in PDF viewer can be disabled by this policy.
Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This change needs to be backported to the upstream pdf.js repo so it doesn't get clobbered on the next update (like I almost just did).
https://github.com/mozilla/pdf.js

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Brendan had mentioned on comment 13 that that wasn't needed anymore

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Err, what? That sure hasn't happened yet and my first diff was definitely going to be reverting this change.

Comment 26

[Brendan Dahl [:bdahl]]

As discussed on IRC, removing that code from the PDF.js repo was delayed and I forgot to notify Ryan. It's removed now.

Comment 27

[Abe - QA (:Abe_LV)]

We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760