1284921 - Android N in user agent is breaking some websites

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Mike Kaply [:mkaply]]

The Android N user agent for Firefox contains N as the version which appears to be breaking some websites.

Looking at Chrome, they use 6.0.99, not N.

We need to do the same.

Comment 1

[Mike Kaply [:mkaply]]

Chrome grabs the major and minor versions explicitly:

https://cs.chromium.org/chromium/src/content/common/user_agent.cc?sq=package:chromium&dr=C&rcl=1467803755&l=90

https://cs.chromium.org/chromium/src/content/common/user_agent.cc?sq=package:chromium&dr=C&rcl=1467803755&l=53

We must be querying something different.

Comment 2

[Mike Kaply [:mkaply]]

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#776

    rv = infoService->GetPropertyAsAString(
        NS_LITERAL_STRING("release_version"), androidVersion);

https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsSystemInfo.cpp#836

  if (mozilla::AndroidBridge::Bridge()->GetStaticStringField(
      "android/os/Build$VERSION", "RELEASE", str)) {
    aInfo->release_version() = str;

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Potential fix

This is basically the same concept as Chrome, only we don't need to parse out all three values. We just need to make sure that we got at least one number from the parse.

See:

https://cs.chromium.org/chromium/src/base/sys_info_android.cc?l=70

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8768592 [details] [diff] [review]
Potential fix

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

Over to froydn for nsSystemInfo (or a 302 -- karlt?).

::: mobile/android/base/java/org/mozilla/gecko/SysInfo.java
@@ +185,5 @@
>      /**
>       * @return the release version string, such as "4.1.2".
>       */
>      public static String getReleaseVersion() {
> +        Log.w(LOG_TAG, android.os.Build.VERSION.RELEASE);

I think you missed this.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8768592 [details] [diff] [review]
Potential fix

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

I have no comment on the SysInfo.java changes.  r=me with some comment changes noted below.

::: xpcom/base/nsSystemInfo.cpp
@@ +813,5 @@
>  }
>  
>  #ifdef MOZ_WIDGET_ANDROID
> +// Default version of Android to fall back to when actual version numbers
> +// cannot be acquired. Use the latest Android release with a higher bug fix

I know that this comment was cut-and-pasted from Chromium, but I think it'd be good to mention circumstances where this can happen--e.g. "some versions of Android return a non-numeric value for the version number".

@@ +815,5 @@
>  #ifdef MOZ_WIDGET_ANDROID
> +// Default version of Android to fall back to when actual version numbers
> +// cannot be acquired. Use the latest Android release with a higher bug fix
> +// version to avoid unnecessarily comparison errors with the latest release.
> +// This should be manually kept up to date on each Android release.

Does that mean that we have to constantly remember to update this each time we want to support a new Android release?  Is there any way to derive this from the environment?

Comment 6

[Mike Kaply [:mkaply]]

> Does that mean that we have to constantly remember to update this each time we want to support a new Android release?  Is there any way to derive this from the environment?

Sadly yes. There does not appear to be a way to get this from the environment (hence the reason Chrome hard codes it as well).

I'll update the comment.

Comment 7

[Mike Kaply [:mkaply]]

// Prerelease versions of Android use a letter instead of version numbers.
// Unfortunately this breaks websites due to the user agent.
// Chrome works around this by hardcoding an Android version when a
// numeric version can't be obtained. We're doing the same.
// This version will need to be updated whenever there is a new official
// Android release.
// See: https://cs.chromium.org/chromium/src/base/sys_info_android.cc?l=61

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Final patch

For posterity, this is the final patch.

I moved the version to a "const" after I did my testing and somehow ended up writing as if it were Javascript.

This patches used a #DEFINE for the default android version and updates the comment.

Comment 9

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/9efa66f1c6a15449be7a3c37ade8e104ce2afef8
Bug 1284921- Hardcode Android version for non-numeric releases. r=nfroyd

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9efa66f1c6a1

Comment 11

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: We have a partner who is prepping to release Firefox 48 Fennec possibly on Android N,

Having this fix in will allow them to test websites properly on prelease Android N with release Firefox 48.

Patch is low risk.

Comment 12

[Gerry Chang [:gchang]]

Per comment #11, track this on 48/49.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Mike, any reason why you don't ask for an uplift request?

Comment 14

[Mike Kaply [:mkaply]]

Comment on attachment 8770604 [details] [diff] [review]
Final patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Firefox won't function properly with Android N prerelease
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low. No change on current Android.
[String/UUID change made/needed]:

> Mike, any reason why you don't ask for an uplift request?

I thought you were supposed to ask for tracking before uplift.

Requesting uplift.

Honestly, I'm quite confused when it comes to all the various flags.

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8770604 [details] [diff] [review]
Final patch

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

This patch fixes android version number. Take it in 48 beta 9. This should be fixed in fennec 48 beta 9.

Comment 16

[Gerry Chang [:gchang]]

Hi Mike,
Can you also create uplift request for 49 aurora?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2b7160a603f1

Comment 18

[Mike Kaply [:mkaply]]

Comment on attachment 8770604 [details] [diff] [review]
Final patch

Correcting accidental release approval request to aurora

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8770604 [details] [diff] [review]
Final patch

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

Per comment #15, take it in aurora.

Comment 20

[u549602]

Verified as fixed on latest Nightly(50.0a1 2016-07-19) and Beta (48.0b9) builds on a Nexus 6P with android 7.0. this issue was verified by accessing whatsmyua.com 

Still waiting for Aurora uplift.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5de1c40fd9bf