Eliminate resource leaks from zip::ZipFiles and

third_party/zlib/google/zip_reader_unittest.cc

 // Copyright (c) 2011 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 "third_party/zlib/google/zip_reader.h"
 
 #include <set>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 162 matching lines @@
 
   base::MessageLoop message_loop_;
 };
 
 TEST_F(ZipReaderTest, Open_ValidZipFile) {
   ZipReader reader;
   ASSERT_TRUE(reader.Open(test_zip_file_));
 }
 
 TEST_F(ZipReaderTest, Open_ValidZipPlatformFile) {
+  FileWrapper zip_fd_wrapper(test_zip_file_, FileWrapper::READ_ONLY);

[satorux1, 2014/11/12 07:04:43]

does this have to be moved? otherwise, keep it-as-is?

[jeremyspiegel, 2014/11/12 08:52:51]

This doesn't have to be moved to work correctly, but since the ZipReader does
not take ownership of the file, I think it would be clearer to ensure the
FileWrapper outlives it. I think the unclear ownership semantics of
ZipReader::OpenFromPlatformFile may have led to one of the resource leaks, so in
addition to documenting it in the comment at the function declaration, I think
that changing the test case to be consistent would also help. If you'd prefer to
leave it as-is, that's fine too.

[satorux1, 2014/11/14 02:27:51]

I see. There are many places in the file where ZipReader is created before
FileWrapper (ex. PlatformFileIteration). We can fix everywhere but I guess it's
fine to leave them as-is

[jeremyspiegel, 2014/11/14 23:49:25]

Ah I missed those. Sounds good, I'll leave this as-is.

   ZipReader reader;
-  FileWrapper zip_fd_wrapper(test_zip_file_, FileWrapper::READ_ONLY);
   ASSERT_TRUE(reader.OpenFromPlatformFile(zip_fd_wrapper.platform_file()));
 }
 
 TEST_F(ZipReaderTest, Open_NonExistentFile) {
   ZipReader reader;
   ASSERT_FALSE(reader.Open(test_data_dir_.AppendASCII("nonexistent.zip")));
 }
 
 TEST_F(ZipReaderTest, Open_ExistentButNonZipFile) {
   ZipReader reader;
@@ skipping 372 matching lines @@
 
     // More than necessary byte read limit: must pass.
     EXPECT_TRUE(reader.ExtractCurrentEntryToString(16, &contents));
     EXPECT_EQ(i, contents.size());
     EXPECT_EQ(0, memcmp(contents.c_str(), "0123456", i));
   }
   reader.Close();
 }
 
 }  // namespace zip