[Oilpan] Apply RefCountedGarbageCollectedEventTarget on AbstractWorker

Apply RefCountedGarbageCollectedEventTarget on AbstractWorker and its child classes.

Leave WorkerGlobalScope and its inheritances as RefCounted, 
because its migration needs to move ExecutionContext,
Document, and all Node hierarchies onto Oilpan heap.



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

[peria, 2015-08-12 14:53:20]

Patchset #2 (id:20001) has been deleted

[peria, 2015-08-12 14:53:31]

Patchset #2 (id:40001) has been deleted

[peria, 2015-08-12 14:53:42]

Patchset #2 (id:60001) has been deleted

[peria, 2015-08-12 14:53:54]

Patchset #2 (id:80001) has been deleted

[peria, 2015-08-12 14:54:03]

Patchset #2 (id:100001) has been deleted

[peria, 2015-08-12 14:54:14]

Patchset #3 (id:130001) has been deleted

[peria, 2015-08-12 14:54:21]

Patchset #3 (id:150001) has been deleted

[peria, 2015-08-12 14:54:30]

Patchset #2 (id:110001) has been deleted

[peria, 2015-08-12 14:54:41]

Patchset #1 (id:1) has been deleted

[peria, 2015-08-12 14:54:50]

Patchset #1 (id:170001) has been deleted

[peria, 2015-08-12 14:55:01]

Patchset #2 (id:210001) has been deleted

[peria, 2015-08-12 14:55:47]

Patchset #1 (id:190001) has been deleted

[peria, 2015-08-12 14:55:55]

Patchset #1 (id:230001) has been deleted

[peria, 2015-08-12 15:32:08]

Patchset #1 (id:250001) has been deleted

[peria, 2015-08-12 15:33:05]

Patchset #1 (id:270001) has been deleted

[peria, 2015-08-13 02:56:23]

Patchset #1 (id:290001) has been deleted

[peria, 2015-08-13 02:56:31]

Patchset #1 (id:310001) has been deleted

[peria, 2015-08-13 02:56:39]

Patchset #1 (id:330001) has been deleted

[peria, 2015-08-13 03:09:12]

Patchset #1 (id:350001) has been deleted

[peria, 2015-08-13 04:18:32]

peria@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[peria, 2015-08-13 04:18:33]

PTL

[haraken, 2015-08-13 06:09:23]

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

