Oilpan: move DOMWindow object to the oilpan heap.

Oilpan: move DOMWindow object to the oilpan heap.

Turn the global Window into a garbage collected object.

R=haraken@chromium.org,ager@chromium.org,wibling@chromium.org,zerny@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170065

[sof, 2014-03-25 16:29:37]

Please take a look.

One test is coming through as occasionally flaky
(fast/dom/inline-event-attributes-release -- it counts the number of live
nodes.) It started flaking here when rebasing on top of a trunk having Event as
garbage collected, but didn't before. Yet to verify if it reproduces without
this CL.

[sof, 2014-03-25 19:03:18]

On 2014/03/25 16:29:37, sof wrote:
> Please take a look.
> 
> One test is coming through as occasionally flaky
> (fast/dom/inline-event-attributes-release -- it counts the number of live
> nodes.) It started flaking here when rebasing on top of a trunk having Event
as
> garbage collected, but didn't before. Yet to verify if it reproduces without
> this CL.

That flake is not from this CL; reproduces on trunk.

[haraken, 2014-03-26 01:13:32]

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
File Source/bindings/v8/V8DOMWrapper.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.h:75: inline void
V8DOMWrapper::setNativeInfoGarbageCollected(v8::Handle<v8::Object> wrapper,
const WrapperTypeInfo* type, void* object)

The GC interaction between V8 and oilpan is complicated, so I'm not really happy
to add one more version of setNativeInfo. (We really need to be careful about
what should do in derefObject() -- whether we should call deref() or we should
delete a persistent handle or we should do nothing.)

Instead of introducing setNativeInfoGarbageCollected, can we just use
setNativeInfoWithPersistentHandle? We'll need to create multiple persistent
handles for one DOMWindow wrapper (one persistent handle for one hidden
prototype chain), but it would be OK because # of DOMWindow objects are limited.

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.cpp
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.cpp:509: ASSERT(!m_document ||
m_document->isStopped());

Do we need !m_document? In this CL, I don't think we need it because DOMWindow
keeps a RefPtr to the document.

In the future, m_document will become Member<Document>. At that point, we won't
be able to check m_document->isStopped() anyway since we're not allowed to touch
m_document in DOMWindow's destructor.

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.h:94: class DOMWindow FINAL : public
RefCountedWillBeRefCountedGarbageCollected<DOMWindow>, public ScriptWrappable,
public EventTargetWithInlineData, public FrameDestructionObserver, public
Supplementable<DOMWindow>, public LifecycleContext<DOMWindow> {

Don't you need to use WillBeHeapSupplementable<DOMWindow>? Is it safe to do that
in a separate CL?

[sof, 2014-03-26 06:36:46]

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
File Source/bindings/v8/V8DOMWrapper.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.h:75: inline void
V8DOMWrapper::setNativeInfoGarbageCollected(v8::Handle<v8::Object> wrapper,
const WrapperTypeInfo* type, void* object)
On 2014/03/26 01:13:32, haraken wrote:
> 
> The GC interaction between V8 and oilpan is complicated, so I'm not really
happy
> to add one more version of setNativeInfo. (We really need to be careful about
> what should do in derefObject() -- whether we should call deref() or we should
> delete a persistent handle or we should do nothing.)
> 
> Instead of introducing setNativeInfoGarbageCollected, can we just use
> setNativeInfoWithPersistentHandle? We'll need to create multiple persistent
> handles for one DOMWindow wrapper (one persistent handle for one hidden
> prototype chain), but it would be OK because # of DOMWindow objects are
limited.

I did explore that option. Handful of tests were leaking documents (rather
badly) due to DOMWindows not being released.

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.cpp
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.cpp:509: ASSERT(!m_document ||
m_document->isStopped());
On 2014/03/26 01:13:32, haraken wrote:
> 
> Do we need !m_document? In this CL, I don't think we need it because DOMWindow
> keeps a RefPtr to the document.
> 
> In the future, m_document will become Member<Document>. At that point, we
won't
> be able to check m_document->isStopped() anyway since we're not allowed to
touch
> m_document in DOMWindow's destructor.

It's not needed -- my bad, will remove. (Forgot to remove it as part of
investigating why/how DOMWindows were leaking with the aforementioned use of
persistent handles on the wrapper.)

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.h:94: class DOMWindow FINAL : public
RefCountedWillBeRefCountedGarbageCollected<DOMWindow>, public ScriptWrappable,
public EventTargetWithInlineData, public FrameDestructionObserver, public
Supplementable<DOMWindow>, public LifecycleContext<DOMWindow> {
On 2014/03/26 01:13:32, haraken wrote:
> 
> Don't you need to use WillBeHeapSupplementable<DOMWindow>? Is it safe to do
that
> in a separate CL?

I would prefer to do it later; as long as the supplements don't touch their
DOMWindow when destructing it ought to be fine (and if they were already, that'd
be a problem already, GCed DOMWindow or not). I'll verify.

[Mads Ager (chromium), 2014-03-26 07:29:43]

LGTM if we make the setNativeInfoGarbageCollected more obscure so it doesn't
look like the right thing to do for garbage collected objects in general. :)

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
File Source/bindings/v8/V8DOMWrapper.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.h:75: inline void
V8DOMWrapper::setNativeInfoGarbageCollected(v8::Handle<v8::Object> wrapper,
const WrapperTypeInfo* type, void* object)
On 2014/03/26 06:36:46, sof wrote:
> On 2014/03/26 01:13:32, haraken wrote:
> > 
> > The GC interaction between V8 and oilpan is complicated, so I'm not really
> happy
> > to add one more version of setNativeInfo. (We really need to be careful
about
> > what should do in derefObject() -- whether we should call deref() or we
should
> > delete a persistent handle or we should do nothing.)
> > 
> > Instead of introducing setNativeInfoGarbageCollected, can we just use
> > setNativeInfoWithPersistentHandle? We'll need to create multiple persistent
> > handles for one DOMWindow wrapper (one persistent handle for one hidden
> > prototype chain), but it would be OK because # of DOMWindow objects are
> limited.
> 
> I did explore that option. Handful of tests were leaking documents (rather
> badly) due to DOMWindows not being released.

