Add smart pointers to CookieStore and CookieJar #28683

abigailfox · 2024-05-16T22:55:07Z

`1963873`

Add smart pointers to CookieStore and CookieJar
https://bugs.webkit.org/show_bug.cgi?id=274285
rdar://128231784

Reviewed by Chris Dumez and Ryosuke Niwa.

* Source/WebCore/Modules/cookie-store/CookieStore.cpp:
(WebCore::CookieStore::MainThreadBridge::ensureOnMainThread):
(WebCore::CookieStore::MainThreadBridge::get):
(WebCore::CookieStore::get):
(WebCore::CookieStore::getAll):
(WebCore::CookieStore::set):
(WebCore::CookieStore::eventListenersDidChange):
* Source/WebCore/loader/CookieJar.cpp:
(WebCore::CookieJar::cookies const):
(WebCore::CookieJar::cookieRequestHeaderFieldProxy):
(WebCore::CookieJar::setCookies):
(WebCore::CookieJar::cookiesEnabled):
(WebCore::CookieJar::cookieRequestHeaderFieldValue const):
(WebCore::CookieJar::getRawCookies const):
(WebCore::CookieJar::setRawCookie):
(WebCore::CookieJar::deleteCookie):

Canonical link: https://commits.webkit.org/279064@main

a0d0a7e

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-16T22:58:18Z

EWS run on previous version of this PR (hash 8b78e38)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	⏳ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	⏳ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-17T01:47:52Z

EWS run on previous version of this PR (hash a6d69d7)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2024-05-17T01:50:47Z

Source/WebCore/Modules/cookie-store/CookieStore.cpp

@@ -272,7 +272,7 @@ void CookieStore::get(CookieStoreGetOptions&& options, Ref<DeferredPromise>&& pr
        promise->resolve<IDLDictionary<CookieListItem>>(CookieListItem(WTFMove(cookies[0])));
    };

-    m_mainThreadBridge->get(WTFMove(options), WTFMove(completionHandler));
+    Ref { m_mainThreadBridge }->get(WTFMove(options), WTFMove(completionHandler));


We should add a protectedMainBridge() function that returns a Ref, since this is done several times.

cdumez · 2024-05-17T01:52:33Z

Source/WebCore/loader/CookieJar.cpp

        frameID = frame->loader().frameID();

    std::pair<String, bool> result;
-    if (auto* session = m_storageSessionProvider->storageSession())
+    if (WeakPtr session = Ref { m_storageSessionProvider }->storageSession())


We should add a protectedStorageSessionProvider() since this is done several times.

rniwa · 2024-05-17T01:52:47Z

Source/WebCore/Modules/cookie-store/CookieStore.cpp

@@ -272,7 +272,7 @@ void CookieStore::get(CookieStoreGetOptions&& options, Ref<DeferredPromise>&& pr
        promise->resolve<IDLDictionary<CookieListItem>>(CookieListItem(WTFMove(cookies[0])));
    };

-    m_mainThreadBridge->get(WTFMove(options), WTFMove(completionHandler));
+    Ref { m_mainThreadBridge }->get(WTFMove(options), WTFMove(completionHandler));


Rather than keep repeating Ref { m_mainThreadBridge }, we should add protectedMainThreadBridge function which returns a Ref.

rniwa · 2024-05-17T01:53:09Z

Source/WebCore/Modules/cookie-store/CookieStore.cpp

