Oilpan: Move MainThreadTaskRunner into Oilpan heap.

Oilpan: Move MainThreadTaskRunner into Oilpan heap.

Fixes a raw pointer in MainThreadTaskRunner.

BUG=509911
R=haraken@chromium.org, keishi@chromium.org, oilpan-reviews@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200972

[haraken, 2015-08-18 07:04:18]

LGTM

[sof, 2015-08-18 07:15:30]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-08-18 07:15:31]

https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
File Source/core/dom/MainThreadTaskRunner.h (left):

https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
Source/core/dom/MainThreadTaskRunner.h:67: WeakPtrFactory<MainThreadTaskRunner>
m_weakFactory;
Doesn't this need to be eagerly finalized?

[haraken, 2015-08-18 08:08:54]

https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
File Source/core/dom/MainThreadTaskRunner.h (left):

https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
Source/core/dom/MainThreadTaskRunner.h:67: WeakPtrFactory<MainThreadTaskRunner>
m_weakFactory;
On 2015/08/18 07:15:31, sof wrote:
> Doesn't this need to be eagerly finalized?

I don't see any issue in not eagerly finalizing the MainThreadTaskRunner, but I
agree that it's nasty to let the MainThreadTask keep WeakPtr<MainThreadRunner>.
(i.e., it's nasty to have a WeakPtr to an on-heap object -- it should be a
WeakPersistent or WeakMember.)

Would it be better to add #if !ENABLE(OILPAN) to m_weakFactory and use a
WeakPersistent from MainThreadTask to the MainThreadTaskRunner? (Maybe does it
need to be CrossThreadWeakPersistent?)

[sof, 2015-08-18 13:54:45]

https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
File Source/core/dom/MainThreadTaskRunner.h (left):

https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
Source/core/dom/MainThreadTaskRunner.h:67: WeakPtrFactory<MainThreadTaskRunner>
m_weakFactory;
On 2015/08/18 08:08:54, haraken wrote:
> On 2015/08/18 07:15:31, sof wrote:
> > Doesn't this need to be eagerly finalized?
> 
> I don't see any issue in not eagerly finalizing the MainThreadTaskRunner, but
I
> agree that it's nasty to let the MainThreadTask keep
WeakPtr<MainThreadRunner>.
> (i.e., it's nasty to have a WeakPtr to an on-heap object -- it should be a
> WeakPersistent or WeakMember.)
> 
> Would it be better to add #if !ENABLE(OILPAN) to m_weakFactory and use a
> WeakPersistent from MainThreadTask to the MainThreadTaskRunner? (Maybe does it
> need to be CrossThreadWeakPersistent?)
> 

Could you explain your reasoning behind that assessment?

It is a problem to have this task runner be able to access m_context if
it(Document) has already been finalized. Which it might have been if this object
isn't eagerly finalized.

[haraken, 2015-08-18 14:03:49]

On 2015/08/18 13:54:45, sof wrote:
>
https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
> File Source/core/dom/MainThreadTaskRunner.h (left):
> 
>
https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
> Source/core/dom/MainThreadTaskRunner.h:67:
WeakPtrFactory<MainThreadTaskRunner>
> m_weakFactory;
> On 2015/08/18 08:08:54, haraken wrote:
> > On 2015/08/18 07:15:31, sof wrote:
> > > Doesn't this need to be eagerly finalized?
> > 
> > I don't see any issue in not eagerly finalizing the MainThreadTaskRunner,
but
> I
> > agree that it's nasty to let the MainThreadTask keep
> WeakPtr<MainThreadRunner>.
> > (i.e., it's nasty to have a WeakPtr to an on-heap object -- it should be a
> > WeakPersistent or WeakMember.)
> > 
> > Would it be better to add #if !ENABLE(OILPAN) to m_weakFactory and use a
> > WeakPersistent from MainThreadTask to the MainThreadTaskRunner? (Maybe does
it
> > need to be CrossThreadWeakPersistent?)
> > 
> 
> Could you explain your reasoning behind that assessment?
> 
> It is a problem to have this task runner be able to access m_context if
> it(Document) has already been finalized. Which it might have been if this
object
> isn't eagerly finalized.

ah, thanks, I now understand the problem.

I think the problem is not that the MainThreadTaskRunner is not eagerly
finalized but that there is an unmanaged pointer (i.e., the WeakPtr from the
MainThreadTask to the MainThreadTaskRunner) even after the MainThreadTaskRunner
is determined to die. So I think a better fix would be to use WeakPersistent for
the MainThreadTask => MainThreadTaskRunner pointer.

[sof, 2015-08-18 14:13:57]

