724878 - Make nsPermissionManager input asynchronous

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Followup bug to bug 702344.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

The objective of this bug is to make nsPermissionManager input asynchronous. I hope that we can simply make _initialization_ asynchronous and further calls synchronous.

Comment 2

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> The objective of this bug is to make nsPermissionManager input asynchronous.
> I hope that we can simply make _initialization_ asynchronous and further
> calls synchronous.

what are the implications for this for snappy?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

From what I understand of my conversations with mak, nsPermissionManager IO takes place in permanence when we request a page. I understand that making it fully asynchronous should make page loading more responsive. I have not attempted to investigate this part further yet, so I may be wrong.

Comment 4

[Alon Zakai (:azakai)]

I instrumented a build and tested on a macbook. Init takes 3 ms, all other operations takes less than a ms, generally a lot less. UpdateDB and AddInternal sometimes get close to 1 ms, but that's about it. So thankfully this does not look so bad at first glance (I recall it was pretty bad on e10s mobile last year, due to lots of cross-process traffic, but that is no longer an issue there anyhow).

With an old profile and an old permissions database file, things might be different though.

Comment 5

[(dormant account)]

We have slow sql telemetry data. Any IO operation is likely to take a few seconds at some point.

Comment 6

[Alon Zakai (:azakai)]

Doing engineering to make operations asynchronous and so forth can definitely help with the bad cases that can happen. We could also attack this from another direction, though: We could make sure that the datafiles we have are within sane limits, like make sure the files are within reasonable size, prune old data, etc.

My suspicion is that if the permissions database is "reasonable", then we probably don't need to rewrite the manager to be asynchronous (data in comment 4 seems to support that). And if it is "unreasonable" then making it asynchronous would help but not completely fix the problem either.

Comment 7

[(dormant account)]

(In reply to Alon Zakai (:azakai) from comment #6)
> Doing engineering to make operations asynchronous and so forth can
> definitely help with the bad cases that can happen. We could also attack
> this from another direction, though: We could make sure that the datafiles
> we have are within sane limits, like make sure the files are within
> reasonable size, prune old data, etc.
> 
> My suspicion is that if the permissions database is "reasonable", then we
> probably don't need to rewrite the manager to be asynchronous (data in
> comment 4 seems to support that). And if it is "unreasonable" then making it
> asynchronous would help but not completely fix the problem either.

I used to think this way too. However various telemetry has shown that IO has nasty perf outliers and as long as we have main thread IO firefox UI will jank horribly during large downloads, background IO activity from other apps, etc. To achieve smooth UX, we need to get rid of all main thread IO.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Backtrace from 30sec+ synchronous write() under nsPermissionManager::Add

I guess this is the right bug for this?

This happened while I was repo sync'ing a b2g checkout.  Firefox went totally unresponsive for about 30s and then I attached with gdb.  I see this fairly often when building gecko or sync'ing repos, was able to catch in gdb this time.

As Taras notes, there's no such thing as "sync IO is OK enough ...", because you're dealing with a physical device with basically unbounded wait times for operations (unlike, say, executing a CPU instruction or reading RAM).  Citing measurements taken from idle desktop-class machines is totally bogus.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch

This is making insertions, deletions and removals from the PermissionManager asyncs. The only sync calls to the DB happens during initialization and re-initialization. Having this async is less obviously important because we will have to block anyway as soon as we will have to read the DB and the amount of work to make that async isn't low. I would recommend making that part a follow-up.

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 657517 [details] [diff] [review]
Patch

Ideally, I would have Yoric to review this but he might be offline very soon for an extended period of time.
Taras, Yoric is offline, could you review this? :)

Comment 11

[Mounir Lamouri (:mounir)]

https://tbpl.mozilla.org/?tree=Try&rev=70055ed2e040

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 657517 [details] [diff] [review]
Patch

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

Canceling review for the moment, as I will not be in position to review this before a few weeks. Mounir, if you still need that review when I return, please feel free to r? me again.

Comment 13

[(dormant account)]

Comment on attachment 657517 [details] [diff] [review]
Patch

Sorry to bounce you again. I don't own this code, my review would not help in getting this landed it.

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 657517 [details] [diff] [review]
Patch

