Browser Plugin: PpapiCommandBufferProxy should see a lost context if the embedder has deleted the P…

Browser Plugin: PpapiCommandBufferProxy should see a lost context if the embedder has deleted the PluginInstance

In cross-process navigation we swap out PluginInstances and delete the swapped out instance.
PpapiCommandBufferProxy is sitting in WebGraphicsContext3DCommandBufferImpl, and is unaware of the destruction.
It attempts to talk to the embedder to flush over and over again and hangs the guest renderer.

With this change, the renderer's compositor will realize that the context is lost, and will drop it.

BUG=none
TEST=manually.


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

[piman, 2012-05-18 02:42:45]

It's useful to clean up this case, but I would like to understand why we get in
it. We shouldn't have to lose contexts for proper termination.
When the plugin is removed, the host will send a sync DidDestroy message, where
the guest is supposed to clean up - including destroying contexts - before the
resources are destroyed on the host side.
Why isn't that happening, can we fix it?

https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppapi_comm...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppapi_comm...
ppapi/proxy/ppapi_command_buffer_proxy.cc:265: if (state.error !=
gpu::error::kNoError ||
See comments in the other files, I'd rather the
PpapiHostMsg_PPBGraphics3D_GetState/new PpapiHostMsg_PPBGraphics3D_Flush return
failure so that we can keep the generation accounting and not clobber existing
state with uninitialized values.

https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppb_graphi...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppb_graphi...
ppapi/proxy/ppb_graphics_3d_proxy.cc:252: state->error =
gpu::error::kLostContext;
Mmh, this is a little bogus, because we're not filling the structure completely.
It would be better to return a success bool, so that we can keep the existing
state, but change to kLostContext on the plugin side and increase the generation
(that will probably be needed for a pepper plugin inside of a guest).
That will also remove the need for the hack in
PpapiCommandBufferProxy::UpdateState

[Fady Samuel, 2012-05-18 02:55:42]

On 2012/05/18 02:42:45, piman wrote:
> It's useful to clean up this case, but I would like to understand why we get
in
> it. We shouldn't have to lose contexts for proper termination.
> When the plugin is removed, the host will send a sync DidDestroy message,
where
> the guest is supposed to clean up - including destroying contexts - before the
> resources are destroyed on the host side.
> Why isn't that happening, can we fix it?
> 
>
https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppapi_comm...
> File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppapi_comm...
> ppapi/proxy/ppapi_command_buffer_proxy.cc:265: if (state.error !=
> gpu::error::kNoError ||
> See comments in the other files, I'd rather the
> PpapiHostMsg_PPBGraphics3D_GetState/new PpapiHostMsg_PPBGraphics3D_Flush
return
> failure so that we can keep the generation accounting and not clobber existing
> state with uninitialized values.
> 
>
https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppb_graphi...
> File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10387182/diff/1/ppapi/proxy/ppb_graphi...
> ppapi/proxy/ppb_graphics_3d_proxy.cc:252: state->error =
> gpu::error::kLostContext;
> Mmh, this is a little bogus, because we're not filling the structure
completely.
> It would be better to return a success bool, so that we can keep the existing
> state, but change to kLostContext on the plugin side and increase the
generation
> (that will probably be needed for a pepper plugin inside of a guest).
> That will also remove the need for the hack in
> PpapiCommandBufferProxy::UpdateState

I will address the code in the morning. In a nutshell, when we navigate away to
another process, if the current process has another guest in it, it will stay
alive and all the RenderViews will stay alive in case we navigate back. If I
recall correctly, about a month ago, you suggested that the
GuestToEmbedderChannel (earlier called GuestToHostChannel) should not report
lost context to the PpapiCommandBufferProxy because the PpapiCommandBufferProxy
may be running and owned by the compositor thread. 

GuestToEmbedderChannel doesn't know or keep track of the
PpapiCommandBufferProxys that are out there and so they can outlive the plugin
instance and continue sending bogus flush messages to the embedder.

[Fady Samuel, 2012-05-18 19:27:48]

Addressed your initial comments. Yes, I agree this isn't the ideal solution, and
we shouldn't be using failure paths for navigation, but I can't think of a
better solution off the top of my head.

http://codereview.chromium.org/10387182/diff/1/ppapi/proxy/ppapi_command_buff...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

http://codereview.chromium.org/10387182/diff/1/ppapi/proxy/ppapi_command_buff...
ppapi/proxy/ppapi_command_buffer_proxy.cc:265: if (state.error !=
gpu::error::kNoError ||
On 2012/05/18 02:42:46, piman wrote:
> See comments in the other files, I'd rather the
> PpapiHostMsg_PPBGraphics3D_GetState/new PpapiHostMsg_PPBGraphics3D_Flush
return
> failure so that we can keep the generation accounting and not clobber existing
> state with uninitialized values.

Done.

http://codereview.chromium.org/10387182/diff/1/ppapi/proxy/ppb_graphics_3d_pr...
File ppapi/proxy/ppb_graphics_3d_proxy.cc (right):

http://codereview.chromium.org/10387182/diff/1/ppapi/proxy/ppb_graphics_3d_pr...
ppapi/proxy/ppb_graphics_3d_proxy.cc:252: state->error =
gpu::error::kLostContext;
On 2012/05/18 02:42:46, piman wrote:
> Mmh, this is a little bogus, because we're not filling the structure
completely.
> It would be better to return a success bool, so that we can keep the existing
> state, but change to kLostContext on the plugin side and increase the
generation
> (that will probably be needed for a pepper plugin inside of a guest).
> That will also remove the need for the hack in
> PpapiCommandBufferProxy::UpdateState

Done.

[piman, 2012-05-18 20:01:43]

You're missing the changes to the messages definition in this CL.

http://codereview.chromium.org/10387182/diff/5002/ppapi/proxy/ppapi_command_b...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

http://codereview.chromium.org/10387182/diff/5002/ppapi/proxy/ppapi_command_b...
ppapi/proxy/ppapi_command_buffer_proxy.cc:269: last_state_ = state;
Don't do that if !success, because then you would be overwriting last_state_
with uninitialized data.

http://codereview.chromium.org/10387182/diff/5002/ppapi/proxy/ppapi_command_b...
ppapi/proxy/ppapi_command_buffer_proxy.cc:271: last_state_.error =
gpu::error::kLostContext;
To be fully correct, you should also increase the generation since you changed
the state.

[Fady Samuel, 2012-05-18 20:44:42]

https://chromiumcodereview.appspot.com/10387182/diff/5002/ppapi/proxy/ppapi_c...
File ppapi/proxy/ppapi_command_buffer_proxy.cc (right):

https://chromiumcodereview.appspot.com/10387182/diff/5002/ppapi/proxy/ppapi_c...
ppapi/proxy/ppapi_command_buffer_proxy.cc:269: last_state_ = state;
On 2012/05/18 20:01:43, piman wrote:
> Don't do that if !success, because then you would be overwriting last_state_
> with uninitialized data.

Done.

https://chromiumcodereview.appspot.com/10387182/diff/5002/ppapi/proxy/ppapi_c...
ppapi/proxy/ppapi_command_buffer_proxy.cc:271: last_state_.error =
gpu::error::kLostContext;
On 2012/05/18 20:01:43, piman wrote:
> To be fully correct, you should also increase the generation since you changed
> the state.

Done.

[piman, 2012-05-18 20:48:10]

lgtm

[commit-bot: I haz the power, 2012-05-18 20:49:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10387182/4

[commit-bot: I haz the power, 2012-05-19 00:04:37]

Change committed as 137981