Regression(PSON) Google OAuth is broken in private sessions
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Dec 2018 00:08:42 +0000 (00:08 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Dec 2018 00:08:42 +0000 (00:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192337
<rdar://problem/46353558>

Reviewed by Alex Christensen.

Source/WebKit:

In WebPageProxy::swapToWebProcess(), we would call removeWebPage() on the old WebProcessProxy and then
addExistingWebPage() on the new WebProcessProxy, as you would expect in case of process swap.

The issue is that WebProcessProxy::removeWebPage() calls WebProcessPool::pageEndUsingWebsiteDataStore()
which would cause the session to get destroyed assuming this was the last page using it. We would
therefore lose session cookies after a process-swap in private session.

To address the issue, a parameter to WebProcessPool::pageEndUsingWebsiteDataStore() and
WebProcessPool::pageBeginUsingWebsiteDataStore() to control if we want to tell the WebProcessPool
about the page beginning / ending its use of the session. In the case of a process-swap, we make
sure the process pool is not notified.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::reattachToWebProcess):
(WebKit::WebPageProxy::swapToWebProcess):
(WebKit::WebPageProxy::finishAttachingToWebProcess):
(WebKit::WebPageProxy::close):
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::createWebPage):
(WebKit::WebProcessProxy::addExistingWebPage):
(WebKit::WebProcessProxy::removeWebPage):
* UIProcess/WebProcessProxy.h:

Tools:

Add API test coverage.

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKitCocoa/GetSessionCookie.html: Added.
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
* TestWebKitAPI/Tests/WebKitCocoa/SetSessionCookie.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238828 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/UIProcess/WebProcessProxy.h
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKitCocoa/GetSessionCookie.html [new file with mode: 0644]
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
Tools/TestWebKitAPI/Tests/WebKitCocoa/SetSessionCookie.html [new file with mode: 0644]

index 5c796a8..e45b28e 100644 (file)
@@ -1,5 +1,36 @@
 2018-12-03  Chris Dumez  <cdumez@apple.com>
 
+        Regression(PSON) Google OAuth is broken in private sessions
+        https://bugs.webkit.org/show_bug.cgi?id=192337
+        <rdar://problem/46353558>
+
+        Reviewed by Alex Christensen.
+
+        In WebPageProxy::swapToWebProcess(), we would call removeWebPage() on the old WebProcessProxy and then
+        addExistingWebPage() on the new WebProcessProxy, as you would expect in case of process swap.
+
+        The issue is that WebProcessProxy::removeWebPage() calls WebProcessPool::pageEndUsingWebsiteDataStore()
+        which would cause the session to get destroyed assuming this was the last page using it. We would
+        therefore lose session cookies after a process-swap in private session.
+
+        To address the issue, a parameter to WebProcessPool::pageEndUsingWebsiteDataStore() and
+        WebProcessPool::pageBeginUsingWebsiteDataStore() to control if we want to tell the WebProcessPool
+        about the page beginning / ending its use of the session. In the case of a process-swap, we make
+        sure the process pool is not notified.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::reattachToWebProcess):
+        (WebKit::WebPageProxy::swapToWebProcess):
+        (WebKit::WebPageProxy::finishAttachingToWebProcess):
+        (WebKit::WebPageProxy::close):
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::createWebPage):
+        (WebKit::WebProcessProxy::addExistingWebPage):
+        (WebKit::WebProcessProxy::removeWebPage):
+        * UIProcess/WebProcessProxy.h:
+
+2018-12-03  Chris Dumez  <cdumez@apple.com>
+
         [PSON] Request by the client to process-swap is ignored if the window has an opener
         https://bugs.webkit.org/show_bug.cgi?id=192267
         <rdar://problem/46386886>
index b2593cc..77fc452 100644 (file)
@@ -727,13 +727,16 @@ void WebPageProxy::reattachToWebProcess()
     ASSERT(!isValid());
     RELEASE_LOG_IF_ALLOWED(Process, "%p WebPageProxy::reattachToWebProcess\n", this);
 
-    m_process->removeWebPage(*this, m_pageID);
+    m_process->removeWebPage(*this, m_pageID, WebProcessProxy::EndsUsingDataStore::Yes);
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
 
     auto& processPool = m_process->processPool();
     m_process = processPool.createNewWebProcessRespectingProcessCountLimit(m_websiteDataStore.get());
     m_isValid = true;
 
+    m_process->addExistingWebPage(*this, m_pageID, WebProcessProxy::BeginsUsingDataStore::Yes);
+    m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID, *this);
+
     finishAttachingToWebProcess();
 }
 
