Bug 150355 – Initial NSURLSession WebResourceLoader implementation

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

[Alex Christensen]

Initial NSURLSession WebResourceLoader implementation

[Alex Christensen]

Created attachment 263551 [details]
Patch

[Alex Christensen]

Created attachment 263553 [details]
Patch

[Alex Christensen]

Created attachment 263588 [details]
Patch

[Alex Christensen]

Created attachment 263589 [details]
Patch

[Alex Christensen]

1560 tests ran as expected, 347 didn't:
Unexpected flakiness: text-only failures (212)
Unexpected flakiness: image-only failures (8)
Unexpected flakiness: timeouts (79)
Regressions: Unexpected text-only failures (2)
Regressions: Unexpected crashes (46)

So, a pretty good start.

[Alexey Proskuryakov]

Comment on attachment 263589 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:290
> +    

Blank line here.

> Source/WebKit2/NetworkProcess/NetworkingSession.h:74
> +    virtual ~NetworkingDataTaskClient() { };

The semicolon is not needed here.

> Source/WebKit2/NetworkProcess/cocoa/NetworkingSessionCocoa.mm:135
> +            WebCore::ResourceResponse resourceResponse(response);

Please add ASSERT(isMainThread()) in all functions that touch ResourceRequest or ResourceResponse. These classes are not safe to use from secondary threads.

> Source/WebKit2/NetworkProcess/cocoa/NetworkingSessionCocoa.mm:150
> +        // FIXME: Look into why this being on the main thread causes fewer crashes, but more crashes with the other callbacks.

:-/

> Source/WebKit2/NetworkProcess/cocoa/NetworkingSessionCocoa.mm:190
> +    m_session = [NSURLSession sessionWithConfiguration:configuration delegate:static_cast<id>(m_sessionDelegate.get()) delegateQueue:m_sessionQueue.get()];

Hmm, don't we want to have delegates called on main thread? Why is there a queue at all?

[Antti Koivisto]

Comment on attachment 263589 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:123
> +#if !USE(NSURLSESSION)
>      const WebCore::ResourceResponse& response() const { return m_response; }
> +#endif

Why #if out m_response? It is going to be needed eventually and should be readily available already.

> Source/WebKit2/NetworkProcess/NetworkingSession.h:64
> +class NetworkingSession;

Why "Networking" instead of "Network" like other network process classes?

> Source/WebKit2/NetworkProcess/NetworkingSession.h:77
> +class NetworkingDataTask : public RefCounted<NetworkingDataTask> {

Maybe use "NetworkSession" as namespace for these related types, NetworkSessionDataTask or just NetworkSessionTask? (what kind of other tasks are there going to be?)

similarly NetworkingDataTaskClient -> NetworkSessionTaskClient?

> Source/WebKit2/NetworkProcess/NetworkingSession.h:111
> +    ~NetworkingSession() { LockHolder locker(m_dataTaskMapLock); ASSERT(m_dataTaskMap.isEmpty()); }

This locker doesn't do anything useful in non-debug builds and shouldn't be necessary in debug builds either.

> Source/WebKit2/NetworkProcess/cocoa/NetworkingSessionCocoa.mm:93
> +    if (auto networkingTask = _session->dataTaskForIdentifier(task.taskIdentifier)) {

Please use auto* for pointer types: auto* networkingTask

> Source/WebKit2/NetworkProcess/cocoa/NetworkingSessionCocoa.mm:94
> +        if (auto client = networkingTask->client()) {

Please use auto* for pointer types: auto* client

etc everywhere in this patch.

>> Source/WebKit2/NetworkProcess/cocoa/NetworkingSessionCocoa.mm:150
>> +        // FIXME: Look into why this being on the main thread causes fewer crashes, but more crashes with the other callbacks.
> 
> :-/

The threading story seems bit shaky. Can we just make callbacks in the main thread for the first iteration?

[Alex Christensen]

Comment on attachment 263589 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:123
>> +#endif
> 
> Why #if out m_response? It is going to be needed eventually and should be readily available already.

Aha!  I was copying it on a non-main thread, and I was seeing AtomicString assertion failures.  I need to copy on the main thread, then I won't need to get rid of m_response.

[Antti Koivisto]

Comment on attachment 263589 [details]
Patch

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

> Source/WebKit2/config.h:76
> +#ifndef USE_NSURLSESSION

This should probably be something like USE_NETWORK_SESSION. The layer covered by this is not platform specific (though the implementation is).

[Alex Christensen]

(In reply to comment #6)
> > Source/WebKit2/NetworkProcess/cocoa/NetworkingSessionCocoa.mm:190
> > +    m_session = [NSURLSession sessionWithConfiguration:configuration delegate:static_cast<id>(m_sessionDelegate.get()) delegateQueue:m_sessionQueue.get()];
> 
> Hmm, don't we want to have delegates called on main thread? Why is there a
> queue at all?
My next patch will use [NSOperationQueue mainQueue] to put all the callbacks on the main thread.  And I replace the lock with ASSERT(isMainThread());

[Alex Christensen]

Created attachment 263741 [details]
Patch

[Antti Koivisto]

Comment on attachment 263741 [details]
Patch

r=me

[Alex Christensen]

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