Fixes a crash caused due to a call to NPP_DestroyStream occuring in the conte...

Fixes a crash caused due to a call to NPP_DestroyStream occuring in the context of NPP_NewStream. 
The plugin would invoke NPN_Evaluate to display an alert in the context of NewStream. This would cause the didFail IPC to be dispatched which would cause the plugin to invoke another call to NPP_NewStream which would repeat these steps and crash.

The didFail call from the renderer did not honor the deferred load flag which we set in WebPluginImpl
prior to dispatching stream IPCs to the plugin. 

Fix is to dispatch the didFail call when we receive an ack from the plugin indicating that it is ready to accept stream data.

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

The other change is in WebPluginImpl::TearDownPluginInstance, where we run through the list of resource
clients and cancel them. We call didFail on these clients here, which occurs anyway through the PluginDestroyed
code path.


Bug=20063
Test=Convered by interactive UI test.



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

[jam, 2009-08-25 02:01:34]

On Mon, Aug 24, 2009 at 6:50 PM, <ananta@chromium.org> wrote:

> Reviewers: John Abd-El-Malek,
>
> Description:
> Fixes a crash caused due to a call to NPP_DestroyStream occuring in the
> context of NPP_NewStream.
> The plugin would invoke NPN_Evaluate to display an alert in the context
> of NewStream.
> This would cause the didFail IPC to be dispatched which would cause the
> plugin to invoke another
> call to NPP_NewStream which would repeat these steps and crash.
>
> The didFail call from the renderer did not honor the deferred load flag
> which we set in WebPluginImpl
> prior to dispatching stream IPCs to the plugin.
>

Is it possible to fix this upstream in WebKit by not calling didFail if
deferred loading is set?


