New Zip::ZipFromMemory API.

=== First commit ===

New ZipReader::ExtractCurrentEntryToRefCountedMemory API.

New API which extract a zip entry into a memory buffer.

A bug in unzip.c was fixed that would cause uncompressing a file bigger than
the uncompressed size declared in the zip file's metadata to fail with a CRC
error.

=== Second commit ===

New Zip::ZipFromMemory API.

New API which allows a zip file to be modified (files added, replaced and
removed) from memory, without having to do a round trip to the disk.
Unfortunatelly, minizip does not support replacing or removing files from a
zip archive, therefore when that needs to happen, the file is recreated from
scratch.

=== Third commit ===

Use Zip::ZipFromMemory in a couple places.

Removed unused references to zip.h

======

TEST=out/Release/unit_tests --gtest_filter=Zip*

[João Eiras, 2014-02-25 15:20:53]

So, this is the review I told by e-mail. I pushed each commit separately so at
least the deltas are more useful.

The code has tests, works just fine. Worthwhile to keep ? What do you think ?

[satorux1, 2014-02-26 04:05:46]

Took a quick look. Here's high level comments:

1) I think ability to manipulate zip files in memory would be useful

2) I think we should introduce zip_writer.h/.cc/_unittest.cc to factor out
zipping code, like we did with zip_reader* for unzipping code.

3) I think use of RefCountedMemory and scoped_refptr<> should be avoided, unless
they are absolutely needed. These are tricky to use and error-prone. I think we
can avoid them in this code.

That said, I'd be happy to review if you start from 2). Once it's done,
zip.h/.cc will be a very think wrapper around zip_reader.h and zip_writer.h, and
we may be able to get rid of the wrapper completely.

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
File third_party/zlib/contrib/minizip/unzip.c (left):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
third_party/zlib/contrib/minizip/unzip.c:1711:
(uInt)pfile_in_zip_read_info->rest_read_uncompressed;
why was this part removed?

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
File third_party/zlib/google/zip.h (right):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip.h:68: bool ZipFromMemory(const base::FilePath&
zip_path,
I think it's not a good idea to add new APIs in this file.

A cleaner approach would be to first introduce zip_writer{.h,.cc,_unittest.cc}
like we did for zip_reader* to factor out zipping code.

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:201: base::RefCountedMemory** output)
const;
I found this API complex. RefCountedMemory is tricky to use. I'd like it just to
be std::string.

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:204: scoped_refptr<base::RefCountedMemory>*
output) const;
scoped_refptr<> should only be used when an object belongs to multiple threads.
I think scoped_refptr<> should be avoided here.

Besides, function overloading is not allowed.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[João Eiras, 2014-02-26 17:35:27]

On 2014/02/26 04:05:46, satorux at chromium.org wrote:
> Took a quick look. Here's high level comments:
> 
> 1) I think ability to manipulate zip files in memory would be useful
> 
> 2) I think we should introduce zip_writer.h/.cc/_unittest.cc to factor out
> zipping code, like we did with zip_reader* for unzipping code.
> 

So, move zip::Zip* to zip_writer and move zip::Unzip to zip_reader ?

> 3) I think use of RefCountedMemory and scoped_refptr<> should be avoided,
unless
> they are absolutely needed. These are tricky to use and error-prone. I think
we
> can avoid them in this code.
> 

Comment in line under ExtractCurrentEntryToRefCountedMemory

> That said, I'd be happy to review if you start from 2). Once it's done,
> zip.h/.cc will be a very think wrapper around zip_reader.h and zip_writer.h,
and
> we may be able to get rid of the wrapper completely.
>

[João Eiras, 2014-02-26 17:35:38]

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
File third_party/zlib/contrib/minizip/unzip.c (left):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
third_party/zlib/contrib/minizip/unzip.c:1711:
(uInt)pfile_in_zip_read_info->rest_read_uncompressed;
On 2014/02/26 04:05:47, satorux at chromium.org wrote:
> why was this part removed?

From the review description 

"A bug in unzip.c was fixed that would cause uncompressing a file bigger than 
the uncompressed size declared in the zip file's metadata to fail with a CRC 
error."

