1054744 - Replace use of boolean flags in MessageChannel with ChannelFlags apis

Status: NEW

(Core :: IPC, defect, P3)

Description

[Jim Mathies [:jimm]]

Attachment: patch v.1

Follow up after bug 874437 sticks, get rid of boolean flags that have piled up in MessageChannel.

Comment 1

[Jim Mathies [:jimm]]

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

Comment 2

[Benjamin Smedberg]

+    MOZ_ASSERT(aFlags & REQUIRE_URGENT_SCRIPT_BLOCKING ? NS_IsMainThread() : true);
+    mFlags |= aFlags;

Can any flag changes happen while the channel is in operation? Because if any changes happen after initial channel setup, then this is a giant read/write footgun. mFlags isn't an atomic, and |= operations can't be atomic anyway.

Comment 3

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

Comment on attachment 8474228 [details] [diff] [review]
patch v.1

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

::: ipc/glue/MessageChannel.cpp
@@ +1709,5 @@
>      PostErrorNotifyTask();
>  }
>  
>  void
> +MessageChannel::SetChannelFlags(uint32_t aFlags)

Maybe we should assert in this function and the next that we haven't opened the channel yet. I think !mLink should work. Then we'll have to move the existing SetChannelFlags above the Open calls, but that looks easy. Hopefully that will allay bsmedberg's concern.

@@ +1714,2 @@
>  {
> +    MOZ_ASSERT(aFlags & REQUIRE_URGENT_SCRIPT_BLOCKING ? NS_IsMainThread() : true);

Can you put this in ClearFlags as well?

::: ipc/glue/MessageChannel.h
@@ +92,5 @@
>        // WindowsMessageLoop code should enable deferred native message
>        // handling to prevent deadlocks. Should only be used for protocols
>        // that manage child processes which might create native UI, like
>        // plugins.
> +      REQUIRE_DEFERRED_MESSAGE_PROTECTION     = 1 << 0,

The REQUIRE_ prefix on these doesn't really seem necessary. Can we just remove it and change REQUIRE_DEFAULT to DEFAULT_FLAGS? Also, can you please put an empty line after each flag?