Bug 150857 – Create a NetworkSession for each SessionID

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

[Alex Christensen]

Allow multiple NetworkSessions

[Alex Christensen]

Created attachment 264726 [details]
Patch

[Alex Christensen]

Created attachment 264728 [details]
Patch

[Alex Christensen]

Created attachment 264731 [details]
Patch

[Darin Adler]

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?

[Alex Christensen]

Created attachment 264738 [details]
Patch

[Alex Christensen]

Created attachment 264739 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 264753 [details]
Patch

[Anders Carlsson]

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.

[Alex Christensen]

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

[Alex Christensen]

http://trac.webkit.org/changeset/191998