217034 2020-09-27 07:27:24 -0700 [GTK] Stop using the default website data store for the inspector 2020-09-29 08:59:47 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED Gtk P2 Normal --- 1 cgarcia webkit-unassigned bugs-noreply ews-watchlist hi joepeck mcatanzaro oldest_to_newest 1692351 0 cgarcia 2020-09-27 07:27:24 -0700 GTK port should never use the default data store. 1692352 1 409829 cgarcia 2020-09-27 07:30:58 -0700 Created attachment 409829 Patch 1692398 2 409829 mcatanzaro 2020-09-27 09:45:08 -0700 Comment on attachment 409829 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? 1692483 3 cgarcia 2020-09-27 22:43:52 -0700 (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 1692531 4 mcatanzaro 2020-09-28 06:51:03 -0700 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? 1692535 5 cgarcia 2020-09-28 07:14:00 -0700 (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. 1692536 6 mcatanzaro 2020-09-28 07:25:09 -0700 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. 1692842 7 cgarcia 2020-09-28 23:51:33 -0700 (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. 1692843 8 ews-feeder 2020-09-29 00:20:18 -0700 Committed r267729: <https://trac.webkit.org/changeset/267729> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409829. 1692928 9 mcatanzaro 2020-09-29 08:59:47 -0700 (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. 409829 2020-09-27 07:30:58 -0700 2020-09-29 00:20:18 -0700 Patch wk2-inspector-data-store.diff text/plain 4734 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggZDU1NWQ4MGNiZmU1Li42MzdkOGNmMmE5ZDcgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMg KzEsMTcgQEAKKzIwMjAtMDktMjcgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2Fs aWEuY29tPgorCisgICAgICAgIFtHVEtdIFN0b3AgdXNpbmcgdGhlIGRlZmF1bHQgd2Vic2l0ZSBk YXRhIHN0b3JlIGZvciB0aGUgaW5zcGVjdG9yCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0yMTcwMzQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGUgR1RLIHNob3VsZCBuZXZlciB1c2UgdGhlIGRlZmF1bHQg d2Vic2l0ZSBkYXRhIHN0b3JlLiBVc2UgYSBkZWRpY2F0ZWQgd2Vic2l0ZSBkYXRhIHN0b3JlIGZv ciB0aGUgaW5zcGVjdG9yCisgICAgICAgIHdpdGggaXRzIG93biBjb25maWd1cmF0aW9uIHRvIHN0 b3JlIHBlcnNpc3RlbnQgZGF0YS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9JbnNwZWN0b3IvZ3Rr L1dlYkluc3BlY3RvclByb3h5R3RrLmNwcDoKKyAgICAgICAgKFdlYktpdDo6aW5zcGVjdG9yV2Vi c2l0ZURhdGFTdG9yZSk6CisgICAgICAgIChXZWJLaXQ6OldlYkluc3BlY3RvclByb3h5OjpwbGF0 Zm9ybUNyZWF0ZUZyb250ZW5kUGFnZSk6CisKIDIwMjAtMDktMjcgIENhcmxvcyBHYXJjaWEgQ2Ft cG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtTT1VQXSBXZWJTb2NrZXQ6IGNv b2tpZXMgc2V0IGluIHJlcXVlc3QgZG9uJ3QgYXBwZWFyIGluIHRoZSBpbnNwZWN0b3IKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0luc3BlY3Rvci9ndGsvV2ViSW5zcGVjdG9y UHJveHlHdGsuY3BwIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvSW5zcGVjdG9yL2d0ay9XZWJJ bnNwZWN0b3JQcm94eUd0ay5jcHAKaW5kZXggZjQxY2E0NGViMmQ2Li43MmRlYjBjNWE2MmMgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0luc3BlY3Rvci9ndGsvV2ViSW5zcGVj dG9yUHJveHlHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0luc3BlY3Rvci9n dGsvV2ViSW5zcGVjdG9yUHJveHlHdGsuY3BwCkBAIC00Miw2ICs0Miw3IEBACiAjaW5jbHVkZSAi V2ViUGFnZUdyb3VwLmgiCiAjaW5jbHVkZSAiV2ViUHJvY2Vzc1Bvb2wuaCIKICNpbmNsdWRlICJX ZWJQcm9jZXNzUHJveHkuaCIKKyNpbmNsdWRlICJXZWJzaXRlRGF0YVN0b3JlLmgiCiAjaW5jbHVk ZSA8V2ViQ29yZS9DZXJ0aWZpY2F0ZUluZm8uaD4KICNpbmNsdWRlIDxXZWJDb3JlL0d0a1V0aWxp dGllcy5oPgogI2luY2x1ZGUgPFdlYkNvcmUvR3RrVmVyc2lvbmluZy5oPgpAQCAtMTM3LDYgKzEz OCwyNiBAQCBzdGF0aWMgdm9pZCBnZXRDb250ZXh0TWVudUZyb21Qcm9wb3NlZE1lbnUoV0tQYWdl UmVmIHBhZ2VSZWYsIFdLQXJyYXlSZWYgcHJvcG9zZQogICAgICpuZXdNZW51UmVmID0gbWVudUl0 ZW1zOwogfQogCitzdGF0aWMgUmVmPFdlYnNpdGVEYXRhU3RvcmU+IGluc3BlY3RvcldlYnNpdGVE YXRhU3RvcmUoKQoreworICAgIHN0YXRpYyBjb25zdCBjaGFyKiB2ZXJzaW9uZWREaXJlY3Rvcnkg PSAid2Via2l0Z3RrLSIgV0VCS0lUR1RLX0FQSV9WRVJTSU9OX1NUUklORyBHX0RJUl9TRVBBUkFU T1JfUyAiV2ViSW5zcGVjdG9yIiBHX0RJUl9TRVBBUkFUT1JfUzsKKyAgICBTdHJpbmcgYmFzZUNh Y2hlRGlyZWN0b3J5ID0gRmlsZVN5c3RlbTo6cGF0aEJ5QXBwZW5kaW5nQ29tcG9uZW50KEZpbGVT eXN0ZW06OnN0cmluZ0Zyb21GaWxlU3lzdGVtUmVwcmVzZW50YXRpb24oZ19nZXRfdXNlcl9jYWNo ZV9kaXIoKSksIHZlcnNpb25lZERpcmVjdG9yeSk7CisgICAgU3RyaW5nIGJhc2VEYXRhRGlyZWN0 b3J5ID0gRmlsZVN5c3RlbTo6cGF0aEJ5QXBwZW5kaW5nQ29tcG9uZW50KEZpbGVTeXN0ZW06OnN0 cmluZ0Zyb21GaWxlU3lzdGVtUmVwcmVzZW50YXRpb24oZ19nZXRfdXNlcl9kYXRhX2RpcigpKSwg dmVyc2lvbmVkRGlyZWN0b3J5KTsKKworICAgIGF1dG8gY29uZmlndXJhdGlvbiA9IFdlYnNpdGVE YXRhU3RvcmVDb25maWd1cmF0aW9uOjpjcmVhdGUoSXNQZXJzaXN0ZW50OjpZZXMsIFdpbGxDb3B5 UGF0aHNGcm9tRXhpc3RpbmdDb25maWd1cmF0aW9uOjpZZXMpOworICAgIGNvbmZpZ3VyYXRpb24t PnNldE5ldHdvcmtDYWNoZURpcmVjdG9yeShGaWxlU3lzdGVtOjpwYXRoQnlBcHBlbmRpbmdDb21w b25lbnQoYmFzZUNhY2hlRGlyZWN0b3J5LCAiV2ViS2l0Q2FjaGUiKSk7CisgICAgY29uZmlndXJh dGlvbi0+c2V0QXBwbGljYXRpb25DYWNoZURpcmVjdG9yeShGaWxlU3lzdGVtOjpwYXRoQnlBcHBl bmRpbmdDb21wb25lbnQoYmFzZUNhY2hlRGlyZWN0b3J5LCAiYXBwbGljYXRpb25zIikpOworICAg IGNvbmZpZ3VyYXRpb24tPnNldEhTVFNTdG9yYWdlRGlyZWN0b3J5KFN0cmluZyhiYXNlQ2FjaGVE aXJlY3RvcnkpKTsKKyAgICBjb25maWd1cmF0aW9uLT5zZXRDYWNoZVN0b3JhZ2VEaXJlY3Rvcnko RmlsZVN5c3RlbTo6cGF0aEJ5QXBwZW5kaW5nQ29tcG9uZW50KGJhc2VDYWNoZURpcmVjdG9yeSwg IkNhY2hlU3RvcmFnZSIpKTsKKyAgICBjb25maWd1cmF0aW9uLT5zZXRMb2NhbFN0b3JhZ2VEaXJl Y3RvcnkoRmlsZVN5c3RlbTo6cGF0aEJ5QXBwZW5kaW5nQ29tcG9uZW50KGJhc2VEYXRhRGlyZWN0 b3J5LCAibG9jYWxzdG9yYWdlIikpOworICAgIGNvbmZpZ3VyYXRpb24tPnNldEluZGV4ZWREQkRh dGFiYXNlRGlyZWN0b3J5KEZpbGVTeXN0ZW06OnBhdGhCeUFwcGVuZGluZ0NvbXBvbmVudChiYXNl RGF0YURpcmVjdG9yeSwgImluZGV4ZWRkYiIpKTsKKyAgICBjb25maWd1cmF0aW9uLT5zZXRXZWJT UUxEYXRhYmFzZURpcmVjdG9yeShGaWxlU3lzdGVtOjpwYXRoQnlBcHBlbmRpbmdDb21wb25lbnQo YmFzZURhdGFEaXJlY3RvcnksICJkYXRhYmFzZXMiKSk7CisgICAgY29uZmlndXJhdGlvbi0+c2V0 UmVzb3VyY2VMb2FkU3RhdGlzdGljc0RpcmVjdG9yeShGaWxlU3lzdGVtOjpwYXRoQnlBcHBlbmRp bmdDb21wb25lbnQoYmFzZURhdGFEaXJlY3RvcnksICJpdHAiKSk7CisgICAgY29uZmlndXJhdGlv bi0+c2V0U2VydmljZVdvcmtlclJlZ2lzdHJhdGlvbkRpcmVjdG9yeShGaWxlU3lzdGVtOjpwYXRo QnlBcHBlbmRpbmdDb21wb25lbnQoYmFzZURhdGFEaXJlY3RvcnksICJzZXJ2aWNld29ya2VycyIp KTsKKyAgICBjb25maWd1cmF0aW9uLT5zZXREZXZpY2VJZEhhc2hTYWx0c1N0b3JhZ2VEaXJlY3Rv cnkoRmlsZVN5c3RlbTo6cGF0aEJ5QXBwZW5kaW5nQ29tcG9uZW50KGJhc2VEYXRhRGlyZWN0b3J5 LCAiZGV2aWNlaWRoYXNoc2FsdHMiKSk7CisgICAgcmV0dXJuIFdlYnNpdGVEYXRhU3RvcmU6OmNy ZWF0ZShXVEZNb3ZlKGNvbmZpZ3VyYXRpb24pLCBQQUw6OlNlc3Npb25JRDo6ZGVmYXVsdFNlc3Np b25JRCgpKTsKK30KKwogV2ViUGFnZVByb3h5KiBXZWJJbnNwZWN0b3JQcm94eTo6cGxhdGZvcm1D cmVhdGVGcm9udGVuZFBhZ2UoKQogewogICAgIEFTU0VSVChpbnNwZWN0ZWRQYWdlKCkpOwpAQCAt MTUyLDExICsxNzMsMTUgQEAgV2ViUGFnZVByb3h5KiBXZWJJbnNwZWN0b3JQcm94eTo6cGxhdGZv cm1DcmVhdGVGcm9udGVuZFBhZ2UoKQogICAgIHByZWZlcmVuY2VzLT5zZXRKYXZhU2NyaXB0UnVu dGltZUZsYWdzKHsKICAgICB9KTsKICAgICBhdXRvIHBhZ2VHcm91cCA9IFdlYlBhZ2VHcm91cDo6 Y3JlYXRlKGluc3BlY3RvclBhZ2VHcm91cElkZW50aWZpZXJGb3JQYWdlKGluc3BlY3RlZFBhZ2Uo KSkpOworICAgIGF1dG8gd2Vic2l0ZURhdGFTdG9yZSA9IGluc3BlY3RvcldlYnNpdGVEYXRhU3Rv cmUoKTsKKyAgICBhdXRvJiBwcm9jZXNzUG9vbCA9IGluc3BlY3RvclByb2Nlc3NQb29sKGluc3Bl Y3Rpb25MZXZlbCgpKTsKKyAgICBwcm9jZXNzUG9vbC5zZXRQcmltYXJ5RGF0YVN0b3JlKHdlYnNp dGVEYXRhU3RvcmUuZ2V0KCkpOwogCiAgICAgYXV0byBwYWdlQ29uZmlndXJhdGlvbiA9IEFQSTo6 UGFnZUNvbmZpZ3VyYXRpb246OmNyZWF0ZSgpOwotICAgIHBhZ2VDb25maWd1cmF0aW9uLT5zZXRQ cm9jZXNzUG9vbCgmaW5zcGVjdG9yUHJvY2Vzc1Bvb2woaW5zcGVjdGlvbkxldmVsKCkpKTsKKyAg ICBwYWdlQ29uZmlndXJhdGlvbi0+c2V0UHJvY2Vzc1Bvb2woJnByb2Nlc3NQb29sKTsKICAgICBw YWdlQ29uZmlndXJhdGlvbi0+c2V0UHJlZmVyZW5jZXMocHJlZmVyZW5jZXMucHRyKCkpOwogICAg IHBhZ2VDb25maWd1cmF0aW9uLT5zZXRQYWdlR3JvdXAocGFnZUdyb3VwLnB0cigpKTsKKyAgICBw YWdlQ29uZmlndXJhdGlvbi0+c2V0V2Vic2l0ZURhdGFTdG9yZSh3ZWJzaXRlRGF0YVN0b3JlLnB0 cigpKTsKICAgICBtX2luc3BlY3RvclZpZXcgPSBHVEtfV0lER0VUKHdlYmtpdFdlYlZpZXdCYXNl Q3JlYXRlKCpwYWdlQ29uZmlndXJhdGlvbi5wdHIoKSkpOwogICAgIGdfb2JqZWN0X2FkZF93ZWFr X3BvaW50ZXIoR19PQkpFQ1QobV9pbnNwZWN0b3JWaWV3KSwgcmVpbnRlcnByZXRfY2FzdDx2b2lk Kio+KCZtX2luc3BlY3RvclZpZXcpKTsKICAgICBnX3NpZ25hbF9jb25uZWN0KG1faW5zcGVjdG9y VmlldywgImRlc3Ryb3kiLCBHX0NBTExCQUNLKGluc3BlY3RvclZpZXdEZXN0cm95ZWQpLCB0aGlz KTsK