[haraken, 2015-08-13 06:09:24]

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Sh...
File Source/core/workers/SharedWorker.h (right):

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Sh...
Source/core/workers/SharedWorker.h:44: class CORE_EXPORT SharedWorker final :
public AbstractWorker, public WillBeHeapSupplementable<SharedWorker> {

You need to change WillBeSupplementable to HeapSupplementable. Supplementable
classes and supplementing classes need to be shipped at the same time.

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Wo...
File Source/core/workers/WorkerMessagingProxy.h (right):

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Wo...
Source/core/workers/WorkerMessagingProxy.h:100: InProcessWorkerBase*
m_workerObject;

This is interesting -- this looks like a real weak pointer. WorkerMessagingProxy
outlives the InProcessWorkerBase, and WorkerMessagingProxy::m_workerObject is
cleared in InProcessWorkerBase's destructor.

You can use RawPtrWillBeWeakPersistent. Also you can add #if !ENABLE(OILPAN) to
the code that clears out the WorkerMessagingProxy::m_workerObject.

[peria, 2015-08-13 06:34:00]

Patchset #2 (id:380001) has been deleted

[peria, 2015-08-13 06:35:28]

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Sh...
File Source/core/workers/SharedWorker.h (right):

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Sh...
Source/core/workers/SharedWorker.h:44: class CORE_EXPORT SharedWorker final :
public AbstractWorker, public WillBeHeapSupplementable<SharedWorker> {
On 2015/08/13 06:09:23, haraken wrote:
> 
> You need to change WillBeSupplementable to HeapSupplementable. Supplementable
> classes and supplementing classes need to be shipped at the same time.
> 

Done.

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Wo...
File Source/core/workers/WorkerMessagingProxy.h (right):

https://codereview.chromium.org/1212643004/diff/280031/Source/core/workers/Wo...
Source/core/workers/WorkerMessagingProxy.h:100: InProcessWorkerBase*
m_workerObject;
On 2015/08/13 06:09:23, haraken wrote:
> 
> This is interesting -- this looks like a real weak pointer.
WorkerMessagingProxy
> outlives the InProcessWorkerBase, and WorkerMessagingProxy::m_workerObject is
> cleared in InProcessWorkerBase's destructor.
> 
> You can use RawPtrWillBeWeakPersistent. Also you can add #if !ENABLE(OILPAN)
to
> the code that clears out the WorkerMessagingProxy::m_workerObject.

InProcessWorkerBase is being on heap in this CL, so I change this pointer to
WeakPersistent, and remove setting nullptr.

[haraken, 2015-08-13 07:58:49]

LGTM

https://codereview.chromium.org/1212643004/diff/400001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/1212643004/diff/400001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorker.cpp:110: return serviceWorker;

return getOrCreate(executionContext, worker);

[falken, 2015-08-13 08:03:29]

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

[falken, 2015-08-13 08:03:30]

typo: Oilapn

[falken, 2015-08-13 08:03:59]

Ah sorry, that's just the subject. Sorry for noise.

[peria, 2015-08-13 08:12:42]

On 2015/08/13 08:03:59, falken wrote:
> Ah sorry, that's just the subject. Sorry for noise.

Fixed. Thanks.

[peria, 2015-08-13 08:18:51]

The CQ bit was checked by peria@chromium.org

[peria, 2015-08-13 08:18:52]

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/1212643004/#ps420001
(title: "Work for a nit")

[commit-bot: I haz the power, 2015-08-13 08:18:55]

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

[commit-bot: I haz the power, 2015-08-13 09:21:41]

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

[commit-bot: I haz the power, 2015-08-13 09:21:42]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sof, 2015-08-13 20:44:01]

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

[sof, 2015-08-13 20:44:01]

https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
File Source/core/workers/WorkerMessagingProxy.cpp (left):

https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
Source/core/workers/WorkerMessagingProxy.cpp:205: m_workerObject = nullptr;
Why is clearing this promptly redundant now - can you guarantee that the posted
task from workerObjectDestroyed() won't touch m_workerObject somehow? (it seems
tidier overall to clear it to make the worker object detachment obvious.)

[haraken, 2015-08-13 23:53:03]

https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
File Source/core/workers/WorkerMessagingProxy.cpp (left):

https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
Source/core/workers/WorkerMessagingProxy.cpp:205: m_workerObject = nullptr;
On 2015/08/13 20:44:01, sof wrote:
> Why is clearing this promptly redundant now - can you guarantee that the
posted
> task from workerObjectDestroyed() won't touch m_workerObject somehow? (it
seems
> tidier overall to clear it to make the worker object detachment obvious.)

workerObjectDestroyed() is called in InProcessWorkerBase's destructor. Thus it
is guaranteed that m_workerObject is already cleared when the
workerObjectDestroyed() gets called.

Maybe worth having ASSERT(!m_workerObject).

[peria, 2015-08-14 03:43:41]

PTAL

https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
File Source/core/workers/WorkerMessagingProxy.cpp (left):

https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
Source/core/workers/WorkerMessagingProxy.cpp:205: m_workerObject = nullptr;
On 2015/08/13 23:53:03, haraken wrote:
> On 2015/08/13 20:44:01, sof wrote:
> > Why is clearing this promptly redundant now - can you guarantee that the
> posted
> > task from workerObjectDestroyed() won't touch m_workerObject somehow? (it
> seems
> > tidier overall to clear it to make the worker object detachment obvious.)
> 
> workerObjectDestroyed() is called in InProcessWorkerBase's destructor. Thus it
> is guaranteed that m_workerObject is already cleared when the
> workerObjectDestroyed() gets called.
> 
> Maybe worth having ASSERT(!m_workerObject).

Done.

[haraken, 2015-08-14 03:44:28]

