Fix javascript-backslash.html layout test failure.

The fix is to break WebURLLoaderImpl into two classes.  It now has a
reference counted inner class that is the ResourceLoaderBridge::Peer.
This change allows the inner class to stick around after the loader
has been destroyed.  That is necessary since the Peer expects to still
receive OnCompletedRequest after it calls Cancel on the bridge.

R=dglazkov
TEST=covered by layout test
BUG=13786

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

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

4 files changed, 233 insertions, 211 deletions

diff --git a/webkit/api/public/WebURLLoader.h b/webkit/api/public/WebURLLoader.h
index e9027c4..b279987 100644
--- a/webkit/api/public/WebURLLoader.h
+++ b/webkit/api/public/WebURLLoader.h
@@ -42,6 +42,7 @@ namespace WebKit {
 
     class WebURLLoader {
     public:
+        // The WebURLLoader may be deleted in a call to its client.
         virtual ~WebURLLoader() {}
 
         // Load the request synchronously, returning results directly to the
diff --git a/webkit/api/src/ResourceHandle.cpp b/webkit/api/src/ResourceHandle.cpp
index 7b4043c..b7def8d 100644
--- a/webkit/api/src/ResourceHandle.cpp
+++ b/webkit/api/src/ResourceHandle.cpp
@@ -41,7 +41,6 @@
 #include "WrappedResourceRequest.h"
 #include "WrappedResourceResponse.h"
 
-#include "MainThread.h"
 #include "ResourceHandleClient.h"
 #include "ResourceRequest.h"
 
@@ -49,16 +48,6 @@ using namespace WebKit;
 
 namespace WebCore {
 
-static void deleteLoader(void* param)
-{
-    delete static_cast<WebURLLoader*>(param);
-}
-
-static void deleteLoaderSoon(WebURLLoader* loader)
-{
-    callOnMainThread(deleteLoader, loader);
-}
-
 // ResourceHandleInternal -----------------------------------------------------
 
 class ResourceHandleInternal : public WebURLLoaderClient {
@@ -70,8 +59,6 @@ public:
     {
     }
 
-    ~ResourceHandleInternal();
-
     void start();
     void cancel();
     void setDefersLoading(bool);
@@ -92,18 +79,6 @@ public:
     OwnPtr<WebURLLoader> m_loader;
 };
 
-ResourceHandleInternal::~ResourceHandleInternal()
-{
-    if (m_loader.get()) {
-        m_loader->cancel();
-        // The loader is typically already on the stack at this point, so we
-        // need to defer its destruction.
-        // FIXME: We should probably add a "dispose" method on WebURLLoader and
-        // push this deferred destruction logic into the loader implementation.
-        deleteLoaderSoon(m_loader.release());
-    }
-}
-
 void ResourceHandleInternal::start()
 {
     m_loader.set(webKitClient()->createURLLoader());
diff --git a/webkit/glue/weburlloader_impl.cc b/webkit/glue/weburlloader_impl.cc
index df8f077..5b2e0dd 100644
--- a/webkit/glue/weburlloader_impl.cc
+++ b/webkit/glue/weburlloader_impl.cc
@@ -22,6 +22,8 @@
 #include "webkit/api/public/WebURLRequest.h"
 #include "webkit/api/public/WebURLResponse.h"
 #include "webkit/glue/glue_util.h"
+#include "webkit/glue/multipart_response_delegate.h"
+#include "webkit/glue/resource_loader_bridge.h"
 #include "webkit/glue/webkit_glue.h"
 
 using base::Time;
@@ -38,6 +40,8 @@ using WebKit::WebURLResponse;
 
 namespace webkit_glue {
 
+// Utilities ------------------------------------------------------------------
+
 namespace {
 
 class HeaderFlattener : public WebHTTPHeaderVisitor {
@@ -180,52 +184,57 @@ void PopulateURLResponse(
 
 } // namespace
 
-WebURLLoaderImpl::WebURLLoaderImpl()
-    : ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)),
-      client_(NULL) {
-}
+// WebURLLoaderImpl::Context --------------------------------------------------
 
-WebURLLoaderImpl::~WebURLLoaderImpl() {
-}
-
-void WebURLLoaderImpl::loadSynchronously(const WebURLRequest& request,
-                                         WebURLResponse& response,
-                                         WebURLError& error,
-                                         WebData& data) {
-  ResourceLoaderBridge::SyncLoadResponse sync_load_response;
-  Start(request, &sync_load_response);
-
-  const GURL& final_url = sync_load_response.url;
-
-  // TODO(tc): For file loads, we may want to include a more descriptive
-  // status code or status text.
-  const URLRequestStatus::Status& status = sync_load_response.status.status();
-  if (status != URLRequestStatus::SUCCESS &&
-      status != URLRequestStatus::HANDLED_EXTERNALLY) {
-    response.setURL(final_url);
-    error.domain = WebString::fromUTF8(net::kErrorDomain);
-    error.reason = sync_load_response.status.os_error();
-    error.unreachableURL = final_url;
-    return;
-  }
+// This inner class exists since the WebURLLoader may be deleted while inside a
+// call to WebURLLoaderClient.  The bridge requires its Peer to stay alive
+// until it receives OnCompletedRequest.
+class WebURLLoaderImpl::Context : public base::RefCounted<Context>,
+                                  public ResourceLoaderBridge::Peer {
+ public:
+  explicit Context(WebURLLoaderImpl* loader);
+
+  WebURLLoaderClient* client() const { return client_; }
+  void set_client(WebURLLoaderClient* client) { client_ = client; }
+
+  void Cancel();
+  void SetDefersLoading(bool value);
+  void Start(
+      const WebURLRequest& request,
+      ResourceLoaderBridge::SyncLoadResponse* sync_load_response);
+
+  // ResourceLoaderBridge::Peer methods:
+  virtual void OnUploadProgress(uint64 position, uint64 size);
+  virtual void OnReceivedRedirect(const GURL& new_url);
+  virtual void OnReceivedResponse(
+      const ResourceLoaderBridge::ResponseInfo& info, bool content_filtered);
+  virtual void OnReceivedData(const char* data, int len);
+  virtual void OnCompletedRequest(
+      const URLRequestStatus& status, const std::string& security_info);
+  virtual std::string GetURLForDebugging();
 
-  PopulateURLResponse(final_url, sync_load_response, &response);
+ private:
+  friend class base::RefCounted<Context>;
+  ~Context() {}
 
-  data.assign(sync_load_response.data.data(),
-              sync_load_response.data.size());
-}
+  void HandleDataURL();
 
-void WebURLLoaderImpl::loadAsynchronously(const WebURLRequest& request,
-                                          WebURLLoaderClient* client) {
-  DCHECK(!client_);
+  WebURLLoaderImpl* loader_;
+  GURL url_;
+  WebURLLoaderClient* client_;
+  scoped_ptr<ResourceLoaderBridge> bridge_;
+  scoped_ptr<MultipartResponseDelegate> multipart_delegate_;
+  int64 expected_content_length_;
+};
 
-  client_ = client;
-  Start(request, NULL);
+WebURLLoaderImpl::Context::Context(WebURLLoaderImpl* loader)
+    : loader_(loader),
+      client_(NULL) {
 }
 
-void WebURLLoaderImpl::cancel() {
-  // The bridge will still send OnCompletedRequest, which will deref() us,
-  // so we don't do that here.
+void WebURLLoaderImpl::Context::Cancel() {
+  // The bridge will still send OnCompletedRequest, which will Release() us, so
+  // we don't do that here.
   if (bridge_.get())
     bridge_->Cancel();
 
@@ -235,128 +244,15 @@ void WebURLLoaderImpl::cancel() {
 
   // Do not make any further calls to the client.
   client_ = NULL;
+  loader_ = NULL;
 }
 
-void WebURLLoaderImpl::setDefersLoading(bool value) {
+void WebURLLoaderImpl::Context::SetDefersLoading(bool value) {
   if (bridge_.get())
     bridge_->SetDefersLoading(value);
 }
 
-void WebURLLoaderImpl::OnUploadProgress(uint64 position, uint64 size) {
-  if (client_)
-    client_->didSendData(this, position, size);
-}
-
-void WebURLLoaderImpl::OnReceivedRedirect(const GURL& new_url) {
-  if (!client_)
-    return;
-
-  // TODO(darin): We lack sufficient information to construct the
-  // actual redirect response, so we just simulate it here.
-  WebURLResponse response(url_);
-
-  // TODO(darin): We lack sufficient information to construct the
-  // actual request that resulted from the redirect, so we just
-  // report a GET navigation to the new location.
-  WebURLRequest new_request(new_url);
-
-  url_ = new_url;
-  client_->willSendRequest(this, new_request, response);
-
-  // TODO(darin): since new_request is sent as a mutable reference, it is
-  // possible that willSendRequest may have modified it.
-  //
-  // andresca on #webkit confirms that that is intentional, so we'll need
-  // to rework the ResourceLoaderBridge to give us control over what URL
-  // is really loaded (and with what headers) when a redirect is encountered.
-  // TODO(darin): we fail this assertion in some layout tests!
-  //DCHECK(GURL(new_request.url()) == new_url);
-}
-
-void WebURLLoaderImpl::OnReceivedResponse(
-    const ResourceLoaderBridge::ResponseInfo& info,
-    bool content_filtered) {
-  if (!client_)
-    return;
-
-  WebURLResponse response;
-  response.initialize();
-  PopulateURLResponse(url_, info, &response);
-  response.setIsContentFiltered(content_filtered);
-
-  expected_content_length_ = response.expectedContentLength();
-
-  client_->didReceiveResponse(this, response);
-
-  // we may have been cancelled after didReceiveResponse, which would leave us
-  // without a client and therefore without much need to do multipart handling.
-  if (!client_)
-    return;
-
-  DCHECK(!multipart_delegate_.get());
-  if (info.headers && info.mime_type == "multipart/x-mixed-replace") {
-    std::string content_type;
-    info.headers->EnumerateHeader(NULL, "content-type", &content_type);
-
-    std::string boundary = net::GetHeaderParamValue(content_type, "boundary");
-    TrimString(boundary, " \"", &boundary);
-
-    // If there's no boundary, just handle the request normally.  In the gecko
-    // code, nsMultiMixedConv::OnStartRequest throws an exception.
-    if (!boundary.empty()) {
-      multipart_delegate_.reset(
-          new MultipartResponseDelegate(client_, this, response, boundary));
-    }
-  }
-}
-
-void WebURLLoaderImpl::OnReceivedData(const char* data, int len) {
-  if (!client_)
-    return;
-
-  if (multipart_delegate_.get()) {
-    // The multipart delegate will make the appropriate calls to
-    // client_->didReceiveData and client_->didReceiveResponse.
-    multipart_delegate_->OnReceivedData(data, len);
-  } else {
-    client_->didReceiveData(this, data, len, expected_content_length_);
-  }
-}
-
-void WebURLLoaderImpl::OnCompletedRequest(const URLRequestStatus& status,
-                                          const std::string& security_info) {
-  if (multipart_delegate_.get()) {
-    multipart_delegate_->OnCompletedRequest();
-    multipart_delegate_.reset(NULL);
-  }
-
-  if (!client_)
-    return;
-
-  if (status.status() != URLRequestStatus::SUCCESS) {
-    int error_code;
-    if (status.status() == URLRequestStatus::HANDLED_EXTERNALLY) {
-      // By marking this request as aborted we insure that we don't navigate
-      // to an error page.
-      error_code = net::ERR_ABORTED;
-    } else {
-      error_code = status.os_error();
-    }
-    WebURLError error;
-    error.domain = WebString::fromUTF8(net::kErrorDomain);
-    error.reason = error_code;
-    error.unreachableURL = url_;
-    client_->didFail(this, error);
-  } else {
-    client_->didFinishLoading(this);
-  }
-}
-
-std::string WebURLLoaderImpl::GetURLForDebugging() {
-  return url_.spec();
-}
-
-void WebURLLoaderImpl::Start(
+void WebURLLoaderImpl::Context::Start(
     const WebURLRequest& request,
     ResourceLoaderBridge::SyncLoadResponse* sync_load_response) {
   DCHECK(!bridge_.get());
@@ -371,8 +267,9 @@ void WebURLLoaderImpl::Start(
                          &sync_load_response->data,
                          &sync_load_response->status);
     } else {
+      AddRef();  // Balanced in OnCompletedRequest
       MessageLoop::current()->PostTask(FROM_HERE,
-          task_factory_.NewRunnableMethod(&WebURLLoaderImpl::HandleDataURL));
+          NewRunnableMethod(this, &Context::HandleDataURL));
     }
     return;
   }
@@ -462,14 +359,134 @@ void WebURLLoaderImpl::Start(
     return;
   }
 
-  if (!bridge_->Start(this))
+  if (bridge_->Start(this)) {
+    AddRef();  // Balanced in OnCompletedRequest
+  } else {
     bridge_.reset();
+  }
 }
 
-void WebURLLoaderImpl::HandleDataURL() {
+void WebURLLoaderImpl::Context::OnUploadProgress(uint64 position, uint64 size) {
+  if (client_)
+    client_->didSendData(loader_, position, size);
+}
+
+void WebURLLoaderImpl::Context::OnReceivedRedirect(const GURL& new_url) {
   if (!client_)
     return;
 
+  // TODO(darin): We lack sufficient information to construct the
+  // actual redirect response, so we just simulate it here.
+  WebURLResponse response(url_);
+
+  // TODO(darin): We lack sufficient information to construct the
+  // actual request that resulted from the redirect, so we just
+  // report a GET navigation to the new location.
+  WebURLRequest new_request(new_url);
+
+  url_ = new_url;
+  client_->willSendRequest(loader_, new_request, response);
+
+  // TODO(darin): since new_request is sent as a mutable reference, it is
+  // possible that willSendRequest may have modified it.
+  //
+  // andresca on #webkit confirms that that is intentional, so we'll need
+  // to rework the ResourceLoaderBridge to give us control over what URL
+  // is really loaded (and with what headers) when a redirect is encountered.
+  // TODO(darin): we fail this assertion in some layout tests!
+  //DCHECK(GURL(new_request.url()) == new_url);
+}
+
+void WebURLLoaderImpl::Context::OnReceivedResponse(
+    const ResourceLoaderBridge::ResponseInfo& info,
+    bool content_filtered) {
+  if (!client_)
+    return;
+
+  WebURLResponse response;
+  response.initialize();
+  PopulateURLResponse(url_, info, &response);
+  response.setIsContentFiltered(content_filtered);
+
+  expected_content_length_ = response.expectedContentLength();
+
+  client_->didReceiveResponse(loader_, response);
+
+  // we may have been cancelled after didReceiveResponse, which would leave us
+  // without a client and therefore without much need to do multipart handling.
+  if (!client_)
+    return;
+
+  DCHECK(!multipart_delegate_.get());
+  if (info.headers && info.mime_type == "multipart/x-mixed-replace") {
+    std::string content_type;
+    info.headers->EnumerateHeader(NULL, "content-type", &content_type);
+
+    std::string boundary = net::GetHeaderParamValue(content_type, "boundary");
+    TrimString(boundary, " \"", &boundary);
+
+    // If there's no boundary, just handle the request normally.  In the gecko
+    // code, nsMultiMixedConv::OnStartRequest throws an exception.
+    if (!boundary.empty()) {
+      multipart_delegate_.reset(
+          new MultipartResponseDelegate(client_, loader_, response, boundary));
+    }
+  }
+}
+
+void WebURLLoaderImpl::Context::OnReceivedData(const char* data, int len) {
+  if (!client_)
+    return;
+
+  if (multipart_delegate_.get()) {
+    // The multipart delegate will make the appropriate calls to
+    // client_->didReceiveData and client_->didReceiveResponse.
+    multipart_delegate_->OnReceivedData(data, len);
+  } else {
+    client_->didReceiveData(loader_, data, len, expected_content_length_);
+  }
+}
+
+void WebURLLoaderImpl::Context::OnCompletedRequest(
+    const URLRequestStatus& status,
+    const std::string& security_info) {
+  if (multipart_delegate_.get()) {
+    multipart_delegate_->OnCompletedRequest();
+    multipart_delegate_.reset(NULL);
+  }
+
+  if (client_) {
+    if (status.status() != URLRequestStatus::SUCCESS) {
+      int error_code;
+      if (status.status() == URLRequestStatus::HANDLED_EXTERNALLY) {
+        // By marking this request as aborted we insure that we don't navigate
+        // to an error page.
+        error_code = net::ERR_ABORTED;
+      } else {
+        error_code = status.os_error();
+      }
+      WebURLError error;
+      error.domain = WebString::fromUTF8(net::kErrorDomain);
+      error.reason = error_code;
+      error.unreachableURL = url_;
+      client_->didFail(loader_, error);
+    } else {
+      client_->didFinishLoading(loader_);
+    }
+  }
+
+  // We are done with the bridge now, and so we need to release the reference
+  // to ourselves that we took on behalf of the bridge.  This may cause our
+  // destruction.
+  bridge_.reset();
+  Release();
+}
+
+std::string WebURLLoaderImpl::Context::GetURLForDebugging() {
+  return url_.spec();
+}
+
+void WebURLLoaderImpl::Context::HandleDataURL() {
   ResourceLoaderBridge::ResponseInfo info;
   URLRequestStatus status;
   std::string data;
@@ -483,4 +500,57 @@ void WebURLLoaderImpl::HandleDataURL() {
   OnCompletedRequest(status, info.security_info);
 }
 
+// WebURLLoaderImpl -----------------------------------------------------------
+
+WebURLLoaderImpl::WebURLLoaderImpl()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(context_(new Context(this))) {
+}
+
+WebURLLoaderImpl::~WebURLLoaderImpl() {
+  cancel();
+}
+
+void WebURLLoaderImpl::loadSynchronously(const WebURLRequest& request,
+                                         WebURLResponse& response,
+                                         WebURLError& error,
+                                         WebData& data) {
+  ResourceLoaderBridge::SyncLoadResponse sync_load_response;
+  context_->Start(request, &sync_load_response);
+
+  const GURL& final_url = sync_load_response.url;
+
+  // TODO(tc): For file loads, we may want to include a more descriptive
+  // status code or status text.
+  const URLRequestStatus::Status& status = sync_load_response.status.status();
+  if (status != URLRequestStatus::SUCCESS &&
+      status != URLRequestStatus::HANDLED_EXTERNALLY) {
+    response.setURL(final_url);
+    error.domain = WebString::fromUTF8(net::kErrorDomain);
+    error.reason = sync_load_response.status.os_error();
+    error.unreachableURL = final_url;
+    return;
+  }
+
+  PopulateURLResponse(final_url, sync_load_response, &response);
+
+  data.assign(sync_load_response.data.data(),
+              sync_load_response.data.size());
+}
+
+void WebURLLoaderImpl::loadAsynchronously(const WebURLRequest& request,
+                                          WebURLLoaderClient* client) {
+  DCHECK(!context_->client());
+
+  context_->set_client(client);
+  context_->Start(request, NULL);
+}
+
+void WebURLLoaderImpl::cancel() {
+  context_->Cancel();
+}
+
+void WebURLLoaderImpl::setDefersLoading(bool value) {
+  context_->SetDefersLoading(value);
+}
+
 }  // namespace webkit_glue
diff --git a/webkit/glue/weburlloader_impl.h b/webkit/glue/weburlloader_impl.h
index 2ad50c3..9199c74 100644
--- a/webkit/glue/weburlloader_impl.h
+++ b/webkit/glue/weburlloader_impl.h
@@ -5,16 +5,12 @@
 #ifndef WEBKIT_GLUE_WEBURLLOADER_IMPL_H_
 #define WEBKIT_GLUE_WEBURLLOADER_IMPL_H_
 
-#include "base/scoped_ptr.h"
-#include "base/task.h"
+#include "base/ref_counted.h"
 #include "webkit/api/public/WebURLLoader.h"
-#include "webkit/glue/multipart_response_delegate.h"
-#include "webkit/glue/resource_loader_bridge.h"
 
 namespace webkit_glue {
 
-class WebURLLoaderImpl : public WebKit::WebURLLoader,
-                         public ResourceLoaderBridge::Peer {
+class WebURLLoaderImpl : public WebKit::WebURLLoader {
  public:
   WebURLLoaderImpl();
   ~WebURLLoaderImpl();
@@ -31,29 +27,9 @@ class WebURLLoaderImpl : public WebKit::WebURLLoader,
   virtual void cancel();
   virtual void setDefersLoading(bool value);
 
-  // ResourceLoaderBridge::Peer methods:
-  virtual void OnUploadProgress(uint64 position, uint64 size);
-  virtual void OnReceivedRedirect(const GURL& new_url);
-  virtual void OnReceivedResponse(
-      const ResourceLoaderBridge::ResponseInfo& info, bool content_filtered);
-  virtual void OnReceivedData(const char* data, int len);
-  virtual void OnCompletedRequest(
-      const URLRequestStatus& status, const std::string& security_info);
-  virtual std::string GetURLForDebugging();
-
  private:
-  void Start(
-      const WebKit::WebURLRequest& request,
-      ResourceLoaderBridge::SyncLoadResponse* sync_load_response);
-  void HandleDataURL();
-
-  ScopedRunnableMethodFactory<WebURLLoaderImpl> task_factory_;
-
-  GURL url_;
-  WebKit::WebURLLoaderClient* client_;
-  scoped_ptr<ResourceLoaderBridge> bridge_;
-  scoped_ptr<MultipartResponseDelegate> multipart_delegate_;
-  int64 expected_content_length_;
+  class Context;
+  scoped_refptr<Context> context_;
 };
 
 }  // namespace webkit_glue