1235018 - Make IPC message deferrable

Status: NEW

(Core :: IPC, defect, P3)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

In Bug 1166173, we want to do the actual content drawing off main thread. Then, we should make sure that all PTexture, PLayerTransaction and PCompositor message are not sent from child to parent during the off-main painting processing. Otherwise, we will have a lot of IPC actor life-time and layer transaction synchronization problem.

The proposed model will be:
https://github.com/JerryShih/doc/blob/master/off-main-painting/off-main.md#ipc

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: ipc pending mode for async and sync message

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: add new special message for ipc deferring mode. v1

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: move some thread checking from SendMessage() to SendMessageInternal(). v1

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: messageChannel deferring mode implementation. v1

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: change Connected() checking position. v1

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: only child side could setup pending mode. v1

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8719396 [details] [diff] [review]
P1: add new special message for ipc deferring mode. v1

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

::: ipc/glue/MessageChannel.cpp
@@ +280,5 @@
>      CxxStackFrame(const CxxStackFrame&) = delete;
>      CxxStackFrame& operator=(const CxxStackFrame&) = delete;
>  };
>  
> +// Special cancel message.

put CancelMessage, GodbyeMessage and MessageDeferringMessage together.

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8719397 [details] [diff] [review]
P2: move some thread checking from SendMessage() to SendMessageInternal(). v1

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

::: ipc/glue/MessageLink.cpp
@@ +178,5 @@
>  {
>      mChan->AssertWorkerThread();
> +
> +    SendMessageInternal(msg);
> +}

Split code into ProcessLink::SendMessage() and ProcessLink::SendMessageInternal().

Only ProcessLink::SendMessage() will check |AssertWorkerThread|

@@ +290,3 @@
>  
> +    SendMessageInternal(msg);
> +}

Split code into ThreadLink::SendMessage() and ThreadLink::SendMessageInternal().
 
Only ThreadLink::SendMessage() will check |AssertWorkerThread|

::: ipc/glue/MessageLink.h
@@ +119,5 @@
>  };
>  
>  class MessageLink
>  {
> +    friend class MessageChannel;

export the protected interface SendMessageInternal() to MessageChannel.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8719398 [details] [diff] [review]
P3: messageChannel deferring mode implementation. v1

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

The ipc message passing diagram:
https://github.com/JerryShih/doc/blob/master/off-main-painting/ipc.md

All messages after StartPendingMessage() will be deferred until EndPendingMessage() calls.

::: ipc/glue/MessageChannel.cpp
@@ +705,5 @@
> +      return true;
> +    }
> +  }
> +
> +  // If we still in pending mode, put message into mIPCPendingMessage.

s/If we still/If we are still/

@@ +721,5 @@
>  {
>      AssertLinkThread();
>      mMonitor->AssertCurrentThreadOwns();
>  
> +    if (MaybeInterceptSpecialPendingMessage(aMsg)) {

Check if the message is MESSAGE_DEFERRING_START_MESSAGE_TYPE, MESSAGE_DEFERRING_END_MESSAGE_TYPE or queue-needing message.

@@ +2300,5 @@
> +  }
> +
> +  MOZ_ASSERT(!mInPending);
> +  mInPending = true;
> +  mLink->SendMessageInternal(msg.forget());

If child side setups pending mode, we send a special |MESSAGE_DEFERRING_START_MESSAGE_TYPE| message using SendMessageInternal(). Since StartPendingMessage() could be called at any thread, SendMessageInternal() doesn't have worker thread checking guard.

@@ +2324,5 @@
> +  }
> +
> +  MOZ_ASSERT(mInPending);
> +  mInPending = false;
> +  mLink->SendMessageInternal(msg.forget());

Similar to StartPendingMessage().

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8719399 [details] [diff] [review]
P4: change Connected() checking position. v1

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

::: ipc/glue/MessageChannel.cpp
@@ +976,5 @@
> +    if (!Connected()) {
> +        ReportConnectionError("MessageChannel::SendAndWait", msg);
> +        mLastSendError = SyncSendError::NotConnectedBeforeSend;
> +        return false;
> +    }

Should we move the |connect| checking forward?

We still call SendMessage() before checking.
https://hg.mozilla.org/mozilla-central/annotate/e355cacefc881ba360d412853b57e8e060e966f4/ipc/glue/MessageChannel.cpp#l901

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8719400 [details] [diff] [review]
P5: only child side could setup pending mode. v1

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

Try to have a restriction that only child side could setup deferring mode.

Comment 12

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

Comment on attachment 8719396 [details] [diff] [review]
P1: add new special message for ipc deferring mode. v1

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

::: ipc/glue/MessageChannel.cpp
@@ +322,5 @@
> +    }
> +};
> +
> +// Special async message for message deferring.
> +class MessageDeferringMessage : public IPC::Message

Maybe just call this DeferMessage.

::: ipc/glue/ProtocolUtils.h
@@ +41,5 @@
>  // protocol 0.  Oops!  We can get away with this until protocol 0
>  // starts approaching its 65,536th message.
>  enum {
> +    MESSAGE_DEFERRING_START_MESSAGE_TYPE = kuint16max - 8,
> +    MESSAGE_DEFERRING_END_MESSAGE_TYPE = kuint16max - 7,

START_DEFER_MESSAGE_TYPE
END_DEFER_MESSAGE_TYPE

Comment 13

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

Comment on attachment 8719398 [details] [diff] [review]
P3: messageChannel deferring mode implementation. v1

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

::: ipc/glue/MessageChannel.cpp
@@ +372,5 @@
>      mFlags(REQUIRE_DEFAULT),
>      mPeerPidSet(false),
> +    mPeerPid(-1),
> +    mRecordIncomingMessage(false),
> +    mInPending(false)

Let's call these fields mDeferring and mOtherSideDeferring.

@@ +685,5 @@
>      }
>  };
>  
> +bool
> +MessageChannel::MaybeInterceptSpecialPendingMessage(const Message& aMsg)

MaybeInterceptSpecialDeferMessage

@@ +687,5 @@
>  
> +bool
> +MessageChannel::MaybeInterceptSpecialPendingMessage(const Message& aMsg)
> +{
> +  if (aMsg.routing_id() == MSG_ROUTING_NONE) {

The indenting here should use 4 spaces since the rest of the file does.

@@ +2282,5 @@
>      }
>  }
>  
> +bool
> +MessageChannel::StartPendingMessage()

StartDeferring

Comment 14

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

Comment on attachment 8719400 [details] [diff] [review]
P5: only child side could setup pending mode. v1

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

We have an mSide field on the channel. Does that not work for you?

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Bill McCloskey (:billm) from comment #14)
> Comment on attachment 8719400 [details] [diff] [review]
> P5: only child side could setup pending mode. v1
> 
> Review of attachment 8719400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We have an mSide field on the channel. Does that not work for you?

That's a good news. Let me try to use this for checking.

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: add new special message for ipc deferring mode. v2

rebase.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: move some thread checking from SendMessage() to SendMessageInternal(). v2

rebase.

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: messageChannel deferring mode implementation. v2

update for comment 13

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: change Connected() checking position change. v2

rebase.

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: only child side could setup pending mode changes. v2

use |mSide| to check the ipc side.