Bug 180365

Summary: Limit user agent versioning to an upper bound
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, dbates, ggaren, joepeck, mitz, mjs, rniwa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 182629, 192809    
Attachments:
Description Flags
Patch
none
Patch
none
Patch w/ Dan's improvements
none
Patch
none
Patch for landing none

Brent Fulgham
Reported 2017-12-04 10:12:13 PST
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.
Attachments
Patch (16.84 KB, patch)
2017-12-04 11:12 PST, Brent Fulgham
no flags
Patch (16.87 KB, patch)
2017-12-04 11:35 PST, Brent Fulgham
no flags
Patch w/ Dan's improvements (16.38 KB, patch)
2017-12-05 12:45 PST, Brent Fulgham
no flags
Patch (16.43 KB, patch)
2017-12-05 14:24 PST, Brent Fulgham
no flags
Patch for landing (16.32 KB, patch)
2017-12-05 17:16 PST, Brent Fulgham
no flags
Brent Fulgham
Comment 1 2017-12-04 10:12:27 PST
Brent Fulgham
Comment 2 2017-12-04 11:12:28 PST
Brent Fulgham
Comment 3 2017-12-04 11:35:28 PST
mitz
Comment 4 2017-12-05 11:13:58 PST
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 5 2017-12-05 12:32:18 PST
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
Comment 6 2017-12-05 12:45:06 PST
Created attachment 328485 [details] Patch w/ Dan's improvements
Joseph Pecoraro
Comment 7 2017-12-05 13:59:01 PST
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.
mitz
Comment 8 2017-12-05 14:02:34 PST
(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 9 2017-12-05 14:23:26 PST
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
Comment 10 2017-12-05 14:24:40 PST
Joseph Pecoraro
Comment 11 2017-12-05 14:29:52 PST
> > > 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 12 2017-12-05 16:55:09 PST
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
Comment 13 2017-12-05 17:16:24 PST
Created attachment 328533 [details] Patch for landing
WebKit Commit Bot
Comment 14 2017-12-05 17:59:57 PST
Comment on attachment 328533 [details] Patch for landing Clearing flags on attachment: 328533 Committed r225562: <https://trac.webkit.org/changeset/225562>
WebKit Commit Bot
Comment 15 2017-12-05 17:59:59 PST
All reviewed patches have been landed. Closing bug.
Brent Fulgham
Comment 16 2018-04-27 08:37:59 PDT
Note that OS marketing version was re-added to UserAgent in Bug 182629.
Note You need to log in before you can comment on or make changes to this bug.