Use an explicit PID for duplicating Pepper handles rather than the Channel's.
When the browser process launches the plugin, it explicitly tells each side the PID of the other side, and we now use this PID for sharing handles. Previously we'd use the PID from the IPC channel.
Using the PID from the IPC channel creates a race condition because the PID isn't set until the "hello" message from the opposite side is processed, which isn't guaranteed at any particular time.
BUG=168222
Review URL: https://codereview.chromium.org/11722017
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175190 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/nacl_host/nacl_process_host.cc b/chrome/browser/nacl_host/nacl_process_host.cc
index 3119c1b..7d628a7 100644
--- a/chrome/browser/nacl_host/nacl_process_host.cc
+++ b/chrome/browser/nacl_host/nacl_process_host.cc
@@ -12,6 +12,7 @@
#include "base/message_loop.h"
#include "base/metrics/histogram.h"
#include "base/path_service.h"
+#include "base/process_util.h"
#include "base/string_number_conversions.h"
#include "base/string_split.h"
#include "base/string_util.h"
@@ -659,9 +660,10 @@
}
#endif
+ const ChildProcessData& data = process_->GetData();
ChromeViewHostMsg_LaunchNaCl::WriteReplyParams(
reply_msg_, handles_for_renderer,
- channel_handle, process_->GetData().id);
+ channel_handle, base::GetProcId(data.handle), data.id);
chrome_render_message_filter_->Send(reply_msg_);
chrome_render_message_filter_ = NULL;
reply_msg_ = NULL;
diff --git a/chrome/common/render_messages.h b/chrome/common/render_messages.h
index ca43e8d..90dc3db 100644
--- a/chrome/common/render_messages.h
+++ b/chrome/common/render_messages.h
@@ -541,7 +541,7 @@
// a new instance of the Native Client process. The browser will launch
// the process and return an IPC channel handle. This handle will only
// be valid if the NaCl IPC proxy is enabled.
-IPC_SYNC_MESSAGE_CONTROL4_3(ChromeViewHostMsg_LaunchNaCl,
+IPC_SYNC_MESSAGE_CONTROL4_4(ChromeViewHostMsg_LaunchNaCl,
GURL /* manifest_url */,
int /* render_view_id */,
uint32 /* permission_bits */,
@@ -549,6 +549,7 @@
std::vector<nacl::FileDescriptor>
/* imc channel handles */,
IPC::ChannelHandle /* ipc_channel_handle */,
+ base::ProcessId /* plugin_pid */,
int /* plugin_child_id */)
// A renderer sends this to the browser process when it wants to
diff --git a/chrome/renderer/pepper/ppb_nacl_private_impl.cc b/chrome/renderer/pepper/ppb_nacl_private_impl.cc
index bb5abf6..ab0801ad 100644
--- a/chrome/renderer/pepper/ppb_nacl_private_impl.cc
+++ b/chrome/renderer/pepper/ppb_nacl_private_impl.cc
@@ -42,9 +42,10 @@
g_background_thread_sender = LAZY_INSTANCE_INITIALIZER;
struct InstanceInfo {
- InstanceInfo() : plugin_child_id(0) {}
+ InstanceInfo() : plugin_pid(base::kNullProcessId), plugin_child_id(0) {}
GURL url;
ppapi::PpapiPermissions permissions;
+ base::ProcessId plugin_pid;
int plugin_child_id;
IPC::ChannelHandle channel_handle;
};
@@ -105,6 +106,7 @@
perm_bits,
socket_count, &sockets,
&instance_info.channel_handle,
+ &instance_info.plugin_pid,
&instance_info.plugin_child_id))) {
return PP_NACL_FAILED;
}
@@ -159,6 +161,7 @@
FilePath().AppendASCII(instance_info.url.spec()),
instance_info.permissions,
instance_info.channel_handle,
+ instance_info.plugin_pid,
instance_info.plugin_child_id);
if (!renderer_ppapi_host)
return PP_NACL_ERROR_MODULE;
diff --git a/content/browser/pepper_flash_settings_helper_impl.cc b/content/browser/pepper_flash_settings_helper_impl.cc
index 2f9e698..bd6af82 100644
--- a/content/browser/pepper_flash_settings_helper_impl.cc
+++ b/content/browser/pepper_flash_settings_helper_impl.cc
@@ -50,6 +50,7 @@
void PepperFlashSettingsHelperImpl::OnPpapiChannelOpened(
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId /* plugin_pid */,
int /* plugin_child_id */) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
DCHECK(!callback_.is_null());
diff --git a/content/browser/pepper_flash_settings_helper_impl.h b/content/browser/pepper_flash_settings_helper_impl.h
index fc4aa48..de34fcf 100644
--- a/content/browser/pepper_flash_settings_helper_impl.h
+++ b/content/browser/pepper_flash_settings_helper_impl.h
@@ -27,6 +27,7 @@
virtual void GetPpapiChannelInfo(base::ProcessHandle* renderer_handle,
int* renderer_id) OVERRIDE;
virtual void OnPpapiChannelOpened(const IPC::ChannelHandle& channel_handle,
+ base::ProcessId plugin_pid,
int plugin_child_id) OVERRIDE;
virtual bool OffTheRecord() OVERRIDE;
diff --git a/content/browser/plugin_data_remover_impl.cc b/content/browser/plugin_data_remover_impl.cc
index 983f84f..6b9ca8a25 100644
--- a/content/browser/plugin_data_remover_impl.cc
+++ b/content/browser/plugin_data_remover_impl.cc
@@ -165,6 +165,7 @@
virtual void OnPpapiChannelOpened(
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId /* peer_pid */,
int /* child_id */) OVERRIDE {
if (!channel_handle.name.empty())
ConnectToChannel(channel_handle, true);
diff --git a/content/browser/plugin_service_impl.cc b/content/browser/plugin_service_impl.cc
index 6de5dac..40fd937 100644
--- a/content/browser/plugin_service_impl.cc
+++ b/content/browser/plugin_service_impl.cc
@@ -342,7 +342,7 @@
plugin_host->OpenChannelToPlugin(client);
} else {
// Send error.
- client->OnPpapiChannelOpened(IPC::ChannelHandle(), 0);
+ client->OnPpapiChannelOpened(IPC::ChannelHandle(), base::kNullProcessId, 0);
}
}
@@ -354,7 +354,7 @@
plugin_host->OpenChannelToPlugin(client);
} else {
// Send error.
- client->OnPpapiChannelOpened(IPC::ChannelHandle(), 0);
+ client->OnPpapiChannelOpened(IPC::ChannelHandle(), base::kNullProcessId, 0);
}
}
diff --git a/content/browser/ppapi_plugin_process_host.cc b/content/browser/ppapi_plugin_process_host.cc
index 10aef15..d705e574 100644
--- a/content/browser/ppapi_plugin_process_host.cc
+++ b/content/browser/ppapi_plugin_process_host.cc
@@ -284,18 +284,19 @@
void PpapiPluginProcessHost::RequestPluginChannel(Client* client) {
base::ProcessHandle process_handle;
- int renderer_id;
- client->GetPpapiChannelInfo(&process_handle, &renderer_id);
+ int renderer_child_id;
+ client->GetPpapiChannelInfo(&process_handle, &renderer_child_id);
// We can't send any sync messages from the browser because it might lead to
// a hang. See the similar code in PluginProcessHost for more description.
PpapiMsg_CreateChannel* msg = new PpapiMsg_CreateChannel(
- renderer_id, client->OffTheRecord());
+ base::GetProcId(process_handle), renderer_child_id,
+ client->OffTheRecord());
msg->set_unblock(true);
if (Send(msg)) {
sent_requests_.push(client);
} else {
- client->OnPpapiChannelOpened(IPC::ChannelHandle(), 0);
+ client->OnPpapiChannelOpened(IPC::ChannelHandle(), base::kNullProcessId, 0);
}
}
@@ -347,12 +348,14 @@
DVLOG(1) << "PpapiPluginProcessHost" << (is_broker_ ? "[broker]" : "")
<< "CancelRequests()";
for (size_t i = 0; i < pending_requests_.size(); i++) {
- pending_requests_[i]->OnPpapiChannelOpened(IPC::ChannelHandle(), 0);
+ pending_requests_[i]->OnPpapiChannelOpened(IPC::ChannelHandle(),
+ base::kNullProcessId, 0);
}
pending_requests_.clear();
while (!sent_requests_.empty()) {
- sent_requests_.front()->OnPpapiChannelOpened(IPC::ChannelHandle(), 0);
+ sent_requests_.front()->OnPpapiChannelOpened(IPC::ChannelHandle(),
+ base::kNullProcessId, 0);
sent_requests_.pop();
}
}
@@ -368,7 +371,9 @@
Client* client = sent_requests_.front();
sent_requests_.pop();
- client->OnPpapiChannelOpened(channel_handle, process_->GetData().id);
+ const ChildProcessData& data = process_->GetData();
+ client->OnPpapiChannelOpened(channel_handle, base::GetProcId(data.handle),
+ data.id);
}
} // namespace content
diff --git a/content/browser/ppapi_plugin_process_host.h b/content/browser/ppapi_plugin_process_host.h
index 6b50856..1bbe897 100644
--- a/content/browser/ppapi_plugin_process_host.h
+++ b/content/browser/ppapi_plugin_process_host.h
@@ -12,6 +12,7 @@
#include "base/file_path.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
+#include "base/process.h"
#include "content/browser/renderer_host/pepper/browser_ppapi_host_impl.h"
#include "content/browser/renderer_host/pepper/pepper_message_filter.h"
#include "content/public/browser/browser_child_process_host_delegate.h"
@@ -46,6 +47,7 @@
// 0
virtual void OnPpapiChannelOpened(
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId plugin_pid,
int plugin_child_id) = 0;
// Returns true if the current connection is off-the-record.
diff --git a/content/browser/renderer_host/render_message_filter.cc b/content/browser/renderer_host/render_message_filter.cc
index cd5dfad..27975a3 100644
--- a/content/browser/renderer_host/render_message_filter.cc
+++ b/content/browser/renderer_host/render_message_filter.cc
@@ -136,19 +136,20 @@
}
virtual void GetPpapiChannelInfo(base::ProcessHandle* renderer_handle,
- int* renderer_id) {
+ int* renderer_id) OVERRIDE {
*renderer_handle = filter()->peer_handle();
*renderer_id = filter()->render_process_id();
}
virtual void OnPpapiChannelOpened(const IPC::ChannelHandle& channel_handle,
- int plugin_child_id) {
+ base::ProcessId plugin_pid,
+ int plugin_child_id) OVERRIDE {
ViewHostMsg_OpenChannelToPepperPlugin::WriteReplyParams(
- reply_msg(), channel_handle, plugin_child_id);
+ reply_msg(), channel_handle, plugin_pid, plugin_child_id);
SendReplyAndDeleteThis();
}
- virtual bool OffTheRecord() {
+ virtual bool OffTheRecord() OVERRIDE {
return filter()->OffTheRecord();
}
@@ -174,20 +175,22 @@
virtual ~OpenChannelToPpapiBrokerCallback() {}
virtual void GetPpapiChannelInfo(base::ProcessHandle* renderer_handle,
- int* renderer_id) {
+ int* renderer_id) OVERRIDE {
*renderer_handle = filter_->peer_handle();
*renderer_id = filter_->render_process_id();
}
virtual void OnPpapiChannelOpened(const IPC::ChannelHandle& channel_handle,
- int /* plugin_child_id */) {
+ base::ProcessId plugin_pid,
+ int /* plugin_child_id */) OVERRIDE {
filter_->Send(new ViewMsg_PpapiBrokerChannelCreated(routing_id_,
request_id_,
+ plugin_pid,
channel_handle));
delete this;
}
- virtual bool OffTheRecord() {
+ virtual bool OffTheRecord() OVERRIDE {
return filter_->OffTheRecord();
}
diff --git a/content/common/view_messages.h b/content/common/view_messages.h
index aedac377..74f3a3c 100644
--- a/content/common/view_messages.h
+++ b/content/common/view_messages.h
@@ -1214,8 +1214,9 @@
// Reply to ViewHostMsg_OpenChannelToPpapiBroker
// Tells the renderer that the channel to the broker has been created.
-IPC_MESSAGE_ROUTED2(ViewMsg_PpapiBrokerChannelCreated,
+IPC_MESSAGE_ROUTED3(ViewMsg_PpapiBrokerChannelCreated,
int /* request_id */,
+ base::ProcessId /* broker_pid */,
IPC::ChannelHandle /* handle */)
// Reply to ViewHostMsg_RequestPpapiBrokerPermission.
@@ -1858,9 +1859,10 @@
// plugin is hung.
//
// On error an empty string and null handles are returned.
-IPC_SYNC_MESSAGE_CONTROL1_2(ViewHostMsg_OpenChannelToPepperPlugin,
+IPC_SYNC_MESSAGE_CONTROL1_3(ViewHostMsg_OpenChannelToPepperPlugin,
FilePath /* path */,
IPC::ChannelHandle /* handle to channel */,
+ base::ProcessId /* plugin_pid */,
int /* plugin_child_id */)
// Notification that a plugin has created a new plugin instance. The parameters
diff --git a/content/ppapi_plugin/ppapi_thread.cc b/content/ppapi_plugin/ppapi_thread.cc
index 0b67d9a..34221e9 100644
--- a/content/ppapi_plugin/ppapi_thread.cc
+++ b/content/ppapi_plugin/ppapi_thread.cc
@@ -167,7 +167,7 @@
IPC::PlatformFileForTransit PpapiThread::ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& channel,
+ base::ProcessId peer_pid,
bool should_close_source) {
#if defined(OS_WIN)
if (peer_handle_.IsValid()) {
@@ -177,8 +177,8 @@
}
#endif
- return BrokerGetFileHandleForProcess(handle, channel.peer_pid(),
- should_close_source);
+ DCHECK(peer_pid != base::kNullProcessId);
+ return BrokerGetFileHandleForProcess(handle, peer_pid, should_close_source);
}
std::set<PP_Instance>* PpapiThread::GetGloballySeenInstanceIDSet() {
@@ -353,11 +353,14 @@
library_.Reset(library.Release());
}
-void PpapiThread::OnCreateChannel(int renderer_id, bool incognito) {
+void PpapiThread::OnCreateChannel(base::ProcessId renderer_pid,
+ int renderer_child_id,
+ bool incognito) {
IPC::ChannelHandle channel_handle;
if (!plugin_entry_points_.get_interface || // Plugin couldn't be loaded.
- !SetupRendererChannel(renderer_id, incognito, &channel_handle)) {
+ !SetupRendererChannel(renderer_pid, renderer_child_id, incognito,
+ &channel_handle)) {
Send(new PpapiHostMsg_ChannelCreated(IPC::ChannelHandle()));
return;
}
@@ -384,13 +387,14 @@
ns->SetOnLine(PP_FromBool(online));
}
-bool PpapiThread::SetupRendererChannel(int renderer_id,
+bool PpapiThread::SetupRendererChannel(base::ProcessId renderer_pid,
+ int renderer_child_id,
bool incognito,
IPC::ChannelHandle* handle) {
DCHECK(is_broker_ == (connect_instance_func_ != NULL));
IPC::ChannelHandle plugin_handle;
plugin_handle.name = IPC::Channel::GenerateVerifiedChannelID(
- StringPrintf("%d.r%d", base::GetCurrentProcId(), renderer_id));
+ StringPrintf("%d.r%d", base::GetCurrentProcId(), renderer_child_id));
ppapi::proxy::ProxyChannel* dispatcher = NULL;
bool init_result = false;
@@ -399,6 +403,7 @@
new BrokerProcessDispatcher(plugin_entry_points_.get_interface,
connect_instance_func_);
init_result = broker_dispatcher->InitBrokerWithChannel(this,
+ renderer_pid,
plugin_handle,
false);
dispatcher = broker_dispatcher;
@@ -408,6 +413,7 @@
permissions_,
incognito);
init_result = plugin_dispatcher->InitPluginWithChannel(this,
+ renderer_pid,
plugin_handle,
false);
dispatcher = plugin_dispatcher;
diff --git a/content/ppapi_plugin/ppapi_thread.h b/content/ppapi_plugin/ppapi_thread.h
index a70e6d9..8f7c901d 100644
--- a/content/ppapi_plugin/ppapi_thread.h
+++ b/content/ppapi_plugin/ppapi_thread.h
@@ -73,7 +73,7 @@
virtual base::WaitableEvent* GetShutdownEvent() OVERRIDE;
virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& channel,
+ base::ProcessId peer_pid,
bool should_close_source) OVERRIDE;
virtual uint32 Register(
ppapi::proxy::PluginDispatcher* plugin_dispatcher) OVERRIDE;
@@ -90,7 +90,9 @@
// Message handlers.
void OnLoadPlugin(const FilePath& path,
const ppapi::PpapiPermissions& permissions);
- void OnCreateChannel(int renderer_id, bool incognito);
+ void OnCreateChannel(base::ProcessId renderer_pid,
+ int renderer_child_id,
+ bool incognito);
void OnResourceReply(
const ppapi::proxy::ResourceMessageReplyParams& reply_params,
const IPC::Message& nested_msg);
@@ -98,7 +100,8 @@
// Sets up the channel to the given renderer. On success, returns true and
// fills the given ChannelHandle with the information from the new channel.
- bool SetupRendererChannel(int renderer_id,
+ bool SetupRendererChannel(base::ProcessId renderer_pid,
+ int renderer_child_id,
bool incognito,
IPC::ChannelHandle* handle);
diff --git a/content/public/browser/child_process_data.h b/content/public/browser/child_process_data.h
index ef5e808..4b5a61c 100644
--- a/content/public/browser/child_process_data.h
+++ b/content/public/browser/child_process_data.h
@@ -30,8 +30,8 @@
base::ProcessHandle handle;
ChildProcessData(content::ProcessType type)
- : type(type), id(0), handle(base::kNullProcessHandle) {
-}
+ : type(type), id(0), handle(base::kNullProcessHandle) {
+ }
};
} // namespace content
diff --git a/content/public/renderer/renderer_ppapi_host.h b/content/public/renderer/renderer_ppapi_host.h
index f8a14325..da4a9b5 100644
--- a/content/public/renderer/renderer_ppapi_host.h
+++ b/content/public/renderer/renderer_ppapi_host.h
@@ -7,6 +7,7 @@
#include "base/memory/ref_counted.h"
#include "base/platform_file.h"
+#include "base/process.h"
#include "content/common/content_export.h"
#include "ipc/ipc_platform_file.h"
#include "ppapi/c/pp_instance.h"
@@ -67,6 +68,7 @@
const FilePath& file_path,
ppapi::PpapiPermissions permissions,
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId plugin_pid,
int plugin_child_id);
// Returns the RendererPpapiHost associated with the given PP_Instance,
diff --git a/content/renderer/pepper/pepper_broker_impl.cc b/content/renderer/pepper/pepper_broker_impl.cc
index cda23e7..d814ddad 100644
--- a/content/renderer/pepper/pepper_broker_impl.cc
+++ b/content/renderer/pepper/pepper_broker_impl.cc
@@ -55,6 +55,7 @@
}
bool PepperBrokerDispatcherWrapper::Init(
+ base::ProcessId broker_pid,
const IPC::ChannelHandle& channel_handle) {
if (channel_handle.name.empty())
return false;
@@ -70,6 +71,7 @@
new ppapi::proxy::BrokerHostDispatcher());
if (!dispatcher_->InitBrokerWithChannel(dispatcher_delegate_.get(),
+ broker_pid,
channel_handle,
true)) { // Client.
dispatcher_.reset();
@@ -174,10 +176,11 @@
}
void PepperBrokerImpl::OnBrokerChannelConnected(
+ base::ProcessId broker_pid,
const IPC::ChannelHandle& channel_handle) {
scoped_ptr<PepperBrokerDispatcherWrapper> dispatcher(
new PepperBrokerDispatcherWrapper);
- if (!dispatcher->Init(channel_handle)) {
+ if (!dispatcher->Init(broker_pid, channel_handle)) {
ReportFailureToClients(PP_ERROR_FAILED);
return;
}
diff --git a/content/renderer/pepper/pepper_broker_impl.h b/content/renderer/pepper/pepper_broker_impl.h
index 6ac31bb1..d3653bb37 100644
--- a/content/renderer/pepper/pepper_broker_impl.h
+++ b/content/renderer/pepper/pepper_broker_impl.h
@@ -6,6 +6,7 @@
#define CONTENT_RENDERER_PEPPER_PEPPER_BROKER_IMPL_H_
#include "base/memory/ref_counted.h"
+#include "base/process.h"
#include "content/common/content_export.h"
#include "ppapi/proxy/proxy_channel.h"
#include "webkit/plugins/ppapi/plugin_delegate.h"
@@ -37,7 +38,8 @@
PepperBrokerDispatcherWrapper();
~PepperBrokerDispatcherWrapper();
- bool Init(const IPC::ChannelHandle& channel_handle);
+ bool Init(base::ProcessId broker_pid,
+ const IPC::ChannelHandle& channel_handle);
int32_t SendHandleToBroker(PP_Instance instance,
base::SyncSocket::Handle handle);
@@ -60,7 +62,8 @@
void AddPendingConnect(webkit::ppapi::PPB_Broker_Impl* client);
// Called when the channel to the broker has been established.
- void OnBrokerChannelConnected(const IPC::ChannelHandle& channel_handle);
+ void OnBrokerChannelConnected(base::ProcessId broker_pid,
+ const IPC::ChannelHandle& channel_handle);
// Called when we know whether permission to access the PPAPI broker was
// granted.
diff --git a/content/renderer/pepper/pepper_broker_impl_unittest.cc b/content/renderer/pepper/pepper_broker_impl_unittest.cc
index 4a909a1..aa61bf8 100644
--- a/content/renderer/pepper/pepper_broker_impl_unittest.cc
+++ b/content/renderer/pepper/pepper_broker_impl_unittest.cc
@@ -31,8 +31,8 @@
// such as the one in CreatePipe(). Call it twice because without the invalid
// handle check, the posix code would hit a one-time path due to a static
// variable and go through the LOG(FATAL) path.
- EXPECT_FALSE(dispatcher_wrapper.Init(invalid_channel));
- EXPECT_FALSE(dispatcher_wrapper.Init(invalid_channel));
+ EXPECT_FALSE(dispatcher_wrapper.Init(base::kNullProcessId, invalid_channel));
+ EXPECT_FALSE(dispatcher_wrapper.Init(base::kNullProcessId, invalid_channel));
}
// On valid ChannelHandle, initialization should succeed.
@@ -50,7 +50,7 @@
IPC::ChannelHandle valid_channel(kChannelName);
#endif // defined(OS_POSIX));
- EXPECT_TRUE(dispatcher_wrapper.Init(valid_channel));
+ EXPECT_TRUE(dispatcher_wrapper.Init(base::kNullProcessId, valid_channel));
#if defined(OS_POSIX)
EXPECT_EQ(0, ::close(fds[0]));
diff --git a/content/renderer/pepper/pepper_plugin_delegate_impl.cc b/content/renderer/pepper/pepper_plugin_delegate_impl.cc
index 23fdd7b..0c7aa1b3 100644
--- a/content/renderer/pepper/pepper_plugin_delegate_impl.cc
+++ b/content/renderer/pepper/pepper_plugin_delegate_impl.cc
@@ -106,10 +106,12 @@
: public webkit::ppapi::PluginDelegate::OutOfProcessProxy {
public:
HostDispatcherWrapper(webkit::ppapi::PluginModule* module,
+ base::ProcessId peer_pid,
int plugin_child_id,
const ppapi::PpapiPermissions& perms,
bool is_external)
: module_(module),
+ peer_pid_(peer_pid),
plugin_child_id_(plugin_child_id),
permissions_(perms),
is_external_(is_external) {
@@ -134,6 +136,7 @@
module_->pp_module(), local_get_interface, filter, permissions_));
if (!dispatcher_->InitHostWithChannel(dispatcher_delegate_.get(),
+ peer_pid_,
channel_handle,
true, // Client.
preferences)) {
@@ -194,6 +197,8 @@
private:
webkit::ppapi::PluginModule* module_;
+ base::ProcessId peer_pid_;
+
// ID that the browser process uses to idetify the child process for the
// plugin. This isn't directly useful from our process (the renderer) except
// in messages to the browser to disambiguate plugins.
@@ -397,9 +402,10 @@
// Out of process: have the browser start the plugin process for us.
IPC::ChannelHandle channel_handle;
+ base::ProcessId peer_pid;
int plugin_child_id = 0;
render_view_->Send(new ViewHostMsg_OpenChannelToPepperPlugin(
- path, &channel_handle, &plugin_child_id));
+ path, &channel_handle, &peer_pid, &plugin_child_id));
if (channel_handle.name.empty()) {
// Couldn't be initialized.
return scoped_refptr<webkit::ppapi::PluginModule>();
@@ -417,6 +423,7 @@
path,
permissions,
channel_handle,
+ peer_pid,
plugin_child_id,
false)) // is_external = false
return scoped_refptr<webkit::ppapi::PluginModule>();
@@ -429,6 +436,7 @@
const FilePath& path,
ppapi::PpapiPermissions permissions,
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId peer_pid,
int plugin_child_id) {
// We don't call PepperPluginRegistry::AddLiveModule, as this module is
// managed externally.
@@ -436,6 +444,7 @@
path,
permissions,
channel_handle,
+ peer_pid,
plugin_child_id,
true); // is_external = true
}
@@ -472,6 +481,7 @@
const FilePath& path,
ppapi::PpapiPermissions permissions,
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId peer_pid,
int plugin_child_id,
bool is_external) {
scoped_refptr<PepperHungPluginFilter> hung_filter(
@@ -480,6 +490,7 @@
plugin_child_id));
scoped_ptr<HostDispatcherWrapper> dispatcher(
new HostDispatcherWrapper(module,
+ peer_pid,
plugin_child_id,
permissions,
is_external));
@@ -501,20 +512,21 @@
void PepperPluginDelegateImpl::OnPpapiBrokerChannelCreated(
int request_id,
+ base::ProcessId broker_pid,
const IPC::ChannelHandle& handle) {
scoped_refptr<PepperBrokerImpl>* broker_ptr =
pending_connect_broker_.Lookup(request_id);
if (broker_ptr) {
scoped_refptr<PepperBrokerImpl> broker = *broker_ptr;
pending_connect_broker_.Remove(request_id);
- broker->OnBrokerChannelConnected(handle);
+ broker->OnBrokerChannelConnected(broker_pid, handle);
} else {
// There is no broker waiting for this channel. Close it so the broker can
// clean up and possibly exit.
// The easiest way to clean it up is to just put it in an object
// and then close them. This failure case is not performance critical.
PepperBrokerDispatcherWrapper temp_dispatcher;
- temp_dispatcher.Init(handle);
+ temp_dispatcher.Init(broker_pid, handle);
}
}
diff --git a/content/renderer/pepper/pepper_plugin_delegate_impl.h b/content/renderer/pepper/pepper_plugin_delegate_impl.h
index e5513fa..16ea3ab2e 100644
--- a/content/renderer/pepper/pepper_plugin_delegate_impl.h
+++ b/content/renderer/pepper/pepper_plugin_delegate_impl.h
@@ -77,6 +77,7 @@
const FilePath& path,
ppapi::PpapiPermissions permissions,
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId plugin_pid,
int plugin_child_id);
// Removes broker from pending_connect_broker_ if present. Returns true if so.
@@ -119,6 +120,7 @@
int message_id) OVERRIDE;
virtual void OnPpapiBrokerChannelCreated(
int request_id,
+ base::ProcessId broker_pid,
const IPC::ChannelHandle& handle) OVERRIDE;
virtual void OnPpapiBrokerPermissionResult(int request_id,
bool result) OVERRIDE;
@@ -412,6 +414,7 @@
const FilePath& path,
ppapi::PpapiPermissions permissions,
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId plugin_pid,
int plugin_child_id,
bool is_external);
diff --git a/content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc b/content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc
index 18eb6ca..cbcbe9f 100644
--- a/content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc
+++ b/content/renderer/pepper/pepper_proxy_channel_delegate_impl.cc
@@ -26,9 +26,9 @@
IPC::PlatformFileForTransit
PepperProxyChannelDelegateImpl::ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& channel,
+ base::ProcessId remote_pid,
bool should_close_source) {
- return BrokerGetFileHandleForProcess(handle, channel.peer_pid(),
+ return BrokerGetFileHandleForProcess(handle, remote_pid,
should_close_source);
}
diff --git a/content/renderer/pepper/pepper_proxy_channel_delegate_impl.h b/content/renderer/pepper/pepper_proxy_channel_delegate_impl.h
index 243047b..f4d58eb 100644
--- a/content/renderer/pepper/pepper_proxy_channel_delegate_impl.h
+++ b/content/renderer/pepper/pepper_proxy_channel_delegate_impl.h
@@ -20,7 +20,7 @@
virtual base::WaitableEvent* GetShutdownEvent() OVERRIDE;
virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& channel,
+ base::ProcessId remote_pid,
bool should_close_source) OVERRIDE;
};
diff --git a/content/renderer/pepper/renderer_ppapi_host_impl.cc b/content/renderer/pepper/renderer_ppapi_host_impl.cc
index c573c97..31fb3fd 100644
--- a/content/renderer/pepper/renderer_ppapi_host_impl.cc
+++ b/content/renderer/pepper/renderer_ppapi_host_impl.cc
@@ -36,6 +36,7 @@
const FilePath& file_path,
ppapi::PpapiPermissions permissions,
const IPC::ChannelHandle& channel_handle,
+ base::ProcessId plugin_pid,
int plugin_child_id) {
RendererPpapiHost* renderer_ppapi_host = NULL;
// Since we're the embedder, we can make assumptions about the delegate on
@@ -48,6 +49,7 @@
file_path,
permissions,
channel_handle,
+ plugin_pid,
plugin_child_id);
}
return renderer_ppapi_host;
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index de3951f5..bbc86d1 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -6354,9 +6354,9 @@
void RenderViewImpl::OnPpapiBrokerChannelCreated(
int request_id,
+ base::ProcessId broker_pid,
const IPC::ChannelHandle& handle) {
- pepper_helper_->OnPpapiBrokerChannelCreated(request_id,
- handle);
+ pepper_helper_->OnPpapiBrokerChannelCreated(request_id, broker_pid, handle);
}
void RenderViewImpl::OnPpapiBrokerPermissionResult(
diff --git a/content/renderer/render_view_impl.h b/content/renderer/render_view_impl.h
index abe9b57cf..1079af74 100644
--- a/content/renderer/render_view_impl.h
+++ b/content/renderer/render_view_impl.h
@@ -908,6 +908,7 @@
IPC::PlatformFileForTransit file_for_transit,
int message_id);
void OnPpapiBrokerChannelCreated(int request_id,
+ base::ProcessId broker_pid,
const IPC::ChannelHandle& handle);
void OnPpapiBrokerPermissionResult(int request_id, bool result);
void OnCancelDownload(int32 download_id);
diff --git a/content/renderer/render_view_pepper_helper.h b/content/renderer/render_view_pepper_helper.h
index ced5bf8..3449ccc9 100644
--- a/content/renderer/render_view_pepper_helper.h
+++ b/content/renderer/render_view_pepper_helper.h
@@ -12,6 +12,7 @@
#include "base/basictypes.h"
#include "base/memory/ref_counted.h"
#include "base/platform_file.h"
+#include "base/process.h"
#include "ui/base/ime/text_input_type.h"
class TransportDIB;
@@ -75,6 +76,7 @@
// Called by RenderView when ViewMsg_PpapiBrokerChannelCreated.
virtual void OnPpapiBrokerChannelCreated(int request_id,
+ base::ProcessId broker_pid,
const IPC::ChannelHandle& handle) {}
// Called when we know whether permission to access the PPAPI broker was
diff --git a/ipc/ipc_channel.h b/ipc/ipc_channel.h
index e86fc35..34f1bba 100644
--- a/ipc/ipc_channel.h
+++ b/ipc/ipc_channel.h
@@ -129,7 +129,14 @@
void set_listener(Listener* listener);
// Get the process ID for the connected peer.
- // Returns base::kNullProcessId if the peer is not connected yet.
+ //
+ // Returns base::kNullProcessId if the peer is not connected yet. Watch out
+ // for race conditions. You can easily get a channel to another process, but
+ // if your process has not yet processed the "hello" message from the remote
+ // side, this will fail. You should either make sure calling this is either
+ // in response to a message from the remote side (which guarantees that it's
+ // been connected), or you wait for the "connected" notification on the
+ // listener.
base::ProcessId peer_pid() const;
// Send a message over the Channel to the listener on the other end.
diff --git a/ppapi/proxy/broker_dispatcher.cc b/ppapi/proxy/broker_dispatcher.cc
index 97fca5c3..7187852 100644
--- a/ppapi/proxy/broker_dispatcher.cc
+++ b/ppapi/proxy/broker_dispatcher.cc
@@ -21,9 +21,11 @@
bool BrokerDispatcher::InitBrokerWithChannel(
ProxyChannel::Delegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client) {
- return ProxyChannel::InitWithChannel(delegate, channel_handle, is_client);
+ return ProxyChannel::InitWithChannel(delegate, peer_pid, channel_handle,
+ is_client);
}
bool BrokerDispatcher::OnMessageReceived(const IPC::Message& msg) {
diff --git a/ppapi/proxy/broker_dispatcher.h b/ppapi/proxy/broker_dispatcher.h
index de83b2b1..2da4a47 100644
--- a/ppapi/proxy/broker_dispatcher.h
+++ b/ppapi/proxy/broker_dispatcher.h
@@ -20,6 +20,7 @@
// The delegate pointer must outlive this class, ownership is not
// transferred.
virtual bool InitBrokerWithChannel(ProxyChannel::Delegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client);
diff --git a/ppapi/proxy/host_dispatcher.cc b/ppapi/proxy/host_dispatcher.cc
index 7124c17..cea7ab3 100644
--- a/ppapi/proxy/host_dispatcher.cc
+++ b/ppapi/proxy/host_dispatcher.cc
@@ -87,10 +87,12 @@
bool HostDispatcher::InitHostWithChannel(
Delegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client,
const ppapi::Preferences& preferences) {
- if (!Dispatcher::InitWithChannel(delegate, channel_handle, is_client))
+ if (!Dispatcher::InitWithChannel(delegate, peer_pid, channel_handle,
+ is_client))
return false;
AddIOThreadMessageFilter(sync_status_.get());
diff --git a/ppapi/proxy/host_dispatcher.h b/ppapi/proxy/host_dispatcher.h
index 98a7ad5..1b0c9a7 100644
--- a/ppapi/proxy/host_dispatcher.h
+++ b/ppapi/proxy/host_dispatcher.h
@@ -58,6 +58,7 @@
// The delegate pointer must outlive this class, ownership is not
// transferred.
virtual bool InitHostWithChannel(Delegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client,
const Preferences& preferences);
diff --git a/ppapi/proxy/plugin_dispatcher.cc b/ppapi/proxy/plugin_dispatcher.cc
index 1b9c80a5..812b2e9 100644
--- a/ppapi/proxy/plugin_dispatcher.cc
+++ b/ppapi/proxy/plugin_dispatcher.cc
@@ -155,9 +155,11 @@
bool PluginDispatcher::InitPluginWithChannel(
PluginDelegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client) {
- if (!Dispatcher::InitWithChannel(delegate, channel_handle, is_client))
+ if (!Dispatcher::InitWithChannel(delegate, peer_pid, channel_handle,
+ is_client))
return false;
plugin_delegate_ = delegate;
plugin_dispatcher_id_ = plugin_delegate_->Register(this);
diff --git a/ppapi/proxy/plugin_dispatcher.h b/ppapi/proxy/plugin_dispatcher.h
index 2fb0395..1c213b6 100644
--- a/ppapi/proxy/plugin_dispatcher.h
+++ b/ppapi/proxy/plugin_dispatcher.h
@@ -126,6 +126,7 @@
// The delegate pointer must outlive this class, ownership is not
// transferred.
bool InitPluginWithChannel(PluginDelegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client);
diff --git a/ppapi/proxy/plugin_main_nacl.cc b/ppapi/proxy/plugin_main_nacl.cc
index 04d9be3..8733b9c 100644
--- a/ppapi/proxy/plugin_main_nacl.cc
+++ b/ppapi/proxy/plugin_main_nacl.cc
@@ -66,7 +66,7 @@
virtual base::WaitableEvent* GetShutdownEvent() OVERRIDE;
virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& channel,
+ base::ProcessId peer_pid,
bool should_close_source) OVERRIDE;
virtual std::set<PP_Instance>* GetGloballySeenInstanceIDSet() OVERRIDE;
virtual uint32 Register(PluginDispatcher* plugin_dispatcher) OVERRIDE;
@@ -104,7 +104,10 @@
shutdown_event_(true, false) {
IPC::ChannelHandle channel_handle(
"NaCl IPC", base::FileDescriptor(NACL_IPC_FD, false));
- InitWithChannel(this, channel_handle, false); // Channel is server.
+ // We don't have/need a PID since handle sharing happens outside of the
+ // NaCl sandbox.
+ InitWithChannel(this, base::kNullProcessId, channel_handle,
+ false); // Channel is server.
channel()->AddFilter(
new components::ChildTraceMessageFilter(message_loop_));
}
@@ -119,7 +122,7 @@
IPC::PlatformFileForTransit PpapiDispatcher::ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& channel,
+ base::ProcessId peer_pid,
bool should_close_source) {
return IPC::InvalidPlatformFileForTransit();
}
@@ -190,7 +193,8 @@
new PluginDispatcher(::PPP_GetInterface, permissions, incognito);
// The channel handle's true name is not revealed here.
IPC::ChannelHandle channel_handle("nacl", handle.descriptor());
- if (!dispatcher->InitPluginWithChannel(this, channel_handle, false)) {
+ if (!dispatcher->InitPluginWithChannel(this, base::kNullProcessId,
+ channel_handle, false)) {
delete dispatcher;
return;
}
diff --git a/ppapi/proxy/ppapi_messages.h b/ppapi/proxy/ppapi_messages.h
index 71501e31..e29aa5b8 100644
--- a/ppapi/proxy/ppapi_messages.h
+++ b/ppapi/proxy/ppapi_messages.h
@@ -299,8 +299,9 @@
// Creates a channel to talk to a renderer. The plugin will respond with
// PpapiHostMsg_ChannelCreated.
-IPC_MESSAGE_CONTROL2(PpapiMsg_CreateChannel,
- int /* renderer_id */,
+IPC_MESSAGE_CONTROL3(PpapiMsg_CreateChannel,
+ base::ProcessId /* renderer_pid */,
+ int /* renderer_child_id */,
bool /* incognito */)
// Creates a channel to talk to a renderer. This message is only used by the
diff --git a/ppapi/proxy/ppapi_proxy_test.cc b/ppapi/proxy/ppapi_proxy_test.cc
index 5f38908..1af1dfca5 100644
--- a/ppapi/proxy/ppapi_proxy_test.cc
+++ b/ppapi/proxy/ppapi_proxy_test.cc
@@ -202,6 +202,7 @@
PpapiPermissions(),
false));
plugin_dispatcher_->InitPluginWithChannel(&plugin_delegate_mock_,
+ base::kNullProcessId,
channel_handle,
is_client);
plugin_delegate_mock_.set_browser_sender(plugin_dispatcher_.get());
@@ -230,7 +231,7 @@
IPC::PlatformFileForTransit
PluginProxyTestHarness::PluginDelegateMock::ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& /* channel */,
+ base::ProcessId /* remote_pid */,
bool should_close_source) {
return IPC::GetFileHandleForProcess(handle,
base::Process::Current().handle(),
@@ -338,7 +339,8 @@
status_receiver_.release(),
PpapiPermissions::AllPermissions()));
ppapi::Preferences preferences;
- host_dispatcher_->InitHostWithChannel(&delegate_mock_, channel_handle,
+ host_dispatcher_->InitHostWithChannel(&delegate_mock_,
+ base::kNullProcessId, channel_handle,
is_client, preferences);
HostDispatcher::SetForInstance(pp_instance(), host_dispatcher_.get());
}
@@ -361,7 +363,7 @@
IPC::PlatformFileForTransit
HostProxyTestHarness::DelegateMock::ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& /* channel */,
+ base::ProcessId /* remote_pid */,
bool should_close_source) {
return IPC::GetFileHandleForProcess(handle,
base::Process::Current().handle(),
diff --git a/ppapi/proxy/ppapi_proxy_test.h b/ppapi/proxy/ppapi_proxy_test.h
index a72368e4c..e8c15c1 100644
--- a/ppapi/proxy/ppapi_proxy_test.h
+++ b/ppapi/proxy/ppapi_proxy_test.h
@@ -120,7 +120,7 @@
virtual base::WaitableEvent* GetShutdownEvent() OVERRIDE;
virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& /* channel */,
+ base::ProcessId remote_pid,
bool should_close_source) OVERRIDE;
// PluginDispatcher::PluginDelegate implementation.
@@ -202,7 +202,7 @@
virtual base::WaitableEvent* GetShutdownEvent();
virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& /* channel */,
+ base::ProcessId remote_pid,
bool should_close_source) OVERRIDE;
private:
diff --git a/ppapi/proxy/proxy_channel.cc b/ppapi/proxy/proxy_channel.cc
index d1c47a5..81eb9a4 100644
--- a/ppapi/proxy/proxy_channel.cc
+++ b/ppapi/proxy/proxy_channel.cc
@@ -4,6 +4,8 @@
#include "ppapi/proxy/proxy_channel.h"
+#include "base/logging.h"
+#include "base/process_util.h"
#include "ipc/ipc_platform_file.h"
#include "ipc/ipc_test_sink.h"
@@ -16,6 +18,7 @@
ProxyChannel::ProxyChannel()
: delegate_(NULL),
+ peer_pid_(base::kNullProcessId),
test_sink_(NULL) {
}
@@ -24,9 +27,11 @@
}
bool ProxyChannel::InitWithChannel(Delegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client) {
delegate_ = delegate;
+ peer_pid_ = peer_pid;
IPC::Channel::Mode mode = is_client ? IPC::Channel::MODE_CLIENT
: IPC::Channel::MODE_SERVER;
channel_.reset(new IPC::SyncChannel(channel_handle, mode, this,
@@ -38,6 +43,9 @@
void ProxyChannel::InitWithTestSink(IPC::TestSink* test_sink) {
DCHECK(!test_sink_);
test_sink_ = test_sink;
+#if !defined(OS_NACL)
+ peer_pid_ = base::GetCurrentProcId();
+#endif
}
void ProxyChannel::OnChannelError() {
@@ -65,7 +73,8 @@
}
return IPC::InvalidPlatformFileForTransit();
}
- return delegate_->ShareHandleWithRemote(handle, *channel_,
+ DCHECK(peer_pid_ != base::kNullProcessId);
+ return delegate_->ShareHandleWithRemote(handle, peer_pid_,
should_close_source);
}
diff --git a/ppapi/proxy/proxy_channel.h b/ppapi/proxy/proxy_channel.h
index bb08f79c7..d35f98a 100644
--- a/ppapi/proxy/proxy_channel.h
+++ b/ppapi/proxy/proxy_channel.h
@@ -47,7 +47,7 @@
// guarantees as ProxyChannel::ShareHandleWithRemote below.
virtual IPC::PlatformFileForTransit ShareHandleWithRemote(
base::PlatformFile handle,
- const IPC::SyncChannel& channel,
+ base::ProcessId remote_pid,
bool should_close_source) = 0;
};
@@ -55,7 +55,8 @@
// Alternative to InitWithChannel() for unit tests that want to send all
// messages sent via this channel to the given test sink. The test sink
- // must outlive this class.
+ // must outlive this class. In this case, the peer PID will be the current
+ // process ID.
void InitWithTestSink(IPC::TestSink* test_sink);
// Shares a file handle (HANDLE / file descriptor) with the remote side. It
@@ -90,6 +91,7 @@
// The delegate pointer must outlive this class, ownership is not
// transferred.
virtual bool InitWithChannel(Delegate* delegate,
+ base::ProcessId peer_pid,
const IPC::ChannelHandle& channel_handle,
bool is_client);
@@ -101,6 +103,11 @@
// Non-owning pointer. Guaranteed non-NULL after init is called.
ProxyChannel::Delegate* delegate_;
+ // PID of the remote process. Use this instead of the Channel::peer_pid since
+ // this is set synchronously on construction rather than waiting on the
+ // "hello" message from the peer (which introduces a race condition).
+ base::ProcessId peer_pid_;
+
// When we're unit testing, this will indicate the sink for the messages to
// be deposited so they can be inspected by the test. When non-NULL, this
// indicates that the channel should not be used.