Limit user agent versioning to an upper bound

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

Stop reporting more recent user agent version strings, freezing the version at a specific moment in time. We should do this for a number of reasons:
(1) User Agent sniffing is a terrible way to determine whether a browser supports certain features.
(2) Bad User Agent sniffing code on the web create compatibility problems every time we update the versions.
(3) Overly-specific version information provides useful fingerprinting data while providing almost no user benefit.

[Brent Fulgham]

<rdar://problem/34550617>

[Brent Fulgham]

Created attachment 328364 [details]
Patch

[Brent Fulgham]

Created attachment 328366 [details]
Patch

Comment on attachment 328366 [details]
Patch

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

> Source/WebCore/page/cocoa/UserAgent.mm:55
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101304

Looking at the WebKit builds that Apple has shipped so far, they seem to always build with a __MAC_OS_X_VERSION_MIN_REQUIRED ending with “00” (both in system updates and in Safari Technology Preview), so this condition is practically equivalent to __MAC_OS_X_VERSION_MIN_REQUIRED < 101300. So this will make all builds targeting some version of High Sierra or later to use 10_13_4, which seems fine, but is better expressed by explicitly writing 101300. On the other hand, when targeting Sierra and El Capitan, we already know their currently shipping versions, so it seems unnecessary to refer to systemMarketingVersion().

A way to write this that is both explicit and will lend itself to gradual simplification as WebKit drops support for older OSes and that follows our convention of putting the “present and future” case ahead of the “legacy” case would be something like
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
    return "10_13_4";
#elif __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200
    return "10_12_6";
#else
    return "10_11_6";
#endif

[Brent Fulgham]

Comment on attachment 328366 [details]
Patch

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

>> Source/WebCore/page/cocoa/UserAgent.mm:55
>> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101304
> 
> Looking at the WebKit builds that Apple has shipped so far, they seem to always build with a __MAC_OS_X_VERSION_MIN_REQUIRED ending with “00” (both in system updates and in Safari Technology Preview), so this condition is practically equivalent to __MAC_OS_X_VERSION_MIN_REQUIRED < 101300. So this will make all builds targeting some version of High Sierra or later to use 10_13_4, which seems fine, but is better expressed by explicitly writing 101300. On the other hand, when targeting Sierra and El Capitan, we already know their currently shipping versions, so it seems unnecessary to refer to systemMarketingVersion().
> 
> A way to write this that is both explicit and will lend itself to gradual simplification as WebKit drops support for older OSes and that follows our convention of putting the “present and future” case ahead of the “legacy” case would be something like
> #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
>     return "10_13_4";
> #elif __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200
>     return "10_12_6";
> #else
>     return "10_11_6";
> #endif

I like this much better! :-)

[Brent Fulgham]

Created attachment 328485 [details]
Patch w/ Dan's improvements

[Joseph Pecoraro]

Comment on attachment 328485 [details]
Patch w/ Dan's improvements

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

> Source/WebCore/page/cocoa/UserAgent.mm:37
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300

I think this means that the UserAgent numbers are fixed from the system that built WebKit and not from the system that WebKit is running on? Is that what we expect? I guess it is irrelevant as time goes by though, so the simplicity is nice.

> Source/WebCore/page/cocoa/UserAgent.mm:38
> +    return "10_13_4";

Nit: ASCIILiteral() can save a copy. (For each of these char* => WTF::String conversions from a literal):
https://trac.webkit.org/wiki/EfficientStrings

> Source/WebCore/page/cocoa/UserAgent.mm:51
> +    return "605.1.15";

Nit: ASCIILiteral() can save a copy.

> Source/WebKitLegacy/mac/ChangeLog:11
> +        creatin the User Agent string.

Typo: creatin

> LayoutTests/fast/dom/navigator-userAgent-frozen.html:64
> +    if (window.testRunner)
> +        testRunner.dumpAsText();
> +
> +    testVersion(navigator.userAgent);

Style: Weird indentation.

(In reply to Joseph Pecoraro from comment #7)
> Comment on attachment 328485 [details]
> Patch w/ Dan's improvements
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328485&action=review
> 
> > Source/WebCore/page/cocoa/UserAgent.mm:37
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
> 
> I think this means that the UserAgent numbers are fixed from the system that
> built WebKit and not from the system that WebKit is running on?

__MAC_OS_X_VERSION_MIN_REQUIRED is based on the deployment target. WebKit built for one major macOS release can’t run on earlier releases and isn’t supported on later releases, so there is nothing to query at runtime that isn’t know at build time.

[Brent Fulgham]

Comment on attachment 328485 [details]
Patch w/ Dan's improvements

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

>> Source/WebCore/page/cocoa/UserAgent.mm:38
>> +    return "10_13_4";
> 
> Nit: ASCIILiteral() can save a copy. (For each of these char* => WTF::String conversions from a literal):
> https://trac.webkit.org/wiki/EfficientStrings

Will do!

>> Source/WebCore/page/cocoa/UserAgent.mm:51
>> +    return "605.1.15";
> 
> Nit: ASCIILiteral() can save a copy.

Will do.

>> Source/WebKitLegacy/mac/ChangeLog:11
>> +        creatin the User Agent string.
> 
> Typo: creatin

Oops!

>> LayoutTests/fast/dom/navigator-userAgent-frozen.html:64
>> +    testVersion(navigator.userAgent);
> 
> Style: Weird indentation.

Whoops! Fixed.

[Brent Fulgham]

Created attachment 328500 [details]
Patch

[Joseph Pecoraro]

> > > Source/WebCore/page/cocoa/UserAgent.mm:37
> > > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
> > 
> > I think this means that the UserAgent numbers are fixed from the system that
> > built WebKit and not from the system that WebKit is running on?
> 
> __MAC_OS_X_VERSION_MIN_REQUIRED is based on the deployment target. WebKit
> built for one major macOS release can’t run on earlier releases and isn’t
> supported on later releases, so there is nothing to query at runtime that
> isn’t know at build time.

I see. Thanks for the explanation!

[Joseph Pecoraro]

Comment on attachment 328500 [details]
Patch

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

r=me, but since neither of us are a WebKit Owner I suspect you'll need a nod from one.

> LayoutTests/fast/dom/navigator-userAgent-frozen.html:34
> +    var versionComponents = version.split('.');
> +    if (versionComponents.length == 0) {

I don't think String.prototype.split can ever return an empty array, it would always return at least 1 value (the original string). But you could compare `length <= 1` and I think that would be fine for this test.

[Brent Fulgham]

Created attachment 328533 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 328533 [details]
Patch for landing

Clearing flags on attachment: 328533

Committed r225562: <https://trac.webkit.org/changeset/225562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

Note that OS marketing version was re-added to UserAgent in Bug 182629.