>
> Fix is to dispatch the didFail call when we receive an ack from the
> plugin indicating that it
> is ready to accept stream data.
>
> This fixes bug http://code.google.com/p/chromium/issues/detail?id=20063
>
> The other change is in WebPluginImpl::TearDownPluginInstance, where we
> run through the list of resource
> clients and cancel them. We call didFail on these clients here, which
> occurs anyway through the PluginDestroyed
> code path.
>
> We would need a UI test for the DestroyStream issue. To achieve this we
> would need to issue geturlnotify URL
> requests to a webserver which would return 404 errors. Will look into
> this and add it in a subsequent CL.
>
> Bug=20063
>
>
>
> Please review this at http://codereview.chromium.org/174383
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     webkit/glue/webplugin_impl.h
>  M     webkit/glue/webplugin_impl.cc
>
>
> Index: webkit/glue/webplugin_impl.h
> ===================================================================
> --- webkit/glue/webplugin_impl.h        (revision 24128)
> +++ webkit/glue/webplugin_impl.h        (working copy)
> @@ -241,6 +241,7 @@
>     int id;
>     WebPluginResourceClient* client;
>     WebKit::WebURLRequest request;
> +    bool pending_failure_notification;
>     linked_ptr<WebKit::WebURLLoader> loader;
>   };
>
> Index: webkit/glue/webplugin_impl.cc
> ===================================================================
> --- webkit/glue/webplugin_impl.cc       (revision 24128)
> +++ webkit/glue/webplugin_impl.cc       (working copy)
> @@ -771,11 +771,9 @@
>       // The plugin instance could be in the process of deletion here.
>       // Verify if the WebPluginResourceClient instance still exists before
>       // use.
> -      WebPluginResourceClient* resource_client =
> GetClientFromLoader(loader);
> -      if (resource_client) {
> -        loader->cancel();
> -        resource_client->DidFail();
> -        RemoveClient(loader);
> +      ClientInfo* client_info = GetClientInfoFromLoader(loader);
> +      if (client_info) {
> +        client_info->pending_failure_notification = true;
>       }
>     }
>   }
> @@ -944,6 +942,7 @@
>   info.request.setRequestorProcessID(delegate_->GetProcessId());
>   info.request.setTargetType(WebURLRequest::TargetIsObject);
>   info.request.setHTTPMethod(WebString::fromUTF8(method));
> +  info.pending_failure_notification = false;
>
>   if (range_info) {
>     info.request.addHTTPHeaderField(WebString::fromUTF8("Range"),
> @@ -1004,6 +1003,17 @@
>
>     if (client_info.id == resource_id) {
>       client_info.loader->setDefersLoading(defer);
> +
> +      // If we determined that the request had failed via the HTTP headers
> +      // in the response then we send out a failure notification to the
> +      // plugin process, as certain plugins don't handle HTTP failure
> codes
> +      // correctly.
> +      if (!defer && client_info.client &&
> +          client_info.pending_failure_notification) {
> +        client_info.loader->cancel();
> +        client_info.client->DidFail();
> +        clients_.erase(client_index++);
> +      }
>       break;
>     }
>     client_index++;
> @@ -1131,10 +1141,7 @@
>     if (client_info.loader.get())
>       client_info.loader->cancel();
>
> -    WebPluginResourceClient* resource_client = client_info.client;
>     client_index = clients_.erase(client_index);
> -    if (resource_client)
> -      resource_client->DidFail();
>   }
>
>   // This needs to be called now and not in the destructor since the
>
>
>

[ananta, 2009-08-25 02:12:32]

Webkit's plugin implementation does not have this bug, i.e. it does not call
didFail when deferred load flag is set. The sequence in webkit is as below:-
1. First PluginStream::didReceiveResponse is called which sets and resets the
deferred loading flag.
2. It then checks the http status code and calls didCancel which calls
destroyStream which internally sets  and resets the deferred loading flag.

[iyengar, 2009-08-25 03:28:13]

Just to clear things up, FF and Safari both exhibit this behavior. In Webkit
thisis implemented by the NetscapePluginLoader class which handles
didReceiveResponse
and after calling the plugin, checks the http headers for whether the
request failed.
If yes it calls didCancel which calls destroyStream on the PluginStream
object.

Thanks
Ananta

On Mon, Aug 24, 2009 at 7:12 PM, <ananta@chromium.org> wrote:

> Webkit's plugin implementation does not have this bug, i.e. it does not
> call didFail when deferred load flag is set. The sequence in webkit is
> as below:-
> 1. First PluginStream::didReceiveResponse is called which sets and
> resets the deferred loading flag.
> 2. It then checks the http status code and calls didCancel which calls
> destroyStream which internally sets  and resets the deferred loading
> flag.
>
>
>
>
>
> http://codereview.chromium.org/174383
>

[darin (slow to review), 2009-08-25 16:18:53]

On Mon, Aug 24, 2009 at 6:50 PM, <ananta@chromium.org> wrote:

> Reviewers: John Abd-El-Malek,
>
> Description:
> Fixes a crash caused due to a call to NPP_DestroyStream occuring in the
> context of NPP_NewStream.
> The plugin would invoke NPN_Evaluate to display an alert in the context
> of NewStream.
> This would cause the didFail IPC to be dispatched which would cause the
> plugin to invoke another
> call to NPP_NewStream which would repeat these steps and crash.
>
> The didFail call from the renderer did not honor the deferred load flag
> which we set in WebPluginImpl
> prior to dispatching stream IPCs to the plugin.
>
> Fix is to dispatch the didFail call when we receive an ack from the
> plugin indicating that it
> is ready to accept stream data.
>
> This fixes bug http://code.google.com/p/chromium/issues/detail?id=20063
>
> The other change is in WebPluginImpl::TearDownPluginInstance, where we
> run through the list of resource
> clients and cancel them. We call didFail on these clients here, which
> occurs anyway through the PluginDestroyed
> code path.
>
> We would need a UI test for the DestroyStream issue. To achieve this we
> would need to issue geturlnotify URL
> requests to a webserver which would return 404 errors. Will look into
> this and add it in a subsequent CL.
>

It is easy to generate a 404 response by using the URLRequestMockHttpJob.
You can see examples of that in action in errorpage_uitest.cc

-Darin




>
> Bug=20063
>
>
>
> Please review this at http://codereview.chromium.org/174383
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     webkit/glue/webplugin_impl.h
>  M     webkit/glue/webplugin_impl.cc
>
>
> Index: webkit/glue/webplugin_impl.h
> ===================================================================
> --- webkit/glue/webplugin_impl.h        (revision 24128)
> +++ webkit/glue/webplugin_impl.h        (working copy)
> @@ -241,6 +241,7 @@
>     int id;
>     WebPluginResourceClient* client;
>     WebKit::WebURLRequest request;
> +    bool pending_failure_notification;
>     linked_ptr<WebKit::WebURLLoader> loader;
>   };
>
> Index: webkit/glue/webplugin_impl.cc
> ===================================================================
> --- webkit/glue/webplugin_impl.cc       (revision 24128)
> +++ webkit/glue/webplugin_impl.cc       (working copy)
> @@ -771,11 +771,9 @@
>       // The plugin instance could be in the process of deletion here.
>       // Verify if the WebPluginResourceClient instance still exists before
>       // use.
> -      WebPluginResourceClient* resource_client =
> GetClientFromLoader(loader);
> -      if (resource_client) {
> -        loader->cancel();
> -        resource_client->DidFail();
> -        RemoveClient(loader);
> +      ClientInfo* client_info = GetClientInfoFromLoader(loader);
> +      if (client_info) {
> +        client_info->pending_failure_notification = true;
>       }
>     }
>   }
> @@ -944,6 +942,7 @@
>   info.request.setRequestorProcessID(delegate_->GetProcessId());
>   info.request.setTargetType(WebURLRequest::TargetIsObject);
>   info.request.setHTTPMethod(WebString::fromUTF8(method));
> +  info.pending_failure_notification = false;
>
>   if (range_info) {
>     info.request.addHTTPHeaderField(WebString::fromUTF8("Range"),
> @@ -1004,6 +1003,17 @@
>
>     if (client_info.id == resource_id) {
>       client_info.loader->setDefersLoading(defer);
> +
> +      // If we determined that the request had failed via the HTTP headers
> +      // in the response then we send out a failure notification to the
> +      // plugin process, as certain plugins don't handle HTTP failure
> codes
> +      // correctly.
> +      if (!defer && client_info.client &&
> +          client_info.pending_failure_notification) {
> +        client_info.loader->cancel();
> +        client_info.client->DidFail();
> +        clients_.erase(client_index++);
> +      }
>       break;
>     }
>     client_index++;
> @@ -1131,10 +1141,7 @@
>     if (client_info.loader.get())
>       client_info.loader->cancel();
>
> -    WebPluginResourceClient* resource_client = client_info.client;
>     client_index = clients_.erase(client_index);
> -    if (resource_client)
> -      resource_client->DidFail();
>   }
>
>   // This needs to be called now and not in the destructor since the
>
>
>

[ananta, 2009-08-26 18:27:25]

The CL has been updated with a UI test which validates this case.

-Ananta

[jam, 2009-08-26 22:44:45]

lgtm

http://codereview.chromium.org/174383/diff/1058/1067
File chrome/test/interactive_ui/npapi_interactive_test.cc (right):

http://codereview.chromium.org/174383/diff/1058/1067#newcode92
Line 92: automation()->WaitForAppModalDialog(5000);
perhaps use action_max_timeout_ms()?  You need to use timeouts that differ when
running on purify, since things are much slower there.

http://codereview.chromium.org/174383/diff/1058/1064
File webkit/glue/plugins/test/plugin_geturl_test.cc (right):

http://codereview.chromium.org/174383/diff/1058/1064#newcode40
Line 40: 
nit: extra line

[ananta, 2009-08-26 22:58:45]

http://codereview.chromium.org/174383/diff/1058/1067
File chrome/test/interactive_ui/npapi_interactive_test.cc (right):

http://codereview.chromium.org/174383/diff/1058/1067#newcode92
Line 92: automation()->WaitForAppModalDialog(5000);
On 2009/08/26 22:44:46, John Abd-El-Malek wrote:
> perhaps use action_max_timeout_ms()?  You need to use timeouts that differ
when
> running on purify, since things are much slower there.

Done.

http://codereview.chromium.org/174383/diff/1058/1064
File webkit/glue/plugins/test/plugin_geturl_test.cc (right):

http://codereview.chromium.org/174383/diff/1058/1064#newcode40
Line 40: 
On 2009/08/26 22:44:46, John Abd-El-Malek wrote:
> nit: extra line
Removed