Bug 217034 - [GTK] Stop using the default website data store for the inspector
Summary: [GTK] Stop using the default website data store for the inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2020-09-27 07:27 PDT by Carlos Garcia Campos
Modified: 2020-09-29 08:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.62 KB, patch)
2020-09-27 07:30 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-09-27 07:27:24 PDT
GTK port should never use the default data store.
Comment 1 Carlos Garcia Campos 2020-09-27 07:30:58 PDT
Created attachment 409829 [details]
Patch
Comment 2 Michael Catanzaro 2020-09-27 09:45:08 PDT
Comment on attachment 409829 [details]
Patch

Questions:

 *Each inspector is 1:1 associated with a web view. Why doesn't the inspector use that view's data store?
 * Why is it better to create a custom data store using default paths than to just use the default data store?
 * Why does GTK need this but not WPE?
 * Don't applications use the default data store *by default*? What's wrong with this?
 * If GTK port should never use the default data store, shouldn't we have an ASSERT to ensure it doesn't get created?
Comment 3 Carlos Garcia Campos 2020-09-27 22:43:52 PDT
(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 409829 [details]
> Patch
> 
> Questions:
> 
>  *Each inspector is 1:1 associated with a web view. Why doesn't the
> inspector use that view's data store?

The inspector doesn't use the same process pool as the web view, so it uses its own network process. If the web view is ephemeral we still want to save inspector settings. We have always saved the inspector settings globally, and not per application.

>  * Why is it better to create a custom data store using default paths than
> to just use the default data store?

Using the default data store is problematic for GLib API, we end up writing stuff globally, instead of per application, which is not desired in most of the cases. We can't probably change the default locations of website data store, for backwards compatibility reasons, note that these paths are versioned too, not the same as the default ones. After this patch users will lose the inspector settings.

>  * Why does GTK need this but not WPE?

WPE doesn't have WebInspectorProxy, because only the remote inspector is supported in WPE.

>  * Don't applications use the default data store *by default*? What's wrong
> with this?

No, applications use a WebKitWebsiteDataManager that creates a website data store with the default session ID, which is different. The GLib API doesn't use the default data store, but there are places like the inspector where it's created internally.

>  * If GTK port should never use the default data store, shouldn't we have an
> ASSERT to ensure it doesn't get created?

That's the plan, but I need to fix the cases where it's used first. I caught this thanks to the assert I added in patch atached to bug #216041
Comment 4 Michael Catanzaro 2020-09-28 06:51:03 PDT
OK, makes sense, especially for saving ephemeral settings. Except:

(In reply to Carlos Garcia Campos from comment #3)
> We have always saved the inspector settings globally, and not per application.

Problem is this makes it hard to audit that Epiphany/WebKit isn't leaking data into the default data dirs. Currently, as long as you don't open yelp, ~/.var/app/org.gnome.Epiphany/cache/webkitgtk should never exist. But with this change, you have to never open yelp *or* the web inspector, which is harder. ;) What do you think about using a different global location, like "webkitgtk-inspector" instead of the default location?
Comment 5 Carlos Garcia Campos 2020-09-28 07:14:00 PDT
(In reply to Michael Catanzaro from comment #4)
> OK, makes sense, especially for saving ephemeral settings. Except:
> 
> (In reply to Carlos Garcia Campos from comment #3)
> > We have always saved the inspector settings globally, and not per application.
> 
> Problem is this makes it hard to audit that Epiphany/WebKit isn't leaking
> data into the default data dirs. Currently, as long as you don't open yelp,
> ~/.var/app/org.gnome.Epiphany/cache/webkitgtk should never exist. But with
> this change, you have to never open yelp *or* the web inspector, which is
> harder. ;) What do you think about using a different global location, like
> "webkitgtk-inspector" instead of the default location?

It's not the default one, this one is versioned, so the directory is not webkitgtk, but webkitgtk-4.0.
Comment 6 Michael Catanzaro 2020-09-28 07:25:09 PDT
Versioning is good. I approve of versioning. But having webkitgtk-4.0 create both a versioned and an unversioned directory seems pretty confusing, right? One would be led to believe that the webkitgtk directory is used by older versions of WebKit prior to the 4.0 API.
Comment 7 Carlos Garcia Campos 2020-09-28 23:51:33 PDT
(In reply to Michael Catanzaro from comment #6)
> Versioning is good. I approve of versioning. But having webkitgtk-4.0 create
> both a versioned and an unversioned directory seems pretty confusing, right?
> One would be led to believe that the webkitgtk directory is used by older
> versions of WebKit prior to the 4.0 API.

The unversioned directory shouldn't be used anymore, it was a mistake. For next API bump I plan to use default base cache and data dirs to ensure there isn't global directory by default.
Comment 8 EWS 2020-09-29 00:20:18 PDT
Committed r267729: <https://trac.webkit.org/changeset/267729>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409829 [details].
Comment 9 Michael Catanzaro 2020-09-29 08:59:47 PDT
(In reply to Carlos Garcia Campos from comment #7)
> The unversioned directory shouldn't be used anymore, it was a mistake. For
> next API bump I plan to use default base cache and data dirs to ensure there
> isn't global directory by default.

Good idea. Even better if default dirs are based on prgname. It would be a good idea to change this now under #if USE(GTK4) conditions.