linux/mac: Fix race condition when destroying the renderer<->plugin channel

linux/mac: Fix race condition when destroying the renderer<->plugin channel

There is a race condition at plugin destruction on posix:
1- (renderer) WebPluginDelegateProxy1 opens a channel to the plugin
2- (plugin) new channel created, sends FD1 to renderer
3- (renderer) WebPluginDelegateProxy1 receives FD1, establishes the channel name -> FD1 mapping.
[...]
4- (renderer) WebPluginDelegateProxy1 asks the plugin to destroy an instance, and schedules self for delayed deletion which will release the channel and remove the mapping.
5- (plugin) this was the last instance, plugin closes its end of the channel, removes its mapping.
6- (renderer) WebPluginDelegateProxy2 opens a channel to the plugin
7- (plugin) new channel created, sends FD2 to renderer
8- (renderer) WebPluginDelegateProxy2 receives FD2, establishes the channel name -> FD2 mapping *ASSERT* because the mapping already exists (to FD1)
9- (renderer) WebPluginDelegateProxy1 deleted, causes channel host destruction and removing of channel name -> FD1 mapping

The channel host destruction in (9) needs to happen before (8). This CL does that.

BUG=18491
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23102

[jam, 2009-08-11 01:37:47]

http://codereview.chromium.org/165280/diff/1002/4
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/165280/diff/1002/4#newcode218
Line 218: channel_host_ = NULL;
What about other WebPluginDelegateProxy methods that assume channel_host_ is
non-null and can be called in the meantime (i.e. Paint)?  Don't these functions
need to be updated in this change?

[piman, 2009-08-11 03:09:43]

On Mon, Aug 10, 2009 at 6:37 PM, <jam@chromium.org> wrote:
>
> http://codereview.chromium.org/165280/diff/1002/4
> File chrome/renderer/webplugin_delegate_proxy.cc (right):
>
> http://codereview.chromium.org/165280/diff/1002/4#newcode218
> Line 218: channel_host_ =3D NULL;
> What about other WebPluginDelegateProxy methods that assume
> channel_host_ is non-null and can be called in the meantime (i.e.
> Paint)? =A0Don't these functions need to be updated in this change?

So, I actually spent a lot of time trying to figure out all the paths
to the WebPluginDelegateProxy, to make sure that change was sane. I
think that I got them all, but feel free to discuss further. Here's
the detail:
The delegate is created in WebFrameLoaderClient::createPlugin. That
calls RenderView::CreatePluginDelegate, and the RenderView keeps it in
its plugin_delegates_ list, but that is only used to clean up the back
pointer by calling WebPluginDelegate::DropRenderView, which doesn't
access channel_host_.
From WebFrameLoaderClient it is passed directly to the WebPluginImpl,
without keeping it as a reference.
WebPluginImpl keeps the reference to it in delegate_. Mostly it calls
functions on it, except it gives it to its webframe_ member, that
keeps a reference to it accessible only through its
WebFrame::plugin_delegate() member. Every use of
WebFrame::plugin_delegate() that I could find was used to call
WebPluginDelegateProxy::DidFinishLoadWithReason. That function only
calls WebPluginDelegateProxy::Send which safely tests for
channel_host_. Note that before this patch if we called
WebPluginDelegateProxy::DidFinishLoadWithReason after
WebPluginDelegateProxy::PluginDestroyed(), we would send a message
with an invalid route id (which fortunately isn't that bad because
route ids aren't re-used except after wrapping but at that point all
hell breaks loose anyway), now we don't any more.
Back to the WebPluginImpl, it resets its delegate_ member after
calling PluginDestroyed(), so it can't call a function (like Paint) on
WebPluginDelegateProxy if channel_host_ is NULL.
So, we're down to the WebPluginDelegateProxy itself. The only places
it passes itself ('this') to another function/object are:
- MessageLoop::current()->DeleteSoon(FROM_HERE, this);  -- no problem.
- render_view_->PluginDestroyed(this);  -- all this does is remove
itself from the RenderView::plugin_delegates_, and reset the
first_default_plugin_ member, none of which access the channel_host_.
- window_script_object_->set_proxy(this);  -- this makes the
NPObjectStub keep a reference to the delegate, but that's only used to
clean up the back pointer in
WebPluginDelegateProxy::DropWindowScriptObject, which doesn't access
the channel_host_.
- render_view_->OnMissingPluginStatus(this, status);  -- the
RenderView keeps it as first_default_plugin_, only to call
WebPluginDelegateProxy::InstallMissingPlugin(), which only calls Send
which is safe as seen above.
- channel_host_->AddRoute(instance_id_, this, false); -- that's the
only thing remaining, which lets the channel call back into the
WebPluginDelegateProxy on messages. However, the route is removed
before we set channel_host_ to NULL, so it is safe as well.


