LifecycleObserver::contextDestroyed should be explicitly dispatched promptly when a context is dest…

LifecycleObserver::contextDestroyed should be explicitly dispatched promptly when a context is destroyed

Currently LifecycleObserver::contextDestroyed is dispatched within LifecycleContext's destructor. This is problematic in Oilpan because destructors are not allowed to touch other on-heap objects. We need to dispatch LifecycleObserver::contextDestroyed not when the context is destructed but promptly when the context is destroyed (e.g., Document::detach(), WorkerGlobalScope::dispose(), Page::willBeDestroyed()).

BUG=340522

[haraken, 2014-12-23 03:38:01]

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

[haraken, 2014-12-23 03:43:44]

This CL is not yet ready for review.

- This CL is necessary to move LifecycleObserver's hierarchy to Oilpan's heap.

- I don't understand why not having this CL in trunk hasn't been caused any
lifetime issue in trunk... Currently contextDestroyed() is called within
LifecycleContext's destructor, and thus when
LifecycleObserver::lifecycleContext() returns 0 depends on when the
LifecycleContext's destructor gets called. In other words,
ActiveDOMObject::executionContext() returns a valid ExecutionContext or 0
depending on when a GC is triggered. I'm afraid this is an on-going problem in
trunk...

https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
Source/core/frame/LocalDOMWindow.cpp:632:
LifecycleContext<LocalDOMWindow>::contextDestroyed();

This is not a right timing to dispatch contextDestoyed(). It is possible that
LocalDOMWindow gets destructed without LocalDOMWindow::reset() getting called,
and thus this CL hits ASSERT(!m_lifecycleNotifier) in ~LocalDOMWindow().

sof@: Do you probably know a timing when to dispatch
LifecycleContext<LocalDOMWindow>::contextDestoyed()?

[sof, 2014-12-23 14:40:30]

https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
Source/core/frame/LocalDOMWindow.cpp:632:
LifecycleContext<LocalDOMWindow>::contextDestroyed();
On 2014/12/23 03:43:44, haraken wrote:
> 
> This is not a right timing to dispatch contextDestoyed(). It is possible that
> LocalDOMWindow gets destructed without LocalDOMWindow::reset() getting called,
> and thus this CL hits ASSERT(!m_lifecycleNotifier) in ~LocalDOMWindow().
> 
> sof@: Do you probably know a timing when to dispatch
> LifecycleContext<LocalDOMWindow>::contextDestoyed()?

Would it work to tie contextDestroyed() for a DOMWindow to frame destruction
notifications?

[haraken, 2014-12-24 07:11:49]

On 2014/12/23 14:40:30, sof wrote:
>
https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
> Source/core/frame/LocalDOMWindow.cpp:632:
> LifecycleContext<LocalDOMWindow>::contextDestroyed();
> On 2014/12/23 03:43:44, haraken wrote:
> > 
> > This is not a right timing to dispatch contextDestoyed(). It is possible
that
> > LocalDOMWindow gets destructed without LocalDOMWindow::reset() getting
called,
> > and thus this CL hits ASSERT(!m_lifecycleNotifier) in ~LocalDOMWindow().
> > 
> > sof@: Do you probably know a timing when to dispatch
> > LifecycleContext<LocalDOMWindow>::contextDestoyed()?
> 
> Would it work to tie contextDestroyed() for a DOMWindow to frame destruction
> notifications?

I tried a couple of things but couldn't find a good place to dispatch
LifecycleContext<LocalDOMWindow>::contextDestoyed(). It seems there is no
explicit point where LocalDOMWindow is "disposed".

I begin to think that DOMWindowLifecycleObserver is not necessary. Conceptually
DOMWindowLifecycleObserver should be replaced with something like
FrameLifecycleObserver. (Currently we don't have FrameLifecycleObserver, but
have FrameDestructionObserver.)

More generally, I'm considering the following refactoring:

- ContextLifecycleObserver => keep it
- DOMWindowLifecycleObserver => replace it with FrameLifecycleObserver
- FrameDestructionObserver => replace it with FrameLifecycleObserver
- PageDestructionObserver => replace it with FrameLifecycleObserver
- DocumentLifecycleObserver => replace it with ContextLifecycleObserver

That being said, this requires a substantial amount of work. I'm thinking about
a more short-term way to unblock our lazy sweeping work.

[sof, 2014-12-24 09:14:40]

On 2014/12/24 07:11:49, haraken wrote:
> On 2014/12/23 14:40:30, sof wrote:
> >
>
https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
> > File Source/core/frame/LocalDOMWindow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/822863003/diff/1/Source/core/frame/LocalDOMWi...
> > Source/core/frame/LocalDOMWindow.cpp:632:
> > LifecycleContext<LocalDOMWindow>::contextDestroyed();
> > On 2014/12/23 03:43:44, haraken wrote:
> > > 
> > > This is not a right timing to dispatch contextDestoyed(). It is possible
> that
> > > LocalDOMWindow gets destructed without LocalDOMWindow::reset() getting
> called,
> > > and thus this CL hits ASSERT(!m_lifecycleNotifier) in ~LocalDOMWindow().
> > > 
> > > sof@: Do you probably know a timing when to dispatch
> > > LifecycleContext<LocalDOMWindow>::contextDestoyed()?
> > 
> > Would it work to tie contextDestroyed() for a DOMWindow to frame destruction
> > notifications?
> 
> I tried a couple of things but couldn't find a good place to dispatch
> LifecycleContext<LocalDOMWindow>::contextDestoyed(). It seems there is no
> explicit point where LocalDOMWindow is "disposed".
> 
> I begin to think that DOMWindowLifecycleObserver is not necessary.
Conceptually
> DOMWindowLifecycleObserver should be replaced with something like
> FrameLifecycleObserver. (Currently we don't have FrameLifecycleObserver, but
> have FrameDestructionObserver.)
> 

Yes, not used all that much. DOMWindowLifecycleObserver's notifications about
EventListener registration changes is used, but the null checks over window() in
NavigatorGamepad seems to be the only place the LocalDOMWindow's end-of-life is
"observed", afaict.

Have you investigated if those window() uses could be replaced with frame()
instead, and if so, pare down DOMWin*Observer and only providing an observer
interface for window event listeners? That way there's no need to make
LocalDOMWindow's destruction observable. 

Non-Oilpan, isn't LocalDOMWindow::dispose() effectively equal to ~LocalFrame ?
As you say, it wouldn't be non-sensical to tie window destruction notification
to the initiation of frame destruction on the Oilpan side.

[sof, 2014-12-26 08:21:42]

just some trivial comments i forgot to flush earlier..

https://codereview.chromium.org/822863003/diff/1/Source/platform/LifecycleCon...
File Source/platform/LifecycleContext.h (right):

https://codereview.chromium.org/822863003/diff/1/Source/platform/LifecycleCon...
Source/platform/LifecycleContext.h:50: ASSERT(!m_lifecycleNotifier);
Add a comment next to this to indicate what might be missing?

https://codereview.chromium.org/822863003/diff/1/Source/platform/LifecycleCon...
Source/platform/LifecycleContext.h:65: void contextDestroyed()
nit: add an empty line.

[sof, 2015-01-16 09:22:52]

Can be closed, right?

[haraken, 2015-01-16 09:23:22]

On 2015/01/16 09:22:52, sof wrote:
> Can be closed, right?

yes, closed.