Add ScriptForbiddenScope to weak callbacks of DOM wrappers

Add ScriptForbiddenScope to weak callbacks of DOM wrappers

It's dangerous to allow arbitrary JavaScript to run during a V8 GC.

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

[haraken, 2014-07-02 11:17:48]

V8RecursionScope has ASSERT(ScriptForbiddenScope::isScriptForbidden), so the
assert will be caught only in Debug builds. I'm planning to change it to
RELEASE_ASSERT in a follow-up CL.

PTAL.

[jochen (gone - plz use gerrit), 2014-07-02 11:21:54]

https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptWrappable.h:256: ScriptForbiddenScope
forbiddenScope;
why here?

[haraken, 2014-07-02 11:23:13]

https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptWrappable.h:256: ScriptForbiddenScope
forbiddenScope;
On 2014/07/02 11:21:54, jochen wrote:
> why here?

Because this calls releaseObject() for DOM wrappers stored in ScriptWrappable.

[haraken, 2014-07-02 11:23:39]

On 2014/07/02 11:23:13, haraken wrote:
>
https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
> File Source/bindings/core/v8/ScriptWrappable.h (right):
> 
>
https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
> Source/bindings/core/v8/ScriptWrappable.h:256: ScriptForbiddenScope
> forbiddenScope;
> On 2014/07/02 11:21:54, jochen wrote:
> > why here?
> 
> Because this calls releaseObject() for DOM wrappers stored in ScriptWrappable.

Probably are you suggesting to rename the method to clearWrapper or something
like that?

[jochen (gone - plz use gerrit), 2014-07-02 13:06:38]

On 2014/07/02 at 11:23:39, haraken wrote:
> On 2014/07/02 11:23:13, haraken wrote:
> >
https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
> > File Source/bindings/core/v8/ScriptWrappable.h (right):
> > 
> >
https://codereview.chromium.org/368853002/diff/1/Source/bindings/core/v8/Scri...
> > Source/bindings/core/v8/ScriptWrappable.h:256: ScriptForbiddenScope
> > forbiddenScope;
> > On 2014/07/02 11:21:54, jochen wrote:
> > > why here?
> > 
> > Because this calls releaseObject() for DOM wrappers stored in
ScriptWrappable.
> 
> Probably are you suggesting to rename the method to clearWrapper or something
like that?

ah, no, I just misread the diff

lgtm

[haraken, 2014-07-02 13:07:06]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-02 13:08:21]

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

[commit-bot: I haz the power, 2014-07-02 14:07:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13946)

[commit-bot: I haz the power, 2014-07-02 14:50:48]

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

[commit-bot: I haz the power, 2014-07-02 14:50:48]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[haraken, 2014-07-03 01:56:19]

https://codereview.chromium.org/368853002/diff/20001/Source/core/dom/ScriptFo...
File Source/core/dom/ScriptForbiddenScope.cpp (right):

https://codereview.chromium.org/368853002/diff/20001/Source/core/dom/ScriptFo...
Source/core/dom/ScriptForbiddenScope.cpp:20: if (isMainThread())

Since ScriptWrappable and DOMWrapperMap are used in workers as well, I removed
the ASSERT(isMainThread()). I'll consider supporting non-main threads in a
follow-up.

[haraken, 2014-07-03 01:56:22]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-03 01:57:00]

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

[commit-bot: I haz the power, 2014-07-03 03:03:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/28835)

[commit-bot: I haz the power, 2014-07-03 04:10:16]

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

[commit-bot: I haz the power, 2014-07-03 04:10:17]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/28862)

[haraken, 2014-07-03 04:40:58]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-07-03 04:41:54]

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

[commit-bot: I haz the power, 2014-07-03 05:32:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/28901)

[commit-bot: I haz the power, 2014-07-03 07:43:44]

Change committed as 177457

[esprehn, 2014-07-29 23:01:14]

On 2014/07/03 at 07:43:44, commit-bot wrote:
> Change committed as 177457

This is making isMainThread() show up in all the profiles now, I don't think you
want to have that branch. We also probably need to make ScriptForbiddenScope
inline.

[haraken, 2014-07-29 23:22:14]

On 2014/07/29 23:01:14, esprehn wrote:
> On 2014/07/03 at 07:43:44, commit-bot wrote:
> > Change committed as 177457
> 
> This is making isMainThread() show up in all the profiles now, I don't think
you
> want to have that branch. We also probably need to make ScriptForbiddenScope
> inline.

Would it be OK to use a thread-local storage for s_scriptForbiddenCount? I don't
think the performance overhead of accessing thread-local storage would become a
problem.

[abarth-chromium, 2014-07-30 06:01:04]

A revert of this CL has been created in
https://codereview.chromium.org/430763002/ by abarth@chromium.org.

The reason for reverting is: This CL regressed performance.  We'll need to
re-think how to handle this situation.

[abarth-chromium, 2014-07-30 07:03:46]

I failed to revert this CL, but we probably should revert it while we sort out
the performance issues.

[haraken, 2014-07-30 09:35:07]

On 2014/07/30 07:03:46, abarth wrote:
> I failed to revert this CL, but we probably should revert it while we sort out
> the performance issues.

I'll revert it tomorrow and investigate.