Move geolocation timer to frame-specific TaskRunnerTimer.

Move geolocation timer to frame-specific TaskRunnerTimer.

Move geolocation timer |m_timer| to frame-specific TaskRunnerTimer. This
associates it with the geolocation frame's timer task queue. Note that the
geolocation specification under specifies exactly what kind of timeout
should be used, and we will use MiscPlatformAPI. See:

https://www.w3.org/TR/geolocation-API/

This m_timer is used to support, in part, the geolocation
'position options'.

BUG=624694
R=haraken,scheib

Review-Url: https://codereview.chromium.org/2642863002
Cr-Commit-Position: refs/heads/master@{#444794}
Committed: https://chromium.googlesource.com/chromium/src/+/d620cdbebcf22fbc7a76c269cf289ab87f5c646d

[dougt, 2017-01-18 17:15:42]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 17:16:08]

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

[commit-bot: I haz the power, 2017-01-18 19:03:27]

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

[commit-bot: I haz the power, 2017-01-18 19:03:29]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-01-18 19:07:42]

haraken, ptal.

[haraken, 2017-01-18 23:30:56]

Thanks for the contribution!

https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp (right):

https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp:24:
m_timer(TaskRunnerHelper::get(TaskType::Timer, geolocation->frame()),

Do you have any spec that says this should use the timer task source?

Basically the timer task source is for JS timers (setTimeout). If this is an
unspeced task, we should use UnspecedTimer.

[dougt, 2017-01-19 00:16:39]

On 2017/01/18 23:30:56, haraken wrote:
> Thanks for the contribution!
> 
>
https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp (right):
> 
>
https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp:24:
> m_timer(TaskRunnerHelper::get(TaskType::Timer, geolocation->frame()),
> 
> Do you have any spec that says this should use the timer task source?
> 
> Basically the timer task source is for JS timers (setTimeout). If this is an
> unspeced task, we should use UnspecedTimer.

No, the geolocation specification underspecifies exactly what kind of timeout
should be used.  This m_timer is used to support, in part, the geolocation
'position options':

"""
The timeout attribute denotes the maximum length of time (expressed in
milliseconds) that is allowed to pass from the call to getCurrentPosition() or
watchPosition() until the corresponding successCallback is invoked. If the
implementation is unable to successfully acquire a new Position before the given
timeout elapses, and no other errors have occurred in this interval, then the
corresponding errorCallback must be invoked with a PositionError object whose
code attribute is set to TIMEOUT. Note that the time that is spent obtaining the
user permission is not included in the period covered by the timeout attribute.
The timeout attribute only applies to the location acquisition operation.
"""

Given this, should TaskType::UnspecedTimer still be used?

[haraken, 2017-01-19 00:57:25]

On 2017/01/19 00:16:39, dougt wrote:
> On 2017/01/18 23:30:56, haraken wrote:
> > Thanks for the contribution!
> > 
> >
>
https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp:24:
> > m_timer(TaskRunnerHelper::get(TaskType::Timer, geolocation->frame()),
> > 
> > Do you have any spec that says this should use the timer task source?
> > 
> > Basically the timer task source is for JS timers (setTimeout). If this is an
> > unspeced task, we should use UnspecedTimer.
> 
> No, the geolocation specification underspecifies exactly what kind of timeout
> should be used.  This m_timer is used to support, in part, the geolocation
> 'position options':
> 
> """
> The timeout attribute denotes the maximum length of time (expressed in
> milliseconds) that is allowed to pass from the call to getCurrentPosition() or
> watchPosition() until the corresponding successCallback is invoked. If the
> implementation is unable to successfully acquire a new Position before the
given
> timeout elapses, and no other errors have occurred in this interval, then the
> corresponding errorCallback must be invoked with a PositionError object whose
> code attribute is set to TIMEOUT. Note that the time that is spent obtaining
the
> user permission is not included in the period covered by the timeout
attribute.
> The timeout attribute only applies to the location acquisition operation.
> """
> 
> Given this, should TaskType::UnspecedTimer still be used?

Thanks, makes sense. Would you add a link to the spec in the CL description?

If the task is speced but the task source is not specified,
TaskType::MiscPlatformAPI is the one you want to use.

[dougt, 2017-01-19 01:04:53]

Description was changed from

==========
Move geolocation timer to frame-specific TaskRunnerTimer.

Move geolocation timer |m_timer| to frame-specific TaskRunnerTimer. This
associates it with the geolocation frame's timer task queue.

BUG=624694

R=haraken,scheib
==========

to

==========
Move geolocation timer to frame-specific TaskRunnerTimer.

Move geolocation timer |m_timer| to frame-specific TaskRunnerTimer. This
associates it with the geolocation frame's timer task queue. Note that the
geolocation specification under specifies exactly what kind of timeout
should be used, and we will use MiscPlatformAPI. See:

https://www.w3.org/TR/geolocation-API/

This m_timer is used to support, in part, the geolocation
'position options'.


BUG=624694

R=haraken,scheib
==========

[dougt, 2017-01-19 01:05:19]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 01:05:57]

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

[dougt, 2017-01-19 01:11:55]

On 2017/01/19 00:57:25, haraken wrote:
> On 2017/01/19 00:16:39, dougt wrote:
> > On 2017/01/18 23:30:56, haraken wrote:
> > > Thanks for the contribution!
> > > 
> > >
> >
>
https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
> > > File third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2642863002/diff/1/third_party/WebKit/Source/m...
> > > third_party/WebKit/Source/modules/geolocation/GeoNotifier.cpp:24:
> > > m_timer(TaskRunnerHelper::get(TaskType::Timer, geolocation->frame()),
> > > 
> > > Do you have any spec that says this should use the timer task source?
> > > 
> > > Basically the timer task source is for JS timers (setTimeout). If this is
an
> > > unspeced task, we should use UnspecedTimer.
> > 
> > No, the geolocation specification underspecifies exactly what kind of
timeout
> > should be used.  This m_timer is used to support, in part, the geolocation
> > 'position options':
> > 
> > """
> > The timeout attribute denotes the maximum length of time (expressed in
> > milliseconds) that is allowed to pass from the call to getCurrentPosition()
or
> > watchPosition() until the corresponding successCallback is invoked. If the
> > implementation is unable to successfully acquire a new Position before the
> given
> > timeout elapses, and no other errors have occurred in this interval, then
the
> > corresponding errorCallback must be invoked with a PositionError object
whose
> > code attribute is set to TIMEOUT. Note that the time that is spent obtaining
> the
> > user permission is not included in the period covered by the timeout
> attribute.
> > The timeout attribute only applies to the location acquisition operation.
> > """
> > 
> > Given this, should TaskType::UnspecedTimer still be used?
> 
> Thanks, makes sense. Would you add a link to the spec in the CL description?
> 
> If the task is speced but the task source is not specified,
> TaskType::MiscPlatformAPI is the one you want to use.

Yup, done. Thanks.

[haraken, 2017-01-19 01:14:21]

LGTM

[commit-bot: I haz the power, 2017-01-19 02:39:38]

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

[commit-bot: I haz the power, 2017-01-19 02:39:39]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-01-19 06:19:05]

I'm not super familiar here. I think that the comments in *code* could help
justify/explain why this type of timer is used. What instigated this change?
Explain it to future code readers like they don't have any clue what's going on
(as in my case... :) ).

[dougt, 2017-01-19 15:19:32]

On 2017/01/19 06:19:05, scheib wrote:
> I'm not super familiar here. I think that the comments in *code* could help
> justify/explain why this type of timer is used. What instigated this change?
> Explain it to future code readers like they don't have any clue what's going
on
> (as in my case... :) ).

