Fix pepper crash when reattaching plugins in style recalc

Fix pepper crash when reattaching plugins in style recalc

Method document::updateLayoutTree creates ScriptForbiddenScope.

An HTML plugin can be reattached while updating layout tree. Pepper plugin
can be destroyed while reattaching and executes some scripts. This execution
produces crash on ScriptForbidden assert.

BUG=550427
R=pfeldman@chromium.org,dcheng@chromium.org,dgozman@chromium.org

Committed: https://crrev.com/4d5765f4066be1198558f6c07ebd5f53ea74be8e
Cr-Commit-Position: refs/heads/master@{#376084}

[kozy, 2016-02-12 18:26:41]

ptal

[kozy, 2016-02-12 18:28:09]

Patchset #1 (id:1) has been deleted

[kozy, 2016-02-12 18:32:31]

Description was changed from

==========
Fix pepper crash

Method document::updateLayoutTree creates ScriptForbiddenScope.
HTMLPlugin can be reatached while updating layout tree.
Pepper plugin can be destroyed while reattaching and executes some scripts. This
execution produces crash on ScriptForbidden assert.

BUG=550427
R=pfeldman@chromium.org
==========

to

==========
Fix pepper crash

Method document::updateLayoutTree creates ScriptForbiddenScope.
HTMLPlugin can be reatached while updating layout tree.
Pepper plugin can be destroyed while reattaching and executes some scripts. This
execution produces crash on ScriptForbidden assert.

BUG=550427
R=pfeldman@chromium.org,dcheng@chromium.org,dgozman@chromium.org
==========

[kozy, 2016-02-12 18:34:58]

@dchen, please take a look!
PluginScriptForbiddenScope was introduced by you in
https://codereview.chromium.org/1170413003 .

[kozy, 2016-02-12 18:35:15]

kozyatinskiy@chromium.org changed reviewers:
+ dcheng@chromium.org, dgozman@chromium.org

[dcheng, 2016-02-12 19:11:52]

dcheng@chromium.org changed reviewers:
+ esprehn@chromium.org

[dcheng, 2016-02-12 19:11:52]

+esprehn

How frequent is this crash?

The right fix (but much more complicated) is to defer all widget updates to the
message loop: https://crbug.com/561683. Destroying plugins inside layout has
always been problematic...

[kozy, 2016-02-12 19:56:44]

On 2016/02/12 19:11:52, dcheng wrote:
> +esprehn
> 
> How frequent is this crash?
> 
> The right fix (but much more complicated) is to defer all widget updates to
the
> message loop: https://crbug.com/561683. Destroying plugins inside layout has
> always been problematic...

It's at least 0.11% on 50.0.2646.0 for stack traces that calls
blink::V8ScriptRunner::callInternalFunction after destroying plugin.
< 0.01% on 49.0.2623.28
  0.02% on 48.0.2564.97

Speculative idea: maybe we can use WebLocalFrame::requestRunTask API for
destroying plugin.

[kozy, 2016-02-12 20:04:45]

On 2016/02/12 19:56:44, kozyatinskiy wrote:
> On 2016/02/12 19:11:52, dcheng wrote:
> > +esprehn
> > 
> > How frequent is this crash?
> > 
> > The right fix (but much more complicated) is to defer all widget updates to
> the
> > message loop: https://crbug.com/561683. Destroying plugins inside layout has
> > always been problematic...
> 
> It's at least 0.11% on 50.0.2646.0 for stack traces that calls
> blink::V8ScriptRunner::callInternalFunction after destroying plugin.
> < 0.01% on 49.0.2623.28
>   0.02% on 48.0.2564.97
> 
> Speculative idea: maybe we can use WebLocalFrame::requestRunTask API for
> destroying plugin.

Like extracting function body of content::PepperWebPluginImpl::destroy() to
separate task with resources's links and post this task to
WebLocalFrame::requestRunTask.
If it sounds reasonable for you, I can try to do it.

[dcheng, 2016-02-12 20:09:12]

On 2016/02/12 at 20:04:45, kozyatinskiy wrote:
> On 2016/02/12 19:56:44, kozyatinskiy wrote:
> > On 2016/02/12 19:11:52, dcheng wrote:
> > > +esprehn
> > > 
> > > How frequent is this crash?
> > > 
> > > The right fix (but much more complicated) is to defer all widget updates
to
> > the
> > > message loop: https://crbug.com/561683. Destroying plugins inside layout
has
> > > always been problematic...
> > 
> > It's at least 0.11% on 50.0.2646.0 for stack traces that calls
> > blink::V8ScriptRunner::callInternalFunction after destroying plugin.
> > < 0.01% on 49.0.2623.28
> >   0.02% on 48.0.2564.97
> > 
> > Speculative idea: maybe we can use WebLocalFrame::requestRunTask API for
> > destroying plugin.
> 
> Like extracting function body of content::PepperWebPluginImpl::destroy() to
separate task with resources's links and post this task to
WebLocalFrame::requestRunTask.
> If it sounds reasonable for you, I can try to do it.

Ideally, we wouldn't have UpdateSuspendScope at all: it would just always be
deferred. This patch LGTM as an interim solution, but I'd like Elliot to confirm
=)

