Bug 185002 - DocumentLoader::loadMainResource() should WTFMove() the passed ResourceRequest
Summary: DocumentLoader::loadMainResource() should WTFMove() the passed ResourceRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on: 185044
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-25 14:47 PDT by Daniel Bates
Modified: 2018-04-26 13:51 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.28 KB, patch)
2018-04-25 15:07 PDT, Daniel Bates
youennf: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (994.74 KB, application/zip)
2018-04-25 16:18 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (54.22 MB, application/zip)
2018-04-25 17:19 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (907.67 KB, application/zip)
2018-04-25 17:41 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (1.41 MB, application/zip)
2018-04-25 18:05 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (8.12 MB, application/zip)
2018-04-25 19:08 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-04-25 14:47:05 PDT
DocumentLoader::loadMainResource() should WTFMove() the passed ResourceRequest.
Comment 1 Daniel Bates 2018-04-25 15:07:40 PDT
Created attachment 338805 [details]
Patch
Comment 2 EWS Watchlist 2018-04-25 16:18:41 PDT
Comment on attachment 338805 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 3 EWS Watchlist 2018-04-25 16:18:42 PDT
Created attachment 338817 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 4 EWS Watchlist 2018-04-25 17:19:08 PDT
Comment on attachment 338805 [details]
Patch

Attachment 338805 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7461159

Number of test failures exceeded the failure limit.
Comment 5 EWS Watchlist 2018-04-25 17:19:10 PDT
Created attachment 338832 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 6 EWS Watchlist 2018-04-25 17:41:54 PDT
Comment on attachment 338805 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 7 EWS Watchlist 2018-04-25 17:41:56 PDT
Created attachment 338836 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 8 EWS Watchlist 2018-04-25 18:05:54 PDT
Comment on attachment 338805 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 9 EWS Watchlist 2018-04-25 18:05:56 PDT
Created attachment 338839 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 EWS Watchlist 2018-04-25 19:08:28 PDT
Comment on attachment 338805 [details]
Patch

Attachment 338805 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7462864

Number of test failures exceeded the failure limit.
Comment 11 EWS Watchlist 2018-04-25 19:08:36 PDT
Created attachment 338847 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 12 Alex Christensen 2018-04-26 09:58:12 PDT
Comment on attachment 338805 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:1795
> +    ResourceRequest updatedRequest;
>      if (mainResourceLoader())
> -        request = mainResourceLoader()->originalRequest();
> +        updatedRequest = mainResourceLoader()->originalRequest();

Isn't this a change in behavior if mainResourceLoader() is null?
Comment 13 Daniel Bates 2018-04-26 10:41:06 PDT
(In reply to Alex Christensen from comment #12)
> Comment on attachment 338805 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338805&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1795
> > +    ResourceRequest updatedRequest;
> >      if (mainResourceLoader())
> > -        request = mainResourceLoader()->originalRequest();
> > +        updatedRequest = mainResourceLoader()->originalRequest();
> 
> Isn't this a change in behavior if mainResourceLoader() is null?

Yes, it is a change in behavior hence the failures. This is unintentional. Will change this snippet to read:

-    ResourceRequest updatedRequest;
-    if (mainResourceLoader())
-        updatedRequest = mainResourceLoader()->originalRequest();
+    ResourceRequest updatedRequest = mainResourceLoader() ? mainResourceLoader()->originalRequest() : mainResourceRequest.resourceRequest();
Comment 14 Daniel Bates 2018-04-26 10:47:12 PDT
Committed r231052: <https://trac.webkit.org/changeset/231052>
Comment 15 Ryan Haddad 2018-04-26 13:09:22 PDT
This change caused http/tests/security/credentials-main-resource.html to fail on iOS and macOS:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r231061%20(4939)/results.html
Comment 16 WebKit Commit Bot 2018-04-26 13:34:06 PDT
Re-opened since this is blocked by bug 185044
Comment 17 Daniel Bates 2018-04-26 13:35:31 PDT
(In reply to Ryan Haddad from comment #15)
> This change caused http/tests/security/credentials-main-resource.html to
> fail on iOS and macOS:
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r231061%20(4939)/results.html

For historical preservation, here is the diff:

[[
--- /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/http/tests/security/credentials-main-resource-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/http/tests/security/credentials-main-resource-actual.txt
@@ -1,3 +1,4 @@
-ALERT: Authenticated as user: testuser password: testpass
+http://127.0.0.1:8000/security/resources/basic-auth.php?username=testuser&password=testpass - didReceiveAuthenticationChallenge - Simulating cancelled authentication sheet
+ALERT: Sent username:password of (:) which is not what was expected
 Main Resource Credentials: testuser, testpass
]]
Comment 18 Daniel Bates 2018-04-26 13:36:32 PDT
The failure implies that the test depends on DocumentLoader::loadMainResource() making  a copy of the passed ResourceRequest because the passed ResourceRequest is mutated.
Comment 19 Daniel Bates 2018-04-26 13:39:19 PDT
(In reply to Daniel Bates from comment #18)
> The failure implies that the test depends on
> DocumentLoader::loadMainResource() making  a copy of the passed
> ResourceRequest because the passed ResourceRequest is mutated.

Disregard this remark.
Comment 20 Daniel Bates 2018-04-26 13:43:45 PDT
Committed r231068: <https://trac.webkit.org/changeset/231068>
Comment 21 Daniel Bates 2018-04-26 13:51:00 PDT
Committed build fix in <https://trac.webkit.org/changeset/231071>.