1127374 - Make ContentParent::RecvLoadPlugin less failure prone

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

#1 top KillHard cause (see parent bug)

mozilla::dom::PContentChild::SendLoadPlugin(unsigned int const &)

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#978

Comment 1

[Jim Mathies [:jimm]]

Attachment: sync bits

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

This should eliminate the open question of whether or not SendLoadPlugin failures happen in plugin library initialization code. The other possibility would be that the auto-generated protocol code fails for some reason.

Comment 3

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

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

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +91,5 @@
>      static void ReleaseCallee(Class* obj) { }
>  };
>  
>  bool
>  mozilla::plugins::SetupBridge(uint32_t aPluginId,

It's kinda weird that this returns two kinds of failure conditions. Do we actually want to treat a failure in ::Bridge differently? How about having it return an nsresult and then converting the result from ::Bridge to an nsresult?

Comment 4

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #3)
> Comment on attachment 8556719 [details] [diff] [review]
> patch
> 
> Review of attachment 8556719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/ipc/PluginModuleParent.cpp
> @@ +91,5 @@
> >      static void ReleaseCallee(Class* obj) { }
> >  };
> >  
> >  bool
> >  mozilla::plugins::SetupBridge(uint32_t aPluginId,
> 
> It's kinda weird that this returns two kinds of failure conditions. Do we
> actually want to treat a failure in ::Bridge differently? How about having
> it return an nsresult and then converting the result from ::Bridge to an
> nsresult?

Isn't PPluginModule::Bridge auto-generated code? Looks like it's created here - 
https://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PPluginModule.ipdl#37

Comment 5

[Jim Mathies [:jimm]]

will land an additional touchup if we decide on something.

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

Comment 6

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

Sorry, by "it" I meant SetupBridge. So something like this:

if (!PPluginModule::Bridge(...)) {
  return NS_ERROR_UNEXPECTED;
}
return NS_OK;

Comment 7

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #6)
> Sorry, by "it" I meant SetupBridge. So something like this:
> 
> if (!PPluginModule::Bridge(...)) {
>   return NS_ERROR_UNEXPECTED;
> }
> return NS_OK;

I'd like to keep the current fix in place for the weekend so that we can get a feel for whether a majority of the the failures are in the auto-generated PPluginModule code or the plugin code. Sound ok?

Comment 8

[Phil Ringnalda (:philor)]

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