PSA: LifecycleObserver::m_context starts returning nullptr after the context is detached

[Kentaro Hara]

Hi

I've been cleaning up lifecycle observers and fixing lifetime issues around Frame/Window/Document etc. Now I'm going to make the following change to LifecycleObserver:

(before) LifecycleObserver::m_context keeps returning an associated context even after LifecycleObserver::contextDestroyed gets called

(after) LifecycleObserver::m_context returns nullptr after LifecycleObserver::contextDestroyed gets called

For example, this means that ContextLifecycleObserver::getExecutionContext() starts returning nullptr after Document::detach() gets called.

The new behavior is much more correct but has a risk of causing nullptr-deref crashes if your code is assuming that ContextLifecycleObserver::getExecutionContext() never returns nullptr. I've fixed all problematic places detected in layout tests (CL), but it might not be sufficient. My plan is to fix all remaining places once I receive crash reports.

If you have any concern/questions, let me know. Thanks!

P.S. The ultimate goal of this refactoring is to unify all lifecycle observers about Frame/Document/Window into ContextLifecycleObserver. A tracking bug is here.

--

Kentaro Hara, Tokyo, Japan

[Giovanni Ortuño]

This doesn't affect ActiveDOMObject::Stop right?

[Kentaro Hara]

If affects. ActiveDOMObject::getExecutionContext() (which actually is the same thing as ContextLifecycleObserver::getExecutionContext()) starts returning nullptr after ActiveDOMObject::stop() is called.

[Giovanni Ortuño]

Damn, that's going to break bluetooth:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.cpp?sq=package:chromium&dr&rcl=1474485509&l=52

On Bluetooth we need a way to retrieve the context so that we can close all connections to a device before the context is destroyed. We can't just add a if (!getExecutionContext()) because that would mean we start to leak connections to devices. I guess this will be solve once we Onion Soup Web Bluetooth.

Gio 

[Daniel Cheng]

If the connections are supposed to be closed before the context is destroyed, isn't the time to do that in LifecycleContextDestroyed::contextDestroyed()? At that point, getExecutionContext() still works.

Daniel

[Giovanni Ortuño]

I tried that in this cl a while ago: http://crrev.com/1732393002

The problem is that we use the context to get a frame. When LifecycleContextDestroyed::contextDestroyed is called the context has no frame so we crash. Calling getExecutionContext from Stop() returned a context with a frame.

[Giovanni Ortuño]

from the right address:

I tried that in this cl a while ago: http://crrev.com/1732393002

The problem is that we use the context to get a frame. When LifecycleContextDestroyed::contextDestroyed is called the context has no frame so we crash. Calling getExecutionContext from Stop() returned a context with a frame.

On Thu, Sep 22, 2016 at 9:57 AM Daniel Cheng <dch...@chromium.org> wrote:

[Daniel Cheng]

We could just move m_frame = nullptr to the end of Document::shutdown. It's in a pretty arbitrary location at the moment.

However, it's also possible that BluetoothSupplement should just be a Document supplement, if it's supposed to be scoped to document lifetimes anyway.

Daniel