Introduce NaClFileInfoAutoCloser as an RAII wrapper.

Introduce NaClFileInfoAutoCloser as an RAII wrapper.

This eliminates two descriptor leaks -- the downloaded nmf file, and
the nexe itself.  One socket (on Linux) still leak.  Because Unixes
always use the lowest available number and the leaked descriptor is
the highest numbered one while the F@H nexe is running, this implies
that this might be a PPAPI proxy related leak, since proxy hookup is
the last thing that happens before the application itself runs.

See /home/bsy/fd-leak-log for time-lapsed view of what happens when a
F@H NaCl module finishes and another starts up.

TEST= manually running F@H and checking /proc/<renderer>/fd/*
BUG=335186
R=bbudge@chromium.org, dmichael@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247794

[bsy, 2014-01-29 00:58:11]

PTAL

[dmichael (off chromium), 2014-01-29 18:22:27]

Thanks, looks good overall. I just had some nits.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:61: info_ =
NoFileInfo();
Why not do this in the initializer list instead?

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:65: info_ =
pass_ownership;
ditto

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:72: close(get_desc());
tiny stupid nit: seems better to be consistent and either use get_desc
throughout this method or info_.desc, but not both. Or am I missing something
subtle?

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:77: void
NaClFileInfoAutoCloser::set(NaClFileInfo pass_ownership) {
nit: this should probably also be capitalized, and maybe named more
descriptively like:
TakeOwnership
WDYT?

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:80: FreeResources();
nit: It might be good to check on if info_.desc == pass_ownership, and either do
nothing, or (maybe better) just assert that case doesn't happen.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:85: NaClFileInfo nvro;
Some readers won't get nvro (is named return value optimization your intent?),
and I don't think the fact that the compiler can do return optimization is
actually important here (it's cheap either way).

How about something like info_to_return?

And it seems like you could initialize it to info_ right here, rather than
separate assignment?

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.h (right):

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:49:
NaClFileInfoAutoCloser(NaClFileInfo pass_ownership);
nit: explicit

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:55: void
FreeResources();  // Frees owned resources
nit: comment should go above the method. Ditto release().

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:65: NaClFileInfo
release();  // Returns ownership to caller
nit: Probably should be capitalized, since it's semantically more than just a
getter.

[bsy, 2014-01-29 20:51:27]

addressed CR feedback.  found issue with plugin.cc's url_to_file_info_map_ --
seems like an old version was uploaded, but no idea how.  anyway, PTAL.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:61: info_ =
NoFileInfo();
On 2014/01/29 18:22:28, dmichael wrote:
> Why not do this in the initializer list instead?

Done.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:65: info_ =
pass_ownership;
On 2014/01/29 18:22:28, dmichael wrote:
> ditto

Done.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:72: close(get_desc());
On 2014/01/29 18:22:28, dmichael wrote:
> tiny stupid nit: seems better to be consistent and either use get_desc
> throughout this method or info_.desc, but not both. Or am I missing something
> subtle?

not missing anything subtle, just that get_desc() was added after this edit and
i missed it.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:77: void
NaClFileInfoAutoCloser::set(NaClFileInfo pass_ownership) {
On 2014/01/29 18:22:28, dmichael wrote:
> nit: this should probably also be capitalized, and maybe named more
> descriptively like:
> TakeOwnership
> WDYT?

Done.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:80: FreeResources();
On 2014/01/29 18:22:28, dmichael wrote:
> nit: It might be good to check on if info_.desc == pass_ownership, and either
do
> nothing, or (maybe better) just assert that case doesn't happen.

Done.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:85: NaClFileInfo nvro;
On 2014/01/29 18:22:28, dmichael wrote:
> Some readers won't get nvro (is named return value optimization your intent?),
> and I don't think the fact that the compiler can do return optimization is
> actually important here (it's cheap either way).
> 
> How about something like info_to_return?
> 
> And it seems like you could initialize it to info_ right here, rather than
> separate assignment?

Done.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.h (right):

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:49:
NaClFileInfoAutoCloser(NaClFileInfo pass_ownership);
On 2014/01/29 18:22:28, dmichael wrote:
> nit: explicit

Done.  Thanks, this was careless of me.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:55: void
FreeResources();  // Frees owned resources
On 2014/01/29 18:22:28, dmichael wrote:
> nit: comment should go above the method. Ditto release().

Done.

https://codereview.chromium.org/147083014/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:65: NaClFileInfo
release();  // Returns ownership to caller
On 2014/01/29 18:22:28, dmichael wrote:
> nit: Probably should be capitalized, since it's semantically more than just a
> getter.

Done.

[dmichael (off chromium), 2014-01-29 20:59:06]

lgtm, thanks!

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:67: if (-1 !=
info_.desc) {
nit: this could also be get_desc()

[bbudge, 2014-01-29 21:43:31]

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.h (right):

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:49: explicit
NaClFileInfoAutoCloser(NaClFileInfo pass_ownership);
It would be nice if pass_ownership's desc field was set to -1 by this ctor, but
I suppose you'd have to pass it as a pointer here.

Same for TakeOwnership below.

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:61: NaClFileInfo get()
{ return info_; }
Could this return a const & instead?

[bsy, 2014-01-30 00:29:40]

PTAL

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:67: if (-1 !=
info_.desc) {
On 2014/01/29 20:59:06, dmichael wrote:
> nit: this could also be get_desc()

Done.

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/file_downloader.h (right):

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:49: explicit
NaClFileInfoAutoCloser(NaClFileInfo pass_ownership);
On 2014/01/29 21:43:31, bbudge wrote:
> It would be nice if pass_ownership's desc field was set to -1 by this ctor,
but
> I suppose you'd have to pass it as a pointer here.
> 
> Same for TakeOwnership below.

I'll set the whole thing to NoFileInfo(), since the file token fields are single
use and should be part of the ownership passing.

It makes usage a bit more awkward, but I agree that this change makes it a more
defensive API.

https://codereview.chromium.org/147083014/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/file_downloader.h:61: NaClFileInfo get()
{ return info_; }
On 2014/01/29 21:43:31, bbudge wrote:
> Could this return a const & instead?

Done.

[bbudge, 2014-01-30 00:55:41]

LGTM thanks for fixing this!

[bsy, 2014-01-30 01:07:30]

Committed patchset #7 manually as r247794 (presubmit successful).