@@ -98,7 +98,7 @@ void CookieStore::MainThreadBridge::ensureOnMainThread(Function<void(ScriptExecu
 {
    ASSERT(m_cookieStore);

-    RefPtr context = m_cookieStore->scriptExecutionContext();
+    RefPtr context = RefPtr { m_cookieStore.get() }->scriptExecutionContext();


I think the preferred convention is to add protectedCookieStore which returns a Ref and call that here.

rniwa · 2024-05-17T01:53:36Z

Source/WebCore/loader/CookieJar.cpp

        frameID = frame->loader().frameID();

-    if (auto* session = m_storageSessionProvider->storageSession())
+    if (WeakPtr session = Ref { m_storageSessionProvider }->storageSession())


Same comment for adding protectedStorageSessionProvider() instead.

rniwa · 2024-05-17T01:57:31Z

Source/WebCore/loader/CookieJar.cpp

        frameID = frame->loader().frameID();

-    if (auto* session = m_storageSessionProvider->storageSession())
+    if (WeakPtr session = Ref { m_storageSessionProvider }->storageSession())


We should probably make NetworkStorageSession inherit from CanMakeCheckedPtr and use a CheckedPtr here instead. WeakPtr doesn't prevent UAF here because even if WeakPtr was not null at the time of if condition evaluation, it can become null later underneath us. CheckedPtr will prevent UAF in that case.

charliewolfe · 2024-05-17T03:18:42Z

Source/WebCore/loader/CookieJar.cpp

@@ -123,10 +123,10 @@ void CookieJar::setCookies(Document& document, const URL& url, const String& coo
 {
    auto pageID = document.pageID();
    std::optional<FrameIdentifier> frameID;
-    if (auto* frame = document.frame())
+    if (RefPtr frame = document.frame())
        frameID = frame->loader().frameID();


Couldn't these just be frameID = frame->frameID();? Then we wouldn't need the smart pointer.

webkit-early-warning-system · 2024-05-18T03:14:24Z

EWS run on previous version of this PR (hash 30aad1f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

rniwa · 2024-05-18T04:24:21Z

Source/WebCore/platform/network/NetworkStorageSession.h

+class NetworkStorageSession
+    : public CanMakeWeakPtr<NetworkStorageSession>
+    , public CanMakeCheckedPtr<NetworkStorageSession> {
+        WTF_MAKE_NONCOPYABLE(NetworkStorageSession);


Nit: I don't think we normally extra indent macros like this.

webkit-early-warning-system · 2024-05-20T17:20:51Z

EWS run on current version of this PR (hash a0d0a7e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=274285 rdar://128231784 Reviewed by Chris Dumez and Ryosuke Niwa. * Source/WebCore/Modules/cookie-store/CookieStore.cpp: (WebCore::CookieStore::MainThreadBridge::ensureOnMainThread): (WebCore::CookieStore::MainThreadBridge::get): (WebCore::CookieStore::get): (WebCore::CookieStore::getAll): (WebCore::CookieStore::set): (WebCore::CookieStore::eventListenersDidChange): * Source/WebCore/loader/CookieJar.cpp: (WebCore::CookieJar::cookies const): (WebCore::CookieJar::cookieRequestHeaderFieldProxy): (WebCore::CookieJar::setCookies): (WebCore::CookieJar::cookiesEnabled): (WebCore::CookieJar::cookieRequestHeaderFieldValue const): (WebCore::CookieJar::getRawCookies const): (WebCore::CookieJar::setRawCookie): (WebCore::CookieJar::deleteCookie): Canonical link: https://commits.webkit.org/279064@main

webkit-commit-queue · 2024-05-21T17:42:20Z

Committed 279064@main (1963873): https://commits.webkit.org/279064@main

Reviewed commits have been landed. Closing PR #28683 and removing active labels.

abigailfox requested a review from cdumez as a code owner May 16, 2024 22:55

abigailfox self-assigned this May 16, 2024

abigailfox added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label May 16, 2024

abigailfox marked this pull request as draft May 17, 2024 01:19

abigailfox force-pushed the eng/Add-smart-pointers-to-CookieStore-and-CookieJar branch from 8b78e38 to a6d69d7 Compare May 17, 2024 01:47

abigailfox marked this pull request as ready for review May 17, 2024 01:49

abigailfox requested review from ddkilzer and rniwa May 17, 2024 01:50

cdumez reviewed May 17, 2024

View reviewed changes

rniwa reviewed May 17, 2024

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 17, 2024

charliewolfe reviewed May 17, 2024

View reviewed changes

abigailfox removed the merging-blocked Applied to prevent a change from being merged label May 18, 2024

abigailfox force-pushed the eng/Add-smart-pointers-to-CookieStore-and-CookieJar branch from a6d69d7 to 30aad1f Compare May 18, 2024 03:14

cdumez approved these changes May 18, 2024

View reviewed changes

rniwa reviewed May 18, 2024

View reviewed changes

rniwa approved these changes May 18, 2024

View reviewed changes

abigailfox force-pushed the eng/Add-smart-pointers-to-CookieStore-and-CookieJar branch from 30aad1f to a0d0a7e Compare May 20, 2024 17:20

abigailfox added the merge-queue Applied to send a pull request to merge-queue label May 21, 2024

webkit-commit-queue force-pushed the eng/Add-smart-pointers-to-CookieStore-and-CookieJar branch from a0d0a7e to 1963873 Compare May 21, 2024 17:42

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 21, 2024

webkit-commit-queue merged commit 1963873 into WebKit:main May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add smart pointers to CookieStore and CookieJar #28683

Add smart pointers to CookieStore and CookieJar #28683

abigailfox commented May 16, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 16, 2024 •

edited

webkit-early-warning-system commented May 17, 2024 •

edited

cdumez May 17, 2024

cdumez May 17, 2024

rniwa May 17, 2024

rniwa May 17, 2024

rniwa May 17, 2024

rniwa May 17, 2024 •

edited

charliewolfe May 17, 2024

webkit-early-warning-system commented May 18, 2024 •

edited

rniwa May 18, 2024

webkit-early-warning-system commented May 20, 2024 •

edited

webkit-commit-queue commented May 21, 2024

Add smart pointers to CookieStore and CookieJar #28683

Add smart pointers to CookieStore and CookieJar #28683

Conversation

abigailfox commented May 16, 2024 • edited by webkit-early-warning-system

webkit-early-warning-system commented May 16, 2024 • edited

webkit-early-warning-system commented May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rniwa May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 18, 2024 • edited

Choose a reason for hiding this comment

webkit-early-warning-system commented May 20, 2024 • edited

webkit-commit-queue commented May 21, 2024

abigailfox commented May 16, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 16, 2024 •

edited

webkit-early-warning-system commented May 17, 2024 •

edited

rniwa May 17, 2024 •

edited

webkit-early-warning-system commented May 18, 2024 •

edited

webkit-early-warning-system commented May 20, 2024 •

edited