Fixes a flash plugin hang caused by opening google finance ticker symbols in ...

Fixes a flash plugin hang caused by opening google finance ticker symbols in a background tab. 

From what I can tell this bug always existed in Chrome.

The flash plugin gets instantiated and gets initial geometry updates during initialization. We download the plugin url after the geometry update. After these geometry updates the webkit layout timer runs and the plugin gets additional geometry updates. However these don't get sent over to the flash plugin instance in the plugin process as the geometry updates for windowed plugins are only flushed during paint, which does not happen in this case. The flash plugin ends up receiving data before geometry update and ends up behaving in a wierd fashion, i.e. not peeking for messages, etc.

The fact that this is a windowed plugin results in the browser ui thread also getting hung until the plugin gets out of this state.

The fix for the geometry update issue is to remove the deferred geometry update stuff. This only exists
for windowed plugins anyway. The geometry update IPC is a plain routed message and it should not be a big
overhead to send these IPCs to the plugin process.

I found that while this change fixes the basic issue, we still see some hangs in the flash plugin because of it receiving
data before the valid geometry update kicks in. John is working on a fix where we never have to call setFrameRect ourselves
and always honor the setFrameRect calls made by Webkit. This issue can be marked as fixed once both fixes get checked in.

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

Bug=12993
TEST=Open google finance and Ctrl Click on the tickers in the page like Basic Materials, etc.


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

[jam, 2009-06-05 09:57:45]

I'm not sure if I'll have connectivity tomorrow or not, so summarizing my
comments.  If you test with pages with windowed plugins and don't see a perf
regression, then the change is good with the comments below.

http://codereview.chromium.org/119200/diff/12/1019
File chrome/renderer/render_view.cc (left):

http://codereview.chromium.org/119200/diff/12/1019#oldcode2513
Line 2513: 
Since this function is going away, there's no need for the virtual DidPaint in
RenderWidget since it was just used to call this.

http://codereview.chromium.org/119200/diff/12/1017
File chrome/renderer/webplugin_delegate_proxy.cc (left):

http://codereview.chromium.org/119200/diff/12/1017#oldcode388
Line 388: if (!backing_store_canvas_.get() ||
With the above changes, need to make sure this code only runs for windowless
otherwise all the bitmaps are allocated needlessly.

http://codereview.chromium.org/119200/diff/12/1013
File webkit/glue/webplugin_delegate.h (left):

http://codereview.chromium.org/119200/diff/12/1013#oldcode104
Line 104: virtual void FlushGeometryUpdates() = 0;
This should also be removed from webplugin_delegate_impl.h

http://codereview.chromium.org/119200/diff/12/1015
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/119200/diff/12/1015#newcode692
Line 692: &WebPluginImpl::OnDownloadPluginSrcUrl), 0);
I'd first try to see if HandleUrlRequestInternal can be called after the
Initialize() call that ends up calling NPP_New, to match other browsers.  If it
still causes issues, then you can do a post delayed task from there. 
first_geometry_update_ is unnecessary.

[ananta, 2009-06-05 18:42:51]

On 2009/06/05 09:57:45, John Abd-El-Malek wrote:
> I'm not sure if I'll have connectivity tomorrow or not, so summarizing my
> comments.  If you test with pages with windowed plugins and don't see a perf
> regression, then the change is good with the comments below.
> 
> http://codereview.chromium.org/119200/diff/12/1019
> File chrome/renderer/render_view.cc (left):
> 
> http://codereview.chromium.org/119200/diff/12/1019#oldcode2513
> Line 2513: 
> Since this function is going away, there's no need for the virtual DidPaint in
> RenderWidget since it was just used to call this.
> 
> http://codereview.chromium.org/119200/diff/12/1017
> File chrome/renderer/webplugin_delegate_proxy.cc (left):
> 
> http://codereview.chromium.org/119200/diff/12/1017#oldcode388
> Line 388: if (!backing_store_canvas_.get() ||
> With the above changes, need to make sure this code only runs for windowless
> otherwise all the bitmaps are allocated needlessly.
> 
> http://codereview.chromium.org/119200/diff/12/1013
> File webkit/glue/webplugin_delegate.h (left):
> 
> http://codereview.chromium.org/119200/diff/12/1013#oldcode104
> Line 104: virtual void FlushGeometryUpdates() = 0;
> This should also be removed from webplugin_delegate_impl.h
> 
> http://codereview.chromium.org/119200/diff/12/1015
> File webkit/glue/webplugin_impl.cc (right):
> 
> http://codereview.chromium.org/119200/diff/12/1015#newcode692
> Line 692: &WebPluginImpl::OnDownloadPluginSrcUrl), 0);
> I'd first try to see if HandleUrlRequestInternal can be called after the
> Initialize() call that ends up calling NPP_New, to match other browsers.  If
it
> still causes issues, then you can do a post delayed task from there. 
> first_geometry_update_ is unnecessary.

I also found that calling HandleURLRequestInternal from
WebPluginImpl::setFrameRect does cause the Flash plugin to get into this wierd
state, where it does not peek. Debugging the plugin process revealed that it
receives the setFrameGeometry with width 1 and receives data after that. The
valid setFrameGeometry comes in later but causes Flash to get into this state.
Executing the handleURLRequest in a delayed task works consistently on my setup.

Thanks
Ananta

[jam, 2009-06-09 01:14:08]

lgtm