Disallow wrapper creation from visitDOMWrapper

Disallow wrapper creation from visitDOMWrapper

visitDOMWrapper is called from during V8 GC, and it is not safe to create wrapper there, as it may lead to execution of arbitrary javascript while GC.
This CL removes the wrapper creation code from visitDOMWrapper template code, and replaces it with an ASSERT.

BUG=None

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

[kouhei (in TOK), 2015-02-16 03:47:39]

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

[haraken, 2015-02-16 03:56:50]

https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
File Source/bindings/templates/interface.cpp (left):

https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
Source/bindings/templates/interface.cpp:600:
{{set_wrapper_reference_to.name}}->wrap(creationContext, isolate);

Another idea would be just to skip creating a wrapper (and setting a reference)
if the target wrapper does not exist. Will it work?

[kouhei (in TOK), 2015-02-16 04:03:24]

https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
File Source/bindings/templates/interface.cpp (left):

https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
Source/bindings/templates/interface.cpp:600:
{{set_wrapper_reference_to.name}}->wrap(creationContext, isolate);
On 2015/02/16 03:56:49, haraken wrote:
> 
> Another idea would be just to skip creating a wrapper (and setting a
reference)
> if the target wrapper does not exist. Will it work?

Wouldn't it be too fragile? I prefer an ASSERT here.

[haraken, 2015-02-16 04:45:33]

On 2015/02/16 04:03:24, kouhei wrote:
>
https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
> File Source/bindings/templates/interface.cpp (left):
> 
>
https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
> Source/bindings/templates/interface.cpp:600:
> {{set_wrapper_reference_to.name}}->wrap(creationContext, isolate);
> On 2015/02/16 03:56:49, haraken wrote:
> > 
> > Another idea would be just to skip creating a wrapper (and setting a
> reference)
> > if the target wrapper does not exist. Will it work?
> 
> Wouldn't it be too fragile? I prefer an ASSERT here.

What is the semantics required for [SetWrapperReferenceTo]? Even if we don't
have a target wrapper, do we need to create a wrapper and add a reference to it?
I'm wondering if the semantics would be "we need to keep the target wrapper
alive if the wrapper already exists".

[kouhei (in TOK), 2015-02-16 05:28:37]

Changed to RELEASE_ASSERT.

On 2015/02/16 04:45:33, haraken wrote:
> On 2015/02/16 04:03:24, kouhei wrote:
> >
>
https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
> > File Source/bindings/templates/interface.cpp (left):
> > 
> >
>
https://codereview.chromium.org/931543002/diff/1/Source/bindings/templates/in...
> > Source/bindings/templates/interface.cpp:600:
> > {{set_wrapper_reference_to.name}}->wrap(creationContext, isolate);
> > On 2015/02/16 03:56:49, haraken wrote:
> > > 
> > > Another idea would be just to skip creating a wrapper (and setting a
> > reference)
> > > if the target wrapper does not exist. Will it work?
> > 
> > Wouldn't it be too fragile? I prefer an ASSERT here.
> 
> What is the semantics required for [SetWrapperReferenceTo]? Even if we don't
> have a target wrapper, do we need to create a wrapper and add a reference to
it?
> I'm wondering if the semantics would be "we need to keep the target wrapper
> alive if the wrapper already exists".

Discussed offline. I think it is safer to set the rule as: "If target DOM object
non-null, there must be a existing wrapper for it."

[haraken, 2015-02-16 06:05:40]

LGTM

https://codereview.chromium.org/931543002/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/931543002/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.cpp:599:
RELEASE_ASSERT(DOMDataStore::containsWrapper({{set_wrapper_reference_to.name}},
isolate));

Add a comment on why this needs to be release-asserted.

[kouhei (in TOK), 2015-02-16 06:58:56]

New patchsets have been uploaded after l-g-t-m from haraken@chromium.org

[kouhei (in TOK), 2015-02-16 06:59:50]

On 2015/02/16 06:05:40, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/931543002/diff/20001/Source/bindings/template...
> File Source/bindings/templates/interface.cpp (right):
> 
>
https://codereview.chromium.org/931543002/diff/20001/Source/bindings/template...
> Source/bindings/templates/interface.cpp:599:
>
RELEASE_ASSERT(DOMDataStore::containsWrapper({{set_wrapper_reference_to.name}},
> isolate));
> 
> Add a comment on why this needs to be release-asserted.

Done.

[kouhei (in TOK), 2015-02-16 08:44:18]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-02-16 08:44:30]

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

[commit-bot: I haz the power, 2015-02-16 08:48:12]

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

[michaeln, 2015-02-17 23:08:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/931333003/ by michaeln@chromium.org.

The reason for reverting is: Top crasher on canary with a very elevated crash
rate.

BUG=459312,459086.

[jsbell, 2015-03-24 16:52:20]

Did we ever figure out what was wrong with this CL?