Closed Bug 1363541 Opened 7 years ago Closed 4 years ago

Permission Manager should not block the first window from painting

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla77

Project Flags:

Performance Impact

medium

Tracking Flags:

Tracking

Status

firefox77

---

fixed

People

(Reporter: mconley, Assigned: baku)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, perf:responsiveness, Whiteboard: [fxperf:p2])

Attachments

(6 files, 1 obsolete file)

Bug 1363541 - Modernize the PermissionManager - part 1 - nsPermission to mozilla::Permission, r=timhuang 4 years ago Andrea Marchesini [:baku] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1363541 - Modernize the PermissionManager - part 2 - PermissionDelegateHandler to mozilla::PermissionDelegateHandler, r?timhuang 4 years ago Andrea Marchesini [:baku] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1363541 - Modernize the PermissionManager - part 3 - DB handling in a separate thread, r?timhuang 4 years ago Andrea Marchesini [:baku] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1363541 - Modernize the PermissionManager - part 4 - mozilla namespace, r=timhuang 4 years ago Andrea Marchesini [:baku] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1363541 - Modernize the PermissionManager - part 5 - headers, r=timhuang 4 years ago Andrea Marchesini [:baku] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1363541 - Modernize the PermissionManager - part 6 - moving code around, r=timhuang 4 years ago Andrea Marchesini [:baku] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1363541 - Modernize the PermissionManager - part 7 - Shared thread between cookies and permissions, r=timhuang 4 years ago Andrea Marchesini [:baku] 47 bytes, text/x-phabricator-request		Details \| Review

Mike Conley (:mconley) (:⚙️)

Reporter

Description

•

7 years ago

Apparently, Permission Manager does main thread, sync IO (superb), and also runs before the first window paints.

It's first kicked off here: http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/dom/notification/Notification.cpp#736-740

According to mystor, commenting that out just kicks the can, since it's re-initted as soon as resource://gre-resources/hiddenWindow.html is loaded.

We should not do sync IO here, or if that's somehow unavoidable, we should definitely not block first paint.

Nika Layzell [:nika] (ni? for response)

Comment 1

•

7 years ago

Unfortunately the nsIPermissionManger API is very old, very synchronous, and very widely used around our code base. The best we can do is probably to:

a) start trying to read the data asynchronously and allow main thread work to occur while it is being read, and
b) block the main thread when a sync request for the data is made, waiting for that background thread to finish.

We might be able to improve performance a bit by not actually fetching (a.k.a blocking on reading) permissions during initialization, and instead lazily doing it when permissions are requested for a non-resource://, chrome://, or about: URI. about:home does have a permission set on it (uitour - see http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/browser/app/permissions#15), but we could probably hardcode that one in.

If we remove the during page load permission checks (by not requiring them for parent process URIs), we still need this info as we start a content process - as there are permissions which need to be eagerly sent to the content process. We used to do this in a sync IPC from the content to the parent, but now we send down a subset of permissions at content process startup (which of course will require full permission manager initialization). I tried looking into not doing this, but we definitely can't do it easily. Even if we did all of this work, I'm not sure that we start the content process late enough that this will actually make a difference to our startup times.

Most profiles which I've seen don't have very large nsPermissionManager databases, but I know that some people have very large permission manager databases, as addons have been known to dump a lot of data into them.

Do you think this is doable and/or desirable ehsan?

Flags: needinfo?(ehsan)

(no longer active)

Comment 2

•

7 years ago

I think we should certainly do something here.

My preferred option is starting an async load and blocking on it if we have to for now.  

This requires getting a sense of when the first time that a permission check happens actually is.  It would be nice if you can do a basic preliminary test, something like setting a breakpoint in the TestPermissionCommon function and looking at the call stack of when it gets called first.

If we can reasonably easily make our code not call it when starting up and showing a window, which sounds doable to me, then, the steps become:

0) Add some telemetry about the size of the permissions DB.  Hopefully it'll be small for most people.  As we'll do the below we need to add telemetry for whether the async load finishes in time or not, etc. too.

1) Make browser/app/permissions not require IO if it currently does (I don't remember how it's preloaded off the top of my head).  Please check that!

2) Kick off an async load of the permissions DB early in the startup.

3) Add code to block on the async load if it's in progress when you need to access the DB.

4) Cross your fingers that by that time the load has finished.