I am certainly not the owner of this code.

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 657517 [details] [diff] [review]
Patch

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

I doubt anyone really owns this code. Jonas already reviewed a bunch of PermissionManager patches. Maybe he can review that one.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 657517 [details] [diff] [review]
Patch

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

Or Justin?

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 657517 [details] [diff] [review]
Patch

I've got this.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 657517 [details] [diff] [review]
Patch

> void
> nsPermissionManager::UpdateDB(OperationType         aOp,
>-                              mozIStorageStatement* aStmt,
>+                              mozIStorageAsyncStatement* aStmt,
>                               int64_t               aID,
>                               const nsACString     &aHost,
>                               const nsACString     &aType,
>                               uint32_t              aPermission,
>                               uint32_t              aExpireType,
>                               int64_t               aExpireTime,
>                               uint32_t              aAppId,
>                               bool                  aIsInBrowserElement)

I'm tempted to say you should re-indent this, but whatever, I think this kind if indentation is annoying.  :)

>   nsresult RemoveAllInternal(bool aNotifyObservers);
>   nsresult RemoveAllFromMemory();
>   nsresult NormalizeToACE(nsCString &aHost);
>   static void UpdateDB(OperationType         aOp,
>-                       mozIStorageStatement* aStmt,
>+                       mozIStorageAsyncStatement* aStmt,
>                        int64_t               aID,
>                        const nsACString     &aHost,
>                        const nsACString     &aType,
>                        uint32_t              aPermission,
>                        uint32_t              aExpireType,
>                        int64_t               aExpireTime,
>                        uint32_t              aAppId,
>                        bool                  aIsInBrowserElement);

Similarly here if you want to be persnickety.

Comment 19

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f50654b6b00a

This is making the permission manager doing only async DB access except for the initialization parts. Given that you want to have the memory reflecting the DB before doing any action with the permission manager, making the permission manager initialization async would be quite more work and might have a smaller impact.

I'm going to open a follow-up for that part.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f50654b6b00a

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 657517 [details] [diff] [review]
Patch

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +1512,4 @@
>    }
>  
> +  nsCOMPtr<mozIStoragePendingStatement> pending;
> +  rv = aStmt->ExecuteAsync(nullptr, getter_AddRefs(pending));

Making these asynchronous without changing the permission manager API seems destined to create race conditions. Before, a postcondition of setting a permission was that immediately getting that same permission would give you the same value you set. Now, that is not the case.

IMO it would have been better to create new *Async versions of the public methods and then deprecate (or immediate remove) the original synchronous versions.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to Brian Smith (:bsmith) from comment #21)
> Comment on attachment 657517 [details] [diff] [review]
> Patch
> 
> Review of attachment 657517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +1512,4 @@
> >    }
> >  
> > +  nsCOMPtr<mozIStoragePendingStatement> pending;
> > +  rv = aStmt->ExecuteAsync(nullptr, getter_AddRefs(pending));
> 
> Making these asynchronous without changing the permission manager API seems
> destined to create race conditions. Before, a postcondition of setting a
> permission was that immediately getting that same permission would give you
> the same value you set. Now, that is not the case.

This is not exactly true. All permission operations are still synchronous. That means, if you add a permission and check if the permission is there, you will get the expected result. The reason is because nsPermissionManager has a memory representation of the permissions and that part is updated synchronously every time there is an operation and this is used to tests.
The database is only used to initialise the permission manager. We don't read anything from it except to initialise the memory representation of the permissions.

The other side effect I can see from that change is that if you crash after modifying a permission, you might not get that change at reboot (because the db write wasn't finished). I wouldn't say this is a real issue.

By the way, this is exactly why the Init() part of the permission manager is still synchronous: the only way to make it asynchronous without breaking our current code is to have all other operations blocking if the init isn't done, which, might be equivalent of making the init synchronous because I think the initialisation is done lazily when we get the permission service and most of the time, we get it to make an operation.
What we could do though is to no longer make the initialisation lazy and init at startup eagerly so we have high chances to get the init done when we actually use the permission manager. That might not work, that might slow startup speed so this clearly need investigation.