Bug 161027 – Web Inspector: HeapProfiler/ScriptProfiler do not destruct safely when JSContext is destroyed

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Summary:
HeapProfiler/ScriptProfiler do not destruct safely when JSContext is destroyed.

Steps to Reproduce:
1. Launch App with JSContext
2. Remotely inspect JSContext
3. Start a Timeline Profile (ScriptProfiler + HeapAgent)
4. Dealloc JSContext in App
  => CRASH

Notes:
- InspectorScriptAgent and InspectorHeapAgent both need to take care in disconnecting when the JSGlobalObject may be destructing

[Joseph Pecoraro]

<rdar://problem/27871349>

[Joseph Pecoraro]

Created attachment 286525 [details]
[PATCH] Proposed Fix

[Mark Lam]

Comment on attachment 286525 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=286525&action=review

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:142
> +        if (reason == DisconnectReason::InspectedTargetDestroyed) {
> +            // We are inside of JSGlobalObject destruction.
> +            clearHeapSnapshotsWithLock();
> +        } else
> +            clearHeapSnapshots();
> +    }

Too bad we don't allow re-entrant locks.  Otherwise, we can do away with this kind of boilerplate.

What do you think of introducing a ConditionalJSLockHolder class?

// Add this to JSLock.h/cpp:
class ConditionalJSLockHolder {
public:
    ConditionalJSLockHolder(VM& vm, bool needToAcquireLock)
        : m_vm(&exec->vm())
        , m_needToAcquireLock(needToAcquireLock) 
    {
        if (m_needToAcquireLock)
            m_vm->apiLock().lock();
    }

    ~ConditionalJSLockHolder()
    {
        if (m_needToAcquireLock) {
            RefPtr<JSLock> apiLock(&m_vm->apiLock());
            m_vm = nullptr;
            apiLock->unlock();
        }
    }
};

// Use here in InspectorHeapAgent.cpp:
ConditionalJSLockHolder lock(m_environment.vm(), reason != DisconnectReason::InspectedTargetDestroyed);
clearHeapSnapshots(lock);

Would this simplify this patch?

[Joseph Pecoraro]

After discussion with Mark / Geoff we should better architect the interface between the VM/JSGlobalObject + Inspector.

For situations like this (Inspector reaching into a destructing VM) the approaches are typically:

    1. Strong Ref
    The Inspector can strongly Ref the VM to prevent it from destructing
    while it might want to use it.

    2. Weak Ref / "Signal Shutdown"
    The VM can signal it is shutting down and the Inspector can see that
    signal / state and act appropriately. One approach is the Inspector
    can weakly reference the VM, VM shutdown clears the weak reference,
    and the Inspector can then act appropriately later on based on
    whether or not the weak reference is cleared or not.

In the case of JSContext inspection, adding Strong ref behavior (1) by having Inspector strongly referencing the JSGlobalObject/VM when a frontend is opened and releasing that reference when the frontend is closed would change the existing UI behavior. The existing UI behavior is: When the JSContext is deallocated the inspector window for it closes. Maintaining that UI behavior would still be possible (tracking API level references and acting when there are zero), but maybe the new UI behavior (keep JSContext alive as long as a frontend is open) would be preferred. Admittedly, I've wanted to keep an inspector around for a short lived JSContext, to check its state. However I could see this being annoying with auto-attach never closing remote inspector windows for an app that uses many short-lived JSContexts.

The Weak Ref / Signal for inspector destruction right now is DisconnectReason::InspectedTargetDestroyed, which is what the first patch used, but is rather error prone.

I like (1). I think a debugger should keep alive what it is debugging. And ultimately approach (1) would allow for different UI behaviors, while approach (2) doesn't.

[Joseph Pecoraro]

I'm going with (1) here. It is simple. We should move in this direction.

I'll have a follow-up to reduce the work done on disconnect (which the original patch on this bug did as a side effect.

[Joseph Pecoraro]

Created attachment 287021 [details]
[PATCH] Proposed Fix

[Geoffrey Garen]

Comment on attachment 287021 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=287021&action=review

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:136
> +    m_globalObject.vm().ref();

Instead of manual ref / deref, I think we should use a RefPtr<VM> data member.

[Joseph Pecoraro]

Created attachment 287030 [details]
[PATCH] Proposed Fix (RefPtr)

[Build Bot]

Comment on attachment 287030 [details]
[PATCH] Proposed Fix (RefPtr)

Attachment 287030 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1943036

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html

[Build Bot]

Created attachment 287063 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Mark Lam]

Comment on attachment 287030 [details]
[PATCH] Proposed Fix (RefPtr)

View in context: https://bugs.webkit.org/attachment.cgi?id=287030&action=review

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:137
> +    m_strongGlobalObject.set(m_globalObject.vm(), &m_globalObject);

It seems strange to me that you keep a m_strongGlobalObject and at the same time have a m_globalObject reference.  It's also strange that the 2 being separate implies that the m_globalObject ref may be allowed to become invalid once upon a time before you had the m_strongGlobalObject.

I suggest:
1. Remove the m_globalObject C++ ref (which can become a stray pointer when the VM gets destroyed).
2. Rename m_strongGlobalObject to m_globalObject, and initialize it in the JSGlobalObjectInspectorController constructor.
3. Ref the VM in the JSGlobalObjectInspectorController constructor.

What do you think?

[Mark Lam]

Comment on attachment 287030 [details]
[PATCH] Proposed Fix (RefPtr)

View in context: https://bugs.webkit.org/attachment.cgi?id=287030&action=review

>> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:137
>> +    m_strongGlobalObject.set(m_globalObject.vm(), &m_globalObject);
> 
> It seems strange to me that you keep a m_strongGlobalObject and at the same time have a m_globalObject reference.  It's also strange that the 2 being separate implies that the m_globalObject ref may be allowed to become invalid once upon a time before you had the m_strongGlobalObject.
> 
> I suggest:
> 1. Remove the m_globalObject C++ ref (which can become a stray pointer when the VM gets destroyed).
> 2. Rename m_strongGlobalObject to m_globalObject, and initialize it in the JSGlobalObjectInspectorController constructor.
> 3. Ref the VM in the JSGlobalObjectInspectorController constructor.
> 
> What do you think?

Nevermind.  Joe explained that the life-cycle of JSGlobalObjectInspectorController is indeed tied to the JSGlobalObject.  When the JSGlobalObject is destructed, it will call JSGlobalObjectInspectorController::globalObjectDestroyed() to do clean up.  By convention, JSGlobalObjectInspectorController will never access the m_globalObject C++ reference after JSGlobalObjectInspectorController::globalObjectDestroyed() is called.

Also, m_globalObject cannot be a strong JS ref and initialized in the JSGlobalObjectInspectorController constructor is because that will prevent the JSGlobalObject from ever being GCed, even if Web Inspector was never attached.

[Mark Lam]

r=me

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/205033>