...
Created attachment 431371 [details] Patch
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).
(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.
(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.
Created attachment 431380 [details] Patch
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.
(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 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.
(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.
It is a little tricky with DELEGATE_REF_COUNTING_TO_COCOA, because ObjC needs to know how to ref and deref it.
(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 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.
Created attachment 431571 [details] Patch
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.
(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)
Created attachment 431578 [details] Patch
Created attachment 431579 [details] Patch
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.
r279074