Oilpan: fix debug crashes after r174545.

Oilpan: fix debug crashes after r174545.

R=haraken@chromium.org
BUG=

[haraken, 2014-05-22 12:55:04]

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

https://codereview.chromium.org/296943005/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.cpp:68:
V8DOMWrapper::setNativeInfoWithPersistentHandle(wrapper,
&V8HTMLDocument::wrapperTypeInfo, impl, new Persistent<Node>(impl));

I'm not sure if this is right.

If you create a persistent handle here, you will end up creating two Persistent
handles for the Node. This will leak the Node, because the weak callback for the
Node wrapper will be called only once (because the |wrapper| is a hidden
prototype object). For more details, see the comment #5 in
https://codereview.chromium.org/211373002/

I guess a right fix here would be to call setNativeInfoForHiddenWrapper() here.
What happens if we use setNativeInfoForHiddenWrapper()? (Then I guess we don't
need the #if branch.)

sof@: You were working on handling hidden prototype chains of Window objects,
but do you have any idea on this?

[Mads Ager (chromium), 2014-05-22 13:12:27]

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

https://codereview.chromium.org/296943005/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.cpp:68:
V8DOMWrapper::setNativeInfoWithPersistentHandle(wrapper,
&V8HTMLDocument::wrapperTypeInfo, impl, new Persistent<Node>(impl));
On 2014/05/22 12:55:04, haraken wrote:
> 
> I'm not sure if this is right.
> 
> If you create a persistent handle here, you will end up creating two
Persistent
> handles for the Node. This will leak the Node, because the weak callback for
the
> Node wrapper will be called only once (because the |wrapper| is a hidden
> prototype object). For more details, see the comment #5 in
> https://codereview.chromium.org/211373002/
> 
> I guess a right fix here would be to call setNativeInfoForHiddenWrapper()
here.
> What happens if we use setNativeInfoForHiddenWrapper()? (Then I guess we don't
> need the #if branch.)
> 
> sof@: You were working on handling hidden prototype chains of Window objects,
> but do you have any idea on this?

I have been the cause of confusion locally here. I mistakenly thought that the
setNativeInfo call was the one that was doing the ref increase. That is not
true. This is called from:

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

Therefore, Haraken is right that this should use the version that does not
allocate a persistent. The persistent will be allocated in the method above when
it calls associateObjectWithWrapper which will call
setNativeInfoWithPersistentHandle.

[sof, 2014-05-22 13:19:09]

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

https://codereview.chromium.org/296943005/diff/1/Source/bindings/v8/V8DOMWrap...
Source/bindings/v8/V8DOMWrapper.cpp:68:
V8DOMWrapper::setNativeInfoWithPersistentHandle(wrapper,
&V8HTMLDocument::wrapperTypeInfo, impl, new Persistent<Node>(impl));
On 2014/05/22 12:55:04, haraken wrote:
> 
> I'm not sure if this is right.
> 
> If you create a persistent handle here, you will end up creating two
Persistent
> handles for the Node. This will leak the Node, because the weak callback for
the
> Node wrapper will be called only once (because the |wrapper| is a hidden
> prototype object). For more details, see the comment #5 in
> https://codereview.chromium.org/211373002/
> 
> I guess a right fix here would be to call setNativeInfoForHiddenWrapper()
here.
> What happens if we use setNativeInfoForHiddenWrapper()? (Then I guess we don't
> need the #if branch.)
> 
> sof@: You were working on handling hidden prototype chains of Window objects,
> but do you have any idea on this?

Having a pair of Persistents on the same object does seem identical to the
DOMWindow case, with only one of them being cleared in the end.

[zerny-chromium, 2014-05-23 07:04:00]

Fixed by https://codereview.chromium.org/289283020/

Closing this issue.