REGRESSION(r294381): [ Debug ] TestWebKitAPI.WebKit.CookieObserverCrash is a constant crash

[szewai]

3ba1f41

REGRESSION(r294381): [ Debug ] TestWebKitAPI.WebKit.CookieObserverCrash is a constant crash
https://bugs.webkit.org/show_bug.cgi?id=240595

Reviewed by Alex Christensen.

The crash is an assertion failure. The assertion ensures that WebCookieManagerProxy has no cookies observer when it gets
destroyed. The assertion was valid when it was added, since WKHTTPCookieStore does not outlive WebKit::WebsiteDataStore
(r219550), and observers will be removed when WKHTTPCookieStore is deallocated. Since r279074, WKHTTPCookieStore can
outlive WebKit::WebsiteDataStore -- it holds a weak reference to WebKit::WebsiteDataStore instead of a strong reference,
so the assertion does not hold.

* Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:
(WebKit::WebCookieManagerProxy::~WebCookieManagerProxy):

Canonical link: https://commits.webkit.org/250992@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294860 268f45cc-cd09-0410-ab3c-d52691b4dbfc

[cdumez]

Doesn't this mean that the API client may still have a registered cookie observer and yet no longer receive any notifications because the WebCookieManagerProxy was destroyed? Wouldn't this be a bug?

Seems like if the clients is monitoring cookies, they should be getting updates until they unregister their observer.

[szewai]

Yes, just like the failed test. If client has a registered cookie observer but WebsiteDataStore is destroyed, the observer will not get any notification. It looks reasonable to me that client does not get notification about cookies of an ephemeral session if the session is destroyed? (The ref chains are: WKWebsiteDataStore => WebsiteDataStore => NetworkProcessProxy => WebCookieManagerProxy and WKHttpCookieStore => API::HTTPCookieStore -> WebCookieManagerProxy).
I am not sure how this API is supposed to work originally, at least according to current implementation, the assertion seems to not hold.

[cdumez]

I'll defer Brady/Alex here. This seems like suspicious API behavior to me and a regression from your recent refactoring since the assertion wasn't hitting before. Fixing the assertion hit by removing the assertion is always suspicious too.

[szewai]

I'll defer Brady/Alex here. This seems like suspicious API behavior to me and a regression from your recent refactoring since the assertion wasn't hitting before. Fixing the assertion hit by removing the assertion is always suspicious too.

The assertion wasn't hit because WebsiteDataStore was held alive by WebProcessProxy, so WebsiteDataStore is not destroyed as expected during the test.

[cdumez]

@beidson Added this test here:
Crash when a WKHTTPCookieStore outlives its owning WKWebsiteDataStore.
rdar://problem/33341730 and https://bugs.webkit.org/show_bug.cgi?id=174574

He intentionally made sure that the APIHTTPCookieStore would keep its data store alive.

[szewai]

@beidson Added this test here: Crash when a WKHTTPCookieStore outlives its owning WKWebsiteDataStore. rdar://problem/33341730 and https://bugs.webkit.org/show_bug.cgi?id=174574

He intentionally made sure that the APIHTTPCookieStore would keep its data store alive.

Yes, as mentioned in the commit message: this statement that APIHTTPCookieStore would keep its data store alive is no longer valid after https://trac.webkit.org/changeset/279074/webkit (from @achristensen07, reviewed by @cdumez ).
And the test passed because it doesn't execute what it wants to test (at least right before my change): WebsiteDataStore gets destroyed.

[achristensen07]

This is fine. We should really just completely remove WebCookieManagerProxy.

[webkit-early-warning-system]

Committed r294860 (250992@main): https://commits.webkit.org/250992@main

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