Bug 70902 – CRASH() in ScriptExecutionContext::destroyedActiveDOMObject()

• Component: WebCore Misc.
• Status: ASSIGNED

[Yuta Kitamura]

According to Chrome crash logs, there are some CRASH()es triggered by WebSocket/XMLHttpRequest destruction in worker thread (I'm not sure this happens on the main thread).

How this happens:

1. ScriptExecutionContext::stopActiveDOMObjects() iterates every ActiveDOMObject and call its stop() function.
  - During the iteration, we shouldn't add or delete the list of ActiveDOMObjects, because doing so invalidates the iterator. ... (*)
2. WebSocket::stop() may release the last reference to itself; in that case, WebSocket destructor is called.
3. WebSocket destructor calls ScriptExecutionContext::destroyedActiveDOMObject().
4. ScriptExecutionContext::destroyedActiveDOMObject() removes the object from the list of ActiveDOMObjects, which clearly conflicts with (*). So it CRASH()es.

Stack trace:

0x02619470	 [chrome.dll	 - activedomobject.cpp:50]	WebCore::ActiveDOMObject::~ActiveDOMObject()
0x024c90e0	 [chrome.dll	 - websocket.cpp:109]	WebCore::WebSocket::~WebSocket()
0x024c9061	 [chrome.dll	 + 0x00899061]	WebCore::WebSocket::`vector deleting destructor'(unsigned int)
0x02555e3e	 [chrome.dll	 - refcounted.h:141]	WTF::RefCounted<WebCore::WebSocket>::deref()
0x02612bc0	 [chrome.dll	 - scriptexecutioncontext.cpp:271]	WebCore::ScriptExecutionContext::stopActiveDOMObjects()
0x02491fee	 [chrome.dll	 - workerthread.cpp:202]	WebCore::WorkerThreadShutdownStartTask::performTask(WebCore::ScriptExecutionContext *)
0x02510cce	 [chrome.dll	 - workerrunloop.cpp:164]	WebCore::WorkerRunLoop::runInMode(WebCore::WorkerContext *,WebCore::ModePredicate const &)
0x02491f81	 [chrome.dll	 - workerthread.cpp:163]	WebCore::WorkerThread::runEventLoop()
0x02491f04	 [chrome.dll	 - workerthread.cpp:141]	WebCore::WorkerThread::workerThread()
0x02491df7	 [chrome.dll	 - workerthread.cpp:118]	WebCore::WorkerThread::workerThreadStart(void *)
0x0266e5f1	 [chrome.dll	 - threading.cpp:67]	WTF::threadEntryPoint
0x0266ac91	 [chrome.dll	 - threadingwin.cpp:197]	WTF::wtfThreadEntryPoint
0x02c9fd7d	 [chrome.dll	 - threadex.c:348]	_callthreadstartex
0x02c9fe25	 [chrome.dll	 - threadex.c:326]	_threadstartex
0x7c80b698	 [kernel32.dll	 + 0x0000b698]	BaseThreadStart

(XMLHttpRequest destruction also triggers the similar crash.)

[Yuta Kitamura]

Created attachment 112498 [details]
Patch

[Yuta Kitamura]

(In reply to comment #1)
> Created an attachment (id=112498) [details]
> Patch

I'm not sure if my approach is preferred; maybe we should fix ScriptExecutionContext's iteration logic.

[Alexey Proskuryakov]

See also: bug 56350, bug 52722, and especially bug 52719.

> I'm not sure if my approach is preferred; maybe we should fix ScriptExecutionContext's iteration logic.

I'm also not sure. Jeremy, you worked on this most intensively, would you be willing to bring this to conclusion?

A few comments though:

- Generally speaking, it's best to avoid delaying dangerous work on a timer. In many cases, delaying work from a clearly defined dangerous state until random time in the future causes hard to diagnose bugs. That said, I don't see a specific problem in this case.

- Separate task classes for XHR and for WebSocket seem unnecessary. Can they be merged?

- It's sad that a test couldn't be made.

[Darin Adler]

Comment on attachment 112498 [details]
Patch

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

Is always delaying a good thing? Will this hurt performance?

> Source/WebCore/websockets/WebSocket.cpp:454
> +    virtual void performTask(ScriptExecutionContext*)
> +    {
> +        if (m_webSocket->hasPendingActivity())
> +            m_webSocket->unsetPendingActivity(m_webSocket);
> +    }
> +
> +    virtual bool isCleanupTask() const { return true; }

These two functions should be private.

> Source/WebCore/websockets/WebSocket.cpp:469
> +        scriptExecutionContext()->postTask(adoptPtr(new DelayedWebSocketCleanupTask(this)));

I’d like to see a “why” comment explaining why this is delayed.

> Source/WebCore/xml/XMLHttpRequest.cpp:819
> +    scriptExecutionContext()->postTask(adoptPtr(new DelayedXMLHttpRequestCleanupTask(this)));

I’d like to see a “why” comment explaining why this is delayed.

[Jeremy Orlow]

(In reply to comment #3)
> See also: bug 56350, bug 52722, and especially bug 52719.
> 
> > I'm not sure if my approach is preferred; maybe we should fix ScriptExecutionContext's iteration logic.
> 
> I'm also not sure. Jeremy, you worked on this most intensively, would you be willing to bring this to conclusion?

Unfortunately, the specifics are completely paged out of my brain.  IIRC, there should be comments in bugs related to when the code was added.  I'm pretty sure we tried to find a way to simply fix it but could not.  There's a lot of weird behavior and assumptions around this logic.  A lot of hairy code would probably need to be reworked to do it right.  But, this is all based upon vague memories.  :-/

> A few comments though:
> 
> - Generally speaking, it's best to avoid delaying dangerous work on a timer. In many cases, delaying work from a clearly defined dangerous state until random time in the future causes hard to diagnose bugs. That said, I don't see a specific problem in this case.
> 
> - Separate task classes for XHR and for WebSocket seem unnecessary. Can they be merged?
> 
> - It's sad that a test couldn't be made.