Disallow static locals of ScriptWrappable-derived objects.

Disallow static locals of ScriptWrappable-derived objects.

Holding a static Persistent<> to a ScriptWrappable risks retaining the
associated v8 wrapper object (and all it refers to), and risks leaking
information across contexts. For now, disallow them by adding a static assert.

R=
BUG=688569

[sof, 2017-02-07 11:41:03]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 11:41:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-02-07 12:03:38]

Description was changed from

==========
Disallow static locals of ScriptWrappable-derived objects.

Holding a static Persistent<> to a ScriptWrappable risks retaining the
associated v8 wrapper object (and all it refers to), and risks leaking
information across frames. For now, disallow them by adding a static assert.

R=
BUG=688569
==========

to

==========
Disallow static locals of ScriptWrappable-derived objects.

Holding a static Persistent<> to a ScriptWrappable risks retaining the
associated v8 wrapper object (and all it refers to), and risks leaking
information across contexts. For now, disallow them by adding a static assert.

R=
BUG=688569
==========

[sof, 2017-02-07 12:04:21]

sigbjornf@opera.com changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org, haraken@chromium.org

[sof, 2017-02-07 12:04:22]

please take a look.

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

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

[commit-bot: I haz the power, 2017-02-07 13:32:15]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-07 15:47:16]

This verification is nice to have.

I'm just curious but do you have any idea to implement a verification that
detects the following leak?

class Bar : public ScriptWrappable { };

class Foo : public GarbageCollected<Foo> {
  Member<Bar> m_bar;
};

void func() {
  DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
}

[sof, 2017-02-07 16:07:18]

On 2017/02/07 15:47:16, haraken wrote:
> This verification is nice to have.
> 
> I'm just curious but do you have any idea to implement a verification that
> detects the following leak?
> 
> class Bar : public ScriptWrappable { };
> 
> class Foo : public GarbageCollected<Foo> {
>   Member<Bar> m_bar;
> };
> 
> void func() {
>   DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
> }

A no ScriptWrappable-reachable verification would be good to have in place. I
wonder if it would turn up anything?

Will have to think it over, a "no ScriptWrappable scope" that's set up and
checked for by the ScriptWrappable constructor and Member<> might work out..
otherwise the GC plugin should have enough type information, if instructed.
Compile-time errors to be preferred.

[dcheng, 2017-02-07 19:28:08]

On 2017/02/07 16:07:18, sof wrote:
> On 2017/02/07 15:47:16, haraken wrote:
> > This verification is nice to have.
> > 
> > I'm just curious but do you have any idea to implement a verification that
> > detects the following leak?
> > 
> > class Bar : public ScriptWrappable { };
> > 
> > class Foo : public GarbageCollected<Foo> {
> >   Member<Bar> m_bar;
> > };
> > 
> > void func() {
> >   DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
> > }
> 
> A no ScriptWrappable-reachable verification would be good to have in place. I
> wonder if it would turn up anything?
> 
> Will have to think it over, a "no ScriptWrappable scope" that's set up and
> checked for by the ScriptWrappable constructor and Member<> might work out..
> otherwise the GC plugin should have enough type information, if instructed.
> Compile-time errors to be preferred.

One idea that esprehn@ mentioned was porting NeverDestroyed<T> [1] over from
WebKit and using that instead of DEFINE_STATIC_LOCAL.

Then, we could conceivably have the constructor for NeverDestroyed put the
instantiation scope on the check.

[1] https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/NeverDestroyed.h

[sof, 2017-02-07 21:56:08]

On 2017/02/07 19:28:08, dcheng wrote:
> On 2017/02/07 16:07:18, sof wrote:
> > On 2017/02/07 15:47:16, haraken wrote:
> > > This verification is nice to have.
> > > 
> > > I'm just curious but do you have any idea to implement a verification that
> > > detects the following leak?
> > > 
> > > class Bar : public ScriptWrappable { };
> > > 
> > > class Foo : public GarbageCollected<Foo> {
> > >   Member<Bar> m_bar;
> > > };
> > > 
> > > void func() {
> > >   DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
> > > }
> > 
> > A no ScriptWrappable-reachable verification would be good to have in place.
I
> > wonder if it would turn up anything?
> > 
> > Will have to think it over, a "no ScriptWrappable scope" that's set up and
> > checked for by the ScriptWrappable constructor and Member<> might work out..
> > otherwise the GC plugin should have enough type information, if instructed.
> > Compile-time errors to be preferred.
> 
> One idea that esprehn@ mentioned was porting NeverDestroyed<T> [1] over from
> WebKit and using that instead of DEFINE_STATIC_LOCAL.
> 
> Then, we could conceivably have the constructor for NeverDestroyed put the
> instantiation scope on the check.
> 
> [1]
https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/NeverDestroyed.h