Yeah, the problem with that is that there is only one real wrapper that gets a
callback. Therefore, the extra allocated persistent handles will never be
deallocated. This was the same before. Only one of these objects (the real
wrapper object) performs a manual 'ref' of the native pointer and a manual
'deref' in the weak callback from V8 when the wrapper is gone. The other one
(inner global object) just has the raw pointer. That is OK because these objects
always live and die together. The inner global object and the DOMWindow cannot
be distinguished in JavaScript (the DOMWindow is a hidden prototype of the inner
global object that you cannot change and cannot get to).

I don't think we should mess with that setup. Maybe we can find a renaming that
makes it clear that this is a special case that we should not call otherwise.
Right now, the name suggests that this is the one you should call for garbage
collected objects. Maybe rename to setNativeInfoForInnerGlobalObject, make it
take a DOMWindow*, and add a comment that it is safe to not use a persistent
because the inner global object and the DOMWindow wrapper die together and the
DOMWindow wrapper holds a Persistent that keeps the DOMWindow alive until they
die?

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.cpp
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.cpp:117: , m_window(&window)
We could add constructors to Persistent and Member that take references. That
has apparently been done for RefPtr. I personally prefer to have the & here so
it is explicit that we are taking the address of the address. But I would be OK
with adding reference constructors as well.

