Another attempt at landing this patch.

The reliability tests regressions caused by this patch have been addressed by the upstream bug fix https://bugs.webkit.org/show_bug.cgi?id=27769

The IPCs for carrying data requested by plugins have been changed from synchronous IPCs to asynchronous IPCs.

This fixes bug http://code.google.com/p/chromium/issues/detail?id=14323,
where the Flash plugin would not render content on the page if these IPCs were processed while the plugin waited for sync calls like NPN_Evaluate to return.

This CL also fixes the following bugs, which were crashes in reliability test runs when this patch was landed last time. 

http://code.google.com/p/chromium/issues/detail?id=18058
http://code.google.com/p/chromium/issues/detail?id=18059

The crash happens because of NPP_Write calls issued to the plugin while it is waiting for an NPN_Invoke call to return in the context of NPP_NewStream. Inspecting the safari plugin implementation revealed that they defer the resource load before calling the plugin and restore it on return.

We emulate this behavior via an IPC sent from the plugin which serves as an acknowledgement.

Test=covered by UI tests.

Bug=14323,18058,18059

Review URL: http://codereview.chromium.org/159746

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@22369 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 166 insertions, 37 deletions

diff --git a/webkit/glue/plugins/plugin_instance.cc b/webkit/glue/plugins/plugin_instance.cc
index d980e10..79b5b4f 100644
--- a/webkit/glue/plugins/plugin_instance.cc
+++ b/webkit/glue/plugins/plugin_instance.cc
@@ -373,12 +373,10 @@ void PluginInstance::DidReceiveManualResponse(const std::string& url,
     response_url = instance_url_.spec();
   }
 
-  bool cancel = false;
-
   plugin_data_stream_ = CreateStream(-1, url, mime_type, false, NULL);
 
   plugin_data_stream_->DidReceiveResponse(mime_type, headers, expected_length,
-                                          last_modified, true, &cancel);
+                                          last_modified, true);
 }
 
 void PluginInstance::DidReceiveManualData(const char* buffer, int length) {
diff --git a/webkit/glue/plugins/plugin_stream_url.cc b/webkit/glue/plugins/plugin_stream_url.cc
index b050411..2aa2f84 100644
--- a/webkit/glue/plugins/plugin_stream_url.cc
+++ b/webkit/glue/plugins/plugin_stream_url.cc
@@ -44,16 +44,18 @@ void PluginStreamUrl::DidReceiveResponse(const std::string& mime_type,
                                          const std::string& headers,
                                          uint32 expected_length,
                                          uint32 last_modified,
-                                         bool request_is_seekable,
-                                         bool* cancel) {
+                                         bool request_is_seekable) {
   bool opened = Open(mime_type,
                      headers,
                      expected_length,
                      last_modified,
                      request_is_seekable);
   if (!opened) {
+    CancelRequest();
     instance()->RemoveStream(this);
-    *cancel = true;
+  } else {
+    if (id_ > 0)
+      instance()->webplugin()->SetDeferResourceLoading(id_, false);
   }
 }
 
@@ -62,8 +64,11 @@ void PluginStreamUrl::DidReceiveData(const char* buffer, int length,
   if (!open())
     return;
 
-  if (length > 0)
+  if (length > 0) {
     Write(const_cast<char*>(buffer), length, data_offset);
+    if (id_ > 0)
+      instance()->webplugin()->SetDeferResourceLoading(id_, false);
+  }
 }
 
 void PluginStreamUrl::DidFinishLoading() {
diff --git a/webkit/glue/plugins/plugin_stream_url.h b/webkit/glue/plugins/plugin_stream_url.h
index db5d4a5..1f5fe57 100644
--- a/webkit/glue/plugins/plugin_stream_url.h
+++ b/webkit/glue/plugins/plugin_stream_url.h
@@ -48,8 +48,7 @@ class PluginStreamUrl : public PluginStream,
                           const std::string& headers,
                           uint32 expected_length,
                           uint32 last_modified,
-                          bool request_is_seekable,
-                          bool* cancel);
+                          bool request_is_seekable);
   void DidReceiveData(const char* buffer, int length, int data_offset);
   void DidFinishLoading();
   void DidFail();
