Changeset 67013 in webkit


Ignore:
Timestamp:
Sep 8, 2010, 2:16:45 PM (15 years ago)
Author:
Adam Roben
Message:

Teach WorkQueue how to stop waiting on objects on Windows

WorkQueue now uses a subclass of WorkItemWin, HandleWorkItem, to hold
the waited-upon HANDLE and its corresponding wait handle. When a
HANDLE is unregistered, we use the HandleWorkItem to cancel the wait
and destroy the HANDLE.

Fixes <​http://webkit.org/b/42826> <rdar://problem/8222253> Crash in
thread pool because WorkQueue keeps waiting on closed HANDLEs

Reviewed by Anders Carlsson.

  • Platform/CoreIPC/win/ConnectionWin.cpp:

(CoreIPC::Connection::platformInvalidate): Changed to call
WorkQueue::unregisterAndCloseHandle instead of closing our handles
manually.

(CoreIPC::Connection::readEventHandler):
(CoreIPC::Connection::writeEventHandler):
Handle cases where the pipe has already closed by just bailing out.
This can happen if a WorkItem to call one of these functions has
already been scheduled before platformInvalidate is called.

  • Platform/WorkQueue.h: Gave WorkItemWin a virtual destructor, added

HandleWorkItem, changed m_handles to hold HandleWorkItems, and added
functions for unregistering waits.

  • Platform/win/WorkQueueWin.cpp:

(WorkQueue::WorkItemWin::~WorkItemWin): Added. This virtual destructor
ensures that HandleWorkItem's destructor gets called.

(WorkQueue::HandleWorkItem::HandleWorkItem):
(WorkQueue::HandleWorkItem::createByAdoptingHandle):
Added simple constructor/creator.

(WorkQueue::HandleWorkItem::~HandleWorkItem): Closes the handle we
adopted.
(WorkQueue::registerHandle): Changed to create a HandleWorkItemWin and
to store the wait handle in it.
(WorkQueue::unregisterAndCloseHandle): Added. Removes the
HandleWorkItem for this HANDLE from m_handles and then schedules its
wait to be unregistered and the item to be destroyed.
(WorkQueue::platformInvalidate): Added an assertion and removed an
obsolete FIXME.
(WorkQueue::unregisterWaitAndDestroyItemSoon): Added. Calls
unregisterWaitAndDestroyItemCallback on a worker thread, passing it
ownership of the HandleWorkItem.
(WorkQueue::unregisterWaitAndDestroyItemCallback): Added. Adopts the
passed-in HandleWorkItem, then unregisters the handle's wait, then
destroys the HandleWorkItem when the RefPtr holding it goes out of
scope. Destroying the HandleWorkItem closes the handle.