[sof, 2014-03-26 07:44:26]

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.h:94: class DOMWindow FINAL : public
RefCountedWillBeRefCountedGarbageCollected<DOMWindow>, public ScriptWrappable,
public EventTargetWithInlineData, public FrameDestructionObserver, public
Supplementable<DOMWindow>, public LifecycleContext<DOMWindow> {
On 2014/03/26 06:36:46, sof wrote:
> On 2014/03/26 01:13:32, haraken wrote:
> > 
> > Don't you need to use WillBeHeapSupplementable<DOMWindow>? Is it safe to do
> that
> > in a separate CL?
> 
> I would prefer to do it later; as long as the supplements don't touch their
> DOMWindow when destructing it ought to be fine (and if they were already,
that'd
> be a problem already, GCed DOMWindow or not). I'll verify.

The one interesting case is DOMWindowPagePopup::~DOMWindowPagePopup(), which
clears out a pointer on the GCed object it keeps. Currently as a Persistent, so
there's no problem for the time being, but once it becomes a heap supplement,
there's something to address there.

[haraken, 2014-03-26 08:07:16]

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
File Source/bindings/v8/V8DOMWrapper.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.h:75: inline void
V8DOMWrapper::setNativeInfoGarbageCollected(v8::Handle<v8::Object> wrapper,
const WrapperTypeInfo* type, void* object)
On 2014/03/26 07:29:43, Mads Ager (chromium) wrote:
> On 2014/03/26 06:36:46, sof wrote:
> > On 2014/03/26 01:13:32, haraken wrote:
> > > 
> > > The GC interaction between V8 and oilpan is complicated, so I'm not really
> > happy
> > > to add one more version of setNativeInfo. (We really need to be careful
> about
> > > what should do in derefObject() -- whether we should call deref() or we
> should
> > > delete a persistent handle or we should do nothing.)
> > > 
> > > Instead of introducing setNativeInfoGarbageCollected, can we just use
> > > setNativeInfoWithPersistentHandle? We'll need to create multiple
persistent
> > > handles for one DOMWindow wrapper (one persistent handle for one hidden
> > > prototype chain), but it would be OK because # of DOMWindow objects are
> > limited.
> > 
> > I did explore that option. Handful of tests were leaking documents (rather
> > badly) due to DOMWindows not being released.
> 
> Yeah, the problem with that is that there is only one real wrapper that gets a
> callback. Therefore, the extra allocated persistent handles will never be
> deallocated. This was the same before. Only one of these objects (the real
> wrapper object) performs a manual 'ref' of the native pointer and a manual
> 'deref' in the weak callback from V8 when the wrapper is gone. The other one
> (inner global object) just has the raw pointer. That is OK because these
objects
> always live and die together. The inner global object and the DOMWindow cannot
> be distinguished in JavaScript (the DOMWindow is a hidden prototype of the
inner
> global object that you cannot change and cannot get to).
> 
> I don't think we should mess with that setup. Maybe we can find a renaming
that
> makes it clear that this is a special case that we should not call otherwise.
> Right now, the name suggests that this is the one you should call for garbage
> collected objects. Maybe rename to setNativeInfoForInnerGlobalObject, make it
> take a DOMWindow*, and add a comment that it is safe to not use a persistent
> because the inner global object and the DOMWindow wrapper die together and the
> DOMWindow wrapper holds a Persistent that keeps the DOMWindow alive until they
> die?

Yeah. Given that DOMWindow is not the only object that has hidden wrappers
(e.g., HTMLDocument, DedicatedWorkerGlobalScope etc have hidden wrappers as
well), we can probably name it  "setNativeInfoForHiddenWrapper".

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.h:94: class DOMWindow FINAL : public
RefCountedWillBeRefCountedGarbageCollected<DOMWindow>, public ScriptWrappable,
public EventTargetWithInlineData, public FrameDestructionObserver, public
Supplementable<DOMWindow>, public LifecycleContext<DOMWindow> {
On 2014/03/26 07:44:27, sof wrote:
> On 2014/03/26 06:36:46, sof wrote:
> > On 2014/03/26 01:13:32, haraken wrote:
> > > 
> > > Don't you need to use WillBeHeapSupplementable<DOMWindow>? Is it safe to
do
> > that
> > > in a separate CL?
> > 
> > I would prefer to do it later; as long as the supplements don't touch their
> > DOMWindow when destructing it ought to be fine (and if they were already,
> that'd
> > be a problem already, GCed DOMWindow or not). I'll verify.
> 
> The one interesting case is DOMWindowPagePopup::~DOMWindowPagePopup(), which
> clears out a pointer on the GCed object it keeps. Currently as a Persistent,
so
> there's no problem for the time being, but once it becomes a heap supplement,
> there's something to address there.

Makes sense. Agreed with the point that it's safe to handle this in a separate
CL.

[haraken, 2014-03-26 08:16:10]

Double-scanned. LGTM.

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
File Source/bindings/v8/V8DOMWrapper.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.h:54: static inline void
setNativeInfo(v8::Handle<v8::Object>, const WrapperTypeInfo*, void*);

Another thing I'm considering is we want to cause compile error if
setNativeInfo() is called for GarbageCollected objects. Currently it doesn't
cause compiler errors and we cannot notice the issue until we really create a
wrapper and hit the ASSERT in line 68. I have hit the issue a couple of times so
far.

[wibling-chromium, 2014-03-26 08:38:19]

lgtm

[zerny-chromium, 2014-03-26 09:46:24]

lgtm

[sof, 2014-03-26 10:42:03]

Thanks everyone for the careful reviewing.

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
File Source/bindings/v8/V8DOMWrapper.h (right):

https://codereview.chromium.org/211373002/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.h:75: inline void
V8DOMWrapper::setNativeInfoGarbageCollected(v8::Handle<v8::Object> wrapper,
const WrapperTypeInfo* type, void* object)
On 2014/03/26 08:07:16, haraken wrote:
> On 2014/03/26 07:29:43, Mads Ager (chromium) wrote:
...
> > 
> > I don't think we should mess with that setup. Maybe we can find a renaming
> that
> > makes it clear that this is a special case that we should not call
otherwise.
> > Right now, the name suggests that this is the one you should call for
garbage
> > collected objects. Maybe rename to setNativeInfoForInnerGlobalObject, make
it
> > take a DOMWindow*, and add a comment that it is safe to not use a persistent
> > because the inner global object and the DOMWindow wrapper die together and
the
> > DOMWindow wrapper holds a Persistent that keeps the DOMWindow alive until
they
> > die?
> 
> Yeah. Given that DOMWindow is not the only object that has hidden wrappers
> (e.g., HTMLDocument, DedicatedWorkerGlobalScope etc have hidden wrappers as
> well), we can probably name it  "setNativeInfoForHiddenWrapper".

I've renamed it so + added a comment (in V8WindowShell::installDOMWindow())
which
I hope captures reality precisely - please let me know. Thanks for detailed
explanation of the object relationships involved, ager.

The persistent field in this case wasn't being initialized; added that also.

[sof, 2014-03-26 11:51:17]

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow.cpp
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/211373002/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.cpp:117: , m_window(&window)
On 2014/03/26 07:29:43, Mads Ager (chromium) wrote:
> We could add constructors to Persistent and Member that take references. That
> has apparently been done for RefPtr. I personally prefer to have the & here so
> it is explicit that we are taking the address of the address. But I would be
OK
> with adding reference constructors as well.

RefPtr<> has an explicit constructor over references.

I haven't run into the lack of it that often, but it would be consistent to
provide such a version for Persistent&Member also. Unless there are objections,
I/we can address that separately.

[sof, 2014-03-26 15:09:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-26 15:09:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/211373002/260001

[commit-bot: I haz the power, 2014-03-26 15:19:15]

Change committed as 170065