This work is part of finishing the per-frame scheduler (see blink-dev for the
call to action). The CL references the bug # which ties together all of this
work. I tend to think that git blame, at least for understanding why we switched
from one task type to another, is better than annotating in source in all places
where we make this change.

[scheib, 2017-01-19 18:19:36]

On 2017/01/19 15:19:32, dougt wrote:
> On 2017/01/19 06:19:05, scheib wrote:
> > I'm not super familiar here. I think that the comments in *code* could help
> > justify/explain why this type of timer is used. What instigated this change?
> > Explain it to future code readers like they don't have any clue what's going
> on
> > (as in my case... :) ).
> 
> This work is part of finishing the per-frame scheduler (see blink-dev for the
> call to action). The CL references the bug # which ties together all of this
> work. I tend to think that git blame, at least for understanding why we
switched
> from one task type to another, is better than annotating in source in all
places
> where we make this change.

Well LGTM then.

[dougt, 2017-01-19 18:24:35]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-01-19 18:25:02]

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

[commit-bot: I haz the power, 2017-01-19 18:31:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484850275139300,
"parent_rev": "f5a2aee10eb115bb0e2d04b8343ce60b7b6936fa", "commit_rev":
"d620cdbebcf22fbc7a76c269cf289ab87f5c646d"}

[commit-bot: I haz the power, 2017-01-19 18:33:15]

Description was changed from

==========
Move geolocation timer to frame-specific TaskRunnerTimer.

Move geolocation timer |m_timer| to frame-specific TaskRunnerTimer. This
associates it with the geolocation frame's timer task queue. Note that the
geolocation specification under specifies exactly what kind of timeout
should be used, and we will use MiscPlatformAPI. See:

https://www.w3.org/TR/geolocation-API/

This m_timer is used to support, in part, the geolocation
'position options'.


BUG=624694

R=haraken,scheib
==========

to

==========
Move geolocation timer to frame-specific TaskRunnerTimer.

Move geolocation timer |m_timer| to frame-specific TaskRunnerTimer. This
associates it with the geolocation frame's timer task queue. Note that the
geolocation specification under specifies exactly what kind of timeout
should be used, and we will use MiscPlatformAPI. See:

https://www.w3.org/TR/geolocation-API/

This m_timer is used to support, in part, the geolocation
'position options'.

BUG=624694

R=haraken,scheib

