Closed Bug 1127374 Opened 5 years ago Closed 5 years ago

Make ContentParent::RecvLoadPlugin less failure prone

Categories

(Core :: Plug-ins, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s m5+ ---

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 1 obsolete file)

#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
Attached patch sync bits (obsolete) — Splinter Review
Attached patch patchSplinter Review
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.
Attachment #8556666 - Attachment is obsolete: true
Attachment #8556719 - Flags: review?(wmccloskey)
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?
Attachment #8556719 - Flags: review?(wmccloskey) → review+
(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
will land an additional touchup if we decide on something.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f106104d8fd7
Sorry, by "it" I meant SetupBridge. So something like this:

if (!PPluginModule::Bridge(...)) {
  return NS_ERROR_UNEXPECTED;
}
return NS_OK;
(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?
https://hg.mozilla.org/mozilla-central/rev/f106104d8fd7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.