(NetworkProcess) Sync XHRs should load using async ResourceHandles, not ResourceHandle::loadResourceSynchronously Currently when any WebProcess does a sync XHR, the NetworkProcess main thread is blocked. This will free it up.
Created attachment 208663 [details] Patch for EWS I'm getting a lot of "variety" in my layout test results locally, so while I'm trying to sort through that I'd like EWS to take a swipe at this. There's probably still unneeded cruft, no Changelog, etc etc. Just wanna see what the bots say
Attachment 208663 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/SynchronousLoaderClient.cpp', u'Source/WebCore/platform/network/SynchronousLoaderClient.h', u'Source/WebCore/platform/network/mac/SynchronousLoaderClient.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.h', u'Source/WebKit2/NetworkProcess/HostRecord.cpp', u'Source/WebKit2/NetworkProcess/HostRecord.h', u'Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/SchedulableLoader.cpp', u'Source/WebKit2/NetworkProcess/SchedulableLoader.h', u'Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/SynchronousLoadController.cpp', u'Source/WebKit2/NetworkProcess/SynchronousLoadController.h', u'Source/WebKit2/NetworkProcess/SynchronousLoadController.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp']" exit_code: 1 Source/WebKit2/NetworkProcess/SynchronousLoadController.h:26: #ifndef header guard has wrong style, please use: SynchronousLoadController_h [build/header_guard] [5] Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:299: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit2/NetworkProcess/SynchronousLoadController.cpp:60: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit2/NetworkProcess/SynchronousLoadController.cpp:82: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 208687 [details] Patch v1
Comment on attachment 208687 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=208687&action=review > Source/WebCore/ChangeLog:8 > + No new tests (Covered by plenty of existing tests). How did you test what happens with authentication? We have bug 8342 tracking a request to handle it, but I'm not sure how it should work conceptually, and using an async handle would send auth callbacks. Although using SynchronousLoaderClient should do the right wrong thing I guess. > Source/WebCore/ChangeLog:13 > + * platform/network/SynchronousLoaderClient.cpp: I'm now wondering if this class should be named SynchronousHandleClient. > Source/WebCore/platform/network/SynchronousLoaderClient.h:50 > + static ResourceError platformBadSyncXHRResponseError(); "Synchronous"? But more importantly, the new name is even less clear than the old one. What is so special about sync XHR responses that they deserve their own kind of error? This conflates network response and the mechanism we use to get it. > Source/WebKit2/ChangeLog:14 > + The concept is the NetworkResourceLoader is the single loader class that does asynchronous loads, but the existence of a > + SynchronousLoadController holds off on constantly messaging back to the WebProcess and instead accumulates the result of the load. This seems reasonable to me, and I'd like this even more if the class had a descriptive name. SynchronousLoadIPCThrottle? SynchronousLoadResponseAccumulator? > Source/WebKit2/ChangeLog:95 > + * WebProcess/WebCoreSupport/WebPlatformStrategies.cpp: > + (WebKit::WebPlatformStrategies::loadResourceSynchronously): Short circuit blocked loads without consulting the NetworkProcess. Why is this necessary? If it's easy to do, I'd prefer this check to be in NetworkProcess: - it's closer to networking knowledge this way; - that would a light defense against a rogue WebContent process, not allowing it to ever send requests to blocked ports (talking about the bright future where WebContent has no network access); - there is no performance impact either way, because good guys don't do this. > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:-105 > - HashMap<ResourceLoadIdentifier, RefPtr<SyncNetworkResourceLoader>>::iterator syncEnd = m_syncNetworkResourceLoaders.end(); > - for (HashMap<ResourceLoadIdentifier, RefPtr<SyncNetworkResourceLoader>>::iterator i = m_syncNetworkResourceLoaders.begin(); i != syncEnd; ++i) > - i->value->abort(); How did you test what happens if WebContent process terminates while a sync request is outstanding? This could be a reasonably common user scenario. > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h:53 > - void scheduleLoader(PassRefPtr<SchedulableLoader>); > + void scheduleLoader(PassRefPtr<NetworkResourceLoader>); Unrelated to this patch, but it's not right to use PassRefPtr here - all callers just pass a raw pointer, and API contract is that the caller still has the pointer in order to be able to call removeLoader(). So, there is no transfer of ownership in theory or practice. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:137 > + if (m_synchronousLoadController) > + m_synchronousLoadController.clear(); Why is this necessary? We have so much logic that depends on whether the load is sync or async that it seems prudent to retain the knowledge until destruction. Also, there should be no null check before clear(). > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:189 > + // Only notify the WebProcess of the response for asynchronous loads. The comment seems unnecessary. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:222 > + if (m_synchronousLoadController) { We sometimes check isSynchronous(), and other times m_synchronousLoadController. Is there a rule? > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:249 > + m_synchronousLoadController->didFinishLoading(); > + else > + send(Messages::WebResourceLoader::DidFinishResourceLoad(finishTime)); Alternatively, we could have IPC adapters for both sync and async cases, and get rid of if() blocks everywhere. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:276 > + m_synchronousLoadController->willSendRequest(handle->firstRequest(), newRequest); Hmm, what if we have multiple redirects? Sounds like this should be current, not first. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:304 > + if (m_synchronousLoadController) > + m_synchronousLoadController->cancel(); > + else > + didFail(m_handle.get(), cancelledError(m_request)); Each instance makes me wish for an "AsynchronousLoadClient" more (not with that name, obviously). > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:119 > + virtual bool isSynchronous() { return m_synchronousLoadController; } Why is this virtual? > Source/WebKit2/NetworkProcess/SynchronousLoadController.cpp:36 > +#include <wtf/text/CString.h> Where is it needed? I can't see it, or think of a good use for CString in this class. > Source/WebKit2/NetworkProcess/SynchronousLoadController.h:43 > + > + Extra line. > Source/WebKit2/NetworkProcess/SynchronousLoadController.h:72 > + OwnPtr<WTF::Vector<uint8_t>> m_responseData; Please no WTF:: > Source/WebKit2/NetworkProcess/SynchronousLoadController.mm:38 > +void SynchronousLoadController::platformSynthesizeErrorResponse() I can't think of a reason why we are doing this for sync responses, but not for async ones. Probably just not thinking hard enough.
(In reply to comment #4) > (From update of attachment 208687 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208687&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests (Covered by plenty of existing tests). > > How did you test what happens with authentication? > We have bug 8342 tracking a request to handle it, but I'm not sure how it should work conceptually, and using an async handle would send auth callbacks. Although using SynchronousLoaderClient should do the right wrong thing I guess. As you guessed, authentication with sync XHRs is busted today, and still busted after this patch. The difference is that after this patch, it can be made to work! > > Source/WebCore/ChangeLog:13 > > + * platform/network/SynchronousLoaderClient.cpp: > > I'm now wondering if this class should be named SynchronousHandleClient. Probably! Renaming that existing code should be a separate patch. > > > Source/WebCore/platform/network/SynchronousLoaderClient.h:50 > > + static ResourceError platformBadSyncXHRResponseError(); > > "Synchronous"? Yup. > But more importantly, the new name is even less clear than the old one. What is so special about sync XHR responses that they deserve their own kind of error? This conflates network response and the mechanism we use to get it. I think the new name is more specific/precise. That doesn't make it more clear. I'm not sure what would make it more clear. > > Source/WebKit2/ChangeLog:14 > > + The concept is the NetworkResourceLoader is the single loader class that does asynchronous loads, but the existence of a > > + SynchronousLoadController holds off on constantly messaging back to the WebProcess and instead accumulates the result of the load. > > This seems reasonable to me, and I'd like this even more if the class had a descriptive name. SynchronousLoadIPCThrottle? SynchronousLoadResponseAccumulator? I kind of like that second one. I'll play with it. > > Source/WebKit2/ChangeLog:95 > > + * WebProcess/WebCoreSupport/WebPlatformStrategies.cpp: > > + (WebKit::WebPlatformStrategies::loadResourceSynchronously): Short circuit blocked loads without consulting the NetworkProcess. > > Why is this necessary? Turns out that at least one of the existing Sync XHR layout tests relied on it! > If it's easy to do, I'd prefer this check to be in NetworkProcess: > - it's closer to networking knowledge this way; > - that would a light defense against a rogue WebContent process, not allowing it to ever send requests to blocked ports (talking about the bright future where WebContent has no network access); > - there is no performance impact either way, because good guys don't do this. I can make that change. > > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:-105 > > - HashMap<ResourceLoadIdentifier, RefPtr<SyncNetworkResourceLoader>>::iterator syncEnd = m_syncNetworkResourceLoaders.end(); > > - for (HashMap<ResourceLoadIdentifier, RefPtr<SyncNetworkResourceLoader>>::iterator i = m_syncNetworkResourceLoaders.begin(); i != syncEnd; ++i) > > - i->value->abort(); > > How did you test what happens if WebContent process terminates while a sync request is outstanding? This could be a reasonably common user scenario. Sync requests are now no different from async requests in this regard. Our automated tests don't cover this type of scenario. I can test it manually as I follow up on all of this feedback. > > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h:53 > > - void scheduleLoader(PassRefPtr<SchedulableLoader>); > > + void scheduleLoader(PassRefPtr<NetworkResourceLoader>); > > Unrelated to this patch, but it's not right to use PassRefPtr here - all callers just pass a raw pointer, and API contract is that the caller still has the pointer in order to be able to call removeLoader(). So, there is no transfer of ownership in theory or practice. Agreed. > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:137 > > + if (m_synchronousLoadController) > > + m_synchronousLoadController.clear(); > > Why is this necessary? We have so much logic that depends on whether the load is sync or async that it seems prudent to retain the knowledge until destruction. It is not necessary. I agree with this. > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:222 > > + if (m_synchronousLoadController) { > > We sometimes check isSynchronous(), and other times m_synchronousLoadController. Is there a rule? No. The cruft is leftover from combining Scheduleable and NetworkResource loaders. I cleaned it up a little bit but didn't follow through in the end. Will do. > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:249 > > + m_synchronousLoadController->didFinishLoading(); > > + else > > + send(Messages::WebResourceLoader::DidFinishResourceLoad(finishTime)); > > Alternatively, we could have IPC adapters for both sync and async cases, and get rid of if() blocks everywhere. I considered that, but decided against pursuing it because the "do IPC or not" decision is not the only behavior difference. Maybe some of the NetworkResourceLoader logic could fit into the async adapter, and that approach would be worth pursuing. I'll take a closer look. > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:276 > > + m_synchronousLoadController->willSendRequest(handle->firstRequest(), newRequest); > > Hmm, what if we have multiple redirects? Sounds like this should be current, not first. I agree with you completely. I was attempting to maintain current behavior (i.e. WebCore::SynchronousLoaderClient). Like authentication, this is one of those "It would now be really easy to make this work correctly" cases that probably deserves a followup bug. > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:304 > > + if (m_synchronousLoadController) > > + m_synchronousLoadController->cancel(); > > + else > > + didFail(m_handle.get(), cancelledError(m_request)); > > Each instance makes me wish for an "AsynchronousLoadClient" more (not with that name, obviously). I think you're right. > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:119 > > + virtual bool isSynchronous() { return m_synchronousLoadController; } > > Why is this virtual? Because I suck. > > Source/WebKit2/NetworkProcess/SynchronousLoadController.cpp:36 > > +#include <wtf/text/CString.h> > > Where is it needed? I can't see it, or think of a good use for CString in this class. From debugging printf's while testing the patch. > > > Source/WebKit2/NetworkProcess/SynchronousLoadController.h:43 > > + > > + > > Extra line. > > > Source/WebKit2/NetworkProcess/SynchronousLoadController.h:72 > > + OwnPtr<WTF::Vector<uint8_t>> m_responseData; > > Please no WTF:: Okay. > > Source/WebKit2/NetworkProcess/SynchronousLoadController.mm:38 > > +void SynchronousLoadController::platformSynthesizeErrorResponse() > > I can't think of a reason why we are doing this for sync responses, but not for async ones. Probably just not thinking hard enough. I only surmised this through code archeology, and don't know if it's actually a valid concern: It seems the "loadResourceSynchronously" code in the WebProcess demands that there be a response, even if the load actually failed before the response came in. This may or may not still be important today, but I was hesitant to change it since it's actually exposed to JS.
Will probably have to skip two tests in WK2 as their results will now be different depending on whether or not the network process is enabled. This is covered in https://bugs.webkit.org/show_bug.cgi?id=119857
Created attachment 208837 [details] Patch v2
Attachment 208837 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/wk2/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/SynchronousLoaderClient.cpp', u'Source/WebCore/platform/network/SynchronousLoaderClient.h', u'Source/WebCore/platform/network/mac/SynchronousLoaderClient.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.cpp', u'Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.h', u'Source/WebKit2/NetworkProcess/HostRecord.cpp', u'Source/WebKit2/NetworkProcess/HostRecord.h', u'Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h', u'Source/WebKit2/NetworkProcess/NetworkLoaderClient.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/SchedulableLoader.cpp', u'Source/WebKit2/NetworkProcess/SchedulableLoader.h', u'Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/SyncNetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp', u'Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.h', u'Source/WebKit2/NetworkProcess/mac/SynchronousNetworkLoaderClient.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1 Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit2/NetworkProcess/NetworkLoaderClient.h:49: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/NetworkLoaderClient.h:55: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.h:59: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.h:44: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.h:50: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Style snafu's fixed locally
Comment on attachment 208837 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=208837&action=review Looks great to me. A few things I don't quite understand yet, and would like to talk through in person: - simple-cross-origin-denied-events tests; - what platformBadSynchronousXHRResponseError means; - is there anything suspicious with 6+1 handling. > LayoutTests/platform/wk2/TestExpectations:277 > +# https://webkit.org/b/119857 > +http/tests/xmlhttprequest/simple-cross-origin-denied-events-post-sync.html > +http/tests/xmlhttprequest/simple-cross-origin-denied-events-sync.html I think that it's better to put bug URL on the same line with test path. Not quite sure, but some tools might use that. > Source/WebKit2/ChangeLog:114 > +2013-08-13 Brady Eidson <beidson@apple.com> Duplicate ChangeLog. > Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.cpp:49 > + // This message is DispatchMessageEvenWhenWaitingForSyncReply to avoid a situation where the NetworkProcess is deadlocked waiting for 6 connections > + // waiting for 6 connections to complete while the WebProcess is waiting for a 7th (Synchronous XHR) to complete. "waiting for 6 connection" twice. > Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.cpp:53 > +void AsynchronousNetworkLoaderClient::canAuthenticateAgainstProtectionSpace(NetworkResourceLoader* loader, const WebCore::ProtectionSpace& protectionSpace) Is there a reason to not be using namespace WebCore in this file? > Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.cpp:99 > + > + > + > + Lots of extra blank lines. > Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.h:50 > + virtual void willSendRequest(NetworkResourceLoader*, WebCore::ResourceRequest& request, const WebCore::ResourceResponse& redirectResponse) OVERRIDE; > + virtual void canAuthenticateAgainstProtectionSpace(NetworkResourceLoader*, const WebCore::ProtectionSpace&) OVERRIDE; > + virtual void didReceiveResponse(NetworkResourceLoader*, const WebCore::ResourceResponse&) OVERRIDE; > + virtual void didReceiveBuffer(NetworkResourceLoader*, WebCore::SharedBuffer*, int encodedDataLength) OVERRIDE; > + virtual void didSendData(NetworkResourceLoader*, unsigned long long bytesSent, unsigned long long totalBytesToBeSent) OVERRIDE; > + virtual void didFinishLoading(NetworkResourceLoader*, double finishTime) OVERRIDE; > + virtual void didFail(NetworkResourceLoader*, const WebCore::ResourceError& error) OVERRIDE; These should all be private. Other code should never know about this derived class and call its methods (create() returns a pointer to NetworkLoaderClient). > Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.h:61 > + virtual ~SynchronousNetworkLoaderClient() OVERRIDE; > + > + virtual void willSendRequest(NetworkResourceLoader*, WebCore::ResourceRequest& proposedRequest, const WebCore::ResourceResponse& redirectResponse) OVERRIDE; > + virtual void canAuthenticateAgainstProtectionSpace(NetworkResourceLoader*, const WebCore::ProtectionSpace&) OVERRIDE; > + virtual void didReceiveResponse(NetworkResourceLoader*, const WebCore::ResourceResponse&) OVERRIDE; > + virtual void didReceiveBuffer(NetworkResourceLoader*, WebCore::SharedBuffer*, int encodedDataLength) OVERRIDE; > + virtual void didSendData(NetworkResourceLoader*, unsigned long long bytesSent, unsigned long long totalBytesToBeSent) OVERRIDE { } > + virtual void didFinishLoading(NetworkResourceLoader*, double finishTime) OVERRIDE; > + virtual void didFail(NetworkResourceLoader*, const WebCore::ResourceError& error) OVERRIDE; > + > + virtual bool isSynchronous() OVERRIDE { return true; } These should all be private too.
Comment on attachment 208837 [details] Patch v2 r=me with changes made during in person discussion, plus not skipping the tests, and with changes suggested in prior review, and assuming all tests pass. > - simple-cross-origin-denied-events tests; This was an actual bug, and the fix appears to be to delete obsolete code that synthesizes an HTTP response for 401 authentication failures (and also synthesizes more response types just for good measure that turned bad). It wasn't actually that we got to CORS checks due to different ordering, but that NetworkProcess was sending a bad response/error pair that confused WebProcess. > - what platformBadSynchronousXHRResponseError means; Brady will just rename it back, we don't feel like thinking about this too much now. > - is there anything suspicious with 6+1 handling. Brady says that it works the same as before.
(In reply to comment #11) > (From update of attachment 208837 [details]) > > - is there anything suspicious with 6+1 handling. > > Brady says that it works the same as before. To clarify further on the record, the 6+1 handling logic is mostly in HostRecord and the Scheduler, and is unchanged by this patch.
http://trac.webkit.org/changeset/154183
<rdar://problem/14916066>