This fix is verified by the new test in zip_reader_unittest.cc.

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
File third_party/zlib/google/zip.h (right):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip.h:68: bool ZipFromMemory(const base::FilePath&
zip_path,
On 2014/02/26 04:05:47, satorux at chromium.org wrote:
> I think it's not a good idea to add new APIs in this file.
> 
> A cleaner approach would be to first introduce zip_writer{.h,.cc,_unittest.cc}
> like we did for zip_reader* to factor out zipping code.

Which APIs would belong there then ? All the zip::Zip* functions ?

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:201: base::RefCountedMemory** output)
const;
On 2014/02/26 04:05:47, satorux at chromium.org wrote:
> I found this API complex. RefCountedMemory is tricky to use. I'd like it just
to
> be std::string.

Tricky in what way ?

I'd have to warn that files inside zip archives can be quite bit. The typical
semantic for std::string is to copy and use them as if they were small pieces of
text, not big blobs like multi-megabyte images. Using RefCountedMemory makes
sure that it is a blob that won't be copied that easily.
It's quite convenient to ask the FILE thread to extract a file from an archive,
and return the contents to the other thread from which the request originated
(ok, ok, using RefCountedString::TakeString is easy).
That is exactly why I preferred this type.  What else would you suggest that
implies the buffer returned is a big blob that should not be trivially copied ?

The same applies to ZipFromMemory: making copies of the input buffers would be
quite overkill. It's quite trivial to wrap a buffer with a RefCountedMemory
object. The other way, converting a raw buffer, a vector or something else to
std::string would require a copy.

[satorux1, 2014-02-27 06:57:00]

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
File third_party/zlib/contrib/minizip/unzip.c (left):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
third_party/zlib/contrib/minizip/unzip.c:1711:
(uInt)pfile_in_zip_read_info->rest_read_uncompressed;
On 2014/02/26 17:35:38, João Eiras wrote:
> On 2014/02/26 04:05:47, satorux at http://chromium.org wrote:
> > why was this part removed?
> 
> From the review description 
> 
> "A bug in unzip.c was fixed that would cause uncompressing a file bigger than 
> the uncompressed size declared in the zip file's metadata to fail with a CRC 
> error."
> 
> This fix is verified by the new test in zip_reader_unittest.cc.

Is this issue fixed in the upstream? Otherwise, would it be possible to upstream
the change to the zlib project?

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
File third_party/zlib/google/zip.h (right):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip.h:68: bool ZipFromMemory(const base::FilePath&
zip_path,
On 2014/02/26 17:35:38, João Eiras wrote:
> On 2014/02/26 04:05:47, satorux at http://chromium.org wrote:
> > I think it's not a good idea to add new APIs in this file.
> > 
> > A cleaner approach would be to first introduce
zip_writer{.h,.cc,_unittest.cc}
> > like we did for zip_reader* to factor out zipping code.
> 
> Which APIs would belong there then ? All the zip::Zip* functions ?

Here's what I meant:

1) zip.h just contains a bunch of functions. 
2) If we are to add more features for zipping, would be nicer to have a class
3) Which we did for unzipping code, with ZipReader
https://codereview.chromium.org/8508003
4) So my suggestion was to introduce ZipWriter class and factor out zipping code
into the new class, that would give cleaner implementation and API.

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:201: base::RefCountedMemory** output)
const;
On 2014/02/26 17:35:38, João Eiras wrote:
> On 2014/02/26 04:05:47, satorux at http://chromium.org wrote:
> > I found this API complex. RefCountedMemory is tricky to use. I'd like it
just
> to
> > be std::string.
> 
> Tricky in what way ?

Reference counting in general is tricky. It makes the ownership of an object
unclear (i.e. who owns the last reference of that object?), which may cause
hard-to-debug memory related issues (ex. cyclic references, and an object living
too long because of the last reference held unexpectedly).


