Bug 170967 – ASAN Crash running LayoutTests/inspector/worker tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

Summary:
ASAN Crash running LayoutTests/inspector/worker tests

Crash:
Crashed Thread:        0  Dispatch queue: com.apple.main-thread

    #0 0x11a673672 in WebCore::WorkerMessagingProxy::postMessageToPageInspector(WTF::String const&)::$_3::operator()() const (/Volumes/Data/slave/sierra-asan-release-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x2f0f672)
    #1 0x112099403 in WTF::RunLoop::performWork() (/Volumes/Data/slave/sierra-asan-release-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1abe403)
    #2 0x112099c6e in WTF::RunLoop::performWork(void*) (/Volumes/Data/slave/sierra-asan-release-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1abec6e)
    #3 0x7fff89ed83b0 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0xa43b0)
    #4 0x7fff89eb963b in __CFRunLoopDoSources0 (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x8563b)
    #5 0x7fff89eb8b25 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x84b25)

Notes:
- Looks like this can happen if the WorkerMessagingProxy (which deletes itself) deletes itself while there is a still a postMessageToPageInspector block used for that proxy.
- I was unable to reproduce the test running LayoutTests/inspector/worker tests with GuardMalloc 100 iterations
- I was able to reproduce by forcing this theory to happen (call postMessageToPageInspector when triggering the proxy to go away)

Normal solution for these kinds of situations is to use ref counted objects.

[Joseph Pecoraro]

<rdar://problem/31256437>

[Joseph Pecoraro]

Created attachment 307419 [details]
[PATCH] Proposed Fix

[Alex Christensen]

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

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

> Source/WebCore/workers/WorkerMessagingProxy.cpp:242
> -        delete this;
> +        deref();

This is strange lifetime management.  Is there really nothing that can own this?  This change does make it so we can protect it in the lambda, though, so r=me on that.

[WebKit Commit Bot]

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

Clearing flags on attachment: 307419

Committed r215528: <http://trac.webkit.org/changeset/215528>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 307419 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307419&action=review
> 
> > Source/WebCore/workers/WorkerMessagingProxy.cpp:242
> > -        delete this;
> > +        deref();
> 
> This is strange lifetime management.  Is there really nothing that can own
> this?  This change does make it so we can protect it in the lambda, though,
> so r=me on that.

This is the clue I had to go on:

    WorkerGlobalScopeProxy& m_contextProxy; // The proxy outlives the worker to perform thread shutdown.

---

From the Worker Creator side (right now only Document on the Main Thread).

The Worker object creates the Messaging proxy:

    inline Worker::Worker(ScriptExecutionContext& context, JSC::RuntimeFlags runtimeFlags)
        ...
        , m_contextProxy(WorkerGlobalScopeProxy::create(*this))

And ~Worker tells the proxy to shut down the thread and destroy itself:

    Worker::~Worker()
    {
        ...
        m_contextProxy.workerObjectDestroyed();
    }

Which then goes through phases:

    • if WorkerThread => terminate worker thread
    • terminate self

----

From the Worker Thread side.

The WorkerGlobalScope has close() which terminates the thread but doesn't get rid of the Worker object which is the only object allowed to eventually cause deletion of the Proxy.

----

So it looks like the current behavior is:

  • Worker creates the Proxy
  • Worker can be destroyed before the WorkerGlobalScope (WorkerThread)
    => if the Worker object is GC'd but the worker thread is still alive and running
    => in this case someone must terminate the worker thread.

In that case, it is the responsibility of the Proxy to:

  • Terminate the Worker Thread
  • Destroy itself

So this really is a situation where there isn't a really good strong owner, since we want GC to be able to delete everything.