Closed Bug 1188550 Opened 5 years ago Closed 5 years ago

Don't directly use MOZ_APP_DISPLAYNAME in User-Agent headers

Categories

(Android Background Services Graveyard :: Core, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: rnewman, Assigned: martianwars, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(4 files, 2 obsolete files)

If we use non-ASCII names, we tend to break servers.
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 :)
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 :)
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.)
Attachment #8643150 - Flags: review?(mh+mozilla)
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.
Attachment #8643151 - Flags: review?(rnewman)
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
Attachment #8643150 - Flags: review?(mh+mozilla)
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!
Attachment #8643151 - Flags: review?(rnewman) → review+
Nick, did this land?
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
(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!
Assignee: nalexander → nobody
Mentor: nalexander
Status: ASSIGNED → NEW
Flags: needinfo?(nalexander)
Whiteboard: [lang=java][good first bug]
@Kalpesh,
Mentor: vivekb.balakrishnan
Flags: needinfo?(kalpeshk2011)
@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.
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.
Flags: needinfo?(kalpeshk2011) → needinfo?(vivekb.balakrishnan)
@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.
Flags: needinfo?(vivekb.balakrishnan)
Attached patch displayascii.patch (obsolete) — Splinter Review
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
Assignee: nobody → kalpeshk2011
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8683809 - Flags: review?(vivekb.balakrishnan)
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
Attachment #8683809 - Flags: review?(vivekb.balakrishnan) → feedback+
Flags: needinfo?(vivekb.balakrishnan)
Attached patch displayascii.patch (obsolete) — Splinter Review
I hope it is okay now :)
Attachment #8683809 - Attachment is obsolete: true
Attachment #8683869 - Flags: review?(vivekb.balakrishnan)
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
Attachment #8683869 - Flags: review?(vivekb.balakrishnan) → feedback+
What is the command to try a build on the try server? I have access but I'm not using it :(
Attachment #8683869 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8684082 - Flags: review?(vivekb.balakrishnan)
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
Attachment #8684082 - Flags: review?(vivekb.balakrishnan)
Attachment #8684082 - Flags: review?(nalexander)
Attachment #8684082 - Flags: review+
Flags: needinfo?(vivekb.balakrishnan)
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.
Attachment #8684082 - Flags: review?(nalexander) → review+
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?
Flags: needinfo?(rfkelly)
Flags: needinfo?(bobm)
I hope it's okay
Attachment #8684379 - Flags: review?(nalexander)
*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.
Flags: needinfo?(rfkelly)
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.
Attachment #8684379 - Flags: review?(nalexander) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/86a15337b0cb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: needinfo?(bobm)
You need to log in before you can comment on or make changes to this bug.