Reland: Fix WebViewPlugin::scheduleAnimation crash

Reland: 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.

In additional to the original CL https://codereview.chromium.org/1137663006/
This CL also switched the destruction order of pepper instance and the
throttler. Engaging throttle during destruction cause crash and doesn't make
sense anyway.

R=tommycli,raymes
BUG=483068, 487607
Committed: https://crrev.com/933b352b80d2cf8443bf18934b4296573c404a73
Cr-Commit-Position: refs/heads/master@{#330638}

[trchen, 2015-05-13 21:46:44]

Patchset #1 (id:1) has been deleted

[trchen, 2015-05-13 21:49:20]

In addition to the change in the original CL, this new CL also switches the
destruction order of the throttler. This should actually fix the crash I think.
PTAL. Thanks!

[tommycli, 2015-05-13 22:07:02]

lgtm sans:

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

https://codereview.chromium.org/1141793002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.h:107:
scoped_ptr<PluginInstanceThrottlerImpl> throttler_;
I think this isn't quite necessary.

This throttler_ is set before the plugin itself is initialized. Once the plugin
instance_ is initialized, the throttler ownership passes to the instance_, and
this member is null.

If this is non-null in destroy(), it means the plugin was never initialized.

[trchen, 2015-05-13 22:17:05]

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

https://codereview.chromium.org/1141793002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.h:107:
scoped_ptr<PluginInstanceThrottlerImpl> throttler_;
On 2015/05/13 22:07:02, tommycli wrote:
> I think this isn't quite necessary.
> 
> This throttler_ is set before the plugin itself is initialized. Once the
plugin
> instance_ is initialized, the throttler ownership passes to the instance_, and
> this member is null.
> 
> If this is non-null in destroy(), it means the plugin was never initialized.

Yep. Actually all changes in PepperWebPluginImpl are probably no-op. I made the
change anyway just as a matter of principle. (Comforting my OCD...)

Though what really matters is we need more test coverage in this area. I
actually wanted to write a test for this, but couldn't find a template to
initialize a frame with plugins. Do you have good ideas?

[tommycli, 2015-05-13 22:24:58]

On 2015/05/13 22:17:05, trchen wrote:
>
https://codereview.chromium.org/1141793002/diff/20001/content/renderer/pepper...
> File content/renderer/pepper/pepper_webplugin_impl.h (right):
> 
>
https://codereview.chromium.org/1141793002/diff/20001/content/renderer/pepper...
> content/renderer/pepper/pepper_webplugin_impl.h:107:
> scoped_ptr<PluginInstanceThrottlerImpl> throttler_;
> On 2015/05/13 22:07:02, tommycli wrote:
> > I think this isn't quite necessary.
> > 
> > This throttler_ is set before the plugin itself is initialized. Once the
> plugin
> > instance_ is initialized, the throttler ownership passes to the instance_,
and
> > this member is null.
> > 
> > If this is non-null in destroy(), it means the plugin was never initialized.
> 
> Yep. Actually all changes in PepperWebPluginImpl are probably no-op. I made
the
> change anyway just as a matter of principle. (Comforting my OCD...)
> 
> Though what really matters is we need more test coverage in this area. I
> actually wanted to write a test for this, but couldn't find a template to
> initialize a frame with plugins. Do you have good ideas?

Hmm... take a look at PluginPowerSaverBrowserTest.* and
PrerenderBrowserTest.PrerenderContentSettingDetect

Those are end-to-end style tests, but I think they don't trigger the race
condition which is causing this crash.

[trchen, 2015-05-14 23:58:41]

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

[trchen, 2015-05-14 23:58:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps40001
(title: "reverting no-op changes in PepperWebPluginImpl")

[commit-bot: I haz the power, 2015-05-15 00:00:34]

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

[commit-bot: I haz the power, 2015-05-15 00:50:21]

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

[commit-bot: I haz the power, 2015-05-15 00:50:24]

Dry run: This issue passed the CQ dry run.

[raymes, 2015-05-15 05:06:09]

Are you able to tickle this using a variant of PluginPowerSaverBrowserTest?

[trchen, 2015-05-15 06:01:19]

On 2015/05/15 05:06:09, raymes wrote:
> Are you able to tickle this using a variant of PluginPowerSaverBrowserTest?

Still trying. Needs to steal lots of boilerplate code from RenderViewTest and
PpapiUnittest. If I haven't uploaded a new CL by noon tomorrow (PST) that means
I failed and went to sleep. :)

[trchen, 2015-05-15 09:50:13]

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

[trchen, 2015-05-15 09:50:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps60001
(title: "test added")

[commit-bot: I haz the power, 2015-05-15 09:50:22]

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

[trchen, 2015-05-15 09:51:27]

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

[trchen, 2015-05-15 09:51:27]

+jam@ for content_constants.

[commit-bot: I haz the power, 2015-05-15 10:09:22]

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

[commit-bot: I haz the power, 2015-05-15 10:09:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[trchen, 2015-05-15 11:35:25]

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

[trchen, 2015-05-15 11:35:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps80001
(title: "revise test")

[commit-bot: I haz the power, 2015-05-15 11:35:41]

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

[commit-bot: I haz the power, 2015-05-15 11:59:54]

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

[commit-bot: I haz the power, 2015-05-15 11:59:56]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[trchen, 2015-05-15 20:22:26]

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

[trchen, 2015-05-15 20:22:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps100001
(title: "fix test resource loading")

[commit-bot: I haz the power, 2015-05-15 20:23:02]

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

[trchen, 2015-05-15 20:53:14]

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

[trchen, 2015-05-15 20:53:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps120001
(title: "correct gyp dependencies for test")

[commit-bot: I haz the power, 2015-05-15 20:56:21]

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

[trchen, 2015-05-15 20:58:50]

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

[trchen, 2015-05-15 20:58:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps140001
(title: "filepath windows issue")

[commit-bot: I haz the power, 2015-05-15 21:00:51]

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

[trchen, 2015-05-15 23:39:59]

Hello reviewers, I think this CL is ready now. PTAL. Thanks!

[commit-bot: I haz the power, 2015-05-16 01:35:18]

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

[commit-bot: I haz the power, 2015-05-16 01:35:19]

Dry run: This issue passed the CQ dry run.

[raymes, 2015-05-18 04:38:02]

lgtm! Nice work, thanks for adding the test.

[raymes, 2015-05-18 04:39:07]

https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/pepper_webplugin_impl_browsertest.cc (right):

https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
content/renderer/pepper/pepper_webplugin_impl_browsertest.cc:91: static void
DummyCallback(void*, int32_t) {}
nit: newline below

https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
content/renderer/pepper/pepper_webplugin_impl_browsertest.cc:187:
TEST_F(PepperWebPluginImplBrowserTest, NotEngageThrottleDuringDestroy) {
nit: perhaps add a comment saying what this does with a link to the bug.

[jam, 2015-05-18 15:44:04]

https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/plugin_power_saver_helper.cc (right):

https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
content/renderer/pepper/plugin_power_saver_helper.cc:126: if (plugin_module_name
== content::kAlwaysThrottleTestPluginName)
we shouldn't have knowledge of this test plugin name in shipping code.

can this be a command line flag instead, i.e. --always-throttle-plugins?

[trchen, 2015-05-18 20:25:18]

On 2015/05/18 15:44:04, jam wrote:
>
https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
> File content/renderer/pepper/plugin_power_saver_helper.cc (right):
> 
>
https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
> content/renderer/pepper/plugin_power_saver_helper.cc:126: if
(plugin_module_name
> == content::kAlwaysThrottleTestPluginName)
> we shouldn't have knowledge of this test plugin name in shipping code.
> 
> can this be a command line flag instead, i.e. --always-throttle-plugins?

Yep, that may be a cleaner option. By the way there is another constant string
ppapi::kPowerSaverTestPluginName that does similar thing (throttler will be
enabled, but won't throttle blindly.). I can convert that into a flag too.

[tommycli, 2015-05-18 20:27:32]

On 2015/05/18 20:25:18, trchen wrote:
> On 2015/05/18 15:44:04, jam wrote:
> >
>
https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
> > File content/renderer/pepper/plugin_power_saver_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/1141793002/diff/140001/content/renderer/peppe...
> > content/renderer/pepper/plugin_power_saver_helper.cc:126: if
> (plugin_module_name
> > == content::kAlwaysThrottleTestPluginName)
> > we shouldn't have knowledge of this test plugin name in shipping code.
> > 
> > can this be a command line flag instead, i.e. --always-throttle-plugins?
> 
> Yep, that may be a cleaner option. By the way there is another constant string
> ppapi::kPowerSaverTestPluginName that does similar thing (throttler will be
> enabled, but won't throttle blindly.). I can convert that into a flag too.

That would be awesome.

[trchen, 2015-05-18 23:11:14]

Done. PTAL thanks!

[trchen, 2015-05-18 23:11:21]

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

[trchen, 2015-05-18 23:11:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps160001
(title: "remove PPS test plugin name constant in favor of command line switch")

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

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

[trchen, 2015-05-19 00:28:31]

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

[trchen, 2015-05-19 00:28:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps180001
(title: "oops, we still need special PPS test plugin.")

[commit-bot: I haz the power, 2015-05-19 00:29:16]

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

[commit-bot: I haz the power, 2015-05-19 02:04:30]

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

[commit-bot: I haz the power, 2015-05-19 02:04:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[trchen, 2015-05-19 02:42:23]

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

[trchen, 2015-05-19 02:42:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps200001
(title: "don't forget to propagate to renderer")

[commit-bot: I haz the power, 2015-05-19 02:42:38]

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

[commit-bot: I haz the power, 2015-05-19 03:53:06]

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

[commit-bot: I haz the power, 2015-05-19 03:53:07]

Dry run: This issue passed the CQ dry run.

[jam, 2015-05-19 15:58:01]

lgtm

https://codereview.chromium.org/1141793002/diff/200001/content/renderer/peppe...
File content/renderer/pepper/plugin_power_saver_helper.cc (right):

https://codereview.chromium.org/1141793002/diff/200001/content/renderer/peppe...
content/renderer/pepper/plugin_power_saver_helper.cc:140: if
(override_for_testing_ == Always)
nit: i think you need brace brackets here per style guide

[trchen, 2015-05-19 21:34:36]

The CQ bit was checked by trchen@chromium.org

[trchen, 2015-05-19 21:34:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, jam@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1141793002/#ps220001
(title: "fix style nit")

[commit-bot: I haz the power, 2015-05-19 21:37:56]

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

[commit-bot: I haz the power, 2015-05-19 22:33:32]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-05-19 22:35:20]

Patchset 11 (id:??) landed as
https://crrev.com/933b352b80d2cf8443bf18934b4296573c404a73
Cr-Commit-Position: refs/heads/master@{#330638}