Location:
trunk/WebKit2
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKit2/ChangeLog

    r67005 r67013  
     12010-09-08  Adam Roben  <aroben@apple.com>
     2
     3        Teach WorkQueue how to stop waiting on objects on Windows
     4
     5        WorkQueue now uses a subclass of WorkItemWin, HandleWorkItem, to hold
     6        the waited-upon HANDLE and its corresponding wait handle. When a
     7        HANDLE is unregistered, we use the HandleWorkItem to cancel the wait
     8        and destroy the HANDLE.
     9
     10        Fixes <http://webkit.org/b/42826> <rdar://problem/8222253> Crash in
     11        thread pool because WorkQueue keeps waiting on closed HANDLEs
     12
     13        Reviewed by Anders Carlsson.
     14
     15        * Platform/CoreIPC/win/ConnectionWin.cpp:
     16        (CoreIPC::Connection::platformInvalidate): Changed to call
     17        WorkQueue::unregisterAndCloseHandle instead of closing our handles
     18        manually.
     19
     20        (CoreIPC::Connection::readEventHandler):
     21        (CoreIPC::Connection::writeEventHandler):
     22        Handle cases where the pipe has already closed by just bailing out.
     23        This can happen if a WorkItem to call one of these functions has
     24        already been scheduled before platformInvalidate is called.
     25
     26        * Platform/WorkQueue.h: Gave WorkItemWin a virtual destructor, added
     27        HandleWorkItem, changed m_handles to hold HandleWorkItems, and added
     28        functions for unregistering waits.
     29
     30        * Platform/win/WorkQueueWin.cpp:
     31        (WorkQueue::WorkItemWin::~WorkItemWin): Added. This virtual destructor
     32        ensures that HandleWorkItem's destructor gets called.
     33
     34        (WorkQueue::HandleWorkItem::HandleWorkItem):
     35        (WorkQueue::HandleWorkItem::createByAdoptingHandle):
     36        Added simple constructor/creator.
     37
     38        (WorkQueue::HandleWorkItem::~HandleWorkItem): Closes the handle we
     39        adopted.
     40        (WorkQueue::registerHandle): Changed to create a HandleWorkItemWin and
     41        to store the wait handle in it.
     42        (WorkQueue::unregisterAndCloseHandle): Added. Removes the
     43        HandleWorkItem for this HANDLE from m_handles and then schedules its
     44        wait to be unregistered and the item to be destroyed.
     45        (WorkQueue::platformInvalidate): Added an assertion and removed an
     46        obsolete FIXME.
     47        (WorkQueue::unregisterWaitAndDestroyItemSoon): Added. Calls
     48        unregisterWaitAndDestroyItemCallback on a worker thread, passing it
     49        ownership of the HandleWorkItem.
     50        (WorkQueue::unregisterWaitAndDestroyItemCallback): Added. Adopts the
     51        passed-in HandleWorkItem, then unregisters the handle's wait, then
     52        destroys the HandleWorkItem when the RefPtr holding it goes out of
     53        scope. Destroying the HandleWorkItem closes the handle.
     54
    1552010-09-08  Adam Roben  <aroben@apple.com>
    256

  • trunk/WebKit2/Platform/CoreIPC/win/ConnectionWin.cpp

    r66231 r67013  
    9999        return;
    100100
    101     // FIXME: Unregister the handles.
    102 
    103     ::CloseHandle(m_readState.hEvent);
     101    m_connectionQueue.unregisterAndCloseHandle(m_readState.hEvent);
    104102    m_readState.hEvent = 0;
    105103
    106     ::CloseHandle(m_writeState.hEvent);
     104    m_connectionQueue.unregisterAndCloseHandle(m_writeState.hEvent);
    107105    m_writeState.hEvent = 0;
    108106
    … …  
    113111void Connection::readEventHandler()
    114112{
     113    if (m_connectionPipe == INVALID_HANDLE_VALUE)
     114        return;
     115
    115116    while (true) {
    116117        // Check if we got some data.
    … …  
    232233void Connection::writeEventHandler()
    233234{
     235    if (m_connectionPipe == INVALID_HANDLE_VALUE)
     236        return;
     237
    234238    DWORD numberOfBytesWritten = 0;
    235239    if (!::GetOverlappedResult(m_connectionPipe, &m_writeState, &numberOfBytesWritten, FALSE)) {

  • trunk/WebKit2/Platform/WorkQueue.h

    r66999 r67013  
    6969#elif PLATFORM(WIN)
    7070    void registerHandle(HANDLE, PassOwnPtr<WorkItem>);
    71     void unregisterHandle(HANDLE);
     71    void unregisterAndCloseHandle(HANDLE);
    7272#elif PLATFORM(QT)
    7373    void connectSignal(QObject*, const char* signal, PassOwnPtr<WorkItem>);
    … …  
    9797    public:
    9898        static PassRefPtr<WorkItemWin> create(PassOwnPtr<WorkItem>, WorkQueue*);
     99        virtual ~WorkItemWin();
    99100
    100101        WorkItem* item() const { return m_item.get(); }
    101102        WorkQueue* queue() const { return m_queue; }
    102103
    103     private:
     104    protected:
    104105        WorkItemWin(PassOwnPtr<WorkItem>, WorkQueue*);
    105106
     107    private:
    106108        OwnPtr<WorkItem> m_item;
    107109        WorkQueue* m_queue;
     110    };
     111
     112    class HandleWorkItem : public WorkItemWin {
     113    public:
     114        static PassRefPtr<HandleWorkItem> createByAdoptingHandle(HANDLE, PassOwnPtr<WorkItem>, WorkQueue*);
     115        virtual ~HandleWorkItem();
     116
     117        void setWaitHandle(HANDLE waitHandle) { m_waitHandle = waitHandle; }
     118        HANDLE waitHandle() const { return m_waitHandle; }
     119
     120    private:
     121        HandleWorkItem(HANDLE, PassOwnPtr<WorkItem>, WorkQueue*);
     122
     123        HANDLE m_handle;
     124        HANDLE m_waitHandle;
    108125    };
    109126
    … …  
    115132    void performWorkOnRegisteredWorkThread();
    116133
     134    static void unregisterWaitAndDestroyItemSoon(PassRefPtr<HandleWorkItem>);
     135    static DWORD WINAPI unregisterWaitAndDestroyItemCallback(void* context);
     136
    117137    volatile LONG m_isWorkThreadRegistered;
    118138
    … …  
    121141
    122142    Mutex m_handlesLock;
    123     HashMap<HANDLE, RefPtr<WorkItemWin> > m_handles;
     143    HashMap<HANDLE, RefPtr<HandleWorkItem> > m_handles;
    124144#elif PLATFORM(QT)
    125145    class WorkItemQt;

  • trunk/WebKit2/Platform/win/WorkQueueWin.cpp

    r66999 r67013  
    3939}
    4040
     41WorkQueue::WorkItemWin::~WorkItemWin()
     42{
     43}
     44
     45inline WorkQueue::HandleWorkItem::HandleWorkItem(HANDLE handle, PassOwnPtr<WorkItem> item, WorkQueue* queue)
     46    : WorkItemWin(item, queue)
     47    , m_handle(handle)
     48    , m_waitHandle(0)
     49{
     50    ASSERT_ARG(handle, handle);
     51}
     52
     53PassRefPtr<WorkQueue::HandleWorkItem> WorkQueue::HandleWorkItem::createByAdoptingHandle(HANDLE handle, PassOwnPtr<WorkItem> item, WorkQueue* queue)
     54{
     55    return adoptRef(new HandleWorkItem(handle, item, queue));
     56}
     57
     58WorkQueue::HandleWorkItem::~HandleWorkItem()
     59{
     60    ::CloseHandle(m_handle);
     61}
     62
    4163void WorkQueue::handleCallback(void* context, BOOLEAN timerOrWaitFired)
    4264{
    … …  
    6688void WorkQueue::registerHandle(HANDLE handle, PassOwnPtr<WorkItem> item)
    6789{
    68     RefPtr<WorkItemWin> itemWin = WorkItemWin::create(item, this);
    69 
    70     // Add the item.
     90    RefPtr<HandleWorkItem> handleItem = HandleWorkItem::createByAdoptingHandle(handle, item, this);
     91
     92    {
     93        MutexLocker lock(m_handlesLock);
     94        ASSERT_ARG(handle, !m_handles.contains(handle));
     95        m_handles.set(handle, handleItem);
     96    }
     97
     98    HANDLE waitHandle;
     99    if (!::RegisterWaitForSingleObject(&waitHandle, handle, handleCallback, handleItem.get(), INFINITE, WT_EXECUTEDEFAULT)) {
     100        DWORD error = ::GetLastError();
     101        ASSERT_NOT_REACHED();
     102    }
     103    handleItem->setWaitHandle(waitHandle);
     104}
     105
     106void WorkQueue::unregisterAndCloseHandle(HANDLE handle)
     107{
     108    RefPtr<HandleWorkItem> item;
    71109    {
    72110        MutexLocker locker(m_handlesLock);
    73         m_handles.set(handle, itemWin);
    74     }
    75 
    76     // FIXME: We need to hold onto waitHandle so that we can unregister the wait later.
    77     HANDLE waitHandle;
    78     if (!::RegisterWaitForSingleObject(&waitHandle, handle, handleCallback, itemWin.get(), INFINITE, WT_EXECUTEDEFAULT)) {
    79         DWORD error = ::GetLastError();
    80         ASSERT_NOT_REACHED();
    81     }
     111        ASSERT_ARG(handle, m_handles.contains(handle));
     112        item = m_handles.take(handle);
     113    }
     114
     115    unregisterWaitAndDestroyItemSoon(item.release());
    82116}
    83117
    … …  
    147181void WorkQueue::platformInvalidate()
    148182{
    149     // FIXME: Stop the thread and do other cleanup.
     183#if !ASSERT_DISABLED
     184    MutexLocker lock(m_handlesLock);
     185    ASSERT(m_handles.isEmpty());
     186#endif
    150187}
    151188
    … …  
    165202        ::QueueUserWorkItem(workThreadCallback, this, WT_EXECUTEDEFAULT);
    166203}
     204
     205void WorkQueue::unregisterWaitAndDestroyItemSoon(PassRefPtr<HandleWorkItem> item)
     206{
     207    // We're going to make a blocking call to ::UnregisterWaitEx before closing the handle. (The
     208    // blocking version of ::UnregisterWaitEx is much simpler than the non-blocking version.) If we
     209    // do this on the current thread, we'll deadlock if we're currently in a callback function for
     210    // the wait we're unregistering. So instead we do it asynchronously on some other worker thread.
     211
     212    ::QueueUserWorkItem(unregisterWaitAndDestroyItemCallback, item.leakRef(), WT_EXECUTEDEFAULT);
     213}
     214
     215DWORD WINAPI WorkQueue::unregisterWaitAndDestroyItemCallback(void* context)
     216{
     217    ASSERT_ARG(context, context);
     218    RefPtr<HandleWorkItem> item = adoptRef(static_cast<HandleWorkItem*>(context));
     219
     220    // Now that we know we're not in a callback function for the wait we're unregistering, we can
     221    // make a blocking call to ::UnregisterWaitEx.
     222    if (!::UnregisterWaitEx(item->waitHandle(), INVALID_HANDLE_VALUE)) {
     223        DWORD error = ::GetLastError();
     224        ASSERT_NOT_REACHED();
     225    }
     226
     227    return 0;
     228}
Note: See TracChangeset for help on using the changeset viewer.