Closed Bug 1098055 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Plug-ins, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m5+ ---

People

(Reporter: jimm, Assigned: jimm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
See Also: → 1098057
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jmathies
Comment on attachment 8522195 [details] [diff] [review]
patch

try builds came up green.
Attachment #8522195 - Flags: review?(wmccloskey)
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.
Attachment #8522195 - Flags: review?(wmccloskey)
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8522195 - Attachment is obsolete: true
Attachment #8523209 - Flags: review?(wmccloskey)
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.
Attachment #8523209 - Flags: review?(wmccloskey) → review+
Attached patch patch (r=billm)Splinter Review
fixing a typo.
Attachment #8523313 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: e10s-plugins
https://hg.mozilla.org/mozilla-central/rev/f25dd1871ad5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.