> I'd have to warn that files inside zip archives can be quite bit. The typical
> semantic for std::string is to copy and use them as if they were small pieces
of
> text, not big blobs like multi-megabyte images. Using RefCountedMemory makes
> sure that it is a blob that won't be copied that easily.
> It's quite convenient to ask the FILE thread to extract a file from an
archive,
> and return the contents to the other thread from which the request originated
> (ok, ok, using RefCountedString::TakeString is easy).
> That is exactly why I preferred this type.  What else would you suggest that
> implies the buffer returned is a big blob that should not be trivially copied
?


Not sure if I understand your correctly. If you want to make it uncopiable, can
your client code make it hard to do so?

For instance, suppose we have a function UncompressToMemory() that takes
std::string* as output parameter, you can do something like:

  std::string* output = new std::string;
  PostTaskAndReply(
      BrowserThread::FILE,
      FROM_HERE,
      base::Bind(&UncompressToMemory(file_name, output),
      base::Bind(&DidUncompressToMemory, output));


// This function will receive the uncompressed data in |output|
void DidUncompressToMemory(std::string* raw_output) {
   scoped_ptr<std::string> output(raw_output);
   // Then, |output| owns the uncompressed data and it's uncopiable.
   ...
  
}

This is an idiom often used in Chrome code.

Note that you could also use PostTaskAndReplyWithResult() but the return value
is copied (the compiler may be able to do return value optimization to get rid
of the copy, but it might not).


> The same applies to ZipFromMemory: making copies of the input buffers would be
> quite overkill. It's quite trivial to wrap a buffer with a RefCountedMemory
> object. The other way, converting a raw buffer, a vector or something else to
> std::string would require a copy.

I'm also confused here. Looking at the code,
ZipReader::ExtractCurrentEntryToRefCountedMemory() internally uses
std::vector<unsigned char> (should probably be std::string) for accumulating
uncompressed data. What's wrong with this function takes std::string* and
accumulate uncompressed data into that string object via the pointer? The
PostTaskAndReply() idiom I presented above would just work fine with this API.

Am I misunderstanding something?

[satorux1, 2014-02-27 16:39:02]

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/google/...
third_party/zlib/google/zip_reader.h:201: base::RefCountedMemory** output)
const;
On 2014/02/27 06:57:00, satorux at chromium.org wrote:
> On 2014/02/26 17:35:38, João Eiras wrote:
> > On 2014/02/26 04:05:47, satorux at http://chromium.org wrote:
> > > I found this API complex. RefCountedMemory is tricky to use. I'd like it
> just
> > to
> > > be std::string.
> > 
> > Tricky in what way ?
> 
> Reference counting in general is tricky. It makes the ownership of an object
> unclear (i.e. who owns the last reference of that object?), which may cause
> hard-to-debug memory related issues (ex. cyclic references, and an object
living
> too long because of the last reference held unexpectedly).
> 
> 
> > I'd have to warn that files inside zip archives can be quite bit. The
typical
> > semantic for std::string is to copy and use them as if they were small
pieces
> of
> > text, not big blobs like multi-megabyte images. Using RefCountedMemory makes
> > sure that it is a blob that won't be copied that easily.
> > It's quite convenient to ask the FILE thread to extract a file from an
> archive,
> > and return the contents to the other thread from which the request
originated
> > (ok, ok, using RefCountedString::TakeString is easy).
> > That is exactly why I preferred this type.  What else would you suggest that
> > implies the buffer returned is a big blob that should not be trivially
copied
> ?
> 
> 
> Not sure if I understand your correctly. If you want to make it uncopiable,
can
> your client code make it hard to do so?
> 
> For instance, suppose we have a function UncompressToMemory() that takes
> std::string* as output parameter, you can do something like:
> 
>   std::string* output = new std::string;
>   PostTaskAndReply(
>       BrowserThread::FILE,
>       FROM_HERE,
>       base::Bind(&UncompressToMemory(file_name, output),
>       base::Bind(&DidUncompressToMemory, output));
> 
> 
> // This function will receive the uncompressed data in |output|
> void DidUncompressToMemory(std::string* raw_output) {
>    scoped_ptr<std::string> output(raw_output);
>    // Then, |output| owns the uncompressed data and it's uncopiable.
>    ...
>   
> }
> 
> This is an idiom often used in Chrome code.

Maybe your concern was when the input data was big? If so, scoped_ptr<> can also
solve that problem. Suppose we have CompressFromMemoryToMemory() which takes a
large blog as the input data, you can do:


   // input_data is a scoped_ptr<std::string>
   std::string* output = new std::string;
   PostTaskAndReply(
       BrowserThread::FILE,
       FROM_HERE,
       base::Bind(&CompressFromMemoryToMemory(base::Passed(&input_data),
output),
       base::Bind(&DidCompressFromMemoryToMemory, output));

void CompressFromMemory(scoped_ptr<std::string>input_data, std::string* output)
{
   ...
}

Using base::Passed(&input_data), the input data is passed to the callback of
CompressFromMemoryToMemory() without copying the large data.
 

> Note that you could also use PostTaskAndReplyWithResult() but the return value
> is copied (the compiler may be able to do return value optimization to get rid
> of the copy, but it might not).
> 
> 
> > The same applies to ZipFromMemory: making copies of the input buffers would
be
> > quite overkill. It's quite trivial to wrap a buffer with a RefCountedMemory
> > object. The other way, converting a raw buffer, a vector or something else
to
> > std::string would require a copy.
> 
> I'm also confused here. Looking at the code,
> ZipReader::ExtractCurrentEntryToRefCountedMemory() internally uses
> std::vector<unsigned char> (should probably be std::string) for accumulating
> uncompressed data. What's wrong with this function takes std::string* and
> accumulate uncompressed data into that string object via the pointer? The
> PostTaskAndReply() idiom I presented above would just work fine with this API.
> 
> Am I misunderstanding something?

[João Eiras, 2014-02-27 17:12:01]

> > Which APIs would belong there then ? All the zip::Zip* functions ?
> 
> Here's what I meant:
> 
> 1) zip.h just contains a bunch of functions. 
> 2) If we are to add more features for zipping, would be nicer to have a class
> 3) Which we did for unzipping code, with ZipReader
> https://codereview.chromium.org/8508003
> 4) So my suggestion was to introduce ZipWriter class and factor out zipping
code
> into the new class, that would give cleaner implementation and API.
> 

