Summary: | Move NetworkStorageSession management to WebCore | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | beidson, buildbot, ossy, pvollan, rniwa, sam | ||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2016-07-25 15:00:51 PDT
Created attachment 284527 [details]
Patch
Created attachment 284528 [details]
Patch
Created attachment 284529 [details]
Patch
Created attachment 284530 [details]
Patch
Created attachment 284531 [details]
Patch
Created attachment 284534 [details]
Patch
Created attachment 284537 [details]
Patch
Created attachment 284538 [details]
Patch
Comment on attachment 284538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284538&action=review After all of these changes, does SessionTracker need to remain "a thing"? > Source/WebCore/ChangeLog:8 > + No new tests. No change in behaviour. Great, but... > Source/WebCore/ChangeLog:22 > + (WebCore::NetworkStorageSession::forEach): > + Moved from forEachNetworkStorageSession and fixed the FIXME. This function now includes the default NetworkStorageSession. ...This is a behavior change? > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:86 > +static HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>& globalSessionMap() > +{ > + static NeverDestroyed<HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>> map; > + return map; > +} > + > +NetworkStorageSession* NetworkStorageSession::storageSession(SessionID sessionID) > +{ > + if (sessionID == SessionID::defaultSessionID()) > + return &defaultStorageSession(); > + return globalSessionMap().get(sessionID); > +} CFNet versions, but then.... > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:67 > +static HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>& globalSessionMap() > +{ > + static NeverDestroyed<HashMap<SessionID, std::unique_ptr<NetworkStorageSession>>> map; > + return map; > +} > + > +NetworkStorageSession* NetworkStorageSession::storageSession(SessionID sessionID) > +{ > + if (sessionID == SessionID::defaultSessionID()) > + return &defaultStorageSession(); > + return globalSessionMap().get(sessionID); > +} ...Soup versions? Can't these be in shared code? > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:94 > +void NetworkStorageSession::destroySession(SessionID sessionID) > +{ > + ASSERT(globalSessionMap().contains(sessionID)); > + globalSessionMap().remove(sessionID); > +} > + > +void NetworkStorageSession::forEach(std::function<void(const WebCore::NetworkStorageSession&)> functor) > +{ > + functor(defaultStorageSession()); > + for (auto& storageSession : globalSessionMap().values()) > + functor(*storageSession); And these, etc etc... (In reply to comment #9) > Comment on attachment 284538 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284538&action=review > > After all of these changes, does SessionTracker need to remain "a thing"? > > > Source/WebCore/ChangeLog:8 > > + No new tests. No change in behaviour. > > Great, but... > > > Source/WebCore/ChangeLog:22 > > + (WebCore::NetworkStorageSession::forEach): > > + Moved from forEachNetworkStorageSession and fixed the FIXME. This function now includes the default NetworkStorageSession. > > ...This is a behavior change? This would be a behavior change if I didn't also change the call sites to not do the same thing to the default NetworkStorageSession. With those changes it is not a behavior change. Created attachment 284541 [details]
Patch
Comment on attachment 284541 [details] Patch Attachment 284541 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1753385 New failing tests: http/tests/cookies/private-cookie-storage.html http/tests/security/contentSecurityPolicy/report-same-origin-no-cookies-when-private-browsing-toggled.php http/tests/security/private-browsing-http-auth.html Created attachment 284545 [details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 284541 [details] Patch Attachment 284541 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1753459 Number of test failures exceeded the failure limit. Created attachment 284553 [details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
I have a vision for Sessions, and I am curious if this gets us closer to it or further away. My vision is that we stop passing SessionIDs around, and invert the equation. Add a WebCore::Session object (name TBD) and have it own instances of all the things SessionIDs are currently used to track. For instance, it would have a MemoryCache instance, rather than MemoryCache having a mapping of SessionIDs to CachedResourceMaps. Additionally, the WebCore::Session could contain an NetworkStorageSession, or, NetworkStorageSession could be folded into it. Created attachment 285680 [details]
Patch
(In reply to comment #16) > I have a vision for Sessions, and I am curious if this gets us closer to it > or further away. This patch gets us closer to that. We will still need to have SessionID's, though, because we need to pass them across IPC. (In reply to comment #16) > I have a vision for Sessions, and I am curious if this gets us closer to it > or further away. > > My vision is that we stop passing SessionIDs around... But then... (In reply to comment #18) > This patch gets us closer to that. We will still need to have SessionID's, > though, because we need to pass them across IPC. These two comments are at odds. (In reply to comment #19) > (In reply to comment #16) > > I have a vision for Sessions, and I am curious if this gets us closer to it > > or further away. > > > > My vision is that we stop passing SessionIDs around... > > But then... > > (In reply to comment #18) > > This patch gets us closer to that. We will still need to have SessionID's, > > though, because we need to pass them across IPC. > > These two comments are at odds. Not really. Having WebCore invert the relationship to use a Session object that contains a Session identifier is fine. Just as we have Page from which we can get a Page identifier for IPC purposes. Moving the NetworkStorageSession management to WebCore helps us progress towards the existence of a WebCore::Session because before that can be done, everything that a SessionID is used for must be moved to WebCore. Created attachment 285690 [details]
Patch
Created attachment 285715 [details]
Patch
(In reply to comment #24) > https://trac.webkit.org/changeset/204327 It broke rhe WinCairo build. Comment on attachment 285715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285715&action=review > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:-74 > - if (RetainPtr<CFHTTPCookieStorageRef> cookieStorage = NetworkStorageSession::defaultStorageSession().cookieStorage()) This null check was removed in this change. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebFrameNetworkingContext.mm:73 > CFHTTPCookieStorageSetCookieAcceptPolicy(networkStorageSession.cookieStorage().get(), policy); This needs a null check. See https://bugs.webkit.org/show_bug.cgi?id=160758 (In reply to comment #25) > (In reply to comment #24) > > https://trac.webkit.org/changeset/204327 > > It broke rhe WinCairo build. just to document, fixed by https://trac.webkit.org/changeset/204366 |