Add a scoping object to help block scripting during plugin destruction.

Add a scoping object to help block scripting during plugin destruction.

This doesn't block all scripting in plugin destruction: apparently,
being able to script the DOM in an inconsistent state is a "feature" of
Flash.

Instead, the blocks are scoped to instances where the Frame/Document
will be destroyed or replaced shortly afterwards: this is an extension
of the earlier efforts to block synchronous scripting by plugins in
frame detach. The eventual goal is to remove the need to run a nested
message loop in these circumstances, so that the frame navigation and
detach logic can be rewritten to be much easier to reason about.

BUG=498088
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196832

[dcheng, 2015-06-10 01:21:39]

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

[dcheng, 2015-06-10 01:21:40]

The Chrome-side patch is at https://codereview.chromium.org/1172143004.

In order to avoid breaking Flash, I've only very slightly widened the scope when
we block script. Previously, we only block scripting after we're fairly far
along in Frame detach. No one complained about this change.

However, we'd like to avoid running a nested message loop at all during Frame
detach/navigation. This widens the net to also capture Flash content attempting
to script when a Flash plugin element is destroyed by a JS event handler like
unload.

The eventual goal is to eliminate the need to use a sync IPC in certain cases
for plugin destruction.

[haraken, 2015-06-10 02:14:37]

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
File Source/platform/ReentrantScriptForbiddenScope.h (right):

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
Source/platform/ReentrantScriptForbiddenScope.h:12: // Similar to
ScriptForbiddenScope, but more selective. This is intended to help

I don't fully understand the difference. What's an issue of just using
ScriptForbiddenScope for your purpose?

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
Source/platform/ReentrantScriptForbiddenScope.h:38: // a nested message loop.

Great comment!

[dcheng, 2015-06-10 02:18:40]

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
File Source/platform/ReentrantScriptForbiddenScope.h (right):

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
Source/platform/ReentrantScriptForbiddenScope.h:12: // Similar to
ScriptForbiddenScope, but more selective. This is intended to help
On 2015/06/10 at 02:14:37, haraken wrote:
> I don't fully understand the difference. What's an issue of just using
ScriptForbiddenScope for your purpose?

ScriptForbiddenScope would block all script execution.

This would break things like FrameLoader::commitProvisionalLoad, where we still
need JS for unload event handlers to run. The only thing I want to block is
plugins calling back into Blink to execute script.

Sorry this comment is really long, but it's a very messy area and I couldn't
think of a good way to shorten it.

[haraken, 2015-06-10 02:22:19]

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
File Source/platform/ReentrantScriptForbiddenScope.h (right):

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
Source/platform/ReentrantScriptForbiddenScope.h:12: // Similar to
ScriptForbiddenScope, but more selective. This is intended to help
On 2015/06/10 02:18:40, dcheng wrote:
> On 2015/06/10 at 02:14:37, haraken wrote:
> > I don't fully understand the difference. What's an issue of just using
> ScriptForbiddenScope for your purpose?
> 
> ScriptForbiddenScope would block all script execution.
> 
> This would break things like FrameLoader::commitProvisionalLoad, where we
still
> need JS for unload event handlers to run. The only thing I want to block is
> plugins calling back into Blink to execute script.
> 
> Sorry this comment is really long, but it's a very messy area and I couldn't
> think of a good way to shorten it.

Makes sense. If this is a plugin-specific thing, shall we rename
ReentrantScriptForbiddenScope to PluginScriptForbiddenScope?
ReentrantScriptForbiddenScope is confusing because ScriptForbiddenScope is
reentrant as well.

[dcheng, 2015-06-10 03:11:03]

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
File Source/platform/ReentrantScriptForbiddenScope.h (right):

https://codereview.chromium.org/1170413003/diff/1/Source/platform/ReentrantSc...
Source/platform/ReentrantScriptForbiddenScope.h:12: // Similar to
ScriptForbiddenScope, but more selective. This is intended to help
On 2015/06/10 at 02:22:19, haraken wrote:
> On 2015/06/10 02:18:40, dcheng wrote:
> > On 2015/06/10 at 02:14:37, haraken wrote:
> > > I don't fully understand the difference. What's an issue of just using
> > ScriptForbiddenScope for your purpose?
> > 
> > ScriptForbiddenScope would block all script execution.
> > 
> > This would break things like FrameLoader::commitProvisionalLoad, where we
still
> > need JS for unload event handlers to run. The only thing I want to block is
> > plugins calling back into Blink to execute script.
> > 
> > Sorry this comment is really long, but it's a very messy area and I couldn't
> > think of a good way to shorten it.
> 
> Makes sense. If this is a plugin-specific thing, shall we rename
ReentrantScriptForbiddenScope to PluginScriptForbiddenScope?
ReentrantScriptForbiddenScope is confusing because ScriptForbiddenScope is
reentrant as well.

Done.

[haraken, 2015-06-10 03:28:38]

LGTM

https://codereview.chromium.org/1170413003/diff/20001/Source/platform/PluginS...
File Source/platform/PluginScriptForbiddenScope.cpp (right):

https://codereview.chromium.org/1170413003/diff/20001/Source/platform/PluginS...
Source/platform/PluginScriptForbiddenScope.cpp:18:
++s_reentrantScriptForbiddenCount;

s_reentrantScriptForbiddenCount => s_pluginScriptForbiddenCount

[dcheng, 2015-06-10 03:31:48]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-06-10 03:31:48]

https://codereview.chromium.org/1170413003/diff/20001/Source/platform/PluginS...
File Source/platform/PluginScriptForbiddenScope.cpp (right):

https://codereview.chromium.org/1170413003/diff/20001/Source/platform/PluginS...
Source/platform/PluginScriptForbiddenScope.cpp:18:
++s_reentrantScriptForbiddenCount;
On 2015/06/10 at 03:28:38, haraken wrote:
> s_reentrantScriptForbiddenCount => s_pluginScriptForbiddenCount

Done.

[dcheng, 2015-06-10 03:37:33]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-06-10 03:37:34]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1170413003/#ps40001
(title: "Rename static counter")

[commit-bot: I haz the power, 2015-06-10 03:37:38]

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

[commit-bot: I haz the power, 2015-06-10 05:08:36]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196832