Closed Bug 1284921 Opened 4 years ago Closed 4 years ago

Android N in user agent is breaking some websites

Categories

(Core :: Widget: Android, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 + verified
firefox49 + fixed
fennec 50+ ---
firefox50 --- verified

People

(Reporter: mkaply, Assigned: mkaply)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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;
Blocks: android-n
Severity: normal → major
Attached patch Potential fixSplinter Review
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
Assignee: nobody → mozilla
Attachment #8768592 - Flags: review?(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.
Attachment #8768592 - Flags: review?(rnewman) → review?(nfroyd)
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Component: General → Widget: Android
Product: Firefox for Android → Core
tracking-fennec: ? → 50+
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?
Attachment #8768592 - Flags: review?(nfroyd) → review+
> 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.
// 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
Attached patch Final patchSplinter Review
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.
https://hg.mozilla.org/integration/fx-team/rev/9efa66f1c6a15449be7a3c37ade8e104ce2afef8
Bug 1284921- Hardcode Android version for non-numeric releases. r=nfroyd
https://hg.mozilla.org/mozilla-central/rev/9efa66f1c6a1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
[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.
Per comment #11, track this on 48/49.
Mike, any reason why you don't ask for an uplift request?
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(mozilla)
Attachment #8770604 - Flags: approval-mozilla-release?
Attachment #8770604 - Flags: approval-mozilla-beta?
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.
Attachment #8770604 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Mike,
Can you also create uplift request for 49 aurora?
Flags: needinfo?(mozilla)
Comment on attachment 8770604 [details] [diff] [review]
Final patch

Correcting accidental release approval request to aurora
Flags: needinfo?(mozilla)
Attachment #8770604 - Flags: approval-mozilla-release? → approval-mozilla-aurora?
Comment on attachment 8770604 [details] [diff] [review]
Final patch

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

Per comment #15, take it in aurora.
Attachment #8770604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.