This is the story: 1. The UI process close the last page 2. The connection of the web process is invalidated 3. The work queue of the connection is still running and it detects disconnection and call connectionDidClose 4. m_client is 0 so connectionDidClose does a null dereference That means we need a null check in connectionDidClose. I don't know exactly how ports other than Qt affected by the problem but a null check does not costs too much so I think it is better to handle that in the common part of the implementation. I investigated a lot of how should that fixed correctly and I think this is the best.
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>