Bug 162611 – [WPE][GTK] Allow distributors to brand user agent

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

Also, fix related issues in this function.

[Michael Catanzaro]

(In reply to comment #1)
> Also, fix related issues in this function.

Just kidding

[Michael Catanzaro]

Created attachment 289961 [details]
Patch

[Zan Dobersek]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

Ping reviewers

[Michael Catanzaro]

Created attachment 291809 [details]
Patch

[Michael Catanzaro]

Created attachment 293018 [details]
Patch

[Michael Catanzaro]

(In reply to comment #7)
> Ping reviewers

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

Ubuntu and Fedora have been applying this patch for years... any hope of reconsideration?

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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

[Michael Catanzaro]

Created attachment 392784 [details]
Patch

[Michael Catanzaro]

Created attachment 392785 [details]
Patch

[Adrian Perez]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 393766 [details]
Patch

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

(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 :\

[Michael Catanzaro]

It should boil down to the vfprintf(stderr, format, args) call at the bottom of vprintf_stderr_common() in Assertions.cpp.

[Konstantin Tokarev]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 393772 [details]
Patch

[Carlos Alberto Lopez Perez]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

It doesn't work anyway though. I found: bug #204399.

[Michael Catanzaro]

Created attachment 393776 [details]
Patch

[Michael Catanzaro]

Ping Adrian!

[Adrian Perez]

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;
   }

[Michael Catanzaro]

Created attachment 394301 [details]
Patch for landing

[EWS]

Committed r258877: <https://trac.webkit.org/changeset/258877>

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

[Michael Catanzaro]

Reopening to attach new patch.

[Michael Catanzaro]

Created attachment 394311 [details]
Patch for landing

[EWS]

Committed r258883: <https://trac.webkit.org/changeset/258883>

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