Closed
Bug 1098055
Opened 10 years ago
Closed 10 years ago
Sync SendMessage calls from plugin may deadlock in the new plugin model
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(e10sm5+)
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
e10s | m5+ | --- |
People
(Reporter: jimm, Assigned: jimm)
References
Details
Attachments
(1 file, 3 obsolete files)
1.08 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → jmathies
Assignee | ||
Comment 2•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=027b22bf5574
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8522195 [details] [diff] [review] patch try builds came up green.
Attachment #8522195 -
Flags: review?(wmccloskey)
Updated•10 years ago
|
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)
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eb74e2e33452
Attachment #8523209 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Blocks: e10s-plugins
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25dd1871ad5
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f25dd1871ad5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•