Attachment 356429 Details for Bug 192337 – Patch

[patch] Patch

bug-192337-20181203160634.patch (text/plain), 18.18 KB, created by Chris Dumez on 2018-12-03 16:06:35 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-12-03 16:06:35 PST

Size: 18.18 KB

patch

obsolete

>Subversion Revision: 238826
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 5c796a8ddbf7096a9c7544bf1573c3c326285b2d..e45b28e69fc6f7320b38c114ab279b9afc6c906d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,34 @@
>+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
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index b2593ccb9e408e4dab2d3efae584ec3da6f1a452..77fc452e0a9468c31d71e634a59c923958dca2df 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -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);
> 
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index cc740116fc3d70786ebf22b173d146786a83dff2..3e5328114c3145200d6ebf57e8a3297cf44b2ed1 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -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();
> 
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index e42377950f646a63397900b333eaba2a1353b170..6f79a832642161e0c410c80501dadf469e405866 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -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(); }
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 044a7fe2a655ebf147cf7a4af40df420c298c8b2..fc3bf0fedea43c9d47bb4039d661b712f57f171d 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,18 @@
>+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
>diff --git a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>index 010376f9bb44824b78886e0ef76aa00535c6eeb5..6d7fa37d9ccef498e32838c0cd63715862b0449d 100644
>--- a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>+++ b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>@@ -173,6 +173,8 @@
> 		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 */; };
>@@ -1077,6 +1079,7 @@
> 				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 */,
>@@ -1202,6 +1205,7 @@
> 				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 */,
>@@ -1492,6 +1496,8 @@
> 		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>"; };
>@@ -2791,6 +2797,7 @@
> 				CDE195B21CFE0ADE0053D256 /* FullscreenTopContentInset.html */,
> 				636353A61E9861940009F8AF /* GeolocationGetCurrentPositionResult.html */,
> 				07E1F6A11FFC44F90096C7EC /* getDisplayMedia.html */,
>+				467C565121B5ECDF0057516D /* GetSessionCookie.html */,
> 				F47D30ED1ED28A6C000482E1 /* gif-and-file-input.html */,
> 				510477761D298E57009747EB /* IDBDeleteRecovery.html */,
> 				5104776F1D298D85009747EB /* IDBDeleteRecovery.sqlite3 */,
>@@ -2875,6 +2882,7 @@
> 				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
>index 0000000000000000000000000000000000000000..010bb77f9927af434bc878436f3133cb27dc38d0
>--- /dev/null
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/GetSessionCookie.html
>@@ -0,0 +1,3 @@
>+<script>
>+window.webkit.messageHandlers.testHandler.postMessage(document.cookie);
>+</script>
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 1d642d554ac15583351253a4d0c735b70e65e71b..fd00363eee22a5d318adcdf77571192f5e295df0 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -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
>index 0000000000000000000000000000000000000000..937e274bf0bd6168499fb95553dacf4bf76372a2
>--- /dev/null
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/SetSessionCookie.html
>@@ -0,0 +1,3 @@
>+<script>
>+document.cookie = "foo=bar";
>+</script>

Actions: View | Formatted Diff | Diff

Attachments on bug 192337: 356418 | 356420 | 356429