Bug 181240 - Implement Cache API partitioning based on ClientOrigin
Summary: Implement Cache API partitioning based on ClientOrigin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-03 06:12 PST by youenn fablet
Modified: 2018-01-05 17:56 PST (History)
10 users (show)

See Also:


Attachments
Patch (60.44 KB, patch)
2018-01-03 07:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (61.71 KB, patch)
2018-01-03 10:00 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (63.47 KB, patch)
2018-01-03 10:18 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (63.59 KB, patch)
2018-01-04 00:29 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Updated test (65.04 KB, patch)
2018-01-05 15:28 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-01-03 06:12:24 PST
Currently, we are only using the client origin but we should also use the top origin.
Comment 1 youenn fablet 2018-01-03 07:01:25 PST
rdar://problem/36164145
Comment 2 youenn fablet 2018-01-03 07:04:28 PST
Created attachment 330381 [details]
Patch
Comment 3 youenn fablet 2018-01-03 10:00:57 PST
Created attachment 330396 [details]
Patch
Comment 4 EWS Watchlist 2018-01-03 10:03:34 PST
Attachment 330396 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 youenn fablet 2018-01-03 10:18:23 PST
Created attachment 330398 [details]
Patch
Comment 6 EWS Watchlist 2018-01-03 10:21:02 PST
Attachment 330398 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:187:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:223:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Alex Christensen 2018-01-03 12:44:36 PST
Comment on attachment 330398 [details]
Patch

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

> Source/WebCore/page/ClientOrigin.h:75
> +    std::optional<SecurityOriginData> topOrigin, clientOrigin;

We usually have each declaration on its own line.
Comment 8 youenn fablet 2018-01-04 00:29:02 PST
Created attachment 330451 [details]
Patch for landing
Comment 9 EWS Watchlist 2018-01-04 00:32:06 PST
Attachment 330451 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:187:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:223:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 WebKit Commit Bot 2018-01-04 00:49:16 PST
Comment on attachment 330451 [details]
Patch for landing

Clearing flags on attachment: 330451

Committed r226401: <https://trac.webkit.org/changeset/226401>
Comment 11 WebKit Commit Bot 2018-01-04 00:49:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Matt Lewis 2018-01-05 10:29:39 PST
This caused the test http/tests/cache-storage/cache-clearing-origin.https.html to time out consistently on all WK2 platforms. 

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcache-storage%2Fcache-clearing-origin.https.html

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r226448%20(2057)/results.html

Diff:

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/cache-storage/cache-clearing-origin.https-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/cache-storage/cache-clearing-origin.https-actual.txt
@@ -1,5 +1,6 @@
+CONSOLE MESSAGE: line 2451: Unhandled Promise Rejection: Error: assert_equals: client origin of cache 1 expected "https://localhost:8443" but got "https://127.0.0.1:8443"
+#PID UNRESPONSIVE - com.apple.WebKit.WebContent.Development (pid 52826)
+FAIL: Timed out waiting for notifyDone to be called
 
-
-PASS Create a cache storage from localhost and clear it 
-PASS Clearing disk cache of a given origin 
-
+#EOF
+#EOF
Comment 13 youenn fablet 2018-01-05 10:37:39 PST
Will look at it over the weekend. Could you add an expectation in the meantime?
Comment 14 Matt Lewis 2018-01-05 10:47:46 PST
This is causing the trees to be very red. Rather than setting an expectation, I would rather roll back the change until it can be landed without issue.
Comment 15 Matt Lewis 2018-01-05 11:01:54 PST
Reverted r226401 for reason:

This caused timeouts on multiple platforms.

Committed r226453: <https://trac.webkit.org/changeset/226453>
Comment 16 youenn fablet 2018-01-05 15:25:39 PST
(In reply to Matt Lewis from comment #15)
> Reverted r226401 for reason:
> 
> This caused timeouts on multiple platforms.
> 
> Committed r226453: <https://trac.webkit.org/changeset/226453>

Reason was the modified test was not always deterministic, leading to the timeout.
Sorting the cache representation in the test fixes the timeout.
I will reland with the updated test.
Comment 17 youenn fablet 2018-01-05 15:28:58 PST
Created attachment 330596 [details]
Updated test
Comment 18 EWS Watchlist 2018-01-05 15:30:39 PST
Attachment 330596 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:187:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/Modules/cache/DOMCacheStorage.cpp:223:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 WebKit Commit Bot 2018-01-05 17:56:27 PST
Comment on attachment 330596 [details]
Updated test

Clearing flags on attachment: 330596

Committed r226481: <https://trac.webkit.org/changeset/226481>
Comment 20 WebKit Commit Bot 2018-01-05 17:56:29 PST
All reviewed patches have been landed.  Closing bug.