Open Bug 1318387 Opened 3 years ago Updated 7 months ago

BroadcastChannel should wait until the security checks are completed

Categories

(Core :: DOM: Core & HTML, defect, P3)

50 Branch
defect

Tracking

()

People

(Reporter: baku, Unassigned)

Details

Attachments

(1 file)

Attached patch broadcast.patchSplinter Review
In BackgroundParentImpl we have a security check to be sure that the origin sent in the BroadcastChannel Parent CTOR matches with the principal. In case the security check fails, we kill the child process.

-The issue here, is that, the actor is immediately active and it can send and receive messages.
We should wait until the security check is completed.
Attachment #8811821 - Flags: review?(bugs)
Comment on attachment 8811821 [details] [diff] [review]
broadcast.patch

> BroadcastChannelParent::RecvPostMessage(const ClonedMessageData& aData)
> {
>   AssertIsOnBackgroundThread();
> 
>   if (NS_WARN_IF(!mService)) {
>     return IPC_FAIL_NO_REASON(this);
>   }
> 
>-  mService->PostMessage(this, aData, mOriginChannelKey);
>+  // This should not happen.
>+  if (mStatus == eDestroyed) {
Useless comment as such. Please explain why this should not happen

>-  mService->UnregisterActor(this, mOriginChannelKey);
>-  mService = nullptr;
>+  // This should not happen.
>+  if (mStatus == eDestroyed || mStatus == eClosing) {
>+    return IPC_FAIL_NO_REASON(this);
Also here, improve the comment. 

>+  if (NS_WARN_IF(!mPendingMessages.IsEmpty()) ||
>+      NS_WARN_IF(!mDeliveringMessages.IsEmpty())) {
>+    return IPC_FAIL_NO_REASON(this);
>+  }
What guarantees this? Add a comment.

>-  return new BroadcastChannelParent(originChannelKey);
>+  RefPtr<BroadcastChannelParent> agent =
>+    new BroadcastChannelParent(originChannelKey);
>+
>+  return agent.forget().take();

This definitely requires a comment that who will call release on the agent.
(Seems to happen in DeallocPBroadcastChannelParent)

>   NS_IMETHOD Run() override
>   {
>-    MOZ_ASSERT(NS_IsMainThread());
>+    // This runnable is dispatch to the main-thread first. Here we do some
is dispatched


Is this all tested both in e10s and non-e10s?
Attachment #8811821 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4749e8571cb
BroadcastChannel should become active only after the security checks, r=smaug
Yes, I don't think there is an easy way to make the security check and having the correct order without being racy.
I NI smaug to see if he has some ideas here.

The bug here is: when we activate the BroadcastChannelParent actor, we start dispatching the messages to/from it. but in the meantime, we don't know if we have pending BroadcastChannelParent actors to be activated for the same context (window, in this case).
Flags: needinfo?(bugs)
So I'm lost how this new setup makes anything more racy than before.
Flags: needinfo?(bugs)
We activate the BroadcastChannel parent actors after a runnable dispatched to the main-thread. This means that the order of activation is kept correct, but, the message order can change. This because each actor has its own queue of event for what has to be dispatched, but nothing guarantees that 2 actors have their queue in sync, if activates after their main-thread runnable.

Before everything worked because the actors were immediately active and all was in sync, following the order of IPC messages received by the PBackground thread on the parent process.

In this test: https://dxr.mozilla.org/mozilla-central/source/dom/broadcastchannel/tests/test_ordering.html
racily, the order is not correct.

I guess we can keep the current setup and, maybe, improve the security model differently directly on the PBackground thread.
Flags: needinfo?(bugs)
Could we possibly in BroadcastChannelService level pause all the parents while some new parent is having its security check being done?
Flags: needinfo?(bugs)
:baku, are you planning to resume the work here soon-ish?
Flags: needinfo?(amarchesini)
No. I have other priorities at the moment. If somebody wants to take this bug, I can help.
Assignee: amarchesini → nobody
Flags: needinfo?(amarchesini)
Priority: -- → P3
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.