Bug 226992 – Break ref cycle between API::HTTPCookieStore and WebKit::WebsiteDataStore

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 431371 [details]
Patch

[Chris Dumez]

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).

[Sihui Liu]

(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.

[Chris Dumez]

(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.

[Sihui Liu]

Created attachment 431380 [details]
Patch

[Alex Christensen]

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.

[Sihui Liu]

(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!

[Sihui Liu]

(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!

[Sam Weinig]

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.

[Sam Weinig]

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.

[Sam Weinig]

(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.

[Alex Christensen]

It is a little tricky with DELEGATE_REF_COUNTING_TO_COCOA, because ObjC needs to know how to ref and deref it.

[Sam Weinig]

(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.

[Alex Christensen]

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.

[Sihui Liu]

Created attachment 431571 [details]
Patch

[Alex Christensen]

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.

[Sihui Liu]

(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)

[Alex Christensen]

Created attachment 431578 [details]
Patch

[Alex Christensen]

Created attachment 431579 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

r279074