Bug 155401 – WebKit can easily crash below NetworkSession::dataTaskForIdentifier() with NSURLSession enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

WebKit can easily crash below NetworkSession::dataTaskForIdentifier() with NSURLSession enabled

[Sam Weinig]

<rdar://problem/25129946>

[Sam Weinig]

Created attachment 273861 [details]
Patch

[Sam Weinig]

Created attachment 273862 [details]
Patch

[Sam Weinig]

Created attachment 273863 [details]
Patch

[Alex Christensen]

Comment on attachment 273863 [details]
Patch

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

This is subtle, but a correct improvement to the NetworkSession lifetime.  r=me

> Source/WebKit2/ChangeLog:14
> +        - Made NetworkSession RefCounted.

https://imgflip.com/i/10qt2g

> Source/WebKit2/ChangeLog:23
> +

Please mention in the changelog that you also changed the SessionTracker to track a NetworkSession which contains a NetworkStorageSession if using NETWORK_SESSION or just a NetworkStorageSession otherwise so that the NetworkSession/NetworkStorageSession management is done automatically.  This is an improvement.

> Source/WebKit2/NetworkProcess/NetworkDataTask.h:145
> +    RefPtr<NetworkSession> m_session;

Why not Ref<NetworkSession> ?

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:-61
> -        ASSERT_NOT_REACHED();

We may as well keep the ASSERT_NOT_REACHED after the WTFLogAlways.  If this ever happens, we want people to not be able to miss it, especially if they're using a debug build.

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-322
> -        } else
> -            ASSERT_NOT_REACHED();

Yep, this is a better design. Removing these assertions is great.

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:336
> +    if (!m_networkStorageSession)
> +        return WebCore::NetworkStorageSession::defaultStorageSession();
> +    return *m_networkStorageSession;

It would be nice to store the type from the constructor so we can assert m_type is Ephemeral if m_networkStorageSession and m_type is Normal if we are returning the defaultStorageSession.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:77
> +    SessionTracker::forEachNetworkStorageSession([&] (const NetworkStorageSession& networkStorageSession) {

This is nice, too.  No more public map.

[Sam Weinig]

Committed r198083: <http://trac.webkit.org/changeset/198083>