Thanks for the pointer, the Oilpan-based singletons "deserve" an abstraction
akin to it by now. I can take a closer look.

[sof, 2017-02-08 21:34:15]

On 2017/02/07 21:56:08, sof wrote:
> On 2017/02/07 19:28:08, dcheng wrote:
> > On 2017/02/07 16:07:18, sof wrote:
> > > On 2017/02/07 15:47:16, haraken wrote:
> > > > This verification is nice to have.
> > > > 
> > > > I'm just curious but do you have any idea to implement a verification
that
> > > > detects the following leak?
> > > > 
> > > > class Bar : public ScriptWrappable { };
> > > > 
> > > > class Foo : public GarbageCollected<Foo> {
> > > >   Member<Bar> m_bar;
> > > > };
> > > > 
> > > > void func() {
> > > >   DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
> > > > }
> > > 
> > > A no ScriptWrappable-reachable verification would be good to have in
place.
> I
> > > wonder if it would turn up anything?
> > > 
> > > Will have to think it over, a "no ScriptWrappable scope" that's set up and
> > > checked for by the ScriptWrappable constructor and Member<> might work
out..
> > > otherwise the GC plugin should have enough type information, if
instructed.
> > > Compile-time errors to be preferred.
> > 
> > One idea that esprehn@ mentioned was porting NeverDestroyed<T> [1] over from
> > WebKit and using that instead of DEFINE_STATIC_LOCAL.
> > 
> > Then, we could conceivably have the constructor for NeverDestroyed put the
> > instantiation scope on the check.
> > 
> > [1]
> https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/NeverDestroyed.h
> 
> Thanks for the pointer, the Oilpan-based singletons "deserve" an abstraction
> akin to it by now. I can take a closer look.

Exploring that in https://codereview.chromium.org/2680843006/ - it didn't end up
particularly clean, but it adds the ScriptWrappable checking.

MouseEventManager::dragState()'s reachable Node + HTMLTableElements
StylePropertySet singletons (they contain embedded CSSStyleDeclaration
references) are two immediate sources of assert failure. I haven't looked into
details if those are effectively benign, or not.

[dcheng, 2017-02-08 22:19:48]

On 2017/02/08 21:34:15, sof wrote:
> On 2017/02/07 21:56:08, sof wrote:
> > On 2017/02/07 19:28:08, dcheng wrote:
> > > On 2017/02/07 16:07:18, sof wrote:
> > > > On 2017/02/07 15:47:16, haraken wrote:
> > > > > This verification is nice to have.
> > > > > 
> > > > > I'm just curious but do you have any idea to implement a verification
> that
> > > > > detects the following leak?
> > > > > 
> > > > > class Bar : public ScriptWrappable { };
> > > > > 
> > > > > class Foo : public GarbageCollected<Foo> {
> > > > >   Member<Bar> m_bar;
> > > > > };
> > > > > 
> > > > > void func() {
> > > > >   DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
> > > > > }
> > > > 
> > > > A no ScriptWrappable-reachable verification would be good to have in
> place.
> > I
> > > > wonder if it would turn up anything?
> > > > 
> > > > Will have to think it over, a "no ScriptWrappable scope" that's set up
and
> > > > checked for by the ScriptWrappable constructor and Member<> might work
> out..
> > > > otherwise the GC plugin should have enough type information, if
> instructed.
> > > > Compile-time errors to be preferred.
> > > 
> > > One idea that esprehn@ mentioned was porting NeverDestroyed<T> [1] over
from
> > > WebKit and using that instead of DEFINE_STATIC_LOCAL.
> > > 
> > > Then, we could conceivably have the constructor for NeverDestroyed put the
> > > instantiation scope on the check.
> > > 
> > > [1]
> > https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/NeverDestroyed.h
> > 
> > Thanks for the pointer, the Oilpan-based singletons "deserve" an abstraction
> > akin to it by now. I can take a closer look.
> 
> Exploring that in https://codereview.chromium.org/2680843006/ - it didn't end
up
> particularly clean, but it adds the ScriptWrappable checking.
> 
> MouseEventManager::dragState()'s reachable Node + HTMLTableElements
> StylePropertySet singletons (they contain embedded CSSStyleDeclaration
> references) are two immediate sources of assert failure. I haven't looked into
> details if those are effectively benign, or not.

The MouseEventManager one is safe as long as the rest of the drag-and-drop code
is bug-free =P

When we start a drag, we /should/ be resetting the source node and the
DataTransfer: in theory, there won't be cross-context leaks as long as we
correctly manage this...

