Oilpan: avoid InjectedScriptManager::CallbackData-induced leaks.

Oilpan: avoid InjectedScriptManager::CallbackData-induced leaks.

Move CallbackData to the Oilpan heap, so as to avoid it having to
keep a Persistent<> reference to its InjectedScriptHost.

If left as a persistent, it would leave the v8 wrapper object uncollected
and prevent a clean shutdown.

R=haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192684

[sof, 2015-03-27 13:29:08]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-03-27 13:29:09]

please take a look.

Takes care of the crashes brought up by


https://groups.google.com/a/chromium.org/forum/#!topic/oilpan-reviews/rclxg-m...

[haraken, 2015-03-27 13:35:47]

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

[haraken, 2015-03-27 13:35:48]

LGTM

https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:61:
callbackData->handle.clear();

Do we need this? (Can we rely on the GC to destruct the ScopedPersistent?)

[sof, 2015-03-27 13:54:56]

https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:61:
callbackData->handle.clear();
On 2015/03/27 13:35:48, haraken wrote:
> 
> Do we need this? (Can we rely on the GC to destruct the ScopedPersistent?)

Having it linger until a GC caused v8 asserts on the inspector-protocol tests
upon shutdown. This provides the same lifetime as the OwnPtr-ed CaklbackData
that's currently on trunk. I've not investigated the alternative.

[haraken, 2015-03-27 14:06:40]

On 2015/03/27 13:54:56, sof wrote:
>
https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
> File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):
> 
>
https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
> Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:61:
> callbackData->handle.clear();
> On 2015/03/27 13:35:48, haraken wrote:
> > 
> > Do we need this? (Can we rely on the GC to destruct the ScopedPersistent?)
> 
> Having it linger until a GC caused v8 asserts on the inspector-protocol tests
> upon shutdown. This provides the same lifetime as the OwnPtr-ed CaklbackData
> that's currently on trunk. I've not investigated the alternative.

Makes sense. It might be better to introduce CallbackData::dispose() and
explicitly call in both oilpan and non-oilpan.

[sof, 2015-03-27 14:20:15]

On 2015/03/27 14:06:40, haraken wrote:
> On 2015/03/27 13:54:56, sof wrote:
> >
>
https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
> > File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1040703002/diff/60001/Source/bindings/core/v8...
> > Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:61:
> > callbackData->handle.clear();
> > On 2015/03/27 13:35:48, haraken wrote:
> > > 
> > > Do we need this? (Can we rely on the GC to destruct the ScopedPersistent?)
> > 
> > Having it linger until a GC caused v8 asserts on the inspector-protocol
tests
> > upon shutdown. This provides the same lifetime as the OwnPtr-ed CaklbackData
> > that's currently on trunk. I've not investigated the alternative.
> 
> Makes sense. It might be better to introduce CallbackData::dispose() and
> explicitly call in both oilpan and non-oilpan.

Yes, that would be tidier. Done.

[sof, 2015-03-27 14:45:36]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-03-27 14:45:36]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1040703002/#ps80001
(title: "Add explicit dispose()")

[commit-bot: I haz the power, 2015-03-27 14:46:08]

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

[commit-bot: I haz the power, 2015-03-27 15:49:20]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192684