188843 2018-08-22 07:29:03 -0700 NetworkCache::Storage::lastStableVersion should be a developer-only feature 2018-08-23 00:00:17 -0700 1 1 1 Unclassified WebKit Page Loading WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 koivisto webkit-unassigned beidson cdumez cgarcia commit-queue dbates ews-watchlist ggaren mcatanzaro mitz oldest_to_newest 1452613 0 koivisto 2018-08-22 07:29:03 -0700 System WebKit should always delete old cache versions. 1452614 1 koivisto 2018-08-22 07:29:14 -0700 <rdar://problem/43574100> 1452625 2 347792 koivisto 2018-08-22 08:10:09 -0700 Created attachment 347792 patch 1452650 3 347792 mcatanzaro 2018-08-22 09:08:30 -0700 Comment on attachment 347792 patch View in context: https://bugs.webkit.org/attachment.cgi?id=347792&action=review > Source/WebKit/Shared/mac/ChildProcessMac.mm:743 > +#if PLATFORM(MAC) > +bool ChildProcess::isSystemWebKit() > +{ > + static bool isSystemWebKit = [] { > + return [[webKit2Bundle() bundlePath] hasPrefix:@"/System/"]; > + }(); > + return isSystemWebKit; > +} > +#endif You can implement this now for all platforms. On non-Cocoa ports, it's system WebKit #if !ENABLE(DEVELOPER_MODE). 1452652 4 mcatanzaro 2018-08-22 09:09:28 -0700 BTW it would be convenient if ENABLE(DEVELOPER_MODE) was defined on Cocoa ports as well. Currently that only works for CMake ports (so all non-Cocoa ports). 1452782 5 347792 dbates 2018-08-22 13:34:11 -0700 Comment on attachment 347792 patch View in context: https://bugs.webkit.org/attachment.cgi?id=347792&action=review > Source/WebKit/Shared/mac/ChildProcessMac.mm:740 > + static bool isSystemWebKit = [] { > + return [[webKit2Bundle() bundlePath] hasPrefix:@"/System/"]; > + }(); This will treat an installed development WebKit root or Xcode installed development WebKit as a system install. Is this OK? Otherwise, we should make use of the codesign framework API/SPI to actually check that the process is "restricted" and/or signed by Apple. 1452815 6 ggaren 2018-08-22 14:50:10 -0700 I think it's OK to treat installing roots as an "upgrade" event that clears old caches just like a real upgrade would. 1452819 7 ggaren 2018-08-22 14:55:14 -0700 > BTW it would be convenient if ENABLE(DEVELOPER_MODE) was defined on Cocoa > ports as well. Currently that only works for CMake ports (so all non-Cocoa > ports). I read some of the DEVELOPER_MODE code and I'm not sure I fully understand the concept -- or perhaps I disagree with it. In general, we want the code we test to be the same as the code customers will run. That way, we know that our testing is accurate. But DEVELOPER_MODE seems to be a step away from that. Maybe you should send email to webkit-dev explaining the thesis of DEVELOPER_MODE and some examples of when to use or not use it. Then we can discuss. 1452820 8 347792 ggaren 2018-08-22 14:55:38 -0700 Comment on attachment 347792 patch r=me 1452864 9 mcatanzaro 2018-08-22 15:51:55 -0700 Well the thesis is straightforward: sometimes you really do need to do things differently for developers. For instance, the title of this changelog: "NetworkCache::Storage::lastStableVersion should be a developer-only feature" Normally we use it for loading content from the source directory rather than the install prefix. E.g. in developer mode, hyphenation dictionaries, secondary processes, and the InjectedBundle are all loaded from the source directory instead of the install prefix. It would not be very good for developers if the UI process were to try launching the system WebKitWebProcess, for example. Anyway, you don't need to enable it for Cocoa here, but you should use it here for other ports, because this is exactly the sort of thing it's intended for. E.g.: bool ChildProcess::isSystemWebKit() { #if ENABLE(DEVELOPER_MODE) return false; #elif PLATFORM(MAC) static bool isSystemWebKit = [] { return [[webKit2Bundle() bundlePath] hasPrefix:@"/System/"]; }(); return isSystemWebKit; #else return true; #endif } and then you can remove the other PLATFORM(MAC) guards: #if PLATFORM(MAC) /// Allow the last stable version of the cache to co-exist with the latest development one. static const unsigned lastStableVersion = 12; #endif #if PLATFORM(MAC) if (directoryVersion == lastStableVersion) return; #endif since clearly there's nothing really platform-specific there. 1452995 10 koivisto 2018-08-22 23:42:22 -0700 > since clearly there's nothing really platform-specific there. The 'stable version' here refers to the version a developer likely has installed as system WebKit. That is platform specific. (the idea is to prevent newer builds of WebKit unnecessarily wiping out the caches for the system WebKit) 1453005 11 347792 commit-queue 2018-08-23 00:00:15 -0700 Comment on attachment 347792 patch Clearing flags on attachment: 347792 Committed r235220: <https://trac.webkit.org/changeset/235220> 1453006 12 commit-queue 2018-08-23 00:00:17 -0700 All reviewed patches have been landed. Closing bug. 347792 2018-08-22 08:10:09 -0700 2018-08-23 00:00:15 -0700 patch cache-old-version-system-3.patch text/plain 2941 koivisto SW5kZXg6IFNvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQvQ2hhbmdlTG9nCShyZXZpc2lvbiAyMzUxNzEpCisrKyBTb3VyY2UvV2ViS2l0L0NoYW5nZUxv Zwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDE4LTA4LTIyICBBbnR0aSBLb2l2 aXN0byAgPGFudHRpQGFwcGxlLmNvbT4KKworICAgICAgICBOZXR3b3JrQ2FjaGU6OlN0b3JhZ2U6 Omxhc3RTdGFibGVWZXJzaW9uIHNob3VsZCBiZSBhIGRldmVsb3Blci1vbmx5IGZlYXR1cmUKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE4ODg0MworICAg ICAgICA8cmRhcjovL3Byb2JsZW0vNDM1NzQxMDA+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgKiBOZXR3b3JrUHJvY2Vzcy9jYWNoZS9OZXR3b3JrQ2Fj aGVTdG9yYWdlLmNwcDoKKyAgICAgICAgKFdlYktpdDo6TmV0d29ya0NhY2hlOjpTdG9yYWdlOjpk ZWxldGVPbGRWZXJzaW9ucyk6CisKKyAgICAgICAgRGVsZXRlIG9sZCBjYWNoZSB2ZXJzaW9ucyB1 bmNvbmRpdGlvbmFsbHkgaWYgd2UgYXJlIHN5c3RlbSBXZWJLaXQuCisKKyAgICAgICAgKiBTaGFy ZWQvQ2hpbGRQcm9jZXNzLmg6CisgICAgICAgICogU2hhcmVkL21hYy9DaGlsZFByb2Nlc3NNYWMu bW06CisgICAgICAgIChXZWJLaXQ6OkNoaWxkUHJvY2Vzczo6aXNTeXN0ZW1XZWJLaXQpOgorCisg ICAgICAgIEZpbmQgb3V0IGlmIFdlYktpdCBpcyBpbnN0YWxsZWQgdW5kZXIgJy9TeXN0ZW0vJy4K KwogMjAxOC0wOC0yMiAgQW50dGkgS29pdmlzdG8gIDxhbnR0aUBhcHBsZS5jb20+CiAKICAgICAg ICAgVXNlIE9wdGlvblNldCBmb3IgTmV0d29ya0NhY2hlOjpTdG9yYWdlOjpUcmF2ZXJzZUZsYWdz CkluZGV4OiBTb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL2NhY2hlL05ldHdvcmtDYWNoZVN0 b3JhZ2UuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2Fj aGUvTmV0d29ya0NhY2hlU3RvcmFnZS5jcHAJKHJldmlzaW9uIDIzNTE3MCkKKysrIFNvdXJjZS9X ZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUvTmV0d29ya0NhY2hlU3RvcmFnZS5jcHAJKHdvcmtp bmcgY29weSkKQEAgLTI2LDYgKzI2LDcgQEAKICNpbmNsdWRlICJjb25maWcuaCIKICNpbmNsdWRl ICJOZXR3b3JrQ2FjaGVTdG9yYWdlLmgiCiAKKyNpbmNsdWRlICJDaGlsZFByb2Nlc3MuaCIKICNp bmNsdWRlICJMb2dnaW5nLmgiCiAjaW5jbHVkZSAiTmV0d29ya0NhY2hlQ29kZXJzLmgiCiAjaW5j bHVkZSAiTmV0d29ya0NhY2hlRmlsZVN5c3RlbS5oIgpAQCAtMTEzNyw3ICsxMTM4LDcgQEAgdm9p ZCBTdG9yYWdlOjpkZWxldGVPbGRWZXJzaW9ucygpCiAgICAgICAgICAgICBpZiAoZGlyZWN0b3J5 VmVyc2lvbiA+PSB2ZXJzaW9uKQogICAgICAgICAgICAgICAgIHJldHVybjsKICNpZiBQTEFURk9S TShNQUMpCi0gICAgICAgICAgICBpZiAoZGlyZWN0b3J5VmVyc2lvbiA9PSBsYXN0U3RhYmxlVmVy c2lvbikKKyAgICAgICAgICAgIGlmICghQ2hpbGRQcm9jZXNzOjppc1N5c3RlbVdlYktpdCgpICYm IGRpcmVjdG9yeVZlcnNpb24gPT0gbGFzdFN0YWJsZVZlcnNpb24pCiAgICAgICAgICAgICAgICAg cmV0dXJuOwogI2VuZGlmCiAKSW5kZXg6IFNvdXJjZS9XZWJLaXQvU2hhcmVkL0NoaWxkUHJvY2Vz cy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQvU2hhcmVkL0NoaWxkUHJvY2Vzcy5oCShy ZXZpc2lvbiAyMzUxNzApCisrKyBTb3VyY2UvV2ViS2l0L1NoYXJlZC9DaGlsZFByb2Nlc3MuaAko d29ya2luZyBjb3B5KQpAQCAtNzcsNiArNzcsMTAgQEAgcHVibGljOgogCiAgICAgSVBDOjpNZXNz YWdlUmVjZWl2ZXJNYXAmIG1lc3NhZ2VSZWNlaXZlck1hcCgpIHsgcmV0dXJuIG1fbWVzc2FnZVJl Y2VpdmVyTWFwOyB9CiAKKyNpZiBQTEFURk9STShNQUMpCisgICAgc3RhdGljIGJvb2wgaXNTeXN0 ZW1XZWJLaXQoKTsKKyNlbmRpZgorCiBwcm90ZWN0ZWQ6CiAgICAgZXhwbGljaXQgQ2hpbGRQcm9j ZXNzKCk7CiAgICAgdmlydHVhbCB+Q2hpbGRQcm9jZXNzKCk7CkluZGV4OiBTb3VyY2UvV2ViS2l0 L1NoYXJlZC9tYWMvQ2hpbGRQcm9jZXNzTWFjLm1tCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQvU2hhcmVkL21hYy9DaGlsZFByb2Nlc3NNYWMubW0JKHJldmlzaW9uIDIzNTE3MCkKKysrIFNv dXJjZS9XZWJLaXQvU2hhcmVkL21hYy9DaGlsZFByb2Nlc3NNYWMubW0JKHdvcmtpbmcgY29weSkK QEAgLTczMiw2ICs3MzIsMTYgQEAgdm9pZCBDaGlsZFByb2Nlc3M6OnNldFFPUyhpbnQgbGF0ZW5j eVFPUwogICAgIHRhc2tfcG9saWN5X3NldChtYWNoX3Rhc2tfc2VsZigpLCBUQVNLX09WRVJSSURF X1FPU19QT0xJQ1ksICh0YXNrX3BvbGljeV90KSZxb3NpbmZvLCBUQVNLX1FPU19QT0xJQ1lfQ09V TlQpOwogfQogCisjaWYgUExBVEZPUk0oTUFDKQorYm9vbCBDaGlsZFByb2Nlc3M6OmlzU3lzdGVt V2ViS2l0KCkKK3sKKyAgICBzdGF0aWMgYm9vbCBpc1N5c3RlbVdlYktpdCA9IFtdIHsKKyAgICAg ICAgcmV0dXJuIFtbd2ViS2l0MkJ1bmRsZSgpIGJ1bmRsZVBhdGhdIGhhc1ByZWZpeDpAIi9TeXN0 ZW0vIl07CisgICAgfSgpOworICAgIHJldHVybiBpc1N5c3RlbVdlYktpdDsKK30KKyNlbmRpZgor CiB9IC8vIG5hbWVzcGFjZSBXZWJLaXQKIAogI2VuZGlmCg==