Fix a regression that accesses file system synchronously

in URLRequestFileJob::Factory.

This regression came from revision 5944. In addition to
performance penalty, the change in 5944 introduced a new
issue: the FileJob -> FileDirJob redirection code was
not hit for directory path without trailing slash. Revision
5999 tried to fix that with a questionable approach. 

The meat of this change is reverting the relevant code in
5944 and 5999. Due to the reverting, this change also
need to have the following code changes:

(1) revision 10408 fixes a bug on "file:///". We don't
regress the fix but the scenario goes through a different
code part. Some comments are changed accordingly and a
unit test is added.

(2) revision 23944 fixes a related crash. The code change
in 23944 is still valid but the unit test needs to be
rewritten.


BUG=43116,1474,18686
TEST=URLRequestTest.FileDir*

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

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

4 files changed, 50 insertions, 44 deletions

diff --git a/net/url_request/url_request_file_dir_job.cc b/net/url_request/url_request_file_dir_job.cc
index a0d67b2..19a1aaf 100644
--- a/net/url_request/url_request_file_dir_job.cc
+++ b/net/url_request/url_request_file_dir_job.cc
@@ -211,24 +211,3 @@ void URLRequestFileDirJob::CompleteRead() {
     }
   }
 }
-
-bool URLRequestFileDirJob::IsRedirectResponse(
-    GURL* location, int* http_status_code) {
-  // If the URL did not have a trailing slash, treat the response as a redirect
-  // to the URL with a trailing slash appended.
-  std::string path = request_->url().path();
-  if (path.empty() || (path[path.size() - 1] != '/')) {
-    // This happens when we discovered the file is a directory, so needs a
-    // slash at the end of the path.
-    std::string new_path = path;
-    new_path.push_back('/');
-    GURL::Replacements replacements;
-    replacements.SetPathStr(new_path);
-
-    *location = request_->url().ReplaceComponents(replacements);
-    *http_status_code = 301;  // simulate a permanent redirect
-    return true;
-  }
-
-  return false;
-}
diff --git a/net/url_request/url_request_file_dir_job.h b/net/url_request/url_request_file_dir_job.h
index e0c0fec..0322f10 100644
--- a/net/url_request/url_request_file_dir_job.h
+++ b/net/url_request/url_request_file_dir_job.h
@@ -25,7 +25,6 @@ class URLRequestFileDirJob
   virtual bool ReadRawData(net::IOBuffer* buf, int buf_size, int *bytes_read);
   virtual bool GetMimeType(std::string* mime_type) const;
   virtual bool GetCharset(std::string* charset);
-  virtual bool IsRedirectResponse(GURL* location, int* http_status_code);
 
   // DirectoryLister::DirectoryListerDelegate methods:
   virtual void OnListFile(const file_util::FileEnumerator::FindInfo& data);
diff --git a/net/url_request/url_request_file_job.cc b/net/url_request/url_request_file_job.cc
index 585929e..003a29d 100644
--- a/net/url_request/url_request_file_job.cc
+++ b/net/url_request/url_request_file_job.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-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.
 