Review-Url: https://codereview.chromium.org/2642863002
Cr-Commit-Position: refs/heads/master@{#444794}
Committed:
https://chromium.googlesource.com/chromium/src/+/d620cdbebcf22fbc7a76c269cf28...
==========

[commit-bot: I haz the power, 2017-01-19 18:33:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d620cdbebcf22fbc7a76c269cf28...

[jianli, 2017-01-19 20:45:24]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2640403002/ by jianli@chromium.org.

The reason for reverting is: Seems to be the cause for failed test
fast/dom/Geolocation/window-close-crash.html

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....

[haraken, 2017-01-19 23:58:22]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org, tzik@chromium.org

[haraken, 2017-01-19 23:58:23]

On 2017/01/19 20:45:24, jianli wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/2640403002/ by mailto:jianli@chromium.org.
> 
> The reason for reverting is: Seems to be the cause for failed test
> fast/dom/Geolocation/window-close-crash.html
> 
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....

I don't understand why this CL causes a memory leak like this.

tzik@, dcheng@: Do you have any idea on this?

[haraken, 2017-01-20 00:05:02]

On 2017/01/19 23:58:23, haraken wrote:
> On 2017/01/19 20:45:24, jianli wrote:
> > A revert of this CL (patchset #2 id:20001) has been created in
> > https://codereview.chromium.org/2640403002/ by mailto:jianli@chromium.org.
> > 
> > The reason for reverting is: Seems to be the cause for failed test
> > fast/dom/Geolocation/window-close-crash.html
> > 
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> 
> I don't understand why this CL causes a memory leak like this.
> 
> tzik@, dcheng@: Do you have any idea on this?

Sorry, it turned out that this CL is not a culprit.

[jianli, 2017-01-20 00:08:46]

On 2017/01/20 00:05:02, haraken wrote:
> On 2017/01/19 23:58:23, haraken wrote:
> > On 2017/01/19 20:45:24, jianli wrote:
> > > A revert of this CL (patchset #2 id:20001) has been created in
> > > https://codereview.chromium.org/2640403002/ by mailto:jianli@chromium.org.
> > > 
> > > The reason for reverting is: Seems to be the cause for failed test
> > > fast/dom/Geolocation/window-close-crash.html
> > > 
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > 
> > I don't understand why this CL causes a memory leak like this.
> > 
> > tzik@, dcheng@: Do you have any idea on this?
> 
> Sorry, it turned out that this CL is not a culprit.

But the bot turned green right after this revert was landed. See
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....

[haraken, 2017-01-20 00:09:35]

On 2017/01/20 00:08:46, jianli wrote:
> On 2017/01/20 00:05:02, haraken wrote:
> > On 2017/01/19 23:58:23, haraken wrote:
> > > On 2017/01/19 20:45:24, jianli wrote:
> > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > https://codereview.chromium.org/2640403002/ by
mailto:jianli@chromium.org.
> > > > 
> > > > The reason for reverting is: Seems to be the cause for failed test
> > > > fast/dom/Geolocation/window-close-crash.html
> > > > 
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > 
> > > I don't understand why this CL causes a memory leak like this.
> > > 
> > > tzik@, dcheng@: Do you have any idea on this?
> > 
> > Sorry, it turned out that this CL is not a culprit.
> 
> But the bot turned green right after this revert was landed. See
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....

Sorry again! I was chatting with Dougt and confused. Yes, this CL is a culprit
of the leak.

[dougt, 2017-01-20 00:17:52]

On 2017/01/20 00:09:35, haraken wrote:
> On 2017/01/20 00:08:46, jianli wrote:
> > On 2017/01/20 00:05:02, haraken wrote:
> > > On 2017/01/19 23:58:23, haraken wrote:
> > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > https://codereview.chromium.org/2640403002/ by
> mailto:jianli@chromium.org.
> > > > > 
> > > > > The reason for reverting is: Seems to be the cause for failed test
> > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > 
> > > > I don't understand why this CL causes a memory leak like this.
> > > > 
> > > > tzik@, dcheng@: Do you have any idea on this?
> > > 
> > > Sorry, it turned out that this CL is not a culprit.
> > 
> > But the bot turned green right after this revert was landed. See
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> 
> Sorry again! I was chatting with Dougt and confused. Yes, this CL is a culprit
> of the leak.


Just to be clear -- I was confused, not haraken.

With this cl, the leaking docs are:

- Document 0x302beb765d00 URL:
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
- Document 0x302beb7680b8 URL: about:blank
- Document 0x302beb762848 URL:
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
#LEAK - renderer pid 49525
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})


when constructing the m_timer, frame is valid.  if I null it out, we do not
leak.  That is if I hack this out:

index c7f578b378db..45162b1b00fc 100644
--- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
+++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
@@ -34,8 +34,7 @@ RefPtr<WebTaskRunner> TaskRunnerHelper::get(TaskType type,
LocalFrame* frame) {
     case TaskType::Timer:
     case TaskType::UnspecedTimer:
     case TaskType::MiscPlatformAPI:
-      return frame ? frame->frameScheduler()->timerTaskRunner()
-                   : Platform::current()->currentThread()->getWebTaskRunner();
+      return Platform::current()->currentThread()->getWebTaskRunner();
     case TaskType::UnspecedLoading:
     case TaskType::Networking:
       return frame ? frame->frameScheduler()->loadingTaskRunner()


The leak goes away.

[haraken, 2017-01-20 00:24:06]

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

[haraken, 2017-01-20 00:24:07]

On 2017/01/20 00:17:52, dougt wrote:
> On 2017/01/20 00:09:35, haraken wrote:
> > On 2017/01/20 00:08:46, jianli wrote:
> > > On 2017/01/20 00:05:02, haraken wrote:
> > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > https://codereview.chromium.org/2640403002/ by
> > mailto:jianli@chromium.org.
> > > > > > 
> > > > > > The reason for reverting is: Seems to be the cause for failed test
> > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > 
> > > > > I don't understand why this CL causes a memory leak like this.
> > > > > 
> > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > 
> > > > Sorry, it turned out that this CL is not a culprit.
> > > 
> > > But the bot turned green right after this revert was landed. See
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > 
> > Sorry again! I was chatting with Dougt and confused. Yes, this CL is a
culprit
> > of the leak.
> 
> 
> Just to be clear -- I was confused, not haraken.
> 
> With this cl, the leaking docs are:
> 
> - Document 0x302beb765d00 URL:
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> - Document 0x302beb7680b8 URL: about:blank
> - Document 0x302beb762848 URL:
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> #LEAK - renderer pid 49525
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> 
> 
> when constructing the m_timer, frame is valid.  if I null it out, we do not
> leak.  That is if I hack this out:
> 
> index c7f578b378db..45162b1b00fc 100644
> --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner> TaskRunnerHelper::get(TaskType type,
> LocalFrame* frame) {
>      case TaskType::Timer:
>      case TaskType::UnspecedTimer:
>      case TaskType::MiscPlatformAPI:
> -      return frame ? frame->frameScheduler()->timerTaskRunner()
> -                   :
Platform::current()->currentThread()->getWebTaskRunner();
> +      return Platform::current()->currentThread()->getWebTaskRunner();
>      case TaskType::UnspecedLoading:
>      case TaskType::Networking:
>        return frame ? frame->frameScheduler()->loadingTaskRunner()
> 
> 
> The leak goes away.

Thanks for the digging!

It looks strange that using frame->frameScheduler()->timerTaskRunner() leads to
the leak whereas currentThread()->getWebTaskRunner() does not leak...

[dcheng, 2017-01-20 08:11:09]

On 2017/01/20 00:24:07, haraken wrote:
> On 2017/01/20 00:17:52, dougt wrote:
> > On 2017/01/20 00:09:35, haraken wrote:
> > > On 2017/01/20 00:08:46, jianli wrote:
> > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > > https://codereview.chromium.org/2640403002/ by
> > > mailto:jianli@chromium.org.
> > > > > > > 
> > > > > > > The reason for reverting is: Seems to be the cause for failed test
> > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > 
> > > > > > I don't understand why this CL causes a memory leak like this.
> > > > > > 
> > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > 
> > > > > Sorry, it turned out that this CL is not a culprit.
> > > > 
> > > > But the bot turned green right after this revert was landed. See
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > 
> > > Sorry again! I was chatting with Dougt and confused. Yes, this CL is a
> culprit
> > > of the leak.
> > 
> > 
> > Just to be clear -- I was confused, not haraken.
> > 
> > With this cl, the leaking docs are:
> > 
> > - Document 0x302beb765d00 URL:
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > - Document 0x302beb7680b8 URL: about:blank
> > - Document 0x302beb762848 URL:
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > #LEAK - renderer pid 49525
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > 
> > 
> > when constructing the m_timer, frame is valid.  if I null it out, we do not
> > leak.  That is if I hack this out:
> > 
> > index c7f578b378db..45162b1b00fc 100644
> > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner> TaskRunnerHelper::get(TaskType type,
> > LocalFrame* frame) {
> >      case TaskType::Timer:
> >      case TaskType::UnspecedTimer:
> >      case TaskType::MiscPlatformAPI:
> > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > -                   :
> Platform::current()->currentThread()->getWebTaskRunner();
> > +      return Platform::current()->currentThread()->getWebTaskRunner();
> >      case TaskType::UnspecedLoading:
> >      case TaskType::Networking:
> >        return frame ? frame->frameScheduler()->loadingTaskRunner()
> > 
> > 
> > The leak goes away.
> 
> Thanks for the digging!
> 
> It looks strange that using frame->frameScheduler()->timerTaskRunner() leads
to
> the leak whereas currentThread()->getWebTaskRunner() does not leak...

I guess what's happening is that when the frame is detached, any tasks remaining
in the frame's task queue don't get run. Hence we get a leak.

[kinuko, 2017-01-20 08:27:09]

On 2017/01/20 00:24:07, haraken wrote:
> On 2017/01/20 00:17:52, dougt wrote:
> > On 2017/01/20 00:09:35, haraken wrote:
> > > On 2017/01/20 00:08:46, jianli wrote:
> > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > > https://codereview.chromium.org/2640403002/ by
> > > mailto:jianli@chromium.org.
> > > > > > > 
> > > > > > > The reason for reverting is: Seems to be the cause for failed test
> > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > 
> > > > > > I don't understand why this CL causes a memory leak like this.
> > > > > > 
> > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > 
> > > > > Sorry, it turned out that this CL is not a culprit.
> > > > 
> > > > But the bot turned green right after this revert was landed. See
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > 
> > > Sorry again! I was chatting with Dougt and confused. Yes, this CL is a
> culprit
> > > of the leak.
> > 
> > 
> > Just to be clear -- I was confused, not haraken.
> > 
> > With this cl, the leaking docs are:
> > 
> > - Document 0x302beb765d00 URL:
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > - Document 0x302beb7680b8 URL: about:blank
> > - Document 0x302beb762848 URL:
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > #LEAK - renderer pid 49525
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > 
> > 
> > when constructing the m_timer, frame is valid.  if I null it out, we do not
> > leak.  That is if I hack this out:
> > 
> > index c7f578b378db..45162b1b00fc 100644
> > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner> TaskRunnerHelper::get(TaskType type,
> > LocalFrame* frame) {
> >      case TaskType::Timer:
> >      case TaskType::UnspecedTimer:
> >      case TaskType::MiscPlatformAPI:
> > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > -                   :
> Platform::current()->currentThread()->getWebTaskRunner();
> > +      return Platform::current()->currentThread()->getWebTaskRunner();
> >      case TaskType::UnspecedLoading:
> >      case TaskType::Networking:
> >        return frame ? frame->frameScheduler()->loadingTaskRunner()
> > 
> > 
> > The leak goes away.
> 
> Thanks for the digging!
> 
> It looks strange that using frame->frameScheduler()->timerTaskRunner() leads
to
> the leak whereas currentThread()->getWebTaskRunner() does not leak...

Don't we reset task queue when the corresponding frame goes away?

[dcheng, 2017-01-20 08:31:26]

On 2017/01/20 08:27:09, kinuko wrote:
> On 2017/01/20 00:24:07, haraken wrote:
> > On 2017/01/20 00:17:52, dougt wrote:
> > > On 2017/01/20 00:09:35, haraken wrote:
> > > > On 2017/01/20 00:08:46, jianli wrote:
> > > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > > > https://codereview.chromium.org/2640403002/ by
> > > > mailto:jianli@chromium.org.
> > > > > > > > 
> > > > > > > > The reason for reverting is: Seems to be the cause for failed
test
> > > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > 
> > > > > > > I don't understand why this CL causes a memory leak like this.
> > > > > > > 
> > > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > > 
> > > > > > Sorry, it turned out that this CL is not a culprit.
> > > > > 
> > > > > But the bot turned green right after this revert was landed. See
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > 
> > > > Sorry again! I was chatting with Dougt and confused. Yes, this CL is a
> > culprit
> > > > of the leak.
> > > 
> > > 
> > > Just to be clear -- I was confused, not haraken.
> > > 
> > > With this cl, the leaking docs are:
> > > 
> > > - Document 0x302beb765d00 URL:
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > > - Document 0x302beb7680b8 URL: about:blank
> > > - Document 0x302beb762848 URL:
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > > #LEAK - renderer pid 49525
> > >
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > > 
> > > 
> > > when constructing the m_timer, frame is valid.  if I null it out, we do
not
> > > leak.  That is if I hack this out:
> > > 
> > > index c7f578b378db..45162b1b00fc 100644
> > > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner> TaskRunnerHelper::get(TaskType
type,
> > > LocalFrame* frame) {
> > >      case TaskType::Timer:
> > >      case TaskType::UnspecedTimer:
> > >      case TaskType::MiscPlatformAPI:
> > > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > > -                   :
> > Platform::current()->currentThread()->getWebTaskRunner();
> > > +      return Platform::current()->currentThread()->getWebTaskRunner();
> > >      case TaskType::UnspecedLoading:
> > >      case TaskType::Networking:
> > >        return frame ? frame->frameScheduler()->loadingTaskRunner()
> > > 
> > > 
> > > The leak goes away.
> > 
> > Thanks for the digging!
> > 
> > It looks strange that using frame->frameScheduler()->timerTaskRunner() leads
> to
> > the leak whereas currentThread()->getWebTaskRunner() does not leak...
> 
> Don't we reset task queue when the corresponding frame goes away?

Maybe not running the task means we don't free resources elsewhere though? It
looks like the timer callback does some cleanup work:
https://chromium.googlesource.com/chromium/src/+/dd4298c2034b2f5cf6e58239eb20...

[kinuko, 2017-01-20 08:36:53]

On 2017/01/20 08:31:26, dcheng wrote:
> On 2017/01/20 08:27:09, kinuko wrote:
> > On 2017/01/20 00:24:07, haraken wrote:
> > > On 2017/01/20 00:17:52, dougt wrote:
> > > > On 2017/01/20 00:09:35, haraken wrote:
> > > > > On 2017/01/20 00:08:46, jianli wrote:
> > > > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > > > > https://codereview.chromium.org/2640403002/ by
> > > > > mailto:jianli@chromium.org.
> > > > > > > > > 
> > > > > > > > > The reason for reverting is: Seems to be the cause for failed
> test
> > > > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > 
> > > > > > > > I don't understand why this CL causes a memory leak like this.
> > > > > > > > 
> > > > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > > > 
> > > > > > > Sorry, it turned out that this CL is not a culprit.
> > > > > > 
> > > > > > But the bot turned green right after this revert was landed. See
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > 
> > > > > Sorry again! I was chatting with Dougt and confused. Yes, this CL is a
> > > culprit
> > > > > of the leak.
> > > > 
> > > > 
> > > > Just to be clear -- I was confused, not haraken.
> > > > 
> > > > With this cl, the leaking docs are:
> > > > 
> > > > - Document 0x302beb765d00 URL:
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > > > - Document 0x302beb7680b8 URL: about:blank
> > > > - Document 0x302beb762848 URL:
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > > > #LEAK - renderer pid 49525
> > > >
> > >
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > > > 
> > > > 
> > > > when constructing the m_timer, frame is valid.  if I null it out, we do
> not
> > > > leak.  That is if I hack this out:
> > > > 
> > > > index c7f578b378db..45162b1b00fc 100644
> > > > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner> TaskRunnerHelper::get(TaskType
> type,
> > > > LocalFrame* frame) {
> > > >      case TaskType::Timer:
> > > >      case TaskType::UnspecedTimer:
> > > >      case TaskType::MiscPlatformAPI:
> > > > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > > > -                   :
> > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > +      return Platform::current()->currentThread()->getWebTaskRunner();
> > > >      case TaskType::UnspecedLoading:
> > > >      case TaskType::Networking:
> > > >        return frame ? frame->frameScheduler()->loadingTaskRunner()
> > > > 
> > > > 
> > > > The leak goes away.
> > > 
> > > Thanks for the digging!
> > > 
> > > It looks strange that using frame->frameScheduler()->timerTaskRunner()
leads
> > to
> > > the leak whereas currentThread()->getWebTaskRunner() does not leak...
> > 
> > Don't we reset task queue when the corresponding frame goes away?
> 
> Maybe not running the task means we don't free resources elsewhere though? It
> looks like the timer callback does some cleanup work:
>
https://chromium.googlesource.com/chromium/src/+/dd4298c2034b2f5cf6e58239eb20...

Yeah I guess so.  It might have been useful if we could specify how we want to
do for the pending tasks when the frame goes away (like shutdown behavior in
sequenced worker pool)

[kinuko, 2017-01-20 08:39:56]

On 2017/01/20 08:36:53, kinuko wrote:
> On 2017/01/20 08:31:26, dcheng wrote:
> > On 2017/01/20 08:27:09, kinuko wrote:
> > > On 2017/01/20 00:24:07, haraken wrote:
> > > > On 2017/01/20 00:17:52, dougt wrote:
> > > > > On 2017/01/20 00:09:35, haraken wrote:
> > > > > > On 2017/01/20 00:08:46, jianli wrote:
> > > > > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > > > > A revert of this CL (patchset #2 id:20001) has been created
in
> > > > > > > > > > https://codereview.chromium.org/2640403002/ by
> > > > > > mailto:jianli@chromium.org.
> > > > > > > > > > 
> > > > > > > > > > The reason for reverting is: Seems to be the cause for
failed
> > test
> > > > > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > > 
> > > > > > > > > I don't understand why this CL causes a memory leak like this.
> > > > > > > > > 
> > > > > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > > > > 
> > > > > > > > Sorry, it turned out that this CL is not a culprit.
> > > > > > > 
> > > > > > > But the bot turned green right after this revert was landed. See
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > 
> > > > > > Sorry again! I was chatting with Dougt and confused. Yes, this CL is
a
> > > > culprit
> > > > > > of the leak.
> > > > > 
> > > > > 
> > > > > Just to be clear -- I was confused, not haraken.
> > > > > 
> > > > > With this cl, the leaking docs are:
> > > > > 
> > > > > - Document 0x302beb765d00 URL:
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > > > > - Document 0x302beb7680b8 URL: about:blank
> > > > > - Document 0x302beb762848 URL:
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > > > > #LEAK - renderer pid 49525
> > > > >
> > > >
> > >
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > > > > 
> > > > > 
> > > > > when constructing the m_timer, frame is valid.  if I null it out, we
do
> > not
> > > > > leak.  That is if I hack this out:
> > > > > 
> > > > > index c7f578b378db..45162b1b00fc 100644
> > > > > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner> TaskRunnerHelper::get(TaskType
> > type,
> > > > > LocalFrame* frame) {
> > > > >      case TaskType::Timer:
> > > > >      case TaskType::UnspecedTimer:
> > > > >      case TaskType::MiscPlatformAPI:
> > > > > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > > > > -                   :
> > > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > +      return
Platform::current()->currentThread()->getWebTaskRunner();
> > > > >      case TaskType::UnspecedLoading:
> > > > >      case TaskType::Networking:
> > > > >        return frame ? frame->frameScheduler()->loadingTaskRunner()
> > > > > 
> > > > > 
> > > > > The leak goes away.
> > > > 
> > > > Thanks for the digging!
> > > > 
> > > > It looks strange that using frame->frameScheduler()->timerTaskRunner()
> leads
> > > to
> > > > the leak whereas currentThread()->getWebTaskRunner() does not leak...
> > > 
> > > Don't we reset task queue when the corresponding frame goes away?
> > 
> > Maybe not running the task means we don't free resources elsewhere though?
It
> > looks like the timer callback does some cleanup work:
> >
>
https://chromium.googlesource.com/chromium/src/+/dd4298c2034b2f5cf6e58239eb20...
> 
> Yeah I guess so.  It might have been useful if we could specify how we want to
> do for the pending tasks when the frame goes away (like shutdown behavior in
> sequenced worker pool)

Geolocation can do the same cleanup instead in contextDestroyed maybe...?

[haraken, 2017-01-20 09:38:08]

On 2017/01/20 08:39:56, kinuko wrote:
> On 2017/01/20 08:36:53, kinuko wrote:
> > On 2017/01/20 08:31:26, dcheng wrote:
> > > On 2017/01/20 08:27:09, kinuko wrote:
> > > > On 2017/01/20 00:24:07, haraken wrote:
> > > > > On 2017/01/20 00:17:52, dougt wrote:
> > > > > > On 2017/01/20 00:09:35, haraken wrote:
> > > > > > > On 2017/01/20 00:08:46, jianli wrote:
> > > > > > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > > > > > A revert of this CL (patchset #2 id:20001) has been
created
> in
> > > > > > > > > > > https://codereview.chromium.org/2640403002/ by
> > > > > > > mailto:jianli@chromium.org.
> > > > > > > > > > > 
> > > > > > > > > > > The reason for reverting is: Seems to be the cause for
> failed
> > > test
> > > > > > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > > > > > 
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > > > 
> > > > > > > > > > I don't understand why this CL causes a memory leak like
this.
> > > > > > > > > > 
> > > > > > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > > > > > 
> > > > > > > > > Sorry, it turned out that this CL is not a culprit.
> > > > > > > > 
> > > > > > > > But the bot turned green right after this revert was landed. See
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > 
> > > > > > > Sorry again! I was chatting with Dougt and confused. Yes, this CL
is
> a
> > > > > culprit
> > > > > > > of the leak.
> > > > > > 
> > > > > > 
> > > > > > Just to be clear -- I was confused, not haraken.
> > > > > > 
> > > > > > With this cl, the leaking docs are:
> > > > > > 
> > > > > > - Document 0x302beb765d00 URL:
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > > > > > - Document 0x302beb7680b8 URL: about:blank
> > > > > > - Document 0x302beb762848 URL:
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > > > > > #LEAK - renderer pid 49525
> > > > > >
> > > > >
> > > >
> > >
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > > > > > 
> > > > > > 
> > > > > > when constructing the m_timer, frame is valid.  if I null it out, we
> do
> > > not
> > > > > > leak.  That is if I hack this out:
> > > > > > 
> > > > > > index c7f578b378db..45162b1b00fc 100644
> > > > > > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner>
TaskRunnerHelper::get(TaskType
> > > type,
> > > > > > LocalFrame* frame) {
> > > > > >      case TaskType::Timer:
> > > > > >      case TaskType::UnspecedTimer:
> > > > > >      case TaskType::MiscPlatformAPI:
> > > > > > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > > > > > -                   :
> > > > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > > +      return
> Platform::current()->currentThread()->getWebTaskRunner();
> > > > > >      case TaskType::UnspecedLoading:
> > > > > >      case TaskType::Networking:
> > > > > >        return frame ? frame->frameScheduler()->loadingTaskRunner()
> > > > > > 
> > > > > > 
> > > > > > The leak goes away.
> > > > > 
> > > > > Thanks for the digging!
> > > > > 
> > > > > It looks strange that using frame->frameScheduler()->timerTaskRunner()
> > leads
> > > > to
> > > > > the leak whereas currentThread()->getWebTaskRunner() does not leak...
> > > > 
> > > > Don't we reset task queue when the corresponding frame goes away?
> > > 
> > > Maybe not running the task means we don't free resources elsewhere though?
> It
> > > looks like the timer callback does some cleanup work:
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/dd4298c2034b2f5cf6e58239eb20...
> > 
> > Yeah I guess so.  It might have been useful if we could specify how we want
to
> > do for the pending tasks when the frame goes away (like shutdown behavior in
> > sequenced worker pool)
> 
> Geolocation can do the same cleanup instead in contextDestroyed maybe...?

Thanks for the digging! That makes sense.

Would it be crazy (waste performance) to drain all pending tasks when detaching
the frame? It's a bit error-prone if developers have to assume that timers may
not run.

[alex clarke (OOO till 29th), 2017-01-20 09:39:19]

On 2017/01/20 09:38:08, haraken wrote:
> On 2017/01/20 08:39:56, kinuko wrote:
> > On 2017/01/20 08:36:53, kinuko wrote:
> > > On 2017/01/20 08:31:26, dcheng wrote:
> > > > On 2017/01/20 08:27:09, kinuko wrote:
> > > > > On 2017/01/20 00:24:07, haraken wrote:
> > > > > > On 2017/01/20 00:17:52, dougt wrote:
> > > > > > > On 2017/01/20 00:09:35, haraken wrote:
> > > > > > > > On 2017/01/20 00:08:46, jianli wrote:
> > > > > > > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > > > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > > > > > > A revert of this CL (patchset #2 id:20001) has been
> created
> > in
> > > > > > > > > > > > https://codereview.chromium.org/2640403002/ by
> > > > > > > > mailto:jianli@chromium.org.
> > > > > > > > > > > > 
> > > > > > > > > > > > The reason for reverting is: Seems to be the cause for
> > failed
> > > > test
> > > > > > > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > > > > > > 
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > > > > 
> > > > > > > > > > > I don't understand why this CL causes a memory leak like
> this.
> > > > > > > > > > > 
> > > > > > > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > > > > > > 
> > > > > > > > > > Sorry, it turned out that this CL is not a culprit.
> > > > > > > > > 
> > > > > > > > > But the bot turned green right after this revert was landed.
See
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > 
> > > > > > > > Sorry again! I was chatting with Dougt and confused. Yes, this
CL
> is
> > a
> > > > > > culprit
> > > > > > > > of the leak.
> > > > > > > 
> > > > > > > 
> > > > > > > Just to be clear -- I was confused, not haraken.
> > > > > > > 
> > > > > > > With this cl, the leaking docs are:
> > > > > > > 
> > > > > > > - Document 0x302beb765d00 URL:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > > > > > > - Document 0x302beb7680b8 URL: about:blank
> > > > > > > - Document 0x302beb762848 URL:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > > > > > > #LEAK - renderer pid 49525
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > > > > > > 
> > > > > > > 
> > > > > > > when constructing the m_timer, frame is valid.  if I null it out,
we
> > do
> > > > not
> > > > > > > leak.  That is if I hack this out:
> > > > > > > 
> > > > > > > index c7f578b378db..45162b1b00fc 100644
> > > > > > > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner>
> TaskRunnerHelper::get(TaskType
> > > > type,
> > > > > > > LocalFrame* frame) {
> > > > > > >      case TaskType::Timer:
> > > > > > >      case TaskType::UnspecedTimer:
> > > > > > >      case TaskType::MiscPlatformAPI:
> > > > > > > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > > > > > > -                   :
> > > > > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > > > +      return
> > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > > >      case TaskType::UnspecedLoading:
> > > > > > >      case TaskType::Networking:
> > > > > > >        return frame ? frame->frameScheduler()->loadingTaskRunner()
> > > > > > > 
> > > > > > > 
> > > > > > > The leak goes away.
> > > > > > 
> > > > > > Thanks for the digging!
> > > > > > 
> > > > > > It looks strange that using
frame->frameScheduler()->timerTaskRunner()
> > > leads
> > > > > to
> > > > > > the leak whereas currentThread()->getWebTaskRunner() does not
leak...
> > > > > 
> > > > > Don't we reset task queue when the corresponding frame goes away?
> > > > 
> > > > Maybe not running the task means we don't free resources elsewhere
though?
> > It
> > > > looks like the timer callback does some cleanup work:
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/dd4298c2034b2f5cf6e58239eb20...
> > > 
> > > Yeah I guess so.  It might have been useful if we could specify how we
want
> to
> > > do for the pending tasks when the frame goes away (like shutdown behavior
in
> > > sequenced worker pool)
> > 
> > Geolocation can do the same cleanup instead in contextDestroyed maybe...?
> 
> Thanks for the digging! That makes sense.
> 
> Would it be crazy (waste performance) to drain all pending tasks when
detaching
> the frame? It's a bit error-prone if developers have to assume that timers may
> not run.

We could add a scheduler api to mass cancel everything posted if useful.

[alex clarke (OOO till 29th), 2017-01-20 09:40:12]

On 2017/01/20 09:39:19, alex clarke wrote:
> On 2017/01/20 09:38:08, haraken wrote:
> > On 2017/01/20 08:39:56, kinuko wrote:
> > > On 2017/01/20 08:36:53, kinuko wrote:
> > > > On 2017/01/20 08:31:26, dcheng wrote:
> > > > > On 2017/01/20 08:27:09, kinuko wrote:
> > > > > > On 2017/01/20 00:24:07, haraken wrote:
> > > > > > > On 2017/01/20 00:17:52, dougt wrote:
> > > > > > > > On 2017/01/20 00:09:35, haraken wrote:
> > > > > > > > > On 2017/01/20 00:08:46, jianli wrote:
> > > > > > > > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > > > > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > > > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > > > > > > > A revert of this CL (patchset #2 id:20001) has been
> > created
> > > in
> > > > > > > > > > > > > https://codereview.chromium.org/2640403002/ by
> > > > > > > > > mailto:jianli@chromium.org.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > The reason for reverting is: Seems to be the cause for
> > > failed
> > > > > test
> > > > > > > > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > > > > > > > 
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > > > > > 
> > > > > > > > > > > > I don't understand why this CL causes a memory leak like
> > this.
> > > > > > > > > > > > 
> > > > > > > > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > > > > > > > 
> > > > > > > > > > > Sorry, it turned out that this CL is not a culprit.
> > > > > > > > > > 
> > > > > > > > > > But the bot turned green right after this revert was landed.
> See
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > > 
> > > > > > > > > Sorry again! I was chatting with Dougt and confused. Yes, this
> CL
> > is
> > > a
> > > > > > > culprit
> > > > > > > > > of the leak.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Just to be clear -- I was confused, not haraken.
> > > > > > > > 
> > > > > > > > With this cl, the leaking docs are:
> > > > > > > > 
> > > > > > > > - Document 0x302beb765d00 URL:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > > > > > > > - Document 0x302beb7680b8 URL: about:blank
> > > > > > > > - Document 0x302beb762848 URL:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > > > > > > > #LEAK - renderer pid 49525
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > > > > > > > 
> > > > > > > > 
> > > > > > > > when constructing the m_timer, frame is valid.  if I null it
out,
> we
> > > do
> > > > > not
> > > > > > > > leak.  That is if I hack this out:
> > > > > > > > 
> > > > > > > > index c7f578b378db..45162b1b00fc 100644
> > > > > > > > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > > > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > > > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner>
> > TaskRunnerHelper::get(TaskType
> > > > > type,
> > > > > > > > LocalFrame* frame) {
> > > > > > > >      case TaskType::Timer:
> > > > > > > >      case TaskType::UnspecedTimer:
> > > > > > > >      case TaskType::MiscPlatformAPI:
> > > > > > > > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > > > > > > > -                   :
> > > > > > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > > > > +      return
> > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > > > >      case TaskType::UnspecedLoading:
> > > > > > > >      case TaskType::Networking:
> > > > > > > >        return frame ?
frame->frameScheduler()->loadingTaskRunner()
> > > > > > > > 
> > > > > > > > 
> > > > > > > > The leak goes away.
> > > > > > > 
> > > > > > > Thanks for the digging!
> > > > > > > 
> > > > > > > It looks strange that using
> frame->frameScheduler()->timerTaskRunner()
> > > > leads
> > > > > > to
> > > > > > > the leak whereas currentThread()->getWebTaskRunner() does not
> leak...
> > > > > > 
> > > > > > Don't we reset task queue when the corresponding frame goes away?
> > > > > 
> > > > > Maybe not running the task means we don't free resources elsewhere
> though?
> > > It
> > > > > looks like the timer callback does some cleanup work:
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/dd4298c2034b2f5cf6e58239eb20...
> > > > 
> > > > Yeah I guess so.  It might have been useful if we could specify how we
> want
> > to
> > > > do for the pending tasks when the frame goes away (like shutdown
behavior
> in
> > > > sequenced worker pool)
> > > 
> > > Geolocation can do the same cleanup instead in contextDestroyed maybe...?
> > 
> > Thanks for the digging! That makes sense.
> > 
> > Would it be crazy (waste performance) to drain all pending tasks when
> detaching
> > the frame? It's a bit error-prone if developers have to assume that timers
may
> > not run.
> 
> We could add a scheduler api to mass cancel everything posted if useful.

This would literally call clear() on the queues freeing the memory (I'm assuming
deleting the closure would release the memory).

[alex clarke (OOO till 29th), 2017-01-20 09:40:14]

On 2017/01/20 09:39:19, alex clarke wrote:
> On 2017/01/20 09:38:08, haraken wrote:
> > On 2017/01/20 08:39:56, kinuko wrote:
> > > On 2017/01/20 08:36:53, kinuko wrote:
> > > > On 2017/01/20 08:31:26, dcheng wrote:
> > > > > On 2017/01/20 08:27:09, kinuko wrote:
> > > > > > On 2017/01/20 00:24:07, haraken wrote:
> > > > > > > On 2017/01/20 00:17:52, dougt wrote:
> > > > > > > > On 2017/01/20 00:09:35, haraken wrote:
> > > > > > > > > On 2017/01/20 00:08:46, jianli wrote:
> > > > > > > > > > On 2017/01/20 00:05:02, haraken wrote:
> > > > > > > > > > > On 2017/01/19 23:58:23, haraken wrote:
> > > > > > > > > > > > On 2017/01/19 20:45:24, jianli wrote:
> > > > > > > > > > > > > A revert of this CL (patchset #2 id:20001) has been
> > created
> > > in
> > > > > > > > > > > > > https://codereview.chromium.org/2640403002/ by
> > > > > > > > > mailto:jianli@chromium.org.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > The reason for reverting is: Seems to be the cause for
> > > failed
> > > > > test
> > > > > > > > > > > > > fast/dom/Geolocation/window-close-crash.html
> > > > > > > > > > > > > 
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > > > > > 
> > > > > > > > > > > > I don't understand why this CL causes a memory leak like
> > this.
> > > > > > > > > > > > 
> > > > > > > > > > > > tzik@, dcheng@: Do you have any idea on this?
> > > > > > > > > > > 
> > > > > > > > > > > Sorry, it turned out that this CL is not a culprit.
> > > > > > > > > > 
> > > > > > > > > > But the bot turned green right after this revert was landed.
> See
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....
> > > > > > > > > 
> > > > > > > > > Sorry again! I was chatting with Dougt and confused. Yes, this
> CL
> > is
> > > a
> > > > > > > culprit
> > > > > > > > > of the leak.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Just to be clear -- I was confused, not haraken.
> > > > > > > > 
> > > > > > > > With this cl, the leaking docs are:
> > > > > > > > 
> > > > > > > > - Document 0x302beb765d00 URL:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/resources/window-close-popup.html
> > > > > > > > - Document 0x302beb7680b8 URL: about:blank
> > > > > > > > - Document 0x302beb762848 URL:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
file:///usr/local/google/home/dft/builds/chromium/src/third_party/WebKit/LayoutTests/fast/dom/Geolocation/window-close-crash.html
> > > > > > > > #LEAK - renderer pid 49525
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
({"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,77],"numberOfLiveResources":[0,6],"numberOfLiveSuspendableObjects":[2,4]})
> > > > > > > > 
> > > > > > > > 
> > > > > > > > when constructing the m_timer, frame is valid.  if I null it
out,
> we
> > > do
> > > > > not
> > > > > > > > leak.  That is if I hack this out:
> > > > > > > > 
> > > > > > > > index c7f578b378db..45162b1b00fc 100644
> > > > > > > > --- a/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > > > +++ b/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
> > > > > > > > @@ -34,8 +34,7 @@ RefPtr<WebTaskRunner>
> > TaskRunnerHelper::get(TaskType
> > > > > type,
> > > > > > > > LocalFrame* frame) {
> > > > > > > >      case TaskType::Timer:
> > > > > > > >      case TaskType::UnspecedTimer:
> > > > > > > >      case TaskType::MiscPlatformAPI:
> > > > > > > > -      return frame ? frame->frameScheduler()->timerTaskRunner()
> > > > > > > > -                   :
> > > > > > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > > > > +      return
> > > Platform::current()->currentThread()->getWebTaskRunner();
> > > > > > > >      case TaskType::UnspecedLoading:
> > > > > > > >      case TaskType::Networking:
> > > > > > > >        return frame ?
frame->frameScheduler()->loadingTaskRunner()
> > > > > > > > 
> > > > > > > > 
> > > > > > > > The leak goes away.
> > > > > > > 
> > > > > > > Thanks for the digging!
> > > > > > > 
> > > > > > > It looks strange that using
> frame->frameScheduler()->timerTaskRunner()
> > > > leads
> > > > > > to
> > > > > > > the leak whereas currentThread()->getWebTaskRunner() does not
> leak...
> > > > > > 
> > > > > > Don't we reset task queue when the corresponding frame goes away?
> > > > > 
> > > > > Maybe not running the task means we don't free resources elsewhere
> though?
> > > It
> > > > > looks like the timer callback does some cleanup work:
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/dd4298c2034b2f5cf6e58239eb20...
> > > > 
> > > > Yeah I guess so.  It might have been useful if we could specify how we
> want
> > to
> > > > do for the pending tasks when the frame goes away (like shutdown
behavior
> in
> > > > sequenced worker pool)
> > > 
> > > Geolocation can do the same cleanup instead in contextDestroyed maybe...?
> > 
> > Thanks for the digging! That makes sense.
> > 
> > Would it be crazy (waste performance) to drain all pending tasks when
> detaching
> > the frame? It's a bit error-prone if developers have to assume that timers
may
> > not run.
> 
> We could add a scheduler api to mass cancel everything posted if useful.

This would literally call clear() on the queues freeing the memory (I'm assuming
deleting the closure would release the memory).

[haraken, 2017-01-20 09:40:18]

> Thanks for the digging! That makes sense.
> 
> Would it be crazy (waste performance) to drain all pending tasks when
detaching
> the frame? It's a bit error-prone if developers have to assume that timers may
> not run.

https://codereview.chromium.org/2642103002/ is hitting the same problem.

[haraken, 2017-01-20 09:54:53]

> > We could add a scheduler api to mass cancel everything posted if useful.
> 
> This would literally call clear() on the queues freeing the memory (I'm
assuming
> deleting the closure would release the memory).

It's not enough to delete the closure. The timer needs to actually fire in the
case of Geolocation and ImageLoader.

If it's nasty, we can ask developers to not assume that timers always fire.

[kinuko, 2017-01-20 13:06:10]

On 2017/01/20 09:54:53, haraken wrote:
> > > We could add a scheduler api to mass cancel everything posted if useful.
> > 
> > This would literally call clear() on the queues freeing the memory (I'm
> assuming
> > deleting the closure would release the memory).
> 
> It's not enough to delete the closure. The timer needs to actually fire in the
> case of Geolocation and ImageLoader.
> 
> If it's nasty, we can ask developers to not assume that timers always fire.

I thought what if we could specify behavior on frame detach like shutdown
behavior in base tasks:
https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?type=c...

thought it might be an overkill.  Anyways it'd be nice to have *some* legit way
to post a task that can still run after frame detach in the per-frame scheduling
world, for modules that live longer than frames this change could be a bit
disrupting.

[tzik, 2017-01-20 19:15:40]

On 2017/01/20 09:54:53, haraken wrote:
> > > We could add a scheduler api to mass cancel everything posted if useful.
> > 
> > This would literally call clear() on the queues freeing the memory (I'm
> assuming
> > deleting the closure would release the memory).
> 
> It's not enough to delete the closure. The timer needs to actually fire in the
> case of Geolocation and ImageLoader.

I think that's true. If we remove m_timer.startOneShot(0, BLINK_FROM_HERE); in
GeoNotifier::setFatalError(), the leak happen even without this CL. Geolocation
seems to have a strong reference cycle containing a non GCed object.

> 
> If it's nasty, we can ask developers to not assume that timers always fire.

+1.

[dcheng, 2017-01-20 19:20:40]

On 2017/01/20 19:15:40, tzik wrote:
> On 2017/01/20 09:54:53, haraken wrote:
> > > > We could add a scheduler api to mass cancel everything posted if useful.
> > > 
> > > This would literally call clear() on the queues freeing the memory (I'm
> > assuming
> > > deleting the closure would release the memory).
> > 
> > It's not enough to delete the closure. The timer needs to actually fire in
the
> > case of Geolocation and ImageLoader.
> 
> I think that's true. If we remove m_timer.startOneShot(0, BLINK_FROM_HERE); in
> GeoNotifier::setFatalError(), the leak happen even without this CL.
Geolocation
> seems to have a strong reference cycle containing a non GCed object.
> 
> > 
> > If it's nasty, we can ask developers to not assume that timers always fire.
> 
> +1.

Hmmm. I'm not sure if this is a good idea...

It's relatively obvious that if you destroy an object holding a Timer, that the
Timer will be cancelled.
It's not so obvious that tearing down something seemingly unrelated (the frame)
will also automatically cancel Timer callbacks and prevent crucial code like
this from running.

Would it be bad to just throw all the pending callbacks in a WebFrameScheduler
into the process-wide task queue?

[dcheng, 2017-01-20 19:25:51]

On 2017/01/20 19:20:40, dcheng wrote:
> On 2017/01/20 19:15:40, tzik wrote:
> > On 2017/01/20 09:54:53, haraken wrote:
> > > > > We could add a scheduler api to mass cancel everything posted if
useful.
> > > > 
> > > > This would literally call clear() on the queues freeing the memory (I'm
> > > assuming
> > > > deleting the closure would release the memory).
> > > 
> > > It's not enough to delete the closure. The timer needs to actually fire in
> the
> > > case of Geolocation and ImageLoader.
> > 
> > I think that's true. If we remove m_timer.startOneShot(0, BLINK_FROM_HERE);
in
> > GeoNotifier::setFatalError(), the leak happen even without this CL.
> Geolocation
> > seems to have a strong reference cycle containing a non GCed object.
> > 
> > > 
> > > If it's nasty, we can ask developers to not assume that timers always
fire.
> > 
> > +1.
> 
> Hmmm. I'm not sure if this is a good idea...
> 
> It's relatively obvious that if you destroy an object holding a Timer, that
the
> Timer will be cancelled.
> It's not so obvious that tearing down something seemingly unrelated (the
frame)
> will also automatically cancel Timer callbacks and prevent crucial code like
> this from running.
> 
> Would it be bad to just throw all the pending callbacks in a WebFrameScheduler
> into the process-wide task queue?

(I guess this doesn't really help either: the timer is holding a reference to a
task runner associated with a detached WebFrameScheduler, so if it posts more
tasks after that, they won't get transferred...)

But I do think it will very hard to work with a system that randomly (from the
developer's perspective) doesn't run cleanup callbacks.

[haraken, 2017-01-21 01:55:16]

On 2017/01/20 19:25:51, dcheng wrote:
> On 2017/01/20 19:20:40, dcheng wrote:
> > On 2017/01/20 19:15:40, tzik wrote:
> > > On 2017/01/20 09:54:53, haraken wrote:
> > > > > > We could add a scheduler api to mass cancel everything posted if
> useful.
> > > > > 
> > > > > This would literally call clear() on the queues freeing the memory
(I'm
> > > > assuming
> > > > > deleting the closure would release the memory).
> > > > 
> > > > It's not enough to delete the closure. The timer needs to actually fire
in
> > the
> > > > case of Geolocation and ImageLoader.
> > > 
> > > I think that's true. If we remove m_timer.startOneShot(0,
BLINK_FROM_HERE);
> in
> > > GeoNotifier::setFatalError(), the leak happen even without this CL.
> > Geolocation
> > > seems to have a strong reference cycle containing a non GCed object.
> > > 
> > > > 
> > > > If it's nasty, we can ask developers to not assume that timers always
> fire.
> > > 
> > > +1.
> > 
> > Hmmm. I'm not sure if this is a good idea...
> > 
> > It's relatively obvious that if you destroy an object holding a Timer, that
> the
> > Timer will be cancelled.
> > It's not so obvious that tearing down something seemingly unrelated (the
> frame)
> > will also automatically cancel Timer callbacks and prevent crucial code like
> > this from running.
> > 
> > Would it be bad to just throw all the pending callbacks in a
WebFrameScheduler
> > into the process-wide task queue?
> 
> (I guess this doesn't really help either: the timer is holding a reference to
a
> task runner associated with a detached WebFrameScheduler, so if it posts more
> tasks after that, they won't get transferred...)
> 
> But I do think it will very hard to work with a system that randomly (from the
> developer's perspective) doesn't run cleanup callbacks.

Agreed with dcheng@.

Why can't we simply run all pending tasks just before the frame scheduler gets
destroyed?

BTW, when does the frame scheduler get destroyed in the sequence of
Frame::detach()?