[sof, 2017-02-09 15:38:44]

On 2017/02/08 22:19:48, dcheng wrote:
> On 2017/02/08 21:34:15, sof wrote:
> > On 2017/02/07 21:56:08, sof wrote:
> > > On 2017/02/07 19:28:08, dcheng wrote:
> > > > On 2017/02/07 16:07:18, sof wrote:
> > > > > On 2017/02/07 15:47:16, haraken wrote:
> > > > > > This verification is nice to have.
> > > > > > 
> > > > > > I'm just curious but do you have any idea to implement a
verification
> > that
> > > > > > detects the following leak?
> > > > > > 
> > > > > > class Bar : public ScriptWrappable { };
> > > > > > 
> > > > > > class Foo : public GarbageCollected<Foo> {
> > > > > >   Member<Bar> m_bar;
> > > > > > };
> > > > > > 
> > > > > > void func() {
> > > > > >   DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
> > > > > > }
> > > > > 
> > > > > A no ScriptWrappable-reachable verification would be good to have in
> > place.
> > > I
> > > > > wonder if it would turn up anything?
> > > > > 
> > > > > Will have to think it over, a "no ScriptWrappable scope" that's set up
> and
> > > > > checked for by the ScriptWrappable constructor and Member<> might work
> > out..
> > > > > otherwise the GC plugin should have enough type information, if
> > instructed.
> > > > > Compile-time errors to be preferred.
> > > > 
> > > > One idea that esprehn@ mentioned was porting NeverDestroyed<T> [1] over
> from
> > > > WebKit and using that instead of DEFINE_STATIC_LOCAL.
> > > > 
> > > > Then, we could conceivably have the constructor for NeverDestroyed put
the
> > > > instantiation scope on the check.
> > > > 
> > > > [1]
> > >
https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/NeverDestroyed.h
> > > 
> > > Thanks for the pointer, the Oilpan-based singletons "deserve" an
abstraction
> > > akin to it by now. I can take a closer look.
> > 
> > Exploring that in https://codereview.chromium.org/2680843006/ - it didn't
end
> up
> > particularly clean, but it adds the ScriptWrappable checking.
> > 
> > MouseEventManager::dragState()'s reachable Node + HTMLTableElements
> > StylePropertySet singletons (they contain embedded CSSStyleDeclaration
> > references) are two immediate sources of assert failure. I haven't looked
into
> > details if those are effectively benign, or not.
> 
> The MouseEventManager one is safe as long as the rest of the drag-and-drop
code
> is bug-free =P
> 
> When we start a drag, we /should/ be resetting the source node and the
> DataTransfer: in theory, there won't be cross-context leaks as long as we
> correctly manage this...

Yes, we have to resort to code reasoning for cases where such references are
safe & valid.

https://codereview.chromium.org/2680843006/ is now complete, but I'm leaning
towards this being too much machinery to add.

[dcheng, 2017-02-09 18:28:45]

On 2017/02/09 15:38:44, sof wrote:
> On 2017/02/08 22:19:48, dcheng wrote:
> > On 2017/02/08 21:34:15, sof wrote:
> > > On 2017/02/07 21:56:08, sof wrote:
> > > > On 2017/02/07 19:28:08, dcheng wrote:
> > > > > On 2017/02/07 16:07:18, sof wrote:
> > > > > > On 2017/02/07 15:47:16, haraken wrote:
> > > > > > > This verification is nice to have.
> > > > > > > 
> > > > > > > I'm just curious but do you have any idea to implement a
> verification
> > > that
> > > > > > > detects the following leak?
> > > > > > > 
> > > > > > > class Bar : public ScriptWrappable { };
> > > > > > > 
> > > > > > > class Foo : public GarbageCollected<Foo> {
> > > > > > >   Member<Bar> m_bar;
> > > > > > > };
> > > > > > > 
> > > > > > > void func() {
> > > > > > >   DEFINE_STATIC_LOCAL(Persistent<Foo>, foo, new Foo);
> > > > > > > }
> > > > > > 
> > > > > > A no ScriptWrappable-reachable verification would be good to have in
> > > place.
> > > > I
> > > > > > wonder if it would turn up anything?
> > > > > > 
> > > > > > Will have to think it over, a "no ScriptWrappable scope" that's set
up
> > and
> > > > > > checked for by the ScriptWrappable constructor and Member<> might
work
> > > out..
> > > > > > otherwise the GC plugin should have enough type information, if
> > > instructed.
> > > > > > Compile-time errors to be preferred.
> > > > > 
> > > > > One idea that esprehn@ mentioned was porting NeverDestroyed<T> [1]
over
> > from
> > > > > WebKit and using that instead of DEFINE_STATIC_LOCAL.
> > > > > 
> > > > > Then, we could conceivably have the constructor for NeverDestroyed put
> the
> > > > > instantiation scope on the check.
> > > > > 
> > > > > [1]
> > > >
> https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/NeverDestroyed.h
> > > > 
> > > > Thanks for the pointer, the Oilpan-based singletons "deserve" an
> abstraction
> > > > akin to it by now. I can take a closer look.
> > > 
> > > Exploring that in https://codereview.chromium.org/2680843006/ - it didn't
> end
> > up
> > > particularly clean, but it adds the ScriptWrappable checking.
> > > 
> > > MouseEventManager::dragState()'s reachable Node + HTMLTableElements
> > > StylePropertySet singletons (they contain embedded CSSStyleDeclaration
> > > references) are two immediate sources of assert failure. I haven't looked
> into
> > > details if those are effectively benign, or not.
> > 
> > The MouseEventManager one is safe as long as the rest of the drag-and-drop
> code
> > is bug-free =P
> > 
> > When we start a drag, we /should/ be resetting the source node and the
> > DataTransfer: in theory, there won't be cross-context leaks as long as we
> > correctly manage this...
> 
> Yes, we have to resort to code reasoning for cases where such references are
> safe & valid.
> 
> https://codereview.chromium.org/2680843006/ is now complete, but I'm leaning
> towards this being too much machinery to add.

