Closed Bug 1443829 Opened 3 years ago Closed 2 years ago

Implement stub Enterprise Policies Manager component for non-browser apps

Categories

(Firefox :: Enterprise Policies, enhancement, P3)

60 Branch
enhancement

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox60 --- affected

People

(Reporter: Felipe, Unassigned)

References

Details

Attachments

(1 file)

If we have a simple implementation of @mozilla.org/browser/enterprisepolicies;1 to be used for non-browser apps, we can avoid having to check for "if (Services.policies)" in any non-browser code, as well as avoiding breakage and test failures when we forget about this, such as bug 1442866, bug 1443769
Jorg, Richard, would any of you be willing to implement this?
Flags: needinfo?(richard.marti)
Flags: needinfo?(jorgk)
My programming knowledge is too low to do this.

Maybe a crazy idea from someone that knows too less: what about moving enterprisepolicies to toolkit? Then TB could also use this for his enterprise users. Again, I don't know what this would mean implementation wise for us.
Flags: needinfo?(richard.marti)
Basically this will need a new manifest to exist in toolkit, but only added if the app being built is not browser:
https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/moz.build#21

And a copy implementation of the EnterprisePolicies.js file, that simply contains the boilerplat, and implements the public functions:
get status()  -> always return INACTIVE
isAllowed() -> always return true
observe() -> doesn't do anything


Eventually we can move this to toolkit, but it would be a lot more work (most policies are related to the browser UI), so this is a simpler solution to avoid the breakage that is cropping up
Sounds like a good idea. However, the breakage we've seen in bug 1442866 and bug 1443769 was rather light (so far). Is there more planned?

Since I know close to nothing about Enterprise Policies, I'm afraid you'd spend more time guiding me that doing the patch yourself. If I read comment #3 correctly, it needs some conditional build in that moz.build you quoted, plus an almost empty EnterprisePoliciesNonBrowser.js which supplies the three functions you mentioned. Looking the existing EnterprisePolicies.js file (which is rather large), I'm not sure how to create this dummy.
Flags: needinfo?(jorgk)
Ok cool, I'll work on this and ask you both for testing. I hope to land this before the merge (or get it uplifted to beta), so that we don't have to worry about having to add the `if (Services.policies)` to any patch that we might want to uplift
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Thanks a lot, of course Richard and I can test this.
Jorg or Richard, can you test to see if this compiles properly with Thunderbird, and also run tests? I'm not sure if I got all the boilerplate right.

In particular, Firefox has a file called browser/installer/package-manifest.in, where the component needs to be listed, but it appears that there's no equivalent for that in toolkit, so I'm not sure if it's unnecessary or if this will break something.
Thank you. I'll get to it later today.
Flags: needinfo?(jorgk)
Comment on attachment 8958353 [details]
Bug 1443829 - Implement stub Enterprise Policies Manager component for non-browser apps.

https://reviewboard.mozilla.org/r/227286/#review233126

Good plan generally. Below is the only bit I'm a little confused by...

I haven't checked if the removal of `Services.policies` nullchecks is exhaustive.

