Bug 104922 - [WK2] Frequent NetworkProcess crashes due to null connection when closing tabs
Summary: [WK2] Frequent NetworkProcess crashes due to null connection when closing tabs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-12-13 09:13 PST by Alexey Proskuryakov
Modified: 2012-12-13 10:34 PST (History)
1 user (show)

See Also:


Attachments
proposed patch (3.57 KB, patch)
2012-12-13 09:17 PST, Alexey Proskuryakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-12-13 09:13:27 PST
When a tab is closed, WebProcess goes away, but NetworkProcess will still sometimes attempt to send messages to it if there were outstanding loads.

NetworkResourceLoader and NetworkConnectionToWebProcess both have m_connection that is zeroed on on connection close, and that causes null pointer crashes in IPC machinery.

This is easy to reproduce on sites that use long-standing connections, like gmail.

<rdar://problem/12870065>
Comment 1 Alexey Proskuryakov 2012-12-13 09:17:36 PST
Created attachment 179283 [details]
proposed patch
Comment 2 Oliver Hunt 2012-12-13 09:30:32 PST
Comment on attachment 179283 [details]
proposed patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-147
> -    m_connection->unregisterObserver(this);

Why arbitrarily delta this and not have if (RefPtr<blah> connection = m_connection) connection->unregisterObserver(this) ?

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-148
> -    m_connection = 0;

If connection is null this doesn't hurt, does it hurt to explicitly clear m_connection if it is non-null?
Comment 3 Alexey Proskuryakov 2012-12-13 10:04:17 PST
Comment on attachment 179283 [details]
proposed patch

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

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-147
>> -    m_connection->unregisterObserver(this);
> 
> Why arbitrarily delta this and not have if (RefPtr<blah> connection = m_connection) connection->unregisterObserver(this) ?

There is another unregisterObserver call in destructor, there is no reason to do this twice.

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-148
>> -    m_connection = 0;
> 
> If connection is null this doesn't hurt, does it hurt to explicitly clear m_connection if it is non-null?

I'm not sure if I understand the question.

m_connection being null is why this crash happens, so it definitely hurts to clear it.
Comment 4 Alexey Proskuryakov 2012-12-13 10:34:08 PST
Committed <http://trac.webkit.org/changeset/137610>.