Attempt to fix a CloseHandle crasher in the renderer process. The crash is triggered by Nacl.

Attempt to fix a CloseHandle crasher in the renderer process. The crash is triggered by Nacl.

Based on the crash dump, the crash occurs while loading a Nacl module in the renderer process. The Nacl translate
thread has a valid file handle which is created by the Nacl host in the browser. It then calls into the Nacl loader
to load the module which fails. The Nacl loading code in LaunchSelLdr function is closing the file handle which is
passed in. Based on comments in the PnaclTranslateThread class, ownership of the file handle is only transferred on success.

Thus when the call returns the PnaclTranslateThread code tries to close the file handle which is already closed. In the
meantime the Windows handle is reused to something else which is tracked by our handle tracker. The second CloseHandle
attempt causes a CHECK to fire because we are closing a handle which is being tracked.

Fix is to not close the file handle in the PnaclTranslateThread class as ownership is transferred on
call to LaunchSelHdr.

BUG=426582, 475872
Committed: https://crrev.com/ae9c454b37f91d1578a4f9e25ee2d03915059f9a
Cr-Commit-Position: refs/heads/master@{#329251}

[ananta, 2015-05-08 23:05:22]

ananta@chromium.org changed reviewers:
+ cpu@chromium.org

[ananta, 2015-05-08 23:06:37]

ananta@chromium.org changed reviewers:
+ bbudge@chromium.org

[ananta, 2015-05-08 23:06:37]

+bbudge for nacl owners

[bbudge, 2015-05-08 23:58:37]

bbudge@chromium.org changed reviewers:
+ mseaborn@chromium.org

[bbudge, 2015-05-08 23:58:37]

LaunchSelLdr is also called when loading a nexe (not invoking the PNaCl
translator.) Does this change leak the handle in that case?

+mseaborn, who is also familiar with this code. I am OOO next week.

[ananta, 2015-05-09 01:29:18]

On 2015/05/08 23:58:37, bbudge wrote:
> LaunchSelLdr is also called when loading a nexe (not invoking the PNaCl
> translator.) Does this change leak the handle in that case?
> 
> +mseaborn, who is also familiar with this code. I am OOO next week.

From cursory code inspection it appears that the handle may be leaked in that
case. Wondering
if we can change the PnaclTranslateThread class to not close the handle on error
and rely on
the underlying code to do it.

[Mark Seaborn, 2015-05-11 15:23:41]

mseaborn@chromium.org changed reviewers:
+ jvoung@chromium.org

[Mark Seaborn, 2015-05-11 15:25:42]

+jvoung, who is more familiar with the PNaCl code paths than me.

Whichever way you fix this, could you possibly add some comments to say what the
ownership transfer rules are for these functions?

[jvoung (off chromium), 2015-05-11 16:59:44]

On 2015/05/09 01:29:18, ananta wrote:
> On 2015/05/08 23:58:37, bbudge wrote:
> > LaunchSelLdr is also called when loading a nexe (not invoking the PNaCl
> > translator.) Does this change leak the handle in that case?
> > 
> > +mseaborn, who is also familiar with this code. I am OOO next week.
> 
> From cursory code inspection it appears that the handle may be leaked in that
> case. Wondering
> if we can change the PnaclTranslateThread class to not close the handle on
error
> and rely on
> the underlying code to do it.

Thanks for figuring out the double-close. Looks like one behavior came from
271918 and the other came from 279069.

In any case, yes I think having PnaclTranslateThread not close on error, and
letting LaunchSelLdr take care of closing would be better. ppb_nacl_private.h
actually does say "LaunchSelLdr takes the ownership of the file handle." now and
it'll be consistent with the non-PNaCl case.

There's a few error bail-outs between PnaclTranslateThread's call to
LoadHelperNaClModule() and LaunchSelLdr() where the file isn't closed, but they
are mainly memory allocation checks... (1) Plugin::LoadHelperNaClModule (2)
ServiceRuntime::StartSelLdr that I noticed.

[ananta, 2015-05-11 20:51:56]

On 2015/05/11 16:59:44, jvoung wrote:
> On 2015/05/09 01:29:18, ananta wrote:
> > On 2015/05/08 23:58:37, bbudge wrote:
> > > LaunchSelLdr is also called when loading a nexe (not invoking the PNaCl
> > > translator.) Does this change leak the handle in that case?
> > > 
> > > +mseaborn, who is also familiar with this code. I am OOO next week.
> > 
> > From cursory code inspection it appears that the handle may be leaked in
that
> > case. Wondering
> > if we can change the PnaclTranslateThread class to not close the handle on
> error
> > and rely on
> > the underlying code to do it.
> 
> Thanks for figuring out the double-close. Looks like one behavior came from
> 271918 and the other came from 279069.
> 
> In any case, yes I think having PnaclTranslateThread not close on error, and
> letting LaunchSelLdr take care of closing would be better. ppb_nacl_private.h
> actually does say "LaunchSelLdr takes the ownership of the file handle." now
and
> it'll be consistent with the non-PNaCl case.
> 
> There's a few error bail-outs between PnaclTranslateThread's call to
> LoadHelperNaClModule() and LaunchSelLdr() where the file isn't closed, but
they
> are mainly memory allocation checks... (1) Plugin::LoadHelperNaClModule (2)
> ServiceRuntime::StartSelLdr that I noticed.

Changed PncalTranslateThread to not close on error and added comments in the
helper nacl functions
to indicate that ownership of file info is transferred.

[jvoung (off chromium), 2015-05-11 21:09:11]

LGTM thanks

[ananta, 2015-05-11 21:24:04]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-05-11 21:27:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137833003/40001

[commit-bot: I haz the power, 2015-05-11 22:24:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-11 22:24:51]

Patchset 3 (id:??) landed as
https://crrev.com/ae9c454b37f91d1578a4f9e25ee2d03915059f9a
Cr-Commit-Position: refs/heads/master@{#329251}