Summary: | [WPE][GTK] Allow distributors to brand user agent | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, aperez, bugs-noreply, cgarcia, clopez, ews-watchlist, gyuyoung.kim, jbicha, mcatanzaro, olivier.blin, ryuan.choi, sergio, tpopela | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=204399 | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 142074 | ||||||||||||||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2016-09-27 08:19:43 PDT
Also, fix related issues in this function. (In reply to comment #1) > Also, fix related issues in this function. Just kidding Created attachment 289961 [details]
Patch
Comment on attachment 289961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289961&action=review > Source/WebCore/platform/gtk/UserAgentGtk.cpp:132 > +#ifdef USER_AGENT_GTK_DISTRIBUTOR_NAME Usually we go with defined(MACRO) && MACRO -- unless an empty string is an option(?). (In reply to comment #4) > Comment on attachment 289961 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289961&action=review > > > Source/WebCore/platform/gtk/UserAgentGtk.cpp:132 > > +#ifdef USER_AGENT_GTK_DISTRIBUTOR_NAME > > Usually we go with defined(MACRO) && MACRO -- unless an empty string is an > option(?). Usually our macros are defined to 0 or 1 where a boolean comparison is more natural. This one is either defined to a string if it's to be used, or not defined at all. (In reply to comment #5) > Usually our macros are defined to 0 or 1 where a boolean comparison is more > natural. This one is either defined to a string if it's to be used, or not > defined at all. To be clear, the reason we usually need both checks is that we don't want to enable the feature if the preprocessor variable is defined to 0. That's not a concern in this case. If you define USER_AGENT_GTK_DISTRIBUTOR_NAME to an empty string, that's pretty silly, but harmless (you'll get an extra semicolon in the UA). Ping reviewers Created attachment 291809 [details]
Patch
Created attachment 293018 [details]
Patch
(In reply to comment #7) > Ping reviewers What can I do to get this approved? What if I move the distributor to the end of the UA, would that be accepted? Distributor branding is not optional for us and I hate carrying this downstream forever. (In reply to comment #11) > What can I do to get this approved? What if I move the distributor to the > end of the UA, would that be accepted? Let's move it to the end, and also put WebKitGTK+ there. And instead of using a preprocessor conditional, let's read it from the NAME and VERSION_ID fields in /etc/os-release. Should look like e.g.: Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/603.1 (KHTML, like Gecko) Version/10.0 Safari/603.1 Fedora/25 WebKitGTK+/603.1 Epiphany/3.22.5 (In reply to comment #12) > Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/603.1 (KHTML, like > Gecko) Version/10.0 Safari/603.1 Fedora/25 WebKitGTK+/603.1 Epiphany/3.22.5 I'm really bad at copy-pasting. The distributor should only be there once: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/603.1 (KHTML, like Gecko) Version/10.0 Safari/603.1 Fedora/25 WebKitGTK+/603.1 Epiphany/3.22.5 Also looks like we need to bump Safari version from Version/10.0 to Version/11.0. Comment on attachment 293018 [details]
Patch
The /etc/os-release plan was a bad idea. It's not good enough because there is no field that includes a name that is guaranteed to be suitable for use in the user agent. E.g. the name could be "Debian GNU/Linux" which is not suitable.
Comment on attachment 293018 [details]
Patch
In the end, I've come to believe Carlos Garcia was right all along and we shouldn't do this.
(In reply to Michael Catanzaro from comment #15) > Comment on attachment 293018 [details] > Patch > > In the end, I've come to believe Carlos Garcia was right all along and we > shouldn't do this. Um, I forgot I ever said this. We're still using this patch in Fedora. I've removed Epiphany's distro branding feature, and advised Ubuntu to apply this patch instead. Ubuntu and Fedora have been applying this patch for years... any hope of reconsideration? Carlos, if I change this to be a CMake build option, could we get it in? I'd really like to reduce downstream patches as far as possible. This is being used by both Ubuntu and Fedora, and combined that accounts for probably 80% of our users. (In reply to Michael Catanzaro from comment #18) > Carlos, if I change this to be a CMake build option, could we get it in? I'd > really like to reduce downstream patches as far as possible. This is being > used by both Ubuntu and Fedora, and combined that accounts for probably 80% > of our users. Let's do that Created attachment 392784 [details]
Patch
Created attachment 392785 [details]
Patch
Comment on attachment 392785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392785&action=review > Source/WebCore/ChangeLog:3 > + [GTK] Allow distributors to brand user agent The patch (and bug title) should include “[WPE]” :) > Source/WebCore/platform/glib/UserAgentGLib.cpp:93 > + uaString.appendLiteral(DISTRIBUTOR_BRANDING "; "); Let's imagine for a moment that I don't trust what people will pass in the build option as a valid User-Agent header value: if somebody puts something here which does not conform to RFC 7231 (section 5.5.3, more specifically) they will have web sites fails in mysterious ways when the server does strict validation of the header. I have seen some CDNs (Cloudfare, for example) returning HTTP 400 (Bad Request) if the UA string has invalid contents. And trust me that people will knock their heads against the wall for days without end before realizing that they have added garbage to the UA string. Therefore, that when custom branding is added we need to unconditionally call isValidUserAgentHeaderValue()—not just an assertion—, log an error message very noticeably, and abort() the process... Unless you can come up with some way of checking the passed value when running CMake. > Source/cmake/OptionsGTK.cmake:20 > +set(DISTRIBUTOR_BRANDING "" CACHE STRING "Branding to add to user agent string") The name “DISTRIBUTOR_BRANDING” kind of implies that this is some general branding, to be used only by distributors (what if I am not one and still want to use the option?) and not just for the user agent. The description of the option clarifies it, yet I think a better name could be something like “USER_AGENT_BRANDING”. (In reply to Adrian Perez from comment #22) > Therefore, that when custom branding is added we need to unconditionally > call isValidUserAgentHeaderValue()—not just an assertion—, log an error > message very noticeably, and abort() the process... So... RELEASE_ASSERT()? > Unless you can come up > with some way of checking the passed value when running CMake. Certainly not. > The name “DISTRIBUTOR_BRANDING” kind of implies that this is some general > branding, to be used only by distributors (what if I am not one and still > want to use the option?) and not just for the user agent. The description > of the option clarifies it, yet I think a better name could be something > like “USER_AGENT_BRANDING”. OK. (In reply to Michael Catanzaro from comment #23) > So... RELEASE_ASSERT()? Adrian, is this OK? There will be more overhead on every request... tbh, I don't think it's worth it, but your call. Assuming RELEASE_ASSERT() is OK... and hoping a runtime check doesn't regress perf... I still don't think it's useful to perform this check at runtime tbh, as configuring invalid branding is pretty unlikely.... Created attachment 393766 [details]
Patch
(In reply to Michael Catanzaro from comment #25) > Assuming RELEASE_ASSERT() is OK... and hoping a runtime check doesn't > regress perf... If you are able to find any benchmark or test where this extra RELEASE_ASSERT can cause any noticeable performance regression I would be very very surprised, please let me know it. > I still don't think it's useful to perform this check at > runtime tbh, as configuring invalid branding is pretty unlikely.... I think its useful for the reasons Adrian commented, specially if you tell the user/distributor what is happening. So instead of using RELEASE_ASSERT and simply crash, I think we should use RELEASE_ASSERT_WITH_MESSAGE() and log some meaningful message before crashing. (In reply to Carlos Alberto Lopez Perez from comment #27) > (In reply to Michael Catanzaro from comment #25) > > Assuming RELEASE_ASSERT() is OK... and hoping a runtime check doesn't > > regress perf... > > If you are able to find any benchmark or test where this extra > RELEASE_ASSERT can cause any noticeable performance regression I would be > very very surprised, please let me know it. > > > I still don't think it's useful to perform this check at > > runtime tbh, as configuring invalid branding is pretty unlikely.... > > I think its useful for the reasons Adrian commented, specially if you tell > the user/distributor what is happening. > > So instead of using RELEASE_ASSERT and simply crash, I think we should use > RELEASE_ASSERT_WITH_MESSAGE() and log some meaningful message before > crashing. Mmm.. not sure if RELEASE_ASSERT_WITH_MESSAGE() does what i think it should do on GTK.. i don't see it logging a message on a quick test :\ It should boil down to the vfprintf(stderr, format, args) call at the bottom of vprintf_stderr_common() in Assertions.cpp. Is it desirable to brand UA of whole engine, or of particular applications? In Qt port we support putting application name (or other string, provided by application) into UA (In reply to Carlos Alberto Lopez Perez from comment #28) > Mmm.. not sure if RELEASE_ASSERT_WITH_MESSAGE() does what i think it should > do on GTK.. i don't see it logging a message on a quick test :\ Yeah, it indeed seems to be busted. Anyway, that should be fixed separately. (In reply to Konstantin Tokarev from comment #30) > Is it desirable to brand UA of whole engine, or of particular applications? > In Qt port we support putting application name (or other string, provided by > application) into UA Yes, our two largest distributors will continue doing this in downstream patches if we don't allow it upstream. I don't necessarily recommend branding by application name anyway. I've started to experiment by removing Epiphany entirely from its user agent string. This makes a big difference on browserdetect.org at least, because Safari version is now detected as 13 instead of 605.1. Probably we'll stick with this approach. Created attachment 393772 [details]
Patch
Comment on attachment 393772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393772&action=review > Source/WebCore/platform/glib/UserAgentGLib.cpp:147 > + RELEASE_ASSERT(isValidUserAgentHeaderValue(userAgent), "%s is not a valid user agent header", userAgent.utf8().data()); Shouldn't this be RELEASE_ASSERT_WITH_MESSAGE? > Source/WebCore/platform/glib/UserAgentGLib.cpp:159 > + RELEASE_ASSERT(isValidUserAgentHeaderValue(userAgent), "%s is not a valid user agent header", userAgent.utf8().data()); ditto (In reply to Carlos Alberto Lopez Perez from comment #33) > Shouldn't this be RELEASE_ASSERT_WITH_MESSAGE? Yeah, calling the right function would certainly help.... It doesn't work anyway though. I found: bug #204399. Created attachment 393776 [details]
Patch
Ping Adrian! Comment on attachment 393776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393776&action=review Patch looks good, thanks a lot for updating it! > Source/WebCore/platform/glib/UserAgentGLib.cpp:147 > + RELEASE_ASSERT_WITH_MESSAGE(isValidUserAgentHeaderValue(userAgent), "%s is not a valid user agent header", userAgent.utf8().data()); If there are concerns about the assertion being made on each network request, an option could be to have a boolean flag to do it only the first time: static bool uaBrandCheckDone = false; if (!uaBrandCheckDone) { RELEASE_ASSERT_WITH_MESSAGE(...); uaBrandCheckDone = true; } Created attachment 394301 [details]
Patch for landing
Committed r258877: <https://trac.webkit.org/changeset/258877> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394301 [details]. Reopening to attach new patch. Created attachment 394311 [details]
Patch for landing
Committed r258883: <https://trac.webkit.org/changeset/258883> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394311 [details]. |