Ensure that we display a sad face for a windowed plugin when the plugin proce...

Ensure that we display a sad face for a windowed plugin when the plugin process crashes. This was a regression introduced by creating the plugin parent on the browser UI thread, which remains valid when the plugin process crashes.

The fix is to track plugin wrapper windows in the PluginProcessHost class and destroy any remaining windows in the destructor. Removed the DestroyWindowTask
which destroys the windows on the UI thread. We can achieve the same result by posting WM_CLOSE messages to these windows.

This fixes http://code.google.com/p/chromium/issues/detail?id=7673

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

[jam, 2009-03-04 20:28:57]

I don't understand why the second part of this changelist is needed?

"The other change is to send over the plugin window to the browser in the 
PluginProcessHostMsg_CreateWindow message instead of its parent. The handler for
this message retrieves the parent and creates a child window accordingly. It
also sets the new window as the parent of the plugin window. "

[ananta, 2009-03-04 21:06:59]

On 2009/03/04 20:28:57, John Abd-El-Malek wrote:
> I don't understand why the second part of this changelist is needed?
> 
> "The other change is to send over the plugin window to the browser in the 
> PluginProcessHostMsg_CreateWindow message instead of its parent. The handler
for
> this message retrieves the parent and creates a child window accordingly. It
> also sets the new window as the parent of the plugin window. "

It is a touch cleaner this way I think. In the future if we ever run the plugins
in lower integrity level on Vista, then the SetParent call from the plugin
process would fail.

[jam, 2009-03-04 22:07:02]

On 2009/03/04 21:06:59, ananta wrote:
> On 2009/03/04 20:28:57, John Abd-El-Malek wrote:
> > I don't understand why the second part of this changelist is needed?
> > 
> > "The other change is to send over the plugin window to the browser in the 
> > PluginProcessHostMsg_CreateWindow message instead of its parent. The handler
> for
> > this message retrieves the parent and creates a child window accordingly. It
> > also sets the new window as the parent of the plugin window. "
> 
> It is a touch cleaner this way I think. In the future if we ever run the
plugins
> in lower integrity level on Vista, then the SetParent call from the plugin
> process would fail.

hmm, I had specifically done it this way so that only the absolute minimum
regarding the plugin process' implementation details is present in the browser
process code.  That's why the browser provides the bare minimum: a way to create
a window on a specific thread.  I'd prefer it if it's still kept that way.  If
we ever run lower integrity level on Vista, we can revisit this.

[ananta, 2009-03-04 22:29:59]

On 2009/03/04 22:07:02, John Abd-El-Malek wrote:
> On 2009/03/04 21:06:59, ananta wrote:
> > On 2009/03/04 20:28:57, John Abd-El-Malek wrote:
> > > I don't understand why the second part of this changelist is needed?
> > > 
> > > "The other change is to send over the plugin window to the browser in the 
> > > PluginProcessHostMsg_CreateWindow message instead of its parent. The
handler
> > for
> > > this message retrieves the parent and creates a child window accordingly.
It
> > > also sets the new window as the parent of the plugin window. "
> > 
> > It is a touch cleaner this way I think. In the future if we ever run the
> plugins
> > in lower integrity level on Vista, then the SetParent call from the plugin
> > process would fail.
> 
> hmm, I had specifically done it this way so that only the absolute minimum
> regarding the plugin process' implementation details is present in the browser
> process code.  That's why the browser provides the bare minimum: a way to
create
> a window on a specific thread.  I'd prefer it if it's still kept that way.  If
> we ever run lower integrity level on Vista, we can revisit this.

Fair enough. Done.

-Ananta

[jam, 2009-03-04 22:50:41]

lgtm, please add a blurb about why closing the hwnd is necessary.

http://codereview.chromium.org/40120/diff/7/8
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/40120/diff/7/8#newcode336
Line 336: switch(message) {
since you're only looking for one message here, I think this code would be a lot
briefer if you just do

if (message == WM_PAINT && !IsWindow(GetWindow(window, GW_CHILD)) {
  PostMessage(window, WM_CLOSE, 0, 0);

[ananta, 2009-03-04 23:27:56]

On 2009/03/04 22:50:41, John Abd-El-Malek wrote:
> lgtm, please add a blurb about why closing the hwnd is necessary.
> 
> http://codereview.chromium.org/40120/diff/7/8
> File chrome/browser/plugin_process_host.cc (right):
> 
> http://codereview.chromium.org/40120/diff/7/8#newcode336
> Line 336: switch(message) {
> since you're only looking for one message here, I think this code would be a
lot
> briefer if you just do
> 
> if (message == WM_PAINT && !IsWindow(GetWindow(window, GW_CHILD)) {
>   PostMessage(window, WM_CLOSE, 0, 0);

As per our discussion updated the CB to track windows in a map and post WM_CLOSE
messages to remaining windows in the destructor of PluginProcessHost. The
entries in the map are erased when we receive the DestroyWindow IPC from the
renderer.

[jam, 2009-03-05 00:38:05]

http://codereview.chromium.org/40120/diff/13/14
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/40120/diff/13/14#newcode384
Line 384: // destroy existing plugin parent windows.
perhaps say 

To ensure that the sad face shows up (and we don't leak HWNDs), we should
destroy ...

http://codereview.chromium.org/40120/diff/13/15
File chrome/browser/plugin_process_host.h (right):

http://codereview.chromium.org/40120/diff/13/15#newcode148
Line 148: std::map<HWND, bool> plugin_parent_windows_map_;
this should be a set since the "bool" is unused.

[ananta, 2009-03-05 01:34:13]

http://codereview.chromium.org/40120/diff/13/14
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/40120/diff/13/14#newcode384
Line 384: // destroy existing plugin parent windows.
On 2009/03/05 00:38:05, John Abd-El-Malek wrote:
> perhaps say 
> 
> To ensure that the sad face shows up (and we don't leak HWNDs), we should
> destroy ...

Done.

http://codereview.chromium.org/40120/diff/13/15
File chrome/browser/plugin_process_host.h (right):

http://codereview.chromium.org/40120/diff/13/15#newcode148
Line 148: std::map<HWND, bool> plugin_parent_windows_map_;
On 2009/03/05 00:38:05, John Abd-El-Malek wrote:
> this should be a set since the "bool" is unused.

Done.

[jam, 2009-03-05 01:47:08]

lgtm