On 2015/08/14 03:43:41, peria wrote:
> PTAL
> 
>
https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
> File Source/core/workers/WorkerMessagingProxy.cpp (left):
> 
>
https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
> Source/core/workers/WorkerMessagingProxy.cpp:205: m_workerObject = nullptr;
> On 2015/08/13 23:53:03, haraken wrote:
> > On 2015/08/13 20:44:01, sof wrote:
> > > Why is clearing this promptly redundant now - can you guarantee that the
> > posted
> > > task from workerObjectDestroyed() won't touch m_workerObject somehow? (it
> > seems
> > > tidier overall to clear it to make the worker object detachment obvious.)
> > 
> > workerObjectDestroyed() is called in InProcessWorkerBase's destructor. Thus
it
> > is guaranteed that m_workerObject is already cleared when the
> > workerObjectDestroyed() gets called.
> > 
> > Maybe worth having ASSERT(!m_workerObject).
> 
> Done.

LGTM

[peria, 2015-08-14 03:49:47]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-08-14 03:50:04]

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

[sof, 2015-08-14 05:05:50]

On 2015/08/13 23:53:03, haraken wrote:
>
https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
> File Source/core/workers/WorkerMessagingProxy.cpp (left):
> 
>
https://codereview.chromium.org/1212643004/diff/420001/Source/core/workers/Wo...
> Source/core/workers/WorkerMessagingProxy.cpp:205: m_workerObject = nullptr;
> On 2015/08/13 20:44:01, sof wrote:
> > Why is clearing this promptly redundant now - can you guarantee that the
> posted
> > task from workerObjectDestroyed() won't touch m_workerObject somehow? (it
> seems
> > tidier overall to clear it to make the worker object detachment obvious.)
> 
> workerObjectDestroyed() is called in InProcessWorkerBase's destructor. Thus it
> is guaranteed that m_workerObject is already cleared when the
> workerObjectDestroyed() gets called.
> 
> Maybe worth having ASSERT(!m_workerObject).

Yes, don't want this to break if that call moves. Or require the code reader to
understand lifetimes of several objects in combination here -- their
relationships are already quite involved and subtle.

[sof, 2015-08-14 05:06:57]

https://codereview.chromium.org/1212643004/diff/440001/Source/core/workers/Wo...
File Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1212643004/diff/440001/Source/core/workers/Wo...
Source/core/workers/WorkerMessagingProxy.cpp:205: ASSERT(!m_workerObject);
Spelling it out why it must be so would be a useful breadcrumb to the reader.

[peria, 2015-08-14 05:08:02]

The CQ bit was unchecked by peria@chromium.org

[peria, 2015-08-14 05:22:55]

Patchset #5 (id:460001) has been deleted

[peria, 2015-08-14 05:23:10]

https://codereview.chromium.org/1212643004/diff/440001/Source/core/workers/Wo...
File Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1212643004/diff/440001/Source/core/workers/Wo...
Source/core/workers/WorkerMessagingProxy.cpp:205: ASSERT(!m_workerObject);
On 2015/08/14 05:06:57, sof wrote:
> Spelling it out why it must be so would be a useful breadcrumb to the reader.

Done.

[sof, 2015-08-14 06:17:56]

lgtm

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorker.cpp:106: if (!worker)
nit: this null check is redundant.

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerContainer.h:46: #include
"wtf/PassRefPtr.h"
nit: wtf/ includes in header files involved in this CL can be trimmed a bit.

[peria, 2015-08-14 07:36:14]

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorker.cpp:106: if (!worker)
On 2015/08/14 06:17:55, sof wrote:
> nit: this null check is redundant.

Done.

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/1212643004/diff/480001/Source/modules/service...
Source/modules/serviceworkers/ServiceWorkerContainer.h:46: #include
"wtf/PassRefPtr.h"
On 2015/08/14 06:17:55, sof wrote:
> nit: wtf/ includes in header files involved in this CL can be trimmed a bit.

Done.
I may still have some #include which can be removed, though.

[peria, 2015-08-14 07:36:20]

The CQ bit was checked by peria@chromium.org

[peria, 2015-08-14 07:36:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1212643004/#ps520001
(title: "Remove some redundant includes")

[commit-bot: I haz the power, 2015-08-14 07:36:41]

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

[commit-bot: I haz the power, 2015-08-14 09:55:19]

Committed patchset #7 (id:520001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200532