It looks like, at the moment, the EnterprisePoliciesContent stuff adds at least 3-4K of JS overhead per process. We shouldn't ever need to actually load this stuff unless an enterprise policy is active, so we should be able to easily avoid this by only loading the relevant process script when enterprise policies are active.

The code running in the content process needs to somehow know whether there are active policies or not, which is the information provided by this component. It could live outside, but that would be pretty strange. However, the API surface in the content process is very limited, and currently doesn't need to run unless an about: page is being opened. It could be lazy-started only when an about: page loads. So, with the three conditions: - making it lazy start in the content process - whitelist about:blank (and maybe neterror and certerror) - Bug 1469072 - Move Activity Stream into its own content process it would only be initialized in the Activity Stream process. (right now there's no policy to block Activity Stream, but I don't want to whitelist that because it has been a relatively common request for new policies) There are tests that will fail due to the hot-reloading done by tests, but it's fixable. Since this overhead is smaller than others that are also filed, I'm describing the plan above in case someone wants to do it.. Otherwise I don't think it's urgent to get to it before this 3-4K starts being the biggest overhead.

Possibly a simpler alternative is to just move the class to simpleServices.js, and then have it use the key-value store from bug 1463587, rather than registering listeners. That would make it considerably simpler, and it shouldn't take up much memory when it's not instantiated.

That said... I still think my original suggestion is probably the simplest solution. If we only load the process script when there are active enterprise policies, then the service simply won't exist in the content process when there are no policies, and we can assume that everything is allowed. We should still replace the listeners with direct shared data access when bug 1463587 lands, though.

Yeah, I just saw bug 1463587 and it looks like it would greatly simplify the code in EnterprisePoliciesContent.js (as most of that code is dealing with the fact that initialProcessData doesn't update after the process has started) I also think that the custom factory code there is uneeded too, so that's another clean-up (In reply to Kris Maglione [:kmag] from comment #3) > That said... I still think my original suggestion is probably the simplest > solution. > > If we only load the process script when there are active enterprise > policies, then the service simply won't exist in the content process when > there are no policies, and we can assume that everything is allowed. Which process script do you mean?

(In reply to :Felipe Gomes (needinfo me!) from comment #4) > (In reply to Kris Maglione [:kmag] from comment #3) > > That said... I still think my original suggestion is probably the simplest > > solution. > > > > If we only load the process script when there are active enterprise > > policies, then the service simply won't exist in the content process when > > there are no policies, and we can assume that everything is allowed. > > Which process script do you mean? I mean that, rather than registering the enterprise policy service in a chrome.manifest, we can load it as a process script when we know it's needed, and then register the service with the component registrar.

So I didn't go as far as doing the dynamic registration because I think that will be unnecessary for now. But it could be done in a follow-up and it will be much simpler with the simplifications to EnterprisePoliciesContent.js from these patches. The main reason I don't want to do that is because we would need to change consumers to first check for Services.policies, and the API was designed so that you could always call Services.policies.isAllowed() With bug 1472212 this problem is mitigated because it will only be loaded in the activity stream process, or any other process that loads some other about: tab (which presumably will also be grouped together by Fission) I'm changing the bug title to reflect this, and I will file a follow-up for the dynamic registration

Comment on attachment 8995343 [details] Bug 1470324 - Don't load enterprise policies in the content during startup. https://reviewboard.mozilla.org/r/259802/#review266860

Comment on attachment 8995344 [details] Bug 1470324 - Use sharedData instead of initialProcessData in the enterprise policies code. https://reviewboard.mozilla.org/r/259804/#review266862 ::: browser/components/enterprisepolicies/EnterprisePoliciesContent.js:59 (Diff revision 1) > > getActivePolicies() { > throw Cr.NS_ERROR_NOT_AVAILABLE; > } You may as well get rid of this while you're here. XPConnect will take care of throwing an appropriate error for any method that isn't implemented on a wrapped JS object.

Comment on attachment 8995345 [details] Bug 1470324 - Clean up some unused code in EnterprisePoliciesContent.js. https://reviewboard.mozilla.org/r/259806/#review266864 Thanks

> rv = NS_ERROR_FACTORY_NOT_REGISTERED; > - } else { > + } else if (!path.EqualsLiteral("blank") && > + !path.EqualsLiteral("neterror") && > + !path.EqualsLiteral("certerror") && > + !path.EqualsLiteral("home") && > + !path.EqualsLiteral("newtab")) { > nsCOMPtr<nsIEnterprisePolicies> policyManager = > do_GetService("@mozilla.org/browser/enterprisepolicies;1", &rv2); Hm. I was wondering if we need to check for `about:welcome` in `nsAboutProtocolHandler.cpp` as well since Activity Stream runs in three different origins: https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/about/AboutRedirector.cpp#91-97?

(In reply to Jay Lim [:imjching] from comment #16) > Hm. I was wondering if we need to check for `about:welcome` in > `nsAboutProtocolHandler.cpp` as well since Activity Stream runs in three > different origins: Thanks for the tip, Jay! So, originally I wasn't going to add these exceptions in the code, but in order to not need to wait on bug 1472212 to make this bug effective, I added the about:home, about:welcome and about:newtab exceptions in nsAboutProtocolHandler.cpp I'll file a follow-up bug dependent on bug 1472212 to remove them once bug 1472212 lands :) (In reply to Kris Maglione [:kmag] from comment #11) > > getActivePolicies() { > > throw Cr.NS_ERROR_NOT_AVAILABLE; > > } > > You may as well get rid of this while you're here. XPConnect will take care > of throwing an appropriate error for any method that isn't implemented on a > wrapped JS object. Done! (in the clean-up patch) I sent this last night to tryserver and everything looked ok, so I'll land it now

Pushed by felipc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/dc4aad741068 Don't load enterprise policies in the content during startup. r=kmag https://hg.mozilla.org/integration/autoland/rev/e042233ec1ba Use sharedData instead of initialProcessData in the enterprise policies code. r=kmag https://hg.mozilla.org/integration/autoland/rev/a5bc7fb873cb Clean up some unused code in EnterprisePoliciesContent.js. r=kmag

https://hg.mozilla.org/mozilla-central/rev/dc4aad741068 https://hg.mozilla.org/mozilla-central/rev/e042233ec1ba https://hg.mozilla.org/mozilla-central/rev/a5bc7fb873cb