Initial NSURLSession WebResourceLoader implementation
Created attachment 263551 [details] Patch
Created attachment 263553 [details] Patch
Created attachment 263588 [details] Patch
Created attachment 263589 [details] Patch
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.
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?
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?
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.
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).
(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());
Created attachment 263741 [details] Patch
Comment on attachment 263741 [details] Patch r=me
http://trac.webkit.org/changeset/191457