Bug 154984 – Use NetworkSession for pings when using NetworkSession

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

[Alex Christensen]

Use NetworkSession for pings when using NetworkSession

[Alex Christensen]

Created attachment 272779 [details]
Patch

[Alex Christensen]

Created attachment 272786 [details]
Patch

[Darin Adler]

Comment on attachment 272786 [details]
Patch

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

> Source/WebKit2/NetworkProcess/PingLoad.h:71
> +    virtual void willPerformHTTPRedirection(const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, RedirectCompletionHandler completionHandler)
> +    {
> +        completionHandler({ });
> +        delete this;
> +    }
> +    virtual void didReceiveChallenge(const WebCore::AuthenticationChallenge&, ChallengeCompletionHandler completionHandler)
> +    {
> +        completionHandler(AuthenticationChallengeDisposition::Cancel, { });
> +        delete this;
> +    }
> +    virtual void didReceiveResponseNetworkSession(const WebCore::ResourceResponse&, ResponseCompletionHandler completionHandler)
> +    {
> +        completionHandler(WebCore::PolicyAction::PolicyIgnore);
> +        delete this;
> +    }
> +    virtual void didReceiveData(RefPtr<WebCore::SharedBuffer>&&) { ASSERT_NOT_REACHED(); }
> +    virtual void didCompleteWithError(const WebCore::ResourceError&) { delete this; }
> +    virtual void didBecomeDownload() { ASSERT_NOT_REACHED(); }
> +    virtual void didSendData(uint64_t totalBytesSent, uint64_t totalBytesExpectedToSend) { ASSERT_NOT_REACHED(); }
> +    virtual void wasBlocked() { delete this; }
> +    virtual void cannotShowURL() { delete this; }

These should all be marked override, and not virtual.

[Alex Christensen]

Created attachment 272833 [details]
Patch

[Alex Christensen]

Comment on attachment 272833 [details]
Patch

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

> Source/WebKit2/NetworkProcess/PingLoad.h:69
> +    void didSendData(uint64_t totalBytesSent, uint64_t totalBytesExpectedToSend) override { ASSERT_NOT_REACHED(); }

This assertion was being hit in many tests.  Will commit without it if reviewed.

[Antti Koivisto]

Comment on attachment 272833 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Use NetworkSession for pings when using NetworkSession

The title could be reformulated with just one 'NetworkSession'

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:145
>      RefPtr<NetworkingContext> context = RemoteNetworkingContext::create(loadParameters.sessionID, loadParameters.shouldClearReferrerOnHTTPSToHTTPRedirect);
>  
> +#if USE(NETWORK_SESSION)
> +    // PingLoad manages its own lifetime, deleting itself when its purpose has been fulfilled.
> +    new PingLoad(loadParameters);
> +#else
>      // PingHandle manages its own lifetime, deleting itself when its purpose has been fulfilled.
>      new PingHandle(context.get(), loadParameters.request, loadParameters.allowStoredCredentials == AllowStoredCredentials, PingHandle::UsesAsyncCallbacks::Yes);
> +#endif

RemoteNetworkingContext doesn't seems to be used in USE(NETWORK_SESSION) path. It should be moved inside the #else branch.

[Alex Christensen]

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

[Darin Adler]

Comment on attachment 272833 [details]
Patch

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

> Source/WebKit2/NetworkProcess/PingLoad.h:34
> +class PingLoad : public NetworkDataTaskClient {

Should mark the class final. Also, I think that it could be private inheritance rather than public.

> Source/WebKit2/NetworkProcess/PingLoad.h:36
> +    PingLoad(const NetworkResourceLoadParameters& parameters)

Typically we’d want to mark this explicit.

Also, in a class that deletes itself like this, I think it’s better to only expose a static member function to trigger this load and not return the pointer. That way the caller doesn’t have to write what looks like incorrect code.

> Source/WebKit2/NetworkProcess/PingLoad.h:37
> +        : m_timeoutTimer(*this, &PingLoad::timeoutTimerFired)

I new code, a bit more elegant to write this with a lambda:

    : m_timeoutTimer(*this, [this]() {
        delete this;
    })

> Source/WebKit2/NetworkProcess/PingLoad.h:43
> +        if (auto* networkSession = SessionTracker::networkSession(parameters.sessionID)) {
> +            m_task = NetworkDataTask::create(*networkSession, *this, parameters.request, parameters.allowStoredCredentials, parameters.contentSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect);
> +            m_task->resume();
> +        } else
> +            ASSERT_NOT_REACHED();

The if does us no good. If you really want an assertion then I’d just write:

    auto* networkSession = SessionTracker::networkSession(parameters.sessionID);
    ASSERT(networkSession);
    ...

But I’d suggest not bothering with the assertion.