697314 - Defer importing xpinstall permissions until necessary

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Currently on the startup of a new version of an app we import any xpinstall whitelist prefs into the permissions manager. This is obviously causing the permissions manager to spin up its database because the single pref we currently import on first run is costing us 100ms.

Comment 1

[Dave Townsend [:mossop]]

Additionally Fennec frequently doesn't save prefs when quitting so we attempt to re-import on most startups, after the first run this only costs 15ms but still.

Of course if the permissions manager has to come up at some point during startup then it probably doesn't matter that we do it in the add-ons manager

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Is there any downside to deferring this to when there's a requested addon install? Aside from the management UI, of course - I figure that could send an observer notification to flush any changes.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #2)
> Is there any downside to deferring this to when there's a requested addon
> install? Aside from the management UI, of course - I figure that could send
> an observer notification to flush any changes.

No downside, the only reason I did it on first startup is because previously there were three different places that were importing the prefs to the DB and I wanted to centralise it. Some signal to a central place seems like a good option.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 818840 [details] [diff] [review]
Patch v1

Review of attachment 818840 [details] [diff] [review]:
-----------------------------------------------------------------

Since all importPermissions does now is call out to PermissionsUtils, wouldn't it be easier to just do that directly from each place rather than call the observer service to trigger it?

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +1806,5 @@
>      this.enabledAddons = "";
>  
>      Services.prefs.addObserver(PREF_EM_MIN_COMPAT_APP_VERSION, this, false);
>      Services.prefs.addObserver(PREF_EM_MIN_COMPAT_PLATFORM_VERSION, this, false);
> +    Services.obs.addObserver(this, NOTIFICATION_FLUSH_PERMISSIONS, false);

false for a string?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 818840 [details] [diff] [review]
Patch v1

14:18 <•Mossop> So basically ignore everything I said in that review

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

 https://hg.mozilla.org/integration/fx-team/rev/3f41909b9433

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Blair McBride [:Unfocused] from comment #6)
> 14:18 <•Mossop> So basically ignore everything I said in that review

Why?

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f41909b9433

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> Why?

For the first point, he misunderstood what I was doing. For the second point, he misread addObserver as notifyObservers.

Comment 11

[Mihai Morar, (:MihaiMorar)]

Marking as [qa-] based on the existing automated test in the patch.