1376987 - Label PContent::Msg_SetPermissionsWithKey message

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This patch labels the SetPermissionsWithKey message with the SystemGroup. The main concern here is that we might fire some sort of permission changed notification that would synchronously affect content. I did find some notifications, but they all seem to use chrome JS.

There's also a concern here that a page could observe that it didn't have a permission before and suddenly it does have the permission. Is it possible to query them in that way?

I also had to fix an issue where SystemGroup isn't initialized at startup or shutdown.

Comment 1

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

Comment on attachment 8882012 [details] [diff] [review]
patch

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

> This patch labels the SetPermissionsWithKey message with the SystemGroup. The main concern here is that we might fire some sort of permission changed notification that would synchronously affect content. I did find some notifications, but they all seem to use chrome JS.
>
> There's also a concern here that a page could observe that it didn't have a permission before and suddenly it does have the permission. Is it possible to query them in that way?
>
> I also had to fix an issue where SystemGroup isn't initialized at startup or shutdown.

We can potentially need to use the data sent by SetPermissionsWithKey quickly in the content process. We send this down immediately before sending down the beginning of a HTTP response, for example, and as the document loads and or parses, we will need to consume the permission information in the content process. We currently make sure to send down this message before we reply to the content process to get this ordering.

If we can label networking IPC messages, then it may be possible to label this message, at least in that case.

This send: http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/dom/ipc/ContentParent.cpp#1396 should probably be unlabeled. It occurs very early during program startup as we need to make sure to get all of the permission information into the content process before we do any action which might depend on it.
Same deal with the service worker permissions: http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/dom/ipc/ContentParent.cpp#2491, as a service worker load could start at any time and we need to be sure that the permissions are present.

We should be able to label http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/dom/ipc/ContentParent.cpp#5136 if we can label HTTP responses, I would think. But then we have a race condition where we have two TabGroups which both load resources with the same origin, A and B. A performs load first and we dispatch the event to A's tabgroup, then B does the load, and doesn't send the permissions, as they were already sent for A. B could then be scheduled to complete the load before we schedule handling A's message, and we have a race.

I'm not sure how easily the other call-sites could be labeled, but they are very rare.

It's unfortunate, but these messages are currently implemented such that they have required ordering relative to other messages being sent to the content process, some of which are labeled and some of which are not. I think we could get most of the benefit out of labeling the call due to HTTP responses, but we'd need to add some fancy logic to tell whether the permissions have been transmitted but not received and potentially re-transmit with a different TabGroup.

If it helps, this message only ever needs a happens-before relationship with messages which are sent after it. We don't ever need to flush messages which were previously sent in any TabGroup, but we do need to make sure that no messages which are sent after the message are processed until after this message is processed. Not sure we have a good way of expressing that other than unlabeled.

TL;DR, this won't work, and we cannot label these messages with the SystemGroup as they strongly depend on ordering.

::: xpcom/threads/SystemGroup.cpp
@@ +87,5 @@
>  SystemGroup::Dispatch(const char* aName,
>                        TaskCategory aCategory,
>                        already_AddRefed<nsIRunnable>&& aRunnable)
>  {
> +  if (!SystemGroupImpl::Get()) {

I think you should be using SystemGroupImpl::Initialized() here IIRC.

Comment 2

[Andrew Overholt [:overholt]]

Do we still need to do this?

Comment 3

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

No, we don't. We aren't doing the Quantum DOM stuff anymore :-).