1161169 - [e10s] mContentParent pointer in plugin code is worrisome

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

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

I was looking at this pointer today and a couple things worry me. First, it isn't going to work when we have multiple content processes, since they can share the same plugin. Second, there doesn't appear to be any code to clear mContentParent if the ContentParent goes away. That's probably not very likely, but it would still be better to get rid of this pointer.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

I'll clean this up.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: MozReview Request: Bug 1161169: Clean up usage of mContentParent and clearly identify it as specifically for async plugin init; r?billm, jimm

mContentParent is really just to be used while handling a synchronous
ContentParent::RecvLoadPlugin call when async plugin init turned on.

In any other context, using it will be unsafe.

This patch adds comments and assertions to ensure that this value isn't set
otherwise, and converts the one use of mContentParent outside of async plugin
init to use an alternative mechanism for identifying the content process.

Review commit: https://reviewboard.mozilla.org/r/37913/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37913/

Comment 3

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

Comment on attachment 8726294 [details]
MozReview Request: Bug 1161169: Clean up usage of mContentParent and clearly identify it as specifically for async plugin init; r?billm, jimm

https://reviewboard.mozilla.org/r/37913/#review34533

Thanks Aaron!

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8726294 [details]
MozReview Request: Bug 1161169: Clean up usage of mContentParent and clearly identify it as specifically for async plugin init; r?billm, jimm

https://reviewboard.mozilla.org/r/37913/#review34669

::: dom/plugins/ipc/PluginModuleParent.h:425
(Diff revision 1)
> +                               base::ProcessId aContentPid,

Please add an entry for this parameter up in the comment header.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8726294 [details]
MozReview Request: Bug 1161169: Clean up usage of mContentParent and clearly identify it as specifically for async plugin init; r?billm, jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37913/diff/1-2/

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3438e0228ed7

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3438e0228ed7