Make AbstractWorker a ContextLifecycleObserver rather than an ActiveDOMObject

Make AbstractWorker a ContextLifecycleObserver rather than an ActiveDOMObject

Per the plan described in https://groups.google.com/a/chromium.org/d/topic/blink-dev/UwRGJgLBElo/discussion,
this CL reduces ActiveDOMObjects in the code base.

- This CL replaces ServiceWorker::m_wasStopped with ContextLifecycleObserver::executionContext().

- This CL removes an 'if(AbstractWorker::hasPendingActivity)' check because it always returns true.

BUG=589507
Committed: https://crrev.com/21ab87586c14afb2a22c91b05d932563cba9569d
Cr-Commit-Position: refs/heads/master@{#378332}

[haraken, 2016-02-29 08:33:57]

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

[haraken, 2016-02-29 08:33:57]

PTAL

[kinuko, 2016-02-29 09:33:52]

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (left):

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:47:
suspendIfNeeded();
We no longer need this?

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.h:37: bool
hasPendingActivity() const override;
Can we keep a brief comment for overriding methods (with new base class names)?

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.h (right):

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.h:78: bool
hasPendingActivity() const override;
ditto

[haraken, 2016-02-29 10:52:47]

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (left):

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:47:
suspendIfNeeded();
On 2016/02/29 09:33:51, kinuko wrote:
> We no longer need this?

No. Actually we didn't need this even before this CL since InProcessWorkerBase
doesn't implement suspend/resume. (Accurately speaking, it was needed just to
avoid hitting an assert in ActiveDOMObject.h.)

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/1744973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/InProcessWorkerBase.h:37: bool
hasPendingActivity() const override;
On 2016/02/29 09:33:52, kinuko wrote:
> Can we keep a brief comment for overriding methods (with new base class
names)?

Done.

[haraken, 2016-02-29 11:20:21]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-02-29 11:20:21]

(cc Sigbjorn for sanity checking)

[haraken, 2016-02-29 11:20:43]

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

[commit-bot: I haz the power, 2016-02-29 11:20:53]

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

[commit-bot: I haz the power, 2016-02-29 12:28:31]

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

[commit-bot: I haz the power, 2016-02-29 12:28:32]

Dry run: This issue passed the CQ dry run.

[sof, 2016-02-29 13:34:32]

looks fine.

Kicked off a mac_blink_oilpan_rel trybot run; i _believe_ that is now building
with enable_oilpan=0.

[kinuko, 2016-02-29 13:53:24]

lgtm, thanks.

[haraken, 2016-02-29 23:37:06]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-02-29 23:37:41]

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

[commit-bot: I haz the power, 2016-03-01 00:06:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-01 00:07:59]

Description was changed from

==========
Make AbstractWorker a ContextLifecycleObserver rather than an ActiveDOMObject

Per the plan described in
https://groups.google.com/a/chromium.org/d/topic/blink-dev/UwRGJgLBElo/discus...,
this CL reduces ActiveDOMObjects in the code base.

- This CL replaces ServiceWorker::m_wasStopped with
ContextLifecycleObserver::executionContext().

- This CL removes an 'if(AbstractWorker::hasPendingActivity)' check because it
always returns true.

BUG=589507
==========

to

==========
Make AbstractWorker a ContextLifecycleObserver rather than an ActiveDOMObject

Per the plan described in
https://groups.google.com/a/chromium.org/d/topic/blink-dev/UwRGJgLBElo/discus...,
this CL reduces ActiveDOMObjects in the code base.

- This CL replaces ServiceWorker::m_wasStopped with
ContextLifecycleObserver::executionContext().

- This CL removes an 'if(AbstractWorker::hasPendingActivity)' check because it
always returns true.

BUG=589507

Committed: https://crrev.com/21ab87586c14afb2a22c91b05d932563cba9569d
Cr-Commit-Position: refs/heads/master@{#378332}
==========

[commit-bot: I haz the power, 2016-03-01 00:08:00]

Patchset 3 (id:??) landed as
https://crrev.com/21ab87586c14afb2a22c91b05d932563cba9569d
Cr-Commit-Position: refs/heads/master@{#378332}

[sof, 2016-03-02 19:59:57]

https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp:114: if
(!executionContext())
Using !executionContext() here rather than an explicit m_wasStopped is fine.

[haraken, 2016-03-03 02:39:38]

https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp:114: if
(!executionContext())
On 2016/03/02 19:59:57, sof wrote:
> Using !executionContext() here rather than an explicit m_wasStopped is fine.

Right.

[sof, 2016-03-03 10:03:14]

On 2016/03/03 02:39:38, haraken wrote:
>
https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp
(right):
> 
>
https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp:114: if
> (!executionContext())
> On 2016/03/02 19:59:57, sof wrote:
> > Using !executionContext() here rather than an explicit m_wasStopped is fine.
> 
> Right.

I was wrong, thinking that m_lifecycleContext would eventually be cleared, but
it extends the lifetime of workers for too long. cf. these leaks
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...

[haraken, 2016-03-03 10:05:23]

On 2016/03/03 10:03:14, sof wrote:
> On 2016/03/03 02:39:38, haraken wrote:
> >
>
https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1744973002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp:114: if
> > (!executionContext())
> > On 2016/03/02 19:59:57, sof wrote:
> > > Using !executionContext() here rather than an explicit m_wasStopped is
fine.
> > 
> > Right.
> 
> I was wrong, thinking that m_lifecycleContext would eventually be cleared, but
> it extends the lifetime of workers for too long. cf. these leaks
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...

Nice catch... Let's revert this. We need to (somehow) land
https://codereview.chromium.org/1752693002 first.

[haraken, 2016-03-03 10:05:47]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1765493002/ by haraken@chromium.org.

The reason for reverting is: I noticed that we're not ready to remove the
m_stopped variable (because
LifecycleObserver does not yet have an ability to clear
LifecycleObserver::m_context when the context is destroyed). We need to fix the
LifecycleObserver before landing this fix..