1188550 - Don't directly use MOZ_APP_DISPLAYNAME in User-Agent headers

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Core, defect)

Description

[Richard Newman [:rnewman]]

If we use non-ASCII names, we tend to break servers.

Comment 1

[Nick Alexander :nalexander [he/him]]

Heh, what a world we live in.

rnewman suggested a MOZ_APP_PROGRAMMATIC_NAME; I wonder if we should be more focused and go for MOZ_ANDROID_BACKGROUND_SERVICE_USER_AGENT_NAME -- or something /slightly/ shorter :)

Comment 2

[Richard Newman [:rnewman]]

In my hack I also changed the Sync defaultClientName(Context) method, but other than that it's just user agents.

I'd also go for something like MOZ_APP_ASCII_DISPLAYNAME, which is exactly what is says on the tin :)

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1188550 - Part 1: Add MOZ_APP_ASCII_DISPLAYNAME. r?glandium

Bug 1188550 - Part 1: Add MOZ_APP_ASCII_DISPLAYNAME. r?glandium

Mozilla has a set of web services that crash when presented with a
non-ASCII User-Agent string.  Fennec wants to expose the
MOZ_APP_DISPLAYNAME to such services, but there's no expectation that
the display name is ASCII.  This patch adds a new ASCII display name
that defaults to the regular display name, exposes it to branding
(important, since the non-ASCII display names will be set by
partner-specific branding), and tries to prevent mis-configuration at
configure-time.

Checking for ASCII using just configure.in and the shell proved beyond
me, so I did it in Python.  (There is precedent for using Python in
configure.in.)

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1188550 - Part 2: Use MOZ_APP_ASCII_DISPLAYNAME in background service User-Agent strings. r?rnewman

Bug 1188550 - Part 2: Use MOZ_APP_ASCII_DISPLAYNAME in background service User-Agent strings. r?rnewman

I elected to keep the use of MOZ_APP_DISPLAYNAME in the Sync client
name.  I see no reason that non-ASCII characters should impact us
there.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8643150 [details]
MozReview Request: Bug 1188550 - Part 1: Add MOZ_APP_ASCII_DISPLAYNAME. r?glandium

https://reviewboard.mozilla.org/r/14623/#review13489

Just use MOZ_APP_UA_NAME

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8643151 [details]
MozReview Request: Bug 1188550 - Part 2: Use MOZ_APP_ASCII_DISPLAYNAME in background service User-Agent strings. r?rnewman

https://reviewboard.mozilla.org/r/14625/#review13687

Ship It!

Comment 7

[Richard Newman [:rnewman]]

Nick, did this land?

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Nick, did this land?

No, and I'm not going to get to these changes anytime soon.  This is a good first bug or good next bug.

What needs to happen: take the existing patches, throw away the first, and update the second to use MOZ_APP_UA_NAME instead of MOZ_APP_ASCII_DISPLAYNAME.  You'll need to "expose" MOZ_APP_UA_NAME in mobile/android/base/moz.build.  Then make sure everything builds correctly and flag me for review!

Comment 9

[Vivek Balakrishnan[:vivek]]

@Kalpesh,

Comment 10

[Vivek Balakrishnan[:vivek]]

@Kalpesh,

This one is very easy. what needs to be done is https://bugzilla.mozilla.org/show_bug.cgi?id=1188550#c8

Let me know if you are interested.

Comment 11

[Kalpesh Krishna [:martianwars]]

Yeah @vivek, I would definitely be interested. I didn't quite understand what Nick meant by throwing away the first patch and updating the second.

Comment 12

[Vivek Balakrishnan[:vivek]]

@Kalpesh,

First of all, sorry for the delay.

If I understand correctly, we don't need to add a new flag MOZ_APP_ASCII_DISPLAYNAME which means we don't need patch 1.
Just replace MOZ_APP_ASCII_DISPLAYNAME with MOZ_APP_UA_NAME  flag in patch 2 and then expose MOZ_APP_UA_NAME in mobile/android/base/moz.build 
Let me know if this was not clear.

Comment 13

[Kalpesh Krishna [:martianwars]]

Attachment: displayascii.patch

Hey Vivek I hope it is okay. Could you teach me the correct way of editing patches? I ended up making my own patch here

Comment 14

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8683809 [details] [diff] [review]
displayascii.patch

Review of attachment 8683809 [details] [diff] [review]:
-----------------------------------------------------------------

Kalpesh,

Ideally, I would use the pull or import commands under information section in reviewboard to download the patch(es).
Or if you are into mq based development, you can use "Download diff" button on the top panel to download and then import into patch queue.

The patch needs one small change without which it cannot be built.
Can you please update the patch addressing my following comment.

::: mobile/android/base/background/healthreport/HealthReportConstants.java
@@ +10,5 @@
>  public class HealthReportConstants {
>    public static final String HEALTH_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".health";
>    public static final String GLOBAL_LOG_TAG = "GeckoHealth";
>  
> +  public static final String USER_AGENT = "Firefox-Android-HealthReport/" + AppConstants.MOZ_APP_VERSION + " (" + AppConstants.MOZ_APP_UA_NAME + ")";

This will fail the build. You need to add MOZ_APP_UA_NAME to AppConstants.in

Comment 15

[Kalpesh Krishna [:martianwars]]

Attachment: displayascii.patch

I hope it is okay now :)

