Bug 171422 - Start of support for multiple WebsiteDataStore/SessionIDs per process
Summary: Start of support for multiple WebsiteDataStore/SessionIDs per process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-28 00:54 PDT by Brady Eidson
Modified: 2017-05-01 14:21 PDT (History)
10 users (show)

See Also:


Attachments
Preliminary patch (45.79 KB, patch)
2017-04-28 00:55 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.55 MB, application/zip)
2017-04-28 02:19 PDT, Build Bot
no flags Details
Patch (53.80 KB, patch)
2017-04-28 09:23 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (55.70 KB, patch)
2017-04-28 09:35 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (56.03 KB, patch)
2017-04-28 09:39 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (56.54 KB, patch)
2017-04-28 10:26 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (48.82 KB, patch)
2017-04-28 10:40 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (58.10 KB, patch)
2017-04-28 11:10 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (58.11 KB, patch)
2017-04-28 11:16 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (59.68 KB, patch)
2017-04-28 13:14 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (59.68 KB, patch)
2017-04-28 13:18 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-04-28 00:54:18 PDT
Start of support for multiple WebsiteDataStore/SessionIDs per process
Comment 1 Brady Eidson 2017-04-28 00:55:43 PDT
Created attachment 308508 [details]
Preliminary patch

No ChangeLogs because this isn't quite ready to land (testing things locally), but review can start!
Comment 2 Alex Christensen 2017-04-28 01:34:21 PDT
Comment on attachment 308508 [details]
Preliminary patch

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

We should make a test that makes a WKWebView with a persistent cookie storage at a known location, close the WKWebView and destroy the session, then make another WKWebView with a persistent cookie storage at the same known location and make sure that the cookies are there.  I could imagine some problems with closing cookie storages that such a test would help prevent.

> Source/WebKit2/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:71
> +#if PLATFORM(IOS)

Why only iOS and not Mac?

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:86
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

Why only Mac and not iOS?

> Source/WebKit2/WebProcess/WebProcess.cpp:521
> +void WebProcess::destroySessionOrWebsiteDataStore(SessionID sessionID)

I think destroySession would be a better name.
Comment 3 Build Bot 2017-04-28 02:19:03 PDT
Comment on attachment 308508 [details]
Preliminary patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/tabular-data/processing-model-1/span-limits.html
Comment 4 Build Bot 2017-04-28 02:19:04 PDT
Created attachment 308512 [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 5 Brady Eidson 2017-04-28 09:13:37 PDT
(In reply to Alex Christensen from comment #2)
> Comment on attachment 308508 [details]
> Preliminary patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308508&action=review
> 
> We should make a test that makes a WKWebView with a persistent cookie
> storage at a known location, close the WKWebView and destroy the session,
> then make another WKWebView with a persistent cookie storage at the same
> known location and make sure that the cookies are there.  I could imagine
> some problems with closing cookie storages that such a test would help
> prevent.
> 
> > Source/WebKit2/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:71
> > +#if PLATFORM(IOS)
> 
> Why only iOS and not Mac?

Per NetworkProcessCreationParameters and WebProcessCreationParameters, we currently only need sandbox handle for the cookie path on iOS.

> > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:86
> > +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> 
> Why only Mac and not iOS?

Per NetworkProcessCreationParameters and WebProcessCreationParameters, we currently only need the identifying data on Mac

> > Source/WebKit2/WebProcess/WebProcess.cpp:521
> > +void WebProcess::destroySessionOrWebsiteDataStore(SessionID sessionID)
> 
> I think destroySession would be a better name.

K.
Comment 6 Brady Eidson 2017-04-28 09:23:23 PDT
Created attachment 308538 [details]
Patch
Comment 7 Build Bot 2017-04-28 09:25:14 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 8 Build Bot 2017-04-28 09:25:24 PDT
Attachment 308538 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Brady Eidson 2017-04-28 09:35:33 PDT
Created attachment 308540 [details]
Patch
Comment 10 Brady Eidson 2017-04-28 09:39:18 PDT
Created attachment 308542 [details]
Patch
Comment 11 Brady Eidson 2017-04-28 10:26:23 PDT
Created attachment 308552 [details]
Patch
Comment 12 Geoffrey Garen 2017-04-28 10:29:17 PDT
Comment on attachment 308552 [details]
Patch

r=me
Comment 13 Brady Eidson 2017-04-28 10:40:06 PDT
Created attachment 308555 [details]
Patch
Comment 14 Brady Eidson 2017-04-28 11:10:43 PDT
Created attachment 308561 [details]
Patch
Comment 15 Brady Eidson 2017-04-28 11:16:00 PDT
Created attachment 308562 [details]
Patch
Comment 16 Brady Eidson 2017-04-28 13:14:03 PDT
Created attachment 308587 [details]
Patch
Comment 17 WebKit Commit Bot 2017-04-28 13:15:34 PDT
Comment on attachment 308587 [details]
Patch

Rejecting attachment 308587 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 308587, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3628567
Comment 18 Brady Eidson 2017-04-28 13:18:06 PDT
Created attachment 308588 [details]
Patch
Comment 19 WebKit Commit Bot 2017-04-28 13:48:44 PDT
Comment on attachment 308588 [details]
Patch

Clearing flags on attachment: 308588

Committed r215941: <http://trac.webkit.org/changeset/215941>
Comment 20 WebKit Commit Bot 2017-04-28 13:48:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Alexey Proskuryakov 2017-04-29 19:47:23 PDT
I suspect that this broke all perf tests: https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Perf%29/builds/296/steps/perf-test/logs/stdio
Comment 22 Ryan Haddad 2017-05-01 14:21:32 PDT
This also caused an API test failure on ios-simulator: https://bugs.webkit.org/show_bug.cgi?id=171513