Plugin Placeholders: Factor out common BindWebFrame method into base class.

Plugin Placeholders: Factor out common BindWebFrame method into base class.

This also makes PluginPlaceholder be instantiable on its own. It no longer
has any pure virtual methods.

This patch is necessary to enable https://codereview.chromium.org/1158063002/ without needing to create a TestPluginPlaceholder subclass (because PluginPlaceholder has a pure virtual BindWebFrame method).

This is a portion of https://codereview.chromium.org/1126073003/, which got reverted. It's possible this one will cause crashes and need to be reverted, but at least then we'll narrow down the cause very well.

BUG=NONE

[tommycli, 2015-05-28 21:09:45]

tommycli@chromium.org changed reviewers:
+ bauerb@chromium.org

[tommycli, 2015-05-28 21:10:37]

bauerb: PTAL. This is a smaller portion of that patch that caused mystery
crashes on Windows.

[tommycli, 2015-05-28 21:14:25]

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

[commit-bot: I haz the power, 2015-05-28 21:15:52]

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

[tommycli, 2015-05-28 21:24:55]

bauerb, jochen: One more thing that may disturb you.

BindWebFrame is called twice because it's called recursively and reentrantly.

Here's the stacktrace for the second call:
#0 0x7fc9d51fd52e base::debug::StackTrace::StackTrace()
#1 0x7fc9e3726583 plugins::PluginPlaceholder::BindWebFrame()
#2 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
#3 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
#4 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
#5 0x7fc9d6701359
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
#6 0x7fc9c93893c8 blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
#7 0x7fc9c851d346 blink::ScriptController::windowProxy()
#8 0x7fc9c91dae9f blink::LocalFrame::windowProxy()
#9 0x7fc9c855f76a blink::toV8Context()
#10 0x7fc9d67b20fa blink::WebLocalFrameImpl::mainWorldScriptContext()
#11 0x7fc9e37265c1 plugins::PluginPlaceholder::BindWebFrame()
#12 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
#13 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
#14 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
#15 0x7fc9d6701359
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
#16 0x7fc9c93840a9 blink::FrameLoader::dispatchDidClearDocumentOfWindowObject()
#17 0x7fc9c9383eb6 blink::FrameLoader::receivedFirstData()
#18 0x7fc9c936bf1a blink::DocumentLoader::ensureWriter()
#19 0x7fc9c936a49e blink::DocumentLoader::commitData()
...

This is probably unintended, but it doesn't seem to me that this could cause
those mystery crashes above. The way to fix this double call isn't really
obvious to me either.

[chrishtr, 2015-05-28 21:27:50]

On 2015/05/28 at 21:24:55, tommycli wrote:
> bauerb, jochen: One more thing that may disturb you.
> 
> BindWebFrame is called twice because it's called recursively and reentrantly.
> 
> Here's the stacktrace for the second call:
> #0 0x7fc9d51fd52e base::debug::StackTrace::StackTrace()
> #1 0x7fc9e3726583 plugins::PluginPlaceholder::BindWebFrame()
> #2 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
> #3 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
> #4 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
> #5 0x7fc9d6701359
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> #6 0x7fc9c93893c8
blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
> #7 0x7fc9c851d346 blink::ScriptController::windowProxy()
> #8 0x7fc9c91dae9f blink::LocalFrame::windowProxy()
> #9 0x7fc9c855f76a blink::toV8Context()
> #10 0x7fc9d67b20fa blink::WebLocalFrameImpl::mainWorldScriptContext()
> #11 0x7fc9e37265c1 plugins::PluginPlaceholder::BindWebFrame()
> #12 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
> #13 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
> #14 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
> #15 0x7fc9d6701359
blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> #16 0x7fc9c93840a9
blink::FrameLoader::dispatchDidClearDocumentOfWindowObject()
> #17 0x7fc9c9383eb6 blink::FrameLoader::receivedFirstData()
> #18 0x7fc9c936bf1a blink::DocumentLoader::ensureWriter()
> #19 0x7fc9c936a49e blink::DocumentLoader::commitData()
> ...
> 
> This is probably unintended, but it doesn't seem to me that this could cause
those mystery crashes above. The way to fix this double call isn't really
obvious to me either.

We need a bit called m_dispatchingDidClearWindowObject in FrameLoader and an
early out in dispatchDidClearWindowObjectInMainWorld if it's true.

[tommycli, 2015-05-28 21:34:26]