diff --git a/webkit/glue/plugins/test/plugin_get_javascript_url_test.cc b/webkit/glue/plugins/test/plugin_get_javascript_url_test.cc
index 74d01f1..ebdd745 100644
--- a/webkit/glue/plugins/test/plugin_get_javascript_url_test.cc
+++ b/webkit/glue/plugins/test/plugin_get_javascript_url_test.cc
@@ -17,11 +17,19 @@
 // The maximum chunk size of stream data.
 #define STREAM_CHUNK 197
 
+const int kNPNEvaluateTimerID = 100;
+const int kNPNEvaluateTimerElapse = 50;
+
+
 namespace NPAPIClient {
 
 ExecuteGetJavascriptUrlTest::ExecuteGetJavascriptUrlTest(NPP id, NPNetscapeFuncs *host_functions)
   : PluginTest(id, host_functions),
-    test_started_(false) {
+    test_started_(false),
+#ifdef OS_WIN
+    window_(NULL),
+#endif
+    npn_evaluate_context_(false) {
 }
 
 NPError ExecuteGetJavascriptUrlTest::SetWindow(NPWindow* pNPWindow) {
@@ -30,15 +38,64 @@ NPError ExecuteGetJavascriptUrlTest::SetWindow(NPWindow* pNPWindow) {
     HostFunctions()->geturlnotify(id(), url.c_str(), "_top",
                                   reinterpret_cast<void*>(SELF_URL_STREAM_ID));
     test_started_ = true;
+
+#ifdef OS_WIN
+    HWND window_handle = reinterpret_cast<HWND>(pNPWindow->window);
+    if (!::GetProp(window_handle, L"Plugin_Instance")) {
+      ::SetProp(window_handle, L"Plugin_Instance", this);
+      // We attempt to retreive the NPObject for the plugin instance identified
+      // by the NPObjectLifetimeTestInstance2 class as it may not have been
+      // instantiated yet.
+      SetTimer(window_handle, kNPNEvaluateTimerID, kNPNEvaluateTimerElapse,
+               TimerProc);
+    }
+    window_ = window_handle;
+#endif
   }
+
   return NPERR_NO_ERROR;
 }
 
+#ifdef OS_WIN
+void CALLBACK ExecuteGetJavascriptUrlTest::TimerProc(
+    HWND window, UINT message, UINT timer_id, unsigned long elapsed_time) {
+  ExecuteGetJavascriptUrlTest* this_instance =
+      reinterpret_cast<ExecuteGetJavascriptUrlTest*>
+          (::GetProp(window, L"Plugin_Instance"));
+
+  NPObject *window_obj = NULL;
+  this_instance->HostFunctions()->getvalue(this_instance->id(),
+                                           NPNVWindowNPObject,
+                                           &window_obj);
+  if (!window_obj) {
+    this_instance->SetError("Failed to get NPObject for plugin instance2");
+    this_instance->SignalTestCompleted();
+    return;
+  }
+
+  std::string script = "javascript:window.location";
+  NPString script_string;
+  script_string.UTF8Characters = script.c_str();
+  script_string.UTF8Length = static_cast<unsigned int>(script.length());
+  NPVariant result_var;
+
+  this_instance->npn_evaluate_context_ = true;
+  NPError result = this_instance->HostFunctions()->evaluate(
+      this_instance->id(), window_obj, &script_string, &result_var);
+  this_instance->npn_evaluate_context_ = false;
+}
+#endif
+
 NPError ExecuteGetJavascriptUrlTest::NewStream(NPMIMEType type, NPStream* stream,
                               NPBool seekable, uint16* stype) {
   if (stream == NULL)
     SetError("NewStream got null stream");
 
+  if (npn_evaluate_context_) {
+    SetError("NewStream received in context of NPN_Evaluate");
+    return NPERR_NO_ERROR;
+  }
+
   COMPILE_ASSERT(sizeof(unsigned long) <= sizeof(stream->notifyData),
                  cast_validity_check);
   unsigned long stream_id = reinterpret_cast<unsigned long>(stream->notifyData);
@@ -53,6 +110,10 @@ NPError ExecuteGetJavascriptUrlTest::NewStream(NPMIMEType type, NPStream* stream
 }
 
 int32 ExecuteGetJavascriptUrlTest::WriteReady(NPStream *stream) {
+  if (npn_evaluate_context_) {
+    SetError("WriteReady received in context of NPN_Evaluate");
+    return NPERR_NO_ERROR;
+  }
   return STREAM_CHUNK;
 }
 
@@ -63,6 +124,11 @@ int32 ExecuteGetJavascriptUrlTest::Write(NPStream *stream, int32 offset, int32 l
   if (len < 0 || len > STREAM_CHUNK)
     SetError("Write got bogus stream chunk size");
 
+  if (npn_evaluate_context_) {
+    SetError("Write received in context of NPN_Evaluate");
+    return len;
+  }
+
   COMPILE_ASSERT(sizeof(unsigned long) <= sizeof(stream->notifyData),
                  cast_validity_check);
   unsigned long stream_id = reinterpret_cast<unsigned long>(stream->notifyData);
@@ -83,6 +149,15 @@ NPError ExecuteGetJavascriptUrlTest::DestroyStream(NPStream *stream, NPError rea
   if (stream == NULL)
     SetError("NewStream got null stream");
 
+#ifdef OS_WIN
+  KillTimer(window_, kNPNEvaluateTimerID);
+#endif
+
+  if (npn_evaluate_context_) {
+    SetError("DestroyStream received in context of NPN_Evaluate");
+    return NPERR_NO_ERROR;
+  }
+
   COMPILE_ASSERT(sizeof(unsigned long) <= sizeof(stream->notifyData),
                  cast_validity_check);
   unsigned long stream_id = reinterpret_cast<unsigned long>(stream->notifyData);
@@ -100,6 +175,12 @@ NPError ExecuteGetJavascriptUrlTest::DestroyStream(NPStream *stream, NPError rea
 void ExecuteGetJavascriptUrlTest::URLNotify(const char* url, NPReason reason, void* data) {
   COMPILE_ASSERT(sizeof(unsigned long) <= sizeof(data),
                  cast_validity_check);
+
+  if (npn_evaluate_context_) {
+    SetError("URLNotify received in context of NPN_Evaluate");
+    return;
+  }
+
   unsigned long stream_id = reinterpret_cast<unsigned long>(data);
   switch (stream_id) {
     case SELF_URL_STREAM_ID:
diff --git a/webkit/glue/plugins/test/plugin_get_javascript_url_test.h b/webkit/glue/plugins/test/plugin_get_javascript_url_test.h
index cff6775..5c2540d 100644
--- a/webkit/glue/plugins/test/plugin_get_javascript_url_test.h
+++ b/webkit/glue/plugins/test/plugin_get_javascript_url_test.h
@@ -28,8 +28,18 @@ class ExecuteGetJavascriptUrlTest : public PluginTest {
   virtual void    URLNotify(const char* url, NPReason reason, void* data);
 
  private:
+#if defined(OS_WIN)
+  static void CALLBACK TimerProc(HWND window, UINT message, UINT timer_id,
+                                 unsigned long elapsed_time);
+#endif
   bool test_started_;
+  // This flag is set to true in the context of the NPN_Evaluate call.
+  bool npn_evaluate_context_;
   std::string self_url_;
+
+#if defined(OS_WIN)
+  HWND window_;
+#endif
 };
 
 } // namespace NPAPIClient
diff --git a/webkit/glue/webplugin.h b/webkit/glue/webplugin.h
index 07d2470..074549a 100644
--- a/webkit/glue/webplugin.h
+++ b/webkit/glue/webplugin.h
@@ -131,6 +131,10 @@ class WebPlugin {
   virtual void ResourceClientDeleted(
       WebPluginResourceClient* resource_client) {}
 
+  // Defers the loading of the resource identified by resource_id. This is
+  // controlled by the defer parameter.
+  virtual void SetDeferResourceLoading(int resource_id, bool defer) = 0;
+
  private:
   DISALLOW_EVIL_CONSTRUCTORS(WebPlugin);
 };
@@ -146,8 +150,7 @@ class WebPluginResourceClient {
                                   const std::string& headers,
                                   uint32 expected_length,
                                   uint32 last_modified,
-                                  bool request_is_seekable,
-                                  bool* cancel) = 0;
+                                  bool request_is_seekable) = 0;
   virtual void DidReceiveData(const char* buffer, int length,
                               int data_offset) = 0;
   virtual void DidFinishLoading() = 0;
diff --git a/webkit/glue/webplugin_impl.cc b/webkit/glue/webplugin_impl.cc
index 826bfb9..0d7524b 100644
--- a/webkit/glue/webplugin_impl.cc
+++ b/webkit/glue/webplugin_impl.cc
@@ -119,6 +119,9 @@ class MultiPartResponseClient : public WebURLLoaderClient {
   // Receives individual part data from a multipart response.
   virtual void didReceiveData(
       WebURLLoader*, const char* data, int data_size, long long) {
+    // TODO(ananta)
+    // We should defer further loads on multipart resources on the same lines
+    // as regular resources requested by plugins to prevent reentrancy.
     resource_client_->DidReceiveData(
         data, data_size, byte_range_lower_bound_);
   }
@@ -475,6 +478,7 @@ void WebPluginImpl::CancelResource(int id) {
   for (size_t i = 0; i < clients_.size(); ++i) {
     if (clients_[i].id == id) {
       if (clients_[i].loader.get()) {
+        clients_[i].loader->setDefersLoading(false);
         clients_[i].loader->cancel();
         RemoveClient(i);
       }
@@ -911,16 +915,23 @@ NPObject* WebPluginImpl::GetPluginScriptableObject() {
 
 WebPluginResourceClient* WebPluginImpl::GetClientFromLoader(
     WebURLLoader* loader) {
+  ClientInfo* client_info = GetClientInfoFromLoader(loader);
+  if (client_info)
+    return client_info->client;
+  return NULL;
+}
+
+WebPluginImpl::ClientInfo* WebPluginImpl::GetClientInfoFromLoader(
+    WebURLLoader* loader) {
   for (size_t i = 0; i < clients_.size(); ++i) {
     if (clients_[i].loader.get() == loader)
-      return clients_[i].client;
+      return &clients_[i];
   }
 
   NOTREACHED();
   return 0;
 }
 
-
 void WebPluginImpl::willSendRequest(WebURLLoader* loader,
                                     WebURLRequest& request,
                                     const WebURLResponse&) {
@@ -995,17 +1006,17 @@ void WebPluginImpl::didReceiveResponse(WebURLLoader* loader,
     }
   }
 
+  // Calling into a plugin could result in reentrancy if the plugin yields
+  // control to the OS like entering a modal loop etc. Prevent this by
+  // stopping further loading until the plugin notifies us that it is ready to
+  // accept data
+  loader->setDefersLoading(true);
+
   client->DidReceiveResponse(
       base::SysWideToNativeMB(http_response_info.mime_type),
       base::SysWideToNativeMB(GetAllHeaders(resource_response)),
       http_response_info.expected_length,
-      http_response_info.last_modified, request_is_seekable, &cancel);
-
-  if (cancel) {
-    loader->cancel();
-    RemoveClient(loader);
-    return;
-  }
+      http_response_info.last_modified, request_is_seekable);
 
   // Bug http://b/issue?id=925559. The flash plugin would not handle the HTTP
   // error codes in the stream header and as a result, was unaware of the
@@ -1041,15 +1052,16 @@ void WebPluginImpl::didReceiveData(WebURLLoader* loader,
     DCHECK(multi_part_handler != NULL);
     multi_part_handler->OnReceivedData(buffer, length);
   } else {
+    loader->setDefersLoading(true);
     client->DidReceiveData(buffer, length, 0);
   }
 }
 
 void WebPluginImpl::didFinishLoading(WebURLLoader* loader) {
-  WebPluginResourceClient* client = GetClientFromLoader(loader);
-  if (client) {
+  ClientInfo* client_info = GetClientInfoFromLoader(loader);
+  if (client_info && client_info->client) {
     MultiPartResponseHandlerMap::iterator index =
-        multi_part_response_map_.find(client);
+      multi_part_response_map_.find(client_info->client);
     if (index != multi_part_response_map_.end()) {
       delete (*index).second;
       multi_part_response_map_.erase(index);
@@ -1057,19 +1069,24 @@ void WebPluginImpl::didFinishLoading(WebURLLoader* loader) {
       WebView* web_view = webframe_->GetView();
       web_view->GetDelegate()->DidStopLoading(web_view);
     }
-    client->DidFinishLoading();
+    loader->setDefersLoading(true);
+    client_info->client->DidFinishLoading();
+    // The WebPluginResourceClient pointer gets deleted soon after a call to
+    // DidFinishLoading.
+    client_info->client = NULL;
   }
-
-  RemoveClient(loader);
 }
 
 void WebPluginImpl::didFail(WebURLLoader* loader,
                             const WebURLError&) {
-  WebPluginResourceClient* client = GetClientFromLoader(loader);
-  if (client)
-    client->DidFail();
-
-  RemoveClient(loader);
+  ClientInfo* client_info = GetClientInfoFromLoader(loader);
+  if (client_info && client_info->client) {
+    loader->setDefersLoading(true);
+    client_info->client->DidFail();
+    // The WebPluginResourceClient pointer gets deleted soon after a call to
+    // DidFinishLoading.
+    client_info->client = NULL;
+  }
 }
 
 void WebPluginImpl::RemoveClient(size_t i) {
@@ -1286,9 +1303,21 @@ void WebPluginImpl::InitiateHTTPRangeRequest(const char* url,
                       GURL(complete_url_string), range_info, true);
 }
 
+void WebPluginImpl::SetDeferResourceLoading(int resource_id, bool defer) {
+  std::vector<ClientInfo>::iterator client_index = clients_.begin();
+  while (client_index != clients_.end()) {
+    ClientInfo& client_info = *client_index;
+
+    if (client_info.id == resource_id) {
+      client_info.loader->setDefersLoading(defer);
+      break;
+    }
+    client_index++;
+  }
+}
+
 void WebPluginImpl::HandleHttpMultipartResponse(
-    const WebURLResponse& response,
-    WebPluginResourceClient* client) {
+    const WebURLResponse& response, WebPluginResourceClient* client) {
   std::string multipart_boundary;
   if (!MultipartResponseDelegate::ReadMultipartBoundary(
           response, &multipart_boundary)) {
diff --git a/webkit/glue/webplugin_impl.h b/webkit/glue/webplugin_impl.h
index eb8ada0..04d0e89 100644
--- a/webkit/glue/webplugin_impl.h
+++ b/webkit/glue/webplugin_impl.h
@@ -41,6 +41,7 @@ class Widget;
 
 namespace WebKit {
 class WebURLResponse;
+class WebURLLoader;
 }
 
 namespace webkit_glue {
@@ -264,9 +265,6 @@ class WebPluginImpl : public WebPlugin,
   virtual void didFinishLoading(WebKit::WebURLLoader* loader);
   virtual void didFail(WebKit::WebURLLoader* loader, const WebKit::WebURLError&);
 
-  // Helper function
-  WebPluginResourceClient* GetClientFromLoader(WebKit::WebURLLoader* loader);
-
   // Helper function to remove the stored information about a resource
   // request given its index in m_clients.
   void RemoveClient(size_t i);
@@ -297,6 +295,8 @@ class WebPluginImpl : public WebPlugin,
                                 intptr_t existing_stream, bool notify_needed,
                                 intptr_t notify_data);
 
+  void SetDeferResourceLoading(int resource_id, bool defer);
+
   // Ignore in-process plugins mode for this flag.
   bool IsOffTheRecord() { return false; }
 
@@ -333,6 +333,10 @@ class WebPluginImpl : public WebPlugin,
     linked_ptr<WebKit::WebURLLoader> loader;
   };
 
+  // Helper functions
+  WebPluginResourceClient* GetClientFromLoader(WebKit::WebURLLoader* loader);
+  ClientInfo* GetClientInfoFromLoader(WebKit::WebURLLoader* loader);
+
   std::vector<ClientInfo> clients_;
 
   bool windowless_;