Bug 188843 – NetworkCache::Storage::lastStableVersion should be a developer-only feature

• Component: Page Loading
• Status: RESOLVED FIXED

[Antti Koivisto]

System WebKit should always delete old cache versions.

[Antti Koivisto]

<rdar://problem/43574100>

[Antti Koivisto]

Created attachment 347792 [details]
patch

[Michael Catanzaro]

Comment on attachment 347792 [details]
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).

[Michael Catanzaro]

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

[Daniel Bates]

Comment on attachment 347792 [details]
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.

[Geoffrey Garen]

I think it's OK to treat installing roots as an "upgrade" event that clears old caches just like a real upgrade would.

[Geoffrey Garen]

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

[Geoffrey Garen]

Comment on attachment 347792 [details]
patch

r=me

[Michael Catanzaro]

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.

[Antti Koivisto]

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

[WebKit Commit Bot]

Comment on attachment 347792 [details]
patch

Clearing flags on attachment: 347792

Committed r235220: <https://trac.webkit.org/changeset/235220>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.