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

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

s/sent/processed/

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 4

[Jim Mathies [:jimm]]

sorry, thought this was low priority. will get to it today.

Comment 5

[Eric Fowler]

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

[Jim Mathies [:jimm]]

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.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Ensure plugin module bridging doesn't preempt async plugin init messages (r3)

Fixing something that I missed in r2. Carrying forward r+.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/790fa96d7af8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/790fa96d7af8