Pepper: Fix renderer crash on plugin destruction.

Pepper: Fix renderer crash on plugin destruction.

The FileDownloader refactor caused callbacks to be more tightly bound to
PnaclCoordinator than they were previously. Before the refactor, callbacks that
were invoked as the pexe was downloaded (or the cached translated nexe was
received) were generated through the CompletionCallbackFactory interface, which
would cause them to be cancelled when PnaclCoordinator was destroyed.

This change checks that the plugin instance is still alive before calling any
of the callbacks in the PPP_PexeStreamHandler interface.

I tried conducting some local testing, but didn't manage to hit quite the same
codepath as the one reported in the bug.

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

[dmichael (off chromium), 2014-08-04 20:58:48]

In the longer term it might be better for the PexeDownloader to have an owner
(like NexeLoadManager or something) that deletes it more explicitly when the
instance shuts down. Eventually I guess the PPP_ stuff will go away, and maybe
something like WeakPtr on the target will solve the problem more neatly.

A comment somewhere indicating that PexeDownloader is responsible for deleting
itself might be good.

For a quick fix, lgtm

https://codereview.chromium.org/433633003/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/433633003/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1599:
weak_factory_.GetWeakPtr()));
nit: I just realized this could probably be base::Unretained; I don't think
weak_factory_ is buying you much.

[teravest, 2014-08-04 21:04:39]

Yes, it'd be better for PexeDownloader to have an owner.
We'll have to be careful about ownership; I imagine that soon, we could have
multiple of these running at the same time for a given NaCl application (once
there's PNaCl dynamic loading support).

I've added a comment as you suggest.

On 2014/08/04 20:58:48, dmichael wrote:
> In the longer term it might be better for the PexeDownloader to have an owner
> (like NexeLoadManager or something) that deletes it more explicitly when the
> instance shuts down. Eventually I guess the PPP_ stuff will go away, and maybe
> something like WeakPtr on the target will solve the problem more neatly.
> 
> A comment somewhere indicating that PexeDownloader is responsible for deleting
> itself might be good.
> 
> For a quick fix, lgtm
> 
>
https://codereview.chromium.org/433633003/diff/20001/components/nacl/renderer...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
> 
>
https://codereview.chromium.org/433633003/diff/20001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1599:
> weak_factory_.GetWeakPtr()));
> nit: I just realized this could probably be base::Unretained; I don't think
> weak_factory_ is buying you much.

[teravest, 2014-08-04 21:52:40]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-08-04 21:54:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/433633003/40001

[commit-bot: I haz the power, 2014-08-05 00:50:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-05 06:41:09]

Change committed as 287472