Add instrumentation to help tracking issue 638400.

Add instrumentation to help tracking issue 638400.

This CL forces a dump without crash when a RenderFrameImpl is destroyed after creating a pepper plugin, but before initializing it, testing the theory that the RenderFrameImpl may be destroyed during plugin initialization.

BUG=638400
Committed: https://crrev.com/246881fe95d4a2cdf4fcd0dab7c1b987e0f4d8f3
Cr-Commit-Position: refs/heads/master@{#416353}

[lfg, 2016-09-02 18:36:22]

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

[commit-bot: I haz the power, 2016-09-02 18:37:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-09-02 19:40:39]

lfg@chromium.org changed reviewers:
+ creis@chromium.org

[lfg, 2016-09-02 19:40:39]

Charlie, please take a look.

[Charlie Reis, 2016-09-02 20:04:10]

Sure, seems reasonable.  LGTM with nits.

Can you say a bit about what theory you're testing in the CL description?  Is it
that you're expecting the RenderFrame is being destroyed after CreatePlugin but
before the plugin calls back via PepperInstanceCreated?

If that's true, then PepperWebPluginImpl's init_data_->render_frame is probably
garbage, right?  (I wonder if plugin or script code can detach the frame in the
midst of initialization?)

https://codereview.chromium.org/2307463007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2307463007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6248: plugin_creation_state_ =
CreatePluginState::NONE;
Is there a reason for this to be an enum?  Seems like a is_plugin_initializing_
bool might be clearer, if we don't need to support more states.  (It's a bit
unexpected to go back to NONE here rather than something like INITIALIZED, until
you realize there's only two states.)

[lfg, 2016-09-02 20:25:38]

Description was changed from

==========
Add instrumentation to help tracking issue 638400.

BUG=638400
==========

to

==========
Add instrumentation to help tracking issue 638400.

This CL forces a dump without crash when a RenderFrameImpl is destroyed after
creating a pepper plugin, but before initializing it, testing the theory that
the RenderFrameImpl may be destroyed during plugin initialization.

BUG=638400
==========

[lfg, 2016-09-02 20:28:00]

On 2016/09/02 20:04:10, Charlie Reis wrote:
> Sure, seems reasonable.  LGTM with nits.
> 
> Can you say a bit about what theory you're testing in the CL description?  Is
it
> that you're expecting the RenderFrame is being destroyed after CreatePlugin
but
> before the plugin calls back via PepperInstanceCreated?
> 
> If that's true, then PepperWebPluginImpl's init_data_->render_frame is
probably
> garbage, right?  (I wonder if plugin or script code can detach the frame in
the
> midst of initialization?)

That's right, the minidumps suggested that the RenderFrame is garbage. Since
createPlugin() is a virtual function in RenderFrame, I think RenderFrame has to
be valid when createPlugin is called (though I didn't check if the compiler
devirtualizes it).

https://codereview.chromium.org/2307463007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2307463007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6248: plugin_creation_state_ =
CreatePluginState::NONE;
On 2016/09/02 20:04:10, Charlie Reis wrote:
> Is there a reason for this to be an enum?  Seems like a
is_plugin_initializing_
> bool might be clearer, if we don't need to support more states.  (It's a bit
> unexpected to go back to NONE here rather than something like INITIALIZED,
until
> you realize there's only two states.)

Yes, initially I had 3 states, unitialized, initializing, initialized, but then
I realized I didn't need this. The bool makes more sense, changed.

[lfg, 2016-09-02 20:28:11]

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

[commit-bot: I haz the power, 2016-09-02 20:28:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-09-02 20:52:04]

Thanks!  LGTM.

[lfg, 2016-09-02 21:17:55]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2016-09-02 21:17:56]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2016-09-02 21:18:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 21:37:50]

Description was changed from

==========
Add instrumentation to help tracking issue 638400.

This CL forces a dump without crash when a RenderFrameImpl is destroyed after
creating a pepper plugin, but before initializing it, testing the theory that
the RenderFrameImpl may be destroyed during plugin initialization.

BUG=638400
==========

to

==========
Add instrumentation to help tracking issue 638400.

This CL forces a dump without crash when a RenderFrameImpl is destroyed after
creating a pepper plugin, but before initializing it, testing the theory that
the RenderFrameImpl may be destroyed during plugin initialization.

BUG=638400
==========

[commit-bot: I haz the power, 2016-09-02 21:37:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-02 21:40:59]

Description was changed from

==========
Add instrumentation to help tracking issue 638400.

This CL forces a dump without crash when a RenderFrameImpl is destroyed after
creating a pepper plugin, but before initializing it, testing the theory that
the RenderFrameImpl may be destroyed during plugin initialization.

BUG=638400
==========

to

==========
Add instrumentation to help tracking issue 638400.

This CL forces a dump without crash when a RenderFrameImpl is destroyed after
creating a pepper plugin, but before initializing it, testing the theory that
the RenderFrameImpl may be destroyed during plugin initialization.

BUG=638400

Committed: https://crrev.com/246881fe95d4a2cdf4fcd0dab7c1b987e0f4d8f3
Cr-Commit-Position: refs/heads/master@{#416353}
==========

[commit-bot: I haz the power, 2016-09-02 21:41:01]

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

[lfg, 2016-09-07 15:18:32]

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

The reason for reverting is: We collected enough information, reverting..