How does this sound?  I think the upside of this plan is that we can do it without too much code changes and complexity/risk.

Flags: needinfo?(ehsan)

Nika Layzell [:nika] (ni? for response)

Comment 3

•

7 years ago

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #2)
> I think we should certainly do something here.
> 
> My preferred option is starting an async load and blocking on it if we have
> to for now.  
> 
> This requires getting a sense of when the first time that a permission check
> happens actually is.  It would be nice if you can do a basic preliminary
> test, something like setting a breakpoint in the TestPermissionCommon
> function and looking at the call stack of when it gets called first.

In order to delay this enough to be useful, I'd have to create a definition of the set of origins which we want to allow setting permissions on. I imagine that this set will probably be something along the lines of `http`, `https`, `ftp`, `file`, and perhaps `webextension` URIs. For an initial test I would probably whitelist resource:// chrome:// and about: principals to be checked while the permission manager is not initialized, and would add a print statement when the permission manager would be initialized. I'll see how late in the startup process I can get this. Currently the earliest point in time when I know we _need_ the permission manager to be initialized is process startup, as we need to send down permissions for any file:// URIs or URIs which have registered service workers at process startup. I don't think I'll be able to push this past that point in time.

> If we can reasonably easily make our code not call it when starting up and
> showing a window, which sounds doable to me, then, the steps become:
> 
> 0) Add some telemetry about the size of the permissions DB.  Hopefully it'll
> be small for most people.  As we'll do the below we need to add telemetry
> for whether the async load finishes in time or not, etc. too.
> 
> 1) Make browser/app/permissions not require IO if it currently does (I don't
> remember how it's preloaded off the top of my head).  Please check that!

I think it does perform IO right now (and is probably used by repacks - I'm not sure if we can remove this particular IO - that being said I think it is loaded from the JAR so it shouldn't be too awful).

> 2) Kick off an async load of the permissions DB early in the startup.

I'm thinking we'll want to use a Permissions DB thread to perform this I/O as it will allow us to fill the backing nsTHashtable off main thread as well, and then just hand the data back to the nsPermissionManager on the main thread when we're done, rather than a series of AddInternal calls for each permission as they are loaded from the DB. I imagine it would be much quicker.

> 3) Add code to block on the async load if it's in progress when you need to
> access the DB.
>
> 4) Cross your fingers that by that time the load has finished.

I can add a telemetry probe for when the first request is made as to whether or not the permission manager DB was ready (the probe would probably be Permission Manager wait time)

> How does this sound?  I think the upside of this plan is that we can do it
> without too much code changes and complexity/risk.

Yes, this is pretty much what I was planning to do when I suggested this to you yesterday. Sounds good to me.

Nika Layzell [:nika] (ni? for response)

Comment 4

•

7 years ago

I've done some looking into what happens before the window appears which would trigger the permission manager to be initialized before we paint the first window. There are a few things:

0) The NotificationTelemetryService initializes the permission manager very early on. This should be easy to move to a permission-manager-initialized observer.

1) We eagerly make the content browser remote after DOMContentLoaded is fired (http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1127) - Making a browser remote starts a content process and forces permission manager initialization. This occurs before the window is shown.

2) We start loading 2 HTTP uris before the first window is painted. The order of these being loaded seems to be non-deterministic, and sometimes they load after the window is painted, and sometimes before.
https://location.services.mozilla.com/v1/country?key=no-mozilla-api-key
http://detectportal.firefox.com/success.txt

3) If we don't initialize for the above events, then we will initialize the browser when performing sessionrestore.

4) It doesn't seem like we preallocate a new content process before the window is rendered.

The startup process may take long enough that if we start early on (perhaps when we get the profile directory?) in a secondary thread to load the permission manager we could have it initialized before DOMContentLoaded is fired, saving us the need to block there.

If we do that, our biggest problems will be with our migrations code, which access services like the history service in order to perform history migrations. We wouldn't be able to run these migrations on background threads, so we would probably have to make whatever unlucky main thread event triggers the permission manager to be initialized synchronously run the migration if the background permission manager initializer doesn't do it correctly.

Flags: needinfo?(ehsan)

Nika Layzell [:nika] (ni? for response)

Comment 5

•

7 years ago

Assigning to self while I investigate.

Assignee: nobody → michael

(no longer active)

Comment 6

•

7 years ago

We talked about this in person with Michael, and he has a plan!

Flags: needinfo?(ehsan)