Bug 226992 - Break ref cycle between API::HTTPCookieStore and WebKit::WebsiteDataStore
Summary: Break ref cycle between API::HTTPCookieStore and WebKit::WebsiteDataStore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-14 15:21 PDT by Sihui Liu
Modified: 2021-06-21 11:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.29 KB, patch)
2021-06-14 15:26 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (11.61 KB, patch)
2021-06-14 16:24 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (11.96 KB, patch)
2021-06-16 11:18 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (12.19 KB, patch)
2021-06-16 11:48 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.33 KB, patch)
2021-06-16 11:53 PDT, Alex Christensen
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2021-06-14 15:21:50 PDT
...
Comment 1 Sihui Liu 2021-06-14 15:26:58 PDT
Created attachment 431371 [details]
Patch
Comment 2 Chris Dumez 2021-06-14 15:37:03 PDT
Comment on attachment 431371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431371&action=review

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:161
> +    return WebKit::toAPI(&WebKit::toImpl(dataStoreRef)->cookieStore().leakRef());

It seems unexpected that a "Get" function would leak a ref, this seems wrong.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2060
> +        return makeRef(*m_cookieStore);

Why the makeRef() ?

> Tools/WebKitTestRunner/TestInvocation.cpp:152
> +    auto cookieStore = adoptWK(WKWebsiteDataStoreGetHTTPCookieStore(TestController::singleton().websiteDataStore()));

Clearly wrong to adopt the result of a "Get" function. There are clear patterns we need to follow and a "Get" function doesn't ref for you (unlike a "Create" or a "Copy" function).
Comment 3 Sihui Liu 2021-06-14 16:11:20 PDT
(In reply to Chris Dumez from comment #2)
> Comment on attachment 431371 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431371&action=review
> 
> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:161
> > +    return WebKit::toAPI(&WebKit::toImpl(dataStoreRef)->cookieStore().leakRef());
> 
> It seems unexpected that a "Get" function would leak a ref, this seems wrong.

We can rename it to create or getOrCreate. I am posting the patch as I am not sure if this is the right way to break the cycle.

> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2060
> > +        return makeRef(*m_cookieStore);
> 
> Why the makeRef() ?

Try to do a getOrCreate.

> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:152
> > +    auto cookieStore = adoptWK(WKWebsiteDataStoreGetHTTPCookieStore(TestController::singleton().websiteDataStore()));
> 
> Clearly wrong to adopt the result of a "Get" function. There are clear
> patterns we need to follow and a "Get" function doesn't ref for you (unlike
> a "Create" or a "Copy" function).

