Revert 71334 - Fix Pepper URL Loader callbacks.

[Not applying the "obvious" fix, since it might lead to callbacks being called
incorrectly.]

Add some tests (for aborting calls).

BUG=none
TEST=ppapi_tests

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

TBR=viettrungluu@chromium.org,jam@chromium.org,brettw@chromium.org
Review URL: http://codereview.chromium.org/6254003

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

ppapi/tests/test_file_io.cc

@@ -484,10 +484,5 @@ std::string TestFileIO::TestAbortCalls() {
     }
   }
 
-  // TODO(viettrungluu): Also test that Close() aborts callbacks.
-  // crbug.com/69457
-
   PASS();
 }
-
-// TODO(viettrungluu): Test Close(). crbug.com/69457

ppapi/tests/test_url_loader.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ppapi/tests/test_url_loader.h"
 
 #include <stdio.h>
-#include <string.h>
 #include <string>
 
 #include "ppapi/c/dev/ppb_file_io_dev.h"
@@ -51,7 +50,6 @@ void TestURLLoader::RunTest() {
   RUN_TEST(SameOriginRestriction);
   RUN_TEST(StreamToFile);
   RUN_TEST(AuditURLRedirect);
-  RUN_TEST(AbortCalls);
 }
 
 std::string TestURLLoader::ReadEntireFile(pp::FileIO_Dev* file_io,
@@ -72,13 +70,13 @@ std::string TestURLLoader::ReadEntireFile(pp::FileIO_Dev* file_io,
     data->append(buf, rv);
   }
 
-  PASS();
+  return "";
 }
 
 std::string TestURLLoader::ReadEntireResponseBody(pp::URLLoader* loader,
                                                   std::string* body) {
   TestCompletionCallback callback;
-  char buf[2];  // Small so that multiple reads are needed.
+  char buf[256];
 
   for (;;) {
     int32_t rv = loader->ReadResponseBody(buf, sizeof(buf), callback);
@@ -91,7 +89,7 @@ std::string TestURLLoader::ReadEntireResponseBody(pp::URLLoader* loader,
     body->append(buf, rv);
   }
 
-  PASS();
+  return "";
 }
 
 std::string TestURLLoader::LoadAndCompareBody(
@@ -123,7 +121,7 @@ std::string TestURLLoader::LoadAndCompareBody(
   if (body != expected_body)
     return "URLLoader::ReadResponseBody returned unexpected content";
 
-  PASS();
+  return "";
 }
 
 std::string TestURLLoader::TestBasicGET() {
@@ -298,66 +296,7 @@ std::string TestURLLoader::TestAuditURLRedirect() {
   if (response_info.GetRedirectURL().AsString() != "www.google.com")
     return "Redirect URL should be www.google.com";
 
-  PASS();
-}
-
-std::string TestURLLoader::TestAbortCalls() {
-  pp::URLRequestInfo request;
-  request.SetURL("test_url_loader_data/hello.txt");
-
-  TestCompletionCallback callback;
-  int32_t rv;
-
-  // Abort |Open()|.
-  {
-    callback.reset_run_count();
-    rv = pp::URLLoader(*instance_).Open(request, callback);
-    if (callback.run_count() > 0)
-      return "URLLoader::Open ran callback synchronously.";
-    if (rv == PP_ERROR_WOULDBLOCK) {
-      rv = callback.WaitForResult();
-      if (rv != PP_ERROR_ABORTED)
-        return "URLLoader::Open not aborted.";
-    } else if (rv != PP_OK) {
-      return ReportError("URLLoader::Open", rv);
-    }
-  }
-
-  // Abort |ReadResponseBody()|.
-  {
-    char buf[2] = { 0 };
-    {
-      pp::URLLoader loader(*instance_);
-      rv = loader.Open(request, callback);
-      if (rv == PP_ERROR_WOULDBLOCK)
-        rv = callback.WaitForResult();
-      if (rv != PP_OK)
-        return ReportError("URLLoader::Open", rv);
-
-      callback.reset_run_count();
-      rv = loader.ReadResponseBody(buf, sizeof(buf), callback);
-    }  // Destroy |loader|.
-    if (rv == PP_ERROR_WOULDBLOCK) {
-      // Save a copy and make sure |buf| doesn't get written to.
-      char buf_copy[2];
-      memcpy(&buf_copy, &buf, sizeof(buf));
-      rv = callback.WaitForResult();
-      if (rv != PP_ERROR_ABORTED)
-        return "URLLoader::ReadResponseBody not aborted.";
-      if (memcmp(&buf_copy, &buf, sizeof(buf)) != 0)
-        return "URLLoader::ReadResponseBody wrote data after resource "
-               "destruction.";
-    } else if (rv != PP_OK) {
-      return ReportError("URLLoader::ReadResponseBody", rv);
-    }
-  }
-
-  // TODO(viettrungluu): More abort tests (but add basic tests first).
-  // Also test that Close() aborts properly. crbug.com/69457
-
-  PASS();
+  return "";
 }
 
-// TODO(viettrungluu): Add tests for FollowRedirect,
-// Get{Upload,Download}Progress, Close (including abort tests if applicable).
 // TODO(darin): Add a test for GrantUniversalAccess.

ppapi/tests/test_url_loader.h

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -42,7 +42,6 @@ class TestURLLoader : public TestCase {
   std::string TestStreamToFile();
   std::string TestSameOriginRestriction();
   std::string TestAuditURLRedirect();
-  std::string TestAbortCalls();
 
   const PPB_FileIOTrusted_Dev* file_io_trusted_interface_;
 };

webkit/plugins/ppapi/ppb_url_loader_impl.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -21,7 +21,6 @@
 #include "third_party/WebKit/WebKit/chromium/public/WebURLResponse.h"
 #include "webkit/appcache/web_application_cache_host_impl.h"
 #include "webkit/plugins/ppapi/common.h"
-#include "webkit/plugins/ppapi/plugin_module.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/ppb_url_request_info_impl.h"
 #include "webkit/plugins/ppapi/ppb_url_response_info_impl.h"
@@ -233,19 +232,19 @@ PPB_URLLoader_Impl* PPB_URLLoader_Impl::AsPPB_URLLoader_Impl() {
 
 int32_t PPB_URLLoader_Impl::Open(PPB_URLRequestInfo_Impl* request,
                                  PP_CompletionCallback callback) {
-  int32_t rv = ValidateCallback(callback);
-  if (rv != PP_OK)
-    return rv;
-
   if (loader_.get())
     return PP_ERROR_INPROGRESS;
 
+  // We only support non-blocking calls.
+  if (!callback.func)
+    return PP_ERROR_BADARGUMENT;
+
   WebFrame* frame = GetFrame(instance_);
   if (!frame)
     return PP_ERROR_FAILED;
   WebURLRequest web_request(request->ToWebURLRequest(frame));
 
-  rv = CanRequest(frame, web_request.url());
+  int32_t rv = CanRequest(frame, web_request.url());
   if (rv != PP_OK)
     return rv;
 
@@ -264,25 +263,28 @@ int32_t PPB_URLLoader_Impl::Open(PPB_URLRequestInfo_Impl* request,
   loader_->loadAsynchronously(web_request, this);
 
   request_info_ = scoped_refptr<PPB_URLRequestInfo_Impl>(request);
+  pending_callback_ = callback;
 
   // Notify completion when we receive a redirect or response headers.
-  RegisterCallback(callback);
   return PP_ERROR_WOULDBLOCK;
 }
 
 int32_t PPB_URLLoader_Impl::FollowRedirect(PP_CompletionCallback callback) {
-  int32_t rv = ValidateCallback(callback);
-  if (rv != PP_OK)
-    return rv;
+  if (pending_callback_.func)
+    return PP_ERROR_INPROGRESS;
+
+  // We only support non-blocking calls.
+  if (!callback.func)
+    return PP_ERROR_BADARGUMENT;
 
   WebURL redirect_url = GURL(response_info_->redirect_url());
 
-  rv = CanRequest(GetFrame(instance_), redirect_url);
+  int32_t rv = CanRequest(GetFrame(instance_), redirect_url);
   if (rv != PP_OK)
     return rv;
 
+  pending_callback_ = callback;
   loader_->setDefersLoading(false);  // Allow the redirect to continue.
-  RegisterCallback(callback);
   return PP_ERROR_WOULDBLOCK;
 }
 
@@ -314,13 +316,16 @@ bool PPB_URLLoader_Impl::GetDownloadProgress(
 int32_t PPB_URLLoader_Impl::ReadResponseBody(char* buffer,
                                              int32_t bytes_to_read,
                                              PP_CompletionCallback callback) {
-  int32_t rv = ValidateCallback(callback);
-  if (rv != PP_OK)
-    return rv;
   if (!response_info_ || response_info_->body())
     return PP_ERROR_FAILED;
   if (bytes_to_read <= 0 || !buffer)
     return PP_ERROR_BADARGUMENT;
+  if (pending_callback_.func)
+    return PP_ERROR_INPROGRESS;
+
+  // We only support non-blocking calls.
+  if (!callback.func)
+    return PP_ERROR_BADARGUMENT;
 
   user_buffer_ = buffer;
   user_buffer_size_ = bytes_to_read;
@@ -335,24 +340,23 @@ int32_t PPB_URLLoader_Impl::ReadResponseBody(char* buffer,
     return done_status_;
   }
 
-  RegisterCallback(callback);
+  pending_callback_ = callback;
   return PP_ERROR_WOULDBLOCK;
 }
 
 int32_t PPB_URLLoader_Impl::FinishStreamingToFile(
     PP_CompletionCallback callback) {
-  int32_t rv = ValidateCallback(callback);
-  if (rv != PP_OK)
-    return rv;
   if (!response_info_ || !response_info_->body())
     return PP_ERROR_FAILED;
+  if (pending_callback_.func)
+    return PP_ERROR_INPROGRESS;
 
   // We may have already reached EOF.
   if (done_status_ != PP_ERROR_WOULDBLOCK)
     return done_status_;
 
   // Wait for didFinishLoading / didFail.
-  RegisterCallback(callback);
+  pending_callback_ = callback;
   return PP_ERROR_WOULDBLOCK;
 }
 
@@ -363,8 +367,6 @@ void PPB_URLLoader_Impl::Close() {
     WebFrame* frame = instance_->container()->element().document().frame();
     frame->stopLoading();
   }
-  // TODO(viettrungluu): Check what happens to the callback (probably the
-  // wrong thing). May need to post abort here. crbug.com/69457
 }
 
 void PPB_URLLoader_Impl::GrantUniversalAccess() {
@@ -429,7 +431,7 @@ void PPB_URLLoader_Impl::didReceiveData(WebURLLoader* loader,
   if (user_buffer_) {
     RunCallback(FillUserBuffer());
   } else {
-    DCHECK(!pending_callback_.get() || pending_callback_->completed());
+    DCHECK(!pending_callback_.func);
   }
 }
 
@@ -480,31 +482,13 @@ void PPB_URLLoader_Impl::InstanceDestroyed(PluginInstance* instance) {
   // goes out of scope.
 }
 
-int32_t PPB_URLLoader_Impl::ValidateCallback(PP_CompletionCallback callback) {
-  // We only support non-blocking calls.
-  if (!callback.func)
-    return PP_ERROR_BADARGUMENT;
-
-  if (pending_callback_.get() && !pending_callback_->completed())
-    return PP_ERROR_INPROGRESS;
-
-  return PP_OK;
-}
-
-void PPB_URLLoader_Impl::RegisterCallback(PP_CompletionCallback callback) {
-  DCHECK(callback.func);
-  DCHECK(!pending_callback_.get() || pending_callback_->completed());
-
-  PP_Resource resource_id = GetReferenceNoAddRef();
-  CHECK(resource_id);
-  pending_callback_ = new TrackedCompletionCallback(
-      module()->GetCallbackTracker(), resource_id, callback);
-}
-
 void PPB_URLLoader_Impl::RunCallback(int32_t result) {
-  scoped_refptr<TrackedCompletionCallback> callback;
-  callback.swap(pending_callback_);
-  callback->Run(result);  // Will complete abortively if necessary.
+  if (!pending_callback_.func)
+    return;
+
+  PP_CompletionCallback callback = {0};
+  std::swap(callback, pending_callback_);
+  PP_RunCompletionCallback(&callback, result);
 }
 
 size_t PPB_URLLoader_Impl::FillUserBuffer() {

webkit/plugins/ppapi/ppb_url_loader_impl.h

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -7,12 +7,10 @@
 
 #include <deque>
 
-#include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/trusted/ppb_url_loader_trusted.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebURLLoaderClient.h"
-#include "webkit/plugins/ppapi/callbacks.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/resource.h"
 
@@ -91,17 +89,7 @@ class PPB_URLLoader_Impl : public Resource,
   PPB_URLResponseInfo_Impl* response_info() const { return response_info_; }
 
  private:
-  // Check that |callback| is valid (only non-blocking operation is supported)
-  // and that no callback is already pending. Returns |PP_OK| if okay, else
-  // |PP_ERROR_...| to be returned to the plugin.
-  int32_t ValidateCallback(PP_CompletionCallback callback);
-
-  // Sets up |callback| as the pending callback. This should only be called once
-  // it is certain that |PP_ERROR_WOULDBLOCK| will be returned.
-  void RegisterCallback(PP_CompletionCallback callback);
-
   void RunCallback(int32_t result);
-
   size_t FillUserBuffer();
 
   // Converts a WebURLResponse to a URLResponseInfo and saves it.
@@ -122,11 +110,10 @@ class PPB_URLLoader_Impl : public Resource,
   bool RecordDownloadProgress() const;
   bool RecordUploadProgress() const;
 
-  // This will be NULL if the instance has been deleted but this
-  // PPB_URLLoader_Impl was somehow leaked. In general, you should not need to
-  // check this for NULL. However, if you see a NULL pointer crash, that means
-  // somebody is holding a reference to this object longer than the
-  // PluginInstance's lifetime.
+  // This will be NULL if the instance has been deleted but this PPB_URLLoader_Impl was
+  // somehow leaked. In general, you should not need to check this for NULL.
+  // However, if you see a NULL pointer crash, that means somebody is holding
+  // a reference to this object longer than the PluginInstance's lifetime.
   PluginInstance* instance_;
 
   // If true, then the plugin instance is a full-frame plugin and we're just
@@ -135,7 +122,7 @@ class PPB_URLLoader_Impl : public Resource,
   scoped_ptr<WebKit::WebURLLoader> loader_;
   scoped_refptr<PPB_URLRequestInfo_Impl> request_info_;
   scoped_refptr<PPB_URLResponseInfo_Impl> response_info_;
-  scoped_refptr<TrackedCompletionCallback> pending_callback_;
+  PP_CompletionCallback pending_callback_;
   std::deque<char> buffer_;
   int64_t bytes_sent_;
   int64_t total_bytes_to_be_sent_;