BatteryManager: release promise resolver once resolved.

BatteryManager: release promise resolver once resolved.

Do not retain the Promise resolver any longer than necessary.

This prevents a reported document leak for detached frame uses of
navigator.getBattery().

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

[sof, 2014-09-30 11:41:25]

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

[sof, 2014-09-30 11:41:26]

Please take a look.

(fast/dom/navigator-{with-content-}detached-no-crash.html have been continually
showing up in local leak reports when testing Oilpan bits. But not specific to
it being enabled, really.)

[haraken, 2014-09-30 12:05:30]

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

[haraken, 2014-09-30 12:05:31]

LGTM

(It would be great if we could have a way to catch this kind of leaks of promise
resolvers.)

[sof, 2014-09-30 12:13:39]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-30 12:14:37]

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

[yhirano, 2014-09-30 12:15:20]

Calling resolve releases the associated resource, so I don't understand why this
CL reduces a leak.

[sof, 2014-09-30 12:20:34]

On 2014/09/30 12:15:20, yhirano wrote:
> Calling resolve releases the associated resource, so I don't understand why
this
> CL reduces a leak.

Does it clear the ScriptState ?

[yhirano, 2014-09-30 12:21:58]

On 2014/09/30 12:20:34, sof wrote:
> On 2014/09/30 12:15:20, yhirano wrote:
> > Calling resolve releases the associated resource, so I don't understand why
> this
> > CL reduces a leak.
> 
> Does it clear the ScriptState ?

No, but IIUC having a ScriptState doesn't lead to leak.
If my understanding is wrong, we should clear it in resolve() / reject().

[commit-bot: I haz the power, 2014-09-30 13:19:41]

Committed patchset #2 (id:20001) as 182921

[haraken, 2014-09-30 14:07:01]

On 2014/09/30 12:21:58, yhirano wrote:
> On 2014/09/30 12:20:34, sof wrote:
> > On 2014/09/30 12:15:20, yhirano wrote:
> > > Calling resolve releases the associated resource, so I don't understand
why
> > this
> > > CL reduces a leak.
> > 
> > Does it clear the ScriptState ?
> 
> No, but IIUC having a ScriptState doesn't lead to leak.
> If my understanding is wrong, we should clear it in resolve() / reject().

Right. Not clearing ScriptState shouldn't cause leaks. ScriptState just has a
weak pointer to a V8 context.

Although ScriptState has another strong pointer to the V8 context (via
ScriptState::m_perContextData), the strong pointer is explicitly cleared when
the context is disposed (in WindowProxy::disposeContext).

[sof, 2014-09-30 14:29:43]

On 2014/09/30 14:07:01, haraken wrote:
> On 2014/09/30 12:21:58, yhirano wrote:
> > On 2014/09/30 12:20:34, sof wrote:
> > > On 2014/09/30 12:15:20, yhirano wrote:
> > > > Calling resolve releases the associated resource, so I don't understand
> why
> > > this
> > > > CL reduces a leak.
> > > 
> > > Does it clear the ScriptState ?
> > 
> > No, but IIUC having a ScriptState doesn't lead to leak.
> > If my understanding is wrong, we should clear it in resolve() / reject().
> 
> Right. Not clearing ScriptState shouldn't cause leaks. ScriptState just has a
> weak pointer to a V8 context.
> 
> Although ScriptState has another strong pointer to the V8 context (via
> ScriptState::m_perContextData), the strong pointer is explicitly cleared when
> the context is disposed (in WindowProxy::disposeContext).

The two leaking tests are now passes,
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...

[yhirano, 2014-10-01 01:58:33]

