Fix async plugin init to probably handle urgent-priority bridging messages in e10s

RESOLVED FIXED in mozilla38

Status

()

Core
Plug-ins
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

unspecified
mozilla38
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

During async plugin init, it is important that specific messages are sent in order prior to bridging. High priority bridging breaks this and causes bad things to happen.
Crash Signature: [@ PluginModuleMapping::GetModule()]
s/sent/processed/
Summary: Plugin Module protocol bridging must be normal priority in the async case → Fix async plugin init to probably handle urgent-priority bridging messages in e10s
Created attachment 8544810 [details] [diff] [review]
Ensure plugin module bridging doesn't preempt async plugin init messages

This patch delays plugin module bridging in the async init + e10s case until explicitly requested by the content child. This essentially creates a barrier that prevents the urgent-priority bridging from occurring until previous ContentParent to ContentChild messages have been processed.
Attachment #8544810 - Flags: review?(jmathies)

Updated

3 years ago
tracking-e10s: --- → +
Depends on: 1121735
Jim, do you have an ETA on the review for this patch? I can't expand the Nightly test audience for Async Plugin Init until this lands.
Flags: needinfo?(jmathies)

Comment 4

3 years ago
sorry, thought this was low priority. will get to it today.
Flags: needinfo?(jmathies)

Comment 5

3 years ago
I can't edit the platform tag myself, but a heads up that this same crash occurs when visiting hulu.com on linux as well.
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150115030228 CSet: c1f6345f2803
crash: https://crash-stats.mozilla.com/report/index/7c309145-bdd4-4074-ade5-f6bce2150115

Comment 6

3 years ago
Comment on attachment 8544810 [details] [diff] [review]
Ensure plugin module bridging doesn't preempt async plugin init messages

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

::: dom/ipc/PContent.ipdl
@@ +588,5 @@
>      /**
> +     * This call is used by asynchronous plugin instantiation to notify the
> +     * content parent that it is now safe to initiate the plugin bridge.
> +     */
> +    sync RequestPluginBridge(uint32_t aPluginId);

Not a fan of this name, sounds like you are returning a bridge vs. the result of the bridging operation. Unfortunately I'm not coming up with many good alternatives.. maybe something simple like ConnectPluginBridge? Also, please detail what the return result indicates in the comment header.

::: dom/plugins/ipc/PluginBridge.h
@@ +16,5 @@
>  namespace plugins {
>  
>  bool
> +SetupBridge(uint32_t aPluginId, dom::ContentParent* aContentParent,
> +            bool aForceBridgeNow = false);

Do callers use aForceBridgeNow? Doesn't look like it, in which case this can be removed.
Attachment #8544810 - Flags: review?(jmathies) → review+
Created attachment 8550493 [details] [diff] [review]
Ensure plugin module bridging doesn't preempt async plugin init messages (r2)

(In reply to Jim Mathies [:jimm] from comment #6)
> Comment on attachment 8544810 [details] [diff] [review]
> Ensure plugin module bridging doesn't preempt async plugin init messages
> 
> Review of attachment 8544810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/PContent.ipdl
> @@ +588,5 @@
> >      /**
> > +     * This call is used by asynchronous plugin instantiation to notify the
> > +     * content parent that it is now safe to initiate the plugin bridge.
> > +     */
> > +    sync RequestPluginBridge(uint32_t aPluginId);
> 
> Not a fan of this name, sounds like you are returning a bridge vs. the
> result of the bridging operation. Unfortunately I'm not coming up with many
> good alternatives.. maybe something simple like ConnectPluginBridge? Also,
> please detail what the return result indicates in the comment header.

Done.

> 
> ::: dom/plugins/ipc/PluginBridge.h
> @@ +16,5 @@
> >  namespace plugins {
> >  
> >  bool
> > +SetupBridge(uint32_t aPluginId, dom::ContentParent* aContentParent,
> > +            bool aForceBridgeNow = false);
> 
> Do callers use aForceBridgeNow? Doesn't look like it, in which case this can
> be removed.

Yes, in fact it is specifically used by ConnectPluginBridge.
Attachment #8544810 - Attachment is obsolete: true
Attachment #8550493 - Flags: review+
Created attachment 8550494 [details] [diff] [review]
Ensure plugin module bridging doesn't preempt async plugin init messages (r3)

Fixing something that I missed in r2. Carrying forward r+.
Attachment #8550493 - Attachment is obsolete: true
Attachment #8550494 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/790fa96d7af8
https://hg.mozilla.org/mozilla-central/rev/790fa96d7af8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

3 years ago
Duplicate of this bug: 1121735
You need to log in before you can comment on or make changes to this bug.