@@ -779,7 +782,7 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigat
 
         m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
         bool didSuspendPreviousPage = suspendCurrentPageIfPossible(navigation, mainFrameIDInPreviousProcess, processSwapRequestedByClient);
-        m_process->removeWebPage(*this, m_pageID);
+        m_process->removeWebPage(*this, m_pageID, WebProcessProxy::EndsUsingDataStore::No);
 
         m_process = WTFMove(process);
         m_isValid = true;
@@ -795,6 +798,9 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigat
             m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
         }
 
+        m_process->addExistingWebPage(*this, m_pageID, WebProcessProxy::BeginsUsingDataStore::No);
+        m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID, *this);
+
         finishAttachingToWebProcess(didSuspendPreviousPage ? ShouldDelayAttachingDrawingArea::Yes : ShouldDelayAttachingDrawingArea::No);
         completionHandler(didSuspendPreviousPage);
     };
@@ -814,9 +820,6 @@ void WebPageProxy::finishAttachingToWebProcess(ShouldDelayAttachingDrawingArea s
         processDidFinishLaunching();
     }
 
-    m_process->addExistingWebPage(*this, m_pageID);
-    m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID, *this);
-
     updateActivityState();
     updateThrottleState();
 
@@ -996,7 +999,7 @@ void WebPageProxy::close()
     m_process->processPool().removeAllSuspendedPagesForPage(*this);
 
     m_process->send(Messages::WebPage::Close(), m_pageID);
-    m_process->removeWebPage(*this, m_pageID);
+    m_process->removeWebPage(*this, m_pageID, WebProcessProxy::EndsUsingDataStore::Yes);
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
     m_process->processPool().supplement<WebNotificationManagerProxy>()->clearNotifications(this);
 
index cc74011..3e53281 100644 (file)
@@ -428,17 +428,18 @@ Ref<WebPageProxy> WebProcessProxy::createWebPage(PageClient& pageClient, Ref<API
     uint64_t pageID = generatePageID();
     Ref<WebPageProxy> webPage = WebPageProxy::create(pageClient, *this, pageID, WTFMove(pageConfiguration));
 
-    addExistingWebPage(webPage.get(), pageID);
+    addExistingWebPage(webPage.get(), pageID, BeginsUsingDataStore::Yes);
 
     return webPage;
 }
 
-void WebProcessProxy::addExistingWebPage(WebPageProxy& webPage, uint64_t pageID)
+void WebProcessProxy::addExistingWebPage(WebPageProxy& webPage, uint64_t pageID, BeginsUsingDataStore beginsUsingDataStore)
 {
     ASSERT(!m_pageMap.contains(pageID));
     ASSERT(!globalPageMap().contains(pageID));
 
-    m_processPool->pageBeginUsingWebsiteDataStore(webPage);
+    if (beginsUsingDataStore == BeginsUsingDataStore::Yes)
+        m_processPool->pageBeginUsingWebsiteDataStore(webPage);
 
     m_pageMap.set(pageID, &webPage);
     globalPageMap().set(pageID, &webPage);
@@ -457,14 +458,15 @@ void WebProcessProxy::markIsNoLongerInPrewarmedPool()
     send(Messages::WebProcess::MarkIsNoLongerPrewarmed(), 0);
 }
 
