Bug 168230 – [GTK] Update cookie manager API to properly work with ephemeral sessions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

WebKitCookieManager is associated to a particular WebKitWebContexst and uses the default session ID unconditionally.

[Carlos Garcia Campos]

Created attachment 301345 [details]
Patch

[Michael Catanzaro]

Comment on attachment 301345 [details]
Patch

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

It looks mostly good, but I'd like to review this one again after you answer my questions about why you used the weak pointer, which I hope was just a mistake.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:217
> +    // FIXME: Add support for deleting cookies modified since the given timestamp. It should probably be added to libsoup.
> +    if (timestamp == std::chrono::system_clock::from_time_t(0))
> +        deleteAllCookies(session);

Are you planning to fix this soon? I don't think we should proceed with these API changes if the API does not work properly. At least add a runtime warning using g_warning().

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:40
> + * The WebKitCookieManager defines how to setup and handle cookies.

setup (noun) -> set up (verb)

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:43
>   * store cookies, with webkit_cookie_manager_set_persistent_storage(),

Remove the comma

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:144
> +    g_object_add_weak_pointer(G_OBJECT(dataManager), reinterpret_cast<void**>(&manager->priv->dataManager));

Why is the weak pointer needed? I would expect WebKitCookieManager to be owned by the WebKitWebsiteDataManager, so WebKitWebsiteDataManager would keep a ref to ensure the WebKitCookieManager is never destroyed first. Is there some really good reason you don't do that? I think it should be a normal raw pointer instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:165
>   * By default, @cookie_manager doesn't store the cookies persistenly, so you need to call this

