Bug 195623 - Link prefetch not useful for top-level navigation
Summary: Link prefetch not useful for top-level navigation
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Other
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-12 09:03 PDT by Rob Buis
Modified: 2019-04-18 12:42 PDT (History)
18 users (show)

See Also:


Attachments
Patch (19.27 KB, patch)
2019-03-12 09:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.48 MB, application/zip)
2019-03-12 10:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.28 MB, application/zip)
2019-03-12 11:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews206 for win-future (12.98 MB, application/zip)
2019-03-12 11:46 PDT, Build Bot
no flags Details
Patch (15.12 KB, patch)
2019-03-14 14:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.67 MB, application/zip)
2019-03-14 15:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.74 MB, application/zip)
2019-03-14 15:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (2.40 MB, application/zip)
2019-03-14 16:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews201 for win-future (12.89 MB, application/zip)
2019-03-14 17:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (3.38 MB, application/zip)
2019-03-14 19:49 PDT, Build Bot
no flags Details
Patch (26.42 KB, patch)
2019-03-18 08:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.45 MB, application/zip)
2019-03-18 10:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.70 MB, application/zip)
2019-03-18 10:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.30 MB, application/zip)
2019-03-18 10:56 PDT, Build Bot
no flags Details
Patch (25.75 KB, patch)
2019-03-18 11:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.56 MB, application/zip)
2019-03-18 12:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.44 MB, application/zip)
2019-03-18 14:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews204 for win-future (12.87 MB, application/zip)
2019-03-18 18:48 PDT, Build Bot
no flags Details
Patch (23.86 KB, patch)
2019-03-19 09:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.67 MB, application/zip)
2019-03-19 10:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews201 for win-future (12.86 MB, application/zip)
2019-03-19 11:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.26 MB, application/zip)
2019-03-19 11:54 PDT, Build Bot
no flags Details
Patch (24.67 KB, patch)
2019-03-19 13:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.12 MB, application/zip)
2019-03-19 15:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.95 MB, application/zip)
2019-03-19 15:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.25 MB, application/zip)
2019-03-19 16:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (12.98 MB, application/zip)
2019-03-19 16:55 PDT, Build Bot
no flags Details
Patch (25.38 KB, patch)
2019-03-20 04:05 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.85 MB, application/zip)
2019-03-20 05:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.86 MB, application/zip)
2019-03-20 06:19 PDT, Build Bot
no flags Details
Patch (25.84 KB, patch)
2019-03-20 14:30 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.91 MB, application/zip)
2019-03-20 18:38 PDT, Build Bot
no flags Details
Patch (28.44 KB, patch)
2019-03-21 08:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (27.84 KB, patch)
2019-03-21 10:06 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.87 MB, application/zip)
2019-03-21 12:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.72 MB, application/zip)
2019-03-21 17:13 PDT, Build Bot
no flags Details
Patch (27.64 KB, patch)
2019-03-22 02:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.78 MB, application/zip)
2019-03-22 03:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (12.93 MB, application/zip)
2019-03-22 04:05 PDT, Build Bot
no flags Details
Patch (28.88 KB, patch)
2019-03-25 09:44 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.88 MB, application/zip)
2019-03-25 12:15 PDT, Build Bot
no flags Details
Patch (28.88 KB, patch)
2019-03-25 12:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.88 KB, patch)
2019-03-26 12:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.84 KB, patch)
2019-03-26 14:42 PDT, Rob Buis
rbuis: review?
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.63 MB, application/zip)
2019-03-27 00:01 PDT, Build Bot
no flags Details
Patch (31.20 KB, patch)
2019-03-27 07:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (21.14 KB, patch)
2019-03-29 04:56 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (32.48 KB, patch)
2019-03-29 08:37 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.46 MB, application/zip)
2019-03-29 09:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews204 for win-future (12.87 MB, application/zip)
2019-03-29 10:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.27 MB, application/zip)
2019-03-29 10:35 PDT, Build Bot
no flags Details
Patch (35.11 KB, patch)
2019-04-02 01:44 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.46 MB, application/zip)
2019-04-02 02:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.25 MB, application/zip)
2019-04-02 03:35 PDT, Build Bot
no flags Details
Patch (35.28 KB, patch)
2019-04-02 05:11 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (35.80 KB, patch)
2019-04-03 01:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (33.35 KB, patch)
2019-04-03 01:59 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (33.41 KB, patch)
2019-04-03 07:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (33.06 KB, patch)
2019-04-13 05:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (32.96 KB, patch)
2019-04-13 08:17 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (33.05 KB, patch)
2019-04-14 12:55 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2019-04-16 06:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2019-03-12 09:03:04 PDT
Due to cache partitioning this will be a cache miss and the fetch will be for nothing.
Comment 1 Rob Buis 2019-03-12 09:32:05 PDT
Created attachment 364392 [details]
Patch
Comment 2 Build Bot 2019-03-12 10:38:50 PDT
Comment on attachment 364392 [details]
Patch

Attachment 364392 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11476671

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 3 Build Bot 2019-03-12 10:38:52 PDT
Created attachment 364402 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 4 Build Bot 2019-03-12 11:20:42 PDT
Comment on attachment 364392 [details]
Patch

Attachment 364392 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11476877

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 5 Build Bot 2019-03-12 11:20:44 PDT
Created attachment 364415 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 6 Build Bot 2019-03-12 11:46:06 PDT
Comment on attachment 364392 [details]
Patch

