On Windows, create a new TransportDIB::Handle struct which includes the file

On Windows, create a new TransportDIB::Handle struct which includes the file
mapping HANDLE and the source process ID. Duplicating the handle for the
remote process is done in TransportDIB::Map, instead of in various #ifdefs
scattered across the code. Also on windows, remove the struct for the
TransportDIB::Id which contained both the sequence number and the HANDLE and
replace it with just the sequence number.

Fix ThumbnailGenerator by mapping the TransportDIB on Windows and adding
a method to duplicate the file mapping handle before sending across the
channel.

Also, add a ScopedHandle and fix some handle leaks.
BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63232

[kkania, 2010-10-15 19:25:23]

First patch is the unrevert, second is my latest additions. I'll delete the
other code review.

[brettw, 2010-10-18 19:51:16]

Thanks for the much improved diff. I just have some minor comments.

http://codereview.chromium.org/3834003/diff/2001/3001
File app/surface/transport_dib.h (right):

http://codereview.chromium.org/3834003/diff/2001/3001#newcode64
app/surface/transport_dib.h:64: void Close() const;
Can you document this? Under what circumstances should I call close?

http://codereview.chromium.org/3834003/diff/2001/3001#newcode66
app/surface/transport_dib.h:66: bool is_valid() const;
With lowercase naming we require that the functions be inline (if it's named
like a variable, it should be as cheap to call as a variable). So either make it
inline, or just call it IsValid().

Can you also document this about what "valid" means?

http://codereview.chromium.org/3834003/diff/2001/3001#newcode130
app/surface/transport_dib.h:130: class ScopedHandle {
Can you put a blank line before this and add documentation for this class
explaining the proper usage? I'm thinking something like (feel free to expand):

When passing a TransportDIB::Handle across processes, you must always close the
handle, even if you return early, or the handle will be leaked. Typical usage
will be:

MyIPCHandler(TransportDIB::Handle dib_handle) {
  TransportDIB::ScopedHandle handle_scoper(dib_handle);
  ... do some initial stuff, possibly returning early ...

  TransportDIB* dib = TransportDIB::Map(handle_scoper.release());
  // The handle lifetime is now managed by the TransportDIB.

[kkania, 2010-10-18 20:31:18]

http://codereview.chromium.org/3834003/diff/2001/3001
File app/surface/transport_dib.h (right):

http://codereview.chromium.org/3834003/diff/2001/3001#newcode64
app/surface/transport_dib.h:64: void Close() const;
On 2010/10/18 19:51:17, brettw wrote:
> Can you document this? Under what circumstances should I call close?

Done.

http://codereview.chromium.org/3834003/diff/2001/3001#newcode66
app/surface/transport_dib.h:66: bool is_valid() const;
On 2010/10/18 19:51:17, brettw wrote:
> With lowercase naming we require that the functions be inline (if it's named
> like a variable, it should be as cheap to call as a variable). So either make
it
> inline, or just call it IsValid().
> 
> Can you also document this about what "valid" means?

Done.

http://codereview.chromium.org/3834003/diff/2001/3001#newcode130
app/surface/transport_dib.h:130: class ScopedHandle {
On 2010/10/18 19:51:17, brettw wrote:
> Can you put a blank line before this and add documentation for this class
> explaining the proper usage? I'm thinking something like (feel free to
expand):
> 
> When passing a TransportDIB::Handle across processes, you must always close
the
> handle, even if you return early, or the handle will be leaked. Typical usage
> will be:
> 
> MyIPCHandler(TransportDIB::Handle dib_handle) {
>   TransportDIB::ScopedHandle handle_scoper(dib_handle);
>   ... do some initial stuff, possibly returning early ...
> 
>   TransportDIB* dib = TransportDIB::Map(handle_scoper.release());
>   // The handle lifetime is now managed by the TransportDIB.

Done.

[brettw, 2010-10-18 23:17:50]

LGTM

[Scott Hess - ex-Googler, 2010-10-25 21:38:18]

http://codereview.chromium.org/3834003/diff/36001/37003
File app/surface/transport_dib_mac.cc (right):

http://codereview.chromium.org/3834003/diff/36001/37003#newcode19
app/surface/transport_dib_mac.cc:19: }
AFAICT, handle_ is a file descriptor, and since this doesn't reset to
DefaultHandle(), if Close() is called outside the destructor, it will be called
again in the destructor, which may result in closing someone else's file
descriptor.

Or did I miss something?

See http://crbug.com/60313

[kkania, 2010-10-27 18:12:59]

http://codereview.chromium.org/3834003/diff/36001/37003
File app/surface/transport_dib_mac.cc (right):

http://codereview.chromium.org/3834003/diff/36001/37003#newcode19
app/surface/transport_dib_mac.cc:19: }
On 2010/10/25 21:38:19, shess wrote:
> AFAICT, handle_ is a file descriptor, and since this doesn't reset to
> DefaultHandle(), if Close() is called outside the destructor, it will be
called
> again in the destructor, which may result in closing someone else's file
> descriptor.
> 
> Or did I miss something?
> 
> See http://crbug.com/60313

That is correct. However, since Close() is private and currently called only by
the destructor, it is ok. It might be a good idea to go ahead and reset it
anyways though.

[Scott Hess - ex-Googler, 2010-10-27 19:34:01]

http://codereview.chromium.org/3834003/diff/36001/37003
File app/surface/transport_dib_mac.cc (right):

http://codereview.chromium.org/3834003/diff/36001/37003#newcode19
app/surface/transport_dib_mac.cc:19: }
On 2010/10/27 18:12:59, kkania wrote:
> On 2010/10/25 21:38:19, shess wrote:
> > AFAICT, handle_ is a file descriptor, and since this doesn't reset to
> > DefaultHandle(), if Close() is called outside the destructor, it will be
> called
> > again in the destructor, which may result in closing someone else's file
> > descriptor.
> > 
> > Or did I miss something?
> > 
> > See http://crbug.com/60313
> 
> That is correct. However, since Close() is private and currently called only
by
> the destructor, it is ok.

Not if you happen to be multi-threaded.

> It might be a good idea to go ahead and reset it anyways though.

+1.