Bug 179251 - Make ResourceLoader::willSendRequestInternal asynchronous
Summary: Make ResourceLoader::willSendRequestInternal asynchronous
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-03 11:34 PDT by Alex Christensen
Modified: 2018-02-14 13:34 PST (History)
8 users (show)

See Also:


Attachments
Patch (38.11 KB, patch)
2017-11-03 11:36 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (39.11 KB, patch)
2017-11-03 12:45 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.03 MB, application/zip)
2017-11-03 13:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (948.94 KB, application/zip)
2017-11-03 13:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (2.02 MB, application/zip)
2017-11-03 13:52 PDT, Build Bot
no flags Details
Patch (40.45 KB, patch)
2017-11-03 13:54 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.07 MB, application/zip)
2017-11-03 15:10 PDT, Build Bot
no flags Details
Patch (42.66 KB, patch)
2017-11-03 19:00 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (42.68 KB, patch)
2017-11-04 00:12 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (42.71 KB, patch)
2017-11-06 16:48 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-11-03 11:34:29 PDT
Make ResourceLoader::willSendRequestInternal asynchronous
Comment 1 Alex Christensen 2017-11-03 11:36:53 PDT
Created attachment 325925 [details]
Patch
Comment 2 Alex Christensen 2017-11-03 12:45:28 PDT
Created attachment 325937 [details]
Patch
Comment 3 Build Bot 2017-11-03 13:25:41 PDT
Comment on attachment 325937 [details]
Patch

Attachment 325937 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5094128

Number of test failures exceeded the failure limit.
Comment 4 Build Bot 2017-11-03 13:25:42 PDT
Created attachment 325947 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 Build Bot 2017-11-03 13:39:44 PDT
Comment on attachment 325937 [details]
Patch

Attachment 325937 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5094253

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2017-11-03 13:39:46 PDT
Created attachment 325951 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-11-03 13:52:01 PDT
Comment on attachment 325937 [details]
Patch

Attachment 325937 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5094263

Number of test failures exceeded the failure limit.
Comment 8 Build Bot 2017-11-03 13:52:02 PDT
Created attachment 325955 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Alex Christensen 2017-11-03 13:54:13 PDT
Created attachment 325957 [details]
Patch
Comment 10 Build Bot 2017-11-03 15:10:51 PDT
Comment on attachment 325957 [details]
Patch

Attachment 325957 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5095568

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-preflight.any.html
Comment 11 Build Bot 2017-11-03 15:10:53 PDT
Created attachment 325969 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Alex Christensen 2017-11-03 19:00:02 PDT
Created attachment 326000 [details]
Patch
Comment 13 Alex Christensen 2017-11-04 00:12:44 PDT
Created attachment 326022 [details]
Patch
Comment 14 Andy Estes 2017-11-06 16:32:15 PST
Comment on attachment 326022 [details]
Patch

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

> Source/WebCore/loader/SubresourceLoader.cpp:243
> +    ResourceLoader::willSendRequestInternal(WTFMove(newRequest), redirectResponse, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler), redirectResponse = redirectResponse] (ResourceRequest&& request) mutable {

You can just type "redirectResponse" to capture by value. You don't need to type "redirectResponse = redirectResponse".

> Source/WebCore/loader/cache/CachedResource.cpp:297
> +    platformStrategies()->loaderStrategy()->loadResource(frame, *this, WTFMove(request), m_options, [this, frame = makeRef(frame), loggingAllowed = cachedResourceLoader.isAlwaysOnLoggingAllowed()] (RefPtr<SubresourceLoader>&& loader) {

Is capturing |this| safe here? Do you need a CachedResourceHandle?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:93
> +    SubresourceLoader::create(frame, resource, WTFMove(request), options, [this, completionHandler = WTFMove(completionHandler), resource = CachedResourceHandle<CachedResource>(&resource), frame = makeRef(frame)] (RefPtr<SubresourceLoader>&& loader) mutable {

Is it safe to capture |this| here without protecting it?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:104
> +    NetscapePlugInStreamLoader::create(frame, client, WTFMove(request), [this, completionHandler = WTFMove(completionHandler), frame = makeRef(frame)] (RefPtr<NetscapePlugInStreamLoader>&& loader) mutable {

Ditto.

> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp:93
> +    SubresourceLoader::create(frame, resource, WTFMove(request), options, [this, completionHandler = WTFMove(completionHandler)] (RefPtr<WebCore::SubresourceLoader>&& loader) mutable {

Ditto.

> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp:116
> +    NetscapePlugInStreamLoader::create(frame, client, WTFMove(request), [this, completionHandler = WTFMove(completionHandler)] (RefPtr<WebCore::NetscapePlugInStreamLoader>&& loader) mutable {

Ditto.
Comment 15 Alex Christensen 2017-11-06 16:47:45 PST
I added some protectedThis, but classes that inherit from LoaderStrategy are unfortunately process-global and never destroyed.  I didn't protect them.
Comment 16 Alex Christensen 2017-11-06 16:48:10 PST
Created attachment 326169 [details]
Patch
Comment 17 Alex Christensen 2017-11-06 16:56:02 PST
http://trac.webkit.org/r224522
Comment 18 Radar WebKit Bug Importer 2017-11-15 12:16:54 PST
<rdar://problem/35567192>