Comment 16

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8683869 [details] [diff] [review]
displayascii.patch

Review of attachment 8683869 [details] [diff] [review]:
-----------------------------------------------------------------

Kalpesh,

Thanks for the quick turn around.
However, the patch fails to build :(
can you please address my comment and then try a clobber build.

::: mobile/android/base/AppConstants.java.in
@@ +102,5 @@
>      public static final String MOZ_APP_NAME = "@MOZ_APP_NAME@";
>      public static final String MOZ_APP_VENDOR = "@MOZ_APP_VENDOR@";
>      public static final String MOZ_APP_VERSION = "@MOZ_APP_VERSION@";
>      public static final String MOZ_APP_DISPLAYNAME = "@MOZ_APP_DISPLAYNAME@";
> +    public static final String MOZ_APP_UA_NAME = "@MOZ_APP_UA_NAME@";    

Remove the " around MOZ_APP_UA_NAME, seems like MOZ_APP_UA_NAME is already quoted. 
please add comment like we do for MOZILLA_VERSION so that it is clear. 
nit:  trailing space

Comment 17

[Kalpesh Krishna [:martianwars]]

Attachment: displayascii.patch

What is the command to try a build on the try server? I have access but I'm not using it :(

Comment 18

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8684082 [details] [diff] [review]
displayascii.patch

Review of attachment 8684082 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM except for trailing space nit. 

Checkout the Workflow section in https://wiki.mozilla.org/Build:TryChooser for deploying build to try server.
The try: string syntax can be generated online from http://trychooser.pub.build.mozilla.org

I usually choose Build type: Both, Platform: android api 9-10 constrained, android api 11+, android-x86 (Android 4.2) and then choose all Android-only unit tests except for autophones

Please feel free to NI me if you need help.

::: mobile/android/base/AppConstants.java.in
@@ +102,5 @@
>      public static final String MOZ_APP_NAME = "@MOZ_APP_NAME@";
>      public static final String MOZ_APP_VENDOR = "@MOZ_APP_VENDOR@";
>      public static final String MOZ_APP_VERSION = "@MOZ_APP_VERSION@";
>      public static final String MOZ_APP_DISPLAYNAME = "@MOZ_APP_DISPLAYNAME@";
> +    public static final String MOZ_APP_UA_NAME = @MOZ_APP_UA_NAME@;  

nit: Please remove the trailing white space

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8684082 [details] [diff] [review]
displayascii.patch

Review of attachment 8684082 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Please add a line olf explanation to the commit message like:

Bug 1188550 - Don't directly use MOZ_APP_DISPLAYNAME in User-Agent headers. r=vivek

It is possible for the display name to contain Unicode characters, which not all Mozilla service endpoints handle gracefully.  This avoids that issue.

::: mobile/android/base/AppConstants.java.in
@@ +102,5 @@
>      public static final String MOZ_APP_NAME = "@MOZ_APP_NAME@";
>      public static final String MOZ_APP_VENDOR = "@MOZ_APP_VENDOR@";
>      public static final String MOZ_APP_VERSION = "@MOZ_APP_VERSION@";
>      public static final String MOZ_APP_DISPLAYNAME = "@MOZ_APP_DISPLAYNAME@";
> +    public static final String MOZ_APP_UA_NAME = @MOZ_APP_UA_NAME@;  

Add a note about quotation, like below.

Comment 20

[Nick Alexander :nalexander [he/him]]

bobm: rfkelly: this ticket will change the User Agent that Sync and other background services on Fennec show up with.  They'll look like

Firefox-Android-FxAccounts/45.0a1 (Firefox)

where before they looked like

Firefox-Android-FxAccounts/45.0a1 (Firefox|Firefox Beta|Fennec Aurora|Fennec)

This loses the release channel, but I don't think we care about that.  Can you tell me if we need to hit the breaks for this for ops or metrics reasons?

Comment 21

[Kalpesh Krishna [:martianwars]]

Attachment: displayascii.patch with updated commit message

I hope it's okay

Comment 22

[Ryan Kelly [:rfkelly]]

*shrug*

We still get the version number so I don't think we're losing too much.  I don't recall an instance where I've wanted to filter on the release channel rather than the version, but :bobm might.

Comment 23

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8684379 [details] [diff] [review]
displayascii.patch with updated commit message

Review of attachment 8684379 [details] [diff] [review]:
-----------------------------------------------------------------

I'll move the comment up a line (that's our convention) when I land.

Comment 24

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/86a15337b0cb5c0e22c9ad6d6345f91126f44986
Bug 1188550 - Don't directly use MOZ_APP_DISPLAYNAME in User-Agent headers. r=vivek,nalexander.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86a15337b0cb