We can call it WKWebsiteDataStoreCreateHTTPCookieStore I guess. WTR seems to be the only place we use the function.
Comment 4 Chris Dumez 2021-06-14 16:14:02 PDT
(In reply to Sihui Liu from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Comment on attachment 431371 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=431371&action=review
> > 
> > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:161
> > > +    return WebKit::toAPI(&WebKit::toImpl(dataStoreRef)->cookieStore().leakRef());
> > 
> > It seems unexpected that a "Get" function would leak a ref, this seems wrong.
> 
> We can rename it to create or getOrCreate. I am posting the patch as I am
> not sure if this is the right way to break the cycle.
> 
> > 
> > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2060
> > > +        return makeRef(*m_cookieStore);
> > 
> > Why the makeRef() ?
> 
> Try to do a getOrCreate.

I don't see why the makeRef() is needed. AFAICT, you can just `return * m_cookieStore;` and it will ref for you.

> 
> > 
> > > Tools/WebKitTestRunner/TestInvocation.cpp:152
> > > +    auto cookieStore = adoptWK(WKWebsiteDataStoreGetHTTPCookieStore(TestController::singleton().websiteDataStore()));
> > 
> > Clearly wrong to adopt the result of a "Get" function. There are clear
> > patterns we need to follow and a "Get" function doesn't ref for you (unlike
> > a "Create" or a "Copy" function).
> 
> We can call it WKWebsiteDataStoreCreateHTTPCookieStore I guess. WTR seems to
> be the only place we use the function.
Comment 5 Sihui Liu 2021-06-14 16:24:11 PDT
Created attachment 431380 [details]
Patch
Comment 6 Alex Christensen 2021-06-14 16:25:55 PDT
For some reason I recall the solution to this being that the WebsiteDataStore should own a std::unique_ptr<API::HTTPCookieStore> and then API::HTTPCookieStore::ref should call its owning WebsiteDataStore::ref.  I don't remember why I thought that, though.
Comment 7 Sihui Liu 2021-06-14 16:42:45 PDT
(In reply to Alex Christensen from comment #6)
> For some reason I recall the solution to this being that the
> WebsiteDataStore should own a std::unique_ptr<API::HTTPCookieStore> and then
> API::HTTPCookieStore::ref should call its owning WebsiteDataStore::ref.  I
> don't remember why I thought that, though.

I thought about that, but API::HTTPCookieStore already has ref() from ObjectImpl<Object::Type::HTTPCookieStore>. (In reply to Alex Christensen from comment #6)
> For some reason I recall the solution to this being that the
> WebsiteDataStore should own a std::unique_ptr<API::HTTPCookieStore> and then
> API::HTTPCookieStore::ref should call its owning WebsiteDataStore::ref.  I
> don't remember why I thought that, though.

https://bugs.webkit.org/show_bug.cgi?id=194868!
Comment 8 Sihui Liu 2021-06-14 16:42:45 PDT Comment hidden (obsolete)
Comment 9 Sam Weinig 2021-06-14 17:20:26 PDT Comment hidden (obsolete)
Comment 10 Sam Weinig 2021-06-14 17:20:33 PDT
Comment on attachment 431380 [details]
Patch

I'm not sure if we can with the current API contracts in place, but could we just make them the same object? It looks like the only place that creates a HTTPCookieStore is WebsiteDataStore.
Comment 11 Sam Weinig 2021-06-14 17:22:00 PDT
(In reply to Sam Weinig from comment #10)
> Comment on attachment 431380 [details]
> Patch
> 
> I'm not sure if we can with the current API contracts in place, but could we
> just make them the same object? It looks like the only place that creates a
> HTTPCookieStore is WebsiteDataStore.

Actually, the unique_ptr version with delegating ref()/deref() Alex mentioned is probably better than this.
Comment 12 Alex Christensen 2021-06-15 16:42:34 PDT
It is a little tricky with DELEGATE_REF_COUNTING_TO_COCOA, because ObjC needs to know how to ref and deref it.
Comment 13 Sam Weinig 2021-06-16 09:52:09 PDT
(In reply to Alex Christensen from comment #12)
> It is a little tricky with DELEGATE_REF_COUNTING_TO_COCOA, because ObjC
> needs to know how to ref and deref it.

Ah. I think you could make it work by reffing once on creation and once on destruction (thought don't quote me on that) rather than on each ref/deref(), but he just make them the same underlying object also works still I think.
Comment 14 Alex Christensen 2021-06-16 10:28:38 PDT
Comment on attachment 431380 [details]
Patch

I think we need to make HTTPCookieStore::m_owningDataStore weak and keep WebsiteDataStore::m_cookieStore strong. We will need to add a bunch of checks in HTTPCookieStore, but that makes sense because if the WebsiteDataStore is destroyed, then there are no more cookies and nothing we can do with them.
Comment 15 Sihui Liu 2021-06-16 11:18:45 PDT
Created attachment 431571 [details]
Patch
Comment 16 Alex Christensen 2021-06-16 11:30:45 PDT
Comment on attachment 431571 [details]
Patch

I don't think this actually breaks the cycle, and it doesn't take into account that apps can call CFRetain/CFRelease directly on the WKHTTPCookieStore, which wouldn't do the right thing.  I'm going to try uploading another way to fix this.
Comment 17 Sihui Liu 2021-06-16 11:41:07 PDT
(In reply to Alex Christensen from comment #16)
> Comment on attachment 431571 [details]
> Patch
> 
> I don't think this actually breaks the cycle, and it doesn't take into
> account that apps can call CFRetain/CFRelease directly on the
> WKHTTPCookieStore, which wouldn't do the right thing.  I'm going to try
> uploading another way to fix this.

Can you explain more about why it doesn't break the cycle and how CFRetain/CFRelease would affect it? (I am cool with new approach and just curious)
Comment 18 Alex Christensen 2021-06-16 11:48:04 PDT
Created attachment 431578 [details]
Patch
Comment 19 Alex Christensen 2021-06-16 11:53:12 PDT
Created attachment 431579 [details]
Patch
Comment 20 Alex Christensen 2021-06-16 14:09:58 PDT
Object::ref calls CFRetain, which increments the ObjC object's ref count.  You made HTTPCookieStore::ref call WebsiteDataStore::ref, which affects the behavior of Ref/RefPtr but an app can call CFRetain on a WKHTTPCookieStore, but if API::HTTPCookieStore is owned by a std::unique_ptr that retain would not be counted.
Comment 21 Alex Christensen 2021-06-21 11:50:15 PDT
r279074