We could just simplify by removing the exceptions right? For example, we could
move DragState to something like DragController and have it just be a normal
member.

[sof, 2017-02-09 20:26:04]

On 2017/02/09 18:28:45, dcheng wrote:
> On 2017/02/09 15:38:44, sof wrote:
...
> > > > 
> > > > MouseEventManager::dragState()'s reachable Node + HTMLTableElements
> > > > StylePropertySet singletons (they contain embedded CSSStyleDeclaration
> > > > references) are two immediate sources of assert failure. I haven't
looked
> > into
> > > > details if those are effectively benign, or not.
> > > 
> > > The MouseEventManager one is safe as long as the rest of the drag-and-drop
> > code
> > > is bug-free =P
> > > 
> > > When we start a drag, we /should/ be resetting the source node and the
> > > DataTransfer: in theory, there won't be cross-context leaks as long as we
> > > correctly manage this...
> > 
> > Yes, we have to resort to code reasoning for cases where such references are
> > safe & valid.
> > 
> > https://codereview.chromium.org/2680843006/ is now complete, but I'm leaning
> > towards this being too much machinery to add.
> 
> We could just simplify by removing the exceptions right? For example, we could
> move DragState to something like DragController and have it just be a normal
> member.

Right, even as a straight Member<> of MouseEventManager? For some of the other
cases, it's not so easy & natural.

We could actively discourage embedding ScriptWrappables in singletons via
documentation, but an opt-out mechanism is needed, I'd say.

[sof, 2017-02-10 21:54:19]

On 2017/02/09 20:26:04, sof wrote:
> On 2017/02/09 18:28:45, dcheng wrote:
> > On 2017/02/09 15:38:44, sof wrote:
> ...
> > > > > 
> > > > > MouseEventManager::dragState()'s reachable Node + HTMLTableElements
> > > > > StylePropertySet singletons (they contain embedded CSSStyleDeclaration
> > > > > references) are two immediate sources of assert failure. I haven't
> looked
> > > into
> > > > > details if those are effectively benign, or not.
> > > > 
> > > > The MouseEventManager one is safe as long as the rest of the
drag-and-drop
> > > code
> > > > is bug-free =P
> > > > 
> > > > When we start a drag, we /should/ be resetting the source node and the
> > > > DataTransfer: in theory, there won't be cross-context leaks as long as
we
> > > > correctly manage this...
> > > 
> > > Yes, we have to resort to code reasoning for cases where such references
are
> > > safe & valid.
> > > 
> > > https://codereview.chromium.org/2680843006/ is now complete, but I'm
leaning
> > > towards this being too much machinery to add.
> > 
> > We could just simplify by removing the exceptions right? For example, we
could
> > move DragState to something like DragController and have it just be a normal
> > member.
> 
> Right, even as a straight Member<> of MouseEventManager? For some of the other
> cases, it's not so easy & natural.
> 
> We could actively discourage embedding ScriptWrappables in singletons via
> documentation, but an opt-out mechanism is needed, I'd say.

Instead of this CL, I propose doing https://codereview.chromium.org/2687193004/
+ https://codereview.chromium.org/2680843006/

[esprehn, 2017-04-07 10:29:08]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org
- esprehn@chromium.org