On 2015/05/28 21:27:50, chrishtr wrote:
> On 2015/05/28 at 21:24:55, tommycli wrote:
> > bauerb, jochen: One more thing that may disturb you.
> > 
> > BindWebFrame is called twice because it's called recursively and
reentrantly.
> > 
> > Here's the stacktrace for the second call:
> > #0 0x7fc9d51fd52e base::debug::StackTrace::StackTrace()
> > #1 0x7fc9e3726583 plugins::PluginPlaceholder::BindWebFrame()
> > #2 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
> > #3 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
> > #4 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
> > #5 0x7fc9d6701359
> blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> > #6 0x7fc9c93893c8
> blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
> > #7 0x7fc9c851d346 blink::ScriptController::windowProxy()
> > #8 0x7fc9c91dae9f blink::LocalFrame::windowProxy()
> > #9 0x7fc9c855f76a blink::toV8Context()
> > #10 0x7fc9d67b20fa blink::WebLocalFrameImpl::mainWorldScriptContext()
> > #11 0x7fc9e37265c1 plugins::PluginPlaceholder::BindWebFrame()
> > #12 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
> > #13 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
> > #14 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
> > #15 0x7fc9d6701359
> blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> > #16 0x7fc9c93840a9
> blink::FrameLoader::dispatchDidClearDocumentOfWindowObject()
> > #17 0x7fc9c9383eb6 blink::FrameLoader::receivedFirstData()
> > #18 0x7fc9c936bf1a blink::DocumentLoader::ensureWriter()
> > #19 0x7fc9c936a49e blink::DocumentLoader::commitData()
> > ...
> > 
> > This is probably unintended, but it doesn't seem to me that this could cause
> those mystery crashes above. The way to fix this double call isn't really
> obvious to me either.
> 
> We need a bit called m_dispatchingDidClearWindowObject in FrameLoader and an
> early out in dispatchDidClearWindowObjectInMainWorld if it's true.

Oh... that's a really good fix. I'll make it and send it to you for review.

[commit-bot: I haz the power, 2015-05-28 22:21:03]

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

[commit-bot: I haz the power, 2015-05-28 22:21:04]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2015-05-29 10:24:56]

On 2015/05/28 21:24:55, tommycli wrote:
> bauerb, jochen: One more thing that may disturb you.
> 
> BindWebFrame is called twice because it's called recursively and reentrantly.
> 
> Here's the stacktrace for the second call:
> #0 0x7fc9d51fd52e base::debug::StackTrace::StackTrace()
> #1 0x7fc9e3726583 plugins::PluginPlaceholder::BindWebFrame()
> #2 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
> #3 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
> #4 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
> #5 0x7fc9d6701359
> blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> #6 0x7fc9c93893c8
blink::FrameLoader::dispatchDidClearWindowObjectInMainWorld()
> #7 0x7fc9c851d346 blink::ScriptController::windowProxy()
> #8 0x7fc9c91dae9f blink::LocalFrame::windowProxy()
> #9 0x7fc9c855f76a blink::toV8Context()
> #10 0x7fc9d67b20fa blink::WebLocalFrameImpl::mainWorldScriptContext()
> #11 0x7fc9e37265c1 plugins::PluginPlaceholder::BindWebFrame()
> #12 0x7fc9e372686c plugins::PluginPlaceholder::BindWebFrame()
> #13 0x7fc9e372864e WebViewPlugin::didClearWindowObject()
> #14 0x7fc9e372869c WebViewPlugin::didClearWindowObject()
> #15 0x7fc9d6701359
> blink::FrameLoaderClientImpl::dispatchDidClearWindowObjectInMainWorld()
> #16 0x7fc9c93840a9
blink::FrameLoader::dispatchDidClearDocumentOfWindowObject()
> #17 0x7fc9c9383eb6 blink::FrameLoader::receivedFirstData()
> #18 0x7fc9c936bf1a blink::DocumentLoader::ensureWriter()
> #19 0x7fc9c936a49e blink::DocumentLoader::commitData()
> ...
> 
> This is probably unintended, but it doesn't seem to me that this could cause
> those mystery crashes above. 

Actually, this could be the reason: gin::CreateHandle() takes ownership of the
pointer, so if we end up calling it twice, we have two handles that own the same
pointer. The only thing I don't understand is how that isn't crashing
constantly... :)

(BTW, we also probably need to make sure that we delete the PluginPlaceholder if
its plugin is destroyed before BindWebFrame() has been called, right?)

> The way to fix this double call isn't really
> obvious to me either.