https://codereview.chromium.org/1693003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1693003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1735: PluginScriptForbiddenScope
pluginForbidScript;
Note that the reason this works is because some parts of Blink now assert that
we're not in a script forbidden scope before trying to execute script:
previously, we would just return and do nothing.

PluginScriptForbiddenScope is ultimately used by code that implements WebPlugin
in content (?): if //content detects one of these is on the stack, it just won't
try to execute script at all, bypassing the asserts.

[pfeldman, 2016-02-17 01:20:31]

https://codereview.chromium.org/1693003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1693003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1735: PluginScriptForbiddenScope
pluginForbidScript;
How would /core contributor know when to use ScriptForbidden vs
PluginScriptForbidden and why?

[kozy, 2016-02-17 20:28:53]

Elliot, please take a look.

[esprehn, 2016-02-18 02:17:30]

lgtm

https://codereview.chromium.org/1693003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1693003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1735: PluginScriptForbiddenScope
pluginForbidScript;
On 2016/02/17 at 01:20:31, pfeldman wrote:
> How would /core contributor know when to use ScriptForbidden vs
PluginScriptForbidden and why?

PluginScriptForbidden is poorly named, it's more like
PluginDOMScriptingDisabler. It allows plugin scripts to still run, but they're
forbidden from calling back into blink to execute script I think.

In the case of this patch it works by breaking some things in flash probably...
but that's also probably fine. Flash is buggy that it's messing with the page
inside creation/destruction.

[esprehn, 2016-02-18 02:18:53]

Description was changed from

==========
Fix pepper crash

Method document::updateLayoutTree creates ScriptForbiddenScope.
HTMLPlugin can be reatached while updating layout tree.
Pepper plugin can be destroyed while reattaching and executes some scripts. This
execution produces crash on ScriptForbidden assert.

BUG=550427
R=pfeldman@chromium.org,dcheng@chromium.org,dgozman@chromium.org
==========

to

==========
Fix pepper crash when reattaching plugins in style recalc

Method document::updateLayoutTree creates ScriptForbiddenScope.

An HTML plugin can be reattached while updating layout tree. Pepper plugin
can be destroyed while reattaching and executes some scripts. This execution
produces crash on ScriptForbidden assert.

BUG=550427
R=pfeldman@chromium.org,dcheng@chromium.org,dgozman@chromium.org
==========

[kozy, 2016-02-18 02:30:46]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-02-18 02:31:10]

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

[commit-bot: I haz the power, 2016-02-18 03:55:18]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2016-02-18 03:56:46]

Description was changed from

==========
Fix pepper crash when reattaching plugins in style recalc

Method document::updateLayoutTree creates ScriptForbiddenScope.

An HTML plugin can be reattached while updating layout tree. Pepper plugin
can be destroyed while reattaching and executes some scripts. This execution
produces crash on ScriptForbidden assert.

BUG=550427
R=pfeldman@chromium.org,dcheng@chromium.org,dgozman@chromium.org
==========

to

==========
Fix pepper crash when reattaching plugins in style recalc

Method document::updateLayoutTree creates ScriptForbiddenScope.

An HTML plugin can be reattached while updating layout tree. Pepper plugin
can be destroyed while reattaching and executes some scripts. This execution
produces crash on ScriptForbidden assert.

BUG=550427
R=pfeldman@chromium.org,dcheng@chromium.org,dgozman@chromium.org

Committed: https://crrev.com/4d5765f4066be1198558f6c07ebd5f53ea74be8e
Cr-Commit-Position: refs/heads/master@{#376084}
==========

[commit-bot: I haz the power, 2016-02-18 03:56:47]

Patchset 1 (id:??) landed as
https://crrev.com/4d5765f4066be1198558f6c07ebd5f53ea74be8e
Cr-Commit-Position: refs/heads/master@{#376084}