Defer importing xpinstall permissions until necessary

RESOLVED FIXED

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: mossop, Assigned: Unfocused)

Tracking

(Blocks: 1 bug)

8 Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
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
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.
(Reporter)

Comment 3

6 years ago
(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.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Created attachment 818840 [details] [diff] [review]
Patch v1
Attachment #818840 - Flags: review?(dtownsend+bugmail)
(Reporter)

Comment 5

5 years ago
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?
Attachment #818840 - Flags: review?(dtownsend+bugmail) → review-
Comment on attachment 818840 [details] [diff] [review]
Patch v1

14:18 <•Mossop> So basically ignore everything I said in that review
Attachment #818840 - Flags: review- → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: 8 Branch → unspecified
(In reply to Blair McBride [:Unfocused] from comment #6)
> 14:18 <•Mossop> So basically ignore everything I said in that review

Why?

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3f41909b9433
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
OS: All → Mac OS X
Hardware: All → x86
Target Milestone: mozilla27 → ---
Version: unspecified → 8 Branch
Marking as [qa-] based on the existing automated test in the patch.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.