Closed Bug 1161169 Opened 9 years ago Closed 8 years ago

[e10s] mContentParent pointer in plugin code is worrisome

Categories

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

defect

Tracking

(e10s+, firefox40 affected, firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
e10s + ---
firefox40 --- affected
firefox47 --- fixed

People

(Reporter: billm, Assigned: bugzilla)

Details

Attachments

(1 file)

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.
I'll clean this up.
Assignee: nobody → aklotz
Priority: -- → P2
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/
Attachment #8726294 - Flags: review?(wmccloskey)
Attachment #8726294 - Flags: review?(jmathies)
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!
Attachment #8726294 - Flags: review?(wmccloskey) → review+
Attachment #8726294 - Flags: review?(jmathies) → review+
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 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/
https://hg.mozilla.org/mozilla-central/rev/3438e0228ed7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.