Bug 155401 - WebKit can easily crash below NetworkSession::dataTaskForIdentifier() with NSURLSession enabled
Summary: WebKit can easily crash below NetworkSession::dataTaskForIdentifier() with NS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-12 17:41 PST by Sam Weinig
Modified: 2016-03-13 13:11 PDT (History)
7 users (show)

See Also:

Attachments
Patch (47.27 KB, patch)
2016-03-12 17:58 PST, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (48.81 KB, patch)
2016-03-12 18:01 PST, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (48.81 KB, patch)
2016-03-12 18:09 PST, Sam Weinig 		achristensen: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-03-12 17:41:23 PST 
WebKit can easily crash below NetworkSession::dataTaskForIdentifier() with NSURLSession enabled
Comment 1 Sam Weinig 2016-03-12 17:41:54 PST 
<rdar://problem/25129946>
Comment 2 Sam Weinig 2016-03-12 17:58:11 PST 
Created attachment 273861 [details]
Patch
Comment 3 Sam Weinig 2016-03-12 18:01:30 PST 
Created attachment 273862 [details]
Patch
Comment 4 Sam Weinig 2016-03-12 18:09:31 PST 
Created attachment 273863 [details]
Patch
Comment 5 Alex Christensen 2016-03-12 18:37:34 PST 
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.
Comment 6 Sam Weinig 2016-03-13 13:11:46 PDT 
Committed r198083: <http://trac.webkit.org/changeset/198083>