@@ -80,8 +80,15 @@ class URLRequestFileJob::AsyncResolver :
 URLRequestJob* URLRequestFileJob::Factory(
     URLRequest* request, const std::string& scheme) {
   FilePath file_path;
+
+  // We need to decide whether to create URLRequestFileJob for file access or
+  // URLRequestFileDirJob for directory access. To avoid accessing the
+  // filesystem, we only look at the path string here.
+  // The code in the URLRequestFileJob::Start() method discovers that a path,
+  // which doesn't end with a slash, should really be treated as a directory,
+  // and it then redirects to the URLRequestFileDirJob.
   if (net::FileURLToFilePath(request->url(), &file_path) &&
-      file_util::EnsureEndsWithSeparator(&file_path) &&
+      file_util::EndsWithSeparator(file_path) &&
       file_path.IsAbsolute())
     return new URLRequestFileDirJob(request, file_path);
 
@@ -219,15 +226,20 @@ void URLRequestFileJob::DidResolve(
   if (!request_)
     return;
 
+  is_directory_ = file_info.is_directory;
+
   int rv = net::OK;
-  // We use URLRequestFileJob to handle valid and invalid files as well as
-  // invalid directories. For a directory to be invalid, it must either not
-  // exist, or be "\" on Windows. (Windows resolves "\" to "C:\", thus
-  // reporting it as existent.) On POSIX, we don't count any existent
-  // directory as invalid.
-  if (!exists || file_info.is_directory) {
+  // We use URLRequestFileJob to handle files as well as directories without
+  // trailing slash.
+  // If a directory does not exist, we return ERR_FILE_NOT_FOUND. Otherwise,
+  // we will append trailing slash and redirect to FileDirJob.
+  // A special case is "\" on Windows. We should resolve as invalid.
+  // However, Windows resolves "\" to "C:\", thus reports it as existent.
+  // So what happens is we append it with trailing slash and redirect it to
+  // FileDirJob where it is resolved as invalid.
+  if (!exists) {
     rv = net::ERR_FILE_NOT_FOUND;
-  } else {
+  } else if (!is_directory_) {
     int flags = base::PLATFORM_FILE_OPEN |
                 base::PLATFORM_FILE_READ |
                 base::PLATFORM_FILE_ASYNC;
@@ -280,6 +292,19 @@ void URLRequestFileJob::DidRead(int result) {
 
 bool URLRequestFileJob::IsRedirectResponse(GURL* location,
                                            int* http_status_code) {
+  if (is_directory_) {
+    // This happens when we discovered the file is a directory, so needs a
+    // slash at the end of the path.
+    std::string new_path = request_->url().path();
+    new_path.push_back('/');
+    GURL::Replacements replacements;
+    replacements.SetPathStr(new_path);
+
+    *location = request_->url().ReplaceComponents(replacements);
+    *http_status_code = 301;  // simulate a permanent redirect
+    return true;
+  }
+
 #if defined(OS_WIN)
   // Follow a Windows shortcut.
   // We just resolve .lnk file, ignore others.
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index c90df21..1d5fc67 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-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.
 
@@ -962,25 +962,28 @@ TEST_F(URLRequestTest, FileDirRedirectNoCrash) {
   path = path.Append(FILE_PATH_LITERAL("url_request_unittest"));
 
   TestDelegate d;
-  d.set_quit_on_redirect(true);
   TestURLRequest req(net::FilePathToFileURL(path), &d);
   req.Start();
   MessageLoop::current()->Run();
 
-  // Let the directory lister have time to finish its work, which will
-  // cause the URLRequestFileDirJob's ref count to drop to 1.
-  URLRequestFileDirJob* job = static_cast<URLRequestFileDirJob*>(req.job());
-  while (!job->list_complete()) {
-    PlatformThread::Sleep(10);
-    MessageLoop::current()->RunAllPending();
-  }
+  ASSERT_EQ(1, d.received_redirect_count());
+  ASSERT_LT(0, d.bytes_received());
+  ASSERT_FALSE(d.request_failed());
+  ASSERT_TRUE(req.status().is_success());
+}
 
-  // Should not crash during this call!
-  req.FollowDeferredRedirect();
+#if defined(OS_WIN)
+// Don't accept the url "file:///" on windows. See http://crbug.com/1474.
+TEST_F(URLRequestTest, FileDirRedirectSingleSlash) {
+  TestDelegate d;
+  TestURLRequest req(GURL("file:///"), &d);
+  req.Start();
+  MessageLoop::current()->Run();
 
-  // Flush event queue.
-  MessageLoop::current()->RunAllPending();
+  ASSERT_EQ(1, d.received_redirect_count());
+  ASSERT_FALSE(req.status().is_success());
 }
+#endif
 
 TEST_F(URLRequestTestHTTP, RestrictRedirects) {
   ASSERT_TRUE(NULL != server_.get());