1098055 - Sync SendMessage calls from plugin may deadlock in the new plugin model

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Basically, a scenario as such:

chrome -> sync ipc -> content -> sync ipc -> plugin -> sync sendmessage -> chrome

in the old model the chrome sync ipc call would usually be on the plugin channel, talking to a module or instance.

in the new model, PContent is the new exposed connection, but it currently doesn't have deferred message processing on it.

As a work around we can turn DMP on for PContent. But I'd prefer we find some other solution that's more light weight.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=027b22bf5574

Comment 3

[Jim Mathies [:jimm]]

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

try builds came up green.

Comment 4

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

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

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

::: dom/ipc/ContentChild.cpp
@@ +685,5 @@
> +    if (mIsForBrowser) {
> +      // Request Windows message deferral behavior on our channel. This
> +      // applies to the top level and all sub plugin protocols since they
> +      // all share the same channel.
> +      GetIPCChannel()->SetChannelFlags(MessageChannel::REQUIRE_DEFERRED_MESSAGE_PROTECTION);

Do we actually need this on the child? Doing so would be much more likely to affect performance since the child sends many sync messages.

::: dom/ipc/ContentParent.cpp
@@ +894,5 @@
> +    } else {
> +      // Request Windows message deferral behavior on our channel. This
> +      // applies to the top level and all sub plugin protocols since they
> +      // all share the same channel.
> +      GetIPCChannel()->SetChannelFlags(MessageChannel::REQUIRE_DEFERRED_MESSAGE_PROTECTION);

This particular function is only used on b2g to create nested child processes. I don't think we want this here. It seems like the ContentParent constructor would be a better place for this.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #4)
> Comment on attachment 8522195 [details] [diff] [review]
> patch
> 
> Review of attachment 8522195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentChild.cpp
> @@ +685,5 @@
> > +    if (mIsForBrowser) {
> > +      // Request Windows message deferral behavior on our channel. This
> > +      // applies to the top level and all sub plugin protocols since they
> > +      // all share the same channel.
> > +      GetIPCChannel()->SetChannelFlags(MessageChannel::REQUIRE_DEFERRED_MESSAGE_PROTECTION);
> 
> Do we actually need this on the child? Doing so would be much more likely to
> affect performance since the child sends many sync messages.

Hmm, I'll take a look. We have some mechanisms in this code that match up on either side, I'll have to take a look at that. 

> ::: dom/ipc/ContentParent.cpp
> @@ +894,5 @@
> > +    } else {
> > +      // Request Windows message deferral behavior on our channel. This
> > +      // applies to the top level and all sub plugin protocols since they
> > +      // all share the same channel.
> > +      GetIPCChannel()->SetChannelFlags(MessageChannel::REQUIRE_DEFERRED_MESSAGE_PROTECTION);
> 
> This particular function is only used on b2g to create nested child
> processes. I don't think we want this here. It seems like the ContentParent
> constructor would be a better place for this.

I was trying to avoid everything related to b2g and it seemed like I had. Will retest.

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch

No corresponding child doesn't look like an issue. I am wondering if I should be checking some of the flags here, aIsForBrowser, aIsForPreallocated, and aIsNuwaProcess. For desktop, all of these were false. Hopefully this doesn't break a b2g desktop simulator.

Comment 7

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

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

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

::: dom/ipc/ContentParent.cpp
@@ +1924,4 @@
>      // PID along with the warning.
>      nsDebugImpl::SetMultiprocessMode("Parent");
>  
> +#ifdef XP_WIN

At first I wasn't worried about b2g since it doesn't send any sync messages from the parent. However, I guess we do use intr messages occasionally, even in b2g. Just to be safe, let's add |&& !defined(MOZ_B2G)| here.

@@ +1926,5 @@
>  
> +#ifdef XP_WIN
> +    // Request Windows message deferral behavior on our channel. This
> +    // applies to the top level and all sub plugin protocols since they
> +    // all share the same channel.

Could you provide some details about why we're doing this? Just give the situation where we send a CPOW to content which sends a sync message to a plugin.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch (r=billm)

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eb74e2e33452

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch (r=billm)

fixing a typo.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Carsten Book [:Tomcat]]

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