On 2014/09/30 14:29:43, sof wrote:
> On 2014/09/30 14:07:01, haraken wrote:
> > On 2014/09/30 12:21:58, yhirano wrote:
> > > On 2014/09/30 12:20:34, sof wrote:
> > > > On 2014/09/30 12:15:20, yhirano wrote:
> > > > > Calling resolve releases the associated resource, so I don't
understand
> > why
> > > > this
> > > > > CL reduces a leak.
> > > > 
> > > > Does it clear the ScriptState ?
> > > 
> > > No, but IIUC having a ScriptState doesn't lead to leak.
> > > If my understanding is wrong, we should clear it in resolve() / reject().
> > 
> > Right. Not clearing ScriptState shouldn't cause leaks. ScriptState just has
a
> > weak pointer to a V8 context.
> > 
> > Although ScriptState has another strong pointer to the V8 context (via
> > ScriptState::m_perContextData), the strong pointer is explicitly cleared
when
> > the context is disposed (in WindowProxy::disposeContext).
> 
> The two leaking tests are now passes,
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...

I wrote a unittest at https://codereview.chromium.org/621623002/ . That passes
on my local environment (without oilpan).
Having a ScriptState seems not a problem in the test. What do you think?

[yhirano, 2014-10-02 02:29:51]

On 2014/10/01 01:58:33, yhirano wrote:
> On 2014/09/30 14:29:43, sof wrote:
> > On 2014/09/30 14:07:01, haraken wrote:
> > > On 2014/09/30 12:21:58, yhirano wrote:
> > > > On 2014/09/30 12:20:34, sof wrote:
> > > > > On 2014/09/30 12:15:20, yhirano wrote:
> > > > > > Calling resolve releases the associated resource, so I don't
> understand
> > > why
> > > > > this
> > > > > > CL reduces a leak.
> > > > > 
> > > > > Does it clear the ScriptState ?
> > > > 
> > > > No, but IIUC having a ScriptState doesn't lead to leak.
> > > > If my understanding is wrong, we should clear it in resolve() /
reject().
> > > 
> > > Right. Not clearing ScriptState shouldn't cause leaks. ScriptState just
has
> a
> > > weak pointer to a V8 context.
> > > 
> > > Although ScriptState has another strong pointer to the V8 context (via
> > > ScriptState::m_perContextData), the strong pointer is explicitly cleared
> when
> > > the context is disposed (in WindowProxy::disposeContext).
> > 
> > The two leaking tests are now passes,
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...
> 
> I wrote a unittest at https://codereview.chromium.org/621623002/ . That passes
> on my local environment (without oilpan).
> Having a ScriptState seems not a problem in the test. What do you think?

Proposed a fix: https://codereview.chromium.org/622733002/

[sof, 2014-10-02 05:05:13]

On 2014/10/02 02:29:51, yhirano wrote:
> On 2014/10/01 01:58:33, yhirano wrote:
> > On 2014/09/30 14:29:43, sof wrote:
> > > On 2014/09/30 14:07:01, haraken wrote:
> > > > On 2014/09/30 12:21:58, yhirano wrote:
> > > > > On 2014/09/30 12:20:34, sof wrote:
> > > > > > On 2014/09/30 12:15:20, yhirano wrote:
> > > > > > > Calling resolve releases the associated resource, so I don't
> > understand
> > > > why
> > > > > > this
> > > > > > > CL reduces a leak.
> > > > > > 
> > > > > > Does it clear the ScriptState ?
> > > > > 
> > > > > No, but IIUC having a ScriptState doesn't lead to leak.
> > > > > If my understanding is wrong, we should clear it in resolve() /
> reject().
> > > > 
> > > > Right. Not clearing ScriptState shouldn't cause leaks. ScriptState just
> has
> > a
> > > > weak pointer to a V8 context.
> > > > 
> > > > Although ScriptState has another strong pointer to the V8 context (via
> > > > ScriptState::m_perContextData), the strong pointer is explicitly cleared
> > when
> > > > the context is disposed (in WindowProxy::disposeContext).
> > > 
> > > The two leaking tests are now passes,
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Lea...
> > 
> > I wrote a unittest at https://codereview.chromium.org/621623002/ . That
passes
> > on my local environment (without oilpan).
> > Having a ScriptState seems not a problem in the test. What do you think?
> 
> Proposed a fix: https://codereview.chromium.org/622733002/

Good, let's robustify all users of ScriptPromiseResolver.