Preexisting problem, but let's fix it now: persistenly -> persistently

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:176
> +    for (auto* processPool : webkitWebsiteDataManagerGetProcessPools(manager->priv->dataManager)) {

This is a big problem. Either manager->priv->dataManager can be legitimately null here, or you should not have used a weak pointer.

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:197
> +    g_return_if_fail(WEBKIT_IS_WEBSITE_DATA_MANAGER(manager->priv->dataManager));

Ditto. If you're going to use the weak pointer, then the entire class needs to be prepared to deal with the WebKitWebsiteDataManager being null. There's no point to the weak pointer otherwise.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:82
>   * webkit_web_context_get_security_manager() for that.

Remove "for that": the sentence works better without them.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:698
>  WebKitCookieManager* webkit_web_context_get_cookie_manager(WebKitWebContext* context)

I think you should document that this function returns the WebKitCookieManager of this context's WebKitWebsiteDataManager. I know it's an implementation detail, but otherwise I think it's kind of confusing why both WebKitWebContext and WebKitWebsiteDataManager have a WebKitCookieManager.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2286
> + * Get the #WebKitWebsiteDataManager associated to @web_view. If @web_view is not ephemeral

Add a comma after "ephemeral"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2287
> + * the returned #WebKitWebsiteDataManager will be the same as the #WebKitWebView:web-context one.

"the #WebKitWebView:web-context one" doesn't sound great. I would write: "will be the same as the #WebKitWebsiteDataManager of @web_view's #WebKitWebContext."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:103
> +    GRefPtr<WebKitCookieManager> cookieManager;

Aha, so you do keep a ref! Good! Then you don't need the weak pointer in WebKitCookieManager, right? Surely the WebKitCookieManager cannot outlive the WebKitWebsiteDataManager due to this ref.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:104
> +    Vector<WebProcessPool*> processPools;

Is it possible to use Vector<WebProcessPool&> to clarify that the process pools are not null?

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataRecord.cpp:54
> +#if PLATFORM(GTK)
> +    if (hostName == "localhost")
> +        return hostName;
> +#endif

I think this should be an #else condition, not a GTK-specific condition.

[Michael Catanzaro]

Comment on attachment 301345 [details]
Patch

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

>> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:217
>> +        deleteAllCookies(session);
> 
> Are you planning to fix this soon? I don't think we should proceed with these API changes if the API does not work properly. At least add a runtime warning using g_warning().

(Sad OK if you add the g_warning().)

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:115
> +    if (manager->priv->dataManager) {
> +        g_object_remove_weak_pointer(G_OBJECT(manager->priv->dataManager), reinterpret_cast<void**>(&manager->priv->dataManager));
> +
> +        auto sessionID = webkitWebsiteDataManagerGetDataStore(manager->priv->dataManager).websiteDataStore().sessionID();
> +        for (auto* processPool : webkitWebsiteDataManagerGetProcessPools(manager->priv->dataManager))
> +            processPool->supplement<WebCookieManagerProxy>()->stopObservingCookieChanges(sessionID);
> +
> +        manager->priv->dataManager = nullptr;
> +    }

Actually, I wonder if we should do this in ~_WebKitCookieManagerPrivate() instead of dispose. Because the object should technically be left in valid state after dispose, but all of the functions are going to hit criticals if priv->dataManager is null. Since we're not keeping a strong reference to it, it can't be involved in a reference cycle, so it should be safer to do in the destructor instead of dispose.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 301345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301345&action=review
> 
> It looks mostly good, but I'd like to review this one again after you answer
> my questions about why you used the weak pointer, which I hope was just a
> mistake.
> 
> > Source/WebCore/platform/network/soup/CookieJarSoup.cpp:217
> > +    // FIXME: Add support for deleting cookies modified since the given timestamp. It should probably be added to libsoup.
> > +    if (timestamp == std::chrono::system_clock::from_time_t(0))
> > +        deleteAllCookies(session);
> 
> Are you planning to fix this soon? I don't think we should proceed with
> these API changes if the API does not work properly. At least add a runtime
> warning using g_warning().

No, I don't have time, I'll add the warning.

> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:40
> > + * The WebKitCookieManager defines how to setup and handle cookies.
> 
> setup (noun) -> set up (verb)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:43
> >   * store cookies, with webkit_cookie_manager_set_persistent_storage(),
> 
> Remove the comma
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:144
> > +    g_object_add_weak_pointer(G_OBJECT(dataManager), reinterpret_cast<void**>(&manager->priv->dataManager));
> 
> Why is the weak pointer needed? I would expect WebKitCookieManager to be
> owned by the WebKitWebsiteDataManager, so WebKitWebsiteDataManager would
> keep a ref to ensure the WebKitCookieManager is never destroyed first. Is
> there some really good reason you don't do that? I think it should be a
> normal raw pointer instead.

The cookie manager can't be created by the user, it's always created and owned by the website data manager. However, nothing prevents the user from taking a ref to the returned cookie manager. It's a programmer error to use the cookie manager after the data manager is destroyed, that's why all methods are protected with g_return_if_fail(WEBKIT_IS_WEBSITE_DATA_MANAGER(manager->priv->dataManager)). I don't want to take a reference to the data manager. Maybe it isn't worth it just to handle a corner case, so I can simply remove the weak point and use a plan pointer. Users will get a crash sooner or layer in both cases I'm afraid.

> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:165
> >   * By default, @cookie_manager doesn't store the cookies persistenly, so you need to call this
> 
> Preexisting problem, but let's fix it now: persistenly -> persistently
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:176
> > +    for (auto* processPool : webkitWebsiteDataManagerGetProcessPools(manager->priv->dataManager)) {
> 
> This is a big problem. Either manager->priv->dataManager can be legitimately
> null here, or you should not have used a weak pointer.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:197
> > +    g_return_if_fail(WEBKIT_IS_WEBSITE_DATA_MANAGER(manager->priv->dataManager));
> 
> Ditto. If you're going to use the weak pointer, then the entire class needs
> to be prepared to deal with the WebKitWebsiteDataManager being null. There's
> no point to the weak pointer otherwise.

It deals with that already, but using g_return macros.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:82
> >   * webkit_web_context_get_security_manager() for that.
> 
> Remove "for that": the sentence works better without them.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:698
> >  WebKitCookieManager* webkit_web_context_get_cookie_manager(WebKitWebContext* context)
> 
> I think you should document that this function returns the
> WebKitCookieManager of this context's WebKitWebsiteDataManager. I know it's
> an implementation detail, but otherwise I think it's kind of confusing why
> both WebKitWebContext and WebKitWebsiteDataManager have a
> WebKitCookieManager.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2286
> > + * Get the #WebKitWebsiteDataManager associated to @web_view. If @web_view is not ephemeral
> 
> Add a comma after "ephemeral"
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2287
> > + * the returned #WebKitWebsiteDataManager will be the same as the #WebKitWebView:web-context one.
> 
> "the #WebKitWebView:web-context one" doesn't sound great. I would write:
> "will be the same as the #WebKitWebsiteDataManager of @web_view's
> #WebKitWebContext."
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:103
> > +    GRefPtr<WebKitCookieManager> cookieManager;
> 
> Aha, so you do keep a ref! Good! Then you don't need the weak pointer in
> WebKitCookieManager, right? Surely the WebKitCookieManager cannot outlive
> the WebKitWebsiteDataManager due to this ref.

Not surely, but I guess we should assume it, yes.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:104
> > +    Vector<WebProcessPool*> processPools;
> 
> Is it possible to use Vector<WebProcessPool&> to clarify that the process
> pools are not null?

No, I don't think Vector can contain references.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataRecord.cpp:54
> > +#if PLATFORM(GTK)
> > +    if (hostName == "localhost")
> > +        return hostName;
> > +#endif
> 
> I think this should be an #else condition, not a GTK-specific condition.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 301345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301345&action=review
> 
> >> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:217
> >> +        deleteAllCookies(session);
> > 
> > Are you planning to fix this soon? I don't think we should proceed with these API changes if the API does not work properly. At least add a runtime warning using g_warning().
> 
> (Sad OK if you add the g_warning().)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:115
> > +    if (manager->priv->dataManager) {
> > +        g_object_remove_weak_pointer(G_OBJECT(manager->priv->dataManager), reinterpret_cast<void**>(&manager->priv->dataManager));
> > +
> > +        auto sessionID = webkitWebsiteDataManagerGetDataStore(manager->priv->dataManager).websiteDataStore().sessionID();
> > +        for (auto* processPool : webkitWebsiteDataManagerGetProcessPools(manager->priv->dataManager))
> > +            processPool->supplement<WebCookieManagerProxy>()->stopObservingCookieChanges(sessionID);
> > +
> > +        manager->priv->dataManager = nullptr;
> > +    }
> 
> Actually, I wonder if we should do this in ~_WebKitCookieManagerPrivate()
> instead of dispose. Because the object should technically be left in valid
> state after dispose, but all of the functions are going to hit criticals if
> priv->dataManager is null. Since we're not keeping a strong reference to it,
> it can't be involved in a reference cycle, so it should be safer to do in
> the destructor instead of dispose.

I want to stop observing cookie changes asap, and I need to nullify the pointer to ensure it doesn't happen twice.

[Carlos Garcia Campos]

Created attachment 301483 [details]
Patch

[Michael Catanzaro]

Comment on attachment 301483 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        WebKitCookieManager is associated to a particular WebKitWebContexst and uses the default session ID

WebKitWebContext

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataRecord.cpp:52
>  #if PLATFORM(COCOA)
>      if (hostName == String(kCFHTTPCookieLocalFileDomain))
>          return displayNameForLocalFiles();
> +#elif PLATFORM(GTK)
> +    if (hostName == "localhost")
> +        return hostName;
>  #endif

Why not use #else? All non-cocoa ports must want this.

[Michael Catanzaro]

I would put the weak pointer back, with a comment explaining why it's there. It's not great, but I don't have a better solution, and it seems safer than using a raw pointer. Believe it or not, I just didn't consider that the user could ref the SoupCookieManager.

[Carlos Garcia Campos]

Committed r212345: <http://trac.webkit.org/changeset/212345>