That will be a bit awkward... there's lots of shared code between the
ZipFromMemory function and the other Zip functions. Should I move the utility
code (the one inside the anonymous namespace) to separate files ?

[João Eiras, 2014-02-27 17:12:17]

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
File third_party/zlib/contrib/minizip/unzip.c (left):

https://codereview.chromium.org/179963002/diff/70001/third_party/zlib/contrib...
third_party/zlib/contrib/minizip/unzip.c:1711:
(uInt)pfile_in_zip_read_info->rest_read_uncompressed;
On 2014/02/27 06:57:00, satorux at chromium.org wrote:
> On 2014/02/26 17:35:38, João Eiras wrote:
> > On 2014/02/26 04:05:47, satorux at http://chromium.org wrote:
> > > why was this part removed?
> > 
> > From the review description 
> > 
> > "A bug in unzip.c was fixed that would cause uncompressing a file bigger
than 
> > the uncompressed size declared in the zip file's metadata to fail with a CRC

> > error."
> > 
> > This fix is verified by the new test in zip_reader_unittest.cc.
> 
> Is this issue fixed in the upstream? Otherwise, would it be possible to
upstream
> the change to the zlib project? 

unzip.c is from the minizip project. I tried to contact them but the project is
IMO literally dead (no activity). So, someone took the liberty to fork it and
continue working on it, and I sent him my patches
https://github.com/nmoinvaz/minizip

[satorux1, 2014-02-27 17:22:10]

On 2014/02/27 17:12:01, João Eiras wrote:
> > > Which APIs would belong there then ? All the zip::Zip* functions ?
> > 
> > Here's what I meant:
> > 
> > 1) zip.h just contains a bunch of functions. 
> > 2) If we are to add more features for zipping, would be nicer to have a
class
> > 3) Which we did for unzipping code, with ZipReader
> > https://codereview.chromium.org/8508003
> > 4) So my suggestion was to introduce ZipWriter class and factor out zipping
> code
> > into the new class, that would give cleaner implementation and API.
> > 
> 
> That will be a bit awkward... there's lots of shared code between the
> ZipFromMemory function and the other Zip functions. Should I move the utility
> code (the one inside the anonymous namespace) to separate files ?

