Summary: | Crash in web process after the connection had been closed | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | andersca, kenneth, kling, sam | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Balazs Kelemen
2010-12-15 09:48:17 PST
Note that the crash is only visible in the debugger. Created attachment 76743 [details]
Patch
After I got an assert in connectionDidClose while working on other stuff but having this patch in my branch, I decided to fix by not calling connectionDidClose at all when the connection had been invalidated. That would rather need a Qt only change. Created attachment 76988 [details]
Patch
Comment on attachment 76988 [details]
Patch
Looks sensible
Comment on attachment 76988 [details] Patch Clearing flags on attachment: 76988 Committed r74345: <http://trac.webkit.org/changeset/74345> All reviewed patches have been landed. Closing bug. This patch is not correct, m_client must either be an atomic variable or protected by a lock. I'm going to revert this. (In reply to comment #8) > This patch is not correct, m_client must either be an atomic variable or protected by a lock. I'm going to revert this. Please do. Reverted r74345 for reason: Not the correct fix. Committed r74355: <http://trac.webkit.org/changeset/74355> Created attachment 77057 [details]
Patch
Comment on attachment 77057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77057&action=review I think Anders should review, but I do have one comment. > WebKit2/Platform/CoreIPC/Connection.cpp:349 > + m_client = 0; // No need to lock here since the connectino has been closed on the work thread. Typo: connectino Comment on attachment 77057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77057&action=review I’d like to review, but I’d like to hear from Anders whether he thinks this is the right solution. > WebKit2/ChangeLog:8 > + Add mutual exclusion for m_client. Typically for a single pointer we could use a function like the Mac OS X OSAtomicCompareAndSwapPtrBarrier instead of a mutex. I don’t know if that’s desirable here or not. > WebKit2/Platform/CoreIPC/Connection.cpp:342 > - if (!m_client) > + if (!isValid()) Is this better? Generally speaking if code is going to be manipulating m_client directly, I am not sure that isValid is providing a helpful abstraction inside the Connection class. (In reply to comment #13) > (From update of attachment 77057 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77057&action=review > > I’d like to review, but I’d like to hear from Anders whether he thinks this is the right solution. > > > WebKit2/ChangeLog:8 > > + Add mutual exclusion for m_client. > > Typically for a single pointer we could use a function like the Mac OS X OSAtomicCompareAndSwapPtrBarrier instead of a mutex. I don’t know if that’s desirable here or not. I don't have a strong opinion about that. However, if we want atomic functionality then I think it should be introduced into WTF for all platforms. > > > WebKit2/Platform/CoreIPC/Connection.cpp:342 > > - if (!m_client) > > + if (!isValid()) > > Is this better? Generally speaking if code is going to be manipulating m_client directly, I am not sure that isValid is providing a helpful abstraction inside the Connection class. Yes, maybe just saying |!m_client| is more straightforward here. I should refine my opinion about using atomic here. This is not enough in the following situation: { Client* client = ATOMIC_READ(m_client); if (!client) return; --> Right now the listener thread invalidates the connection. --> After that the client is allowed to be destructed so we can end up in --> accessing a deleted object in the following line. client->didCloseOnConnectionWorkQueue(&m_connectionQueue, this); } So the didClose... call should be in a mutually excluded block. Created attachment 78932 [details]
patch v4
Patch with the isValid/m_client change suggested by Darin.
ping (In reply to comment #17) > ping Sorry for not looking at this earlier. I've decided to go with a simpler approach and get rid of the Client::connectionDidCloseOnWorkQueue. Created attachment 81556 [details]
Patch
Committed r77874: <http://trac.webkit.org/changeset/77874> |