-void WebProcessProxy::removeWebPage(WebPageProxy& webPage, uint64_t pageID)
+void WebProcessProxy::removeWebPage(WebPageProxy& webPage, uint64_t pageID, EndsUsingDataStore endsUsingDataStore)
 {
     auto* removedPage = m_pageMap.take(pageID);
     ASSERT_UNUSED(removedPage, removedPage == &webPage);
     removedPage = globalPageMap().take(pageID);
     ASSERT_UNUSED(removedPage, removedPage == &webPage);
 
-    m_processPool->pageEndUsingWebsiteDataStore(webPage);
+    if (endsUsingDataStore == EndsUsingDataStore::Yes)
+        m_processPool->pageEndUsingWebsiteDataStore(webPage);
 
     updateBackgroundResponsivenessTimer();
 
index e423779..6f79a83 100644 (file)
@@ -117,8 +117,12 @@ public:
     static WebProcessProxy* processForIdentifier(WebCore::ProcessIdentifier);
     static WebPageProxy* webPage(uint64_t pageID);
     Ref<WebPageProxy> createWebPage(PageClient&, Ref<API::PageConfiguration>&&);
-    void addExistingWebPage(WebPageProxy&, uint64_t pageID);
-    void removeWebPage(WebPageProxy&, uint64_t pageID);
+
+    enum class BeginsUsingDataStore : bool { No, Yes };
+    void addExistingWebPage(WebPageProxy&, uint64_t pageID, BeginsUsingDataStore);
+
+    enum class EndsUsingDataStore : bool { No, Yes };
+    void removeWebPage(WebPageProxy&, uint64_t pageID, EndsUsingDataStore);
 
     typename WebPageProxyMap::ValuesConstIteratorRange pages() const { return m_pageMap.values(); }
     unsigned pageCount() const { return m_pageMap.size(); }
index 044a7fe..fc3bf0f 100644 (file)
@@ -1,5 +1,20 @@
 2018-12-03  Chris Dumez  <cdumez@apple.com>
 
+        Regression(PSON) Google OAuth is broken in private sessions
+        https://bugs.webkit.org/show_bug.cgi?id=192337
+        <rdar://problem/46353558>
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKitCocoa/GetSessionCookie.html: Added.
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+        * TestWebKitAPI/Tests/WebKitCocoa/SetSessionCookie.html: Added.
+
+2018-12-03  Chris Dumez  <cdumez@apple.com>
+
         [PSON] Request by the client to process-swap is ignored if the window has an opener
         https://bugs.webkit.org/show_bug.cgi?id=192267
         <rdar://problem/46386886>
index 010376f..6d7fa37 100644 (file)
                46397B951DC2C850009A78AE /* DOMNode.mm in Sources */ = {isa = PBXBuildFile; fileRef = 46397B941DC2C850009A78AE /* DOMNode.mm */; };
                4647B1261EBA3B850041D7EF /* ProcessDidTerminate.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 4647B1251EBA3B730041D7EF /* ProcessDidTerminate.cpp */; };
                466C3843210637DE006A88DE /* notify-resourceLoadObserver.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 466C3842210637CE006A88DE /* notify-resourceLoadObserver.html */; };
+               467C565321B5ED130057516D /* GetSessionCookie.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 467C565121B5ECDF0057516D /* GetSessionCookie.html */; };
+               467C565421B5ED130057516D /* SetSessionCookie.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 467C565221B5ECDF0057516D /* SetSessionCookie.html */; };
                46A911592108E6780078D40D /* CustomUserAgent.mm in Sources */ = {isa = PBXBuildFile; fileRef = 46A911582108E66B0078D40D /* CustomUserAgent.mm */; };
                46AE5A3720F9066D00E0873E /* SimpleServiceWorkerRegistrations-3.sqlite3 in Copy Resources */ = {isa = PBXBuildFile; fileRef = 4656A75720F9054F0002E21F /* SimpleServiceWorkerRegistrations-3.sqlite3 */; };
                46C519DA1D355AB200DAA51A /* LocalStorageNullEntries.mm in Sources */ = {isa = PBXBuildFile; fileRef = 46C519D81D355A7300DAA51A /* LocalStorageNullEntries.mm */; };
                                26F52EB218288F240023D412 /* geolocationWatchPosition.html in Copy Resources */,
                                26F52EB318288F240023D412 /* geolocationWatchPositionWithHighAccuracy.html in Copy Resources */,
                                07E1F6A21FFC44FA0096C7EC /* getDisplayMedia.html in Copy Resources */,
+                               467C565321B5ED130057516D /* GetSessionCookie.html in Copy Resources */,
                                074994421EA5034B000DA44E /* getUserMedia.html in Copy Resources */,
                                F46A095B1ED8A6E600D4AA55 /* gif-and-file-input.html in Copy Resources */,
                                9B4F8FA7159D52DD002D9F94 /* HTMLCollectionNamedItem.html in Copy Resources */,
                                7A66BDB81EAF18D500CCC924 /* set-long-title.html in Copy Resources */,
                                CE6E81A420A933D500E2C80F /* set-timeout-function.html in Copy Resources */,
                                52B8CF9815868D9100281053 /* SetDocumentURI.html in Copy Resources */,
