Bug 162611

Summary: [WPE][GTK] Allow distributors to brand user agent
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Michael Catanzaro 2016-09-27 08:19:43 PDT
Allow distributions to add branding to user agent, right after X11, if desired. This seems to be the safest place to put it to avoid breakage.
Comment 1 Michael Catanzaro 2016-09-27 08:22:56 PDT
Also, fix related issues in this function.
Comment 2 Michael Catanzaro 2016-09-27 08:51:41 PDT
(In reply to comment #1)
> Also, fix related issues in this function.

Just kidding
Comment 3 Michael Catanzaro 2016-09-27 09:55:42 PDT
Created attachment 289961 [details]
Patch
Comment 4 Zan Dobersek 2016-09-28 03:33:06 PDT
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(?).
Comment 5 Michael Catanzaro 2016-09-28 05:23:15 PDT
(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.
Comment 6 Michael Catanzaro 2016-09-29 03:16:47 PDT
(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).
Comment 7 Michael Catanzaro 2016-10-04 11:36:28 PDT
Ping reviewers
Comment 8 Michael Catanzaro 2016-10-17 04:41:01 PDT
Created attachment 291809 [details]
Patch
Comment 9 Michael Catanzaro 2016-10-27 07:21:14 PDT
Created attachment 293018 [details]
Patch
Comment 10 Michael Catanzaro 2016-12-26 15:07:54 PST
(In reply to comment #7)
> Ping reviewers
Comment 11 Michael Catanzaro 2016-12-28 15:13:15 PST
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.
Comment 12 Michael Catanzaro 2017-01-20 05:49:37 PST
(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
Comment 13 Michael Catanzaro 2017-01-20 05:51:40 PST
(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 14 Michael Catanzaro 2017-01-22 08:58:06 PST
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 15 Michael Catanzaro 2017-05-10 13:09:09 PDT
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.
Comment 16 Michael Catanzaro 2017-12-06 08:52:04 PST
(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.
Comment 17 Michael Catanzaro 2020-01-24 08:00:26 PST
Ubuntu and Fedora have been applying this patch for years... any hope of reconsideration?
Comment 18 Michael Catanzaro 2020-02-10 13:13:55 PST
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.
Comment 19 Carlos Garcia Campos 2020-02-11 01:02:13 PST
(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
Comment 20 Michael Catanzaro 2020-03-06 15:20:14 PST
Created attachment 392784 [details]
Patch
Comment 21 Michael Catanzaro 2020-03-06 15:20:41 PST
Created attachment 392785 [details]
Patch
Comment 22 Adrian Perez 2020-03-07 13:21:33 PST
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”.
Comment 23 Michael Catanzaro 2020-03-09 12:31:19 PDT
(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.
Comment 24 Michael Catanzaro 2020-03-11 19:32:43 PDT
(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.
Comment 25 Michael Catanzaro 2020-03-17 10:44:58 PDT
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....
Comment 26 Michael Catanzaro 2020-03-17 10:48:31 PDT
Created attachment 393766 [details]
Patch
Comment 27 Carlos Alberto Lopez Perez 2020-03-17 11:12:12 PDT
(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.
Comment 28 Carlos Alberto Lopez Perez 2020-03-17 11:24:46 PDT
(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 :\
Comment 29 Michael Catanzaro 2020-03-17 11:29:42 PDT
It should boil down to the vfprintf(stderr, format, args) call at the bottom of vprintf_stderr_common() in Assertions.cpp.
Comment 30 Konstantin Tokarev 2020-03-17 11:31:58 PDT
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
Comment 31 Michael Catanzaro 2020-03-17 11:53:04 PDT
(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.
Comment 32 Michael Catanzaro 2020-03-17 11:53:39 PDT
Created attachment 393772 [details]
Patch
Comment 33 Carlos Alberto Lopez Perez 2020-03-17 12:17:14 PDT
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
Comment 34 Michael Catanzaro 2020-03-17 12:29:23 PDT
(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....
Comment 35 Michael Catanzaro 2020-03-17 12:32:20 PDT
It doesn't work anyway though. I found: bug #204399.
Comment 36 Michael Catanzaro 2020-03-17 12:35:46 PDT
Created attachment 393776 [details]
Patch
Comment 37 Michael Catanzaro 2020-03-22 08:20:54 PDT
Ping Adrian!
Comment 38 Adrian Perez 2020-03-23 10:57:44 PDT
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;
   }
Comment 39 Michael Catanzaro 2020-03-23 14:07:14 PDT
Created attachment 394301 [details]
Patch for landing
Comment 40 EWS 2020-03-23 14:50:54 PDT
Committed r258877: <https://trac.webkit.org/changeset/258877>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394301 [details].
Comment 41 Michael Catanzaro 2020-03-23 15:19:40 PDT
Reopening to attach new patch.
Comment 42 Michael Catanzaro 2020-03-23 15:19:43 PDT
Created attachment 394311 [details]
Patch for landing
Comment 43 EWS 2020-03-23 15:59:38 PDT
Committed r258883: <https://trac.webkit.org/changeset/258883>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394311 [details].