On 2015/08/18 14:03:49, haraken wrote:
> On 2015/08/18 13:54:45, sof wrote:
> >
>
https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
> > File Source/core/dom/MainThreadTaskRunner.h (left):
> > 
> >
>
https://codereview.chromium.org/1296243004/diff/1/Source/core/dom/MainThreadT...
> > Source/core/dom/MainThreadTaskRunner.h:67:
> WeakPtrFactory<MainThreadTaskRunner>
> > m_weakFactory;
> > On 2015/08/18 08:08:54, haraken wrote:
> > > On 2015/08/18 07:15:31, sof wrote:
> > > > Doesn't this need to be eagerly finalized?
> > > 
> > > I don't see any issue in not eagerly finalizing the MainThreadTaskRunner,
> but
> > I
> > > agree that it's nasty to let the MainThreadTask keep
> > WeakPtr<MainThreadRunner>.
> > > (i.e., it's nasty to have a WeakPtr to an on-heap object -- it should be a
> > > WeakPersistent or WeakMember.)
> > > 
> > > Would it be better to add #if !ENABLE(OILPAN) to m_weakFactory and use a
> > > WeakPersistent from MainThreadTask to the MainThreadTaskRunner? (Maybe
does
> it
> > > need to be CrossThreadWeakPersistent?)
> > > 
> > 
> > Could you explain your reasoning behind that assessment?
> > 
> > It is a problem to have this task runner be able to access m_context if
> > it(Document) has already been finalized. Which it might have been if this
> object
> > isn't eagerly finalized.
> 
> ah, thanks, I now understand the problem.
> 
> I think the problem is not that the MainThreadTaskRunner is not eagerly
> finalized but that there is an unmanaged pointer (i.e., the WeakPtr from the
> MainThreadTask to the MainThreadTaskRunner) even after the
MainThreadTaskRunner
> is determined to die. So I think a better fix would be to use WeakPersistent
for
> the MainThreadTask => MainThreadTaskRunner pointer.

Right, so as it is this needs to be eagerly finalized.

If you want to change PerformTaskContext to use WeakPersistent<>, you could do
that. But in any case this CL should not be landed in its current form.
That would be more fitting with Oilpan types, but wouldn't solve the problem
here. It still needs to be eagerly finalized.

[sof, 2015-08-18 14:17:24]

On 2015/08/18 14:13:57, sof wrote:
>
...
> That would be more fitting with Oilpan types, but wouldn't solve the problem
> here. It still needs to be eagerly finalized.

Please ignore that last sentence; it will work out with a WeakPersistent<>,
weakness-clearing happens before any finalization.

[Yuta Kitamura, 2015-08-19 05:10:17]

PTAL at PS2.

I assumed eager finalization was not necessary as per
sof's reply. (Correct me if I'm wrong.)

[haraken, 2015-08-19 05:18:17]

https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
File Source/core/dom/MainThreadTaskRunner.cpp (right):

https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
Source/core/dom/MainThreadTaskRunner.cpp:88:
Platform::current()->mainThread()->postTask(location, new
MainThreadTask(createWeakPointerToSelf(), task, false));

Maybe I'm wrong but this task is created on a non-main thread and used by the
main thread, right? Then I guess we need to use CrossThreadWeakPersistent.

[Yuta Kitamura, 2015-08-19 05:56:58]

https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
File Source/core/dom/MainThreadTaskRunner.cpp (right):

https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
Source/core/dom/MainThreadTaskRunner.cpp:88:
Platform::current()->mainThread()->postTask(location, new
MainThreadTask(createWeakPointerToSelf(), task, false));
On 2015/08/19 05:18:17, haraken wrote:
> 
> Maybe I'm wrong but this task is created on a non-main thread and used by the
> main thread, right? Then I guess we need to use CrossThreadWeakPersistent.
> 

You are right; it indeed can be called from non-main thread.

However, CrossThreadWeakPersistent does not exist at this moment.
Can I create it in another patch?

[haraken, 2015-08-19 06:02:58]

On 2015/08/19 05:56:58, Yuta Kitamura wrote:
>
https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
> File Source/core/dom/MainThreadTaskRunner.cpp (right):
> 
>
https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
> Source/core/dom/MainThreadTaskRunner.cpp:88:
> Platform::current()->mainThread()->postTask(location, new
> MainThreadTask(createWeakPointerToSelf(), task, false));
> On 2015/08/19 05:18:17, haraken wrote:
> > 
> > Maybe I'm wrong but this task is created on a non-main thread and used by
the
> > main thread, right? Then I guess we need to use CrossThreadWeakPersistent.
> > 
> 
> You are right; it indeed can be called from non-main thread.
> 
> However, CrossThreadWeakPersistent does not exist at this moment.
> Can I create it in another patch?

Sure!

[sof, 2015-08-19 06:37:56]

https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
File Source/core/dom/MainThreadTaskRunner.cpp (left):

https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
Source/core/dom/MainThreadTaskRunner.cpp:51: WeakPtr<MainThreadTaskRunner>
m_runner;
The GC plugin ought to warn/disallow WeakPtr<T> if T is a GCed type.

[haraken, 2015-08-19 06:46:17]

On 2015/08/19 06:37:56, sof wrote:
>
https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
> File Source/core/dom/MainThreadTaskRunner.cpp (left):
> 
>
https://codereview.chromium.org/1296243004/diff/20001/Source/core/dom/MainThr...
> Source/core/dom/MainThreadTaskRunner.cpp:51: WeakPtr<MainThreadTaskRunner>
> m_runner;
> The GC plugin ought to warn/disallow WeakPtr<T> if T is a GCed type.

Yes, filed a bug: https://code.google.com/p/chromium/issues/detail?id=522357

(Actually we won't have a bandwidth to fix the plugin issues though. Our first
priority is to fix the regression of core/animations/ and the peak memory
increase in memory.top_7_stress, and then flip the oilpan flag.)

[Yuta Kitamura, 2015-08-20 09:46:21]

CrossThreadWeakPersistent is landed. PTAL.

[haraken, 2015-08-20 09:47:05]

On 2015/08/20 09:46:21, Yuta Kitamura wrote:
> CrossThreadWeakPersistent is landed. PTAL.

Thanks, LGTM

[Yuta Kitamura, 2015-08-21 01:55:15]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2015-08-21 01:55:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1296243004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1296243004/40001

[commit-bot: I haz the power, 2015-08-21 07:31:44]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200972