+                               467C565421B5ED130057516D /* SetSessionCookie.html in Copy Resources */,
                                CEBABD491B71687C0051210A /* should-open-external-schemes.html in Copy Resources */,
                                F4E3D80820F70BB9007B58C5 /* significant-text-milestone-article.html in Copy Resources */,
                                F4FC077720F013B600CA043C /* significant-text-milestone.html in Copy Resources */,
                4647B1251EBA3B730041D7EF /* ProcessDidTerminate.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ProcessDidTerminate.cpp; sourceTree = "<group>"; };
                4656A75720F9054F0002E21F /* SimpleServiceWorkerRegistrations-3.sqlite3 */ = {isa = PBXFileReference; lastKnownFileType = file; path = "SimpleServiceWorkerRegistrations-3.sqlite3"; sourceTree = "<group>"; };
                466C3842210637CE006A88DE /* notify-resourceLoadObserver.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "notify-resourceLoadObserver.html"; sourceTree = "<group>"; };
+               467C565121B5ECDF0057516D /* GetSessionCookie.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = GetSessionCookie.html; sourceTree = "<group>"; };
+               467C565221B5ECDF0057516D /* SetSessionCookie.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = SetSessionCookie.html; sourceTree = "<group>"; };
                46A911582108E66B0078D40D /* CustomUserAgent.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = CustomUserAgent.mm; sourceTree = "<group>"; };
                46C519D81D355A7300DAA51A /* LocalStorageNullEntries.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = LocalStorageNullEntries.mm; sourceTree = "<group>"; };
                46C519E21D35629600DAA51A /* LocalStorageNullEntries.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = LocalStorageNullEntries.html; sourceTree = "<group>"; };
                                CDE195B21CFE0ADE0053D256 /* FullscreenTopContentInset.html */,
                                636353A61E9861940009F8AF /* GeolocationGetCurrentPositionResult.html */,
                                07E1F6A11FFC44F90096C7EC /* getDisplayMedia.html */,
+                               467C565121B5ECDF0057516D /* GetSessionCookie.html */,
                                F47D30ED1ED28A6C000482E1 /* gif-and-file-input.html */,
                                510477761D298E57009747EB /* IDBDeleteRecovery.html */,
                                5104776F1D298D85009747EB /* IDBDeleteRecovery.sqlite3 */,
                                2E92B8F6216490C3005B64F0 /* rich-text-attributes.html */,
                                F4E0A295211FC5A300AF7C7F /* selected-text-and-textarea.html */,
                                F4D65DA71F5E46C0009D8C27 /* selected-text-image-link-and-editable.html */,
+                               467C565221B5ECDF0057516D /* SetSessionCookie.html */,
                                F4E3D80720F708E4007B58C5 /* significant-text-milestone-article.html */,
                                F4FC077620F0108100CA043C /* significant-text-milestone.html */,
                                C9B4AD291ECA6EA500F5FEA0 /* silence-long.m4a */,
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/GetSessionCookie.html b/Tools/TestWebKitAPI/Tests/WebKitCocoa/GetSessionCookie.html
new file mode 100644 (file)
index 0000000..010bb77
--- /dev/null
@@ -0,0 +1,3 @@
+<script>
+window.webkit.messageHandlers.testHandler.postMessage(document.cookie);
+</script>
index 1d642d5..fd00363 100644 (file)
@@ -2988,6 +2988,63 @@ TEST(ProcessSwap, UsePrewarmedProcessAfterTerminatingNetworkProcess)
     done = false;
 }
 
+TEST(ProcessSwap, UseSessionCookiesAfterProcessSwapInPrivateBrowsing)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    processPoolConfiguration.get().processSwapsOnNavigation = YES;
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    RetainPtr<WKWebsiteDataStore> ephemeralStore = [WKWebsiteDataStore nonPersistentDataStore];
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    webViewConfiguration.get().websiteDataStore = ephemeralStore.get();
+
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"pson"];
+
+    auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
+    [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"testHandler"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"SetSessionCookie" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pid1 = [webView _webProcessIdentifier];
+
+    // Should process-swap.
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pid2 = [webView _webProcessIdentifier];
+    EXPECT_NE(pid1, pid2);
+
+    // Should process-swap.
+    request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"GetSessionCookie" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pid3 = [webView _webProcessIdentifier];
+    EXPECT_NE(pid2, pid3);
+
+    TestWebKitAPI::Util::run(&receivedMessage);
+    receivedMessage = false;
+
+    EXPECT_EQ(1u, [receivedMessages count]);
+    EXPECT_WK_STREQ(@"foo=bar", receivedMessages.get()[0]);
+}
+
 #if PLATFORM(MAC)
 
 TEST(ProcessSwap, TerminateProcessAfterProcessSwap)
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/SetSessionCookie.html b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SetSessionCookie.html
new file mode 100644 (file)
index 0000000..937e274
--- /dev/null
@@ -0,0 +1,3 @@
+<script>
+document.cookie = "foo=bar";
+</script>