Attachment 364392 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11477510

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 7 Build Bot 2019-03-12 11:46:18 PDT
Created attachment 364419 [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 8 Rob Buis 2019-03-14 14:31:34 PDT
Created attachment 364687 [details]
Patch
Comment 9 Build Bot 2019-03-14 15:27:59 PDT
Comment on attachment 364687 [details]
Patch

Attachment 364687 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11508255

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 10 Build Bot 2019-03-14 15:28:01 PDT
Created attachment 364704 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 11 Build Bot 2019-03-14 15:43:22 PDT
Comment on attachment 364687 [details]
Patch

Attachment 364687 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11508376

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 12 Build Bot 2019-03-14 15:43:24 PDT
Created attachment 364708 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 13 Build Bot 2019-03-14 16:21:48 PDT
Comment on attachment 364687 [details]
Patch

Attachment 364687 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11508667

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 14 Build Bot 2019-03-14 16:21:50 PDT
Created attachment 364712 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 15 Build Bot 2019-03-14 17:20:34 PDT
Comment on attachment 364687 [details]
Patch

Attachment 364687 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11509560

New failing tests:
http/tests/cache/memory-cache-pruning.html
http/tests/cache/link-prefetch-main-resource.html
Comment 16 Build Bot 2019-03-14 17:20:48 PDT
Created attachment 364725 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 17 Build Bot 2019-03-14 19:49:29 PDT
Comment on attachment 364687 [details]
Patch

Attachment 364687 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11511672

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 18 Build Bot 2019-03-14 19:49:31 PDT
Created attachment 364761 [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.6
Comment 19 Rob Buis 2019-03-18 08:57:06 PDT
Created attachment 365017 [details]
Patch
Comment 20 Build Bot 2019-03-18 10:06:58 PDT
Comment on attachment 365017 [details]
Patch

Attachment 365017 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11551328

New failing tests:
http/tests/cache/display-image-unset-allows-cached-image-load.html
http/tests/cache/link-prefetch-main-resource.php
Comment 21 Build Bot 2019-03-18 10:07:00 PDT
Created attachment 365023 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 22 Build Bot 2019-03-18 10:17:22 PDT
Comment on attachment 365017 [details]
Patch

Attachment 365017 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11551341

New failing tests:
http/tests/cache/display-image-unset-allows-cached-image-load.html
Comment 23 Build Bot 2019-03-18 10:17:24 PDT
Created attachment 365024 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 24 Build Bot 2019-03-18 10:56:44 PDT
Comment on attachment 365017 [details]
Patch

Attachment 365017 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11551491

New failing tests:
http/tests/cache/display-image-unset-allows-cached-image-load.html
http/tests/cache/link-prefetch-main-resource.php
Comment 25 Build Bot 2019-03-18 10:56:46 PDT
Created attachment 365030 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 26 Rob Buis 2019-03-18 11:45:33 PDT
Created attachment 365040 [details]
Patch
Comment 27 Build Bot 2019-03-18 12:59:20 PDT
Comment on attachment 365040 [details]
Patch

Attachment 365040 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11553059

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 28 Build Bot 2019-03-18 12:59:22 PDT
Created attachment 365045 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 29 Build Bot 2019-03-18 14:07:22 PDT
Comment on attachment 365040 [details]
Patch

Attachment 365040 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11553295

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 30 Build Bot 2019-03-18 14:07:24 PDT
Created attachment 365058 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 31 Build Bot 2019-03-18 18:48:18 PDT
Comment on attachment 365040 [details]
Patch

Attachment 365040 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11556810

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 32 Build Bot 2019-03-18 18:48:30 PDT
Created attachment 365101 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 33 Rob Buis 2019-03-19 09:40:15 PDT
Created attachment 365173 [details]
Patch
Comment 34 Build Bot 2019-03-19 10:42:04 PDT
Comment on attachment 365173 [details]
Patch

Attachment 365173 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11567417

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 35 Build Bot 2019-03-19 10:42:06 PDT
Created attachment 365183 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 36 Build Bot 2019-03-19 11:51:25 PDT
Comment on attachment 365173 [details]
Patch

Attachment 365173 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11568089

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 37 Build Bot 2019-03-19 11:51:37 PDT
Created attachment 365194 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 38 Build Bot 2019-03-19 11:54:54 PDT
Comment on attachment 365173 [details]
Patch

Attachment 365173 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11567939

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 39 Build Bot 2019-03-19 11:54:56 PDT
Created attachment 365198 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 40 Rob Buis 2019-03-19 13:49:54 PDT
Created attachment 365223 [details]
Patch
Comment 41 Build Bot 2019-03-19 15:02:18 PDT
Comment on attachment 365223 [details]
Patch

Attachment 365223 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11570649

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 42 Build Bot 2019-03-19 15:02:21 PDT
Created attachment 365235 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 43 Build Bot 2019-03-19 15:46:32 PDT
Comment on attachment 365223 [details]
Patch

Attachment 365223 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11571097

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
Comment 44 Build Bot 2019-03-19 15:46:34 PDT
Created attachment 365248 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 45 Build Bot 2019-03-19 16:21:14 PDT
Comment on attachment 365223 [details]
Patch

Attachment 365223 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11571187

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 46 Build Bot 2019-03-19 16:21:17 PDT
Created attachment 365253 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 47 Build Bot 2019-03-19 16:55:36 PDT
Comment on attachment 365223 [details]
Patch

Attachment 365223 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11571929

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 48 Build Bot 2019-03-19 16:55:48 PDT
Created attachment 365265 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 49 Rob Buis 2019-03-20 04:05:02 PDT
Created attachment 365334 [details]
Patch
Comment 50 Build Bot 2019-03-20 05:47:09 PDT
Comment on attachment 365334 [details]
Patch

Attachment 365334 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11579141

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
http/tests/cache/memory-cache-pruning.html
Comment 51 Build Bot 2019-03-20 05:47:21 PDT
Created attachment 365340 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 52 Build Bot 2019-03-20 06:19:51 PDT
Comment on attachment 365334 [details]
Patch

Attachment 365334 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11579379

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 53 Build Bot 2019-03-20 06:19:54 PDT
Created attachment 365342 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 54 Rob Buis 2019-03-20 14:30:44 PDT
Created attachment 365401 [details]
Patch
Comment 55 Build Bot 2019-03-20 18:38:07 PDT
Comment on attachment 365401 [details]
Patch

Attachment 365401 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11588645

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 56 Build Bot 2019-03-20 18:38:20 PDT
Created attachment 365460 [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 57 youenn fablet 2019-03-20 18:41:53 PDT
Comment on attachment 365401 [details]
Patch

This goes in the right direction.
Some suggestions below.
We probably want to solve properly the case of being prefetched resources that match navigation loads.


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

> Source/WebCore/loader/LinkLoader.cpp:279
>      ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();

I think we want the load to not include any credential.
Also, we might want to NOT follow redirections and skip service workers.
Tests might be useful there.

> Source/WebCore/loader/LinkLoader.cpp:281
> +    options.certificateInfoPolicy = CertificateInfoPolicy::IncludeCertificateInfo;

Is it needed because this might fail some cache checks in NetworkProcess?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:808
> +    if (frame() && frame()->page() && m_documentLoader && type != CachedResource::Type::LinkPrefetch) {

Why should we skip content extension rules for prefetch loads?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:215
> +        if (auto entry = PrefetchCache::singleton().take(request.url(), sessionID())) {

The prefetchCache should probably be owned by NetworkProcess instead of being a singleton.
NetworkResourceLoader can access it through its connection

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:217
> +            m_cache->store(request, entry->response(), WTFMove(buffer), nullptr);

Maybe we should have entry->releaseBuffer() instead of entry->buffer() and then WTFMove.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:767
> +    auto request = originalRequest();

Why do we need this copy?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:768
> +    if (request.httpHeaderField(HTTPHeaderName::Purpose) == "prefetch" && (!m_parameters.sourceOrigin || !m_parameters.sourceOrigin->canRequest(request.url()))) {

I think there should always be a m_parameters.sourceOrigin for prefetch resources.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:770
> +        return;

This sounds good as a first approach.
If the navigation load kicks in before the prefetch is finished, the prefetch will not be used.
Can we improve this?

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:36
> +    : m_response(response), m_buffer(WTFMove(buffer))

We could take a ResourceResponse&&

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:47
> +    : m_expirationTimer(*this, &PrefetchCache::expiration)

We could have a better name for expiration.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:96
> +        ASSERT(resources->contains(requestUrl));

Is this guaranteed if we take the requestUrl in between?

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:74
> +    typedef Vector<std::tuple<PAL::SessionID, URL, WallTime>> SessionPrefetchExpirationList;

Could be a Deque?
Comment 58 Rob Buis 2019-03-21 08:40:47 PDT
Created attachment 365546 [details]
Patch
Comment 59 Rob Buis 2019-03-21 10:06:24 PDT
Created attachment 365562 [details]
Patch
Comment 60 Rob Buis 2019-03-21 12:13:18 PDT
Comment on attachment 365401 [details]
Patch

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

>> Source/WebCore/loader/LinkLoader.cpp:279
>>      ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();
> 
> I think we want the load to not include any credential.
> Also, we might want to NOT follow redirections and skip service workers.
> Tests might be useful there.

Ah, I remember now the discussion on the github issue. Should be fixed.

>> Source/WebCore/loader/LinkLoader.cpp:281
>> +    options.certificateInfoPolicy = CertificateInfoPolicy::IncludeCertificateInfo;
> 
> Is it needed because this might fail some cache checks in NetworkProcess?

Correct, this hits "retrieveCacheEntry: Resource does not have required certificate" in NetworkResourceLoader.cpp.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:808
>> +    if (frame() && frame()->page() && m_documentLoader && type != CachedResource::Type::LinkPrefetch) {
> 
> Why should we skip content extension rules for prefetch loads?

We hit an ASSERT in toResourceType in Debug, even without my patch, on prefetches. My thinking was that content extension would be done on the actual navigate anyway.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:215
>> +        if (auto entry = PrefetchCache::singleton().take(request.url(), sessionID())) {
> 
> The prefetchCache should probably be owned by NetworkProcess instead of being a singleton.
> NetworkResourceLoader can access it through its connection

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:217
>> +            m_cache->store(request, entry->response(), WTFMove(buffer), nullptr);
> 
> Maybe we should have entry->releaseBuffer() instead of entry->buffer() and then WTFMove.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:767
>> +    auto request = originalRequest();
> 
> Why do we need this copy?

It is was not my intention to make a copy, sorry! Fixed.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:768
>> +    if (request.httpHeaderField(HTTPHeaderName::Purpose) == "prefetch" && (!m_parameters.sourceOrigin || !m_parameters.sourceOrigin->canRequest(request.url()))) {
> 
> I think there should always be a m_parameters.sourceOrigin for prefetch resources.

Ok I removed the check.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:770
>> +        return;
> 
> This sounds good as a first approach.
> If the navigation load kicks in before the prefetch is finished, the prefetch will not be used.
> Can we improve this?

It may be possible if we track prefetches as soon as possible, i.e when the request is sent. Then when the navigation load is finishing up, there could be a check to see if any prefetch is pending and it could be cancelled. I will try tomorrow, but not too sure about actually testing this.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:36
>> +    : m_response(response), m_buffer(WTFMove(buffer))
> 
> We could take a ResourceResponse&&

Done.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:47
>> +    : m_expirationTimer(*this, &PrefetchCache::expiration)
> 
> We could have a better name for expiration.

I am still thinking :) I'll probably have a better name tomorrow.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:96
>> +        ASSERT(resources->contains(requestUrl));
> 
> Is this guaranteed if we take the requestUrl in between?

I think it is okay because of how PrefetchCache::take works. But I am fine with making these if checks, to be on the safe side.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:74
>> +    typedef Vector<std::tuple<PAL::SessionID, URL, WallTime>> SessionPrefetchExpirationList;
> 
> Could be a Deque?

Done.
Comment 61 Build Bot 2019-03-21 12:36:04 PDT
Comment on attachment 365562 [details]
Patch

Attachment 365562 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11602069

New failing tests:
http/tests/cache/link-prefetch-main-resource.php
Comment 62 Build Bot 2019-03-21 12:36:16 PDT
Created attachment 365591 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 63 Build Bot 2019-03-21 17:13:11 PDT
Comment on attachment 365562 [details]
Patch

Attachment 365562 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11606201

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 64 Build Bot 2019-03-21 17:13:13 PDT
Created attachment 365649 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 65 Yoav Weiss 2019-03-21 21:47:33 PDT
Comment on attachment 365562 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.cpp:284
> +    options.mode = FetchOptions::Mode::Cors;

The current spec PR[1] doesn't automagically set CORS mode, credentials mode and referrer policy, but verifies that they are properly set (e.g. via crossorigin and referrerpolicy attributes)

[1] https://whatpr.org/html/4115/e32a6f8...e4193e2/semantics.html#process-the-linked-resource

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:55
> +static const Seconds expirationTimeout { 5_s };

5 seconds expiration seems rather low
Comment 66 Yoav Weiss 2019-03-21 21:51:04 PDT
(In reply to Yoav Weiss from comment #65)
> Comment on attachment 365562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365562&action=review
> 
> > Source/WebCore/loader/LinkLoader.cpp:284
> > +    options.mode = FetchOptions::Mode::Cors;
> 
> The current spec PR[1] doesn't automagically set CORS mode, credentials mode
> and referrer policy, but verifies that they are properly set (e.g. via
> crossorigin and referrerpolicy attributes)

Regarding Omit mode, that's not what the spec PR currently says, so I wonder if this is required (it would render some same-origin prefetch use-cases rather useless, e.g. prefetching the next step in a shopping cart funnel).

If it is required, we would need to add a `crossorigin` "omit" value, so that it can be set with an attribute.
  
> 
> [1]
> https://whatpr.org/html/4115/e32a6f8...e4193e2/semantics.html#process-the-
> linked-resource
> 
> > Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:55
> > +static const Seconds expirationTimeout { 5_s };
> 
> 5 seconds expiration seems rather low
Comment 67 Rob Buis 2019-03-22 02:07:38 PDT
Created attachment 365697 [details]
Patch
Comment 68 Rob Buis 2019-03-22 02:22:52 PDT
Comment on attachment 365562 [details]
Patch

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

>>> Source/WebCore/loader/LinkLoader.cpp:284
>>> +    options.mode = FetchOptions::Mode::Cors;
>> 
>> The current spec PR[1] doesn't automagically set CORS mode, credentials mode and referrer policy, but verifies that they are properly set (e.g. via crossorigin and referrerpolicy attributes)
>> 
>> [1] https://whatpr.org/html/4115/e32a6f8...e4193e2/semantics.html#process-the-linked-resource
> 
> Regarding Omit mode, that's not what the spec PR currently says, so I wonder if this is required (it would render some same-origin prefetch use-cases rather useless, e.g. prefetching the next step in a shopping cart funnel).
> 
> If it is required, we would need to add a `crossorigin` "omit" value, so that it can be set with an attribute.

So in general here we have to set up decent default values that work for prefetches. Not sure if adding a `crossorigin` should be part of this ppatch.

>>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:55
>>> +static const Seconds expirationTimeout { 5_s };
>> 
>> 5 seconds expiration seems rather low
> 
> 

It is certainly up for discussion :) I had in the back of my mind, I would like to write a test for the timeout, but with a large timeout, it becomes a not so attractive test. But apart from that practical problem, of course we can increase it.
Comment 69 Build Bot 2019-03-22 03:27:15 PDT
Comment on attachment 365697 [details]
Patch

Attachment 365697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11611210

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
Comment 70 Build Bot 2019-03-22 03:27:18 PDT
Created attachment 365700 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 71 Build Bot 2019-03-22 04:05:29 PDT
Comment on attachment 365697 [details]
Patch

Attachment 365697 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11611525

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 72 Build Bot 2019-03-22 04:05:41 PDT
Created attachment 365705 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 73 Yoav Weiss 2019-03-22 05:52:43 PDT
(In reply to Rob Buis from comment #68)
> Comment on attachment 365562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365562&action=review
> 
> >>> Source/WebCore/loader/LinkLoader.cpp:284
> >>> +    options.mode = FetchOptions::Mode::Cors;
> >> 
> >> The current spec PR[1] doesn't automagically set CORS mode, credentials mode and referrer policy, but verifies that they are properly set (e.g. via crossorigin and referrerpolicy attributes)
> >> 
> >> [1] https://whatpr.org/html/4115/e32a6f8...e4193e2/semantics.html#process-the-linked-resource
> > 
> > Regarding Omit mode, that's not what the spec PR currently says, so I wonder if this is required (it would render some same-origin prefetch use-cases rather useless, e.g. prefetching the next step in a shopping cart funnel).
> > 
> > If it is required, we would need to add a `crossorigin` "omit" value, so that it can be set with an attribute.
> 
> So in general here we have to set up decent default values that work for
> prefetches. Not sure if adding a `crossorigin` should be part of this ppatch.

Note that for preload, we have avoided setting "magical" values (e.g. for "font" destinations), and left that as extra work (but more predictable) to the developer. I'm open to discuss that decision though. Mind commenting on the PR[1], saying that you'd prefer setting the values, rather than checking for them.

Youenn - thoughts on the above would be appreciated as well

[1] https://github.com/whatwg/html/pull/4115

> 
> >>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:55
> >>> +static const Seconds expirationTimeout { 5_s };
> >> 
> >> 5 seconds expiration seems rather low
> > 
> > 
> 
> It is certainly up for discussion :) I had in the back of my mind, I would
> like to write a test for the timeout, but with a large timeout, it becomes a
> not so attractive test. But apart from that practical problem, of course we
> can increase it.
Comment 74 Ryosuke Niwa 2019-03-22 19:57:03 PDT
Comment on attachment 365697 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:35
> +PrefetchEntry::PrefetchEntry(WebCore::ResourceResponse&& response, RefPtr<WebCore::SharedBuffer>&& buffer)

We need to clear this cache upon receiving memory pressure.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:87
> +        std::tie(sessionID, requestUrl, timestamp) = m_sessionExpirationList.first();

Use .random()
Comment 75 Ryosuke Niwa 2019-03-22 19:58:53 PDT
It looks like it's unsound to prefetch cross-site because the content can be read with Spectre class of attacks. We'd have to solve that problem before we can ever enable this feature.
Comment 76 youenn fablet 2019-03-24 07:01:47 PDT
> We need to clear this cache upon receiving memory pressure.

Good point.

(In reply to Ryosuke Niwa from comment #75)
> It looks like it's unsound to prefetch cross-site because the content can be
> read with Spectre class of attacks. We'd have to solve that problem before
> we can ever enable this feature.

The prefetch cache is in NetworkProcess.
The plan is to only use it for now for navigations so only the right process should get the response.
Should we extend prefetch to other loads (I dislike this though), we should apply the regular sanitization including CORS and CORP as done for other resources coming from network or from network cache.
Comment 77 youenn fablet 2019-03-25 03:48:51 PDT
We might also want to limit prefetches for now, maybe with something like:
- third-party iframes cannot trigger prefetches
- Number of prefetches of a given page is limited (to 1 maybe?)
I would add a FIXME in LinkLoader::prefetchIfNeeded about this.

SameOrigin for credentials is probably ok.
CORS mode does not make a lot of sense in that context.
We actually do not want to trigger CORS or CORP checks on the response since the target is navigation loads. 
FetchOptions::Mode::Navigate makes some sense but has some implications with regards to our implementation. Probably that is the one making most sense currently since we will treat it as if it is a top level navigation.
Comment 78 youenn fablet 2019-03-25 09:06:05 PDT
I wonder how/whether we want want load/error events.
Should load event be fired when load starts or when load finishes? Can the timing of the load event leak some information if done at load finish?
If load fails, should error event be sent?

If we want to support load/error events when load finishes, we might need to provide an opaque response to the web process. This might be best handled by a new mode, so that NetworkLoadChecker does the necessary sanitization. And we do not want the body to be transmitted in any case.

One safe way might be to fire the load event whenever the load starts and the error event if the load is not even started.

Ideally, we would then directly return didReceiveResponse/didFinishLoading.
But we will then need to handle the case of loads that last longer than what the WebProcess thinks they do.
Comment 79 Rob Buis 2019-03-25 09:44:57 PDT
Created attachment 365869 [details]
Patch
Comment 80 Rob Buis 2019-03-25 09:46:40 PDT
(In reply to youenn fablet from comment #76)
> > We need to clear this cache upon receiving memory pressure.
> 
> Good point.

Agreed, I added code for this in the latest patch. Note that I am not sure how to test it though.
Comment 81 Rob Buis 2019-03-25 09:48:21 PDT
(In reply to youenn fablet from comment #77)
> We might also want to limit prefetches for now, maybe with something like:
> - third-party iframes cannot trigger prefetches
> - Number of prefetches of a given page is limited (to 1 maybe?)
> I would add a FIXME in LinkLoader::prefetchIfNeeded about this.

I added the FIXME.

> SameOrigin for credentials is probably ok.

Using SameOrigin now.

> CORS mode does not make a lot of sense in that context.
> We actually do not want to trigger CORS or CORP checks on the response since
> the target is navigation loads. 
> FetchOptions::Mode::Navigate makes some sense but has some implications with
> regards to our implementation. Probably that is the one making most sense
> currently since we will treat it as if it is a top level navigation.

Using FetchOptions::Mode::Navigate now.
Comment 82 youenn fablet 2019-03-25 09:49:45 PDT
(In reply to Rob Buis from comment #80)
> (In reply to youenn fablet from comment #76)
> > > We need to clear this cache upon receiving memory pressure.
> > 
> > Good point.
> 
> Agreed, I added code for this in the latest patch. Note that I am not sure
> how to test it though.

We might need a testRunner API to trigger memory pressure in Network Process.
This might be done in a follow-up patch though, w/o the memory pressure cleanup.
Comment 83 Build Bot 2019-03-25 12:15:09 PDT
Comment on attachment 365869 [details]
Patch

Attachment 365869 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11661967

New failing tests:
http/tests/cache/link-prefetch-main-resource.html
Comment 84 Build Bot 2019-03-25 12:15:22 PDT
Created attachment 365880 [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 85 Rob Buis 2019-03-25 12:33:06 PDT
Created attachment 365881 [details]
Patch
Comment 86 Rob Buis 2019-03-26 12:38:20 PDT
Created attachment 365987 [details]
Patch
Comment 87 Rob Buis 2019-03-26 14:42:42 PDT
Created attachment 366000 [details]
Patch
Comment 88 youenn fablet 2019-03-26 15:13:18 PDT
Comment on attachment 366000 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:213
> +    if (m_parameters.options.mode == FetchOptions::Mode::Navigate) {

We probably want to restrict to top level navigation for now.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:768
> +    }

We should probably add a check on didReceiveResponse/didReceiveBuffer to not send data if it is a prefetch request.

> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:1
> +<!-- webkit-test-runner [ experimental:LinkPrefetch=false ] -->

Should be true?

> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:10
> +    internals.settings.setStorageBlockingPolicy('BlockThirdParty')

Why is it needed?

> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:40
> +<link rel="prefetch" href="http://localhost:8000/cache/resources/prefetched-main-resource.php" onload="setTimeout(loadAfterPrefetch, 0);">

We might want to contribute these tests to WPT maybe.

> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:4
> +    header("Access-Control-Allow-Origin: http://127.0.0.1:8000");

Maybe we should have echo "parent.window.postMessage('PASS', '*');";

> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:12
> +echo "parent.window.postMessage('nonprefetch', '*');";

Maybe we should have echo "parent.window.postMessage('FAIL', '*');";
Comment 89 Build Bot 2019-03-27 00:01:55 PDT
Comment on attachment 366000 [details]
Patch

Attachment 366000 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11680402

New failing tests:
fast/visual-viewport/ios/min-scale-greater-than-one.html
Comment 90 Build Bot 2019-03-27 00:01:57 PDT
Created attachment 366057 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 91 Yoav Weiss 2019-03-27 01:40:20 PDT
(In reply to youenn fablet from comment #78)
> I wonder how/whether we want want load/error events.
> Should load event be fired when load starts or when load finishes? Can the
> timing of the load event leak some information if done at load finish?
> If load fails, should error event be sent?
> 
> If we want to support load/error events when load finishes, we might need to
> provide an opaque response to the web process. This might be best handled by
> a new mode, so that NetworkLoadChecker does the necessary sanitization. And
> we do not want the body to be transmitted in any case.
> 
> One safe way might be to fire the load event whenever the load starts and
> the error event if the load is not even started.

Firing at load start would diverge from typical `load` event semantics. The opaque response approach seems best to me.

> 
> Ideally, we would then directly return didReceiveResponse/didFinishLoading.
> But we will then need to handle the case of loads that last longer than what
> the WebProcess thinks they do.
Comment 92 Rob Buis 2019-03-27 07:39:06 PDT
Created attachment 366069 [details]
Patch
Comment 93 Rob Buis 2019-03-27 09:26:40 PDT
Comment on attachment 366000 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:213
>> +    if (m_parameters.options.mode == FetchOptions::Mode::Navigate) {
> 
> We probably want to restrict to top level navigation for now.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:768
>> +    }
> 
> We should probably add a check on didReceiveResponse/didReceiveBuffer to not send data if it is a prefetch request.

Done.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:1
>> +<!-- webkit-test-runner [ experimental:LinkPrefetch=false ] -->
> 
> Should be true?

I think it has no effect, probably since link prefetch is an experimental flag and will get enabled on test runs by default, so I removed the line.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:10
>> +    internals.settings.setStorageBlockingPolicy('BlockThirdParty')
> 
> Why is it needed?

I think this was needed at some point but it does not have an effect anymore, I removed it.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:40
>> +<link rel="prefetch" href="http://localhost:8000/cache/resources/prefetched-main-resource.php" onload="setTimeout(loadAfterPrefetch, 0);">
> 
> We might want to contribute these tests to WPT maybe.

I hope they are good enough :)

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:4
>> +    header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> 
> Maybe we should have echo "parent.window.postMessage('PASS', '*');";

Done.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:12
>> +echo "parent.window.postMessage('nonprefetch', '*');";
> 
> Maybe we should have echo "parent.window.postMessage('FAIL', '*');";

Done.
Comment 94 youenn fablet 2019-03-28 22:34:33 PDT
Comment on attachment 366069 [details]
Patch

This seems almost ready.
Some comments and suggestions.

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

> Source/WebCore/loader/LinkLoader.cpp:279
> +    // FIXME further prefetch limitations:

s/FIXME further/FIXME: Add further prefetch restrictions/

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:470
> +        send(Messages::WebResourceLoader::DidReceiveResponse { response, willWaitForContinueDidReceiveResponse });

So, the idea would be to keep sending the prefetch to the memory cache if same origin.
That makes some sense for optimizations.
The fetch PR for defining is not really aligned but we could reconcile both progressively.

My question is what happens for cross origin prefetch in the web process?
I guess they do not go in the memory cache.

Do we have cases of loading where we go directly from send request to didFinishLoading without going through didReceiveResponse?
It seems we could do:
if (isCrossOriginPrefetch(originalRequest()) {
    send(Messages::WebResourceLoader::DidReceiveResponse { ResourceResponse { }, false });
    return completionHandler(PolicyAction::Use);
}

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:67
> +static const Seconds expirationTimeout { 5_s };

We might want to update this in the future, 5 seconds might not be long enough.
I wonder whether it should be something like 5_s after the page doing the prefetch navigated away.
This might add some complexity to the implementation though.
Not for this patch.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:89
> +    return *map;

We use m_sessionPrefetches.ensure nowadays.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:59
> +    void clear();

We might need to clear part of the prefetch cache in NetworkProcess::destroySession.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:61
> +    std::unique_ptr<PrefetchEntry> take(URL, PAL::SessionID);

const URL&.
I believe we usually pass SessionID parameter first.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:62
> +    void store(URL, WebCore::ResourceResponse&&, RefPtr<WebCore::SharedBuffer>&&, PAL::SessionID);

URL&&

> LayoutTests/platform/mac-wk1/TestExpectations:699
> +webkit.org/b/195623 http/tests/cache/link-prefetch-main-resource.html [ Failure ]

Let's skip it as well since we miss the implementation.

> LayoutTests/platform/win/TestExpectations:4308
> +webkit.org/b/195623 http/tests/cache/link-prefetch-main-resource.html [ Timeout ]

Let's skip it then
Comment 95 youenn fablet 2019-03-28 22:36:39 PDT
Btw, the test is doing an iframe load.
Should we do a top level navigation prefetch load instead?
Comment 96 youenn fablet 2019-03-28 22:52:19 PDT
Rethinking about it, we do not want cross origin prefetch to end in the memory cache with an empty response. Maybe that was your original idea?
I would anyway make it clear that compeltionHandler is called with use in any prefetch
Comment 97 Rob Buis 2019-03-29 04:56:34 PDT
Created attachment 366266 [details]
Patch
Comment 98 Build Bot 2019-03-29 04:58:29 PDT
Attachment 366266 [details] did not pass style-queue:


ERROR: LayoutTests/platform/win/TestExpectations:4306:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/mac-wk1/TestExpectations:699:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/win/TestExpectations:4306:  Path does not exist.  [test/expectations] [5]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 99 Rob Buis 2019-03-29 08:37:23 PDT
Created attachment 366273 [details]
Patch
Comment 100 Build Bot 2019-03-29 09:45:45 PDT
Comment on attachment 366273 [details]
Patch

Attachment 366273 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11703243

New failing tests:
http/tests/cache/link-prefetch-main-resource-iframe.html
Comment 101 Build Bot 2019-03-29 09:45:47 PDT
Created attachment 366278 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 102 Rob Buis 2019-03-29 09:49:13 PDT
Comment on attachment 366069 [details]
Patch

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

>> Source/WebCore/loader/LinkLoader.cpp:279
>> +    // FIXME further prefetch limitations:
> 
> s/FIXME further/FIXME: Add further prefetch restrictions/

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:470
>> +        send(Messages::WebResourceLoader::DidReceiveResponse { response, willWaitForContinueDidReceiveResponse });
> 
> So, the idea would be to keep sending the prefetch to the memory cache if same origin.
> That makes some sense for optimizations.
> The fetch PR for defining is not really aligned but we could reconcile both progressively.
> 
> My question is what happens for cross origin prefetch in the web process?
> I guess they do not go in the memory cache.
> 
> Do we have cases of loading where we go directly from send request to didFinishLoading without going through didReceiveResponse?
> It seems we could do:
> if (isCrossOriginPrefetch(originalRequest()) {
>     send(Messages::WebResourceLoader::DidReceiveResponse { ResourceResponse { }, false });
>     return completionHandler(PolicyAction::Use);
> }

I don't think we have cases of didFinishLoading without going through didReceiveResponse.

Sending an empty ResourceResponse actually hits an ASSERT on WebCore side.

I don't think it is too important if the prefetch ends up in the memory cache as it will be a cache miss anyway (partitioning). But I'll think on this further.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:67
>> +static const Seconds expirationTimeout { 5_s };
> 
> We might want to update this in the future, 5 seconds might not be long enough.
> I wonder whether it should be something like 5_s after the page doing the prefetch navigated away.
> This might add some complexity to the implementation though.
> Not for this patch.

Ok.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:89
>> +    return *map;
> 
> We use m_sessionPrefetches.ensure nowadays.

Oops, forgot about this, will try to fix it soon.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:59
>> +    void clear();
> 
> We might need to clear part of the prefetch cache in NetworkProcess::destroySession.

When would that be useful?

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:61
>> +    std::unique_ptr<PrefetchEntry> take(URL, PAL::SessionID);
> 
> const URL&.
> I believe we usually pass SessionID parameter first.

Done.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:62
>> +    void store(URL, WebCore::ResourceResponse&&, RefPtr<WebCore::SharedBuffer>&&, PAL::SessionID);
> 
> URL&&

I think that makes things more complex so I left it off for now.

>> LayoutTests/platform/mac-wk1/TestExpectations:699
>> +webkit.org/b/195623 http/tests/cache/link-prefetch-main-resource.html [ Failure ]
> 
> Let's skip it as well since we miss the implementation.

Done.

>> LayoutTests/platform/win/TestExpectations:4308
>> +webkit.org/b/195623 http/tests/cache/link-prefetch-main-resource.html [ Timeout ]
> 
> Let's skip it then

Done.
Comment 103 Rob Buis 2019-03-29 09:50:37 PDT
(In reply to youenn fablet from comment #95)
> Btw, the test is doing an iframe load.
> Should we do a top level navigation prefetch load instead?

Yes that sounds useful. I added such a test and renamed the iframe test to make it more clear it is using an iframe.
Comment 104 Rob Buis 2019-03-29 09:54:54 PDT
(In reply to youenn fablet from comment #96)
> Rethinking about it, we do not want cross origin prefetch to end in the
> memory cache with an empty response. Maybe that was your original idea?
> I would anyway make it clear that compeltionHandler is called with use in
> any prefetch

My first patches were concentrated on the memory cache which turned out to be the wrong approach. Since then I have not been too concerned with it because of the cache miss. I assume at this point making cached result empty is kind of efficient, compared to storing the full result. Do we even really need an entry for prefetches in the memory cache? Again I need to think about it, but let me know if you get convinced of the best way to go here.
Comment 105 Build Bot 2019-03-29 10:25:39 PDT
Comment on attachment 366273 [details]
Patch

Attachment 366273 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11703360

New failing tests:
http/tests/cache/link-prefetch-main-resource-iframe.html
Comment 106 Build Bot 2019-03-29 10:25:51 PDT
Created attachment 366280 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 107 Build Bot 2019-03-29 10:35:20 PDT
Comment on attachment 366273 [details]
Patch

Attachment 366273 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11703301

New failing tests:
http/tests/cache/link-prefetch-main-resource-iframe.html
Comment 108 Build Bot 2019-03-29 10:35:24 PDT
Created attachment 366281 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 109 Rob Buis 2019-04-02 01:44:48 PDT
Created attachment 366476 [details]
Patch
Comment 110 Build Bot 2019-04-02 02:53:10 PDT
Comment on attachment 366476 [details]
Patch

Attachment 366476 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11737128

New failing tests:
http/tests/cache/link-prefetch-main-resource-iframe.html
Comment 111 Build Bot 2019-04-02 02:53:12 PDT
Created attachment 366479 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 112 Build Bot 2019-04-02 03:34:59 PDT
Comment on attachment 366476 [details]
Patch

Attachment 366476 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11737203

New failing tests:
http/tests/cache/link-prefetch-main-resource-iframe.html
Comment 113 Build Bot 2019-04-02 03:35:02 PDT
Created attachment 366481 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 114 Rob Buis 2019-04-02 05:11:20 PDT
Created attachment 366488 [details]
Patch
Comment 115 youenn fablet 2019-04-02 15:50:50 PDT
> My first patches were concentrated on the memory cache which turned out to
> be the wrong approach. Since then I have not been too concerned with it
> because of the cache miss. I assume at this point making cached result empty
> is kind of efficient, compared to storing the full result. Do we even really
> need an entry for prefetches in the memory cache? Again I need to think
> about it, but let me know if you get convinced of the best way to go here.

We certainly do not want cross-origin prefetches to go in the memory cache.
We could be ok with same-origin prefetches to go in the memory cache but I feel like it might not be worth the added complexity.
Comment 116 youenn fablet 2019-04-02 16:03:56 PDT
Comment on attachment 366488 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +               http/tests/cache/link-prefetch-main-resource.html

Double line.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:213
> +    if (isMainResource() && m_parameters.options.mode == FetchOptions::Mode::Navigate) {

Let's restrict it to isMainFrameLoad().
I would think iframes do not need to opt out of partitioning and preload could be used instead for these.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:467
> +        send(Messages::WebResourceLoader::DidReceiveResponse { response, false });

I think we should do ResourceResponse { } instead of passing response.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:770
> +    const auto& request = originalRequest();

request is only used once, maybe we can remove it.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:64
> +    return resources->take(url);

Do we need the call to resources->contains().
If not there, I would believe take will return nullptr.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:92
> +void PrefetchCache::expiration()

How about clearExpiredEntries?

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:36
> +#include <wtf/text/WTFString.h>

CompletionHandler.h/Seconds.h do not seem needed.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:50
> +    mutable RefPtr<WebCore::SharedBuffer> m_buffer;

We can probably simplify this by removing mutable and making releaseBuffer() not const.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:54
> +    WTF_MAKE_NONCOPYABLE(PrefetchCache); WTF_MAKE_FAST_ALLOCATED;

Add a \n
Comment 117 Rob Buis 2019-04-03 01:39:21 PDT
Created attachment 366585 [details]
Patch
Comment 118 Rob Buis 2019-04-03 01:59:15 PDT
Created attachment 366586 [details]
Patch
Comment 119 Rob Buis 2019-04-03 07:38:02 PDT
Created attachment 366602 [details]
Patch
Comment 120 Rob Buis 2019-04-03 07:39:34 PDT
Comment on attachment 366488 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +               http/tests/cache/link-prefetch-main-resource.html
> 
> Double line.

Fixed.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:213
>> +    if (isMainResource() && m_parameters.options.mode == FetchOptions::Mode::Navigate) {
> 
> Let's restrict it to isMainFrameLoad().
> I would think iframes do not need to opt out of partitioning and preload could be used instead for these.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:467
>> +        send(Messages::WebResourceLoader::DidReceiveResponse { response, false });
> 
> I think we should do ResourceResponse { } instead of passing response.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:770
>> +    const auto& request = originalRequest();
> 
> request is only used once, maybe we can remove it.

Done.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:64
>> +    return resources->take(url);
> 
> Do we need the call to resources->contains().
> If not there, I would believe take will return nullptr.

Right, not needed.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:92
>> +void PrefetchCache::expiration()
> 
> How about clearExpiredEntries?

Sorry, I forgot about this issue! I like the name, done.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:36
>> +#include <wtf/text/WTFString.h>
> 
> CompletionHandler.h/Seconds.h do not seem needed.

Done.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:50
>> +    mutable RefPtr<WebCore::SharedBuffer> m_buffer;
> 
> We can probably simplify this by removing mutable and making releaseBuffer() not const.

Done.

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:54
>> +    WTF_MAKE_NONCOPYABLE(PrefetchCache); WTF_MAKE_FAST_ALLOCATED;
> 
> Add a \n

Done.
Comment 121 Rob Buis 2019-04-03 07:42:50 PDT
(In reply to youenn fablet from comment #115)
> > My first patches were concentrated on the memory cache which turned out to
> > be the wrong approach. Since then I have not been too concerned with it
> > because of the cache miss. I assume at this point making cached result empty
> > is kind of efficient, compared to storing the full result. Do we even really
> > need an entry for prefetches in the memory cache? Again I need to think
> > about it, but let me know if you get convinced of the best way to go here.
> 
> We certainly do not want cross-origin prefetches to go in the memory cache.
> We could be ok with same-origin prefetches to go in the memory cache but I
> feel like it might not be worth the added complexity.

Okay, thanks for clarifying. In the latest patch I now use CachingPolicy::DisallowCaching to not use the memory cache for prefetches at all.
Comment 122 youenn fablet 2019-04-12 08:34:00 PDT
Comment on attachment 366602 [details]
Patch

r=me

LGTM.
Some other things we might need:
- Web Inspector integration?
- Handling of still loading prefetched responses

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

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:34
> +#include <wtf/text/WTFString.h>

probably not needed.
Comment 123 Darin Adler 2019-04-12 08:59:35 PDT
Comment on attachment 366602 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadInfo.h:47
> +    Prefetch = 0x0400,
>  };
>  const uint16_t ResourceTypeMask = 0x03FF;

Seems dangerous that there’s a new type that’s not in ResourceTypeMask. Shouldn’t this have been changed to 0x7FF?
Comment 124 Darin Adler 2019-04-12 09:03:12 PDT
Comment on attachment 366602 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:46
> +    clear();

This seems unnecessary. I don’t see any work done in clear() that has any value if the objects are being destroyed immediately afterward. Seems like this will just make the destructor slower with no benefit.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:86
> +    auto& map = m_sessionPrefetches.add(sessionID, nullptr).iterator->value;
> +    if (!map)
> +        map = std::make_unique<PrefetchEntriesMap>();
> +    return *map;

Could use HashMap::ensure instead of add to avoid the if statement.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:49
> +class PrefetchEntry {
> +    WTF_MAKE_FAST_ALLOCATED;
> +public:
> +    PrefetchEntry(WebCore::ResourceResponse&&, RefPtr<WebCore::SharedBuffer>&&);
> +
> +    const WebCore::ResourceResponse& response() const { return m_response; }
> +    RefPtr<WebCore::SharedBuffer> releaseBuffer() { return m_buffer.releaseNonNull(); }
> +
> +private:
> +    WebCore::ResourceResponse m_response;
> +    RefPtr<WebCore::SharedBuffer> m_buffer;
> +};

Why a class instead of a struct for this? I think it would be better with a struct. I also think this should be a member instead of a separate struct. PrefetchCache::Entry.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:66
> +    typedef HashMap<URL, std::unique_ptr<PrefetchEntry>> PrefetchEntriesMap;

New code should us "using" instead of typedef.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:70
> +    typedef HashMap<PAL::SessionID, std::unique_ptr<PrefetchEntriesMap>> SessionPrefetchResourceMap;

Ditto.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:73
> +    typedef Deque<std::tuple<PAL::SessionID, URL, WallTime>> SessionPrefetchExpirationList;

Ditto.
Comment 125 Rob Buis 2019-04-13 05:07:39 PDT
Created attachment 367385 [details]
Patch
Comment 126 Darin Adler 2019-04-13 07:44:08 PDT
Comment on attachment 367385 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:55
> +    struct Entry {
> +        Entry(WebCore::ResourceResponse&&, RefPtr<WebCore::SharedBuffer>&&);
> +
> +        const WebCore::ResourceResponse& response() const { return m_response; }
> +        RefPtr<WebCore::SharedBuffer> releaseBuffer() { return m_buffer.releaseNonNull(); }
> +
> +        WebCore::ResourceResponse m_response;
> +        RefPtr<WebCore::SharedBuffer> m_buffer;
> +    };

More about what I meant when I suggested this be a struct: This struct can simply have "response" and "buffer" public data members. No need for the "m_" prefix for public struct data members. Also likely don’t need to define a constructor since aggregate construction should work. Could keep the releaseBuffer function if you like, or callers can just write buffer.releaseNonNull() directly. So ideally the struct just has two lines in its body, keeping it simpler.
Comment 127 Darin Adler 2019-04-13 07:44:38 PDT
Thanks for taking all my suggestions, by the way!
Comment 128 Rob Buis 2019-04-13 08:17:29 PDT
Created attachment 367388 [details]
Patch
Comment 129 Rob Buis 2019-04-14 12:55:18 PDT
Created attachment 367404 [details]
Patch
Comment 130 WebKit Commit Bot 2019-04-14 13:52:52 PDT
Comment on attachment 367404 [details]
Patch

Clearing flags on attachment: 367404

Committed r244248: <https://trac.webkit.org/changeset/244248>
Comment 131 WebKit Commit Bot 2019-04-14 13:52:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 132 Radar WebKit Bug Importer 2019-04-14 13:54:08 PDT
<rdar://problem/49887960>
Comment 133 Ryan Haddad 2019-04-15 08:54:01 PDT
The two tests added with this change are consistently failing an assertion on macOS and iOS Debug WK2 bots:

http/tests/cache/link-prefetch-main-resource-iframe.html
http/tests/cache/link-prefetch-main-resource.html

ASSERTION FAILED: !response.isNull()
./loader/SubresourceLoader.cpp(315) : virtual void WebCore::SubresourceLoader::didReceiveResponse(const WebCore::ResourceResponse &, CompletionHandler<void ()> &&)
1   0x591628019 WTFCrash
2   0x57f1f112b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x581fac83f WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse const&, WTF::CompletionHandler<void ()>&&)
4   0x57913a41d WebKit::WebResourceLoader::didReceiveResponse(WebCore::ResourceResponse const&, bool)
5   0x5796d3a28 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>, 0ul, 1ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>)
6   0x5796d3950 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WebCore::ResourceResponse, bool>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool))
7   0x5796d2eee void IPC::handleMessage<Messages::WebResourceLoader::DidReceiveResponse, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool))
8   0x5796d27f0 WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&)
9   0x57912c466 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
10  0x57806272c IPC::Connection::dispatchMessage(IPC::Decoder&)
11  0x578054851 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
12  0x5780634f7 IPC::Connection::dispatchOneIncomingMessage()
13  0x578084668 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
14  0x578084579 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
15  0x59165249d WTF::Function<void ()>::operator()() const
16  0x5916b4713 WTF::RunLoop::performWork()
17  0x5916b50a4 WTF::RunLoop::performWork(void*)
18  0x7fff3656cd81 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
19  0x7fff3662465c __CFRunLoopDoSource0
20  0x7fff3654fd30 __CFRunLoopDoSources0
21  0x7fff3654f1ad __CFRunLoopRun
22  0x7fff3654ea07 CFRunLoopRunSpecific
23  0x7fff3582cd96 RunCurrentEventLoopInMode
24  0x7fff3582cb06 ReceiveNextEventCommon
25  0x7fff3582c884 _BlockUntilNextEventMatchingListInModeWithFilter
26  0x7fff33adfa73 _DPSNextEvent
27  0x7fff34275e34 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
28  0x7fff33ad4885 -[NSApplication run]
29  0x7fff33aa3a72 NSApplicationMain
30  0x7fff5ec4df57 _xpc_objc_main
31  0x7fff5ec4cbaa xpc_main
LEAK: 2 WebPageProxy

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r244259%20(7447)/results.html
Comment 134 Rob Buis 2019-04-15 10:37:47 PDT
Hi,

(In reply to Ryan Haddad from comment #133)
> The two tests added with this change are consistently failing an assertion
> on macOS and iOS Debug WK2 bots:

So mac-debug and mac-wk2 are not covering this? If so I have no real way of detecting this (except an expensive local WK2 Debug build).
I think I know what is going on and can try to fix it tomorrow.
Comment 135 Rob Buis 2019-04-16 06:49:08 PDT
Reopening to attach new patch.
Comment 136 Rob Buis 2019-04-16 06:49:10 PDT
Created attachment 367534 [details]
Patch
Comment 137 Ryan Haddad 2019-04-17 09:32:05 PDT
Is this ready for review? I'd like to roll out the breaking change if it isn't.
Comment 138 Rob Buis 2019-04-17 10:56:28 PDT
Comment on attachment 367534 [details]
Patch

Ready for review. Another option is to convert the ASSERT to an if check with early return for null response. Both options seem like no-ops to me, so it may be best to just not call it for our case.
Comment 139 Alex Christensen 2019-04-17 11:21:43 PDT
Comment on attachment 367534 [details]
Patch

If that fixes the assertion and doesn't cause any more problems, r=me.
Comment 140 Ryan Haddad 2019-04-17 16:01:40 PDT
Comment on attachment 367534 [details]
Patch

Marking cq+ since EWS is green.
Comment 141 WebKit Commit Bot 2019-04-17 16:29:50 PDT
Comment on attachment 367534 [details]
Patch

Clearing flags on attachment: 367534

Committed r244409: <https://trac.webkit.org/changeset/244409>
Comment 142 WebKit Commit Bot 2019-04-17 16:29:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 143 Shawn Roberts 2019-04-18 08:57:11 PDT
It looks like after https://trac.webkit.org/changeset/244409

http/tests/cache/memory-cache-pruning.html is crashing on Mac WK2 Debug builds.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcache%2Fmemory-cache-pruning.html

Trying to reproduce locally, it may be triggering off another test. Will get repro steps soon hopefully. It appears related tho. 

ERROR: Unhandled web process message 'WebUserContentController:RemoveAllContentRuleLists'
/Volumes/Data/slave/mojave-debug/build/Source/WebKit/WebProcess/WebProcess.cpp(762) : virtual void WebKit::WebProcess::didReceiveMessage(IPC::Connection &, IPC::Decoder &)
ASSERTION FAILED: resources->contains(requestUrl)
/Volumes/Data/slave/mojave-debug/build/Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp(92) : void WebKit::PrefetchCache::clearExpiredEntries()


https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r244422%20(2348)/http/tests/cache/memory-cache-pruning-crash-log.txt
Comment 144 Shawn Roberts 2019-04-18 09:45:57 PDT
rolled out in http://trac.webkit.org/changeset/244424/webkit

Following up causing more assertion failures in Mac WK2 Debug builds.
Comment 145 Shawn Roberts 2019-04-18 12:42:41 PDT
Sorry, linked wrong revision.

Rolled out in https://trac.webkit.org/changeset/244428/webkit