Bug 51115 – Crash in web process after the connection had been closed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Balazs Kelemen]

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.

[Balazs Kelemen]

Note that the crash is only visible in the debugger.

[Balazs Kelemen]

Created attachment 76743 [details]
Patch

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 76988 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 76988 [details]
Patch

Looks sensible

[Balazs Kelemen]

Comment on attachment 76988 [details]
Patch

Clearing flags on attachment: 76988

Committed r74345: <http://trac.webkit.org/changeset/74345>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Anders Carlsson]

This patch is not correct, m_client must either be an atomic variable or protected by a lock. I'm going to revert this.

[Kenneth Rohde Christiansen]

(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.

[Anders Carlsson]

Reverted r74345 for reason:

Not the correct fix.

Committed r74355: <http://trac.webkit.org/changeset/74355>

[Balazs Kelemen]

Created attachment 77057 [details]
Patch

[Darin Adler]

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

[Darin Adler]

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.

[Balazs Kelemen]

(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.

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 78932 [details]
patch v4

Patch with the isValid/m_client change suggested by Darin.

[Balazs Kelemen]

ping

[Anders Carlsson]

(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.

[Anders Carlsson]

Created attachment 81556 [details]
Patch

[Anders Carlsson]

Committed r77874: <http://trac.webkit.org/changeset/77874>