::: toolkit/modules/tests/browser/browser_Troubleshoot.js:164
(Diff revision 2)
>          styloChromeResult: {
>            type: "boolean",
>          },
>          policiesStatus: {
>            type: "number",
> +          required: true,

What does this change do?
Attachment #8958353 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #11)
> ::: toolkit/modules/tests/browser/browser_Troubleshoot.js:164
> (Diff revision 2)
> >          styloChromeResult: {
> >            type: "boolean",
> >          },
> >          policiesStatus: {
> >            type: "number",
> > +          required: true,
> 
> What does this change do?

Nothing right now, except ensuring that the test will fail in the future if `data.policiesStatus` is not present in the data that Troubleshoot.jsm sends to about:support
Blocks: 1440573
I tried the patch and about:support loads without error with our forked aboutSupport.js. This change in toolkit's aboutSupport.js:

-    if (Services.policies) {
+    if (AppConstants.MOZ_BUILD_APP == "browser") {

is not yet in our file.
Hmm I sent this to tryserver, and unfortunately the removal of `if (Services.policies)` still break things under xpcshell tests, and I don't know how to solve that yet.

What happens is that I'm building the stub implementation on the condition of MOZ_BUILD_APP != "browser", so that in Firefox we get the implementation from browser/components/enterprisepolicies, and elsewhere we get the stub from toolkit/components/enterprisepolicies.

The problem is that, on automation, it's built as "browser" (as expected), but it looks like that during xpcshell tests, the components from the browser/ directory are not registered.


Needinfo'ing Ahal because I imagine he will be able to advise on how to properly do this
Flags: needinfo?(ahalberstadt)
Felipe and Richard, many thanks for looking into this. I was busy with bustage the last few days and it looks like there will be another version to test coming. I'm watching the bug and can test it then.
Flags: needinfo?(jorgk)
I'm not really familiar with component registration (or even xpcshell for that matter), so I won't be much help. Redirecting to :ted, he might have some ideas.
Flags: needinfo?(ahalberstadt) → needinfo?(ted)
xpcshell has a bit of a weird environment and it doesn't load the Firefox app components by default. We have opt-in support for individual tests (or entire test manifests) to enable them, like:
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/browser/components/about/test/unit/xpcshell.ini#4

We stub out some other services in xpcshell tests, like the idle service:
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/testing/xpcshell/head.js#231

So we ought to be able to stub this out as well. Since we have some tests that run with the Firefox app components and some without, I assume we'd want to be able to detect whether this service is already registered, so that tests that want to use the real Firefox implementation can do so? Is that as simple as testing `"@whatever" in Cc`? If so then this should be fairly straightforward. If you want to use the stub implementation in this patch for xpcshell tests, you could put it in `TESTING_JS_MODULES` and then load it via `resource://testing-common/` like:
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/testing/xpcshell/moz.build#13
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/testing/xpcshell/head.js#405
Flags: needinfo?(ted)
Ok, this is a good direction, although I don't know what the solution is yet, because the problem is slightly different.

The problem is not in tests.. The problem is in any real codepath that contains a call to this component (`Services.policies`) and is reached during xpcshell tests.

Previously, all these codepaths were first checking for its existence, but the very purpose of this bug is to make that not necessary. So I implemented a stub implementation that lives in toolkit, to be used by thunderbird/etc..

The problem is that the real implementation is inside browser/components/, and the stub one is conditional to `CONFIG['MOZ_BUILD_APP'] != 'browser'`.  Which means that in xpcshell tests that run from a Firefox build, neither implementation exists.


Btw, testing as `"@foo" in Cc` works. So from what you said, it looks like the easiest would be to implement a stub in head.js with that. But that means there will be two stub implementations (the one implemented in this patch, and the one in xpcshell/head.js).. And now I'm wondering if there's a way to have only one.



Needinfo'ing you again to see what you suggest from this :)
Flags: needinfo?(ted)
I think we're on the same page here, but maybe I just didn't explain what I was thinking clearly enough!

I think that you should make the xpcshell test harness load and register the stub implementation when the real implementation is not present. (It will have to be conditional because some tests run with `firefox-appdir = browser` which should load all the browser components.) To do that without duplicating the stub implementation, I would suggest adding it to `TESTING_JS_MODULES`, which will let you load it from a `resource://testing-common/` URL from head.js.

Does that make sense?
Flags: needinfo?(ted)
See Also: → 1461330
Priority: -- → P3
not working on this at the moment, so un-assigning in case someone else has interest in taking it.

not sure how useful this is anymore, vs. going all the way to implementing support for Thunderbird
Assignee: felipc → nobody
Status: ASSIGNED → NEW

For reasons explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1461330#c6, I don't think this bug is useful. We should just take a simpler approach and bring the component over to toolkit, instead of trying to have a stub copy in toolkit and the full copy in browser.

I'll close this bug to avoid someone wasting time on it.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.