Suppress ScriptForbiddenScope in Document.cpp and FrameView.cpp in release mode

Suppress ScriptForbiddenScope in Document.cpp and FrameView.cpp in release mode

r178351 enabled ScriptForbiddenScope in release builds, then we hit a lot of crashes from real-world plugins (see the below bugs). We should suppress the ScriptForbiddenScopes in Document.cpp and FrameView.cpp in release builds.

BUG=395299, 395081, 395164
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178518

[haraken, 2014-07-19 01:18:29]

PTAL

This CL fixes a couple of release-block bugs.

[esprehn, 2014-07-19 02:09:54]

Can we put those a lot father down the stacks where the real script entry
from plugins is?

On Friday, July 18, 2014, <haraken@chromium.org> wrote:

> Reviewers: esprehn, abarth,
>
> Message:
> PTAL
>
> This CL fixes a couple of release-block bugs.
>
>
> Description:
> Suppress ScriptForbiddenScope in Document.cpp and FrameView.cpp in release
> mode
>
> r178351 enabled ScriptForbiddenScope in release builds, then we hit a lot
> of
> crashes from real-world plugins (see the below bugs). We should suppress
> the
> ScriptForbiddenScopes in Document.cpp and FrameView.cpp in release builds.
>
> BUG=395299,395081,395164
>
> Please review this at https://codereview.chromium.org/403133002/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+23, -0 lines):
>   M Source/core/dom/Document.cpp
>   M Source/core/frame/FrameView.cpp
>   M Source/core/rendering/compositing/RenderLayerCompositor.cpp
>   M Source/platform/ScriptForbiddenScope.h
>
>
> Index: Source/core/dom/Document.cpp
> diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
> index 49356a4fe39fc01647d07b10cc2f83cb3148da23..
> 345d03c8e3b193bb56d92953ea68df573f5f18ac 100644
> --- a/Source/core/dom/Document.cpp
> +++ b/Source/core/dom/Document.cpp
> @@ -1634,6 +1634,7 @@ bool Document::hasPendingForcedStyleRecalc() const
>  void Document::updateDistributionIfNeeded()
>  {
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      if (!childNeedsDistributionRecalc())
>          return;
> @@ -1644,6 +1645,7 @@ void Document::updateDistributionIfNeeded()
>  void Document::updateStyleInvalidationIfNeeded()
>  {
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      if (!isActive())
>          return;
> @@ -1658,6 +1660,7 @@ void Document::updateStyleInvalidationIfNeeded()
>  void Document::updateDistributionForNodeIfNeeded(Node* node)
>  {
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      if (node->inDocument()) {
>          updateDistributionIfNeeded();
> @@ -1776,6 +1779,7 @@ void Document::updateRenderTree(StyleRecalcChange
> change)
>      ASSERT(isMainThread());
>
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      if (change != Force && !needsRenderTreeUpdate())
>          return;
> @@ -1922,6 +1926,7 @@ void Document::updateLayout()
>      ASSERT(isMainThread());
>
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      RefPtr<FrameView> frameView = view();
>      if (frameView && frameView->isInPerformLayout()) {
> @@ -2106,6 +2111,7 @@ void
Document::unscheduleUseShadowTreeUpdate(SVGUseElement&
> element)
>  void Document::updateUseShadowTreesIfNeeded()
>  {
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      if (m_useElementsNeedingUpdate.isEmpty())
>          return;
> Index: Source/core/frame/FrameView.cpp
> diff --git a/Source/core/frame/FrameView.cpp
> b/Source/core/frame/FrameView.cpp
> index 5dfe396c492c156d84ea8364dfced26f23cce545..
> 18d76dd3008eec7d44632980c832d8897f155990 100644
> --- a/Source/core/frame/FrameView.cpp
> +++ b/Source/core/frame/FrameView.cpp
> @@ -767,6 +767,7 @@ void FrameView::performLayout(RenderObject*
> rootForThisLayout, bool inSubtreeLay
>      TRACE_EVENT0("blink", "FrameView::performLayout");
>
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      ASSERT(!isInPerformLayout());
>      lifecycle().advanceTo(DocumentLifecycle::InPerformLayout);
> @@ -835,6 +836,7 @@ void FrameView::layout(bool allowSubtree)
>      ASSERT(m_frame->page());
>
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      if (isInPerformLayout() || !m_frame->document()->isActive())
>          return;
> Index: Source/core/rendering/compositing/RenderLayerCompositor.cpp
> diff --git a/Source/core/rendering/compositing/RenderLayerCompositor.cpp
> b/Source/core/rendering/compositing/RenderLayerCompositor.cpp
> index b8d830f4fb857407374ab94baa29da8619e919a1..
> 6688618837a66e47c010081acefa9603c0839921 100644
> --- a/Source/core/rendering/compositing/RenderLayerCompositor.cpp
> +++ b/Source/core/rendering/compositing/RenderLayerCompositor.cpp
> @@ -192,6 +192,7 @@ void RenderLayerCompositor::updateIfNeededRecursive()
>      ASSERT(!m_renderView.needsLayout());
>
>      ScriptForbiddenScope forbidScript;
> +    ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
>
>      // FIXME: enableCompositingModeIfNeeded can trigger a
> CompositingUpdateRebuildTree,
>      // which asserts that it's not InCompositingUpdate.
> Index: Source/platform/ScriptForbiddenScope.h
> diff --git a/Source/platform/ScriptForbiddenScope.h b/Source/platform/
> ScriptForbiddenScope.h
> index 0d60c90cb0d942e02ea00a17a2542ab4ac8d7742..
> bd76cc6560205887670d2c5b267dc5549b206b0e 100644
> --- a/Source/platform/ScriptForbiddenScope.h
> +++ b/Source/platform/ScriptForbiddenScope.h
> @@ -24,6 +24,20 @@ public:
>          TemporaryChange<unsigned> m_change;
>      };
>
> +    // FIXME: This should be removed. SuppressScriptForbiddenScopeInRelease
> is used
> +    // to exceptionally allow script execution in ScriptForbiddenScope,
> because
> +    // some real-world plugins try to execute script in
> ScriptForbiddenScope.
> +    // This is unsafe and we should get rid of all the unsafe script
> executions.
> +    class PLATFORM_EXPORT SuppressScriptForbiddenScopeInRelease {
> +    public:
> +        SuppressScriptForbiddenScopeInRelease();
> +        ~SuppressScriptForbiddenScopeInRelease();
> +#if !ASSERT_ENABLED
> +    private:
> +        TemporaryChange<unsigned> m_change;
> +#endif
> +    };
> +
>      static bool isScriptForbidden();
>  };
>
>
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2014-07-19 03:32:45]

> Can we put those a lot father down the stacks where the real script entry
> from plugins is?

Done. PTAL.

[haraken, 2014-07-19 03:39:49]

https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:687:
ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
thisSuppressionShouldBeRemoved;

It's scary we're invoking JavaScript inside a destructor...

See a stack trace: https://code.google.com/p/chromium/issues/detail?id=395299

sof@: Probably is this problematic in oilpan? ~WebPluginContainerImpl() can
execute arbitrary JavaScript.

Either way I think we should land this CL sooner to fix outstanding release
block bugs.

[sof, 2014-07-19 06:14:44]

https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.cpp:687:
ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
thisSuppressionShouldBeRemoved;
On 2014/07/19 03:39:49, haraken wrote:
> 
> It's scary we're invoking JavaScript inside a destructor...
> 
> See a stack trace: https://code.google.com/p/chromium/issues/detail?id=395299
> 

I can't access that one (Cc: me if you like, might help..), but I'm guessing
this happens when allowing scripting when calling PluginDestroyed on the plugin;
see
content/renderer/npapi/webplugin_impl.cc:WebPluginImpl::TearDownPluginInstance()


> sof@: Probably is this problematic in oilpan? ~WebPluginContainerImpl() can
> execute arbitrary JavaScript.
> 

Calling into v8 during sweeping is definitely problematic. To quote ager when
this came earlier as a possible reason for a crash seen,

   "Entering V8 during sweeping (either weak processing or finalization) can
lead to nested GCs which we do not support."

[haraken, 2014-07-19 06:26:49]

On 2014/07/19 06:14:44, sof wrote:
>
https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
> File Source/web/WebPluginContainerImpl.cpp (right):
> 
>
https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
> Source/web/WebPluginContainerImpl.cpp:687:
> ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> thisSuppressionShouldBeRemoved;
> On 2014/07/19 03:39:49, haraken wrote:
> > 
> > It's scary we're invoking JavaScript inside a destructor...
> > 
> > See a stack trace:
https://code.google.com/p/chromium/issues/detail?id=395299
> > 
> 
> I can't access that one (Cc: me if you like, might help..), but I'm guessing
> this happens when allowing scripting when calling PluginDestroyed on the
plugin;
> see
>
content/renderer/npapi/webplugin_impl.cc:WebPluginImpl::TearDownPluginInstance()

I was ccing sigbjorn@. Now cced to sigbjornf@. It looks like sof@ is not
registered to the issue tracker.

[esprehn, 2014-07-19 06:36:30]

I'll look into fixing these stacks after we've identified them.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof, 2014-07-19 06:38:19]

On 2014/07/19 06:26:49, haraken wrote:
> On 2014/07/19 06:14:44, sof wrote:
> >
>
https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
> > File Source/web/WebPluginContainerImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/403133002/diff/60001/Source/web/WebPluginCont...
> > Source/web/WebPluginContainerImpl.cpp:687:
> > ScriptForbiddenScope::SuppressScriptForbiddenScopeInRelease
> > thisSuppressionShouldBeRemoved;
> > On 2014/07/19 03:39:49, haraken wrote:
> > > 
> > > It's scary we're invoking JavaScript inside a destructor...
> > > 
> > > See a stack trace:
> https://code.google.com/p/chromium/issues/detail?id=395299
> > > 
> > 
> > I can't access that one (Cc: me if you like, might help..), but I'm guessing
> > this happens when allowing scripting when calling PluginDestroyed on the
> plugin;
> > see
> >
>
content/renderer/npapi/webplugin_impl.cc:WebPluginImpl::TearDownPluginInstance()
> 
> I was ccing sigbjorn@. Now cced to sigbjornf@. It looks like sof@ is not
> registered to the issue tracker.

Thank you (a long&twisty story why "sof" isn't.)

Non-owner lgtm to land this particular suppression to unbreak plugins that do
depend on script execution this late, but do we have Oilpan release asserts in
place for catching re-entrancy problems like this?

[haraken, 2014-07-19 06:53:19]

> I'll look into fixing these stacks after we've identified them.

Are you fine with landing the patch set 4 to fix the outstanding release block
bugs at the moment?

[esprehn, 2014-07-19 07:10:37]

On 2014/07/19 06:53:19, haraken wrote:
> > I'll look into fixing these stacks after we've identified them.
> 
> Are you fine with landing the patch set 4 to fix the outstanding release block
> bugs at the moment?

Yes, but can we rename the something scary like AllowSuperUnsafeScript?

Lgtm

[haraken, 2014-07-19 14:57:53]

On 2014/07/19 07:10:37, esprehn wrote:
> On 2014/07/19 06:53:19, haraken wrote:
> > > I'll look into fixing these stacks after we've identified them.
> > 
> > Are you fine with landing the patch set 4 to fix the outstanding release
block
> > bugs at the moment?
> 
> Yes, but can we rename the something scary like AllowSuperUnsafeScript?

Done.

[haraken, 2014-07-19 14:58:11]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-19 14:58:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/403133002/80001

[commit-bot: I haz the power, 2014-07-19 16:12:13]

Change committed as 178518