Fix WebViewPlugin::scheduleAnimation crash

Fix WebViewPlugin::scheduleAnimation crash

The crash was probably due to accessing a dangling pointer to the plugin
container during a small time frame between PepperWebPluginImpl::destroy()
and the destructor being called. (Speculated from source since no reliable
repro is found.)

This CL clears eveything in the destroy() function as if the destructor has
been called, only delaying memory release.

R=tommycli
BUG=483068
Committed: https://crrev.com/6a9b5b10ff3515adb47718fc23ebe039a9e3b9ff
Cr-Commit-Position: refs/heads/master@{#329309}

[trchen, 2015-05-11 20:58:39]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-11 20:58:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137663006/1

[trchen, 2015-05-11 21:05:12]

Hello Tommy,
This CL should fix the crash pointer bug. PTAL. Thanks!

Not very sure how to test this though. I think it is possible to artificially
synthesize the race, but I couldn't find a pepper unittest as a template. I feel
our test coverage in this area could be improved.

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

The CQ bit was unchecked by commit-bot@chromium.org

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

Dry run: This issue passed the CQ dry run.

[tommycli, 2015-05-11 21:57:23]

https://codereview.chromium.org/1137663006/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/1137663006/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_webplugin_impl.cc:135: throttler_.reset();
If the plugin has been initialized, the throttler_ variable here should be NULL.
This is because the throttler ownership is passed to the instance in 103.

However, your diagnosis might still make sense. It might be correct to clear
instance_->throttler_ in PepperPluginInstanceImpl::Delete().

That would probably fix your proposed "bad" sequence of events in the bug.

[trchen, 2015-05-11 22:09:18]

https://codereview.chromium.org/1137663006/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/1137663006/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_webplugin_impl.cc:135: throttler_.reset();
On 2015/05/11 21:57:23, tommycli wrote:
> If the plugin has been initialized, the throttler_ variable here should be
NULL.
> This is because the throttler ownership is passed to the instance in 103.
> 
> However, your diagnosis might still make sense. It might be correct to clear
> instance_->throttler_ in PepperPluginInstanceImpl::Delete().
> 
> That would probably fix your proposed "bad" sequence of events in the bug.

Done.

[trchen, 2015-05-11 22:09:29]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-11 22:12:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137663006/20001

[tommycli, 2015-05-11 22:46:57]

lgtm. I think this will fix your proposed 'bad' sequence of events. thanks for
looking into this!

[commit-bot: I haz the power, 2015-05-11 23:14:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 23:14:19]

Dry run: This issue passed the CQ dry run.

[trchen, 2015-05-11 23:31:43]

The CQ bit was checked by trchen@chromium.org

[trchen, 2015-05-11 23:32:25]

The CQ bit was unchecked by trchen@chromium.org

[trchen, 2015-05-11 23:35:12]

trchen@chromium.org changed reviewers:
+ raymes@chromium.org

[trchen, 2015-05-11 23:35:12]

Just realize we still need owner's approval. PTAL. Thanks!

[raymes, 2015-05-12 00:36:30]

lgtm

https://codereview.chromium.org/1137663006/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/1137663006/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.cc:127: container_ = nullptr;
I'm not sure that this will actually have any impact on anything but it
shouldn't hurt either. In some cases I believe the container is used during
destruction of the plugin but not in this class it seems.

[trchen, 2015-05-12 01:35:33]

https://codereview.chromium.org/1137663006/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/1137663006/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.cc:127: container_ = nullptr;
On 2015/05/12 00:36:30, raymes wrote:
> I'm not sure that this will actually have any impact on anything but it
> shouldn't hurt either. In some cases I believe the container is used during
> destruction of the plugin but not in this class it seems.

Yes I think you're right. Accessing a destroyed plugin is technically
use-after-free anyway. I cleared it just in case because the pre-roller most
likely got its dangling pointer from here.

[trchen, 2015-05-12 01:35:39]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2015-05-12 01:37:45]

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

[commit-bot: I haz the power, 2015-05-12 02:01:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-12 02:03:20]

Patchset 2 (id:??) landed as
https://crrev.com/6a9b5b10ff3515adb47718fc23ebe039a9e3b9ff
Cr-Commit-Position: refs/heads/master@{#329309}

[trchen, 2015-05-13 21:07:45]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1127293004/ by trchen@chromium.org.

The reason for reverting is: This is converting the original crash to another
crash. Will need another null check.

BUG=487607.