Closed
Bug 1284921
Opened 8 years ago
Closed 8 years ago
Android N in user agent is breaking some websites
Categories
(Core Graveyard :: Widget: Android, defect)
Core Graveyard
Widget: Android
Tracking
(firefox48+ verified, firefox49+ fixed, fennec50+, firefox50 verified)
RESOLVED
FIXED
mozilla50
People
(Reporter: mkaply, Assigned: mkaply)
References
Details
Attachments
(2 files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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;
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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)
Updated•8 years ago
|
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Component: General → Widget: Android
Product: Firefox for Android → Core
tracking-fennec: ? → 50+
Comment 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
> 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.
Assignee | ||
Comment 7•8 years ago
|
||
// 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
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9efa66f1c6a15449be7a3c37ade8e104ce2afef8 Bug 1284921- Hardcode Android version for non-numeric releases. r=nfroyd
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9efa66f1c6a1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 11•8 years ago
|
||
[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.
tracking-firefox48:
--- → ?
tracking-firefox49:
--- → ?
Comment 13•8 years ago
|
||
Mike, any reason why you don't ask for an uplift request?
Flags: needinfo?(mozilla)
Updated•8 years ago
|
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Comment 16•8 years ago
|
||
Hi Mike, Can you also create uplift request for 49 aurora?
Flags: needinfo?(mozilla)
Assignee | ||
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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+
Comment 20•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/5de1c40fd9bf
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•