Moving utility code into separate files sounds good. We already have
zip_internal.h/cc

[João Eiras, 2014-03-19 13:21:44]

Ping :)

[João Eiras, 2014-03-19 13:23:08]

On 2014/03/19 13:21:44, João Eiras wrote:
> Ping :)

[João Eiras, 2014-03-19 13:23:57]

On 2014/03/19 13:23:08, João Eiras wrote:
> On 2014/03/19 13:21:44, João Eiras wrote:
> > Ping :)

Btw, should I split this into smaller reviews, each for separate integration, or
is all this code in one go fine enough ? (I  do see reviews which are much
bigger than this, so sholdn't be an issue).

[hshi1, 2014-03-19 17:33:26]

https://codereview.chromium.org/179963002/diff/130001/third_party/zlib/google...
File third_party/zlib/google/zip.cc (right):

https://codereview.chromium.org/179963002/diff/130001/third_party/zlib/google...
third_party/zlib/google/zip.cc:190: iter != src_relative_paths.end() && success;
++iter) {
the "&& success" part is redundant because the "break" statement at line 195
will exit the "for" loop when "success" is set to false.

https://codereview.chromium.org/179963002/diff/130001/third_party/zlib/google...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/179963002/diff/130001/third_party/zlib/google...
third_party/zlib/google/zip_reader.h:194: // non-zero.
nit: "positive non-zero" is redundant, just "positive" is sufficient.

https://codereview.chromium.org/179963002/diff/130001/third_party/zlib/google...
third_party/zlib/google/zip_reader.h:196: int max_read_bytes,
question: this seems to assume that data size cannot exceed 0x7fffffff bytes?

[João Eiras, 2014-03-20 13:52:45]

https://codereview.chromium.org/179963002/diff/130001/third_party/zlib/google...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/179963002/diff/130001/third_party/zlib/google...
third_party/zlib/google/zip_reader.h:196: int max_read_bytes,
On 2014/03/19 17:33:27, hshi1 wrote:
> question: this seems to assume that data size cannot exceed 0x7fffffff bytes?

size_t would be better, yes, though I wonder would want to read more than
2gigabytes into memory ? :) Still, std::string is limited by
std::string::max_size(), which is close to 2^32 or 2^64.

[satorux1, 2014-03-24 06:16:07]

Sorry for the belated response. I took a quick look. Overall the code looked
good but the patch was a bit too big. Could you split this into smaller patches?
That'd be easier to review.

1) the fix in third_party/zlib/contrib/minizip/unzip.c

2) moving of code to zip_internal.h/cc

3) very minor changes like removing thid_party/zlib/google/zip.h from files that
don't need this header file

4) introduction of zip_writer.h/cc

5) new features

https://codereview.chromium.org/179963002/diff/150001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/179963002/diff/150001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:712:
base::StringPiece("dummy file", 10));
please drop ", 10". this should be unnecessary.

https://codereview.chromium.org/179963002/diff/150001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:726:
base::StringPiece("dummy file", 10));
ditto

https://codereview.chromium.org/179963002/diff/150001/third_party/zlib/contri...
File third_party/zlib/contrib/minizip/unzip.c (left):

https://codereview.chromium.org/179963002/diff/150001/third_party/zlib/contri...
third_party/zlib/contrib/minizip/unzip.c:1711:
(uInt)pfile_in_zip_read_info->rest_read_uncompressed;
please create a separate patch for this change.

https://codereview.chromium.org/179963002/diff/150001/third_party/zlib/google...
File third_party/zlib/google/zip_internal.h (right):

https://codereview.chromium.org/179963002/diff/150001/third_party/zlib/google...
third_party/zlib/google/zip_internal.h:98: 
Could you create a separate patch for this?