>
> http://codereview.chromium.org/165280
>

[jam, 2009-08-11 04:07:15]

lgtm, thanks for the thorough writeup.  You've alleviated my concern.

On 2009/08/11 03:09:43, piman wrote:
> On Mon, Aug 10, 2009 at 6:37 PM, <jam@chromium.org> wrote:
> >
> > http://codereview.chromium.org/165280/diff/1002/4
> > File chrome/renderer/webplugin_delegate_proxy.cc (right):
> >
> > http://codereview.chromium.org/165280/diff/1002/4#newcode218
> > Line 218: channel_host_ =3D NULL;
> > What about other WebPluginDelegateProxy methods that assume
> > channel_host_ is non-null and can be called in the meantime (i.e.
> > Paint)? =A0Don't these functions need to be updated in this change?
> 
> So, I actually spent a lot of time trying to figure out all the paths
> to the WebPluginDelegateProxy, to make sure that change was sane. I
> think that I got them all, but feel free to discuss further. Here's
> the detail:
> The delegate is created in WebFrameLoaderClient::createPlugin. That
> calls RenderView::CreatePluginDelegate, and the RenderView keeps it in
> its plugin_delegates_ list, but that is only used to clean up the back
> pointer by calling WebPluginDelegate::DropRenderView, which doesn't
> access channel_host_.
> From WebFrameLoaderClient it is passed directly to the WebPluginImpl,
> without keeping it as a reference.
> WebPluginImpl keeps the reference to it in delegate_. Mostly it calls
> functions on it, except it gives it to its webframe_ member, that
> keeps a reference to it accessible only through its
> WebFrame::plugin_delegate() member. Every use of
> WebFrame::plugin_delegate() that I could find was used to call
> WebPluginDelegateProxy::DidFinishLoadWithReason. That function only
> calls WebPluginDelegateProxy::Send which safely tests for
> channel_host_. Note that before this patch if we called
> WebPluginDelegateProxy::DidFinishLoadWithReason after
> WebPluginDelegateProxy::PluginDestroyed(), we would send a message
> with an invalid route id (which fortunately isn't that bad because
> route ids aren't re-used except after wrapping but at that point all
> hell breaks loose anyway), now we don't any more.
> Back to the WebPluginImpl, it resets its delegate_ member after
> calling PluginDestroyed(), so it can't call a function (like Paint) on
> WebPluginDelegateProxy if channel_host_ is NULL.
> So, we're down to the WebPluginDelegateProxy itself. The only places
> it passes itself ('this') to another function/object are:
> - MessageLoop::current()->DeleteSoon(FROM_HERE, this);  -- no problem.
> - render_view_->PluginDestroyed(this);  -- all this does is remove
> itself from the RenderView::plugin_delegates_, and reset the
> first_default_plugin_ member, none of which access the channel_host_.
> - window_script_object_->set_proxy(this);  -- this makes the
> NPObjectStub keep a reference to the delegate, but that's only used to
> clean up the back pointer in
> WebPluginDelegateProxy::DropWindowScriptObject, which doesn't access
> the channel_host_.
> - render_view_->OnMissingPluginStatus(this, status);  -- the
> RenderView keeps it as first_default_plugin_, only to call
> WebPluginDelegateProxy::InstallMissingPlugin(), which only calls Send
> which is safe as seen above.
> - channel_host_->AddRoute(instance_id_, this, false); -- that's the
> only thing remaining, which lets the channel call back into the
> WebPluginDelegateProxy on messages. However, the route is removed
> before we set channel_host_ to NULL, so it is safe as well.
> 
> 
> >
> > http://codereview.chromium.org/165280
> >