Use GUID to generate unique temp file names and retire GetTempFileName

base/files/file_util_win.cc

Index: base/files/file_util_win.cc
diff --git a/base/files/file_util_win.cc b/base/files/file_util_win.cc
index 65dc5ce1e2fe7495624cc6d8c16b954395eb11ac..f741db06e41902d632f4063b0dcea478d23d3fe7 100644
--- a/base/files/file_util_win.cc
+++ b/base/files/file_util_win.cc
@@ -20,6 +20,7 @@
 
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
+#include "base/guid.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram.h"
@@ -340,25 +341,31 @@ FILE* CreateAndOpenTemporaryFileInDir(const FilePath& dir, FilePath* path) {
 bool CreateTemporaryFileInDir(const FilePath& dir, FilePath* temp_file) {
   ThreadRestrictions::AssertIOAllowed();
 
-  wchar_t temp_name[MAX_PATH + 1];
-
-  if (!GetTempFileName(dir.value().c_str(), L"", 0, temp_name)) {
+  FilePath::StringType temp_name = UTF8ToUTF16(base::GenerateGUID() + ".tmp");

[grt (UTC plus 2), 2017/04/05 11:26:34]

ASCIIToUTF16 since you know the GUID is ASCII

[chengx, 2017/04/05 18:22:34]

Done.

+  FilePath temp_path_name = dir.Append(temp_name);
+  HANDLE file_handle =

[grt (UTC plus 2), 2017/04/05 11:26:34]

either use base::win::ScopedHandle or base::File. the latter is preferred if the
flags you need are supported.

[chengx, 2017/04/05 18:22:34]

Done. Changed to base::File.

+      ::CreateFile(temp_path_name.value().c_str(), GENERIC_READ | GENERIC_WRITE,
+                   FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CREATE_ALWAYS,

[grt (UTC plus 2), 2017/04/05 11:26:34]

regarding CREATE_ALWAYS: we have no guarantee that the existing GUID-named file
was created by Chrome or for what purpose it was created. blindly clobbering
someone else's file (even if it is in tmp) doesn't seem nice to me. i think
CREATE_NEW is the right thing here.

[grt (UTC plus 2), 2017/04/05 11:26:34]

nit: NULL -> nullptr

[chengx, 2017/04/05 18:22:34]

Done.

I know nullptr is preferred. However, "NULL" is used for all ohter CreatFile in
this file, so I used NULL instead of nullptr for consistency. Maybe I should
change all other NULL's to nullptr's in another CL, or simply ignore them? Any
suggestion please?

[chengx, 2017/04/05 18:22:34]

I agree. Changed CREATE_ALWAYS to CREATE_NEW.

+                   FILE_ATTRIBUTE_NORMAL, NULL);
+  if (file_handle == INVALID_HANDLE_VALUE) {
     DPLOG(WARNING) << "Failed to get temporary file name in "
                    << UTF16ToUTF8(dir.value());
     return false;
   }
+  ::CloseHandle(file_handle);
 
-  wchar_t long_temp_name[MAX_PATH + 1];
-  DWORD long_name_len = GetLongPathName(temp_name, long_temp_name, MAX_PATH);
-  if (long_name_len > MAX_PATH || long_name_len == 0) {
-    // GetLongPathName() failed, but we still have a temporary file.
-    *temp_file = FilePath(temp_name);
+  wchar_t full_path_name[MAX_PATH + 1];
+  DWORD full_path_name_len =
+      GetLongPathName(temp_path_name.value().c_str(), full_path_name, MAX_PATH);

[grt (UTC plus 2), 2017/04/05 11:26:34]

could you dig through the blamelist to figure out why this is here? i don't get
it. it's a waste of IO as far as i can tell, and can fail for various reasons.
my first guess was "maybe this is some trick to get a full path", but it isn't.
i don't see why this function doesn't simply return temp_path_name in call
cases.

[chengx, 2017/04/05 18:22:34]

I had the same doubts here too. So I made some changes here which failed
TEST_F(FileUtilTest, CreateTemporaryFileInDirLongPathTest). In more details, the
assertion failures were
https://cs.chromium.org/chromium/src/base/files/file_util_unittest.cc?q=file_...
and
https://cs.chromium.org/chromium/src/base/files/file_util_unittest.cc?q=file_...

I think the reason why this API call is needed is documented here.
https://cs.chromium.org/chromium/src/base/files/file_util_unittest.cc?q=file_...

Also, as mentioned in the accepted answer below: 
http://stackoverflow.com/questions/31249019/what-is-the-difference-between-ge...
"GetLongPathName requires disk access, so a relative path will be probably be
resolved by using the current working directory, too."

In short, I think one reason is that this API call tells if the users have full
access to the higher level directories. There may be other reasons I am not
aware of yet.

[grt (UTC plus 2), 2017/04/06 10:35:02]

Yes, GetLongPathName will fail if the user doesn't have accesses to some parent
dirs. It's good that we have test coverage for this. I'm not sure why
CreateTemporaryFileInDir tries to figure out the long path name, though. Why
does it matter? If the caller passes in a valid path to a directory and this fn
is able to create a temp file in it, isn't its job done? Why does it also try to
get the long path name of the given directory? I can't think of a reason why one
would expect that CreateTemporaryFileInDir("C:\SHORT~1\PATH~1") would return
"C:\Short but really long\Path where you want a file\foo.tmp". Can you?

I think changing this is out of scope for the current CL. In your
experimentation, I think it's worth checking to see if there's a performance
impact of GetLongPathName when the dir has many files. Perhaps that will be
motivation to remove this in a separate change.

+  if (full_path_name_len > MAX_PATH || full_path_name_len == 0) {
+    // GetFullPathName() failed, but we still have a temporary file.
+    *temp_file = temp_path_name;
     return true;
   }
 
-  FilePath::StringType long_temp_name_str;
-  long_temp_name_str.assign(long_temp_name, long_name_len);
-  *temp_file = FilePath(long_temp_name_str);
+  FilePath::StringType full_path_name_str;
+  full_path_name_str.assign(full_path_name, full_path_name_len);
+  *temp_file = FilePath(full_path_name_str);
   return true;
 }