Set needsLayout on WebViewPlugin's container when initialized

Set needsLayout on WebViewPlugin's container when initialized

A race condition is suspected in which the first setNeedsLayout call happens between a layout and a paint for a webview plugin. This change ensures that setNeedsLayout... is called on the plugin's container at initialization, which occurs during the correct document lifecycle position.

TBR=bauerb@chromium.org,tommycli@chromium.org
BUG=493375
Committed: https://crrev.com/f1bc79582bc8e82060e73119a01e29b5894c16df
Cr-Commit-Position: refs/heads/master@{#339763}

[Stephen Chennney, 2015-07-21 19:39:12]

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

[commit-bot: I haz the power, 2015-07-21 19:39:41]

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

[Stephen Chennney, 2015-07-21 19:42:03]

Should we try committing this speculatively while we work on the blink side
testing?

[Stephen Chennney, 2015-07-21 20:16:31]

schenney@chromium.org changed reviewers:
+ bauerb@chromium.org, tommycli@chromium.org
- chrishtr@chromium.org

[commit-bot: I haz the power, 2015-07-21 20:38:40]

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

[commit-bot: I haz the power, 2015-07-21 20:38:41]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2015-07-21 20:43:58]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org
- bauerb@chromium.org, tommycli@chromium.org

[chrishtr, 2015-07-21 20:43:58]

https://codereview.chromium.org/1244113003/diff/1/components/plugins/renderer...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/1244113003/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.cc:114: if (container_) {
Please add a comment summarizing why setNeedsLayout needs to be called here.

https://codereview.chromium.org/1244113003/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.cc:115: container_->setNeedsLayout();
This call is definitely needed. Here is what can happen otherwise:

1. The plugin WebView is created in WebViewPlugin::Create.
2. Lifecycle methods on the plugin WebView happen that cause it to call
WebWidget::scheduleAnimation() on the WebWidget which contains the WebView. In
this case the WebWidget is the WebViewPlugin class, so
WebViewPlugin::scheduleAnimation is called.
3. However, when WebViewPlugin::scheduleAnimation is called, container_ is still
null since initialize has not yet been called. So the scheduleAnimation is
dropped on the floor [very bad!]
4. WebPlugin::initialize() is called on the WebViewPlugin by the parent frame of
the plugin. This sets container_, and now we can propagate the scheduleAnimation
upward to the parent frame, so we need to do that at this time, hence this CL.

These steps above can happen in various orders, since the steps to initialize a
document involve timers that exist in Blink and don't have a specified order.
e.g. initialize() is called from FrameView::updateWidgets, which is in turn
called by a timer callback.

Unfortunately I was not able to get things to happen in the right order to cause
state to be invalid, but definitely dropping a scheduleAnimate on the floor is
incorrect.

[Stephen Chennney, 2015-07-21 20:58:16]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2015-07-21 20:58:53]

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

[Stephen Chennney, 2015-07-21 21:03:11]

https://codereview.chromium.org/1244113003/diff/1/components/plugins/renderer...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/1244113003/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.cc:114: if (container_) {
On 2015/07/21 20:43:58, chrishtr wrote:
> Please add a comment summarizing why setNeedsLayout needs to be called here.

Done.

https://codereview.chromium.org/1244113003/diff/1/components/plugins/renderer...
components/plugins/renderer/webview_plugin.cc:115: container_->setNeedsLayout();
On 2015/07/21 20:43:58, chrishtr wrote:
> This call is definitely needed. Here is what can happen otherwise:
> 
> 1. The plugin WebView is created in WebViewPlugin::Create.
> 2. Lifecycle methods on the plugin WebView happen that cause it to call
> WebWidget::scheduleAnimation() on the WebWidget which contains the WebView. In
> this case the WebWidget is the WebViewPlugin class, so
> WebViewPlugin::scheduleAnimation is called.
> 3. However, when WebViewPlugin::scheduleAnimation is called, container_ is
still
> null since initialize has not yet been called. So the scheduleAnimation is
> dropped on the floor [very bad!]
> 4. WebPlugin::initialize() is called on the WebViewPlugin by the parent frame
of
> the plugin. This sets container_, and now we can propagate the
scheduleAnimation
> upward to the parent frame, so we need to do that at this time, hence this CL.
> 
> These steps above can happen in various orders, since the steps to initialize
a
> document involve timers that exist in Blink and don't have a specified order.
> e.g. initialize() is called from FrameView::updateWidgets, which is in turn
> called by a timer callback.
> 
> Unfortunately I was not able to get things to happen in the right order to
cause
> state to be invalid, but definitely dropping a scheduleAnimate on the floor is
> incorrect.

Acknowledged.

[commit-bot: I haz the power, 2015-07-21 22:27:00]

Committed patchset #2 (id:20001)

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

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

[chrishtr, 2015-07-22 14:07:23]

lgtm