Don't load EnterprisePoliciesContent until a policy is checked

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: kmag, Assigned: Felipe)

Tracking

(Depends on 1 bug, Blocks 1 bug)

57 Branch
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [overhead:5k][qf:p3:f64][fxperf:p1])

Attachments

(3 attachments)

Reporter

Description

Last year
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.
Whiteboard: [overhead:5k] → [overhead:5k][fxperf]
Reporter

Comment 2

Last year
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.
Reporter

Comment 3

Last year
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?
Depends on: 1463587
Reporter

Comment 5

Last year
(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.
Whiteboard: [overhead:5k][fxperf] → [overhead:5k][qf:p3:f64][fxperf:p3]
Assignee: nobody → felipc
Whiteboard: [overhead:5k][qf:p3:f64][fxperf:p3] → [overhead:5k][qf:p3:f64][fxperf:p1]
Depends on: 1472212
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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
Summary: Don't load EnterprisePoliciesContent when there are no enterprise policies → Don't load EnterprisePoliciesContent until a policy is checked
Reporter

Comment 10

11 months ago
mozreview-review
Comment on attachment 8995343 [details]
Bug 1470324 - Don't load enterprise policies in the content during startup.

https://reviewboard.mozilla.org/r/259802/#review266860
Attachment #8995343 - Flags: review?(kmaglione+bmo) → review+
Reporter

Comment 11

11 months ago
mozreview-review
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.
Attachment #8995344 - Flags: review?(kmaglione+bmo) → review+
Reporter

Comment 12

11 months ago
mozreview-review
Comment on attachment 8995345 [details]
Bug 1470324 - Clean up some unused code in EnterprisePoliciesContent.js.

https://reviewboard.mozilla.org/r/259806/#review266864

Thanks
Attachment #8995345 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

11 months ago
>              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?
Flags: needinfo?(felipc)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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
No longer depends on: 1472212
Flags: needinfo?(felipc)

Comment 21

11 months ago
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
Depends on: 1479114

Comment 22

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc4aad741068
https://hg.mozilla.org/mozilla-central/rev/e042233ec1ba
https://hg.mozilla.org/mozilla-central/rev/a5bc7fb873cb
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.