Bug 150857

Summary: Create a NetworkSession for each SessionID
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150834    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch andersca: review+

Description Alex Christensen 2015-11-03 14:23:26 PST
Allow multiple NetworkSessions
Comment 1 Alex Christensen 2015-11-03 14:29:34 PST
Created attachment 264726 [details]
Patch
Comment 2 Alex Christensen 2015-11-03 14:37:53 PST
Created attachment 264728 [details]
Patch
Comment 3 Alex Christensen 2015-11-03 14:47:09 PST
Created attachment 264731 [details]
Patch
Comment 4 Darin Adler 2015-11-03 14:51:02 PST
Comment on attachment 264728 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:195
> +    RELEASE_ASSERT(networkSession);

Why do we need to assert here at all? Particularly, why a RELEASE_ASSERT?

> Source/WebKit2/NetworkProcess/NetworkSession.h:95
> +    explicit NetworkDataTask(NetworkSession&, NetworkSessionTaskClient&, RetainPtr<NSURLSessionDataTask>);

The argument to this function should just be NSURLSessionDataTask *, not a RetainPtr. If you were trying to save retain count churn, maybe RetainPtr&&.

> Source/WebKit2/NetworkProcess/NetworkSession.h:115
> +    NetworkSession(Type, WebCore::SessionID);
> +    ~NetworkSession();

A little non-standard in style to declare the constructor and destructor after declaring other member functions.

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:177
> +static std::unique_ptr<NetworkSession>& defaultNetworkSession()
> +{
> +    ASSERT(isMainThread());
> +    static NeverDestroyed<std::unique_ptr<NetworkSession>> session;
> +    return session;
> +}

No need for this helper function.

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:183
> +    if (!defaultNetworkSession())
> +        defaultNetworkSession() = std::make_unique<NetworkSession>(NetworkSession::Type::Normal, WebCore::SessionID::defaultSessionID());
> +    return *defaultNetworkSession();

No helper function or make_unique needed:

    ASSERT(isMainThread());
    static NeverDestroyed<NetworkSession> session(NetworkSession::Type::Normal, WebCore::SessionID::defaultSessionID());
    return session;

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:202
> +        RELEASE_ASSERT(dataTask);

Why do we need to assert here at all? Particularly, why a RELEASE_ASSERT?
Comment 5 Alex Christensen 2015-11-03 15:26:20 PST
Created attachment 264738 [details]
Patch
Comment 6 Alex Christensen 2015-11-03 15:28:40 PST
Created attachment 264739 [details]
Patch
Comment 7 Alex Christensen 2015-11-03 15:57:05 PST
Comment on attachment 264739 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:195
> +    for (auto* dataTask : m_dataTaskMap.values())
> +        dataTask->cancel();

I'm not sure if this is correct.  I had an ASSERT(m_dataTaskMap.isEmpty()) before, but I wasn't sure that will always be true.  Now I think it might.
Comment 8 Alex Christensen 2015-11-03 16:36:11 PST
Created attachment 264753 [details]
Patch
Comment 9 Anders Carlsson 2015-11-03 16:41:48 PST
Comment on attachment 264753 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkSession.h:103
> +    friend class NetworkDataTask;

Why friend?

> Source/WebKit2/Shared/SessionTracker.cpp:85
> +    return map.get();

I think you can just return map here.
Comment 10 Alex Christensen 2015-11-03 16:49:30 PST
(In reply to comment #9)
> Comment on attachment 264753 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264753&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkSession.h:103
> > +    friend class NetworkDataTask;
> 
> Why friend?
A NetworkDataTask adds itself to m_dataTaskMap in its constructor and removes itself in the destructor.
> 
> > Source/WebKit2/Shared/SessionTracker.cpp:85
> > +    return map.get();
> 
> I think you can just return map here.
Yep, and a few other places in this file.
Comment 11 Alex Christensen 2015-11-03 16:51:31 PST
http://trac.webkit.org/changeset/191998